openapi-generator icon indicating copy to clipboard operation
openapi-generator copied to clipboard

Restore "authNames" when applying authentication for operations

Open noordawod opened this issue 2 years ago • 3 comments

This pr brings back 2 lost "features" that were present earlier (and of course with the proper explanation as to why we need them back), and introduces a couple of new features as well:

  1. Inside a function, private-style variables (var _json) make no sense – so removed the underscore.
  2. ~~The authNames were mysteriously dropped, why? These are important information to each endpoint, and the authentication implementation may need to check which authentications to apply for each operation.~~
  3. Removed use of the bang operator (!) when activating the authentication implementation – just use what Dart gives us out of the box: authentication?.applyToParams(…)
  4. Converted the applyToParams method to a Future<void>. Custom implementations may need to retrieve data from persistent stores, for example, and so may need this.
  5. ~~Since Authentication is an interface, and in case ApiClient is inherited, one can now declare the generic T extends Authentication class upon extension. Helps to access properties added in the inherited class (yes, this is backward-compatible and Dart, luckily, doesn't scream about it.)~~

PR checklist

  • [X] Read the contribution guidelines.
  • [X] 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.
  • [X] 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.
  • [X] File the PR against the correct branch: master (6.1.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
  • [X] If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@jimschubert @dr4ke616 @karismann @Zomzog @andrewemery @4brunu @yutaka0m @wing328 @jaumard @josh-burton @amondnet @sbu-WBT @kuhnroyal @agilob @ahmednfwela

noordawod avatar Aug 06 '22 20:08 noordawod

I remember this being changed due to https://github.com/OpenAPITools/openapi-generator/pull/11360 , as a fix for https://github.com/OpenAPITools/openapi-generator/issues/7057 will this issue regress after this PR ?

ahmednfwela avatar Aug 07 '22 14:08 ahmednfwela

I remember this being changed due to #11360 , as a fix for #7057 will this issue regress after this PR ?

Hmm, I didn't know about the openapi requirement. It makes sense, so I will revert back the changes but keep the Future nature of applyToParams, which is okay I think.

noordawod avatar Aug 07 '22 14:08 noordawod

yes, I think making it a Future is a good improvement, especially since it's a common scenario to persist and read auth tokens from local storage, which is usually a Future

ahmednfwela avatar Aug 07 '22 14:08 ahmednfwela

Any one has more feedback on this change?

cc @jaumard (2018/09) @josh-burton (2019/12) @amondnet (2019/12) @sbu-WBT (2020/12) @kuhnroyal (2020/12) @agilob (2020/12) @ahmednfwela (2021/08)

wing328 avatar Aug 14 '22 03:08 wing328

LGTM

ahmednfwela avatar Aug 14 '22 18:08 ahmednfwela

Hello, I just upgraded OpenAPI generator and I'm not at all happy with applyToParams being a Future. It breaks my whole application and just doesn't make sense. When a custom implementation is used, the credentials should just be passed to the class instead of being fetched inside the class. Even the name of the function already means that it only applies and not that it fetches the credentials. Sorry, but this change has to be reverted. The uses case of this is invalid as it means the architecture of the applications is broken. The function for applying the credentials to the requests should never be responsible for fetching the credentials. This design also means the credentials could be fetched on every request.

provokateurin avatar Sep 15 '22 18:09 provokateurin

"It breaks my whole application and just doesn't make sense." -- made sense to us all ☺️

"When a custom implementation is used, the credentials should just be passed to the class instead of being fetched inside the class" -- that's one way to do it, which can be done with applyToParams being a Future.

"The uses case of this is invalid as it means the architecture of the applications is broken." -- could it be that your app's architectural design choices are wrong?

"This design also means the credentials could be fetched on every request." -- makes perfect sense to me; fresh credentials from the source of truth.

Would you like to share your use case? Maybe we can assist you.

noordawod avatar Sep 15 '22 18:09 noordawod

sorry i just had to rant a little. i don't want to bother anymore with openapi generator since it breaks so often. this is a breaking change and it has been merged into a minor version. this is inacceptable, but i don't care anymore.

provokateurin avatar Sep 15 '22 18:09 provokateurin

@jld3103 can you share the specific code that broke by upgrading this version? it would be helpful if you could open a new issue

ahmednfwela avatar Sep 15 '22 22:09 ahmednfwela