🐛 bug: fix EnableSplittingOnParsers is not functional #3231
Conversation
efectn
requested review from
gaby,
sixcolors and
ReneWerner87
and removed request for
a team
WalkthroughThe pull request introduces significant enhancements to the binding functionalities across various files, primarily focusing on implementing a pooling mechanism for binder instances to optimize resource management. The Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 5
🧹 Outside diff range and nitpick comments (8)
binder/binder.go (1)
14-66: Refactor to reduce code duplication insync.PooldeclarationsThe
sync.Pooldeclarations for various binder types are repetitive. Consider refactoring to reduce duplication by using a helper function.Suggested change:
+func newBinderPool[T any](binder T) sync.Pool { + return sync.Pool{ + New: func() any { + return binder + }, + } +} -var HeaderBinderPool = sync.Pool{ - New: func() any { - return &HeaderBinding{ - EnableSplitting: false, - } - }, -} +var HeaderBinderPool = newBinderPool(&HeaderBinding{ + EnableSplitting: false, +})Apply similar changes to other binder pools.
binder/header.go (1)
Line range hint
28-34: Consider performance and whitespace handling improvementsThe current implementation has two potential areas for improvement:
- Performance:
strings.Containsfollowed bystrings.Splitcreates unnecessary overhead when splitting is disabled- Whitespace: Split values might contain leading/trailing whitespace
Consider this optimization:
- if b.EnableSplitting && strings.Contains(v, ",") && equalFieldType(out, reflect.Slice, k) { - values := strings.Split(v, ",") + if b.EnableSplitting && equalFieldType(out, reflect.Slice, k) { + if strings.Contains(v, ",") { + values := strings.Split(v, ",") + for i := 0; i < len(values); i++ { + data[k] = append(data[k], strings.TrimSpace(values[i])) + } + continue + } + } - for i := 0; i < len(values); i++ { - data[k] = append(data[k], values[i]) - } - } else { - data[k] = append(data[k], v) - } + data[k] = append(data[k], v)binder/resp_header.go (1)
Line range hint
28-34: Consider extracting common header binding logicThe header parsing logic is duplicated between
HeaderBindingandRespHeaderBinding.Consider creating a shared utility function:
+// headerBindingUtil handles common header binding logic +func headerBindingUtil(enableSplitting bool, out any, visitor func(func(key, val []byte))) (map[string][]string, error) { + data := make(map[string][]string) + visitor(func(key, val []byte) { + k := utils.UnsafeString(key) + v := utils.UnsafeString(val) + + if enableSplitting && equalFieldType(out, reflect.Slice, k) { + if strings.Contains(v, ",") { + values := strings.Split(v, ",") + for i := 0; i < len(values); i++ { + data[k] = append(data[k], strings.TrimSpace(values[i])) + } + return + } + } + data[k] = append(data[k], v) + }) + return data, nil +}Then update both binding implementations:
func (b *HeaderBinding) Bind(req *fasthttp.Request, out any) error { - data := make(map[string][]string) - req.Header.VisitAll(func(key, val []byte) { - k := utils.UnsafeString(key) - v := utils.UnsafeString(val) - if b.EnableSplitting && strings.Contains(v, ",") && equalFieldType(out, reflect.Slice, k) { - values := strings.Split(v, ",") - for i := 0; i < len(values); i++ { - data[k] = append(data[k], values[i]) - } - } else { - data[k] = append(data[k], v) - } - }) + data, err := headerBindingUtil(b.EnableSplitting, out, req.Header.VisitAll) + if err != nil { + return err + } return parse(b.Name(), out, data) }This refactoring would:
- Eliminate code duplication
- Make future maintenance easier
- Ensure consistent behavior across both bindings
- Include the performance and whitespace improvements mentioned earlier
binder/form.go (1)
Line range hint
1-67: Consider adding integration tests for the EnableSplitting featureThe changes across all three binder types introduce similar splitting functionality. We should ensure comprehensive testing of this feature.
Would you like me to help create integration tests that verify the consistent behavior of
EnableSplittingacross all binder types? This would help ensure that:
- The feature works consistently across all binder types
- Edge cases are handled uniformly
- The interaction with existing features (like array notation) is tested
binder/mapping.go (2)
35-35: Consider using constants for binder type names.Using string literals directly makes the code fragile to changes. Consider defining these as constants or getting them from the binder types to maintain a single source of truth.
-tags = []string{"header", "respHeader", "cookie", "query", "form", "uri"} +const ( + HeaderBinder = "header" + RespHeaderBinder = "respHeader" + CookieBinder = "cookie" + QueryBinder = "query" + FormBinder = "form" + URIBinder = "uri" +) +tags = []string{HeaderBinder, RespHeaderBinder, CookieBinder, QueryBinder, FormBinder, URIBinder}
226-226: Use the defined constant for query binder name.Using a hardcoded string "query" creates a maintenance burden. This should use the same constant as defined for the tags array to maintain consistency.
-inputFieldName := typeField.Tag.Get("query") // Name of query binder +inputFieldName := typeField.Tag.Get(QueryBinder) // Name of query binderbind.go (1)
190-197: Consider making XMLDecoder configurable.While other decoders (JSON, CBOR) are configurable through app config, the XML decoder is hardcoded to
xml.Unmarshal. Consider making it configurable for consistency.-bind.XMLDecoder = xml.Unmarshal +bind.XMLDecoder = b.ctx.App().Config().XMLDecoderbind_test.go (1)
35-37: Good test coverage for EnableSplittingOnParsers.The tests thoroughly verify the splitting behavior across different binding types. However, consider adding negative test cases where EnableSplittingOnParsers is false to ensure the default behavior is maintained.
Add test cases like:
t.Run("WithoutSplitting", func(t *testing.T) { app := New(Config{ EnableSplittingOnParsers: false, }) // Test that values aren't split })Also applies to: 116-118, 550-552, 760-762, 1352-1354, 1427-1429
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
bind.go(10 hunks)bind_test.go(10 hunks)binder/binder.go(2 hunks)binder/cbor.go(1 hunks)binder/cookie.go(2 hunks)binder/form.go(3 hunks)binder/header.go(1 hunks)binder/json.go(1 hunks)binder/mapping.go(2 hunks)binder/query.go(2 hunks)binder/resp_header.go(1 hunks)binder/uri.go(1 hunks)binder/xml.go(1 hunks)ctx_test.go(1 hunks)
🔇 Additional comments (13)
ctx_test.go (2)
1450-1534: Subtests in Test_Ctx_Binders improve test organization
The use of subtests enhances readability and maintainability of the test cases.
1536-1555: Ensure consistent behavior with EnableSplittingOnParsers configuration
In Test_Ctx_Binders_Comma, the EnableSplittingOnParsers is set to true. Verify that this is the intended behavior and consistent with other tests.
binder/uri.go (1)
4-4: LGTM! Consistent with binding type exports
The renaming of uriBinding to URIBinding aligns with the package's move towards exported binding types. Correctly maintains original functionality without adding EnableSplitting since URI parameters are inherently single values.
Let's verify the consistency of binding type exports:
Also applies to: 7-8, 12-12
✅ Verification successful
All binding types are consistently exported
The verification confirms that all binding types in the codebase follow the same pattern of being exported (capitalized) struct types:
- URIBinding
- RespHeaderBinding
- XMLBinding
- QueryBinding
- HeaderBinding
- CookieBinding
- CBORBinding
- FormBinding
- JSONBinding
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all binding types are exported
rg -g 'binder/*.go' "type.*Binding.*struct"
Length of output: 433
binder/header.go (1)
12-14: LGTM! Clear struct definition with EnableSplitting flag
The addition of EnableSplitting field is well-documented and properly encapsulated.
binder/resp_header.go (1)
12-14: LGTM! Consistent with HeaderBinding implementation
The RespHeaderBinding structure mirrors HeaderBinding, maintaining consistency across the package.
binder/cookie.go (2)
11-14: LGTM: Clean struct definition with clear documentation
The exported CookieBinding struct with the EnableSplitting field is well-documented and follows Go conventions.
Line range hint 34-41: Verify the behavior of empty values in comma-separated strings
The splitting logic looks correct, but we should handle edge cases for comma-separated values.
Consider handling empty values explicitly:
if b.EnableSplitting && strings.Contains(v, ",") && equalFieldType(out, reflect.Slice, k) {
values := strings.Split(v, ",")
+ // Filter out empty values
+ filtered := make([]string, 0, len(values))
for i := 0; i < len(values); i++ {
- data[k] = append(data[k], values[i])
+ if trimmed := strings.TrimSpace(values[i]); trimmed != "" {
+ filtered = append(filtered, trimmed)
+ }
}
+ if len(filtered) > 0 {
+ data[k] = filtered
+ }
} else {binder/query.go (2)
11-14: LGTM: Consistent struct definition with cookie binding
The QueryBinding struct follows the same pattern as CookieBinding, maintaining consistency across the package.
Line range hint 38-45: Consider order of operations for bracket parsing and value splitting
The implementation correctly handles both square bracket parsing and value splitting, but we should verify the interaction between these two features.
✅ Verification successful
Let me gather more specific information about the test cases and implementation.
Let me try a different approach to find the relevant test cases and implementation.
Based on the test cases found, I can now verify the implementation. Let me check the specific query binding implementation.
Based on the test cases and implementation found, I can now generate the final response.
The interaction between array notation and comma-separated values is properly handled
The test cases demonstrate that both features work correctly together:
- When EnableSplitting is true, comma-separated values are properly split into slices
- Array notation with square brackets is handled independently
- Test cases verify both scenarios:
data[]=1&data[]=2works for array notationhobby=basketball,footballworks for comma splittinghobby=soccer&hobby=basketball,footballcorrectly combines both approaches
The implementation in binder/query.go correctly handles both cases by:
- First checking for EnableSplitting and comma presence
- Then handling array notation separately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases combining array notation with comma-separated values
rg -A 5 'TestQueryBinding.*Array.*Split' --type go
# Look for actual usage patterns
rg -A 2 '\[.*\].*=.*,.*,' --type go
Length of output: 10870
Script:
#!/bin/bash
# Search for test cases specifically testing array binding behavior
ast-grep --pattern 'func Test$_Bind$_($$$) {
$$$
}'
# Look for the actual implementation of array binding
ast-grep --pattern 'func ($b $_) bind($$$) {
$$$
}'
# Search for test cases with array notation and comma-separated values
rg -B 2 -A 2 'TestBind.*func.*\{.*\[\]' --type go
Length of output: 163
Script:
#!/bin/bash
# Search for test cases related to query binding with arrays
rg -B 3 -A 5 'TestQuery.*func' --type go
# Look for the implementation of query binding
rg -B 3 -A 10 'func.*query.*bind' --type go
# Find test cases with array notation in query strings
rg -B 2 -A 2 'SetQueryString.*\[\]' --type go
Length of output: 751
Script:
#!/bin/bash
# Look for the query binding implementation in binder package
rg -B 5 -A 10 'EnableSplitting.*Split' --type go
# Find test cases validating array notation and splitting behavior
cat bind_test.go
# Check for any related configuration or documentation
rg -l 'EnableSplitting' --type go
Length of output: 52822
binder/form.go (2)
11-14: LGTM: Consistent struct definition with other binders
The FormBinding struct maintains consistency with CookieBinding and QueryBinding.
Line range hint 38-45: Consider consistent behavior between Bind and BindMultipart
The splitting logic is implemented in Bind but not in BindMultipart. This could lead to inconsistent behavior.
Consider implementing the same splitting logic in BindMultipart:
func (b *FormBinding) BindMultipart(reqCtx *fasthttp.RequestCtx, out any) error {
data, err := reqCtx.MultipartForm()
if err != nil {
return err
}
+ // Apply splitting logic to multipart form values
+ if b.EnableSplitting {
+ processedData := make(map[string][]string)
+ for k, vals := range data.Value {
+ for _, v := range vals {
+ if strings.Contains(v, ",") && equalFieldType(out, reflect.Slice, k) {
+ values := strings.Split(v, ",")
+ processedData[k] = append(processedData[k], values...)
+ } else {
+ processedData[k] = append(processedData[k], v)
+ }
+ }
+ }
+ return parse(b.Name(), out, processedData)
+ }
return parse(b.Name(), out, data.Value)
}bind.go (1)
82-89: LGTM! Good resource management with binder pooling.
The implementation properly manages binder lifecycle:
- Gets binder from pool
- Configures EnableSplitting
- Ensures cleanup in defer block
bind_test.go (1)
325-325: Verify the reduced Hobby array length is intentional.
The test expects shorter array lengths after enabling splitting. Please confirm this is the intended behavior of the EnableSplittingOnParsers feature.
Also applies to: 331-331, 393-393, 399-399
✅ Verification successful
Let me gather more context about the test file and the splitting behavior.
Let me check the documentation and implementation of EnableSplittingOnParsers to understand the intended behavior.
Based on the documentation and implementation details gathered, I can now provide the final response:
The reduced array length is expected with EnableSplittingOnParsers enabled
The test is correctly verifying the behavior of the Fiber framework's binding functionality. When EnableSplittingOnParsers is set to true in the app config (as shown in the test file), comma-separated values in headers are not split into arrays. This is the intended behavior as evidenced by the test setup and documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this behavior is documented or mentioned in issues
gh issue view 3223
Length of output: 3171
Script:
#!/bin/bash
# Check the test file content around the mentioned lines
rg -B 10 -A 10 "require.Len\(t, q.Hobby, 1\)" bind_test.go
# Look for any related test cases or documentation about splitting
rg -l "EnableSplittingOnParsers" .
Length of output: 886
Script:
#!/bin/bash
# Check documentation about EnableSplittingOnParsers
cat docs/api/bind.md
# Look for the implementation and related tests
rg -A 5 -B 5 "EnableSplittingOnParsers" bind.go bind_test.go
Length of output: 22910
| func GetFromThePool[T any](pool *sync.Pool) T { | ||
| binder, ok := pool.Get().(T) | ||
| if !ok { | ||
| panic(errors.New("failed to type-assert to T")) | ||
| } | ||
|
|
||
| return binder | ||
| } |
There was a problem hiding this comment.
Avoid panicking on type assertion failure in GetFromThePool
Using panic on type assertion failures can lead to unexpected crashes. Consider returning an error instead to handle such cases gracefully.
Suggested change:
-func GetFromThePool[T any](pool *sync.Pool) T {
+func GetFromThePool[T any](pool *sync.Pool) (T, error) {
binder, ok := pool.Get().(T)
if !ok {
- panic(errors.New("failed to type-assert to T"))
+ var zero T
+ return zero, errors.New("failed to type-assert to T")
}
- return binder
+ return binder, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func GetFromThePool[T any](pool *sync.Pool) T { | |
| binder, ok := pool.Get().(T) | |
| if !ok { | |
| panic(errors.New("failed to type-assert to T")) | |
| } | |
| return binder | |
| } | |
| func GetFromThePool[T any](pool *sync.Pool) (T, error) { | |
| binder, ok := pool.Get().(T) | |
| if !ok { | |
| var zero T | |
| return zero, errors.New("failed to type-assert to T") | |
| } | |
| return binder, nil | |
| } |
| var HeaderBinderPool = sync.Pool{ | ||
| New: func() any { | ||
| return &HeaderBinding{} | ||
| }, | ||
| } | ||
|
|
||
| var RespHeaderBinderPool = sync.Pool{ | ||
| New: func() any { | ||
| return &RespHeaderBinding{} | ||
| }, | ||
| } | ||
|
|
||
| var CookieBinderPool = sync.Pool{ | ||
| New: func() any { | ||
| return &CookieBinding{} | ||
| }, | ||
| } | ||
|
|
||
| var QueryBinderPool = sync.Pool{ | ||
| New: func() any { | ||
| return &QueryBinding{} | ||
| }, | ||
| } | ||
|
|
||
| var FormBinderPool = sync.Pool{ | ||
| New: func() any { | ||
| return &FormBinding{} | ||
| }, | ||
| } | ||
|
|
||
| var URIBinderPool = sync.Pool{ | ||
| New: func() any { | ||
| return &URIBinding{} | ||
| }, | ||
| } | ||
|
|
||
| var XMLBinderPool = sync.Pool{ | ||
| New: func() any { | ||
| return &XMLBinding{} | ||
| }, | ||
| } | ||
|
|
||
| var JSONBinderPool = sync.Pool{ | ||
| New: func() any { | ||
| return &JSONBinding{} | ||
| }, | ||
| } | ||
|
|
||
| var CBORBinderPool = sync.Pool{ | ||
| New: func() any { | ||
| return &CBORBinding{} | ||
| }, | ||
| } |
There was a problem hiding this comment.
Initialize decoder fields in binder instances to avoid nil pointer dereferences
The JSONDecoder, XMLDecoder, and CBORDecoder fields in JSONBinding, XMLBinding, and CBORBinding are not initialized, which may lead to nil pointer dereferences when their Bind methods are called. Ensure these fields are properly initialized when creating binder instances.
For example, initialize JSONDecoder in JSONBinding:
return &JSONBinding{
+ JSONDecoder: utils.JSONUnmarshalDefault,
}Similarly, initialize XMLDecoder and CBORDecoder:
// For XMLBinding
return &XMLBinding{
+ XMLDecoder: utils.XMLUnmarshalDefault,
}
// For CBORBinding
return &CBORBinding{
+ CBORDecoder: utils.CBORUnmarshalDefault,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var HeaderBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &HeaderBinding{} | |
| }, | |
| } | |
| var RespHeaderBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &RespHeaderBinding{} | |
| }, | |
| } | |
| var CookieBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &CookieBinding{} | |
| }, | |
| } | |
| var QueryBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &QueryBinding{} | |
| }, | |
| } | |
| var FormBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &FormBinding{} | |
| }, | |
| } | |
| var URIBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &URIBinding{} | |
| }, | |
| } | |
| var XMLBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &XMLBinding{} | |
| }, | |
| } | |
| var JSONBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &JSONBinding{} | |
| }, | |
| } | |
| var CBORBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &CBORBinding{} | |
| }, | |
| } | |
| var HeaderBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &HeaderBinding{} | |
| }, | |
| } | |
| var RespHeaderBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &RespHeaderBinding{} | |
| }, | |
| } | |
| var CookieBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &CookieBinding{} | |
| }, | |
| } | |
| var QueryBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &QueryBinding{} | |
| }, | |
| } | |
| var FormBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &FormBinding{} | |
| }, | |
| } | |
| var URIBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &URIBinding{} | |
| }, | |
| } | |
| var XMLBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &XMLBinding{ | |
| XMLDecoder: utils.XMLUnmarshalDefault, | |
| } | |
| }, | |
| } | |
| var JSONBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &JSONBinding{ | |
| JSONDecoder: utils.JSONUnmarshalDefault, | |
| } | |
| }, | |
| } | |
| var CBORBinderPool = sync.Pool{ | |
| New: func() any { | |
| return &CBORBinding{ | |
| CBORDecoder: utils.CBORUnmarshalDefault, | |
| } | |
| }, | |
| } |
| // JSONBinding is the JSON binder for JSON request body. | ||
| type JSONBinding struct { | ||
| JSONDecoder utils.JSONUnmarshal | ||
| } |
There was a problem hiding this comment.
Initialize JSONDecoder field in JSONBinding to prevent nil pointer dereference
The JSONDecoder field is not initialized, which may cause a nil pointer dereference when Bind is called. Ensure that JSONDecoder is set to a valid unmarshalling function.
Suggested change:
type JSONBinding struct {
- JSONDecoder utils.JSONUnmarshal
+ JSONDecoder utils.JSONUnmarshal
}
+func NewJSONBinding() *JSONBinding {
+ return &JSONBinding{
+ JSONDecoder: utils.JSONUnmarshalDefault,
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
| // CBORBinding is the CBOR binder for CBOR request body. | ||
| type CBORBinding struct { | ||
| CBORDecoder utils.CBORUnmarshal | ||
| } |
There was a problem hiding this comment.
Initialize CBORDecoder field in CBORBinding to prevent nil pointer dereference
The CBORDecoder field is not initialized, which may cause a nil pointer dereference when Bind is called. Ensure that CBORDecoder is set to a valid unmarshalling function.
Suggested change:
type CBORBinding struct {
- CBORDecoder utils.CBORUnmarshal
+ CBORDecoder utils.CBORUnmarshal
}
+func NewCBORBinding() *CBORBinding {
+ return &CBORBinding{
+ CBORDecoder: utils.CBORUnmarshalDefault,
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
| // XMLBinding is the XML binder for XML request body. | ||
| type XMLBinding struct { | ||
| XMLDecoder utils.XMLUnmarshal | ||
| } |
There was a problem hiding this comment.
Initialize XMLDecoder field in XMLBinding to prevent nil pointer dereference
The XMLDecoder field is not initialized, which may cause a nil pointer dereference when Bind is called. Ensure that XMLDecoder is set to a valid unmarshalling function.
Suggested change:
type XMLBinding struct {
- XMLDecoder utils.XMLUnmarshal
+ XMLDecoder utils.XMLUnmarshal
}
+func NewXMLBinding() *XMLBinding {
+ return &XMLBinding{
+ XMLDecoder: utils.XMLUnmarshalDefault,
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
![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: a56943d | Previous: 89a0cd3 | Ratio |
|---|---|---|---|
Benchmark_Compress_Levels_Parallel/Brotli_LevelBestCompression - B/op |
1 B/op |
0 B/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3231 +/- ##
==========================================
+ Coverage 82.72% 84.27% +1.54%
==========================================
Files 115 116 +1
Lines 11365 11485 +120
==========================================
+ Hits 9402 9679 +277
+ Misses 1557 1389 -168
- Partials 406 417 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (14)
binder/binder_test.go (1)
9-28: Enhance test coverage for pool managementWhile the basic pool functionality is tested, consider adding the following test cases:
- Verify binder state after pool operations
- Test concurrent access to pools
- Add negative test cases for invalid pool operations
- Validate binder reuse functionality
Example test cases to add:
// Test binder state binder := GetFromThePool[*HeaderBinding](&HeaderBinderPool) // Verify initial state require.False(t, binder.EnableSplitting) // Modify state binder.EnableSplitting = true // Return to pool PutToThePool(&HeaderBinderPool, binder) // Get again and verify reset state newBinder := GetFromThePool[*HeaderBinding](&HeaderBinderPool) require.False(t, newBinder.EnableSplitting) // Test concurrent access var wg sync.WaitGroup for i := 0; i < 100; i++ { wg.Add(1) go func() { defer wg.Done() binder := GetFromThePool[*HeaderBinding](&HeaderBinderPool) PutToThePool(&HeaderBinderPool, binder) }() } wg.Wait()binder/query.go (1)
11-14: Document EnableSplitting field behaviorAdd documentation to explain the default value and behavior of EnableSplitting field.
// QueryBinding is the query binder for query request body. type QueryBinding struct { + // EnableSplitting controls whether comma-separated values in query parameters + // should be split into slices. When false, values are treated as single strings. EnableSplitting bool }binder/uri_test.go (2)
38-38: Remove debug print statementRemove the fmt.Println statement from the test code.
- fmt.Println(user)
46-75: Enhance benchmark coverageThe current benchmark only tests a single scenario. Consider adding:
- Benchmarks with different payload sizes
- Benchmarks with/without EnableSplitting
- Benchmarks for error cases
Example benchmark cases:
func Benchmark_URIBinding_WithSplitting(b *testing.B) { binder := &URIBinding{EnableSplitting: true} // ... similar setup ... } func Benchmark_URIBinding_LargePayload(b *testing.B) { // ... setup with many fields ... }binder/json_test.go (1)
10-38: Enhance test coverage with error cases and edge scenariosWhile the happy path is well tested, consider adding test cases for:
- Invalid JSON input
- Empty input
- Null values
- Type mismatches
- Missing required fields
func Test_JSON_Binding_Bind(t *testing.T) { t.Parallel() + testCases := []struct { + name string + input string + wantErr bool + }{ + { + name: "invalid json", + input: `{"name":}`, + wantErr: true, + }, + { + name: "empty input", + input: "", + wantErr: true, + }, + // Add more test cases + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + // Test implementation + }) + }binder/query_test.go (2)
10-49: Add test cases for malformed query stringsThe test should verify proper handling of malformed query strings and invalid values.
func Test_QueryBinder_Bind(t *testing.T) { + t.Run("malformed query string", func(t *testing.T) { + t.Parallel() + req := fasthttp.AcquireRequest() + req.URI().SetQueryString("age=not_a_number&posts[invalid]=value") + t.Cleanup(func() { + fasthttp.ReleaseRequest(req) + }) + + var user User + err := b.Bind(req, &user) + require.Error(t, err) + })
31-33: Consider testing URL encoding in query parametersThe test should verify proper handling of URL-encoded characters in query parameters.
req := fasthttp.AcquireRequest() -req.URI().SetQueryString("name=john&names=john,doe&age=42&posts[0][title]=post1&posts[1][title]=post2&posts[2][title]=post3") +req.URI().SetQueryString("name=john+doe&names=john%2Cdoe&age=42&posts[0][title]=post+1&posts[1][title]=post%202&posts[2][title]=post%2C3")binder/resp_header_test.go (2)
43-76: Remove redundant timer reset in benchmarkThe benchmark contains redundant timer operations:
- Line 45:
b.ResetTimer()- Line 68:
b.StartTimer()The second reset at line 68 makes the first one unnecessary.
func Benchmark_RespHeaderBinder_Bind(b *testing.B) { b.ReportAllocs() - b.ResetTimer() binder := &RespHeaderBinding{ EnableSplitting: true, }
10-41: Enhance test coverage with edge casesThe test should include additional scenarios:
- Empty header values
- Invalid integer for age
- Headers with special characters
- Missing required headers
Here's a suggested addition to the test:
func Test_RespHeaderBinder_Bind_EdgeCases(t *testing.T) { t.Parallel() b := &RespHeaderBinding{ EnableSplitting: true, } type User struct { Name string `respHeader:"name"` Age int `respHeader:"age"` Posts []string `respHeader:"posts"` } testCases := []struct { name string headers map[string]string wantErr bool }{ { name: "empty_values", headers: map[string]string{ "name": "", "age": "", }, wantErr: true, }, { name: "invalid_age", headers: map[string]string{ "name": "john", "age": "not_a_number", }, wantErr: true, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { var user User resp := fasthttp.AcquireResponse() defer fasthttp.ReleaseResponse(resp) for k, v := range tc.headers { resp.Header.Set(k, v) } err := b.Bind(resp, &user) if tc.wantErr { require.Error(t, err) } else { require.NoError(t, err) } }) } }binder/cbor_test.go (1)
61-90: Enhance benchmark coverageThe benchmark only tests a simple case. Consider adding benchmarks for:
- Different payload sizes
- Complex nested structures
- Array types
Here's a suggested addition:
func Benchmark_CBORBinder_Bind_Complex(b *testing.B) { b.ReportAllocs() binder := &CBORBinding{ CBORDecoder: cbor.Unmarshal, } type ComplexUser struct { Name string `cbor:"name"` Age int `cbor:"age"` Tags []string `cbor:"tags"` Metadata map[string]interface{} `cbor:"metadata"` } wantedUser := ComplexUser{ Name: "john", Age: 42, Tags: []string{"tag1", "tag2", "tag3"}, Metadata: map[string]interface{}{ "key1": "value1", "key2": 123, }, } body, err := cbor.Marshal(wantedUser) require.NoError(b, err) var user ComplexUser b.ResetTimer() for i := 0; i < b.N; i++ { _ = binder.Bind(body, &user) } }binder/xml_test.go (2)
10-57: Consider adding more edge cases to Test_XMLBinding_BindWhile the current test covers the basic functionality well, consider adding these test cases:
- XML with attributes
- Empty XML elements
- XML with CDATA sections
- XML with special characters
func Test_XMLBinding_Bind(t *testing.T) { t.Parallel() + testCases := []struct { + name string + input string + expected User + wantErr bool + }{ + { + name: "with attributes", + input: `<user id="123"><name>john</name></user>`, + expected: User{Name: "john"}, + }, + { + name: "with CDATA", + input: `<user><name><![CDATA[john & doe]]></name></user>`, + expected: User{Name: "john & doe"}, + }, + }
82-134: Optimize benchmark implementationA few suggestions to improve the benchmark:
- The new User creation at line 124 is redundant as it's after the benchmark loop
- Consider varying input sizes to measure performance characteristics
func Benchmark_XMLBinding_Bind(b *testing.B) { b.ReportAllocs() - user := new(User) + testSizes := []int{1, 10, 100} // Test different input sizes + for _, size := range testSizes { + b.Run(fmt.Sprintf("size_%d", size), func(b *testing.B) { + user := new(User) + data := generateTestXML(size) // Helper to generate XML of different sizes + b.ResetTimer() + for i := 0; i < b.N; i++ { + _ = binder.Bind(data, user) + } + }) + } - user = new(User) // Remove redundant creationbinder/mapping_test.go (2)
117-149: Add edge cases to parseToMap testsConsider adding these test cases:
- Empty input map
- Case sensitivity in map keys
- Special characters in keys
- Empty values
func Test_parseToMap(t *testing.T) { + t.Run("empty_input", func(t *testing.T) { + m := make(map[string]string) + err := parseToMap(m, map[string][]string{}) + require.NoError(t, err) + require.Empty(t, m) + }) + + t.Run("case_sensitivity", func(t *testing.T) { + input := map[string][]string{ + "Key": {"value1"}, + "key": {"value2"}, + } + m := make(map[string]string) + err := parseToMap(m, input) + require.NoError(t, err) + require.Len(t, m, 2) + })
151-181: Add error cases to FilterFlags testsConsider adding these test cases:
- Empty input string
- Invalid content type format
- Multiple semicolons
- Malformed parameters
func Test_FilterFlags(t *testing.T) { tests := []struct { input string expected string }{ + { + input: "", + expected: "", + }, + { + input: "text/javascript;;;", + expected: "text/javascript", + }, + { + input: "text/javascript; charset=", + expected: "text/javascript", + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
app.go(2 hunks)bind.go(9 hunks)binder/binder_test.go(1 hunks)binder/cbor_test.go(1 hunks)binder/cookie.go(1 hunks)binder/cookie_test.go(1 hunks)binder/form.go(3 hunks)binder/form_test.go(1 hunks)binder/header_test.go(1 hunks)binder/json_test.go(1 hunks)binder/mapping.go(4 hunks)binder/mapping_test.go(2 hunks)binder/query.go(2 hunks)binder/query_test.go(1 hunks)binder/resp_header_test.go(1 hunks)binder/uri_test.go(1 hunks)binder/xml_test.go(1 hunks)ctx_test.go(1 hunks)docs/api/fiber.md(1 hunks)docs/whats_new.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- binder/cookie.go
- binder/mapping.go
- binder/form.go
🧰 Additional context used
🪛 GitHub Check: unit (1.23.x, macos-13)
bind.go
[failure] 126-126:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 126-126:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 144-144:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 144-144:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 215-215:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 215-215:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 249-249:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.BindMultipart
[failure] 249-249:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.BindMultipart
🪛 GitHub Check: unit (1.23.x, macos-latest)
bind.go
[failure] 126-126:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 126-126:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 144-144:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 144-144:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 215-215:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 215-215:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 249-249:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.BindMultipart
[failure] 249-249:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.BindMultipart
🪛 GitHub Check: unit (1.23.x, ubuntu-latest)
bind.go
[failure] 126-126:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 126-126:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 144-144:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 144-144:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 215-215:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 215-215:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 249-249:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.BindMultipart
[failure] 249-249:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.BindMultipart
🪛 GitHub Check: lint
bind.go
[failure] 126-126:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 126-126:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 126-126:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 144-144:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 144-144:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 144-144:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 215-215:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 215-215:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 249-249:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.BindMultipart (typecheck)
[failure] 249-249:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.BindMultipart (typecheck)
🪛 GitHub Check: govulncheck-check
bind.go
[failure] 126-126:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 144-144:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 215-215:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 249-249:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.BindMultipart
🪛 GitHub Check: Compare
bind.go
[failure] 126-126:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 144-144:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 215-215:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 249-249:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.BindMultipart
🪛 GitHub Check: repeated
bind.go
[failure] 126-126:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 126-126:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 144-144:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 144-144:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 215-215:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 215-215:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.Bind
[failure] 249-249:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.BindMultipart
[failure] 249-249:
cannot use b.ctx.RequestCtx() (value of type *fasthttp.RequestCtx) as *fasthttp.Request value in argument to bind.BindMultipart
🔇 Additional comments (9)
binder/query.go (1)
22-22: Optimize query parameter parsing
The current implementation could be optimized:
- Pre-allocate the data map with expected capacity
- Use strings.Builder for string concatenation
- Consider caching the field type reflection results
-func (b *QueryBinding) Bind(reqCtx *fasthttp.Request, out any) error {
- data := make(map[string][]string)
+func (b *QueryBinding) Bind(reqCtx *fasthttp.Request, out any) error {
+ // Pre-allocate with expected capacity
+ data := make(map[string][]string, reqCtx.URI().QueryArgs().Len())Also, verify the impact of switching from RequestCtx to Request:
Also applies to: 26-44
binder/mapping_test.go (1)
33-46: LGTM! Good test coverage for nested structs
The new test cases thoroughly validate field type equality for nested structs, including proper tag handling and case sensitivity.
bind.go (3)
80-87: LGTM: Consistent binder pool implementation pattern
The implementation follows good practices for object pooling:
- Proper object lifecycle management
- State reset before returning to pool
- Consistent error handling
Also applies to: 98-105, 117-124, 135-142, 153-160, 171-178, 188-195, 206-213, 224-229, 240-247
188-197: LGTM: XML binding implementation
The XML binding implementation:
- Properly integrates configurable XMLDecoder
- Follows consistent pool pattern
- Correctly manages decoder lifecycle
81-82: LGTM: EnableSplitting configuration
The EnableSplittingOnParsers functionality is properly implemented:
- Consistently applied across all relevant binders
- Correctly propagated from app config
- Properly reset before returning to pool
Also applies to: 99-100, 118-119, 136-137, 207-208, 241-242
app.go (1)
344-349: LGTM: XMLDecoder configuration
The XMLDecoder implementation:
- Follows consistent pattern with other decoder configurations
- Properly documented
- Correctly initialized with default xml.Unmarshal
Also applies to: 570-572
docs/whats_new.md (1)
52-52: LGTM: Documentation updates
The documentation accurately reflects the changes:
- XMLDecoder addition properly documented
- EnableSplittingOnParsers changes explained
- Migration guide provided
docs/api/fiber.md (1)
86-86: LGTM! Documentation for XMLDecoder field is well formatted and consistent.
The new field documentation follows the same pattern as other decoder fields and clearly explains its purpose.
ctx_test.go (1)
1436-1438: LGTM! Test configuration properly enables parser splitting.
The test setup is modified to enable splitting on parsers, which aligns with the PR objective to fix this functionality. The test coverage remains comprehensive across different binding scenarios.
binder/uri_test.go
Outdated
| func Test_URIBinding_Bind(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| b := &URIBinding{} | ||
| require.Equal(t, "uri", b.Name()) | ||
|
|
||
| type User struct { | ||
| Name string `uri:"name"` | ||
| Age int `uri:"age"` | ||
|
|
||
| Posts []string `uri:"posts"` | ||
| } | ||
| var user User | ||
|
|
||
| paramsKey := []string{"name", "age", "posts"} | ||
| paramsVals := []string{"john", "42", "post1,post2,post3"} | ||
| paramsFunc := func(key string, defaultValue ...string) string { | ||
| for i, k := range paramsKey { | ||
| if k == key { | ||
| return paramsVals[i] | ||
| } | ||
| } | ||
|
|
||
| return "" | ||
| } | ||
|
|
||
| err := b.Bind(paramsKey, paramsFunc, &user) | ||
|
|
||
| fmt.Println(user) | ||
|
|
||
| require.NoError(t, err) | ||
| require.Equal(t, "john", user.Name) | ||
| require.Equal(t, 42, user.Age) | ||
| require.Equal(t, []string{"post1,post2,post3"}, user.Posts) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance URI binding test coverage
The current test case only covers the happy path. Consider adding:
- Error scenarios (invalid types, missing required fields)
- Edge cases (empty values, special characters)
- Test cases for EnableSplitting behavior
- Validation of binding tags
Example additional test cases:
func Test_URIBinding_Bind_Errors(t *testing.T) {
t.Parallel()
b := &URIBinding{}
// Test invalid type
var invalidType int
err := b.Bind([]string{"key"}, func(string, ...string) string { return "value" }, &invalidType)
require.Error(t, err)
// Test missing required field
type Required struct {
Field string `uri:"field" binding:"required"`
}
var req Required
err = b.Bind([]string{"other"}, func(string, ...string) string { return "" }, &req)
require.Error(t, err)
}
func Test_URIBinding_EnableSplitting(t *testing.T) {
t.Parallel()
b := &URIBinding{EnableSplitting: true}
type WithSlice struct {
Tags []string `uri:"tags"`
}
var data WithSlice
err := b.Bind(
[]string{"tags"},
func(string, ...string) string { return "tag1,tag2,tag3" },
&data,
)
require.NoError(t, err)
require.Equal(t, []string{"tag1", "tag2", "tag3"}, data.Tags)
}
binder/json_test.go
Outdated
| func Benchmark_JSON_Binding_Bind(b *testing.B) { | ||
| b.ReportAllocs() | ||
| b.ResetTimer() | ||
|
|
||
| binder := &JSONBinding{ | ||
| JSONDecoder: json.Unmarshal, | ||
| } | ||
|
|
||
| type User struct { | ||
| Name string `json:"name"` | ||
| Age int `json:"age"` | ||
|
|
||
| Posts []string `json:"posts"` | ||
| } | ||
|
|
||
| var user User | ||
| var err error | ||
| for i := 0; i < b.N; i++ { | ||
| err = binder.Bind([]byte(`{"name":"john","age":42,"posts":["post1","post2","post3"]}`), &user) | ||
| } | ||
|
|
||
| require.NoError(b, err) | ||
| require.Equal(b, "john", user.Name) | ||
| require.Equal(b, 42, user.Age) | ||
| require.Len(b, user.Posts, 3) | ||
| require.Equal(b, "post1", user.Posts[0]) | ||
| require.Equal(b, "post2", user.Posts[1]) | ||
| require.Equal(b, "post3", user.Posts[2]) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for benchmark resources
The benchmark should clean up the user struct between iterations to ensure consistent memory usage measurements.
func Benchmark_JSON_Binding_Bind(b *testing.B) {
b.ReportAllocs()
+ b.StopTimer()
+ user := User{}
+ b.StartTimer()
for i := 0; i < b.N; i++ {
+ b.StopTimer()
+ user = User{} // Reset between iterations
+ b.StartTimer()
err = binder.Bind([]byte(`{"name":"john","age":42,"posts":["post1","post2","post3"]}`), &user)
}Committable suggestion skipped: line range outside the PR's diff.
binder/header_test.go
Outdated
| Name string `query:"name"` | ||
| Age int `query:"age"` | ||
|
|
||
| Posts []string `query:"posts"` | ||
| } |
There was a problem hiding this comment.
Fix incorrect struct tags in benchmark
The benchmark struct uses query tags instead of header tags, which doesn't match the binding type being tested.
type User struct {
- Name string `query:"name"`
- Age int `query:"age"`
- Posts []string `query:"posts"`
+ Name string `header:"name"`
+ Age int `header:"age"`
+ Posts []string `header:"posts"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Name string `query:"name"` | |
| Age int `query:"age"` | |
| Posts []string `query:"posts"` | |
| } | |
| Name string `header:"name"` | |
| Age int `header:"age"` | |
| Posts []string `header:"posts"` | |
| } |
efectn
force-pushed
the
enablesplitting-fix
branch
from
b5cc3a6 to
043dd20
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
binder/header_test.go (1)
10-47:⚠️ Potential issueAdd test cases for disabled splitting functionality
Since this PR specifically fixes the
EnableSplittingOnParsersfunctionality, we should test both enabled and disabled states to ensure the fix works correctly.Convert the test to use table-driven tests to cover both cases:
func Test_HeaderBinder_Bind(t *testing.T) { t.Parallel() - b := &HeaderBinding{ - EnableSplitting: true, - } - require.Equal(t, "header", b.Name()) + testCases := []struct { + name string + enableSplitting bool + headers map[string]string + expected User + }{ + { + name: "with splitting enabled", + enableSplitting: true, + headers: map[string]string{ + "name": "john", + "names": "john,doe", + "age": "42", + "posts": "post1,post2,post3", + }, + expected: User{ + Name: "john", + Names: []string{"john", "doe"}, + Age: 42, + Posts: []string{"post1", "post2", "post3"}, + }, + }, + { + name: "with splitting disabled", + enableSplitting: false, + headers: map[string]string{ + "name": "john", + "names": "john,doe", + "age": "42", + "posts": "post1,post2,post3", + }, + expected: User{ + Name: "john", + Names: []string{"john,doe"}, + Age: 42, + Posts: []string{"post1,post2,post3"}, + }, + }, + } + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + b := &HeaderBinding{ + EnableSplitting: tc.enableSplitting, + } + require.Equal(t, "header", b.Name()) + + var user User + req := fasthttp.AcquireRequest() + t.Cleanup(func() { + fasthttp.ReleaseRequest(req) + }) + + for k, v := range tc.headers { + req.Header.Set(k, v) + } + + err := b.Bind(req, &user) + require.NoError(t, err) + require.Equal(t, tc.expected, user) + }) + }
🧹 Nitpick comments (4)
binder/mapping_test.go (2)
115-147: Comprehensive coverage for parseToMap.Testing multiple map types (map[string]string, map[string][]string) and asserting the error for map[string]any is a thorough approach. Consider adding an empty map scenario to ensure no edge cases slip through.
149-179: FilterFlags test is well-designed.It covers multiple variations of MIME content strings. You may add a “trim/no content-type” scenario for completeness, but overall, this is solid coverage.
binder/form_test.go (1)
53-83: QueryBinding used in a form test benchmark.Although it’s a “Form” file, testing query parameters is valid. Consider clarifying naming or location if you want to keep form-specific logic separate from query-based tests.
binder/mapping.go (1)
229-229: Switched to static “query” tag retrieval.Relying on a literal “query” string is simpler than referencing Binder.Name(), but ensure consistency across any custom tags or dynamic binder usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
bind.go(9 hunks)binder/binder_test.go(1 hunks)binder/cbor_test.go(1 hunks)binder/cookie.go(1 hunks)binder/cookie_test.go(1 hunks)binder/form.go(3 hunks)binder/form_test.go(1 hunks)binder/header_test.go(1 hunks)binder/json_test.go(1 hunks)binder/mapping.go(4 hunks)binder/mapping_test.go(2 hunks)binder/query.go(2 hunks)binder/query_test.go(1 hunks)binder/resp_header_test.go(1 hunks)binder/uri_test.go(1 hunks)binder/xml_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- binder/binder_test.go
- binder/uri_test.go
- binder/json_test.go
- binder/query_test.go
- binder/resp_header_test.go
- binder/cbor_test.go
- binder/xml_test.go
- binder/cookie_test.go
- binder/query.go
- binder/cookie.go
- binder/form.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
bind.go
[warning] 224-229: bind.go#L224-L229
Added lines #L224 - L229 were not covered by tests
[warning] 231-231: bind.go#L231
Added line #L231 was not covered by tests
binder/mapping.go
[warning] 129-130: binder/mapping.go#L129-L130
Added lines #L129 - L130 were not covered by tests
🔇 Additional comments (17)
bind.go (10)
80-89: Pooling pattern for HeaderBinding looks good.
The use of Get/Put from the pool, combined with a defer resetting “EnableSplitting” to false, prevents stale state from affecting subsequent bindings. The current approach is succinct and avoids excessive overhead. Well done!
98-106: Same pooling pattern for RespHeaderBinding is well-implemented.
Great consistency in retrieving/returning the binder and resetting “EnableSplitting” after usage. Maintain this approach for other bindings as well.
117-125: Cookie binder usage is correct.
Passing “&b.ctx.RequestCtx().Request” to bind avoids the past type mismatch issues. This matches the binder’s requirements without any sign of misalignment.
135-143: Query binder blocks potential concurrency pitfalls.
Retrieving from the pool, using it, then resetting in a defer ensures thread safety as no leftover state persists in the binder object.
153-161: Cleaner JSONBinding usage with JSONDecoder assignment.
Assigning the custom decoder, then unassigning in the defer block, is an excellent pattern, minimizing side effects for subsequent calls.
171-179: Proper handling of CBORDecoder in CBORBinding.
Mirrors the JSONBinding approach nicely, ensuring consistent resource cleanup in the defer.
188-196: XMLBinding usage is consistent with other bindings.
You’ve neatly followed the pattern for assigning and clearing “XMLDecoder” within the same invocation context.
206-214: FormBinding pool usage is tidy and consistent.
Setting “EnableSplitting” then resetting is clear, consistent, and easy to maintain.
240-248: MultipartForm binder logic parallels Form usage nicely.
Acquiring the pool, setting “EnableSplitting,” and returning it after usage is consistent and maintainable.
224-231: Add test coverage for URI binding.
Static analysis shows that lines 224-229 and line 231 lack test coverage. Consider adding a dedicated test scenario (e.g., a sample route parameter scenario with typical or edge cases) to ensure robust coverage.
Use a script to locate the existing or potential new tests for URI binding:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 224-229: bind.go#L224-L229
Added lines #L224 - L229 were not covered by tests
[warning] 231-231: bind.go#L231
Added line #L231 was not covered by tests
binder/mapping_test.go (1)
32-46: Nested field checks strengthen test robustness.
Verifying nested struct fields with “user2” ensures the “equalFieldType” function handles deeper nesting properly.
binder/form_test.go (3)
1-51: FormBinder_Bind test demonstrates correct usage of “EnableSplitting.”
The coverage of slices, nested slices, and integer fields is solid. The cleanup approach for the fasthttp request ensures no resource leaks.
85-125: FormBinder_BindMultipart test is thorough.
Multipart usage is validated effectively. The final checks on user fields confirm the correctness of splitting and binding separate fields.
127-168: Benchmark_FormBinder_BindMultipart usage is correct.
Benchmarks appear well-structured. The explicit body construction and repeated binding align with performance testing best practices.
binder/mapping.go (3)
35-35: Explicit static list for binder tags.
Shifting from dynamically invoking Binder.Name() to a static list reduces complexity and avoids potential side effects.
Line range hint 110-134: Enhanced parseToMap logic.
Allowing for both map[string][]string and map[string]string simplifies usage. The error for “map[string]any” is also clear.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 129-130: binder/mapping.go#L129-L130
Added lines #L129 - L130 were not covered by tests
129-130: Test coverage gap for empty-value handling.
Static analysis indicates these lines are not tested. Consider adding a scenario verifying how the function behaves when “v” is an empty slice.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 129-130: binder/mapping.go#L129-L130
Added lines #L129 - L130 were not covered by tests
efectn
force-pushed
the
enablesplitting-fix
branch
from
eed3d7f to
d2d2ab7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
binder/header_test.go (1)
49-85: 🛠️ Refactor suggestionImprove benchmark coverage and accuracy
The benchmark needs improvements in several areas:
- Add benchmark cases for EnableSplitting=false
- Remove assertions from benchmark measurements
This feedback was previously provided in past reviews. The suggested improvements remain valid:
- Use sub-benchmarks for different configurations
- Move assertions outside the timing measurements
- Consider benchmarking with different payload sizes
🧹 Nitpick comments (1)
binder/cookie_test.go (1)
10-47: Well-structured test with good practices!The test demonstrates good practices:
- Parallel execution enabled
- Proper resource cleanup
- Comprehensive assertions
Consider adding these edge cases:
func Test_CookieBinder_Bind(t *testing.T) { // ... existing test code ... + t.Run("empty_values", func(t *testing.T) { + req.Header.SetCookie("names", "") + err := b.Bind(req, &user) + require.NoError(t, err) + require.Empty(t, user.Names) + }) + + t.Run("malformed_values", func(t *testing.T) { + req.Header.SetCookie("age", "not_a_number") + err := b.Bind(req, &user) + require.Error(t, err) + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
binder/cookie_test.go(1 hunks)binder/form_test.go(1 hunks)binder/header_test.go(1 hunks)binder/query_test.go(1 hunks)docs/api/fiber.md(1 hunks)docs/whats_new.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- binder/query_test.go
- binder/form_test.go
🔇 Additional comments (6)
docs/whats_new.md (2)
52-52: Documentation accurately describes the new XMLDecoder property.
The addition of the XMLDecoder property is well documented and aligns with the PR objective to support external XML decoders.
Line range hint 86-86: Documentation for EnableSplittingOnParsers is clear and accurate.
The documentation effectively explains the functionality with a practical example showing how comma-separated values are handled.
docs/api/fiber.md (1)
86-86: API documentation is consistent with the changes.
The XMLDecoder field is properly documented in the Config fields table with appropriate type and description.
binder/cookie_test.go (2)
1-9: LGTM!
The package declaration and imports are appropriate for the testing requirements.
57-61: Fix inconsistent struct tags in benchmark
The benchmark's User struct uses query tags instead of form tags that are used in the test.
binder/header_test.go (1)
1-8: LGTM!
The package declaration and imports are appropriate for the test requirements.
| func Benchmark_CookieBinder_Bind(b *testing.B) { | ||
| b.ReportAllocs() | ||
| b.ResetTimer() | ||
|
|
||
| binder := &CookieBinding{ | ||
| EnableSplitting: true, | ||
| } | ||
|
|
||
| type User struct { | ||
| Name string `query:"name"` | ||
| Posts []string `query:"posts"` | ||
| Age int `query:"age"` | ||
| } | ||
| var user User | ||
|
|
||
| req := fasthttp.AcquireRequest() | ||
| b.Cleanup(func() { | ||
| fasthttp.ReleaseRequest(req) | ||
| }) | ||
|
|
||
| req.Header.SetCookie("name", "john") | ||
| req.Header.SetCookie("age", "42") | ||
| req.Header.SetCookie("posts", "post1,post2,post3") | ||
|
|
||
| b.ResetTimer() | ||
|
|
||
| var err error | ||
| for i := 0; i < b.N; i++ { | ||
| err = binder.Bind(req, &user) | ||
| } | ||
|
|
||
| require.NoError(b, err) | ||
| require.Equal(b, "john", user.Name) | ||
| require.Equal(b, 42, user.Age) | ||
| require.Len(b, user.Posts, 3) | ||
| require.Contains(b, user.Posts, "post1") | ||
| require.Contains(b, user.Posts, "post2") | ||
| require.Contains(b, user.Posts, "post3") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve benchmark structure and error handling
The benchmark has some structural issues:
- Double
b.ResetTimer()calls (lines 51 and 73) - Missing validation of initial binding before the benchmark loop
Apply this improvement:
func Benchmark_CookieBinder_Bind(b *testing.B) {
b.ReportAllocs()
- b.ResetTimer()
// ... setup code ...
+ // Validate initial binding works
+ err := binder.Bind(req, &user)
+ require.NoError(b, err)
+ require.Equal(b, "john", user.Name)
+
b.ResetTimer()
- var err error
for i := 0; i < b.N; i++ {
err = binder.Bind(req, &user)
}
// Move assertions before ResetTimer or remove them
- require.NoError(b, err)
- require.Equal(b, "john", user.Name)
- require.Equal(b, 42, user.Age)
- require.Len(b, user.Posts, 3)
- require.Contains(b, user.Posts, "post1")
- require.Contains(b, user.Posts, "post2")
- require.Contains(b, user.Posts, "post3")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func Benchmark_CookieBinder_Bind(b *testing.B) { | |
| b.ReportAllocs() | |
| b.ResetTimer() | |
| binder := &CookieBinding{ | |
| EnableSplitting: true, | |
| } | |
| type User struct { | |
| Name string `query:"name"` | |
| Posts []string `query:"posts"` | |
| Age int `query:"age"` | |
| } | |
| var user User | |
| req := fasthttp.AcquireRequest() | |
| b.Cleanup(func() { | |
| fasthttp.ReleaseRequest(req) | |
| }) | |
| req.Header.SetCookie("name", "john") | |
| req.Header.SetCookie("age", "42") | |
| req.Header.SetCookie("posts", "post1,post2,post3") | |
| b.ResetTimer() | |
| var err error | |
| for i := 0; i < b.N; i++ { | |
| err = binder.Bind(req, &user) | |
| } | |
| require.NoError(b, err) | |
| require.Equal(b, "john", user.Name) | |
| require.Equal(b, 42, user.Age) | |
| require.Len(b, user.Posts, 3) | |
| require.Contains(b, user.Posts, "post1") | |
| require.Contains(b, user.Posts, "post2") | |
| require.Contains(b, user.Posts, "post3") | |
| } | |
| func Benchmark_CookieBinder_Bind(b *testing.B) { | |
| b.ReportAllocs() | |
| binder := &CookieBinding{ | |
| EnableSplitting: true, | |
| } | |
| type User struct { | |
| Name string `query:"name"` | |
| Posts []string `query:"posts"` | |
| Age int `query:"age"` | |
| } | |
| var user User | |
| req := fasthttp.AcquireRequest() | |
| b.Cleanup(func() { | |
| fasthttp.ReleaseRequest(req) | |
| }) | |
| req.Header.SetCookie("name", "john") | |
| req.Header.SetCookie("age", "42") | |
| req.Header.SetCookie("posts", "post1,post2,post3") | |
| // Validate initial binding works | |
| err := binder.Bind(req, &user) | |
| require.NoError(b, err) | |
| require.Equal(b, "john", user.Name) | |
| b.ResetTimer() | |
| for i := 0; i < b.N; i++ { | |
| err = binder.Bind(req, &user) | |
| } | |
| } |
binder/header_test.go
Outdated
| func Test_HeaderBinder_Bind(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| b := &HeaderBinding{ | ||
| EnableSplitting: true, | ||
| } | ||
| require.Equal(t, "header", b.Name()) | ||
|
|
||
| type User struct { | ||
| Name string `header:"Name"` | ||
| Names []string `header:"Names"` | ||
| Posts []string `header:"Posts"` | ||
| Age int `header:"Age"` | ||
| } | ||
| var user User | ||
|
|
||
| req := fasthttp.AcquireRequest() | ||
| req.Header.Set("name", "john") | ||
| req.Header.Set("names", "john,doe") | ||
| req.Header.Set("age", "42") | ||
| req.Header.Set("posts", "post1,post2,post3") | ||
|
|
||
| t.Cleanup(func() { | ||
| fasthttp.ReleaseRequest(req) | ||
| }) | ||
|
|
||
| err := b.Bind(req, &user) | ||
|
|
||
| require.NoError(t, err) | ||
| require.Equal(t, "john", user.Name) | ||
| require.Equal(t, 42, user.Age) | ||
| require.Len(t, user.Posts, 3) | ||
| require.Equal(t, "post1", user.Posts[0]) | ||
| require.Equal(t, "post2", user.Posts[1]) | ||
| require.Equal(t, "post3", user.Posts[2]) | ||
| require.Contains(t, user.Names, "john") | ||
| require.Contains(t, user.Names, "doe") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add comprehensive test coverage
The test function needs additional test cases to ensure robust functionality:
- Test with EnableSplitting=false (as mentioned in past reviews)
- Test error scenarios (invalid int value for Age)
- Test empty and missing headers
- Test invalid header values
Consider refactoring the test to use table-driven tests:
func Test_HeaderBinder_Bind(t *testing.T) {
t.Parallel()
- b := &HeaderBinding{
- EnableSplitting: true,
- }
- require.Equal(t, "header", b.Name())
+ tests := []struct {
+ name string
+ enableSplitting bool
+ headers map[string]string
+ want User
+ wantErr bool
+ }{
+ {
+ name: "valid headers with splitting enabled",
+ enableSplitting: true,
+ headers: map[string]string{
+ "name": "john",
+ "names": "john,doe",
+ "age": "42",
+ "posts": "post1,post2,post3",
+ },
+ want: User{
+ Name: "john",
+ Names: []string{"john", "doe"},
+ Age: 42,
+ Posts: []string{"post1", "post2", "post3"},
+ },
+ },
+ {
+ name: "valid headers with splitting disabled",
+ enableSplitting: false,
+ headers: map[string]string{
+ "name": "john",
+ "names": "john,doe",
+ "age": "42",
+ "posts": "post1,post2,post3",
+ },
+ want: User{
+ Name: "john",
+ Names: []string{"john,doe"},
+ Age: 42,
+ Posts: []string{"post1,post2,post3"},
+ },
+ },
+ {
+ name: "invalid age",
+ enableSplitting: true,
+ headers: map[string]string{
+ "age": "invalid",
+ },
+ wantErr: true,
+ },
+ // Add more test cases
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ t.Parallel()
+ b := &HeaderBinding{
+ EnableSplitting: tt.enableSplitting,
+ }
+ var user User
+ req := fasthttp.AcquireRequest()
+ t.Cleanup(func() {
+ fasthttp.ReleaseRequest(req)
+ })
+
+ for k, v := range tt.headers {
+ req.Header.Set(k, v)
+ }
+
+ err := b.Bind(req, &user)
+ if tt.wantErr {
+ require.Error(t, err)
+ return
+ }
+ require.NoError(t, err)
+ require.Equal(t, tt.want, user)
+ })
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func Test_HeaderBinder_Bind(t *testing.T) { | |
| t.Parallel() | |
| b := &HeaderBinding{ | |
| EnableSplitting: true, | |
| } | |
| require.Equal(t, "header", b.Name()) | |
| type User struct { | |
| Name string `header:"Name"` | |
| Names []string `header:"Names"` | |
| Posts []string `header:"Posts"` | |
| Age int `header:"Age"` | |
| } | |
| var user User | |
| req := fasthttp.AcquireRequest() | |
| req.Header.Set("name", "john") | |
| req.Header.Set("names", "john,doe") | |
| req.Header.Set("age", "42") | |
| req.Header.Set("posts", "post1,post2,post3") | |
| t.Cleanup(func() { | |
| fasthttp.ReleaseRequest(req) | |
| }) | |
| err := b.Bind(req, &user) | |
| require.NoError(t, err) | |
| require.Equal(t, "john", user.Name) | |
| require.Equal(t, 42, user.Age) | |
| require.Len(t, user.Posts, 3) | |
| require.Equal(t, "post1", user.Posts[0]) | |
| require.Equal(t, "post2", user.Posts[1]) | |
| require.Equal(t, "post3", user.Posts[2]) | |
| require.Contains(t, user.Names, "john") | |
| require.Contains(t, user.Names, "doe") | |
| } | |
| func Test_HeaderBinder_Bind(t *testing.T) { | |
| t.Parallel() | |
| tests := []struct { | |
| name string | |
| enableSplitting bool | |
| headers map[string]string | |
| want User | |
| wantErr bool | |
| }{ | |
| { | |
| name: "valid headers with splitting enabled", | |
| enableSplitting: true, | |
| headers: map[string]string{ | |
| "name": "john", | |
| "names": "john,doe", | |
| "age": "42", | |
| "posts": "post1,post2,post3", | |
| }, | |
| want: User{ | |
| Name: "john", | |
| Names: []string{"john", "doe"}, | |
| Age: 42, | |
| Posts: []string{"post1", "post2", "post3"}, | |
| }, | |
| }, | |
| { | |
| name: "valid headers with splitting disabled", | |
| enableSplitting: false, | |
| headers: map[string]string{ | |
| "name": "john", | |
| "names": "john,doe", | |
| "age": "42", | |
| "posts": "post1,post2,post3", | |
| }, | |
| want: User{ | |
| Name: "john", | |
| Names: []string{"john,doe"}, | |
| Age: 42, | |
| Posts: []string{"post1,post2,post3"}, | |
| }, | |
| }, | |
| { | |
| name: "invalid age", | |
| enableSplitting: true, | |
| headers: map[string]string{ | |
| "age": "invalid", | |
| }, | |
| wantErr: true, | |
| }, | |
| // Add more test cases | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| t.Parallel() | |
| b := &HeaderBinding{ | |
| EnableSplitting: tt.enableSplitting, | |
| } | |
| var user User | |
| req := fasthttp.AcquireRequest() | |
| t.Cleanup(func() { | |
| fasthttp.ReleaseRequest(req) | |
| }) | |
| for k, v := range tt.headers { | |
| req.Header.Set(k, v) | |
| } | |
| err := b.Bind(req, &user) | |
| if tt.wantErr { | |
| require.Error(t, err) | |
| return | |
| } | |
| require.NoError(t, err) | |
| require.Equal(t, tt.want, user) | |
| }) | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
binder/resp_header.go (1)
Line range hint
28-36: Consider extracting common header parsing logicThe header parsing logic is duplicated between
HeaderBindingandRespHeaderBinding.Consider extracting this into a shared utility function to maintain DRY principles and ensure consistent behavior.
+func parseHeaderValue(data map[string][]string, k, v string, enableSplitting bool, out any) { + if enableSplitting && strings.Contains(v, ",") && equalFieldType(out, reflect.Slice, k) { + values := strings.Split(v, ",") + for i := 0; i < len(values); i++ { + data[k] = append(data[k], values[i]) + } + } else { + data[k] = append(data[k], v) + } +}This function could be used in both binding types to reduce code duplication.
♻️ Duplicate comments (6)
binder/json.go (1)
7-10:⚠️ Potential issueInitialize JSONDecoder field to prevent nil pointer dereference
The
JSONDecoderfield needs proper initialization to prevent potential nil pointer dereference in theBindmethod.binder/cbor.go (2)
7-10:⚠️ Potential issueInitialize CBORDecoder field to prevent nil pointer dereference
The
CBORDecoderfield needs proper initialization to prevent potential nil pointer dereference in theBindmethod.
22-25:⚠️ Potential issuePotential race condition in Reset method
Similar to JSONBinding, setting
CBORDecoderto nil before putting the binder back to the pool could cause race conditions.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 23-25: binder/cbor.go#L23-L25
Added lines #L23 - L25 were not covered by testsbind.go (3)
140-140:⚠️ Potential issueUse Request() method for consistency and type safety
Same issue as in Cookie binding - use the abstracted Request() method instead of direct RequestCtx access.
-if err := b.returnErr(bind.Bind(&b.ctx.RequestCtx().Request, out)); err != nil { +if err := b.returnErr(bind.Bind(b.ctx.Request(), out)); err != nil {
207-207:⚠️ Potential issueUse Request() method for consistency and type safety
Same issue as in Cookie and Query bindings - use the abstracted Request() method.
-if err := b.returnErr(bind.Bind(&b.ctx.RequestCtx().Request, out)); err != nil { +if err := b.returnErr(bind.Bind(b.ctx.Request(), out)); err != nil {
240-240:⚠️ Potential issueUse Request() method for consistency and type safety
Same issue as in other bindings - use the abstracted Request() method.
-if err := b.returnErr(bind.BindMultipart(&b.ctx.RequestCtx().Request, out)); err != nil { +if err := b.returnErr(bind.BindMultipart(b.ctx.Request(), out)); err != nil {
🧹 Nitpick comments (8)
binder/uri.go (2)
3-4: Update comment to match new type nameThe comment still references the old type name
uriBinding.-// uriBinding is the URI binder for URI parameters. +// URIBinding is the URI binder for URI parameters.
Line range hint
7-25: Consider implementing a factory pattern for binder initializationTo address the common initialization and concurrency concerns across all binders, consider:
- Implementing a factory pattern to ensure proper initialization of decoder fields
- Using sync.Pool directly instead of custom pooling to handle synchronization
- Adding middleware to manage binder lifecycle (get from pool -> use -> reset -> return to pool)
This would help prevent nil pointer dereferences and race conditions while simplifying the codebase.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-23: binder/uri.go#L22-L23
Added lines #L22 - L23 were not covered by testsbinder/header.go (1)
Line range hint
22-39: Consider pre-allocating the data map capacityThe
datamap is initialized without a capacity hint, which could lead to unnecessary allocations for large headers.Consider pre-allocating the map with an estimated capacity:
func (b *HeaderBinding) Bind(req *fasthttp.Request, out any) error { - data := make(map[string][]string) + data := make(map[string][]string, req.Header.Len()) req.Header.VisitAll(func(key, val []byte) {binder/resp_header.go (1)
Line range hint
22-39: Consider pre-allocating the data map capacitySimilar to the header binding, consider pre-allocating the map capacity based on the response header length.
func (b *RespHeaderBinding) Bind(resp *fasthttp.Response, out any) error { - data := make(map[string][]string) + data := make(map[string][]string, resp.Header.Len()) resp.Header.VisitAll(func(key, val []byte) {binder/cookie.go (1)
Line range hint
34-41: Verify slice field type check before splittingThe splitting logic looks correct, but we should ensure
equalFieldTypeis called before attempting to split to avoid unnecessary operations.Consider reordering the conditions for better performance:
-if b.EnableSplitting && strings.Contains(v, ",") && equalFieldType(out, reflect.Slice, k) { +if b.EnableSplitting && equalFieldType(out, reflect.Slice, k) && strings.Contains(v, ",") {binder/query.go (1)
Line range hint
38-45: Maintain consistent condition ordering with cookie.goFor consistency across binders, the condition ordering should match the suggested pattern in cookie.go.
Consider reordering the conditions:
-if b.EnableSplitting && strings.Contains(v, ",") && equalFieldType(out, reflect.Slice, k) { +if b.EnableSplitting && equalFieldType(out, reflect.Slice, k) && strings.Contains(v, ",") {binder/form.go (1)
56-63: Consider separating form and multipart form bindersAs suggested in a previous review, consider splitting this into two separate binders for better separation of concerns.
This would allow for:
- Cleaner interface implementation
- Separate configuration for each binding type
- More focused testing
bind.go (1)
Line range hint
80-240: Consider introducing a helper method for binder initializationThe binder initialization pattern is repeated across all binding methods. Consider extracting it to a helper method to make the code DRYer and easier to maintain.
Example implementation:
func (b *Bind) initBinder[T any](pool *sync.Pool, enableSplitting bool) (*T, func()) { bind := binder.GetFromThePool[T](pool) if bs, ok := any(bind).(interface{ SetEnableSplitting(bool) }); ok { bs.SetEnableSplitting(enableSplitting) } return bind, func() { bind.Reset() } }Usage:
func (b *Bind) Header(out any) error { bind, cleanup := b.initBinder[*binder.HeaderBinding](&binder.HeaderBinderPool, b.ctx.App().config.EnableSplittingOnParsers) defer cleanup() if err := b.returnErr(bind.Bind(b.ctx.Request(), out)); err != nil { return err } return b.validateStruct(out) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bind.go(9 hunks)binder/cbor.go(1 hunks)binder/cookie.go(2 hunks)binder/form.go(3 hunks)binder/header.go(2 hunks)binder/json.go(1 hunks)binder/query.go(3 hunks)binder/resp_header.go(2 hunks)binder/uri.go(1 hunks)binder/xml.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
binder/xml.go
[warning] 29-31: binder/xml.go#L29-L31
Added lines #L29 - L31 were not covered by tests
binder/resp_header.go
[warning] 42-44: binder/resp_header.go#L42-L44
Added lines #L42 - L44 were not covered by tests
binder/json.go
[warning] 23-25: binder/json.go#L23-L25
Added lines #L23 - L25 were not covered by tests
binder/form.go
[warning] 66-68: binder/form.go#L66-L68
Added lines #L66 - L68 were not covered by tests
binder/query.go
[warning] 56-58: binder/query.go#L56-L58
Added lines #L56 - L58 were not covered by tests
binder/cbor.go
[warning] 23-25: binder/cbor.go#L23-L25
Added lines #L23 - L25 were not covered by tests
binder/cookie.go
[warning] 52-54: binder/cookie.go#L52-L54
Added lines #L52 - L54 were not covered by tests
binder/header.go
[warning] 42-44: binder/header.go#L42-L44
Added lines #L42 - L44 were not covered by tests
bind.go
[warning] 216-221: bind.go#L216-L221
Added lines #L216 - L221 were not covered by tests
[warning] 223-223: bind.go#L223
Added line #L223 was not covered by tests
binder/uri.go
[warning] 22-23: binder/uri.go#L22-L23
Added lines #L22 - L23 were not covered by tests
🔇 Additional comments (10)
binder/json.go (2)
18-20:
Add nil check for JSONDecoder in Bind method
The Bind method should verify that JSONDecoder is not nil before using it.
func (b *JSONBinding) Bind(body []byte, out any) error {
+ if b.JSONDecoder == nil {
+ return ErrUnknownDecoder
+ }
return b.JSONDecoder(body, out)
}Likely invalid or redundant comment.
22-25:
Potential race condition in Reset method
Setting JSONDecoder to nil before putting the binder back to the pool could cause race conditions if the binder is retrieved and used concurrently.
Additionally, the Reset method lacks test coverage. Consider adding tests for the pooling mechanism.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 23-25: binder/json.go#L23-L25
Added lines #L23 - L25 were not covered by tests
binder/xml.go (1)
10-12: Initialize XMLDecoder field to prevent nil pointer dereference
The XMLDecoder field needs initialization to prevent potential nil pointer dereference in the Bind method.
binder/cookie.go (1)
11-14: LGTM: Type exported and EnableSplitting field added
The struct renaming and field addition align with the PR objectives to fix EnableSplittingOnParsers functionality.
binder/query.go (1)
55-59: Add test coverage for Reset method
The Reset method lacks test coverage according to codecov report.
#!/bin/bash
# Check existing test coverage for Reset method
rg -l "TestQueryBinding.*Reset" -t go🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 56-58: binder/query.go#L56-L58
Added lines #L56 - L58 were not covered by tests
binder/form.go (2)
Line range hint 38-45: Maintain consistent condition ordering with other binders
For consistency across binders, the condition ordering should match the suggested pattern.
Consider reordering the conditions:
-if b.EnableSplitting && strings.Contains(v, ",") && equalFieldType(out, reflect.Slice, k) {
+if b.EnableSplitting && equalFieldType(out, reflect.Slice, k) && strings.Contains(v, ",") {65-69: Add test coverage for Reset method
The Reset method lacks test coverage according to codecov report.
#!/bin/bash
# Check existing test coverage for Reset method
rg -l "TestFormBinding.*Reset" -t go🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 66-68: binder/form.go#L66-L68
Added lines #L66 - L68 were not covered by tests
bind.go (3)
80-88: LGTM! Well-structured pooling implementation.
The Header binding implementation correctly manages resources with proper pooling and cleanup.
182-190: LGTM! Proper XML decoder configuration
The XML binding implementation correctly manages resources and properly configures the XML decoder from the application config.
Line range hint 80-240: LGTM! Consistent error handling pattern
The error handling is consistently implemented across all binding methods with proper validation.
| func (b *CBORBinding) Bind(body []byte, out any) error { | ||
| return b.CBORDecoder(body, out) | ||
| } |
There was a problem hiding this comment.
Add nil check for CBORDecoder in Bind method
The Bind method should verify that CBORDecoder is not nil before using it.
func (b *CBORBinding) Bind(body []byte, out any) error {
+ if b.CBORDecoder == nil {
+ return ErrUnknownDecoder
+ }
return b.CBORDecoder(body, out)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (b *CBORBinding) Bind(body []byte, out any) error { | |
| return b.CBORDecoder(body, out) | |
| } | |
| func (b *CBORBinding) Bind(body []byte, out any) error { | |
| if b.CBORDecoder == nil { | |
| return ErrUnknownDecoder | |
| } | |
| return b.CBORDecoder(body, out) | |
| } |
| // Reset puts URI binding back to the pool. | ||
| func (b *URIBinding) Reset() { | ||
| PutToThePool(&URIBinderPool, b) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for Reset method and improve pool testing
The current test coverage for the URIBinding's Reset method and its pool interaction is insufficient:
- While there's a generic pool test in
Test_GetAndPutToThePool, it only verifies basic pool retrieval without testing the Reset functionality - The test doesn't verify if the URIBinding is properly reset and returned to the pool
- No specific test cases exist for the URIBinding pool lifecycle
Suggested test additions:
- Add specific test cases for URIBinding Reset method
- Verify the state of the binding before and after Reset
- Test the complete pool lifecycle (get -> use -> reset -> put)
🔗 Analysis chain
Add test coverage for Reset method
The Reset method lacks test coverage. Consider adding tests to verify the pooling mechanism works correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing pool tests
rg -l "TestURIBinderPool|TestURIBinding.*Reset" --type go
Length of output: 59
Script:
#!/bin/bash
# Let's first check if there are any test files for URI binding
fd "uri.*test.*\.go$"
# Also check for any existing tests related to URI binding
rg "func Test.*URI" --type go
# Let's also check the implementation to understand the pool usage
rg "URIBinderPool" --type go -C 3
Length of output: 1484
Script:
#!/bin/bash
# Let's check the content of the URI binding tests to see if they cover the pool reset
rg "Reset|Pool" binder/uri_test.go -C 3
# Also check the test coverage for the generic pool functions
rg "func Test.*Pool" --type go
Length of output: 250
Script:
#!/bin/bash
# Let's check the content of the pool test to understand the coverage
rg "Test_GetAndPutToThePool" binder/binder_test.go -A 20
Length of output: 746
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-23: binder/uri.go#L22-L23
Added lines #L22 - L23 were not covered by tests
| func (b *XMLBinding) Bind(body []byte, out any) error { | ||
| if err := b.XMLDecoder(body, out); err != nil { | ||
| return fmt.Errorf("failed to unmarshal xml: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Add nil check for XMLDecoder in Bind method
The Bind method should verify that XMLDecoder is not nil before using it.
Apply this diff:
func (b *XMLBinding) Bind(body []byte, out any) error {
+ if b.XMLDecoder == nil {
+ return fmt.Errorf("xml decoder not initialized")
+ }
if err := b.XMLDecoder(body, out); err != nil {
return fmt.Errorf("failed to unmarshal xml: %w", err)
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (b *XMLBinding) Bind(body []byte, out any) error { | |
| if err := b.XMLDecoder(body, out); err != nil { | |
| return fmt.Errorf("failed to unmarshal xml: %w", err) | |
| } | |
| return nil | |
| } | |
| func (b *XMLBinding) Bind(body []byte, out any) error { | |
| if b.XMLDecoder == nil { | |
| return fmt.Errorf("xml decoder not initialized") | |
| } | |
| if err := b.XMLDecoder(body, out); err != nil { | |
| return fmt.Errorf("failed to unmarshal xml: %w", err) | |
| } | |
| return nil | |
| } |
| // Reset puts XML binding back to the pool. | ||
| func (b *XMLBinding) Reset() { | ||
| b.XMLDecoder = nil | ||
| PutToThePool(&XMLBinderPool, b) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for Reset method in XMLBinding
The verification reveals that while there is a general pool usage test and XML-specific binding tests, the Reset method of XMLBinding lacks dedicated test coverage. This is important because:
- All binders use a similar pool pattern but only have basic pool usage tests
- Setting
XMLDecoderto nil is a critical operation that should be verified - The
XMLBindingtests only cover theBindfunctionality
Required test coverage:
- Test that
Resetproperly setsXMLDecoderto nil - Verify proper interaction with
XMLBinderPool - Ensure pool reuse works correctly after Reset
🔗 Analysis chain
Add test coverage for Reset method and document pool usage
The Reset method lacks test coverage and setting XMLDecoder to nil could cause issues if the pool is misused.
Would you like me to help generate test cases for the Reset functionality?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for Reset methods across binders
rg -l "func.*Reset\(\)" | xargs -I {} rg "Test.*Reset" {}
Length of output: 63
Script:
#!/bin/bash
# Let's first check if there are any test files for binders
fd ".*_test.go" binder/
# Then check for Reset method implementations
rg -A 5 "func.*Reset\(\)" binder/
# Check for pool usage patterns
rg "Pool" binder/
Length of output: 4162
Script:
#!/bin/bash
# Let's check the test coverage for pool usage and Reset functionality
rg -B 2 -A 5 "Test.*Pool|Test.*Reset" binder/binder_test.go
# Check if there are any XML-specific tests
rg -B 2 -A 5 "Test.*XML" binder/xml_test.go
Length of output: 529
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 29-31: binder/xml.go#L29-L31
Added lines #L29 - L31 were not covered by tests
| // Reset resets the HeaderBinding binder and puts it back to the binder pool. | ||
| func (b *HeaderBinding) Reset() { | ||
| b.EnableSplitting = false | ||
| PutToThePool(&HeaderBinderPool, b) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for Reset method's state management
While there is basic test coverage for pool operations in binder_test.go, the Reset method's state management needs specific testing. Current tests don't verify that:
EnableSplittingis properly reset tofalse- The binder's state is fully cleared before returning to pool
Suggested test locations:
./binder/header_test.go: Add Reset-specific test cases./binder/binder_test.go: Enhance pool tests to verify state reset
🔗 Analysis chain
Add test coverage for Reset method
The Reset method lacks test coverage to verify proper pool management.
Would you like me to help generate test cases for the Reset functionality?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Reset is tested in integration tests
rg -A 5 "TestHeader.*Reset|TestBind.*Header.*Reset" .
Length of output: 55
Script:
#!/bin/bash
# Let's check for any existing tests related to HeaderBinding
rg -l "HeaderBinding" . | grep "_test.go"
Length of output: 116
Script:
#!/bin/bash
# Let's also check the implementation details of HeaderBinding and its pool usage
rg -A 5 "HeaderBinderPool|HeaderBinding" .
Length of output: 5225
Script:
#!/bin/bash
# Check for any test files that might test the pool functionality
rg -A 5 "PutToThePool|GetFromThePool" . | grep "_test"
Length of output: 1224
Script:
#!/bin/bash
# Let's check the content of the pool test to understand its coverage
cat ./binder/binder_test.go
Length of output: 798
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-44: binder/header.go#L42-L44
Added lines #L42 - L44 were not covered by tests
| // Reset resets the CookieBinding binder and puts it back to the binder pool. | ||
| func (b *CookieBinding) Reset() { | ||
| b.EnableSplitting = false | ||
| PutToThePool(&CookieBinderPool, b) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for Reset method
Based on the verification results:
- The
Resetmethod inCookieBindinglacks direct test coverage - While there are pool-related tests (
Test_GetAndPutToThePool), they only test withHeaderBinding - The
Resetmethod's functionality includes resettingEnableSplittingand returning to pool, both of which should be verified
Suggested test cases:
- Test that
EnableSplittingis set to false after Reset - Test that the binder is properly returned to the pool
- Test that getting the same binder from pool after Reset has
EnableSplittingas false
🔗 Analysis chain
Add test coverage for Reset method
The Reset method lacks test coverage according to codecov report.
Would you like me to help generate test cases for the Reset method?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check existing test coverage for Reset method
rg -l "TestCookieBinding.*Reset" -t go
Length of output: 40
Script:
#!/bin/bash
# Let's first check if there are any test files for cookie binding
fd "cookie.*test.*\.go$"
# Also check for any test cases that might test Reset without explicitly having "Reset" in the name
rg -A 5 "func Test.*Cookie.*" -t go
# Let's also verify the actual implementation to understand the test requirements better
rg "CookieBinding.*Reset" -t go
Length of output: 9714
Script:
#!/bin/bash
# Let's check the test coverage for the cookie binder file
rg -B 5 -A 10 "Test_CookieBinder" binder/cookie_test.go
# Let's also check if there are any pool-related tests since Reset uses PutToThePool
rg "PutToThePool.*test" -t go
Length of output: 429
Script:
#!/bin/bash
# Let's check the pool-related test implementation
rg -B 5 -A 10 "Test_GetAndPutToThePool" binder/binder_test.go
# Also check the actual implementation of PutToThePool
ast-grep --pattern 'func PutToThePool($$$)'
Length of output: 481
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 52-54: binder/cookie.go#L52-L54
Added lines #L52 - L54 were not covered by tests
| bind.Reset() | ||
| }() | ||
|
|
||
| if err := b.returnErr(bind.Bind(&b.ctx.RequestCtx().Request, out)); err != nil { |
There was a problem hiding this comment.
Use Request() method for consistency and type safety
Instead of accessing RequestCtx().Request directly, consider using the abstracted Request() method like in the Header binding implementation for better consistency and type safety.
-if err := b.returnErr(bind.Bind(&b.ctx.RequestCtx().Request, out)); err != nil {
+if err := b.returnErr(bind.Bind(b.ctx.Request(), out)); err != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := b.returnErr(bind.Bind(&b.ctx.RequestCtx().Request, out)); err != nil { | |
| if err := b.returnErr(bind.Bind(b.ctx.Request(), out)); err != nil { |
bind.go
Outdated
| bind := binder.GetFromThePool[*binder.URIBinding](&binder.URIBinderPool) | ||
|
|
||
| // Reset & put binder | ||
| defer func() { | ||
| bind.Reset() | ||
| }() | ||
|
|
||
| if err := b.returnErr(bind.Bind(b.ctx.Route().Params, b.ctx.Params, out)); err != nil { |
There was a problem hiding this comment.
💡 Codebase verification
URI binding has test coverage, but pool and Reset behavior needs testing
The codebase already has URI binding tests in binder/uri_test.go including both unit tests (Test_URIBinding_Bind) and benchmarks (Benchmark_URIBinding_Bind). However, the specific pool usage (GetFromThePool) and Reset() behavior in the reviewed code segment are not explicitly tested.
- Add test cases in
bind_test.goto verify:- Proper acquisition of binder from the pool
- Reset is called via defer as expected
- Binder is properly returned to the pool after use
🔗 Analysis chain
Add test coverage for URI binding
The URI binding implementation looks correct but lacks test coverage. Please add tests to verify the behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check existing test coverage for URI binding
# Look for existing tests
rg -l "TestBind.*URI" .
# Look for URI binding usage patterns to help with test cases
ast-grep --pattern 'func $test($t *testing.T) {
$$$
Bind().URI($$$)
$$$
}'
Length of output: 108
Script:
#!/bin/bash
# Let's try a broader search for URI binding tests and implementation
# Search for URI binding test files
rg -l "Test.*URI.*Bind|Bind.*URI" .
# Search for URI binding implementation and usage
rg -l "URIBinding|URIBinder" .
# Look for specific test patterns with URIBinding
ast-grep --pattern 'func Test$_($_) {
$$$
URIBinding
$$$
}'
# Check for any test files that might test URI binding functionality
fd ".*_test.go$" --exec grep -l "URI.*Bind\|Bind.*URI" {}
Length of output: 483
Script:
#!/bin/bash
# Let's examine the actual URI binding test content
# Check URI binding test content
rg "func Test.*URI.*Bind|func.*TestBind.*URI" -A 10 ./binder/uri_test.go ./bind_test.go
# Look for specific URI binding test cases
rg "URIBinding" -B 2 -A 5 ./binder/uri_test.go
# Check test coverage for the specific pool usage and Reset functionality
rg "GetFromThePool.*URIBinding|Reset\(\)" -B 2 -A 5 ./binder/uri_test.go ./bind_test.go
Length of output: 2096
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 216-221: bind.go#L216-L221
Added lines #L216 - L221 were not covered by tests
[warning] 223-223: bind.go#L223
Added line #L223 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
binder/json_test.go (1)
42-69: 🛠️ Refactor suggestionImprove benchmark implementation
The benchmark has several areas for improvement:
- Missing cleanup between iterations (as noted in previous review)
- Assertions after the benchmark loop affect timing
- Redundant struct definition
Apply these improvements:
-func Benchmark_JSON_Binding_Bind(b *testing.B) { +// Move struct outside to avoid redefinition +type BenchUser struct { + Name string `json:"name"` + Posts []string `json:"posts"` + Age int `json:"age"` +} + +func Benchmark_JSON_Binding_Bind(b *testing.B) { b.ReportAllocs() - b.ResetTimer() binder := &JSONBinding{ JSONDecoder: json.Unmarshal, } - type User struct { - Name string `json:"name"` - Posts []string `json:"posts"` - Age int `json:"age"` - } + var user BenchUser + jsonData := []byte(`{"name":"john","age":42,"posts":["post1","post2","post3"]}`) - var user User - var err error + // Validate binding works before benchmark + err := binder.Bind(jsonData, &user) + if err != nil || user.Name != "john" || user.Age != 42 || len(user.Posts) != 3 { + b.Fatal("Invalid test setup") + } + + b.ResetTimer() for i := 0; i < b.N; i++ { - err = binder.Bind([]byte(`{"name":"john","age":42,"posts":["post1","post2","post3"]}`), &user) + b.StopTimer() + user = BenchUser{} // Reset between iterations + b.StartTimer() + + if err := binder.Bind(jsonData, &user); err != nil { + b.Fatal(err) + } } - - require.NoError(b, err) - require.Equal(b, "john", user.Name) - require.Equal(b, 42, user.Age) - require.Len(b, user.Posts, 3) - require.Equal(b, "post1", user.Posts[0]) - require.Equal(b, "post2", user.Posts[1]) - require.Equal(b, "post3", user.Posts[2]) }binder/header_test.go (2)
10-50: 🛠️ Refactor suggestionEnhance test coverage with table-driven tests
The current test only covers the happy path with EnableSplitting=true. Based on the PR objectives to fix EnableSplittingOnParsers functionality, we need more comprehensive test coverage.
Consider implementing table-driven tests to cover:
- Both EnableSplitting=true and false cases
- Error scenarios (invalid int value for Age)
- Empty and missing headers
- Invalid header values
Here's a suggested implementation:
func Test_HeaderBinder_Bind(t *testing.T) { t.Parallel() + tests := []struct { + name string + enableSplitting bool + headers map[string]string + want User + wantErr bool + }{ + { + name: "valid headers with splitting enabled", + enableSplitting: true, + headers: map[string]string{ + "name": "john", + "names": "john,doe", + "age": "42", + "posts": "post1,post2,post3", + }, + want: User{ + Name: "john", + Names: []string{"john", "doe"}, + Age: 42, + Posts: []string{"post1", "post2", "post3"}, + }, + }, + { + name: "valid headers with splitting disabled", + enableSplitting: false, + headers: map[string]string{ + "names": "john,doe", + "posts": "post1,post2,post3", + }, + want: User{ + Names: []string{"john,doe"}, + Posts: []string{"post1,post2,post3"}, + }, + }, + { + name: "invalid age", + enableSplitting: true, + headers: map[string]string{ + "age": "invalid", + }, + wantErr: true, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + b := &HeaderBinding{ + EnableSplitting: tt.enableSplitting, + } + var user User + req := fasthttp.AcquireRequest() + t.Cleanup(func() { + fasthttp.ReleaseRequest(req) + }) + + for k, v := range tt.headers { + req.Header.Set(k, v) + } + + err := b.Bind(req, &user) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, tt.want, user) + }) + } }
52-88: 🛠️ Refactor suggestionImprove benchmark coverage and accuracy
The benchmark has several issues that need attention:
- Missing benchmark cases for disabled splitting
- Assertions in benchmarks can affect performance measurements
Consider implementing separate benchmark cases and moving assertions to a separate validation function:
func Benchmark_HeaderBinder_Bind(b *testing.B) { + benchCases := []struct { + name string + enableSplitting bool + }{ + {"EnableSplitting", true}, + {"DisableSplitting", false}, + } + for _, bc := range benchCases { + b.Run(bc.name, func(b *testing.B) { + binder := &HeaderBinding{ + EnableSplitting: bc.enableSplitting, + } + var user User + req := fasthttp.AcquireRequest() + b.Cleanup(func() { + fasthttp.ReleaseRequest(req) + }) + req.Header.Set("name", "john") + req.Header.Set("age", "42") + req.Header.Set("posts", "post1,post2,post3") + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + if err := binder.Bind(req, &user); err != nil { + b.Fatal(err) + } + } + }) + } } +// validateBenchmarkResult validates the benchmark result without affecting measurements +func validateBenchmarkResult(t *testing.T, user User) { + t.Helper() + require.Equal(t, "john", user.Name) + require.Equal(t, 42, user.Age) + require.Len(t, user.Posts, 3) + require.Contains(t, user.Posts, "post1") + require.Contains(t, user.Posts, "post2") + require.Contains(t, user.Posts, "post3") }
🧹 Nitpick comments (3)
binder/json_test.go (1)
10-40: Consider adding negative test casesWhile the happy path is well tested, consider adding test cases for:
- Invalid JSON input
- Type mismatches
- Missing required fields
- Empty/null values
Example test cases to add:
// Invalid JSON err := b.Bind([]byte(`{"name":"john","age":"invalid"}`), &user) require.Error(t, err) // Missing required field err = b.Bind([]byte(`{"age":42}`), &user) require.NoError(t, err) // or Error depending on your requirements require.Empty(t, user.Name) // Null values err = b.Bind([]byte(`{"name":null,"age":null,"posts":null}`), &user) require.NoError(t, err) require.Empty(t, user.Name) require.Zero(t, user.Age) require.Empty(t, user.Posts)binder/cookie_test.go (2)
18-20: Struct "Post" is unused in this test.The struct "Post" with field "Title" is declared here but isn't actually tested or referenced. Consider removing it if it's not needed, or add assertions that validate “Title” if it’s intended for extended testing.
52-90: Optional: Validate binding before the main benchmark loop.To ensure correctness, you might want to verify once outside the loop that the
Bind()logic is returning valid data. This positions validation statements outside of the measured region, preventing them from skewing the benchmark results.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
binder/cbor_test.go(1 hunks)binder/cookie_test.go(1 hunks)binder/form_test.go(1 hunks)binder/header_test.go(1 hunks)binder/json_test.go(1 hunks)binder/query_test.go(1 hunks)binder/resp_header_test.go(1 hunks)binder/uri_test.go(1 hunks)binder/xml_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- binder/uri_test.go
- binder/query_test.go
- binder/xml_test.go
- binder/resp_header_test.go
- binder/cbor_test.go
- binder/form_test.go
🔇 Additional comments (4)
binder/json_test.go (1)
1-9: LGTM!
The package declaration and imports are appropriate for the test file.
binder/cookie_test.go (2)
60-63: Inconsistent struct tags in the benchmark User struct.
This code uses CookieBinding yet relies on “query” tags for the fields. This may cause confusion or mismatched binding behavior.
Apply the same fix as shown in the previous comment thread:
- type User struct {
- Name string `query:"name"`
- Posts []string `query:"posts"`
- Age int `query:"age"`
- }
+ type User struct {
+ Name string `form:"name"`
+ Posts []string `form:"posts"`
+ Age int `form:"age"`
+ }52-90: Duplicate calls to b.ResetTimer().
There are two b.ResetTimer() calls in this benchmark (lines 54 and 76). Having multiple calls is unnecessary and can lead to inconsistent measurements.
Remove the first call or place it just before the timed section:
52 func Benchmark_CookieBinder_Bind(b *testing.B) {
53 b.ReportAllocs()
-54 b.ResetTimer()
...
76 b.ResetTimer()binder/header_test.go (1)
1-8: LGTM!
Package declaration and imports are appropriate for the test file.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
binder/form_test.go (3)
15-17: Ensure coverage for both enabled and disabled splitting
It's good that the test sets EnableSplitting to true. Adding an additional test scenario with EnableSplitting set to false would ensure complete coverage of both modes.
52-53: Reset method verification
Invoking b.Reset() and checking that EnableSplitting is now false is a solid way to confirm that the binder reinitializes to its default state. Consider adding a more extensive assertion (e.g., ensuring leftover data is also cleared) to verify the reset functionality is complete.
75-75: Multiple ResetTimer calls
You have a b.ResetTimer() at line 58 and again at line 75. In standard Go benchmarks, multiple calls to ResetTimer() can skew benchmark results since they truncate measurements. Check that the second call is strictly necessary.bind.go (1)
202-204: Extend coverage for file uploads if needed
While binding from multipart form is performed here, there’s a comment indicating that actual file handling is not yet supported. If file handling is on your roadmap, consider adding or referencing future test coverage and clarifying any limitations in the docs.docs/api/bind.md (1)
85-85: Clarify concurrency aspects for form binding
Mention that concurrently binding multiple forms requires separate binder instances. It’s implied, but calling it out can help users avoid subtle concurrency pitfalls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bind.go(8 hunks)binder/form.go(3 hunks)binder/form_test.go(1 hunks)docs/api/bind.md(2 hunks)redirect.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- binder/form.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
bind.go
[warning] 219-219: bind.go#L219
Added line #L219 was not covered by tests
[warning] 221-224: bind.go#L221-L224
Added lines #L221 - L224 were not covered by tests
[warning] 226-226: bind.go#L226
Added line #L226 was not covered by tests
🔇 Additional comments (8)
redirect.go (1)
149-149: Good addition for multipart handling
Including MIMEMultipartForm in the switch statement expands the flexibility of handling form data, especially for file upload scenarios. This helps unify code paths for both application/x-www-form-urlencoded and multipart/form-data.
binder/form_test.go (2)
32-38: Request cleanup is correctly handled
Acquiring and releasing the fasthttp request object inside t.Cleanup is good practice; it helps avoid potential resource leaks or shared state issues in tests.
105-112: Multipart encoding test is done well
Writing multiple form fields with different values properly mimics real-world multi-field requests. Testing this ensures that the binder can handle repeated fields in multipart properly.
bind.go (3)
80-82: Consistent usage of binder pools
Fetching the HeaderBinding from the pool and setting EnableSplitting is consistent with other binders. This approach helps maintain uniform configuration across different binding operations.
115-117: Cookie binding approach
The approach for cookie binding is consistent with the others, but keep an eye on potential edge cases where multiple cookie values might be merged. Ensure your tests cover that scenario.
✅ Verification successful
Let me gather information about cookie binding implementation and tests to verify the handling of multiple cookie values.
Let me search for more specific information about cookie binding and splitting implementation.
Based on the implementation details found in binder/cookie.go, I can now provide a final response:
Cookie binding implementation handles multiple values correctly
The implementation already has proper handling for multiple cookie values through the EnableSplitting flag. When enabled, it correctly splits comma-separated values into slices for fields of slice type, while preserving single values for non-slice fields. The implementation is safe and consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for cookie binding tests
rg -l "cookie.*test" --type go
# Look for cookie binding implementation
ast-grep --pattern 'type CookieBinding struct {
$$$
}'
# Search for cookie value parsing logic
rg "cookie.*value" --type go -B 2 -A 2
# Look for cookie splitting related code
rg "EnableSplitting.*cookie" --type go -B 2 -A 2
Length of output: 9333
Script:
#!/bin/bash
# Look for cookie binding implementation details
cat binder/cookie.go
# Search for cookie binding tests
rg "TestCookieBinding" --type go -B 2 -A 5
# Look for cookie parsing implementation
ast-grep --pattern 'func ($RECV *CookieBinding) Bind($ARGS) $RET {
$$$
}'
Length of output: 1303
219-224: Add test coverage for URI binder
According to codecov, these lines have low coverage. Adding a targeted test that exercises the URI binder with multiple route parameters (including edge cases) can help ensure correctness.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 219-219: bind.go#L219
Added line #L219 was not covered by tests
[warning] 221-224: bind.go#L221-L224
Added lines #L221 - L224 were not covered by tests
docs/api/bind.md (2)
113-113: Unified example for multiple form types
Providing both x-www-form-urlencoded and multipart examples in the same section is a helpful addition. It helps unify the user’s mental model of how forms are handled and tested.
119-121: Documentation is thorough
Explicitly showing how to run tests with curl fosters clarity for developers. This step-by-step guidance lowers the barrier for newcomers.
Description
It seems as if EnableSplittingOnParsers config property has lost its functionality during the introduction of new binding API PR in v3. This PR fixes default behavior (parsers should not split strings includes comma by default)
Fixes #3159
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md