Support Multiple API Keys by nmuesch · Pull Request #3450 · OpenAPITools/openapi-generator

nmuesch · 2019-07-24T21:33:54Z

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 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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
Filed the PR against the correct branch: master, 4.1.x, 5.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

Follow up to #3210

nmuesch · 2019-07-24T21:35:19Z

Hey @wing328 👋 I opened this PR as a followup to your review on #3210 I haven't done the updates to the sample files yet but was wondering if you could take a look to see if this is more along the lines of what you were thinking (and to confirm I've opened the PR at the right place) Thanks!

bkabrda · 2019-07-26T14:07:45Z

So just to make sure that I understand this PR correctly, it allows adding multiple keys by removing the assumption that there is just one apikey in the context and gets them by names that were specified in the security schemes. Is that correct?

wing328 · 2019-07-29T13:40:58Z

@bkabrda yes, I think the goal is to support multiple API keys as the current approach only supports one API key.

I was expecting the PR to use maps somehow similar to other generators, e.g.:

but there could be other approaches to support multiple API key.

bkabrda · 2019-07-29T14:40:55Z

Right, so I think the potential problem with the current approach of this PR is that the names of the API keys, which are given in the OpenAPI spec, could potentially conflict with other names in the context. These are currently token, basic, accesstoken, which someone could use as names for API keys.

Putting a map with API keys in the context would prevent these collisions, so I think this is a good suggestion. This map would have a reserved keyname there, much like the previously used ContextAPIKey = contextKey("apikey"), so something like ContextAPIKeys = contextKey("apikeys"), which would be of type map[string]string. @nmuesch does that make sense to you?

nmuesch · 2019-08-19T22:12:07Z

Hey, @wing328 and @bkabrda apologies for the delay on this one. These latest commits use a different approach, using the map idea that you had both mentioned. Is this more in line with what you were thinking? If so I can run the script to generate the examples. Thanks for the reviews!

bkabrda · 2019-08-21T10:09:04Z

modules/openapi-generator/src/main/resources/go-experimental/README.mustache


 ```golang
-auth := context.WithValue(context.Background(), sw.ContextAPIKey, sw.APIKey{
+auth := context.WithValue(context.Background(), sw.ContextAPIKeys, sw.APIKey{


This doesn't look right. It sets the context value to be the APIKey, but in fact it should be the map, right?

Good catch! This was indeed outdated. I couldn't find a way to only show the example once and only if there was an API Key auth present in the list of authMethods, so I followed the approach of the other languages READMEs, which was to remove the example and just list each type of key.

Ex:
Python - https://github.com/OpenAPITools/openapi-generator/tree/5956569e7a0d423521168f894bd1ac7b56ad35f1/samples/openapi3/client/petstore/python#documentation-for-authorization
Kotlin - https://github.com/OpenAPITools/openapi-generator/tree/5956569e7a0d423521168f894bd1ac7b56ad35f1/samples/openapi3/client/petstore/kotlin#documentation-for-authorization
(There are other examples for this as well)

So how about adding an example to each entry of the list? I do understand the issue with not being able to implement document this really good, but there should be some example of how to put the map in the context.

Talked about this offline a bit. I'm hesitant to leave example code because the example will not work if there are multiple API keys required. (Because each example will setup a context with a different one-element map) I assume thats why the other client README templates don't have an example.

I did though, add a note that says you have to add this key to a map[string]APIKey inside each APIKey section. What do you think?

Yeah, looks much better now 👍

bkabrda

LGTM now 👍 @wing328 could you give this a look please?

wing328 · 2019-09-03T13:30:01Z

cc @antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) @bkabrda (2019/07)

If no further feedback from anyone, I'll merge it this weekend.

nmuesch · 2019-09-09T18:00:09Z

Hey, just wanted to follow up and see if there was anything else I could do here to get this merged? Thanks 🙇

wing328 · 2019-09-10T02:53:45Z

modules/openapi-generator/src/main/resources/go-experimental/README.mustache

-})
-r, err := client.Service.Operation(auth, args)
-```
+Note, each API key must be added to a map of `map[string]APIKey` where the key is: {{keyParamName}} and passed in as the auth context for each request.


@nmuesch minor feedback. Is there a way we can keep the sample code?

It would be great to show the code to use the API key (single or multiple)

Hey, thanks for the feedback. There was a brief discussion of this here #3450 (review) interested in your thoughts. The main reason I changed it was because it made the example code incorrect for any apps that have multiple keys. I couldn't figure out a way to show the full multi-key example using the template variables.

We can do it in the actual code sample instead and we can do it in another PR instead of this one as I don't want this PR to be opened for too long

wing328 · 2019-09-10T03:01:56Z

modules/openapi-generator/src/main/resources/go-experimental/api.mustache

+					key = auth.Key
+				}
+				{{#isKeyInHeader}}
+				localVarHeaderParams["{{keyParamName}}"] = key


@nmuesch can we check to see if the key is not empty before adding to localVarHeaderParams or localVarQueryParams?

The reason is that sometimes the key can either be put in the header or the URL query string so the goal is to skip the API key in header or URL query string if the value is not set. We can address it in another PR later when someone reports the issue.

wing328 · 2019-10-04T14:21:34Z

@nmuesch thanks for the PR, which has been included in the v4.1.3 release: https://twitter.com/oas_generator/status/1180123829626003456

Support Multiple API Keys

fb0c441

richardwhiuk added the Client: Go label Jul 30, 2019

nmuesch added 3 commits August 19, 2019 17:42

Use maps

6fc514c

Merge branch 'master' into nick/go_api_keys

86bae2c

Fix readme template

e0e441c

bkabrda reviewed Aug 21, 2019

View reviewed changes

nmuesch added 2 commits August 21, 2019 12:38

Update readme

c30515e

Address readme review

effb929

bkabrda approved these changes Aug 23, 2019

View reviewed changes

wing328 reviewed Sep 10, 2019

View reviewed changes

wing328 added the Issue: Bug label Sep 14, 2019

wing328 added this to the 4.1.3 milestone Sep 14, 2019

wing328 merged commit 334d0dc into OpenAPITools:master Sep 14, 2019

Uh oh!

Conversation

nmuesch commented Jul 24, 2019

PR checklist

Description of the PR

Uh oh!

nmuesch commented Jul 24, 2019

Uh oh!

bkabrda commented Jul 26, 2019

Uh oh!

wing328 commented Jul 29, 2019

Uh oh!

bkabrda commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nmuesch commented Aug 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkabrda left a comment

Choose a reason for hiding this comment

Uh oh!

wing328 commented Sep 3, 2019

Uh oh!

nmuesch commented Sep 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wing328 commented Oct 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bkabrda commented Jul 29, 2019 •

edited

Loading