🔥 Feature: BodyParser Now Automatically Parses File Fields in Multipart Requests #3316
Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThe changes enhance the multipart binding functionality in the binder package. The Changes
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
The code below works with these changes package main
import (
"fmt"
"mime/multipart"
"github.com/gofiber/fiber/v3"
)
type Person struct {
File1 *multipart.FileHeader `json:"file1" xml:"file1" form:"file1"`
File2 *multipart.FileHeader `json:"file2" xml:"file2" form:"file2"`
File3 *multipart.FileHeader `json:"file3" xml:"file3" form:"file3"`
}
func main() {
app := fiber.New()
app.Post("/", func(c fiber.Ctx) error {
p := new(Person)
if err := c.Bind().Form(p); err != nil {
return c.Status(fiber.StatusBadRequest).SendString(fmt.Sprint(fiber.ErrBadRequest.Error()))
}
fmt.Printf("File1.Name: %s - File2.Name: %s - File3.Name: %s\n", p.File1.Filename, p.File2.Filename, p.File3.Filename)
return c.SendString(fmt.Sprintf("Data: %v", p))
})
app.Listen(":3000")
} |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
binder/form.go (1)
78-89: Optimize field lookup performance.Using
FieldByNameFuncfor each file can be slow for structs with many fields. Consider creating a field map once during initialization.Create a cached field map:
+type FormBinding struct { + EnableSplitting bool + fieldCache map[reflect.Type]map[string]reflect.StructField +} +func (b *FormBinding) getFieldMap(t reflect.Type) map[string]reflect.StructField { + if b.fieldCache == nil { + b.fieldCache = make(map[reflect.Type]map[string]reflect.StructField) + } + + if fieldMap, ok := b.fieldCache[t]; ok { + return fieldMap + } + + fieldMap := make(map[string]reflect.StructField) + for i := 0; i < t.NumField(); i++ { + field := t.Field(i) + formTag := field.Tag.Get("form") + if formTag == "" { + fieldMap[strings.ToLower(field.Name)] = field + } else { + fieldMap[strings.ToLower(strings.Split(formTag, ",")[0])] = field + } + } + + b.fieldCache[t] = fieldMap + return fieldMap +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
binder/form.go(2 hunks)binder/form_test.go(2 hunks)
🔇 Additional comments (6)
binder/form.go (2)
4-6: LGTM! Required imports added.The new imports are necessary for the reflection-based file binding functionality.
67-72: LGTM! Proper pointer handling.The reflection code correctly handles both pointer and non-pointer struct types.
binder/form_test.go (4)
144-196: LGTM! Comprehensive test for file-only binding.The test thoroughly verifies the binding of multiple files to struct fields.
198-256: LGTM! Good coverage of mixed field types.The test effectively verifies binding of both file and string fields in the same request.
258-316: LGTM! Thorough numeric field testing.The test properly validates binding of files with numeric fields, including type conversion.
364-416: LGTM! Well-structured performance benchmark.The benchmark effectively measures performance with mixed field types and includes proper cleanup.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
binder/form.go (2)
52-78: Add documentation and optimize error handling.The
safeSetfunction is well-structured but needs some improvements:Apply this diff to improve the function:
+// safeSet safely sets a reflect.Value field to a given value, handling any panics +// that might occur during the operation. It returns an error if the field cannot +// be set or if the value type is incompatible with the field type. func safeSet(field reflect.Value, value reflect.Value) (err error) { defer func() { if r := recover(); r != nil { switch x := r.(type) { case string: - err = errors.New(x) + err = errors.New(x) case error: err = x default: err = fmt.Errorf("unknown panic during field assignment: %v", r) } } }() if !field.CanSet() { - return fmt.Errorf("field cannot be set") + return errors.New("field cannot be set") } if !value.Type().AssignableTo(field.Type()) { return fmt.Errorf("cannot assign value of type %v to field of type %v", value.Type(), field.Type()) } field.Set(value) return nil }🧰 Tools
🪛 golangci-lint (1.62.2)
53-53: File is not
gofumpt-ed with-extra(gofumpt)
53-53: named return "err" with type "error" found
(nonamedreturns)
68-68: fmt.Errorf can be replaced with errors.New
(perfsprint)
80-145: Reduce complexity by extracting nested logic.The function has high cyclomatic complexity due to nested conditionals. Consider extracting the file binding logic into separate helper functions.
Apply this diff to improve maintainability:
func (b *FormBinding) bindMultipart(req *fasthttp.Request, out any) error { multipartForm, err := req.MultipartForm() if err != nil { return err } data := make(map[string][]string) // Bind form values for key, values := range multipartForm.Value { err = formatBindData(out, data, key, values, b.EnableSplitting, true) if err != nil { return err } } // Check struct type outValue := reflect.ValueOf(out) if outValue.Kind() == reflect.Ptr { outValue = outValue.Elem() } // If it's a struct, process files if outValue.Kind() == reflect.Struct { - // Bind files - for key, fileHeaders := range multipartForm.File { - if len(fileHeaders) > 0 { - field := outValue.FieldByNameFunc(func(s string) bool { - // Check form tag - field, ok := outValue.Type().FieldByName(s) - if !ok { - return false - } - formTag := field.Tag.Get("form") - if formTag == "" { - return strings.EqualFold(s, key) - } - return strings.EqualFold(strings.Split(formTag, ",")[0], key) - }) - - if field.IsValid() { - if field.Type().Kind() == reflect.Slice && field.Type().Elem().AssignableTo(reflect.TypeOf(fileHeaders[0])) { - // Handle multiple files - slice := reflect.MakeSlice(field.Type(), len(fileHeaders), len(fileHeaders)) - for i, fh := range fileHeaders { - if err := safeSet(slice.Index(i), reflect.ValueOf(fh)); err != nil { - return fmt.Errorf("failed to set file at index %d: %w", i, err) - } - } - if err := safeSet(field, slice); err != nil { - return fmt.Errorf("failed to set files slice: %w", err) - } - } else if field.Type().AssignableTo(reflect.TypeOf(fileHeaders[0])) { - // Handle single file - if err := safeSet(field, reflect.ValueOf(fileHeaders[0])); err != nil { - return fmt.Errorf("failed to set single file: %w", err) - } - } - } - } - } + if err := b.bindFiles(outValue, multipartForm.File); err != nil { + return err + } } return parse(b.Name(), out, data) } +func (b *FormBinding) bindFiles(outValue reflect.Value, files map[string][]*multipart.FileHeader) error { + for key, fileHeaders := range files { + if len(fileHeaders) == 0 { + continue + } + + field := b.findMatchingField(outValue, key) + if !field.IsValid() { + continue + } + + if err := b.bindFileField(field, fileHeaders); err != nil { + return err + } + } + return nil +} + +func (b *FormBinding) findMatchingField(outValue reflect.Value, key string) reflect.Value { + return outValue.FieldByNameFunc(func(s string) bool { + field, ok := outValue.Type().FieldByName(s) + if !ok { + return false + } + formTag := field.Tag.Get("form") + if formTag == "" { + return strings.EqualFold(s, key) + } + return strings.EqualFold(strings.Split(formTag, ",")[0], key) + }) +} + +func (b *FormBinding) bindFileField(field reflect.Value, fileHeaders []*multipart.FileHeader) error { + if field.Type().Kind() == reflect.Slice && field.Type().Elem().AssignableTo(reflect.TypeOf(fileHeaders[0])) { + return b.bindMultipleFiles(field, fileHeaders) + } else if field.Type().AssignableTo(reflect.TypeOf(fileHeaders[0])) { + return b.bindSingleFile(field, fileHeaders[0]) + } + return nil +} + +func (b *FormBinding) bindMultipleFiles(field reflect.Value, fileHeaders []*multipart.FileHeader) error { + slice := reflect.MakeSlice(field.Type(), len(fileHeaders), len(fileHeaders)) + for i, fh := range fileHeaders { + if err := safeSet(slice.Index(i), reflect.ValueOf(fh)); err != nil { + return fmt.Errorf("failed to set file at index %d: %w", i, err) + } + } + if err := safeSet(field, slice); err != nil { + return fmt.Errorf("failed to set files slice: %w", err) + } + return nil +} + +func (b *FormBinding) bindSingleFile(field reflect.Value, fileHeader *multipart.FileHeader) error { + if err := safeSet(field, reflect.ValueOf(fileHeader)); err != nil { + return fmt.Errorf("failed to set single file: %w", err) + } + return nil +}🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 135-135: max-control-nesting: control flow nesting exceeds 5
(revive)
binder/form_test.go (2)
144-196: Consider using table-driven tests.The test is well-structured but could be more maintainable using table-driven tests, especially since there are multiple similar test cases.
Consider refactoring to use table-driven tests:
+type fileTestCase struct { + name string + files map[string]string // filename -> content + expected interface{} + validate func(*testing.T, interface{}) +} +func runFileTests(t *testing.T, cases []fileTestCase) { + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + b := &FormBinding{EnableSplitting: true} + + req := fasthttp.AcquireRequest() + t.Cleanup(func() { + fasthttp.ReleaseRequest(req) + }) + + buf := &bytes.Buffer{} + mw := multipart.NewWriter(buf) + + for field, filename := range tc.files { + fw, err := mw.CreateFormFile(field, filename) + require.NoError(t, err) + _, err = fw.Write([]byte("test content")) + require.NoError(t, err) + } + + require.NoError(t, mw.Close()) + + req.Header.SetContentType(mw.FormDataContentType()) + req.SetBody(buf.Bytes()) + + err := b.Bind(req, tc.expected) + require.NoError(t, err) + + tc.validate(t, tc.expected) + }) + } +} func Test_FormBinder_ShouldBindMultipartFormWithOnlyFiles(t *testing.T) { t.Parallel() - // ... existing test code ... + type Document struct { + File1 *multipart.FileHeader `form:"file1"` + File2 *multipart.FileHeader `form:"file2"` + } + + cases := []fileTestCase{ + { + name: "two files", + files: map[string]string{ + "file1": "test1.txt", + "file2": "test2.txt", + }, + expected: &Document{}, + validate: func(t *testing.T, v interface{}) { + doc := v.(*Document) + require.NotNil(t, doc.File1) + require.Equal(t, "test1.txt", doc.File1.Filename) + require.NotNil(t, doc.File2) + require.Equal(t, "test2.txt", doc.File2.Filename) + }, + }, + } + + runFileTests(t, cases) }
413-465: Enhance benchmark with more varied test data.The benchmark could be more comprehensive by testing different scenarios:
Consider adding these variations:
- Different file sizes
- Multiple files
- Various field types
- Edge cases (empty files, maximum sizes)
Example:
func Benchmark_FormBinder_BindMultipartWithMixedTypes(b *testing.B) { b.Run("small_file", func(b *testing.B) { /* existing code */ }) + b.Run("large_file", func(b *testing.B) { + // Test with 1MB file + content := make([]byte, 1024*1024) + // ... setup and run benchmark + }) + b.Run("multiple_files", func(b *testing.B) { + // Test with multiple files + // ... setup and run benchmark + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
binder/form.go(3 hunks)binder/form_test.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
binder/form.go
53-53: File is not gofumpt-ed with -extra
(gofumpt)
53-53: named return "err" with type "error" found
(nonamedreturns)
68-68: fmt.Errorf can be replaced with errors.New
(perfsprint)
[warning] 135-135: max-control-nesting: control flow nesting exceeds 5
(revive)
|
@SadikSunbul what is the difference to these pr what was produced before ? |
|
thank you for your efforts |
|
You're welcome. I hadn't seen #3309. I'm glad the problem was solved. |
This PR's solution parses multipart.FileHeader.@ReneWerner87 Is PR #3309 completely correct and complete? Because there is still an issue: the example added to the documentation in #3309 is not working properly. PR #3309 does not include a test for multipart.FileHeader. 
|
Is content type |
|
think yes it is almost complete, but we will check it out |
@efectn |
Hey, no problem. Thx for testing |
Description
This pull request introduces improvements to the multipart form data binding functionality in the Fiber framework. The changes address issues related to binding complex data structures, including nested fields and file uploads, enhancing the overall usability and flexibility of form handling. Made to be compatible with fiber v3.
Fixes #3286
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md