search

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

fix: move parser's resolve() from `field` to `finish` event #385

Closed heyask closed 3 months ago

heyask commented 3 months ago

Fix #384

We're encountering a situation where only the first GraphQLUpload argument in a GraphQL request gets processed. Subsequent arguments result in empty uploads. This behavior seems to be linked to the busboy parser's field event being triggered multiple times, corresponding to the number of GraphQLUpload arguments present.

To address this, we've moved the promise resolution to the finish event. This ensures that the promise only resolves after all files have been processed, successfully capturing all uploads in the request.

jaydenseric commented 3 months ago

This change is not correct; what it does is causes the entire multipart request stream to finish uploading before resolving the operations. By design the operations is supposed to resolve before the files start streaming in, so that way GraphQL resolvers can decide how to handle the file upload streams as they come in.

You can see a graphic showing how this approach makes the overall timings for a GraphQL response better here:

https://github.com/jaydenseric/graphql-multipart-request-spec/blob/master/readme.md

As per https://github.com/jaydenseric/graphql-upload/issues/384#issuecomment-2041337054 , make sure that you are consuming readable streams for every upload in resolvers.