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

[Python3] Fix #1445: Safe decoding of response content with try-except #1560

Closed

Conversation

POD666
Copy link

@POD666 POD666 commented Nov 27, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

#1445

@wing328
Copy link
Member

wing328 commented Nov 28, 2018

cc @taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10)

@tomplus
Copy link
Member

tomplus commented Nov 29, 2018

I'm not sure if this approach is safe. Maybe we can pass binary data if we detect that raw content is returned (based on returned headers or a model).

Definitely integral tests for it are needed. We have a use case with uploading file but downloading is not tested.

@wing328
Copy link
Member

wing328 commented Nov 29, 2018

What about using Content-Type in the HTTP response header to determine whether the data (body) is binary or not?

@wing328 wing328 modified the milestones: 3.3.4, 4.0.0 Nov 30, 2018
@POD666
Copy link
Author

POD666 commented Dec 7, 2018

I guess a better option is to keep response data in bytes as long as possible.

@Miiike89
Copy link

Miiike89 commented Jan 2, 2019

Quick qusetion. Will this Fix be in the upcoming 4.0.0 Release?
In the current beta release it is not included.

@spacether
Copy link
Contributor

Does this code path know anything about/use the mime type under the consumes key in the swagger spec? Those mime types can include utf-8 etc definition.

@wing328
Copy link
Member

wing328 commented Mar 25, 2019

Updated the Python Petstore samples. Let's see if the tests pass.

@wing328 wing328 modified the milestones: 4.0.0, 4.0.1 May 13, 2019
@wing328 wing328 modified the milestones: 4.0.1, 4.0.2 May 31, 2019
@wing328 wing328 modified the milestones: 4.0.2, 4.0.3 Jun 20, 2019
@wing328 wing328 modified the milestones: 4.0.3, 4.1.0 Jul 9, 2019
@wing328 wing328 modified the milestones: 4.1.0, 4.1.1 Aug 9, 2019
@wing328 wing328 modified the milestones: 4.1.1, 4.1.2 Aug 26, 2019
@wing328 wing328 modified the milestones: 4.1.2, 4.1.3 Sep 11, 2019
@wing328 wing328 modified the milestones: 4.1.3, 4.2.0 Oct 4, 2019
@wing328 wing328 removed this from the 4.2.0 milestone Oct 30, 2019
@spacether
Copy link
Contributor

spacether commented Jan 27, 2020

@POD666 thank you for this PR.
I am working on cleaning up old python PRs.
Are you still interested in merging this?
Can you resolve the merge conflicts?
@wing328 why was this removed from the milestone?

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.

5 participants