feat: add lazy multipart form streaming#507
Conversation
fredbi
left a comment
There was a problem hiding this comment.
Could only see a few nits, essentially about redundant parts with the existing form options.
Other than that I now fully get that we definitely need a new type with a new interface.
So it looks great. I like the care taken to closing and draining.
one question I couldn't figure out by just reading the code: how would your reader fare if wrapped in a LimitHandler that interrupts the consumption of the body?
I suppose it would just bubble the error, but can't be sure.
A good playground to check how it would operate may be to modify this example there: https://github.com/go-swagger/examples/tree/master/file-server/restapi/operations. That's a minimal example of a file upload server.
So I assume your new Reader would be bound there: https://github.com/go-swagger/examples/blob/869b7a290ed6a55fe1b09e6cdb962cee206b9510/file-server/restapi/operations/uploads/upload_file_parameters.go#L44
and https://github.com/go-swagger/examples/blob/869b7a290ed6a55fe1b09e6cdb962cee206b9510/file-server/restapi/operations/uploads/upload_file_parameters.go#L55
and that the custom handler manipulating the new reader could be demonstrated there https://github.com/go-swagger/examples/blob/869b7a290ed6a55fe1b09e6cdb962cee206b9510/file-server/restapi/configure_file_upload.go#L59
so far looks good. great work
Fred
| request *http.Request | ||
| reader *multipart.Reader | ||
| current *StreamedFile | ||
| maxFiles int |
There was a problem hiding this comment.
// NOTE(fred): could embed config
There was a problem hiding this comment.
Done. MultipartFormStream now embeds multipartFormStreamConfig instead of duplicating its fields.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #507 +/- ##
==========================================
- Coverage 83.43% 83.37% -0.07%
==========================================
Files 64 65 +1
Lines 4521 4715 +194
==========================================
+ Hits 3772 3931 +159
- Misses 581 601 +20
- Partials 168 183 +15 ☔ View full report in Codecov by Harness. |
e38d304 to
5195183
Compare
Add a sequential multipart/form-data reader that exposes file payloads directly from the request body without background goroutines. Define explicit file close, stream drain, and request abort semantics while preserving the existing BindForm behavior. Signed-off-by: Филимоненков Павел Анатольевич <p.filimonenkov@office.st-falcon.ru>
Preserve multipart query precedence, ignore nameless parts, reject multipart/mixed requests, restore a total part limit, and document request body close behavior. Signed-off-by: Филимоненков Павел Анатольевич <p.filimonenkov@office.st-falcon.ru>
5195183 to
678d016
Compare
Signed-off-by: Филимоненков Павел Анатольевич <p.filimonenkov@office.st-falcon.ru>
678d016 to
d820dfd
Compare
Signed-off-by: Филимоненков Павел Анатольевич <p.filimonenkov@office.st-falcon.ru>
@fredbi Thanks, Fred. I’ve pushed a follow-up addressing the review comments. I also added handler-level tests with http.MaxBytesHandler, with the stream’s own body limit disabled. The resulting *http.MaxBytesError is propagated both when reading the file payload and when StreamedFile.Close() drains the unread remainder. The file-server example makes sense as an integration playground. After the runtime API review is settled, I’ll prepare a separate prototype against that example to validate the generated binding, handler ownership, and MaxBytesHandler behavior. Pawel. |
|
I’ve prepared an integration playground based on the file-server example: It includes a black-box httptest.Server/io.Pipe test showing that the handler |
fredbi
left a comment
There was a problem hiding this comment.
All good. Currently the debate has moved to how a handler can use the streamer exposed here and cover all hybrid/mixed cases without banging their head too much. So far excellent implementation.
Just waiting for the example usage to settle before the final validation.
Note: adapting the “untyped API” to benefit from this improvement may be deferred to a future improvement. We’ll focus on the “generated API” first.
Add snapshot accessors for multipart fields and file metadata discovered while advancing the stream. Keep NextFile as the sequential payload API while giving handlers enough state for progress tracking and application-specific validation. Signed-off-by: Филимоненков Павел Анатольевич <p.filimonenkov@office.st-falcon.ru>
|
I’ve pushed the handler-owned version we discussed. Generated binding now only constructs and passes MultipartFormStream. It no longer stores a file, advances the stream, or performs required/multiplicity validation. The handler owns traversal and application-specific validation. The runtime Fields() and Files() accessors expose snapshots of the multipart state discovered so far. The black-box test still verifies that the handler starts consuming the file before the client completes the multipart request body, and now also exercises the discovered state. |
Summary
This PR adds a runtime-level prototype for sequential lazy processing of
multipart/form-data.File payloads are exposed directly from the HTTP request body without buffering
the complete multipart form and without background goroutines.
This is the runtime foundation discussed in:
API and lifecycle
MultipartFormStream.NextFile()advances sequentially to the next file part.request.PostFormandrequest.Formasthey are encountered.
StreamedFile.Close()drains only the current file part.MultipartFormStream.Drain()consumes all remaining parts, collects trailingfields, and closes the request body.
MultipartFormStream.Close()stops multipart processing and closes therequest body without explicitly draining the remaining multipart parts.
MultipartFormStream.Fields()returns a snapshot of multipart fieldsdiscovered so far.
MultipartFormStream.Files()returns a snapshot of file metadata discoveredso far, in wire order.
multiplicity and application-specific validation belong to the handler.
The existing
BindFormbehavior is unchanged.Covered cases
Checks