Feature: handling files from multipart/form-data request
#415
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #415 +/- ##
==========================================
+ Coverage 92.76% 92.80% +0.03%
==========================================
Files 21 22 +1
Lines 3567 3750 +183
==========================================
+ Hits 3309 3480 +171
- Misses 220 226 +6
- Partials 38 44 +6 ☔ View full report in Codecov by Sentry. |
|
@lsdch this looks amazing! I will dig in and give it a thorough review soon, just wanted to say thanks for all the work on this and I like the idea of better file handling & documentation. |
formdata.go
Outdated
| var contentTypeSplitRe = regexp.MustCompile(`,\s*`) | ||
|
|
||
| func NewMimeTypeValidator(encoding *Encoding) MimeTypeValidator { | ||
| return MimeTypeValidator{accept: contentTypeSplitRe.Split(encoding.ContentType, -1)} |
There was a problem hiding this comment.
What the point of using regex? Why not strings.Split or strings.Fileds?
There was a problem hiding this comment.
466d277 Changed it to use strings.Split, I do agree it's better to avoid regex unless necessary.
- form-data -> form - content-type -> contentType
…fallback to `form` struct tag
lsdch
force-pushed
the
multipart-form-files
branch
from
4188450
to
f1a58f1
Compare
WalkthroughThe changes introduce extensive support for handling multipart form data in HTTP requests within the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@danielgtaylor hey, finally took some time to write tests for this feature. Coverage is almost 100%, except for 3 error checks on file I/O that are hard to trigger. API has changed a little, I introduced small convenience features to help working with optional files and content-type. I expect that some of the code could be better optimized, especially the parts using New usage example : type FileData struct {
// Now using huma.FormFile instead of multipart.File
// FormFile is a simple wrapper around multipart.File which provides some convenience features
SomeFile huma.FormFile `form:"some_file" contentType:"image/png" required:"true"`
SomeImage huma.FormFile `form:"some_image" contentType:"image/*"`
SeveralFiles []huma.FormFile `form:"several_files" contentType:"image/png,image/jpeg" required:"true"`
}
type FileHandlerInput struct {
RawBody huma.MultipartFormFiles[FileData]
}
func FileHandler(ctx context.Context, input *FileHandlerInput) (*struct{}, error) {
fileData := input.RawBody.Data()
DoSomeThingWith(fileData.SomeFile)
OrSomethingElseWith(fileData.SeveralFiles)
if fileData.SomeImage.IsSet { // We can now check the presence of optional files
fmt.Printf("Content type: %s", fileData.SomeImage.ContentType) // Actual content type (declared in the form or detected as fallback)
}
}
huma.Register(api,
huma.Operation{
Path: "/handle-files",
Method: http.MethodPost,
OperationID: "Handle files",
}, FileHandler) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- formdata.go (1 hunks)
- huma.go (4 hunks)
- huma_test.go (2 hunks)
Additional Context Used
GitHub Check Runs (1)
codecov/patch success (6)
formdata.go: [warning] 34-34: formdata.go#L34
Added line #L34 was not covered by tests
formdata.go: [warning] 44-44: formdata.go#L44
Added line #L44 was not covered by tests
formdata.go: [warning] 51-51: formdata.go#L51
Added line #L51 was not covered by tests
formdata.go: [warning] 91-91: formdata.go#L91
Added line #L91 was not covered by tests
huma.go: [warning] 641-641: huma.go#L641
Added line #L641 was not covered by tests
huma.go: [warning] 664-664: huma.go#L664
Added line #L664 was not covered by tests
Additional comments not posted (14)
formdata.go (1)
34-34: Increase test coverage for critical error handling paths.The lines handling default MIME types and file opening errors are not covered by tests. Ensuring these are tested will improve the robustness of the feature.
Also applies to: 44-44, 51-51, 91-91
huma.go (3)
629-631: Ensure that the type check forMultipartFormFilesis robust.
641-641: This line was not covered by tests.
664-664: This line was not covered by tests.huma_test.go (10)
14-14: Added import statement forospackage.This import is necessary for file operations in the test cases, particularly for handling multipart file uploads.
739-787: Comprehensive test for multipart file upload handling.This test case effectively validates the handling of multipart form data, including required and optional fields, and checks the OpenAPI spec generation. The use of
assertandrequireensures that the test will halt on the first error, which is appropriate for unit tests.
813-852: Validates error handling for missing required multipart fields.The test correctly checks for HTTP 422 Unprocessable Entity responses when required fields are missing, and it properly uses JSON unmarshalling to verify the error details.
855-867: Properly tests the handling of optional multipart fields.The test ensures that the absence of an optional field does not set the
IsSetflag, which is a crucial check for optional multipart form data handling.
875-909: Checks handling of incorrect cardinality in multipart file uploads.This test case effectively checks for errors when multiple files are uploaded where only one is expected, validating the framework's ability to enforce file upload constraints.
912-943: Validates MIME type enforcement in multipart file uploads.The test case correctly identifies and handles cases where the uploaded file's MIME type does not match the expected type, ensuring robust input validation.
946-962: Ensures default content type handling for multipart file uploads.The test verifies that the default content type "application/octet-stream" is used when no specific content type is provided, which aligns with HTTP standards for binary data.
975-989: Tests wildcard content type handling in multipart file uploads.This test case effectively handles wildcard content types, allowing for flexibility in accepted file types while still enforcing the specified major type.
1003-1015: Properly tests image file upload handling with specific content types.The test ensures that image files are correctly handled and that the content type is validated against the expected types, which is crucial for handling binary data securely.
1035-1047: Tests the system's ability to detect and validate the content type of uploaded images.This test case is crucial for ensuring that the system can correctly identify and validate the content type of uploaded files, which enhances security and data integrity.
| func NewMimeTypeValidator(encoding *Encoding) MimeTypeValidator { | ||
| var mimeTypes = strings.Split(encoding.ContentType, ",") | ||
| for i := range mimeTypes { | ||
| mimeTypes[i] = strings.Trim(mimeTypes[i], " ") | ||
| } | ||
| if len(mimeTypes) == 0 { | ||
| mimeTypes = []string{"application/octet-stream"} | ||
| } | ||
| return MimeTypeValidator{accept: mimeTypes} |
There was a problem hiding this comment.
Consider handling empty ContentType more robustly.
The current implementation defaults to "application/octet-stream" only when no content types are specified. It might be beneficial to handle cases where the content type is provided but empty, ensuring robustness.
- if len(mimeTypes) == 0 {
+ if len(mimeTypes) == 0 || (len(mimeTypes) == 1 && mimeTypes[0] == "") {
mimeTypes = []string{"application/octet-stream"}
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func NewMimeTypeValidator(encoding *Encoding) MimeTypeValidator { | |
| var mimeTypes = strings.Split(encoding.ContentType, ",") | |
| for i := range mimeTypes { | |
| mimeTypes[i] = strings.Trim(mimeTypes[i], " ") | |
| } | |
| if len(mimeTypes) == 0 { | |
| mimeTypes = []string{"application/octet-stream"} | |
| } | |
| return MimeTypeValidator{accept: mimeTypes} | |
| func NewMimeTypeValidator(encoding *Encoding) MimeTypeValidator { | |
| var mimeTypes = strings.Split(encoding.ContentType, ",") | |
| for i := range mimeTypes { | |
| mimeTypes[i] = strings.Trim(mimeTypes[i], " ") | |
| } | |
| if len(mimeTypes) == 0 || (len(mimeTypes) == 1 && mimeTypes[0] == "") { | |
| mimeTypes = []string{"application/octet-stream"} | |
| } | |
| return MimeTypeValidator{accept: mimeTypes} |
| // Validate checks the mime type of the provided file against the expected content type. | ||
| // In the absence of a Content-Type file header, the mime type is detected using [http.DetectContentType]. | ||
| func (v MimeTypeValidator) Validate(fh *multipart.FileHeader, location string) (string, *ErrorDetail) { | ||
| file, err := fh.Open() | ||
| if err != nil { | ||
| return "", &ErrorDetail{Message: "Failed to open file", Location: location} | ||
| } | ||
|
|
||
| mimeType := fh.Header.Get("Content-Type") | ||
| if mimeType == "" { | ||
| var buffer = make([]byte, 1000) | ||
| if _, err := file.Read(buffer); err != nil { | ||
| return "", &ErrorDetail{Message: "Failed to infer file media type", Location: location} | ||
| } | ||
| file.Seek(int64(0), io.SeekStart) | ||
| mimeType = http.DetectContentType(buffer) | ||
| } | ||
| accept := slices.ContainsFunc(v.accept, func(m string) bool { | ||
| if m == "text/plain" || m == "application/octet-stream" { | ||
| return true | ||
| } | ||
| if strings.HasSuffix(m, "/*") && | ||
| strings.HasPrefix(mimeType, strings.TrimRight(m, "*")) { | ||
| return true | ||
| } | ||
| if mimeType == m { | ||
| return true | ||
| } | ||
| return false | ||
| }) | ||
|
|
||
| if accept { | ||
| return mimeType, nil | ||
| } else { | ||
| return mimeType, &ErrorDetail{ | ||
| Message: fmt.Sprintf( | ||
| "Invalid mime type: got %v, expected %v", | ||
| mimeType, strings.Join(v.accept, ","), | ||
| ), | ||
| Location: location, | ||
| Value: mimeType, | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure error handling consistency and improve MIME type validation logic.
The MIME type validation logic could be simplified and made more consistent. Consider using a more structured approach to handle different MIME type categories and improve readability.
- if m == "text/plain" || m == "application/octet-stream" {
+ if strings.HasSuffix(m, "/*") {
+ baseType := strings.TrimRight(m, "/*")
+ if strings.HasPrefix(mimeType, baseType) {
+ return true
+ }
+ } else if mimeType == m {
+ return true
+ }
+ return falseCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // Validate checks the mime type of the provided file against the expected content type. | |
| // In the absence of a Content-Type file header, the mime type is detected using [http.DetectContentType]. | |
| func (v MimeTypeValidator) Validate(fh *multipart.FileHeader, location string) (string, *ErrorDetail) { | |
| file, err := fh.Open() | |
| if err != nil { | |
| return "", &ErrorDetail{Message: "Failed to open file", Location: location} | |
| } | |
| mimeType := fh.Header.Get("Content-Type") | |
| if mimeType == "" { | |
| var buffer = make([]byte, 1000) | |
| if _, err := file.Read(buffer); err != nil { | |
| return "", &ErrorDetail{Message: "Failed to infer file media type", Location: location} | |
| } | |
| file.Seek(int64(0), io.SeekStart) | |
| mimeType = http.DetectContentType(buffer) | |
| } | |
| accept := slices.ContainsFunc(v.accept, func(m string) bool { | |
| if m == "text/plain" || m == "application/octet-stream" { | |
| return true | |
| } | |
| if strings.HasSuffix(m, "/*") && | |
| strings.HasPrefix(mimeType, strings.TrimRight(m, "*")) { | |
| return true | |
| } | |
| if mimeType == m { | |
| return true | |
| } | |
| return false | |
| }) | |
| if accept { | |
| return mimeType, nil | |
| } else { | |
| return mimeType, &ErrorDetail{ | |
| Message: fmt.Sprintf( | |
| "Invalid mime type: got %v, expected %v", | |
| mimeType, strings.Join(v.accept, ","), | |
| ), | |
| Location: location, | |
| Value: mimeType, | |
| } | |
| } | |
| // Validate checks the mime type of the provided file against the expected content type. | |
| // In the absence of a Content-Type file header, the mime type is detected using [http.DetectContentType]. | |
| func (v MimeTypeValidator) Validate(fh *multipart.FileHeader, location string) (string, *ErrorDetail) { | |
| file, err := fh.Open() | |
| if err != nil { | |
| return "", &ErrorDetail{Message: "Failed to open file", Location: location} | |
| } | |
| mimeType := fh.Header.Get("Content-Type") | |
| if mimeType == "" { | |
| var buffer = make([]byte, 1000) | |
| if _, err := file.Read(buffer); err != nil { | |
| return "", &ErrorDetail{Message: "Failed to infer file media type", Location: location} | |
| } | |
| file.Seek(int64(0), io.SeekStart) | |
| mimeType = http.DetectContentType(buffer) | |
| } | |
| accept := slices.ContainsFunc(v.accept, func(m string) bool { | |
| if strings.HasSuffix(m, "/*") { | |
| baseType := strings.TrimRight(m, "/*") | |
| if strings.HasPrefix(mimeType, baseType) { | |
| return true | |
| } | |
| } else if mimeType == m { | |
| return true | |
| } | |
| return false | |
| }) | |
| if accept { | |
| return mimeType, nil | |
| } else { | |
| return mimeType, &ErrorDetail{ | |
| Message: fmt.Sprintf( | |
| "Invalid mime type: got %v, expected %v", | |
| mimeType, strings.Join(v.accept, ","), | |
| ), | |
| Location: location, | |
| Value: mimeType, | |
| } | |
| } |
| func (m *MultipartFormFiles[T]) readSingleFile(key string, opMediaType *MediaType) (FormFile, *ErrorDetail) { | ||
| fileHeaders := m.Form.File[key] | ||
| if len(fileHeaders) == 0 { | ||
| if opMediaType.Schema.requiredMap[key] { | ||
| return FormFile{}, &ErrorDetail{Message: "File required", Location: key} | ||
| } else { | ||
| return FormFile{}, nil | ||
| } | ||
| } else if len(fileHeaders) == 1 { | ||
| validator := NewMimeTypeValidator(opMediaType.Encoding[key]) | ||
| return m.readFile(fileHeaders[0], key, validator) | ||
| } | ||
| return FormFile{}, &ErrorDetail{ | ||
| Message: "Multiple files received but only one was expected", | ||
| Location: key, | ||
| } | ||
| } |
There was a problem hiding this comment.
Clarify error messages for better user understanding.
The error message "Multiple files received but only one was expected" could be enhanced to include the key for which the files were received. This would help in debugging.
- Message: "Multiple files received but only one was expected",
+ Message: fmt.Sprintf("Multiple files received for '%s' but only one was expected", key),Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (m *MultipartFormFiles[T]) readSingleFile(key string, opMediaType *MediaType) (FormFile, *ErrorDetail) { | |
| fileHeaders := m.Form.File[key] | |
| if len(fileHeaders) == 0 { | |
| if opMediaType.Schema.requiredMap[key] { | |
| return FormFile{}, &ErrorDetail{Message: "File required", Location: key} | |
| } else { | |
| return FormFile{}, nil | |
| } | |
| } else if len(fileHeaders) == 1 { | |
| validator := NewMimeTypeValidator(opMediaType.Encoding[key]) | |
| return m.readFile(fileHeaders[0], key, validator) | |
| } | |
| return FormFile{}, &ErrorDetail{ | |
| Message: "Multiple files received but only one was expected", | |
| Location: key, | |
| } | |
| } | |
| func (m *MultipartFormFiles[T]) readSingleFile(key string, opMediaType *MediaType) (FormFile, *ErrorDetail) { | |
| fileHeaders := m.Form.File[key] | |
| if len(fileHeaders) == 0 { | |
| if opMediaType.Schema.requiredMap[key] { | |
| return FormFile{}, &ErrorDetail{Message: "File required", Location: key} | |
| } else { | |
| return FormFile{}, nil | |
| } | |
| } else if len(fileHeaders) == 1 { | |
| validator := NewMimeTypeValidator(opMediaType.Encoding[key]) | |
| return m.readFile(fileHeaders[0], key, validator) | |
| } | |
| return FormFile{}, &ErrorDetail{ | |
| Message: fmt.Sprintf("Multiple files received for '%s' but only one was expected", key), | |
| Location: key, | |
| } | |
| } |
| // Decodes multipart.Form data into *T, returning []*ErrorDetail if any | ||
| // Schema is used to check for validation constraints | ||
| func (m *MultipartFormFiles[T]) Decode(opMediaType *MediaType) []error { | ||
| var ( | ||
| dataType = reflect.TypeOf(m.data).Elem() | ||
| value = reflect.New(dataType) | ||
| errors []error | ||
| ) | ||
| for i := 0; i < dataType.NumField(); i++ { | ||
| field := value.Elem().Field(i) | ||
| structField := dataType.Field(i) | ||
| key := structField.Tag.Get("form") | ||
| if key == "" { | ||
| key = structField.Name | ||
| } | ||
| switch { | ||
| case field.Type() == reflect.TypeOf(FormFile{}): | ||
| file, err := m.readSingleFile(key, opMediaType) | ||
| if err != nil { | ||
| errors = append(errors, err) | ||
| continue | ||
| } | ||
| field.Set(reflect.ValueOf(file)) | ||
| case field.Type() == reflect.TypeOf([]FormFile{}): | ||
| files, errs := m.readMultipleFiles(key, opMediaType) | ||
| if errs != nil { | ||
| errors = slices.Concat(errors, errs) | ||
| continue | ||
| } | ||
| field.Set(reflect.ValueOf(files)) | ||
|
|
||
| default: | ||
| continue | ||
| } | ||
| } | ||
| m.data = value.Interface().(*T) | ||
| return errors | ||
| } |
There was a problem hiding this comment.
Optimize reflection usage for performance.
The use of reflection in the Decode method could be optimized by caching the type and field information if the structure of T does not change frequently. This would reduce the overhead associated with reflection.
// Example: Cache the type information outside of the method if applicable
var cachedType = reflect.TypeOf((*FileData)(nil)).Elem()| @@ -610,6 +610,7 @@ | |||
| } | |||
| rawBodyIndex := -1 | |||
| rawBodyMultipart := false | |||
| rawBodyDecodedMultipart := false | |||
There was a problem hiding this comment.
Consider renaming rawBodyDecodedMultipart to align with Go naming conventions.
- rawBodyDecodedMultipart := false
+ rawBodyDecodedMultipartFlag := falseCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| rawBodyDecodedMultipart := false | |
| rawBodyDecodedMultipartFlag := false |
| if rawBodyMultipart { | ||
| f.Set(reflect.ValueOf(*form)) | ||
| } else { | ||
| f.FieldByName("Form").Set(reflect.ValueOf(form)) | ||
| r := f.Addr(). | ||
| MethodByName("Decode"). | ||
| Call([]reflect.Value{ | ||
| reflect.ValueOf(op.RequestBody.Content["multipart/form-data"]), | ||
| }) | ||
| errs := r[0].Interface().([]error) | ||
| if errs != nil { | ||
| WriteErr(api, ctx, http.StatusUnprocessableEntity, "validation failed", errs...) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider refactoring this block to reduce complexity and improve readability.
- if rawBodyMultipart {
- f.Set(reflect.ValueOf(*form))
- } else {
- f.FieldByName("Form").Set(reflect.ValueOf(form))
- r := f.Addr().
- MethodByName("Decode").
- Call([]reflect.Value{
- reflect.ValueOf(op.RequestBody.Content["multipart/form-data"]),
- })
- errs := r[0].Interface().([]error)
- if errs != nil {
- WriteErr(api, ctx, http.StatusUnprocessableEntity, "validation failed", errs...)
- return
- }
- }
+ handleMultipartForm(f, form, rawBodyMultipart, op, api, ctx)Consider extracting this logic into a separate function handleMultipartForm to handle the multipart form based on the rawBodyMultipart flag.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if rawBodyMultipart { | |
| f.Set(reflect.ValueOf(*form)) | |
| } else { | |
| f.FieldByName("Form").Set(reflect.ValueOf(form)) | |
| r := f.Addr(). | |
| MethodByName("Decode"). | |
| Call([]reflect.Value{ | |
| reflect.ValueOf(op.RequestBody.Content["multipart/form-data"]), | |
| }) | |
| errs := r[0].Interface().([]error) | |
| if errs != nil { | |
| WriteErr(api, ctx, http.StatusUnprocessableEntity, "validation failed", errs...) | |
| return | |
| } | |
| } | |
| if rawBodyMultipart { | |
| f.Set(reflect.ValueOf(*form)) | |
| } else { | |
| f.FieldByName("Form").Set(reflect.ValueOf(form)) | |
| r := f.Addr(). | |
| MethodByName("Decode"). | |
| Call([]reflect.Value{ | |
| reflect.ValueOf(op.RequestBody.Content["multipart/form-data"]), | |
| }) | |
| errs := r[0].Interface().([]error) | |
| if errs != nil { | |
| WriteErr(api, ctx, http.StatusUnprocessableEntity, "validation failed", errs...) | |
| return | |
| } | |
| } | |
| ``` | |
| </details> | |
| <!-- suggestion_end --> | |
| <!-- This is an auto-generated comment by CodeRabbit --> |
|
@lsdch awesome work! I will dig in and do a thorough review ASAP! Thank you. |
This PR aims to improve the handling of
multipart/form-datarequests by:RawBodyismultipart.FormData. The generated schema is more of an example and does not reflect the actual form data structure, which should be specified by the user.MultipartFormFiles, which can be used to typeRawBodyin request inputs and provides logic for decoding and validating files from amultipart/form-datarequest asmultipart.Filevalues, as well as generating the associated schema in the spec. Although this could be extended to support data types other than files, it would require significant refactoring to reuse some of the logic from body/query/path/header parameters decoding, and I was not confident enough to engage into it.There is currently no automated tests for this PR, I would like to first get some feedback before spending more time on this (and still have to get familiar with
humatest).Example usage:
When using
MultipartFormFiles[T], only the fields inTthat implementmultipart.Fileor[]multipart.Fileare considered when generating the spec or resolving a request. The raw form data can be accessed atMultipartFormFiles.Formwhich is amultipart.Form.form-datatag specifies the lookup key to retrieve the encoded file in themultipart.Form. In its absence, the name of the field is used instead.content-typespecifies the acceptable mime types for a file. It is used to document the endpoint in the spec, and validate file format usinghttp.DetectContentTyperequiredmakes the field required. In the case of[]multipart.File, validation checks that at least one file was submitted.Generated operation spec :
{ "/handle-files": { "post": { "operationId": "HandleFiles", "requestBody": { "content": { "multipart/form-data": { "encoding": { "some_file": { "contentType": "image/png" }, "several_files": { "contentType": "image/png,image/jpeg" } }, "schema": { "properties": { "some_file": { "format": "binary", "type": "string" }, "several_files": { "items": { "format": "binary", "type": "string" }, "type": "array" } }, "required": [ "some_file", "several_files" ], "type": "object" } } } } } } }Summary by CodeRabbit
New Features
Tests