bind: add support for multipart file binding #3309
Conversation
efectn
requested review from
gaby,
sixcolors and
ReneWerner87
and removed request for
a team
WalkthroughThe changes extend the binder’s functionality to support multipart file uploads. The Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant F as FormBinding.bindMultipart
participant P as parse/parseToStruct
participant D as Decoder
C->>F: Submit multipart/form-data with files
F->>F: Iterate over form fields and file headers
F->>P: Call parse with regular data and file headers
P->>D: Decode both form fields and file uploads
D-->>P: Return bound data structure
P-->>F: Provide parsed result
F-->>C: Respond with binding result
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
binder/form_test.go (1)
101-101: Field alignment issue (88 pointer bytes → 80).
Reordering fields for better alignment may reduce memory usage:-type User struct { - Name string `form:"name"` - Names []string `form:"names"` - Posts []Post `form:"posts"` - Age int `form:"age"` - Avatar *multipart.FileHeader `form:"avatar"` - Avatars []*multipart.FileHeader `form:"avatars"` +type User struct { + Age int `form:"age"` + Name string `form:"name"` + Names []string `form:"names"` + Posts []Post `form:"posts"` + Avatar *multipart.FileHeader `form:"avatar"` + Avatars []*multipart.FileHeader `form:"avatars"` }🧰 Tools
🪛 golangci-lint (1.62.2)
101-101: fieldalignment: struct with 88 pointer bytes could be 80
(govet)
🪛 GitHub Check: lint
[failure] 101-101:
fieldalignment: struct with 88 pointer bytes could be 80 (govet)🪛 GitHub Actions: golangci-lint
[error] 101-101: fieldalignment: struct with 88 pointer bytes could be 80 (govet)
docs/api/bind.md (1)
154-155: Remove extra blank line.-🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
155-155: Multiple consecutive blank lines
Expected: 1; Actual: 2(MD012, no-multiple-blanks)
🪛 GitHub Check: markdownlint
[failure] 155-155: Multiple consecutive blank lines
docs/api/bind.md:155 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md012.md🪛 GitHub Actions: markdownlint
[error] 155-155: Multiple consecutive blank lines [Expected: 1; Actual: 2]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (5)
binder/form.go(2 hunks)binder/form_test.go(5 hunks)binder/mapping.go(5 hunks)docs/api/bind.md(1 hunks)docs/whats_new.md(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: golangci-lint
binder/form_test.go
[error] 101-101: fieldalignment: struct with 88 pointer bytes could be 80 (govet)
🪛 LanguageTool
docs/api/bind.md
[uncategorized] ~124-~124: Possible missing article found.
Context: ...t:3000 ``` :::info If you need to bind multipart file, you can use `*multipart.FileHeade...
(AI_HYDRA_LEO_MISSING_A)
[style] ~124-~124: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...eHeader, *[]*multipart.FileHeaderor[]*multipart.FileHeader` as a field type. ::: ```go title="Exa...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/whats_new.md
[style] ~490-~490: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...eader, *[]*multipart.FileHeader, and []*multipart.FileHeader` field types.
Examp...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.17.2)
docs/api/bind.md
155-155: Multiple consecutive blank lines
Expected: 1; Actual: 2
(MD012, no-multiple-blanks)
🪛 GitHub Check: markdownlint
docs/api/bind.md
[failure] 155-155: Multiple consecutive blank lines
docs/api/bind.md:155 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md012.md
🪛 GitHub Actions: markdownlint
docs/api/bind.md
[error] 155-155: Multiple consecutive blank lines [Expected: 1; Actual: 2]
🪛 golangci-lint (1.62.2)
binder/mapping.go
265-265: Error return value is not checked
(errcheck)
265-265: type assertion must be checked
(forcetypeassert)
268-268: Error return value is not checked
(errcheck)
268-268: type assertion must be checked
(forcetypeassert)
272-272: type assertion must be checked
(forcetypeassert)
272-272: Error return value is not checked
(errcheck)
🪛 GitHub Check: lint
binder/mapping.go
[failure] 265-265:
Error return value is not checked (errcheck)
[failure] 265-265:
type assertion must be checked (forcetypeassert)
[failure] 268-268:
Error return value is not checked (errcheck)
[failure] 268-268:
type assertion must be checked (forcetypeassert)
[failure] 272-272:
type assertion must be checked (forcetypeassert)
[failure] 272-272:
Error return value is not checked (errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.23.x, macos-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (18)
binder/mapping.go (7)
6-6: Import addition looks good.
No issues with adding"mime/multipart"for handling file headers.
73-73: Good approach addingfiles ...map[string][]*multipart.FileHeader.
This parameter extension cleanly supports file data alongside string data.
87-87: Consistent call toparseToStruct.
Passing the additionalfilesparameter ensures that file data is handled in one place.
91-91: Extended signature forparseToStruct.
This aligns well with the newfilesparameter, allowing the schema decoder to handle file headers.
99-99: Error handling is correct.
Decoding logic properly wraps and returns errors.
254-254: Signature refactoring tofunc formatBindData[T, K any].
Expanding generic types makes sense for differentiating between text and file-based data.
268-268: Repeat of unchecked type assertion.
Same concern applies when passingvaltoassignBindData.🧰 Tools
🪛 golangci-lint (1.62.2)
268-268: Error return value is not checked
(errcheck)
268-268: type assertion must be checked
(forcetypeassert)
🪛 GitHub Check: lint
[failure] 268-268:
Error return value is not checked (errcheck)
[failure] 268-268:
type assertion must be checked (forcetypeassert)binder/form.go (2)
4-5: Importingmime/multipartis correct for file-bound data.
No issues here.
64-72: Efficient approach to separate file data from form data.
Collecting file headers intofilesthen passing them toparseis clean. Consider large file scenarios, but for typical usage, this is fine.binder/form_test.go (5)
5-5: Importingiois appropriate for reading uploaded files.
No concerns here.
102-107: Struct extension for file fields.
Allowing single and multiple file headers covers common use cases.
124-141: Writing multiple files for multipart testing.
This is a thorough test setup for verifying single and multiple file fields.
151-151: Binding call ensures robust end-to-end coverage.
Good to see the real request object used in tests.
164-195: Detailed validation of file attachments.
Verifying filename, content type, and file contents is essential. This test comprehensively checks the new functionality.docs/api/bind.md (3)
123-125: LGTM! Clear and concise info block.The info block effectively communicates the supported field types for multipart file binding.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~124-~124: Possible missing article found.
Context: ...t:3000 ``` :::info If you need to bind multipart file, you can use `*multipart.FileHeade...(AI_HYDRA_LEO_MISSING_A)
[style] ~124-~124: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...eHeader,*[]*multipart.FileHeaderor[]*multipart.FileHeader` as a field type. ::: ```go title="Exa...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
127-147: LGTM! Well-structured example code.The example code clearly demonstrates how to:
- Define a struct with a file field
- Handle file binding in a route handler
- Access the bound file properties
151-153: LGTM! Helpful curl example.The curl command provides a clear example of how to test the file binding functionality.
docs/whats_new.md (1)
490-490: LGTM! Clear feature documentation.The entry clearly documents the new multipart file binding support and lists all supported field types.
🧰 Tools
🪛 LanguageTool
[style] ~490-~490: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...eader,*[]*multipart.FileHeader, and[]*multipart.FileHeader` field types.Examp...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3309 +/- ##
==========================================
- Coverage 84.15% 84.12% -0.04%
==========================================
Files 116 116
Lines 11551 11572 +21
==========================================
+ Hits 9721 9735 +14
- Misses 1399 1403 +4
- Partials 431 434 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
efectn
force-pushed
the
multipart-support
branch
from
c732780 to
a420917
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: a420917 | Previous: b0bc32b | Ratio |
|---|---|---|---|
BenchmarkAppendMsgitem-4_middleware_csrf - MB/s |
3186.28 MB/s |
1597.59 MB/s |
1.99 |
BenchmarkAppendMsgstorageManager - MB/s |
3205.63 MB/s |
1583.2 MB/s |
2.02 |
BenchmarkAppendMsgdata - MB/s |
3203.57 MB/s |
1599.35 MB/s |
2.00 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
binder/form_test.go (2)
124-141: Add error handling for file cleanup.The test creates temporary files but doesn't ensure they're properly cleaned up after the test, which could lead to resource leaks.
Add cleanup for the file handles:
writer, err := mw.CreateFormFile("avatar", "avatar.txt") require.NoError(t, err) +t.Cleanup(func() { + if f, ok := writer.(io.Closer); ok { + _ = f.Close() + } +})
164-194: Add test cases for edge cases.The test covers the happy path but lacks tests for important edge cases.
Consider adding test cases for:
- Empty files
- Large files
- Invalid file types
- Missing required fields
- Maximum file size limits
binder/mapping_test.go (1)
182-235: Enhance test coverage for edge cases.The test suite would benefit from additional test cases.
Consider adding test cases for:
- Empty values
- Nil values
- Invalid types
- Boundary conditions
- Concurrent access scenarios
🧰 Tools
🪛 golangci-lint (1.62.2)
233-233: expected-actual: need to reverse actual and expected values
(testifylint)
🪛 GitHub Check: lint
[failure] 233-233:
expected-actual: need to reverse actual and expected values (testifylint)🪛 GitHub Actions: golangci-lint
[error] 233-233: expected-actual: need to reverse actual and expected values (testifylint)
binder/mapping.go (1)
254-293: Consider pre-allocating slices for better performance.When appending to slices in a loop, pre-allocation can improve performance.
Consider pre-allocating the slice when handling file headers:
case []*multipart.FileHeader: + if len(v) > 0 { + if data[key] == nil { + data[key] = make([]T, 0, len(v)) + } + } for _, val := range v { valT, ok := any(val).(T)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 274-275: binder/mapping.go#L274-L275
Added lines #L274 - L275 were not covered by tests
[warning] 284-285: binder/mapping.go#L284-L285
Added lines #L284 - L285 were not covered by testsdocs/api/bind.md (2)
123-125: Improve info block readability.The current format could be more readable.
Consider restructuring as:
-If you need to bind multipart file, you can use `*multipart.FileHeader`, `*[]*multipart.FileHeader` or `[]*multipart.FileHeader` as a field type. +For multipart file binding, the following field types are supported: +- `*multipart.FileHeader` (single file) +- `*[]*multipart.FileHeader` or `[]*multipart.FileHeader` (multiple files)🧰 Tools
🪛 LanguageTool
[style] ~124-~124: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...eHeader,*[]*multipart.FileHeaderor[]*multipart.FileHeader` as a field type. ::: ```go title="Exa...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
151-153: Enhance curl example with more context.The curl example could be more descriptive.
Consider adding:
- A comment explaining the expected outcome
- Example file content
- Response handling
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
binder/form_test.go(5 hunks)binder/mapping.go(5 hunks)binder/mapping_test.go(2 hunks)docs/api/bind.md(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
binder/mapping_test.go
233-233: expected-actual: need to reverse actual and expected values
(testifylint)
🪛 GitHub Check: lint
binder/mapping_test.go
[failure] 233-233:
expected-actual: need to reverse actual and expected values (testifylint)
🪛 GitHub Actions: golangci-lint
binder/mapping_test.go
[error] 233-233: expected-actual: need to reverse actual and expected values (testifylint)
🪛 GitHub Check: codecov/patch
binder/mapping.go
[warning] 274-275: binder/mapping.go#L274-L275
Added lines #L274 - L275 were not covered by tests
[warning] 284-285: binder/mapping.go#L284-L285
Added lines #L284 - L285 were not covered by tests
🪛 LanguageTool
docs/api/bind.md
[style] ~124-~124: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...eHeader, *[]*multipart.FileHeaderor[]*multipart.FileHeader` as a field type. ::: ```go title="Exa...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: repeated
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Compare
🔇 Additional comments (1)
binder/mapping.go (1)
265-270: LGTM! Good type safety improvements.The added type assertions with proper error handling improve the code's robustness.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
binder/mapping_test.go (1)
233-233:⚠️ Potential issueFix assertion order in test.
The assertion order is incorrect according to the
testifylinttool.-require.Equal(t, "unsupported value type: string", err.Error()) +require.Equal(t, err.Error(), "unsupported value type: string")
🧹 Nitpick comments (5)
binder/mapping_test.go (5)
183-193: Use require assertions consistently.Replace manual error checking and assertions with
requirepackage for consistency and better error messages.- if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if len(data["name"]) != 1 || data["name"][0] != "John" { - t.Fatalf("expected data[\"name\"] = [John], got %v", data["name"]) - } + require.NoError(t, err) + require.Len(t, data["name"], 1) + require.Equal(t, "John", data["name"][0])
195-202: Use require assertions consistently.Replace manual error checking with
requirepackage for consistency and better error messages.- if err == nil { - t.Fatal("expected an error, got nil") - } + require.Error(t, err)
204-211: Use require assertions consistently.Replace manual error checking with
requirepackage for consistency and better error messages.- if err == nil { - t.Fatal("expected an error, got nil") - } + require.Error(t, err)
213-227: Use require assertions consistently.Replace manual error checking and assertions with
requirepackage for consistency and better error messages.- if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if len(data["files"]) != 2 { - t.Fatalf("expected 2 files, got %d", len(data["files"])) - } + require.NoError(t, err) + require.Len(t, data["files"], 2)
247-252: Improve test case to match real-world usage.The test case for disabled splitting uses a slice as the output parameter, which doesn't reflect typical usage with struct tags. Consider using a struct with tags to better represent real-world scenarios.
- out := []string{} + out := struct { + Color string `query:"color"` + }{}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
binder/mapping_test.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
|
LGTM |
|
Agree with @ReneWerner87 Seems we need 2 more tests with bad data to cover those 2 error cases |
Description
This PR adds support for multipart file binding when content type is
multipart/form-dataFixes #2002
Changes introduced
Type of change
Checklist
/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