Skip to content

bind: add support for multipart file binding#3309

Merged
ReneWerner87 merged 7 commits intomainfrom
multipart-support
Feb 25, 2025
Merged

bind: add support for multipart file binding#3309
ReneWerner87 merged 7 commits intomainfrom
multipart-support

Conversation

@efectn
Copy link
Member

@efectn efectn commented Feb 13, 2025

Description

This PR adds support for multipart file binding when content type is multipart/form-data
Fixes #2002

Changes introduced

  • Benchmarks: Describe any performance benchmarks and improvements related to the changes.
  • Documentation Update: Detail the updates made to the documentation and links to the changed files.
  • Changelog/What's New: Include a summary of the additions for the upcoming release notes.
  • Examples: Provide examples demonstrating the new features or changes in action.

Type of change

  • Enhancement (improvement to existing features and functionality)

Checklist

  • Followed the inspiration of the Express.js framework for new functionalities, making them similar in usage.
  • Conducted a self-review of the code and provided comments for complex or critical parts.
  • Updated the documentation in the /docs/ directory for Fiber's documentation.
  • Added or updated unit tests to validate the effectiveness of the changes or new features.
  • Ensured that new and existing unit tests pass locally with the changes.
  • Verified that any new dependencies are essential and have been agreed upon by the maintainers/community.
  • Aimed for optimal performance with minimal allocations in the new code.
  • Provided benchmarks for the new code to analyze and improve upon.

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

@efectn efectn added this to the v3 milestone Feb 13, 2025
@efectn efectn requested a review from a team as a code owner February 13, 2025 14:29
@efectn efectn requested review from ReneWerner87, gaby and sixcolors and removed request for a team February 13, 2025 14:29
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 13, 2025

Walkthrough

The changes extend the binder’s functionality to support multipart file uploads. The bindMultipart method now collects file headers and passes them along to the parsing functions. Test cases are updated with new file-related struct fields to verify file upload binding. Additionally, the mapping functions are modified to accept file data, and the documentation is enhanced with binding examples and corresponding curl commands. Broader API documentation also reflects these updates within the Fiber framework v3 release notes.

Changes

File(s) Change Summary
binder/form.go,
binder/form_test.go,
binder/mapping.go
Enhanced multipart/form-data binding to support file uploads by introducing file header handling, iterating over file parts, and updating function signatures and parsing logic.
docs/api/bind.md Updated documentation to include examples of binding multipart file uploads, with sample struct definitions and curl commands for testing file uploads.
docs/whats_new.md Revised release notes for v3, detailing changes in method signatures, route handling, and the new support for multipart file binding alongside other API improvements.

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
Loading

Possibly related PRs

Suggested reviewers

  • sixcolors
  • gaby
  • efectn

Poem

I’m a rabbit coding with delight,
Files hop in with a joyous bite.
New headers bound in a rhythmic dance,
Each multipart upload gets its chance.
With sniffs and hops, bugs take flight,
Celebrating code in the moonlight!


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a875ec6 and a99d7dc.

📒 Files selected for processing (2)
  • binder/mapping_test.go (6 hunks)
  • docs/whats_new.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/whats_new.md

[style] ~549-~549: 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)

⏰ Context from checks skipped due to timeout of 90000ms (10)
  • GitHub Check: unit (1.24.x, macos-13)
  • GitHub Check: unit (1.24.x, macos-latest)
  • GitHub Check: unit (1.24.x, windows-latest)
  • GitHub Check: unit (1.24.x, ubuntu-latest)
  • 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: Compare
  • GitHub Check: repeated
  • GitHub Check: Analyse
🔇 Additional comments (6)
docs/whats_new.md (1)

549-549: Documentation for the new multipart file binding feature looks good!

This entry clearly documents the new feature that allows binding multipart file uploads to struct fields with supported field types: *multipart.FileHeader, *[]*multipart.FileHeader, and []*multipart.FileHeader. This aligns well with the PR objective to add support for multipart file binding.

🧰 Tools
🪛 LanguageTool

[style] ~549-~549: 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)

binder/mapping_test.go (5)

233-249: Good test coverage for multipart file binding

This test case properly verifies that the formatBindData function can handle multiple file headers, which is a key part of the new multipart file binding feature.


251-258: Assertion order fixed from previous review

The assertion order now correctly follows the expected, actual pattern recommended by testify.


194-259: Comprehensive test suite for formatBindData

The test covers various scenarios including:

  • Happy path with string values
  • Error handling for unsupported value types
  • Bracket notation parsing errors
  • Multipart file header handling
  • Type casting errors

This ensures the new multipart file binding functionality works correctly and handles error cases appropriately.


303-345: Thorough error case testing

The separate test function for error cases provides good coverage of various error scenarios that could occur during binding, including:

  • Different types of unsupported values
  • Bracket notation parsing errors
  • Type casting errors

This helps ensure robust error handling in the binding functionality.


261-283: Verification of data splitting behavior

The test verifies both cases of data splitting:

  1. When splitting is enabled, a comma-separated string is properly split into multiple values
  2. When splitting is disabled, the string is kept as a single value

This ensures consistent behavior when binding data with commas.

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between b0bc32b and ec7c89a.

⛔ Files ignored due to path filters (2)
  • go.mod is excluded by !**/*.mod
  • go.sum is 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 adding files ...map[string][]*multipart.FileHeader.
This parameter extension cleanly supports file data alongside string data.


87-87: Consistent call to parseToStruct.
Passing the additional files parameter ensures that file data is handled in one place.


91-91: Extended signature for parseToStruct.
This aligns well with the new files parameter, 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 to func 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 passing val to assignBindData.

🧰 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: Importing mime/multipart is correct for file-bound data.
No issues here.


64-72: Efficient approach to separate file data from form data.
Collecting file headers into files then passing them to parse is clean. Consider large file scenarios, but for typical usage, this is fine.

binder/form_test.go (5)

5-5: Importing io is 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:

  1. Define a struct with a file field
  2. Handle file binding in a route handler
  3. 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
Copy link

codecov bot commented Feb 13, 2025

Codecov Report

Attention: Patch coverage is 70.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 84.27%. Comparing base (d6d48d8) to head (a99d7dc).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
binder/mapping.go 73.91% 4 Missing and 2 partials ⚠️
binder/form.go 57.14% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3309      +/-   ##
==========================================
+ Coverage   84.25%   84.27%   +0.01%     
==========================================
  Files         116      116              
  Lines       11556    11577      +21     
==========================================
+ Hits         9737     9757      +20     
  Misses       1391     1391              
- Partials      428      429       +1     
Flag Coverage Δ
unittests 84.27% <70.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 tests

docs/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

📥 Commits

Reviewing files that changed from the base of the PR and between ec7c89a and 784f8e2.

📒 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
binder/mapping_test.go (1)

233-233: ⚠️ Potential issue

Fix assertion order in test.

The assertion order is incorrect according to the testifylint tool.

-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 require package 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 require package 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 require package 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 require package 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

📥 Commits

Reviewing files that changed from the base of the PR and between 784f8e2 and a875ec6.

📒 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

@ReneWerner87
Copy link
Member

LGTM
@efectn the error cases are not covered -> thats codecov is red
can we easily cover it ?

@gaby
Copy link
Member

gaby commented Feb 20, 2025

Agree with @ReneWerner87 Seems we need 2 more tests with bad data to cover those 2 error cases

@ReneWerner87 ReneWerner87 merged commit bc4c920 into main Feb 25, 2025
17 of 18 checks passed
@efectn efectn deleted the multipart-support branch February 26, 2025 06:19
@coderabbitai coderabbitai bot mentioned this pull request Mar 3, 2025
20 tasks
@ReneWerner87
Copy link
Member

@coderabbitai create a feature request/proposal for version 3 for a bind feature where you can bind all request resources to your struct
e.g. a ctx.Bind().All(myStruct) method
where then query, body, url etc. data is used together with the tags which are already defined in the struct
it must also be clarified what is done if keys exist in several resources (queryParams, body, url) and/or if these are not there, whether empty values are then set

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 19, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First let's look at the fiber binding related files to understand current architecture
echo "Looking for binding related files in the codebase:"
find . -type f -name "*bind*.go" | grep -v vendor | grep -v test
echo "----------------"

# Let's examine the current ctx.Bind() implementation
echo "Examining current Bind implementation:"
grep -r "func .*Bind" --include="*.go" .
echo "----------------"

# Let's look at how the Bind method is currently defined and used
echo "Looking at how Bind is currently used:"
grep -r "Bind()" --include="*.go" . | grep -v test

Length of output: 9085


🏁 Script executed:

#!/bin/bash
# Let's look at the main bind.go file to understand the Bind struct
echo "Examining bind.go file structure:"
head -n 50 ./bind.go
echo "..."
echo "----------------"

# Let's also look at the ctx.go file to see how Bind() is implemented
echo "Examining Bind() method in ctx.go:"
grep -A 10 "func (c \*DefaultCtx) Bind()" ./ctx.go
echo "----------------"

# Let's see if there are any existing examples of binding in the documentation
echo "Checking for binding documentation:"
find ./docs -type f -name "*.md" | xargs grep -l "bind"

Length of output: 2184


🏁 Script executed:

#!/bin/bash
# Let's check the binding documentation to understand current binding capabilities
echo "Examining bind.md documentation:"
cat ./docs/api/bind.md
echo "----------------"

# Let's look at how struct tags are currently used for binding
echo "Checking some struct tag examples in tests:"
grep -A 15 "type.*struct" ./bind_test.go | head -n 30

Length of output: 18406


@ReneWerner87 Here's a comprehensive feature proposal for the all-in-one binding feature you requested:

Feature Proposal: Universal Request Binding with ctx.Bind().All()

Overview

Add a new All() binding method to bind data from multiple request sources (query parameters, body, URL parameters, headers, cookies) into a single struct using a single method call.

Proposed API

// All binds data from multiple sources into a struct
func (b *Bind) All(out any) error

Usage Example

type User struct {
    // Field can be bound from any source
    ID        int    `param:"id" query:"id" json:"id" form:"id"`
    
    // Fields with source-specific tags
    Name      string `query:"name" json:"name" form:"name"`
    Email     string `json:"email" form:"email"`
    Role      string `header:"x-user-role"`
    SessionID string `cookie:"session_id"`
    
    // Optional source preference definition
    Avatar    *multipart.FileHeader `form:"avatar" binding_source:"form,body"`
}

app.Post("/users/:id", func(c fiber.Ctx) error {
    user := new(User)
    
    if err := c.Bind().All(user); err != nil {
        return err
    }
    
    // All available data is now bound to the user struct
    return c.JSON(user)
})

Source Precedence

When the same field is present in multiple sources, we need a clear precedence rule. I propose the following default precedence order (highest to lowest):

  1. URL Parameters (:param)
  2. Body (JSON/XML/Form)
  3. Query parameters
  4. Headers
  5. Cookies

This order follows a general principle of specificity (URL param is most specific to this request), followed by explicit data (body), then ambient request data (query, headers, cookies).

Custom Source Precedence

For flexibility, developers can define custom precedence with an optional binding_source tag:

type Product struct {
    // Override default precedence (query takes precedence over body)
    ID    int    `param:"id" query:"id" json:"id" binding_source:"param,query,body"`
    
    // Only bind from form data, ignore other sources
    Image *multipart.FileHeader `form:"image" binding_source:"form"`
}

Empty Values Handling

By default, empty values from higher-precedence sources won't overwrite non-empty values from lower-precedence sources.

Example: If a field has value "foo" in the body but an empty string in the query, the final value would be "foo".

This behavior can be changed with a configuration option:

// Override empty values behavior
if err := c.Bind().WithOverrideEmptyValues(true).All(user); err != nil {
    return err
}

Implementation Considerations

  1. Support for all current field types, including file uploads
  2. Integration with existing validation system
  3. Efficient binding to minimize allocations
  4. Clear error messages about binding failures

Benefits

  1. Simplified API for handling data from multiple sources
  2. Reduced code duplication in handlers
  3. Clearer intent in struct definitions
  4. Consistent handling of conflicts between sources

This aligns with Fiber's design principles of Express.js compatibility while offering Go-idiomatic features that improve developer experience.

Would you like me to expand on any aspect of this proposal or provide additional implementation details?

@ReneWerner87
Copy link
Member

@coderabbitai please create the proposal in a new issue, you are welcome to add more information for the implementation and also examples of use

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 19, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@ReneWerner87 I'll create the proposal issue right away with the information from our discussion.

@ReneWerner87 I've created a comprehensive issue with the feature proposal for the universal binding feature. The issue includes:

  • Detailed API description for ctx.Bind().All()
  • Multiple usage examples with different scenarios
  • Implementation details including source precedence and empty value handling
  • Code examples showing how it would work with various data types and validation
  • Technical implementation considerations and performance optimizations
  • Benefits and potential future extensions

Is there anything specific you'd like me to add or modify in the proposal?


🏁 Script executed:

#!/bin/bash
# Create a new issue for the binding feature proposal
gh issue create \
--title "Feature Proposal: Universal Request Binding with ctx.Bind().All()" \
--body "# Universal Request Binding: ctx.Bind().All()

## Overview
This proposal suggests adding a new \`All()\` binding method to the Fiber framework that would bind data from multiple request sources (query parameters, body, URL parameters, headers, cookies) into a single struct using a single method call.

## Motivation
Currently, developers need to call multiple binding methods (c.BodyParser(), c.QueryParser(), etc.) to gather data from different sources. This leads to repetitive code and requires developers to manually handle conflicts when the same field might be present in multiple sources.

## Proposed API

\`\`\`go
// All binds data from multiple sources into a single struct
func (b *Bind) All(out any) error
\`\`\`

## Usage Examples

### Basic Usage

\`\`\`go
type User struct {
    // Field can be bound from any source
    ID        int    \`param:\"id\" query:\"id\" json:\"id\" form:\"id\"\`
    
    // Fields with source-specific tags
    Name      string \`query:\"name\" json:\"name\" form:\"name\"\`
    Email     string \`json:\"email\" form:\"email\"\`
    Role      string \`header:\"x-user-role\"\`
    SessionID string \`cookie:\"session_id\"\`
    
    // File upload support
    Avatar    *multipart.FileHeader \`form:\"avatar\"\`
}

app.Post(\"/users/:id\", func(c fiber.Ctx) error {
    user := new(User)
    
    if err := c.Bind().All(user); err != nil {
        return err
    }
    
    // All available data is now bound to the user struct
    return c.JSON(user)
})
\`\`\`

### With Validation

\`\`\`go
type CreateProductRequest struct {
    Name        string  \`json:\"name\" query:\"name\" validate:\"required,min=3\"\`
    Price       float64 \`json:\"price\" query:\"price\" validate:\"required,gt=0\"\`
    CategoryID  int     \`json:\"category_id\" param:\"category_id\" validate:\"required\"\`
}

app.Post(\"/categories/:category_id/products\", func(c fiber.Ctx) error {
    req := new(CreateProductRequest)
    
    if err := c.Bind().All(req); err != nil {
        return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{
            \"error\": \"Invalid request data\",
            \"details\": err.Error(),
        })
    }
    
    // Input is bound and validated
    return createProduct(c, req)
})
\`\`\`

## Source Precedence

When the same field is present in multiple sources, we need a clear precedence rule. Proposed default precedence order (highest to lowest):

1. URL Parameters (\`:param\`)
2. Body (JSON/XML/Form)
3. Query parameters
4. Headers
5. Cookies

This order follows a general principle of specificity (URL param is most specific to this request), followed by explicit data (body), then ambient request data (query, headers, cookies).

## Custom Source Precedence

For flexibility, developers can define custom precedence with an optional \`binding_source\` tag:

\`\`\`go
type Product struct {
    // Override default precedence (query takes precedence over body)
    ID    int    \`param:\"id\" query:\"id\" json:\"id\" binding_source:\"param,query,body\"\`
    
    // Only bind from form data, ignore other sources
    Image *multipart.FileHeader \`form:\"image\" binding_source:\"form\"\`
}
\`\`\`

## Empty Values Handling

By default, empty values from higher-precedence sources won't overwrite non-empty values from lower-precedence sources.

Example: If a field has value \"foo\" in the body but an empty string in the query, the final value would be \"foo\".

This behavior can be changed with a configuration option:

\`\`\`go
// Override empty values behavior
if err := c.Bind().WithOverrideEmptyValues(true).All(user); err != nil {
    return err
}
\`\`\`

## Implementation Details

### Integration with Existing Bind Interface

The proposed feature would extend the existing \`Bind\` interface:

\`\`\`go
type Bind interface {
    // Existing methods
    Body(out any) error
    Query(out any) error
    Params(out any) error
    Headers(out any) error
    Cookies(out any) error
    
    // New methods
    All(out any) error
    WithOverrideEmptyValues(override bool) Bind
}
\`\`\`

### Internal Implementation Approach

1. Parse each data source independently (reusing existing binding logic)
2. Apply values to the output struct according to precedence rules
3. Handle special cases like file uploads appropriately
4. Provide detailed error messages for binding failures

\`\`\`go
// Pseudo-implementation
func (b *Bind) All(out any) error {
    // Store binding errors to report after all attempts
    var bindingErrors []error
    
    // Get values from each source
    paramValues, paramErr := b.getParamValues()
    if paramErr != nil {
        bindingErrors = append(bindingErrors, paramErr)
    }
    
    bodyValues, bodyErr := b.getBodyValues()
    if bodyErr != nil {
        bindingErrors = append(bindingErrors, bodyErr)
    }
    
    queryValues, queryErr := b.getQueryValues()
    if queryErr != nil {
        bindingErrors = append(bindingErrors, queryErr)
    }
    
    // ... similar for headers and cookies
    
    // Apply values according to precedence
    if err := b.applyValues(out, paramValues, bodyValues, queryValues, headerValues, cookieValues); err != nil {
        return err
    }
    
    // If we had any binding errors but still managed to bind some values, report them
    if len(bindingErrors) > 0 {
        return fmt.Errorf(\"partial binding completed with errors: %v\", bindingErrors)
    }
    
    return nil
}
\`\`\`

### Performance Considerations

1. Avoid unnecessary parsing (lazy-load each source)
2. Reuse existing binding mechanisms where possible
3. Minimize allocations for property mapping
4. Consider caching struct field metadata for repeated bindings

## Benefits

1. Simplified API for handling data from multiple sources
2. Reduced code duplication in handlers
3. Clearer intent in struct definitions 
4. Consistent handling of conflicts between sources
5. Better integration with validation libraries
6. Aligns with RESTful practices where resources can be identified and manipulated through multiple means

## Future Extensions

1. Support for binding to nested structs from different sources
2. Custom binding functions for specific fields
3. Built-in type conversion for common cases (string to int, etc.)
4. Integration with schema validation

This proposal aligns with Fiber's design principles of Express.js compatibility while offering Go-idiomatic features that improve developer experience.

## References

* Related issue: #2002 (the multipart file binding issue)
* Current binding implementation in Fiber v2.x

---

_Requested by: @ReneWerner87_" \
--assignee "ReneWerner87"

Length of output: 6641


@ReneWerner87 I've successfully created the issue with all the details from our discussion. You can find it here:

#3363

The proposal includes comprehensive information about the universal binding feature including the API design, usage examples, source precedence rules, empty value handling, implementation details, and potential future extensions.

Let me know if you'd like me to make any adjustments to the issue or if you have any questions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

🚀 v3 Request: Add Support for *multipart.FileHeader fields to Multipart Binder

3 participants