openapi-generator
openapi-generator copied to clipboard
OpenAPITools
fix(python,asyncio): multipart form data serialization
This PR tackles a few serialization issues in the Python generator templates: serialization of dictionaries and integers in multipart/form-data requests.
I generated a library=asyncio client using latest master and modules/openapi-generator/src/test/resources/3_0/form-multipart-binary-array.yaml. The /multipart-mixed call fails with TypeError: Can not serialize value type: <class 'dict'>.
#18140 resolved this for library=urllib clients, but not for library=asyncio clients. Because tornado clients are deprecated, I didn't try to reproduce the bug there.
With that client created and installed, this snippet demonstrates the dict issues on master, and the fix here:
import asyncio
from example_asyncio_client.api_client import ApiClient as AsyncioApiClient
from example_asyncio_client.models.multipart_mixed_request_marker import (
MultipartMixedRequestMarker as AsyncioMultipartMixedRequestMarker,
)
from example_asyncio_client.models.multipart_mixed_status import (
MultipartMixedStatus as AsyncioMultipartMixedStatus,
)
from example_asyncio_client.configuration import Configuration as AsyncioConfiguration
from example_asyncio_client.api.multipart_api import MultipartApi as AsyncioMultipartApi
asyncio_configuration = AsyncioConfiguration(host="http://localhost:8008")
async def asyncio_main():
async with AsyncioApiClient(asyncio_configuration) as asyncio_api_client:
asyncio_api_instance = AsyncioMultipartApi(asyncio_api_client)
marker = AsyncioMultipartMixedRequestMarker(name="Test")
with open("hello-world.txt", "rb") as f:
file1 = f.read()
try:
await asyncio_api_instance.multipart_mixed(
status=AsyncioMultipartMixedStatus.ALLOWED,
file=file1,
marker=marker,
)
except TypeError as e:
print(f"asyncio.multipart_mixed: {type(e).__name__}: {e}")
if __name__ == "__main__":
asyncio.run(asyncio_main())
The other issue is serializing integers, which is also broken for library=asyncio clients. If a spec lists a multipart/form-data field as a number, then it needs to be stringified before being written to the form body. aiohttp has stated they won't do this for the user: https://github.com/aio-libs/aiohttp/issues/920. urllib3 handles this for the user here: https://github.com/urllib3/urllib3/blob/9316764e90aea8d193cd8f03b0caccdf02af3ba0/src/urllib3/filepost.py#L75-L76
PR checklist
- [x] Read the contribution guidelines.
- [x] Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
- [x] Run the following to build the project and update samples:
(For Windows users, please run the script in Git BASH) Commit all changed files. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example./mvnw clean package ./bin/generate-samples.sh ./bin/configs/*.yaml ./bin/utils/export_docs_generators.sh./bin/generate-samples.sh bin/configs/java*. IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed. - [x] File the PR against the correct branch:
master(upcoming 7.6.0 minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks) - [x] If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.
~@wing328 (or whoever is driving the review of this PR) I have another contribution to propose, which handles (filename, filedata) tuples for file parameters. Would it make more sense to split this PR into two: (1) for the dictionary/integer serialization, and (2) for the multiple file parameter support?~ edit: I did: https://github.com/OpenAPITools/openapi-generator/pull/19329
@cbornet @tomplus @krjakbrjak @fa0311 @multani I'd really appreciate your review, thank you!
@roryschadler sorry for the delay ; would you be able to extend the test suite to show the bug and your change fixes it?
No worries @multani! Happy to. Just to confirm, is that a manually-written test added to https://github.com/OpenAPITools/openapi-generator/tree/0f294a51297d788630c96a146a9a48d1dae3e3e5/samples/openapi3/client/petstore/python/tests, or do you mean a different test suite?
Yes, this folder would be a good place.
The test file you already updated should contain some serialization tests already, maybe it could be a good place to extend with a regression test for your fix!
@multani added. The first test: commit is the additional endpoint, the second is the regression test. Note that I removed the other changed test you mentioned in the rebase, as it was included in my previous PR (https://github.com/OpenAPITools/openapi-generator/pull/19329)
@roryschadler thanks for the tests, I will take a look.
In the meantime, can you regenerate the sample files using the commands in the first message of the PR and commit the result? It should fix the unsuccessful check. Thanks!
I did a basic check if the fix was working and it seems so.
I'm really not familiar with the multipart code, @wing328 if you want to take another look, that would be great :)
@wing328 bumping this up in your inbox. Looking forward to your review, thank you!
@wing328 Checking in on this. Is there anything I can do to support your review?
