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

[BUG][Java] isBasicBearer not used in default template #4178

Closed
5 of 6 tasks
tpeyrard opened this issue Oct 18, 2019 · 9 comments · Fixed by #4278
Closed
5 of 6 tasks

[BUG][Java] isBasicBearer not used in default template #4178

tpeyrard opened this issue Oct 18, 2019 · 9 comments · Fixed by #4278

Comments

@tpeyrard
Copy link

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • What's the version of OpenAPI Generator used?
  • Have you search for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Bounty to sponsor the fix (example)
Description

Generating the code with the default Java template, it seems the new added isBasicBearer is not used:

authentications = new HashMap<String, Authentication>();{{#authMethods}}{{#isBasic}}{{#isBasicBasic}}
authentications.put("{{name}}", new HttpBasicAuth());{{/isBasicBasic}}{{^isBasicBasic}}
authentications.put("{{name}}", new HttpBearerAuth("{{scheme}}"));{{/isBasicBasic}}{{/isBasic}}{{#isApiKey}}

The Bearer part is defined inside the isBasicBasic. I thought it would be within the isBasicBearer.

openapi-generator version

4.1.3

OpenAPI declaration file content or url

Although the problem is at the template level, not the code generation I think, here is the link to the OpenAPI spec:
https://api.criteo.com/marketing/swagger/docs/v.1.0

Command line used for generation

https://github.com/criteo/criteo-marketing-sdk-generator/blob/master/build-java/build.gradle#L66-L93

Thanks :)

@macjohnny
Copy link
Member

@tpeyrard would you like to file a PR to fix this?

@tpeyrard
Copy link
Author

tpeyrard commented Oct 18, 2019 via email

@macjohnny
Copy link
Member

looking at other templates like e.g.

{{#authMethods}}
{{#isBasic}}
{{#isBasicBasic}}
if (this.configuration && (this.configuration.username !== undefined || this.configuration.password !== undefined)) {
headerParameters["Authorization"] = "Basic " + btoa(this.configuration.username + ":" + this.configuration.password);
}
{{/isBasicBasic}}
{{#isBasicBearer}}
if (this.configuration && this.configuration.accessToken) {
const token = this.configuration.accessToken;
const tokenString = typeof token === 'function' ? token("{{name}}", [{{#scopes}}"{{{scope}}}"{{^-last}}, {{/-last}}{{/scopes}}]) : token;
if (tokenString) {
headerParameters["Authorization"] = `Bearer ${tokenString}`;
}
}
{{/isBasicBearer}}
{{/isBasic}}

it seems {{#isBasicBearer}} is correct

@macjohnny
Copy link
Member

however, looking at the code

authentications = new HashMap<String, Authentication>();{{#authMethods}}{{#isBasic}}{{#isBasicBasic}}
authentications.put("{{name}}", new HttpBasicAuth());{{/isBasicBasic}}{{^isBasicBasic}}
authentications.put("{{name}}", new HttpBearerAuth("{{scheme}}"));{{/isBasicBasic}}{{/isBasic}}{{#isApiKey}}

the line

{{^isBasicBasic}} 
 authentications.put("{{name}}", new HttpBearerAuth("{{scheme}}"));{{/isBasicBasic}}

should be added if basic-bearer authentication is enabled, since
{{^isBasicBasic}} is the negation of {{#isBasicBasic}}

@tpeyrard
Copy link
Author

Ok, thanks.
I'll see how to build and add tests to the templates.

@tpeyrard
Copy link
Author

So I tried to "fix" it but there is nothing to fix in fact.
I learnt the "inverted selection" in Mustache, so the current code is not using isBasicBearer, but since there is only one possible choice other than isBasicBasic, it works.

So either I leave the code as-is, or I change it to use the isBasicBearer instead of ^isBasicBasic (pay attention to the ^).

wdyt?

@macjohnny
Copy link
Member

i think isBasicBearer is more future-proof

@addynaikvh
Copy link
Contributor

addynaikvh commented Oct 25, 2019

Sorry i hadn't seen this defect. I fixed it since we needed for for our purpose. I created a PR with the fix.

@tpeyrard
Copy link
Author

tpeyrard commented Oct 25, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants