[typescript-axios]: Only call `JSON.stringify` on JSON request bodies #7580

therockstorm · 2020-10-02T16:29:44Z

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*. For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny @topce @akehir @petejohansonxo @amakhrov

therockstorm · 2020-10-02T16:32:21Z

modules/openapi-generator/src/main/resources/typescript-axios/baseApi.mustache

+ *
+ * @export
+ */
+export const JSON_VENDOR_MIME_PATTERN = new RegExp('application\\/vnd.(.*)+json(;.*)?', 'i');


These should match those at https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java#L6606-L6607

maybe we could create a small function that does the check, similar to

openapi-generator/modules/openapi-generator/src/main/resources/typescript-angular/configuration.mustache

Lines 135 to 148 in 9236d56

/**

* Check if the given MIME is a JSON MIME.

* JSON MIME examples:

* application/json

* application/json; charset=UTF8

* APPLICATION/JSON

* application/vnd.company+json

* @param mime - MIME (Multipurpose Internet Mail Extensions)

* @return True if the given MIME is JSON, false otherwise.

*/

public isJsonMime(mime: string): boolean {

const jsonMime: RegExp = new RegExp('^(application\/json|[^;/ \t]+\/[^;/ \t]+[+]json)[ \t]*(;.*)?$', 'i');

return mime !== null && (jsonMime.test(mime) || mime.toLowerCase() === 'application/json-patch+json');

}

therockstorm · 2020-10-02T16:45:11Z

modules/openapi-generator/src/main/resources/typescript-axios/apiInner.mustache

@@ -203,7 +203,8 @@ export const {{classname}}AxiosParamCreator = function (configuration?: Configur
            localVarRequestOptions.data = localVarFormParams{{#vendorExtensions}}{{^multipartFormData}}.toString(){{/multipartFormData}}{{/vendorExtensions}};
    {{/hasFormParams}}
    {{#bodyParam}}
-            const needsSerialization = (typeof {{paramName}} !== "string") || localVarRequestOptions.headers['Content-Type'] === 'application/json';
+            const contentType = localVarRequestOptions.headers['Content-Type'];
+            const needsSerialization = (typeof {{paramName}} !== "string") && (contentType.match(JSON_MIME_PATTERN) || contentType.match(JSON_VENDOR_MIME_PATTERN));


This could be a breaking change if people were relying on this JSON.stringify with either an empty or non-JSON Content-Type. However, I didn't want to just add an exclusion for application/octet-stream since others (e.g., image/jpeg, etc.) wouldn't want JSON.stringify called either.

therockstorm · 2020-10-06T13:07:58Z

@macjohnny Are you able to take a look at this? Thanks!

therockstorm · 2020-10-07T14:18:50Z

@macjohnny Thanks for the approval. Is this good to merge or did you want me to address your comment?

macjohnny · 2020-10-07T14:22:42Z

It would be great if you would refactor the check into a function

therockstorm · 2020-10-07T16:11:41Z

Looks like a Scala Akka failure in circleci unrelated to my changes. I don't have permission to re-run it.

macjohnny · 2020-10-07T16:49:16Z

@therockstorm I re-started the CI

therockstorm · 2020-10-07T22:17:24Z

All green, @macjohnny!

sgoodrow-zymergen · 2020-10-20T19:38:02Z

const api = new ApiV1({basePath, accessToken});

☝️ wants me to define an isJsonMime function.

Is the default implementation of isJsonMime being shipped as the fallback configuration? I don't think it is, I'm seeing the function as undefined in generated clients when configuring a new API class, and as expected this is now marked as a required field which I don't think was the intention?

Using typescript-axios to test.

macjohnny · 2020-10-21T11:27:03Z

@sgoodrow-zymergen you need to create an instance of Configuration. But I agree, it would be better if the default implementation was separated from the Configuration class and the isJsonMime would be optional in the configuration. Would you like to fix this?

sgoodrow-zymergen · 2020-10-21T16:19:35Z

Ah, shoot, I overlooked that Configuration is a class (thought it was an interface)! My mistake, thanks for the correction.

Will see what time I can scrounge up to factor this down -- no promises but I appreciate the intention. :)

denodaeus · 2021-05-04T18:42:15Z

So, this function is required by the type definitions when utilizing the typescript-axios generator:

"@openapitools/openapi-generator-cli": "^2.2.6",
"generate:client:javascript": "npx @openapitools/openapi-generator-cli generate -g typescript-axios -i ./swagger.json -o ./src",

Is isJsonMime a required function, or does it have some default behavior? It seems really dumb to have to define an implementation for this every time I want to use the generated client.

Here is the error:

src/api/tradeblotter.api.ts:4:34 - error TS2345: Argument of type '{ basePath: string; baseOptions: { withCredentials: boolean; }; }' is not assignable to parameter of type 'Configuration'.
  Property 'isJsonMime' is missing in type '{ basePath: string; baseOptions: { withCredentials: boolean; }; }' but required in type 'Configuration'.

amakhrov · 2021-05-04T19:22:39Z

It looks like the types are messed up. I will be surprised if the generated client code compiles with strictNullChecks at all.

[typescript-axios]: Only serialize JSON request bodies

982f0e9

therockstorm commented Oct 2, 2020

View reviewed changes

therockstorm changed the title ~~Only serialize JSON request bodies~~ [typescript-axios]: Only serialize JSON request bodies Oct 2, 2020

therockstorm changed the title ~~[typescript-axios]: Only serialize JSON request bodies~~ [typescript-axios]: Only call JSON.stringify on JSON request bodies Oct 2, 2020

therockstorm commented Oct 2, 2020

View reviewed changes

macjohnny approved these changes Oct 6, 2020

View reviewed changes

macjohnny added the Client: TypeScript label Oct 6, 2020

macjohnny added this to the 5.0.0 milestone Oct 6, 2020

therockstorm added 2 commits October 7, 2020 09:22

Remove Regex and instead add a method in Configuration class

6e80e8a

Update samples

7b99be4

macjohnny merged commit 8d0053f into OpenAPITools:master Oct 8, 2020

therockstorm deleted the data-1327 branch October 8, 2020 13:22

macjohnny mentioned this pull request Jan 28, 2021

[BUG][typescript-axios] The generated ts file from 5.0.0 cannot be transpiled. #8542

Closed

6 tasks

ahilke mentioned this pull request Feb 4, 2021

[REQ] Honor MIME types specified in body Content-Type header #5218

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[typescript-axios]: Only call `JSON.stringify` on JSON request bodies #7580

[typescript-axios]: Only call `JSON.stringify` on JSON request bodies #7580

therockstorm commented Oct 2, 2020

therockstorm Oct 2, 2020

macjohnny Oct 6, 2020

therockstorm Oct 2, 2020 •

edited

Loading

therockstorm commented Oct 6, 2020

therockstorm commented Oct 7, 2020

macjohnny commented Oct 7, 2020

therockstorm commented Oct 7, 2020 •

edited

Loading

macjohnny commented Oct 7, 2020

therockstorm commented Oct 7, 2020

sgoodrow-zymergen commented Oct 20, 2020 •

edited

Loading

macjohnny commented Oct 21, 2020

sgoodrow-zymergen commented Oct 21, 2020

denodaeus commented May 4, 2021 •

edited

Loading

amakhrov commented May 4, 2021

	/**
	* Check if the given MIME is a JSON MIME.
	* JSON MIME examples:
	* application/json
	* application/json; charset=UTF8
	* APPLICATION/JSON
	* application/vnd.company+json
	* @param mime - MIME (Multipurpose Internet Mail Extensions)
	* @return True if the given MIME is JSON, false otherwise.
	*/
	public isJsonMime(mime: string): boolean {
	const jsonMime: RegExp = new RegExp('^(application\/json\|[^;/ \t]+\/[^;/ \t]+[+]json)[ \t](;.)?$', 'i');
	return mime !== null && (jsonMime.test(mime) \|\| mime.toLowerCase() === 'application/json-patch+json');
	}

[typescript-axios]: Only call JSON.stringify on JSON request bodies #7580

[typescript-axios]: Only call JSON.stringify on JSON request bodies #7580

Conversation

therockstorm commented Oct 2, 2020

PR checklist

therockstorm Oct 2, 2020

Choose a reason for hiding this comment

macjohnny Oct 6, 2020

Choose a reason for hiding this comment

therockstorm Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

therockstorm commented Oct 6, 2020

therockstorm commented Oct 7, 2020

macjohnny commented Oct 7, 2020

therockstorm commented Oct 7, 2020 • edited Loading

macjohnny commented Oct 7, 2020

therockstorm commented Oct 7, 2020

sgoodrow-zymergen commented Oct 20, 2020 • edited Loading

macjohnny commented Oct 21, 2020

sgoodrow-zymergen commented Oct 21, 2020

denodaeus commented May 4, 2021 • edited Loading

amakhrov commented May 4, 2021

[typescript-axios]: Only call `JSON.stringify` on JSON request bodies #7580

[typescript-axios]: Only call `JSON.stringify` on JSON request bodies #7580

therockstorm Oct 2, 2020 •

edited

Loading

therockstorm commented Oct 7, 2020 •

edited

Loading

sgoodrow-zymergen commented Oct 20, 2020 •

edited

Loading

denodaeus commented May 4, 2021 •

edited

Loading