🐛 bug: Fix square bracket notation in Multipart FormData #3235

efectn · 2024-12-06T12:36:01Z

Description

Add square bracket support to multipart binding
Add support for splitting. Depends on the 🐛 bug: fix EnableSplittingOnParsers is not functional #3231 PR

Fixes #3224

Will be backporting to v2 after the PR is get merged

Type of change

Enhancement (improvement to existing features and functionality)

Checklist

Before you submit your pull request, please make sure you meet these requirements:

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

coderabbitai · 2024-12-06T12:36:09Z

Warning

Rate limit exceeded

@gaby has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 58 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 3c3fa8c and 73920c8.

Walkthrough

The changes in this pull request enhance the multipart form data handling in the fiber package. It introduces new test cases in bind_test.go to validate the parsing of collections using square bracket notation and updates the BindMultipart method in binder/form.go to support this notation and handle comma-separated values. A new Demo struct is added to facilitate binding multiple Person entries. The error handling for required fields and nested structures is also improved, ensuring better validation and error messaging.

Changes

File	Change Summary
binder/form.go	Enhanced `BindMultipart` method to support square bracket notation and handle comma-separated values in multipart form data.
bind_test.go	Added tests for multipart form data parsing, including `MultipartCollectionQueryDotNotation` and `MultipartCollectionQuerySquareBrackets`. Introduced `Demo` and `Person` structs for binding multiple entries. Updated benchmarks for body binding.

Assessment against linked issues

Objective	Addressed	Explanation
Support square bracket notation in multipart form data (#3224)	✅
Support comma-separated values in multipart form data fields (#3224)	✅
Maintain ability to transfer binary data alongside structured form data (#3224)	❌	No explicit mention of binary data handling.

Possibly related issues

Issue Feature Request: Support Square Brackets in Multipart Form Data for v3 #3225: The changes support square bracket notation in multipart form data, addressing the same request.

🐰 In the fields where data flows,
A rabbit hops, where the structure grows.
With brackets square, and commas too,
Multipart forms, we bind anew!
So let’s celebrate, with joy and cheer,
For parsing dreams are finally here! 🐇

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

codecov · 2024-12-06T12:37:31Z

Codecov Report

Attention: Patch coverage is 0% with 16 lines in your changes missing coverage. Please review.

Project coverage is 82.72%. Comparing base (4347857) to head (73920c8).

Files with missing lines	Patch %	Lines
binder/form.go	0.00%	16 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3235      +/-   ##
==========================================
- Coverage   82.86%   82.72%   -0.15%     
==========================================
  Files         115      115              
  Lines       11295    11311      +16     
==========================================
- Hits         9360     9357       -3     
- Misses       1537     1555      +18     
- Partials      398      399       +1

Flag	Coverage Δ
unittests	`82.72% <0.00%> (-0.15%)`	⬇️

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.

coderabbitai

Actionable comments posted: 5

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 89a0cd3 and 829d6cf.

📒 Files selected for processing (2)

bind_test.go (4 hunks)
binder/form.go (1 hunks)

🧰 Additional context used

🪛 GitHub Check: codecov/patch

binder/form.go

[warning] 60-67: binder/form.go#L60-L67
Added lines #L60 - L67 were not covered by tests

[warning] 70-77: binder/form.go#L70-L77
Added lines #L70 - L77 were not covered by tests

🪛 golangci-lint (1.62.2)

bind_test.go

998-998: Error return value of writer.WriteField is not checked

(errcheck)

999-999: Error return value of writer.WriteField is not checked

(errcheck)

1000-1000: Error return value of writer.Close is not checked

(errcheck)

1019-1019: Error return value of writer.WriteField is not checked

(errcheck)

1020-1020: Error return value of writer.WriteField is not checked

(errcheck)

1021-1021: Error return value of writer.Close is not checked

(errcheck)

[warning] 1238-1238: unhandled-error: Unhandled error in call to function mime/multipart.Writer.WriteField

(revive)

1238-1238: Error return value of writer.WriteField is not checked

(errcheck)

[warning] 1240-1240: unhandled-error: Unhandled error in call to function mime/multipart.Writer.Close

(revive)

1240-1240: Error return value of writer.Close is not checked

(errcheck)

[warning] 1278-1278: unhandled-error: Unhandled error in call to function mime/multipart.Writer.WriteField

(revive)

1278-1278: Error return value of writer.WriteField is not checked

(errcheck)

[warning] 1279-1279: unhandled-error: Unhandled error in call to function mime/multipart.Writer.WriteField

(revive)

1279-1279: Error return value of writer.WriteField is not checked

(errcheck)

[warning] 1280-1280: unhandled-error: Unhandled error in call to function mime/multipart.Writer.WriteField

(revive)

[warning] 1281-1281: unhandled-error: Unhandled error in call to function mime/multipart.Writer.WriteField

(revive)

[warning] 1282-1282: unhandled-error: Unhandled error in call to function mime/multipart.Writer.WriteField

(revive)

[warning] 1284-1284: unhandled-error: Unhandled error in call to function mime/multipart.Writer.Close

(revive)

🪛 GitHub Check: lint

bind_test.go

[failure] 998-998:
Error return value of writer.WriteField is not checked (errcheck)

[failure] 999-999:
Error return value of writer.WriteField is not checked (errcheck)

[failure] 1000-1000:
Error return value of writer.Close is not checked (errcheck)

[failure] 1019-1019:
Error return value of writer.WriteField is not checked (errcheck)

[failure] 1020-1020:
Error return value of writer.WriteField is not checked (errcheck)

[failure] 1021-1021:
Error return value of writer.Close is not checked (errcheck)

[failure] 1238-1238:
unhandled-error: Unhandled error in call to function mime/multipart.Writer.WriteField (revive)

[failure] 1238-1238:
Error return value of writer.WriteField is not checked (errcheck)

[failure] 1240-1240:
unhandled-error: Unhandled error in call to function mime/multipart.Writer.Close (revive)

[failure] 1240-1240:
Error return value of writer.Close is not checked (errcheck)

bind_test.go

binder/form.go

coderabbitai

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)

bind_test.go (2)

992-1032: LGTM! Consider adding edge cases.

The new test cases effectively validate both dot notation and square bracket syntax for multipart form data binding. The tests are well-structured and consistent with the existing test patterns.

Consider adding test cases for:

Empty arrays

Invalid indices

Mixed notation (e.g., data[0].name and data.1[name])

Special characters in field names

Line range hint 992-1302: Overall implementation looks solid

The implementation successfully adds support for square bracket notation in multipart binding while maintaining compatibility with dot notation. The test coverage is good, though it could be expanded to cover more edge cases. The benchmarks provide performance validation but need the variable fixes mentioned above.

Since this feature provides two different notations for the same functionality, consider:

Documenting the preferred notation in comments

Adding examples in the test file showing when to use each notation

Ensuring consistent performance between both notations through benchmarks

🧰 Tools

🪛 GitHub Check: unit (1.23.x, macos-13)

[failure] 1238-1238:
undefined: t

[failure] 1239-1239:
undefined: t

[failure] 1277-1277:
undefined: t

[failure] 1278-1278:
undefined: t

[failure] 1279-1279:
undefined: t

[failure] 1280-1280:
undefined: t

[failure] 1281-1281:
undefined: t

[failure] 1282-1282:
undefined: t

[failure] 1238-1238:
undefined: t

[failure] 1239-1239:
undefined: t

🪛 GitHub Check: unit (1.23.x, macos-latest)

[failure] 1238-1238:
undefined: t

[failure] 1239-1239:
undefined: t

[failure] 1277-1277:
undefined: t

[failure] 1278-1278:
undefined: t

[failure] 1279-1279:
undefined: t

[failure] 1280-1280:
undefined: t

[failure] 1281-1281:
undefined: t

[failure] 1282-1282:
undefined: t

🪛 GitHub Check: unit (1.23.x, ubuntu-latest)

[failure] 1238-1238:
undefined: t

[failure] 1239-1239:
undefined: t

[failure] 1277-1277:
undefined: t

[failure] 1278-1278:
undefined: t

[failure] 1279-1279:
undefined: t

[failure] 1280-1280:
undefined: t

[failure] 1281-1281:
undefined: t

[failure] 1282-1282:
undefined: t

[failure] 1238-1238:
undefined: t

[failure] 1239-1239:
undefined: t

🪛 GitHub Check: lint

[failure] 1238-1238:
undefined: t

[failure] 1239-1239:
undefined: t

[failure] 1277-1277:
undefined: t

[failure] 1278-1278:
undefined: t

[failure] 1279-1279:
undefined: t

[failure] 1280-1280:
undefined: t

[failure] 1281-1281:
undefined: t

[failure] 1282-1282:
undefined: t (typecheck)

[failure] 1238-1238:
undefined: t

[failure] 1239-1239:
undefined: t

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 829d6cf and 3c3fa8c.

📒 Files selected for processing (1)

bind_test.go (4 hunks)

🧰 Additional context used

🪛 GitHub Check: unit (1.23.x, macos-13)

bind_test.go

[failure] 1238-1238:
undefined: t

[failure] 1239-1239:
undefined: t

[failure] 1277-1277:
undefined: t

[failure] 1278-1278:
undefined: t

[failure] 1279-1279:
undefined: t

[failure] 1280-1280:
undefined: t

[failure] 1281-1281:
undefined: t

[failure] 1282-1282:
undefined: t

[failure] 1238-1238:
undefined: t

[failure] 1239-1239:
undefined: t

🪛 GitHub Check: unit (1.23.x, macos-latest)

bind_test.go

[failure] 1238-1238:
undefined: t

[failure] 1239-1239:
undefined: t

[failure] 1277-1277:
undefined: t

[failure] 1278-1278:
undefined: t

[failure] 1279-1279:
undefined: t

[failure] 1280-1280:
undefined: t

[failure] 1281-1281:
undefined: t

[failure] 1282-1282:
undefined: t

🪛 GitHub Check: unit (1.23.x, ubuntu-latest)

bind_test.go

[failure] 1238-1238:
undefined: t

[failure] 1239-1239:
undefined: t

[failure] 1277-1277:
undefined: t

[failure] 1278-1278:
undefined: t

[failure] 1279-1279:
undefined: t

[failure] 1280-1280:
undefined: t

[failure] 1281-1281:
undefined: t

[failure] 1282-1282:
undefined: t

[failure] 1238-1238:
undefined: t

[failure] 1239-1239:
undefined: t

🪛 GitHub Check: lint

bind_test.go

[failure] 1238-1238:
undefined: t

[failure] 1239-1239:
undefined: t

[failure] 1277-1277:
undefined: t

[failure] 1278-1278:
undefined: t

[failure] 1279-1279:
undefined: t

[failure] 1280-1280:
undefined: t

[failure] 1281-1281:
undefined: t

[failure] 1282-1282:
undefined: t (typecheck)

[failure] 1238-1238:
undefined: t

[failure] 1239-1239:
undefined: t

bind_test.go

gaby · 2024-12-09T00:19:28Z

@efectn Analysis by o1-pro of the whole file.

Key Issues Identified:

Inconsistent key handling in BindMultipart:
When parsing multipart form keys containing square brackets, the code attempts to normalize them using parseParamSquareBrackets. Once a bracketed key is converted (e.g., key becomes k), the code updates data.Value[k] and deletes data.Value[key]. However, in the subsequent logic that splits values by commas, the original key is still used instead of k. This leads to an inconsistency:
```
if strings.Contains(key, "[") {
    k, err := parseParamSquareBrackets(key)
    if err != nil {
        return err
    }
    data.Value[k] = values
    delete(data.Value, key)
    // After this, the logical key should be 'k', not 'key'.
}

if strings.Contains(v, ",") && equalFieldType(out, reflect.Slice, key) {
    delete(data.Value, key)
    values := strings.Split(v, ",")
    for i := 0; i < len(values); i++ {
        data.Value[key] = append(data.Value[key], values[i])
    }
}
```
Here, key should be replaced with k once the key has been normalized. Otherwise, you risk appending data to a key that no longer exists or overwriting the wrong keys. A simple fix is to reassign key = k after normalizing, ensuring that all subsequent references consistently use the updated key.
Modifying the map while iterating in BindMultipart:
The code updates data.Value (by deleting and reassigning entries) within the same for key, values := range data.Value loop. While in current Go implementations this often works without immediate panics, it's not a best practice and can lead to unpredictable behavior or future compatibility issues. A safer approach would be:
- First, collect all necessary key/value transformations into a temporary map.
- After the loop, apply all the collected changes at once to data.Value.
Comma-splitting logic potentially overwriting data in BindMultipart:
Consider the scenario where multiple values in values contain commas. The current code deletes the entry from data.Value and then re-appends split arrays for each comma-containing value. If more than one v contains commas, each subsequent match re-deletes and reassigns the array, possibly discarding previously processed splits from the same key. This may not be intended behavior. If the goal is to combine all comma-separated values into a single array, you may want to accumulate them first, then write them back to data.Value[key] once at the end of processing that key.
Error handling inside VisitAll in Bind():
Inside the Bind() method’s VisitAll callback, if an error occurs (e.g., in parseParamSquareBrackets), the code sets err and returns from the callback. The logic outside the callback checks if err != nil { return err } after iteration. This is correct logically, but it's important to ensure that no further processing occurs after err is set in the callback. The current code seems correct, but just be aware that any operations after setting err within the callback won't be halted until the callback returns. This is not a bug per se, just something to be cautious about.
Reliance on equalFieldType and parseParamSquareBrackets correctness:
If equalFieldType or parseParamSquareBrackets panic or do not handle unexpected input gracefully, it could lead to runtime errors. Ensure these helper functions handle all edge cases.

Summary of Primary Concerns:

The most significant bug is the incorrect usage of key after it has been transformed into k in the multipart parsing logic.
Modifying data.Value while iterating over it could cause unpredictable outcomes; better to gather changes and apply them after the loop.
Repeatedly deleting and reassigning keys when splitting comma-separated values can lead to unexpected loss of data if multiple values contain commas.

🐛 bug: add square bracket notation support to BindMultipart

829d6cf

efectn requested a review from a team as a code owner December 6, 2024 12:36

efectn requested review from gaby, sixcolors and ReneWerner87 and removed request for a team December 6, 2024 12:36

github-actions bot added ☢️ Bug ✏️ Feature labels Dec 6, 2024

efectn added the v3 label Dec 6, 2024

efectn added this to the v3 milestone Dec 6, 2024

coderabbitai bot reviewed Dec 6, 2024

View reviewed changes

bind_test.go Outdated Show resolved Hide resolved

bind_test.go Outdated Show resolved Hide resolved

bind_test.go Outdated Show resolved Hide resolved

bind_test.go Outdated Show resolved Hide resolved

binder/form.go Show resolved Hide resolved

gaby added 2 commits December 6, 2024 22:35

Fix golangci-lint issues

3c3fa8c

Fixing undef variable

2692923

coderabbitai bot reviewed Dec 7, 2024

View reviewed changes

bind_test.go Outdated Show resolved Hide resolved

Fix more lint issues

df12e64

gaby changed the title ~~🐛 bug: add square bracket notation support to BindMultipart~~ 🐛 bug: Fix square bracket notation in Multipart FormData Dec 7, 2024

Merge branch 'main' into multipart-nested

73920c8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 bug: Fix square bracket notation in Multipart FormData #3235

🐛 bug: Fix square bracket notation in Multipart FormData #3235

efectn commented Dec 6, 2024

coderabbitai bot commented Dec 6, 2024 •

edited

Loading

Rate limit exceeded

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

codecov bot commented Dec 6, 2024 •

edited

Loading

coderabbitai bot left a comment

coderabbitai bot left a comment

gaby commented Dec 9, 2024

🐛 bug: Fix square bracket notation in Multipart FormData #3235

Are you sure you want to change the base?

🐛 bug: Fix square bracket notation in Multipart FormData #3235

Conversation

efectn commented Dec 6, 2024

Description

Type of change

Checklist

Commit formatting

coderabbitai bot commented Dec 6, 2024 • edited Loading

Rate limit exceeded

Walkthrough

Changes

Assessment against linked issues

Possibly related issues

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

codecov bot commented Dec 6, 2024 • edited Loading

Codecov Report

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

gaby commented Dec 9, 2024

coderabbitai bot commented Dec 6, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

codecov bot commented Dec 6, 2024 •

edited

Loading