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

File Uploads broken #2606

Closed
rxst opened this issue Mar 24, 2023 · 8 comments · Fixed by #2645
Closed

File Uploads broken #2606

rxst opened this issue Mar 24, 2023 · 8 comments · Fixed by #2645

Comments

@rxst
Copy link

rxst commented Mar 24, 2023

Describe the bug

After uploading a file by standard example file is damaged and saved as broken.

Your Example Website or App

https://github.com/dotansimha/graphql-yoga/tree/0425ee1c4d9a0ec7aad972eb96d50c5328c5850c/examples/file-upload

Steps to Reproduce the Bug or Issue

  1. Clone example from repo
  2. Clear the npm cache
  3. Install dependencies
  4. Run the example by npm run start
  5. Send request by the postman (use not broken file).
  6. Try to open the saved file.

Expected behavior

File opened as the original file

Screenshots or Videos

Screenshot 2023-03-24 at 08 11 26

Platform

  • OS: [macOS, Linux]
  • NodeJS: [e.g. 16.18.1]
  • @graphql-yoga/* version(s): [e.g. 3.7.3]

Additional context

I try to update and downgrade the node version. It's not dependent on it. Maybe it's something about inner coding. So please update the sample or fix it.

@524c
Copy link

524c commented Apr 5, 2023

I was already going crazy looking for problems in my code. Glad I'm not alone with this 🤯🙌

With small images (20k) I have no problems, but try with bigger images, for example 100k and then the save file will be corrupted.

I did some digging, downgrading the current version (3.8.0) to version 3.5.1 (which works). From version 3.6.0 onwards, this is where the problem appears.

Examples images for tests:
elmo1
elmo2

@ardatan
Copy link
Collaborator

ardatan commented Apr 5, 2023

Thank you for sharing details @524c !
I'm able to reproduce it. I think it is incompatibility between file.stream and Node's stream.
So I created this PR to update the example.
#2645

@524c
Copy link

524c commented Apr 5, 2023

Thank you for sharing details @524c ! I'm able to reproduce it. I think it is incompatibility between file.stream and Node's stream. So I created this PR to update the example. #2645

Great, thank you!

@oddeirik
Copy link

We just hit this issue ourselves. In our case the file size is the same, but the content is assembled in incorrect order (tested it with a text file for easier comparison).

Is there a "built-in" way of doing streaming from a file input to a Node stream? We're using @azure/storage-blob for storing file uploads in a storage blob container and it would be nice to just stream the content more or less directly without having to read the full file content first or implement a stream reader ourselves.

@ardatan
Copy link
Collaborator

ardatan commented Apr 18, 2023

The given object implements File object that you can find the whole documentation on MDN as we already linked in the documentation.
File objects also implement Blob which also can be found on MDN.
You can see how it is easy to read the file content here;
https://developer.mozilla.org/en-US/docs/Web/API/Blob#instance_methods

If you can share more details, failing PR or a sandbox, we can help you better.

@oddeirik
Copy link

Well, I'm not entirely sure what triggered this change, but here's essentially what we've been doing for file uploads:

// input is from the resolver
// input.file: Scalars['File'];
// ...
const readableStream = Readable.from(input.file.stream());
const blob = this.getContainer(container).getBlockBlobClient(name);
await blob.uploadStream(stream);

The Readable.from call is because @azure/storage-blob's uploadStream requires a ReadableStream. This worked fine in version 3.5.1 and earlier. However, after upgrading to graphql-yoga version 3.7.3 the file content of file uploads is now jumbled.

@Arno500
Copy link

Arno500 commented Sep 2, 2023

I can confirm there is still an issue there. Moving to file.arrayBuffer() doesn't really fix anything, you loose the ability to upload any big file to a backend without buffering the whole content of the file (which is often not desirable because of the memory consumption and processing time).
I noted that even if I want to use the file.stream(), the whole file get buffered before my mutation is even called, and as reported previously, the stream is corrupted making it impossible to actually use.
I also confirm the bug is not present in 3.5.1, but 4.x.x is definitely affected.
Also, the stream is perfectly usable (still in 3.5.1) in Node 20 using a Readable.from(), without any TS error.
Would it be possible to open this one again? @ardatan

Thank you a lot!

@ardatan
Copy link
Collaborator

ardatan commented Sep 2, 2023

Let's create a new issue with a reproduction or a failing test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants