99designs / 99designs/gqlgen

MultipartForm transport should not be seekable by default

Đang mở
#2,297 1 bình luận 4 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Go
Star
10.8k
Fork
1.3k
Merge trung bình
2 ngày 36 phút
Pull request đã merge (30 ngày)
26

Mô tả

### Description

Currently, when using file upload, `graphql.Upload` includes the data in a property that is an `io.ReadSeeker`:

https://github.com/99designs/gqlgen/blob/779d7cdd4991e3cf4bf1ecbdea1f02664a56ac8d/graphql/upload.go#L9

Because of that, the `MultipartForm` transport has no choice but to fully process the data from the user:

https://github.com/99designs/gqlgen/blob/779d7cdd4991e3cf4bf1ecbdea1f02664a56ac8d/graphql/handler/transport/http_form.go#L125

Developers here have a choice, controlled by the MaxMemory property: data could either be read fully in memory, or could be stored in a temporary file.

While there are some apps that can benefit by having a seekable stream returned, this is causing a tax on every other app that do _not_ have such a requirement. In fact, many (most?) apps would be totally fine with a "regular" readable stream, that can only be read once (`io.Reader`), and forcing them to accept a seekable stream is ...not nice, adding non-insignificant costs. Storing everything in memory is expensive, of course; while caching on disk is cheaper (not free) but much slower. In both cases, also, consumers need to wait for MultipartForm to complete reading all data before they can start processing it.

Take, for example, an application that allows users to upload large files, say 100MB, 1GB, or more. With the current implementation the app must keep the entire data in memory or on disk (in both cases using quite a bit of not-free resources), then once the client has completed the upload (which depends on their Internet speed), the data can start being processed.

A solution that returned to the consumer a stream, instead, would allow consumers to start processing the data right away (e.g. upload to object storage at the same time as the user is sending the data), and would be "free" on resources.

When MultipartForm exposes only a readable (non-seekable) streams, apps that do not need the ability to go back on the stream can immediately benefit of much improved performance and "zero" costs. Apps that do need seekable streams should still be able to implement that on their own side.

What do you think?

I am happy to propose a PR (I've already toyed around with that). The downside is that it would be backwards-incompatible, and would require users that rely on seekable streams to implement that on their own code instead.

### versions
- `go run github.com/99designs/gqlgen version` 0.17.13
- `go version` 1.18.4

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.