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

[typescript-inversify] Fix multipart form uploads #4131

Merged
merged 4 commits into from
Oct 17, 2019
Merged

[typescript-inversify] Fix multipart form uploads #4131

merged 4 commits into from
Oct 17, 2019

Conversation

siada
Copy link
Contributor

@siada siada commented Oct 11, 2019

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

This PR fixes an issue with the typescript-inversify generator for multipart file uploads.

When trying to use this generator within my own code I found that for file uploads, it was adding a content-type header of application/x-www-form-urlencoded;charset=UTF-8 which was incorrect

Based on my own testing I found that when doing a multipart upload using fetch(), the content-type header should be left unset as the browser will itself handle adding the multipart boundary fields

This PR also fixes #4129 as the 'body' param isn't set for www-form or mutlipart requests

@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)

@auto-labeler
Copy link

auto-labeler bot commented Oct 11, 2019

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@@ -152,7 +152,12 @@ export class {{classname}} {

{{#hasFormParams}}
let formData: FormData = new FormData();
{{#isMultipart}}
headers['Content-Type'] = 'multipart/form-data';
Copy link
Member

Choose a reason for hiding this comment

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

It should‘t be set explicitly, as there would be a problem with the separator, or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that’s technically correct, it shouldn’t be explicitly set. But because of the way the HttpClient does an Object.assign to get some default headers, if you leave content-type undefined it becomes application/json which is still wrong

So alongside setting this content-type, I added a check to HttpClient to see if it is a multipart upload and do a delete headers[“content-type”]

Copy link
Member

Choose a reason for hiding this comment

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

ok, I see. maybe

private addJsonHeaders(headers?: Headers) {
return Object.assign({}, {
"Accept": "application/json",
"Content-Type": "application/json"
}, headers);
};

should be fixed instead? @gualtierim what is your opinion?

…ify/HttpClient.mustache

Co-Authored-By: Esteban Gehring <[email protected]>
@macjohnny
Copy link
Member

@siada can you please re-generate the samples and merge the latest master?

@macjohnny macjohnny merged commit 6af234f into OpenAPITools:master Oct 17, 2019
@wing328 wing328 added this to the 4.2.0 milestone Oct 30, 2019
@wing328
Copy link
Member

wing328 commented Oct 31, 2019

@siada thanks for the PR, which has been included in v4.2.0 release: https://twitter.com/oas_generator/status/1189824932345069569

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.

[BUG][typescript-inversify] hasFormData request requires a 'body' parameter that does not exist
3 participants