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

Use MultipartReader to parse file uploads #2135

Merged
merged 1 commit into from
Apr 29, 2022

Conversation

emersion
Copy link
Contributor

Use a streaming MultipartReader to parse requests with file
uploads. The GraphQL multipart request specification guarantees
that the operations and map form fields will come first.

There are two reasons motivating this change:

  • This allows for file uploads without specifying a specific
    filename.
  • This avoids unnecessary copies for requests with more than one
    file. Go's ParseForm already copies the request's body into
    memory or on disk. We were also doing this manually as a second
    step.

I have:

  • (N/A) Added tests covering the bug / feature (see testing)
  • (N/A) Updated any relevant documentation (see docs)

Use a streaming MultipartReader to parse requests with file
uploads. The GraphQL multipart request specification guarantees
that the operations and map form fields will come first.

There are two reasons motivating this change:

- This allows for file uploads without specifying a specific
  filename.
- This avoids unnecessary copies for requests with more than one
  file. Go's ParseForm already copies the request's body into
  memory or on disk. We were also doing this manually as a second
  step.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 74.682% when pulling 001069f on emersion:multipart-reader into 9250f9a on 99designs:master.

@robertmarsal
Copy link
Contributor

FYI after this PR the underlying type of File in graphql.Upload changed from multipart.sectionReadCloser to *transport.bytesReader. This is not a problem because the new type still implements the io.Reader type however the old type was more convenient because it implemented io.Seeker so the file could be sent directly to S3 for example.

Leaving the comment for anyone wondering why their tests are suddenly failing.

@emersion
Copy link
Contributor Author

Yeah, because the HTTP request body cannot be seeked. To restore the previous behavior, you can copy to a temporary file.

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 this pull request may close these issues.

4 participants