[typescript-fetch] Fix uploading files #2900

janbuchar · 2019-05-15T15:20:42Z

The current master converts anything that is not FormData to JSON. This behavior causes operations that accept a file to send requests where the payload is an empty JSON object.

This PR adds another branch that lets Blobs pass without modification.

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

auto-labeler · 2019-05-15T15:20:46Z

👍 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.

wing328 · 2019-05-20T10:47:23Z

samples/client/petstore/typescript-fetch/builds/default/apis/PetApi.ts

@@ -23,7 +23,7 @@ import {
 } from '../models';

 export interface AddPetRequest {
-    body: Pet;
+    pet: Pet;


Look like you're using an old version. please pull the latest master and merge into your branch and then update the samples again.

wing328 · 2019-05-20T10:47:42Z

please copy the TS technical committee as stated in the PR template.

…file-upload

macjohnny · 2019-05-20T11:05:35Z

samples/client/petstore/typescript-fetch/builds/with-interfaces/runtime.ts

        .join('&');
 }

-export function mapValues(data: any, fn: (item: any) => any) {


this should not be removed. did you generate the samples with the most recent master merged into your branch?

I'm completely stumped now. I tried merging upstream/master into my branch before your review. After that, I pulled upstream master into my master and merged it into my branch. This got me back to where I was before @wing328 reviewed my changes.

@Teyras are you sure you re-generated the samples after merging origin/master?

macjohnny · 2019-05-20T11:35:20Z

samples/client/petstore/typescript-fetch/builds/es6-target/runtime.ts

        .join('&');
 }

-export function mapValues(data: any, fn: (item: any) => any) {


please run mvn clean before re-generating the samples.
the mapValues function actually is available in your version of

openapi-generator/modules/openapi-generator/src/main/resources/typescript-fetch/runtime.mustache

Lines 207 to 212 in 73ac448

export function mapValues(data: any, fn: (item: any) => any) {

return Object.keys(data).reduce(

(acc, key) => ({ ...acc, [key]: fn(data[key]) }),

{}

);

}

but it is removed in the generated samples, which indicates that the version you use for generating the samples is not the one you want to merge...

Wow. That seems to have un-removed mapValues, but the body parameter was renamed back to pet again. My code seems to be haunted.

the body to pet renaming could be due to general changes, not necessarily related to your code.

macjohnny

LGTM

janbuchar · 2019-05-22T09:24:40Z

@macjohnny is there anything to do before this can be merged?

macjohnny · 2019-05-22T09:32:41Z

the ci tests detected a discrepancy between the generated samples.
https://circleci.com/gh/OpenAPITools/openapi-generator/6662#tests/containers/2

please ensure that

you merge the most recent master into your branch
then run mvn clean
then re-generate the samples
make sure to commit all changed files

macjohnny · 2019-05-22T09:33:54Z

I think the errors in https://travis-ci.org/OpenAPITools/openapi-generator/builds/534779079?utm_source=github_status&utm_medium=notification will also be gone after that

wing328 · 2019-05-22T10:17:08Z

@macjohnny is right

@Teyras let me know if you need help merging the latest master of the official repo into your branch.

…file-upload

macjohnny · 2019-05-22T11:25:52Z

@Teyras the typescript-fetch tests fail because the tests need to be adapted due to changes of the body-parameter name (this is not related to your PR):

openapi-generator/samples/client/petstore/typescript-fetch/tests/default/test/PetApi.ts

Line 19 in bf7ee5e

return api.addPet({ body: fixture }).then(() => {

should be

return api.addPet({ pet: fixture }).then(() => {

and

openapi-generator/samples/client/petstore/typescript-fetch/tests/default/test/PetApi.ts

Line 32 in bf7ee5e

return api.updatePet({ body: result }).then(() => {

should be

return api.updatePet({ pet: result }).then(() => {

could you please push those changes?

macjohnny · 2019-05-22T11:58:11Z

the tests fail due to

 1) PetApi without custom request options should add and delete Pet:
     ReferenceError: Blob is not defined
      at PetApi.BaseAPI.createFetchParams (/home/travis/build/OpenAPITools/openapi-generator/samples/client/petstore/typescript-fetch/builds/with-npm-version/dist/runtime.js:175:81)

I still don't understand why, since Blob is defined here

https://github.com/microsoft/TypeScript/blob/de96b412724979ad089a1e9edecb217bad91725a/lib/lib.dom.d.ts#L2253-L2257

and this should be included in the tsconfig.json

openapi-generator/samples/client/petstore/typescript-fetch/tests/default/tsconfig.json

Lines 12 to 15 in bf7ee5e

macjohnny · 2019-05-22T12:03:32Z

@Teyras any idea how to fix it?

janbuchar · 2019-05-22T12:08:45Z

@macjohnny nope, Blob is a part of the File API so I would expect it to exist, just like FormData exists. What environment is used to run the tests?

janbuchar · 2019-05-22T13:24:17Z

@macjohnny I think the problem is that the tests run in node, which doesn't have the Blob API. We could install a polyfill (https://www.npmjs.com/package/blob), but we'd have to find a way to make it available without importing it.

janbuchar · 2019-05-23T07:31:29Z

@macjohnny I think that the argument that we should avoid BC breaks still holds - the client was fine in node environments, at least until you tried to upload a Blob to an API (which you couldn't anyway).

If we are to support node, we should probably also find a way to upload Buffer instances...

macjohnny · 2019-05-23T07:34:31Z

@Teyras If I am not mistaken the declare var Blob: any; approach should avoid a breaking change.

janbuchar · 2019-05-23T07:36:11Z

@macjohnny Won't it crash on node during the instanceof call because Blob is undefined?

macjohnny · 2019-05-23T07:40:00Z

@macjohnny Won't it crash on node during the instanceof call because Blob is undefined?
you are right. but this could be avoided by adding

if (typeof Blob === 'undefined') const Blob = function(){}
then the instanceof Blob should just return false

janbuchar · 2019-05-23T07:43:37Z

@macjohnny that looks like a sound solution. However, I think we should also add support for Buffer, since node-fetch supports it in request bodies. What do you think?

macjohnny · 2019-05-23T07:44:19Z

adding support for Buffer sounds good

…file-upload

macjohnny · 2019-05-23T13:03:31Z

samples/client/petstore/typescript-fetch/builds/es6-target/apis/PetApi.ts

        const headerParameters: runtime.HTTPHeaders = {};

        if (this.configuration && this.configuration.accessToken) {
            // oauth required
            if (typeof this.configuration.accessToken === 'function') {
-                headerParameters["Authorization"] = this.configuration.accessToken("petstore_auth", ["read:pets"]);
+                headerParameters["Authorization"] = this.configuration.accessToken("petstore_auth", ["write:pets", "read:pets"]);


why does this change?

It would be due to a fix in oauth scope to propagate these values probably.

macjohnny · 2019-05-23T13:26:58Z

modules/openapi-generator/src/main/resources/typescript-fetch/runtime.mustache

@@ -3,6 +3,9 @@

 export const BASE_PATH = "{{{basePath}}}".replace(/\/+$/, "");

+const Blob = Blob !== undefined ? Blob : function() {};


CI fails due to runtime.ts(17,14): error TS2448: Block-scoped variable 'Blob' used before its declaration.
In https://www.typescriptlang.org/play/index.html you can try

const Blob = Blob !== undefined ? Blob : function () { }; function isBl(a: any) { return a instanceof Blob; }

which does not work.
However, this works:

if (typeof Blob === 'undefined') { const Blob = function () { }; } function isBl(a: any) { return a instanceof Blob; }

Hey, seeing isBl gave me an idea - we could make a function that checks if the argument is a Blob and also returns false when Blob is undefined. I think it would be cleaner than conditionally defining consts.

sounds good

macjohnny · 2019-05-24T09:24:21Z

modules/openapi-generator/src/main/resources/typescript-fetch/runtime.mustache

@@ -3,6 +3,9 @@

 export const BASE_PATH = "{{{basePath}}}".replace(/\/+$/, "");

+const isBlob = (value: any) => Blob !== undefined && value instanceof Blob;


Suggested change

const isBlob = (value: any) => Blob !== undefined && value instanceof Blob;

const isBlob = (value: any) => typeof Blob !== 'undefined' && value instanceof Blob;

since Blob is not necessarily a declared variable

see e.g. https://stackoverflow.com/a/4725697

macjohnny · 2019-05-24T10:49:28Z

the samples seem to have been generated with a different version. it is strange that in each commit e.g. the body: Pet and pet: Pet change back and forth. @Teyras are you using the same very command each time you generate the samples? are you sure you always run mvn clean before generating the samples?

macjohnny · 2019-05-24T10:59:20Z

modules/openapi-generator/src/main/resources/typescript-fetch/runtime.mustache

@@ -3,6 +3,9 @@

 export const BASE_PATH = "{{{basePath}}}".replace(/\/+$/, "");

+const isBlob = (value: any) => typeof Blob !== 'undefined' && value instanceof Blob;
+const isBuffer = (value: any) => typeof Buffer !== 'undefined' && value instanceof Buffer;


maybe it is easier to only support Blob, since Buffer does not exist in the dom typings https://github.com/Microsoft/TypeScript/blob/master/lib/lib.dom.d.ts
or we need to add typings which include Buffer

It should be enough to install @types/node, which seems pretty sensible.

but I imagine a user wouldn't want to install @types/node for a web project, since the node API won't be available in the web...

I don't think a user would care much... create-react-app for typescript installs @types/node too.

janbuchar · 2019-05-27T06:56:14Z

@macjohnny It is possible I occassionally swap /bin/openapi3/typescript-fetch-petstore-all.sh and /bin/typescript-fetch-petstore-all.sh

janbuchar · 2019-05-29T08:05:21Z

@macjohnny could we merge this if I add @types/node or do you think that removing Buffer support is better? Also, does the order of sample generation command I mentioned in the previous command matter?

macjohnny · 2019-05-29T08:12:02Z

@Teyras I think removing buffer support is better for the time being, as it makes this PR 100% backwards compatible.
Concerning the sample generation, I would use bin/typescript-fetch-petstore-all.sh, as this is the script that also the CI uses to check whether the samples are up to date (see

openapi-generator/bin/utils/ensure-up-to-date

Line 42 in f93e75b

"./bin/typescript-fetch-petstore-all.sh"

)

…file-upload

samples/client/petstore/typescript-fetch/tests/default/test/PetApi.ts

…tApi.ts Co-Authored-By: Esteban Marin <[email protected]>

macjohnny · 2019-05-29T11:31:17Z

@wing328 can you re-start the circle-ci tests?

wing328 · 2019-05-31T02:01:21Z

All tests passed 👍

wing328 · 2019-06-03T14:07:06Z

@Teyras thanks again for your contribution, which has been included in the v4.0.1 release (https://twitter.com/oas_generator/status/1135534738658062336)

* 4.1.x: (56 commits) sync master Update compatibility table Ruby client: escape path parameters (OpenAPITools#3039) [gradle plugin] Release 4.0.1 fixes (OpenAPITools#3051) Update version to 4.0.2-SNAPSHOT (OpenAPITools#3047) Map number to double time since float is also parsed as double in Qt5 C++ (OpenAPITools#3046) Prepare 4.0.1 release (OpenAPITools#3041) [gradle] Reworking publishing pipeline (OpenAPITools#2886) [typescript-fetch] Fix uploading files (OpenAPITools#2900) Resolves OpenAPITools#2962 - Add properties config to Maven parameters (OpenAPITools#2963) fix(golang): Check error of xml Encode (OpenAPITools#3027) [C++][Restbed] Add handler callback methods (OpenAPITools#2911) Remove null checks for C# value types (OpenAPITools#2933) [python-server] Support python 3.7 for all server-generators (OpenAPITools#2884) Use golang's provided method names (gin) (OpenAPITools#2983) [python] Adding constructor parameters to Configuration and improving documentation (OpenAPITools#3002) Add new option to maven plugin's readme (OpenAPITools#3025) Engine param in maven plugin. (OpenAPITools#2881) Added support for patterns on model properties (OpenAPITools#2948) [csharp] Make API response headers dictionary case insensitive (OpenAPITools#2998) ...

[typescript-fetch] Fix uploading files

e6b745c

auto-labeler bot added the Client: TypeScript label May 15, 2019

Check for Blob instead of File

8bdc162

wing328 reviewed May 20, 2019

View reviewed changes

janbuchar added 2 commits May 20, 2019 12:54

Merge remote-tracking branch 'upstream/master' into typescript-fetch-…

4a6b00e

…file-upload

Update samples

f621eaa

wing328 added the Issue: Bug label May 20, 2019

wing328 added this to the 4.0.1 milestone May 20, 2019

macjohnny reviewed May 20, 2019

View reviewed changes

janbuchar added 3 commits May 20, 2019 13:11

Merge branch 'master' into typescript-fetch-file-upload

412217b

Update samples

04003ee

Update samples

73ac448

macjohnny reviewed May 20, 2019

View reviewed changes

Update samples

0d1cfc8

macjohnny approved these changes May 20, 2019

View reviewed changes

janbuchar added 3 commits May 22, 2019 12:34

Merge remote-tracking branch 'upstream/master' into typescript-fetch-…

d73aff5

…file-upload

Regenerate samples

df125e6

Bug

bf7ee5e

Manually fix samples

d8514aa

janbuchar added 2 commits May 23, 2019 13:57

Merge remote-tracking branch 'upstream/master' into typescript-fetch-…

b8f470e

…file-upload

Implement support for Buffer and Blob in a backwards-compatible way

68cd6dc

macjohnny reviewed May 23, 2019

View reviewed changes

macjohnny requested changes May 23, 2019

View reviewed changes

Rework how blob and buffer instance checking works

915868a

macjohnny requested changes May 24, 2019

View reviewed changes

janbuchar added 2 commits May 24, 2019 11:29

Check for Blob/Buffer existence properly

a1a3a6d

Avoid using Buffer and Blob in type declarations

f61c1c2

macjohnny reviewed May 24, 2019

View reviewed changes

janbuchar added 2 commits May 29, 2019 12:21

Merge remote-tracking branch 'upstream/master' into typescript-fetch-…

016170d

…file-upload

Remove Buffer support

8adcb8e

macjohnny requested changes May 29, 2019

View reviewed changes

samples/client/petstore/typescript-fetch/tests/default/test/PetApi.ts Outdated Show resolved Hide resolved

samples/client/petstore/typescript-fetch/tests/default/test/PetApi.ts Outdated Show resolved Hide resolved

janbuchar and others added 2 commits May 29, 2019 12:50

Update samples/client/petstore/typescript-fetch/tests/default/test/Pe…

f5c1ed2

…tApi.ts Co-Authored-By: Esteban Marin <[email protected]>

Update samples/client/petstore/typescript-fetch/tests/default/test/Pe…

2ea96f7

…tApi.ts Co-Authored-By: Esteban Marin <[email protected]>

macjohnny approved these changes May 31, 2019

View reviewed changes

wing328 merged commit c509d98 into OpenAPITools:master May 31, 2019

	export function mapValues(data: any, fn: (item: any) => any) {
	return Object.keys(data).reduce(
	(acc, key) => ({ ...acc, [key]: fn(data[key]) }),
	{}
	);
	}

		@@ -3,6 +3,9 @@

		export const BASE_PATH = "{{{basePath}}}".replace(/\/+$/, "");

		const Blob = Blob !== undefined ? Blob : function() {};

		@@ -3,6 +3,9 @@

		export const BASE_PATH = "{{{basePath}}}".replace(/\/+$/, "");

		const isBlob = (value: any) => Blob !== undefined && value instanceof Blob;

	const isBlob = (value: any) => Blob !== undefined && value instanceof Blob;
	const isBlob = (value: any) => typeof Blob !== 'undefined' && value instanceof Blob;

[typescript-fetch] Fix uploading files #2900

[typescript-fetch] Fix uploading files #2900

Conversation

janbuchar commented May 15, 2019 • edited Loading

auto-labeler bot commented May 15, 2019

Choose a reason for hiding this comment

wing328 commented May 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macjohnny left a comment

Choose a reason for hiding this comment

janbuchar commented May 22, 2019

macjohnny commented May 22, 2019

macjohnny commented May 22, 2019

wing328 commented May 22, 2019

macjohnny commented May 22, 2019

macjohnny commented May 22, 2019 • edited Loading

macjohnny commented May 22, 2019

janbuchar commented May 22, 2019

janbuchar commented May 22, 2019

janbuchar commented May 23, 2019

macjohnny commented May 23, 2019

janbuchar commented May 23, 2019

macjohnny commented May 23, 2019 • edited Loading

janbuchar commented May 23, 2019

macjohnny commented May 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macjohnny commented May 24, 2019

macjohnny May 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janbuchar commented May 27, 2019

janbuchar commented May 29, 2019

macjohnny commented May 29, 2019

macjohnny commented May 29, 2019

wing328 commented May 31, 2019

wing328 commented Jun 3, 2019

janbuchar commented May 15, 2019 •

edited

Loading

macjohnny commented May 22, 2019 •

edited

Loading

macjohnny commented May 23, 2019 •

edited

Loading

macjohnny May 24, 2019 •

edited

Loading