Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(python,asyncio): multipart form data serialization #19302

Conversation

roryschadler
Copy link
Contributor

@roryschadler roryschadler commented Aug 5, 2024

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: aio-libs/aiohttp#920. urllib3 handles this for the user here: https://github.com/urllib3/urllib3/blob/9316764e90aea8d193cd8f03b0caccdf02af3ba0/src/urllib3/filepost.py#L75-L76

PR checklist

  • Read the contribution guidelines.
  • 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.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (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 ./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.
  • 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)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@roryschadler
Copy link
Contributor Author

@roryschadler roryschadler changed the title fix(python,asyncio): multipart object serialization and multiple files fix(python,asyncio): multipart form data serialization and multiple files Aug 7, 2024
@roryschadler roryschadler force-pushed the roryschadler/fix-object-serialization-multipart-python-asyncio branch from f2a8321 to 29b4a60 Compare August 7, 2024 18:36
@roryschadler
Copy link
Contributor Author

roryschadler commented Aug 7, 2024

@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: #19329

@roryschadler roryschadler force-pushed the roryschadler/fix-object-serialization-multipart-python-asyncio branch from 29b4a60 to 6351aa8 Compare August 8, 2024 21:40
@roryschadler roryschadler changed the title fix(python,asyncio): multipart form data serialization and multiple files fix(python,asyncio): multipart form data serialization Aug 8, 2024
@roryschadler
Copy link
Contributor Author

@wing328 following up - are you the right person to review this?

@roryschadler
Copy link
Contributor Author

@cbornet @tomplus @krjakbrjak @fa0311 @multani I'd really appreciate your review, thank you!

@multani
Copy link
Contributor

multani commented Aug 22, 2024

@roryschadler sorry for the delay ; would you be able to extend the test suite to show the bug and your change fixes it?

This PR is essentially
<OpenAPITools#18140> but for
the asyncio client.
@roryschadler
Copy link
Contributor Author

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?

@multani
Copy link
Contributor

multani commented Aug 22, 2024

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!

@roryschadler roryschadler force-pushed the roryschadler/fix-object-serialization-multipart-python-asyncio branch from 6351aa8 to edd933a Compare August 23, 2024 02:48
@roryschadler
Copy link
Contributor Author

@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 (#19329)

@multani
Copy link
Contributor

multani commented Aug 23, 2024

@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!

@multani
Copy link
Contributor

multani commented Aug 26, 2024

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 :)

Copy link
Contributor

@multani multani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you adjust the aiohttp test setup? It took me some time to figure out why it wasn't working on my machine :)

The same fix should probably be done for pydantic-v1-aiohttp.

Extends previous commits (and OpenAPITools#18140) to cover the python-pydantic-v1
client as well.
Also brings the second test in line with the first, patching
`urllib3.PoolManager.urlopen`
Copy link
Contributor

@multani multani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for the fix(es) @roryschadler !

@wing328 can you take a look as well?

@roryschadler
Copy link
Contributor Author

@wing328 bumping this up in your inbox. Looking forward to your review, thank you!

@roryschadler
Copy link
Contributor Author

@wing328 Checking in on this. Is there anything I can do to support your review?

@wing328 wing328 added this to the 7.9.0 milestone Sep 9, 2024
@wing328 wing328 merged commit 8171648 into OpenAPITools:master Sep 9, 2024
36 checks passed
@wing328
Copy link
Member

wing328 commented Sep 9, 2024

thanks for the PR, which has been merged

sorry for the delay.

@roryschadler
Copy link
Contributor Author

Thank you, @wing328!

@roryschadler roryschadler deleted the roryschadler/fix-object-serialization-multipart-python-asyncio branch September 9, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants