Skip to content

Conversation

masaeedu
Copy link
Contributor

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 and ./bin/security/{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\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

This change adds support for non-"application/json" MIME types in a body parameter. The logic has some hairy bits I'm not entirely happy with, but I'm trying to follow what the other language generators do:

  • Instead of always emitting a default "Content-Type" of "application/json", emit the first MIME-type from the operation's consumes array if available, and fall back to "application/json" if this is not present.

    This is a little ugly; if the user needs to use a MIME-type from the consumes array that isn't the first one, they need to know to pass it in options.headers["Content-Type"], which isn't very visible. I'd like to make the content type an explicit, required parameter on operations where there's more than one entry in the consumes array, but for now I've just followed what I've seen most of the other language clients.

  • In cases where the body parameter has datatype string and the content type isn't application/json, simply set the request body to the parameter directly. Otherwise, JSON.stringify the parameter as before. This doesn't take into consideration other any other possible dataType values that may represent raw body streams; I am unaware of any such datatypes but if you know of any please point them out.

Hopefully some of the reviewers can provide feedback on these points.

Fixes #6940.

Technical committee reviewers

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx

@masaeedu
Copy link
Contributor Author

masaeedu commented Nov 19, 2017

Note: all the Response changes are unrelated to the PR, running ./run-in-docker.sh mvn clean package && ./bin/typescript-fetch-petstore-all.sh on a clean clone of master will result in the same changes. I believe the Response -> any diff was mistakenly introduced in #6739.

EDIT: I've split these changes out into a separate commit for clarity.

@masaeedu masaeedu force-pushed the 6940-tsfetch-contenttype branch from c3565f7 to 6b678f1 Compare November 19, 2017 01:11
If the spec specifies a list of consumes MIME types, default to the
first MIME type in the list instead of "application/json" (the user may
still override this by passing "Content-Type" in options.headers).

Additionally, only perform explicit JSON serialization if the data type
of the body parameter is not "string", or if it is string, only when the
content type is "application/json".
@masaeedu masaeedu force-pushed the 6940-tsfetch-contenttype branch from 6b678f1 to 598e90b Compare November 19, 2017 01:16
@masaeedu masaeedu changed the title 6940 tsfetch contenttype [TypeScript-Fetch] Support for non-JSON body params Nov 19, 2017
localVarHeaderParameter['Content-Type'] = 'application/json';
{{/consumes}}
{{#consumes.0}}
localVarHeaderParameter['Content-Type'] = '{{mediaType}}';
Copy link
Contributor

Choose a reason for hiding this comment

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

@masaeedu Please use {{{mediaType}}} to use the original value fo the media type as I remember there are edge cases causing issues with the HTML-escaped values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wing328 Good catch. Should we be doing this for things like {{className}}, {{paramName}} etc. as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

@masaeedu yes please

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI. I've filed #7073 to clean up {{mediaType}} in other templates.

@wing328 wing328 added this to the v2.3.0 milestone Nov 28, 2017
@wing328 wing328 merged commit a3ca8d5 into swagger-api:master Dec 19, 2017
@wing328
Copy link
Contributor

wing328 commented Dec 19, 2017

@masaeedu PR merged into master. I'll fix the {{mediaType}} later.

cc @TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx

@wing328
Copy link
Contributor

wing328 commented Dec 19, 2017

@masaeedu fixed via #7210

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.

2 participants