-
-
Notifications
You must be signed in to change notification settings - Fork 7k
[Python] conditionally set auth attributes #4605
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
Conversation
@taxpon @frol @mbohlool @cbornet @kenjones-cisco @tomplus @Jyhess @slash-arun @spacether |
modules/openapi-generator/src/main/resources/python/configuration.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/python/configuration.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/python/configuration.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/python/configuration.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/python/python-experimental/api_client.mustache
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/python/python-experimental/api_client.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/python/configuration.mustache
Outdated
Show resolved
Hide resolved
Not directly related to this PR, is there any reason the generated Python classes do not leverage class inheritance? |
config = Configuration() | ||
config.host = HOST | ||
self.api_client = petstore_api.ApiClient(config) | ||
self.config = Configuration() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optionally pass in the host an access_token inputs into the Configuration
instance rather than setting them with .host .access_token
Which classes, the Configuration class, the model classes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making those updates and updating the unit test. The use case where users may want to pass in empty strings for auth values sounds like a valid test case.
These changes look good.
Sorry, in the model class. For example, in the generated Java, "Cat extends Animal" and "Dog extends Animal". This is done when the OAS spec has the "allOf" keyword and discriminants, and the mustache template uses the "parent" tag. I know doing class inheritance is not required and classes can be flattened, but still it's nice to have class inheritance because the generated classes are less verbose and easier to read. I'm asking because I have submitted a PR for golang to support embedded structs (which is one way in golang to model type inheritance), and I wonder if I should submit a PR for Python too. |
Never mind, I see there is already a PR: #4446
|
Composed inheritance models can have the same properties in multiple child schemas. |
@spacether , I think I have addressed all your comments. |
modules/openapi-generator/src/main/resources/python/configuration.mustache
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Thanks for you contribution!
modules/openapi-generator/src/main/resources/python/configuration.mustache
Outdated
Show resolved
Hide resolved
samples/client/petstore/python-experimental/tests/test_api_client.py
Outdated
Show resolved
Hide resolved
When you have time, please PM me via Slack to have a discussion on this in Jan 2020. |
Based on feedback, I have opened a new PR to replace this one: #4988 |
Fix for #3844:
Currently, the python client obtains the list of security schemes that have been specified in the OAS spec, and automatically adds the authentication attributes in the header, cookie or query. This happens even if the parameters are unset. For example, the "Authorization" header may be set with an empty Bearer token:
With this PR, the bearer access token is no longer set if its value is None.
Tests performed:
PR checklist
./bin/
(or Windows batch scripts under.\bin\windows
) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run./bin/{LANG}-petstore.sh
,./bin/openapi3/{LANG}-petstore.sh
if updating the code or mustache templates for a language ({LANG}
) (e.g. php, ruby, python, etc).master
,4.3.x
,5.0.x
. Default:master
.