bind: add support for multipart file binding #3309
Conversation
efectn
requested review from
gaby,
sixcolors and
ReneWerner87
and removed request for
a team
|
Warning Rate limit exceeded@efectn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
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
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.05% -0.11%
==========================================
Files 116 116
Lines 11551 11572 +21
==========================================
+ Hits 9721 9727 +6
- Misses 1399 1409 +10
- Partials 431 436 +5
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.
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