-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
typescript-fetch: Properly detect and encode container request body param #3517
Conversation
@@ -181,7 +181,7 @@ export class {{classname}} extends runtime.BaseAPI { | |||
{{#hasBodyParam}} | |||
{{#bodyParam}} | |||
{{#isContainer}} | |||
body: requestParameters.{{paramName}}.map({{#items}}{{datatype}}{{/items}}ToJSON), | |||
body: JSON.stringify(requestParameters.{{paramName}}{{#isListContainer}}{{#items}}{{^isPrimitiveType}}.map({{datatype}}ToJSON){{/isPrimitiveType}}{{/items}}{{/isListContainer}}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The body is a string now. Does this work correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the Fetch API:
body
: Any body that you want to add to your request: this can be a Blob, BufferSource, FormData, URLSearchParams, or USVString object. Note that a request using the GET or HEAD method cannot have a body.
Passing random objects and lists on my local results in a nonsense body value of [object Object]
. I believe this has always been broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strange, but how can this
openapi-generator/samples/client/petstore/typescript-fetch/builds/default/apis/PetApi.ts
Line 95 in fae0738
body: PetToJSON(requestParameters.body), |
work, it is also a object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is converted here:
openapi-generator/samples/client/petstore/typescript-fetch/builds/default/runtime.ts
Lines 63 to 65 in fae0738
const body = (context.body instanceof FormData || isBlob(context.body)) | |
? context.body | |
: JSON.stringify(context.body); |
so JSON.stringify is not necessary here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see - did not catch that, thanks!
modules/openapi-generator/src/main/resources/typescript-fetch/apis.mustache
Outdated
Show resolved
Hide resolved
…aram Signed-off-by: Prateek Malhotra <[email protected]> Co-Authored-By: Esteban Gehring <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@someone1 thanks for the PR, which has been included in the 4.1.0 release: https://twitter.com/oas_generator/status/1160000504455319553 |
PR checklist
./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.master
,4.1.x
,5.0.x
. Default:master
.Description of the PR
Passing lists/containers to the body param as-is will send garbage via
fetch
- needed to stringify the contents first and only map over the items if it was a list of non-primitives.fixes #3278
@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @nicokoenig (2018/09) @topce (2018/10) @akehir (2019/07)