Fix multipart File fields

[csymeonides-mf]

No description provided.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (main@67db1a1). Click here to learn what that means.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##             main      #372   +/-   ##
========================================
  Coverage        ?   100.00%           
========================================
  Files           ?        47           
  Lines           ?      1558           
  Branches        ?         0           
========================================
  Hits            ?      1558           
  Misses          ?         0           
  Partials        ?         0           

Impacted Files	Coverage Δ
openapi_python_client/parser/openapi.py	100.00% <100.00%> (ø)
...penapi_python_client/parser/properties/__init__.py	100.00% <100.00%> (ø)
..._python_client/parser/properties/model_property.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67db1a1...7cbeb7e. Read the comment docs.

[csymeonides-mf]

@dbanty This is the other PR I mentioned :)

[csymeonides-mf]

Btw I only had to move those golden record files so that I could import one of them for the e2e test (the directory was golden-record but import doesn't like hyphens so I changed it to golden_record) .

I didn't realise this but there might be another option if you prefer to keep the existing directory name.

[csymeonides-mf]

@dbanty Now rebased

[csymeonides-mf]

@dbanty Now rebased again

[dbanty]

Seems like the main reason for switching the names around was to import the generated code to do a more unit-test thing within the e2e test. The golden-record stuff is really intended to be a snapshot test, an easy way for me to see in reviews how the changes impact the generated code. This makes it easy to pick out accidental changes.

We probably want to have more specific tests of the generated code to make sure they do what's intended eventually, but I think we'd want to target those more (e.g. just generate one specific Jinja template with specific params and make sure it does what it's supposed to). I haven't decided on a strategy to achieve that yet.

So my recommendation here would be to drop out the unit test of the generated code and move the name back to golden-record so it's more obvious what changed.

[csymeonides-mf]

I guess in my mind it was an end-to-end test of the library, but no worries, I'll change it to a unit test.

[dbanty]

Yeah I should probably change the name. It was intended to be integration tests as well but those got really hard to maintain and didn't add much value so it became just snapshot tests. We probably need some middle ground targeted integration tests that are separate from the unit tests of the generator itself but not as complicated as the full-on end-to-end generation snapshot.

[csymeonides-mf]

I couldn't find a way to convert my "end-to-end" test to a unit test, but I suppose the fact that I've added those extra bits to openapi.json means we are asserting that the correct code is generated via the templates. Let me know if there's anything more I could be doing there.

[csymeonides-mf]

@dbanty Now rebased, could we get this into the next release?

[dbanty]

Yes, sorry! I keep starting to review this and getting pulled away 😅. I have a few questions that I'll try and put some comments on today so this can move forward.

[dbanty]

Great, so it seems like there are a few action items then, some of which may be handled in separate tasks:

• Make sure to not include UNSET in any type of body. Right now that means using to_dict() or similar for both application/x-www-form-urlencoded and form/multipart. This is already being done with a combination of this PR and Fix(template): form data model serialization #430 .
• Explicitly set the default content-type to text/plain and use the files input instead of the data input for multipart.
• Support the encoding attribute (maybe just for application/json for now?) so that we know to jsonify certain parameters if specificed. For now it sounds like defaulting any complex objects (arrays, data classes) to json is a sane default since there's no clear way to represent them in plain text.
• Avoid runtime type checking for the above unless necessary (e.g. Union). If we know the type and the encoding for that type we should avoid the overhead.

Some of those things may already be done by the time I comment this because some new commits just came in from @csymeonides-mf 😆 . Thanks for all the feedback and discussion around this!

[csymeonides-mf]

Ok, I don't think I've done all the things on your list, but hopefully enough for now!

[csymeonides-mf]

I've set this back to draft because when I tested it with a string field (not JSON) I realised that httpx doesn't handle them correctly - it converts them into files. Maybe I should be JSON-encoding strings as well? But that sounds wrong. Putting them in data as before will fix this, but again doesn't sound like the right solution.

@p1-ra if you have any ideas that would be much appreciated.

[csymeonides-mf]

@dbanty this also made me realise that the e2e test I had written earlier would have caught this problem... do you have any advice on how we can re-introduce that test in a way that fits your approach? As I said earlier I couldn't find a way to unit-test this, because it involves running the code which has been generated from the jinja templates.

[csymeonides-mf]

@dbanty All fixed now!

[dbanty]

This is great work, thank you! Just a couple suggestions then I think we're good to go.

As far as being able to run the e2e-generated code for better testing, I have two ideas:

1. Generate tests for the generated code. I have seen other generators include tests in the box before, so maybe that's something folks want anyway?
2. Use update instead of generate when regenerating golden-record and manually maintain a tests directory in golden-record which tests the generated code. If we wanted to we could even go so far as spawning a local server to test against (this project actually used to do that using FastAPI when it first started).

In either case, we could then just run pytest in that directory to run those tests. Thoughts on either of those paths? This would be in a different, future PR, I just agree that we need some more thorough tests on the generated code to be more confident in changes going forward.

[csymeonides-mf]

2. Use update instead of generate when regenerating golden-record and manually maintain a tests directory in golden-record which tests the generated code. If we wanted to we could even go so far as spawning a local server to test against (this project actually used to do that using FastAPI when it first started).

This sounds great to me, though maybe we still want a separate test that runs generate to make sure that's covered as well?

[dbanty]

Awesome! Thanks so much for putting all this work into this!