Skip to content

[python] fix deserialize on basic str fails #18800

Merged
wing328 merged 9 commits intoOpenAPITools:masterfrom
fa0311:fix-18774
Jun 6, 2024
Merged

[python] fix deserialize on basic str fails #18800
wing328 merged 9 commits intoOpenAPITools:masterfrom
fa0311:fix-18774

Conversation

@fa0311
Copy link
Contributor

@fa0311 fa0311 commented May 31, 2024

fix #18774
This PR is another approach to what I changed in #18069.

  • TODO: add test

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.

@cbornet (2017/09) @tomplus (2018/10) @krjakbrjak (2023/02) @fa0311 (2023/10) @multani (2023/10)

self.assertEqual(api_response, "{}") # assertion to ensure {} is sent in the body

api_response = api_instance.test_echo_body_tag_response_string(None)
self.assertEqual(api_response, "") # assertion to ensure emtpy string is sent in the body
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fails because None is not a json.
This test is erroneous and has been removed.

Copy link
Member

Choose a reason for hiding this comment

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

the test is for optional body parameter (e.g. Pet). shall we keep it instead?

Copy link
Contributor Author

@fa0311 fa0311 Jun 5, 2024

Choose a reason for hiding this comment

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

the test is for optional body parameter (e.g. Pet).

If that's the case, this test should be performed in petstore_api instead of echo_api.

Since an empty string is not in JSON format, it conflicts with the Content-Type: application/json in the response header.
Alternatively, is there an option to change the response header of echo_api to Content-Type: text/plain ?

Copy link
Member

@wing328 wing328 Jun 5, 2024

Choose a reason for hiding this comment

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

Using Pet just an example. It can be any models (which will be serialized to JSON string before sending to the server).

Since an empty string is not in JSON format, it conflicts with the Content-Type: application/json in the response header.

Understood as confirmed in https://stackoverflow.com/questions/40614416/is-empty-body-correct-if-content-type-is-application-json. but please take a look at axios/axios#4146 as well, which is also a "use case" presented to use previously (and that's why we've added a test to cover it)

With this change, if None is provided, what will be sent to the server? just empty JSON string "" ?

Copy link
Contributor Author

@fa0311 fa0311 Jun 5, 2024

Choose a reason for hiding this comment

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

Using Pet just an example. It can be any models (which will be serialized to JSON string before sending to the server).

In this case, None is not serialized to Pet. Therefore, it is not serialized into a JSON string before being sent to the server. Only empty json will be sent.

but please take a look at axios/axios#4146 as well, which is also a "use case" presented to use previously (and that's why we've added a test to cover it)

Ok, add a process to prevent json deserialize if the response is empty.

With this change, if None is provided, what will be sent to the server? just empty JSON string "" ?

This change does not change what is sent to the server. An empty json is sent.

Currently, the client may send an empty json request.
You are correct, response should also be allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

try:
data = json.loads(response_text)
except ValueError:
data = response_text
Copy link
Contributor Author

Choose a reason for hiding this comment

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

None for content_type is allowed to maintain backward compatibility, but this is deprecated and may return an incorrect response.

@fa0311 fa0311 marked this pull request as ready for review May 31, 2024 04:53
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.

I'm not sure about the startswith() tests for content-type, shouldn't they be plain == instead?

@fa0311
Copy link
Contributor Author

fa0311 commented May 31, 2024

@multani

Thanks for reviewing.
I'll fix it later.

I'm not sure about the startswith() tests for content-type, shouldn't they be plain == instead?

content-type must be startswith to include options such as charset, boundary.
It should probably be parsed a bit more properly, but startwith will also work.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Type#syntax

@fa0311
Copy link
Contributor Author

fa0311 commented Jun 6, 2024

Can you review it again?

@wing328 wing328 added this to the 7.7.0 milestone Jun 6, 2024
@wing328 wing328 merged commit 6ae8a8f into OpenAPITools:master Jun 6, 2024
@wing328
Copy link
Member

wing328 commented Jun 6, 2024

merged. thanks for the fix.

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.

[BUG][PYTHON] Deserialize on basic str fails since new version

3 participants