Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[typescript][node]: Add accept header if produces is not empty #3966

Merged
merged 3 commits into from Oct 2, 2019
Merged

[typescript][node]: Add accept header if produces is not empty #3966

merged 3 commits into from Oct 2, 2019

Conversation

ghost
Copy link

@ghost ghost commented Sep 27, 2019

Uses the correct Accept media types as specified in the OpenApi
specifications.

relates to #3944

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.
    @akehir @macjohnny

Description of the PR

(details of the change, additional tests that have been done, reference to the issue for tracking, etc)

@@ -134,6 +134,9 @@ export class {{classname}} {
.replace('{' + '{{baseName}}' + '}', encodeURIComponent(String({{paramName}}))){{/pathParams}};
let localVarQueryParameters: any = {};
let localVarHeaderParams: any = (<any>Object).assign({}, this.defaultHeaders);
{{#hasProduces}}
localVarHeaderParams.Accept = {{#produces}}{{#-first}}'{{{mediaType}}}'{{/-first}}{{/produces}};
Copy link
Member

Choose a reason for hiding this comment

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

why only the first one?

*/
public async updatePet (body: Pet, options: {headers: {[name: string]: string}} = {headers: {}}) : Promise<{ response: http.ClientResponse; body?: any; }> {
public async updatePet (pet: Pet, options: {headers: {[name: string]: string}} = {headers: {}}) : Promise<{ response: http.ClientResponse; body?: any; }> {
Copy link
Member

Choose a reason for hiding this comment

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

please run the bin/typescript-node-petstore-all.sh script only, not the openapi3 version

@macjohnny
Copy link
Member

thanks for the PR

@macjohnny macjohnny added this to the 4.1.3 milestone Sep 27, 2019
Uses the correct Accept media types as specified in the OpenApi
specifications.

Related to #3944
@ghost
Copy link
Author

ghost commented Sep 29, 2019

Thanks for the review! Somehow managed to copied the {{#-first}} bit from the lua code, the only generator who seems to do it differently... I updated the pull request and the samples look much better now.

Copy link
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

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

LGTM

@macjohnny
Copy link
Member

@ghost
Copy link
Author

ghost commented Oct 1, 2019

After running the client.ts through a debugger I found out that getPetById specifies to produce 'application/xml' and 'application/json'. When accepting both (in that order), the endpoint returns xml. That fails to deserialize...

What to you reckon? Never include 'application/xml'? If 'application/json' is present, don't include anything else?

And how should that logic be implemented? There is no if in mustache, so how to modify the produces field?

@macjohnny
Copy link
Member

thanks for the analysis, good to know.
see e.g.

// to determine the Accept header
const httpHeaderAccepts: string[] = [
];
const httpHeaderAcceptSelected: string | undefined = this.configuration.selectHeaderAccept(httpHeaderAccepts);
if (httpHeaderAcceptSelected !== undefined) {
headers = headers.set('Accept', httpHeaderAcceptSelected);
}
// to determine the Content-Type header
const consumes: string[] = [
'application/json',
'application/xml'
];
const httpContentTypeSelected: string | undefined = this.configuration.selectHeaderContentType(consumes);
if (httpContentTypeSelected !== undefined) {
headers = headers.set('Content-Type', httpContentTypeSelected);
}

// to determine the Accept header
const httpHeaderAccepts: string[] = [
{{#produces}}
'{{{mediaType}}}'{{#hasMore}},{{/hasMore}}
{{/produces}}
];
const httpHeaderAcceptSelected: string | undefined = this.configuration.selectHeaderAccept(httpHeaderAccepts);
if (httpHeaderAcceptSelected !== undefined) {
{{^useHttpClient}}
headers.set('Accept', httpHeaderAcceptSelected);
{{/useHttpClient}}
{{#useHttpClient}}
headers = headers.set('Accept', httpHeaderAcceptSelected);
{{/useHttpClient}}
}

@ghost
Copy link
Author

ghost commented Oct 1, 2019

Ok good idea, would you be happy with this solution:

{{#hasProduces}}
        const produces = [{{#produces}}'{{{mediaType}}}'{{#hasMore}}, {{/hasMore}}{{/produces}}];
        // give precedence to 'application/json'
        if (produces.indexOf('application/json') >= 0) {
            localVarHeaderParams.Accept = 'application/json';
        } else {
            localVarHeaderParams.Accept = produces.join(',');
        }
{{/hasProduces}}

The endpoint may support multiple formats, e.g. 'application/xml',
'application/json'. However, we don't really support xml. In this case
only accept 'application/json'.
Copy link
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

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

LGTM

@macjohnny macjohnny merged commit c136b83 into OpenAPITools:master Oct 2, 2019
Jesse0Michael pushed a commit to Jesse0Michael/openapi-generator that referenced this pull request Oct 3, 2019
…PITools#3966)

* [typescript][node]: Add accept header if produces is not empty

Uses the correct Accept media types as specified in the OpenApi
specifications.

Related to OpenAPITools#3944

* Update samples files

* [typescript][node]: Give presedence to 'application/json'

The endpoint may support multiple formats, e.g. 'application/xml',
'application/json'. However, we don't really support xml. In this case
only accept 'application/json'.
jimschubert added a commit that referenced this pull request Oct 6, 2019
* master: (35 commits)
  [haskell-http-client] update samples (#4073)
  [haskell-http-client] Bump deps to LTS 14.7 (#4068)
  update release for 4.2.0
  [typescript-axios] Fix api generating incorrect seralization type check (#4051)
  prepare 4.1.3 release (#4052)
  typescript-node: form data file (#3967)
  Add a link to blog post on vertx and openapi (#4048)
  better wording for apiNameSuffix option description (#4045)
  [Ruby] fix ruby test, update error message (#4041)
  [PHP] Correctly format JSON in headers (#4024)
  [haskell-http-client] add dateTimeParseFormat cli option - overrides the format string used to parse a datetime (#4037)
  Add frankyjuang to the C# technical committee (#4036)
  Feature/api name suffix (#3918)
  [F#] minor improvements to the generators (#3968)
  Repaired Checkstyle (#4029)
  mockito 3.1.0 (#4035)
  typescript-fetch: fix return type of primitive value (#4028)
  [typescript][node]: Add accept header if produces is not empty (#3966)
  [haskell-http-client] disable unused import warning in Core.hs (#4020)
  Add a link to the tutorial in http4k (#4019)
  ...
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants