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

Don't cast list to tuple in python-prior binding #14014

Merged
merged 2 commits into from
Nov 16, 2022

Conversation

Ichimonji10
Copy link
Contributor

Tweak the python-prior API bindings, so that they no longer cast lists to tuples when making a POST request with a multipart/form-data content-type. This fixes an interaction with
urllib3.request_encode_body, whose fields parameter expects tuples, not lists.

cc @spacether

See: https://urllib3.readthedocs.io/en/stable/reference/urllib3.request.html

Fix: #14012

@Ichimonji10
Copy link
Contributor Author

I tested with the following:

mvn clean package
sudo update-alternatives --set java java-11-openjdk.x86_64
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh

mvn clean install
java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate \
  -i ~/Downloads/sample.yaml \
  -g python-prior \
  -o ~/Downloads/out/

For info on the call to update-alternatives, see #13684.

For info on the generation from sample.yaml, see #14012.

I have also tested this change against an internal service at my company.

Copy link
Contributor

@spacether spacether left a comment

Choose a reason for hiding this comment

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

Hey there. This fix looks great but it has no verification test.
Can you please add a test in the tests_manual directory?
If that test is omitted, then this code could be changed in the future, breaking this feature and we would not see any error indicating that something had broken.

@Ichimonji10
Copy link
Contributor Author

Ack, will do.

@Ichimonji10
Copy link
Contributor Author

WIP, please ignore for now. I will ping when ready for review.

Note to self:

mvn integration-test -f samples/openapi3/client/petstore/python-prior/pom.xml
cd samples/openapi3/client/petstore/python-prior/
./test_python.sh

@Ichimonji10
Copy link
Contributor Author

Ichimonji10 commented Nov 16, 2022

Done. This MR now contains two commits, the first of which adds a test, case, and the second of which adds a code fix:

git checkout urllib3~1
./test_python.sh  # fail

git checkout urllib3
./test_python.sh  # success

EDIT: Rebased so that some comments could be tweaked.

In the spirit of test driven development, this test intentionally fails.
A following commit will fix the code to comply with the test.

See: OpenAPITools#14012
Tweak the python-prior API bindings, so that they no longer cast lists
to tuples when making a POST request with a multipart/form-data
content-type. This fixes an interaction with
`urllib3.request_encode_body`, whose `fields` parameter expects tuples,
not lists.

cc @spacether

See: https://urllib3.readthedocs.io/en/stable/reference/urllib3.request.html

Fix: OpenAPITools#14012
Copy link
Contributor

@spacether spacether left a comment

Choose a reason for hiding this comment

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

This looks great, thank you for the fix and adding the test!

@spacether spacether merged commit 92ecee8 into OpenAPITools:master Nov 16, 2022
@Ichimonji10 Ichimonji10 deleted the urllib3 branch November 16, 2022 18:32
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.

python-prior client casts list to tuple, breaking interaction with urllib3
2 participants