jaydenseric / graphql-upload

Middleware and an Upload scalar to add support for GraphQL multipart requests (file uploads via queries and mutations) to various Node.js GraphQL servers.
https://npm.im/graphql-upload
MIT License
1.42k stars 131 forks source link

Why misorder of operations and map will cause error thrown? #372

Closed Sczlog closed 10 months ago

Sczlog commented 1 year ago

I'm not quitely sure if formdata has order, but some http client implementation, like go http client, use map[string][]string as formdata's data structure which has no order, may cause the order is unreliable on client side.

image

I read source code and found parsing map require operations, but if make a change like this(a tempo workaround, test not updated and may need update), it can whatever operations come first or map come first.

https://github.com/jaydenseric/graphql-upload/commit/4a79f7dd41da691d8c16b49e486f95f0a306129c

ff we get map first, just delay resolve after we get operations, if we get operations first, work like normal.

jaydenseric commented 1 year ago

Multipart HTTP requests definitely support ordering fields in the request; they are streamed one after another in the request body. If certain clients don't allow you to specify the order of fields, then I would consider that an issue with the client that needs to be improved. Even if we relax this graphql-upload implementation of the GraphQL multipart request spec to allow miss ordering the operations field before the map field, that won't solve your problem of the client sending the fields in an unpredictable order because it definitely matters a lot for performance if the file fields are not streamed after operations and map.

Sczlog commented 1 year ago

In this scenario, seems like file is always composing in the last position. (idk why).

And I do some research and w3c form spec said

The parts are sent to the processing agent in the same order the corresponding controls appear in the document stream

So it looks like golang's stdlib didn't implement formdata correctly, try to find some other solution.

jaydenseric commented 10 months ago

Closing because graphql-upload is functioning correctly according to the GraphQL multipart request specification. Any changes to the that specification would have to happen in the spec repo first, then once there is an updated spec, server and client implementations can be updated to conform to the new spec. But I can tell right away, that the ordering of fields is not something likely to ever change in the spec.