Skip to content

Conversation

@labrown
Copy link

@labrown labrown commented Jun 2, 2023

Don't overwrite the provided access_token property during configuration call in python client when the spec has a bearer token security schema.

Ran into this issue with the SpaceTraders.io OpenAPI Spec available here

I think this is the correct solution to #13315.

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/utils/export_docs_generators.sh
    
    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*.
    For Windows users, please run the script in Git BASH.
  • [] In case you are adding a new generator, run the following additional script :
    ./bin/utils/ensure-up-to-date
    
    Commit all changed files.
  • File the PR against the correct branch: master (6.3.0) (minor release - breaking changes with fallbacks), 7.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.

Pinging @spacether @krjakbrjak for attention to this python client change.

labrown added 2 commits June 2, 2023 04:45
Don't overwrite the provided access_token property during
configuration call in python client when the spec has bearer token
security schema
@labrown labrown changed the title Python client access token fix [Python] Respect access_token passed into openapi_client.Configuration call Jun 2, 2023
@spacether
Copy link
Contributor

How about writing a test? There is no verification that your feature is working.

@labrown
Copy link
Author

labrown commented Jun 3, 2023

I've spent a couple hours digging around and am not sure where/how to add a test properly. Can you provide an example Pull Request with a test I could use as a pattern?

@spacether
Copy link
Contributor

spacether commented Jun 3, 2023

Sure thing, please add a test in here:
https://github.com/OpenAPITools/openapi-generator/tree/master/samples/openapi3/client/petstore/python/tests
See any PRs that create or edit files there

"""
self.access_token = access_token
"""Access token
"""
Copy link
Member

Choose a reason for hiding this comment

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

for oauth authentication scheme, we still need access_token. what about removing line 203-205 instead?

Copy link
Member

Choose a reason for hiding this comment

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

Please PM me via Slack so that I can help you with the test.

@labrown
Copy link
Author

labrown commented Jun 4, 2023

Help me understand the thinking behind the current code.

Line 142 sets access_token to None as a default value.
Lines 186-188 set access_token to the passed in value given to the Configuration call.
Lines 196-200 activate when #hasOAuthMethods is true and sets access_token to None. Is there a reason to force it to None for OAuth authentication?
Lines 201-207 activate when hasOAuthMethods is false and hasHttpBearerMethods is true, also setting access_token to None. That seems like the actual bug to me.

Seems like configuration.mustache should generate code that sets self.access_token. It would generate cleaner, more easily understood code.

@wing328
Copy link
Member

wing328 commented Jun 10, 2023

I've merged #15802 instead. Please pull the latest master to give it a try.

Happy to reopen this one if needed.

Thanks again for the PR.

@wing328 wing328 closed this Jun 10, 2023
@labrown
Copy link
Author

labrown commented Jun 11, 2023

I'll try it out and see what happens.

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.

3 participants