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

[core][Python] rest client: handle multiple response types #2134

Closed

Conversation

rienafairefr
Copy link
Contributor

@rienafairefr rienafairefr commented Feb 12, 2019

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, ./bin/openapi3/{LANG}-petstore.sh, ./bin/security/{LANG}-petstore.sh and ./bin/openapi3/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

In OAS 3 it's possible to return multiple content-type, and for now the rest clients in the different languages don't support that, e.g. problems or related PRs: #206 #1367 #440
I've added returnType returnType field to the CodegenResponse class, this way we can get a mapping between the response code and the expected return type of that response, and I've implemented that logic in the Python client, through a response_types parameter dictionary. The Accept header is modified to include all the cases that the client expects, not just application/json.
The REST response is UTF-8 decoded if needed (not decoded in case of a file type), and deserialized according to the response type expected for the HTTP status code that is encountered.

Tested working on a API endpoint that returns a binary file on HTTP 200, and can also send a JSON with an error message for HTTP 401/403/500.

@rienafairefr rienafairefr changed the title rest client: handle multiple response types [core][Python] rest client: handle multiple response types Feb 12, 2019
@spacether
Copy link
Contributor

Hi @rienafairefr, can you fix the Travis-CI errors? It looks like your
modules/openapi-generator/src/main/resources/python/asyncio/rest.mustache is missing import six at the top.

@spacether
Copy link
Contributor

@rienafairefr why not remove the response_type variable and instead pass your response_types dict only?
You are passing the same data in both locations. What do other people think? @wing328

@rienafairefr
Copy link
Contributor Author

@spacether thanks I pushed the missing import six.
I think you're right about removing response_type, I'm pushing that as well, hopefully CI will not blow up ^^

Schema thisResponseSchema = (Schema) response.schema;
if (thisResponseSchema != null) {
CodegenProperty cm = fromProperty("response", thisResponseSchema);
response.dataType = cm.dataType;
Copy link
Contributor

@spacether spacether Apr 8, 2019

Choose a reason for hiding this comment

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

@rienafairefr why are you setting the response's dataType equal to the CodegenProperty dataType here? What is incorrect about how fromResponse is setting response.dataType higher up?

# multiple potential response types
response_types = {
{{#responses}}
{{code}}: {{#dataType}}'{{dataType}}'{{/dataType}}{{^dataType}}None{{/dataType}}{{#hasMore}},{{/hasMore}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use baseType here instead of dataType?
It looks like dataType is used for primitive types. It looks like baseType is used for model names per here:
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java#L2678
Then the baseType is used to import the model name here:
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java#L2382

@spacether
Copy link
Contributor

@rienafairefr what's the status of this PR?
The work that you have done here is great and would be very useful to the end users + clients.
if you resolve the merge conflicts and the open questions I can give this a review now that I am on the review committee.

@spacether
Copy link
Contributor

spacether commented Jan 27, 2020

@rienafairefr thank you for this PR.
I am working on cleaning up old Python PRs.
This feature looks very helpful as it lets the users ingest different models depending upon the response code sent back.
What's the status of this PR?
Are you interested in updating and merging it or would you rather it be closed?

@rienafairefr
Copy link
Contributor Author

I had need for that at some point but not anymore so I guess the PR staled. The feature is still useful IMO, I'll try to look at it

@spacether
Copy link
Contributor

This PR has been closed due to inactivity

@spacether spacether closed this Aug 16, 2022
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.

4 participants