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+Axios: multipart/form-data correctly handled #2002

Merged
merged 13 commits into from
Feb 13, 2019
Merged

Typescript+Axios: multipart/form-data correctly handled #2002

merged 13 commits into from
Feb 13, 2019

Conversation

mvniekerk
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: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @nicokoenig (2018/09) @topce (2018/10)

Description of the PR

When doing a multipart/formdata request, the current state is that it changes the content type to ''application/x-www-form-urlencoded" (violating the contract) and then does a toString() on the request data.

What this PR does is to use FormData as the data, it appends (instead of doing a .set()) and then omits the toString() when the content-type is multipart/form-data

@mvniekerk mvniekerk changed the title Typescript+Axios: multipart/form-data corectly handled Typescript+Axios: multipart/form-data correctly handled Jan 28, 2019
@@ -24,7 +24,7 @@
},
"devDependencies": {
"@types/node": "^8.0.9",
"typescript": "^2.4"
"typescript": "^3.2"
Copy link
Member

Choose a reason for hiding this comment

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

which feature requires typescript 3.2? I would widen the restriction as much as possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted at 9818ee8 -
Thought the spread operators was a later language feature

: configuration.accessToken;
const localVarAccessTokenValue = typeof configuration.accessToken === 'function'
? configuration.accessToken("petstore_auth", ["write:pets", "read:pets"])
: configuration.accessToken;
localVarHeaderParameter["Authorization"] = "Bearer " + localVarAccessTokenValue;
}

if (tags) {
localVarQueryParameter['tags'] = tags.join(COLLECTION_FORMATS["csv"]);
Copy link
Member

Choose a reason for hiding this comment

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

should be adapted too, for consistency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 3ba25dc

public Map<String, Object> postProcessOperations(Map<String, Object> objs) {
objs = super.postProcessOperations(objs);
Map<String, Object> vals = (Map<String, Object>)objs.get("operations");
List<CodegenOperation> operations = (List<CodegenOperation>)vals.get("operation");
Copy link
Member

Choose a reason for hiding this comment

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

add comment what this does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 3ba25dc

@mvniekerk
Copy link
Contributor Author

On an unrelated note - seems like the Shippable pipeline is broken due to debian packages being missing.

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, haven't tested it

@wing328
Copy link
Member

wing328 commented Feb 8, 2019

@mvniekerk the build failed with the following errors:

Status: Downloaded newer image for maven:3-jdk-8
[ERROR] COMPILATION ERROR : 
[ERROR] /gen/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptAxiosClientCodegen.java:[129,5] method does not override or implement a method from a supertype
[ERROR] /gen/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptAxiosClientCodegen.java:[131,21] cannot find symbol
  symbol: method postProcessOperations(java.util.Map<java.lang.String,java.lang.Object>)
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.5.1:compile (default-compile) on project openapi-generator: Compilation failure: Compilation failure: 
[ERROR] /gen/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptAxiosClientCodegen.java:[129,5] method does not override or implement a method from a supertype
[ERROR] /gen/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptAxiosClientCodegen.java:[131,21] cannot find symbol
[ERROR]   symbol: method postProcessOperations(java.util.Map<java.lang.String,java.lang.Object>)
[ERROR] -> [Help 1]

Please take a look and let us know if you need help fixing the issue.

@wing328
Copy link
Member

wing328 commented Feb 11, 2019

Update: Pushed 061cea5 to fix the compilation error. Let's see how it goes.

@wing328 wing328 mentioned this pull request Feb 13, 2019
4 tasks
@wing328
Copy link
Member

wing328 commented Feb 13, 2019

All CI tests passed 👍

@wing328 wing328 merged commit 550774a into OpenAPITools:master Feb 13, 2019
@mvniekerk mvniekerk deleted the bugfix-multipart-form-data branch February 17, 2019 16:30
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
…#2002)

* Typescript 3.2

* Typescript spread operator

* Add vendor extension to the operation

* Remove url.URLSearchParams

* Generate form data in API

* Make axios scripts executable

* Reran generator

* Generate sample code

* Revert to 2.4 Typescript

* COLLECTION_FORMAT.{{collectionFormat}} everywhere for consistency

* Consistency on the CollectionFormats, comment on the vendor extension

* fix compilation error
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.

4 participants