🩹 fix: Embedded struct binding #3203
Conversation
michal-laskowski
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThe changes introduce a new test function in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
binder/mapping.go (1)
206-214: LGTM with minor suggestions for improvement.The changes correctly handle embedded struct binding by checking for anonymous fields and performing case-insensitive field name comparisons. A few suggestions to enhance the code:
- Remove the unnecessary comment on line 211
- Consider extracting the field name comparison logic to reduce code duplication with the similar comparison at the end of the function
Consider applying this refactor:
if structFieldKind == reflect.Struct && typeField.Anonymous { // Loop over embedded struct fields for j := 0; j < structField.NumField(); j++ { fNm := utils.ToLower(structField.Type().Field(j).Name) - if fNm != key { - //this is not the field that we are looking for - continue - } + if fNm == key { + structFieldField := structField.Field(j) - structFieldField := structField.Field(j) + // Can this embedded field be changed? + if !structFieldField.CanSet() { + continue + } - // Can this embedded field be changed? - if !structFieldField.CanSet() { - continue - } - - // Is the embedded struct field type equal to the input? - if structFieldField.Kind() == kind { - return true + // Is the embedded struct field type equal to the input? + if structFieldField.Kind() == kind { + return true + } } } }bind_test.go (1)
1111-1124: Enhance test coverage with additional test cases.While the current test validates basic form binding, consider adding:
- Test cases for multipart/form-data format
- Error scenarios (e.g., malformed data)
- Table-driven tests for multiple scenarios
Example enhancement:
+func Test_Bind_Body_Form_Embedded(t *testing.T) { + tests := []struct { + name string + contentType string + body string + want *Demo + wantErr bool + }{ + { + name: "url-encoded form", + contentType: MIMEApplicationForm, + body: "SomeString=john%2Clong&SomeOtherString=long%2Cjohn&Strings=long%2Cjohn&EmbededStrings=john%2Clong&EmbededString=johny%2Cwalker", + want: &Demo{...}, + }, + { + name: "multipart form", + contentType: MIMEMultipartForm + `;boundary="b"`, + body: "--b\r\nContent-Disposition: form-data; name=\"SomeString\"\r\n\r\njohn,long\r\n--b--", + want: &Demo{...}, + }, + { + name: "malformed data", + contentType: MIMEApplicationForm, + body: "invalid=data&%invalid", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Test implementation + }) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
bind_test.go(1 hunks)binder/mapping.go(1 hunks)
🔇 Additional comments (3)
binder/mapping.go (2)
206-214: Consider performance implications for deeply nested structs.
The current implementation iterates through all fields of embedded structs, which could impact performance for deeply nested structures. Consider:
- Adding early returns when possible
- Implementing field caching for repeated lookups
- Adding benchmarks to measure the impact on different struct hierarchies
Let's verify the usage of embedded structs in the codebase:
206-214: Verify test coverage for embedded struct binding.
Please ensure the test suite covers:
- Case-insensitive field name matching
- Multiple levels of struct embedding
- Edge cases:
- Unexported fields in embedded structs
- Embedded structs with conflicting field names
- Non-struct anonymous fields
Let's verify the test coverage:
bind_test.go (1)
1094-1099: LGTM! Clean test setup following best practices.
The test setup follows Go testing conventions with proper error handling and context management.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
bind_test.go (1)
1100-1110: Optimize struct field alignment for better memory efficiency.The current struct field ordering can be optimized to reduce memory padding and improve cache utilization.
Apply this diff to optimize the field alignment:
type EmbeddedDemo struct { - EmbeddedStrings []string `form:"embedded_strings"` EmbeddedString string `form:"embedded_string"` + EmbeddedStrings []string `form:"embedded_strings"` } type Demo struct { - SomeString string `form:"some_string"` - SomeOtherString string `form:"some_other_string"` - Strings []string `form:"strings"` EmbeddedDemo + Strings []string `form:"strings"` + SomeString string `form:"some_string"` + SomeOtherString string `form:"some_other_string"` }🧰 Tools
🪛 golangci-lint
1100-1100: fieldalignment: struct with 32 pointer bytes could be 24
(govet)
1105-1105: fieldalignment: struct with 88 pointer bytes could be 80
(govet)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
bind_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint
bind_test.go
1100-1100: fieldalignment: struct with 32 pointer bytes could be 24
(govet)
1105-1105: fieldalignment: struct with 88 pointer bytes could be 80
(govet)
🔇 Additional comments (1)
bind_test.go (1)
1119-1124: LGTM! Comprehensive test assertions.
The test assertions thoroughly verify the binding functionality:
- Validates array splitting for both embedded and non-embedded fields
- Verifies correct binding of string values
- Confirms proper handling of embedded struct fields
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
bind_test.go (1)
1100-1110: Consider reordering struct fields for better memory alignment.The current field ordering leads to suboptimal memory padding. Consider reordering fields to minimize struct size:
type EmbeddedDemo struct { - EmbeddedStrings []string `form:"embedded_strings"` EmbeddedString string `form:"embedded_string"` + EmbeddedStrings []string `form:"embedded_strings"` } type Demo struct { - SomeString string `form:"some_string"` - SomeOtherString string `form:"some_other_string"` - Strings []string `form:"strings"` EmbeddedDemo + Strings []string `form:"strings"` + SomeString string `form:"some_string"` + SomeOtherString string `form:"some_other_string"` }🧰 Tools
🪛 golangci-lint
1100-1100: fieldalignment: struct with 32 pointer bytes could be 24
(govet)
1105-1105: fieldalignment: struct with 88 pointer bytes could be 80
(govet)
🪛 GitHub Check: lint
[failure] 1100-1100:
fieldalignment: struct with 32 pointer bytes could be 24 (govet)
[failure] 1105-1105:
fieldalignment: struct with 88 pointer bytes could be 80 (govet)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
bind_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint
bind_test.go
1100-1100: fieldalignment: struct with 32 pointer bytes could be 24
(govet)
1105-1105: fieldalignment: struct with 88 pointer bytes could be 80
(govet)
🪛 GitHub Check: lint
bind_test.go
[failure] 1100-1100:
fieldalignment: struct with 32 pointer bytes could be 24 (govet)
[failure] 1105-1105:
fieldalignment: struct with 88 pointer bytes could be 80 (govet)
🔇 Additional comments (1)
bind_test.go (1)
1111-1124:
Fix inconsistency between form data keys and struct tags.
The form data keys use "Embeded" (one 'd') while struct tags use "embedded" (two 'd's). Although the test passes, this inconsistency should be fixed to ensure the test accurately validates the binding behavior:
-body := []byte("SomeString=john%2Clong&SomeOtherString=long%2Cjohn&Strings=long%2Cjohn&EmbededStrings=john%2Clong&EmbededString=johny%2Cwalker")
+body := []byte("SomeString=john%2Clong&SomeOtherString=long%2Cjohn&Strings=long%2Cjohn&embedded_strings=john%2Clong&embedded_string=johny%2Cwalker")Let's verify if the binder is case-sensitive for form field matching:
|
@michal-laskowski The makefile has a command for fixing the alignment of structs. You can also run the tests locally.
|
|
Added support for other bindings Done
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
binder/resp_header.go (1)
Line range hint
13-23: Consider adding error handling for header parsingWhile the implementation is correct, consider adding explicit error handling for malformed header values to improve robustness.
Here's a suggested enhancement:
func (b *respHeaderBinding) Bind(resp *fasthttp.Response, out any) error { data := make(map[string][]string) resp.Header.VisitAll(func(key, val []byte) { k := utils.UnsafeString(key) v := utils.UnsafeString(val) + // Skip empty or malformed headers + if k == "" || v == "" { + return + } appendValue(data, v, out, k, b.Name()) }) return parse(b.Name(), out, data) }binder/cookie.go (1)
Line range hint
16-32: Review error handling in the cookie visitorThe error variable
erris declared and checked but never assigned within the visitor callback. This means the error check after the visitor loop will always be false.Consider one of these approaches:
func (b *cookieBinding) Bind(reqCtx *fasthttp.RequestCtx, out any) error { data := make(map[string][]string) - var err error reqCtx.Request.Header.VisitAllCookie(func(key, val []byte) { - if err != nil { - return - } k := utils.UnsafeString(key) v := utils.UnsafeString(val) appendValue(data, v, out, k, b.Name()) }) - if err != nil { - return err - } return parse(b.Name(), out, data) }Or if error handling is needed:
func (b *cookieBinding) Bind(reqCtx *fasthttp.RequestCtx, out any) error { data := make(map[string][]string) var err error reqCtx.Request.Header.VisitAllCookie(func(key, val []byte) { if err != nil { return } k := utils.UnsafeString(key) v := utils.UnsafeString(val) + if e := appendValue(data, v, out, k, b.Name()); e != nil { + err = e + return + } }) if err != nil { return err } return parse(b.Name(), out, data) }binder/form.go (1)
Line range hint
15-40: Consider adding debug loggingGiven that this is a critical binding functionality, consider adding debug logging to help troubleshoot binding issues in production.
Add debug logging before and after value appending:
func (b *formBinding) Bind(reqCtx *fasthttp.RequestCtx, out any) error { data := make(map[string][]string) var err error + if fiber.IsDebugMode() { + fiber.Debug("Form binding started for type: %T", out) + } reqCtx.PostArgs().VisitAll(func(key, val []byte) { if err != nil { return } k := utils.UnsafeString(key) v := utils.UnsafeString(val) + if fiber.IsDebugMode() { + fiber.Debug("Processing form field: %s with value: %s", k, v) + } if strings.Contains(k, "[") { k, err = parseParamSquareBrackets(k) } appendValue(data, v, out, k, b.Name()) })binder/mapping_test.go (1)
19-19: Consider adding a positive test caseWhile testing the negative case is good, consider adding a corresponding positive test case where the field name matches exactly to ensure both scenarios are covered.
var dummy2 struct{ f string } require.False(t, equalFieldType(&dummy2, reflect.String, "f", "query")) + // Add positive test case + require.True(t, equalFieldType(&dummy2, reflect.String, "f", "query"))binder/mapping.go (4)
175-184: Consider adding value validation and trimmingWhile the function effectively consolidates value appending logic, consider enhancing it to handle edge cases:
- Trim whitespace from values before appending
- Skip empty values after trimming
func appendValue(to map[string][]string, rawValue string, out any, k string, bindingName string) { if strings.Contains(rawValue, ",") && equalFieldType(out, reflect.Slice, k, bindingName) { values := strings.Split(rawValue, ",") for i := 0; i < len(values); i++ { + value := strings.TrimSpace(values[i]) + if value == "" { + continue + } - to[k] = append(to[k], values[i]) + to[k] = append(to[k], value) } } else { - to[k] = append(to[k], rawValue) + value := strings.TrimSpace(rawValue) + if value != "" { + to[k] = append(to[k], value) + } } }🧰 Tools
🪛 golangci-lint
175-175: File is not
gofumpt-ed with-extra(gofumpt)
Line range hint
186-244: Consider caching field lookups for better performanceThe function performs field lookups on every call, which could be expensive for deeply nested structs with many fields. Consider implementing a cache for field metadata to improve performance.
This could be implemented as a sync.Map at the package level to store field metadata keyed by type and field name. The cache would need to be carefully managed to prevent memory leaks from dynamic types.
Would you like me to provide a detailed implementation of the caching mechanism?
245-256: Add documentation for function parameters and return valueThe function would benefit from proper documentation explaining:
- The purpose of the
bindingNameparameter- The format of the returned key
- The tag parsing behavior
+// getFieldKey returns the binding key for a struct field, using either the tag value +// specified by bindingName or the field name if no tag is present. The returned key +// is always lowercase. For tagged fields, only the part before the first comma is used. +// Parameters: +// - typeField: The struct field to get the key for +// - bindingName: The struct tag name to look up (e.g., "form", "json") +// Returns: +// - The lowercase binding key to use for this field func getFieldKey(typeField reflect.StructField, bindingName string) string {
175-175: Fix gofumpt formatting issuesThe file needs to be formatted with
gofumpt -extra.Run the following command to fix the formatting:
gofumpt -l -w .Also applies to: 186-186
🧰 Tools
🪛 golangci-lint
175-175: File is not
gofumpt-ed with-extra(gofumpt)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
bind_test.go(1 hunks)binder/cookie.go(1 hunks)binder/form.go(1 hunks)binder/header.go(1 hunks)binder/mapping.go(2 hunks)binder/mapping_test.go(1 hunks)binder/query.go(1 hunks)binder/resp_header.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint
binder/mapping.go
175-175: File is not gofumpt-ed with -extra
(gofumpt)
186-186: File is not gofumpt-ed with -extra
(gofumpt)
🔇 Additional comments (13)
binder/header.go (3)
20-22: LGTM: Clean and consistent implementation.
The change aligns with the broader refactoring effort across binding types, making the code more maintainable while addressing the embedded struct binding issue.
20-20: Consider performance implications of the abstraction.
While the abstraction improves code maintainability, we should verify that it doesn't introduce unnecessary allocations or performance overhead, especially since the PR objectives mention performance optimization.
Let's check for potential performance implications:
#!/bin/bash
# Description: Look for performance-related tests and benchmarks
# Check for benchmark tests
echo "Checking for binding benchmarks:"
rg -A 5 "Benchmark.*Bind" --glob "**/*_test.go"
# Look for performance-related comments or documentation
echo -e "\nChecking for performance-related documentation:"
rg -i "performance|benchmark|allocation" --glob "**/bind*.go"20-20: Verify the new appendValue implementation handles all edge cases.
The change simplifies the code by using a shared appendValue function, but we should ensure it maintains the same functionality for header binding edge cases.
Let's verify the implementation and usage:
✅ Verification successful
The appendValue implementation is verified and safe to use
The code analysis reveals that:
- The
appendValuefunction inbinder/mapping.gohandles both single values and comma-separated values appropriately:- For comma-separated values, it splits and appends each value if the target field is a slice
- For single values, it appends directly
- The header binding implementation in
binder/header.gouses this shared function correctly - Comprehensive test coverage exists in
bind_test.goincluding:- Basic header binding (
Test_Bind_Header) - Map binding (
Test_Bind_Header_Map) - Schema validation (
Test_Bind_Header_Schema) - Custom parser support (
Test_Bind_Header_WithSetParserDecoder)
- Basic header binding (
The implementation maintains the same functionality as before while improving code reuse and consistency across different binding types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the appendValue implementation and its test coverage
# Check the appendValue implementation
echo "Checking appendValue implementation:"
ast-grep --pattern 'func appendValue($params) {
$$$
}'
# Look for related test cases
echo -e "\nChecking test coverage for header binding:"
rg -A 5 "Test.*header.*bind" --glob "**/*_test.go"
# Check for any existing header-specific edge case handling
echo -e "\nChecking for header-specific handling:"
rg -B 2 -A 2 "header" --glob "**/bind*.go"
Length of output: 9105
Script:
#!/bin/bash
# Let's try a different approach to find the appendValue implementation and header-specific tests
# Find appendValue implementation
echo "Looking for appendValue implementation:"
rg -A 10 "func appendValue" --glob "**/*.go"
# Find header binding tests
echo -e "\nLooking for header binding tests:"
rg -A 10 "func Test.*Header" --glob "**/*_test.go"
# Find header-specific handling in binder
echo -e "\nLooking for header binding implementation:"
rg -A 10 "type headerBinding" --glob "**/*.go"
Length of output: 20174
binder/resp_header.go (1)
20-20: Verify the behavior of appendValue function
The refactoring to use appendValue looks good and aligns with the standardization effort. However, let's verify the implementation handles all edge cases correctly.
✅ Verification successful
appendValue implementation is safe and consistently used across binders
The verification shows that:
- The
appendValuefunction is well-implemented inbinder/mapping.gowith proper handling of comma-separated values for slice fields - The function is consistently used across all binders (header, resp_header, query, form, cookie)
- The
equalFieldTypehelper properly validates field types including embedded structs - Comprehensive test coverage exists for header binding scenarios
The refactoring aligns with the standardization effort and maintains consistent behavior across all binding contexts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the appendValue implementation and its usage across different binders
# Check the appendValue implementation
echo "Checking appendValue implementation:"
ast-grep --pattern 'func appendValue($$$) {
$$$
}'
# Check for consistent usage across other binders
echo -e "\nChecking consistent usage across binders:"
rg -l "appendValue\(" --type go
# Look for any existing tests covering this functionality
echo -e "\nChecking related tests:"
rg -l "TestBind.*Header" --type go
Length of output: 1053
Script:
#!/bin/bash
# Let's verify the test coverage and implementation details
# Check test cases for header binding
echo "Checking header binding tests:"
rg "func Test.*Header" -A 10 --type go
# Check equalFieldType implementation since it's used in appendValue
echo -e "\nChecking equalFieldType implementation:"
ast-grep --pattern 'func equalFieldType($$$) bool {
$$$
}'
# Check actual usage patterns of appendValue
echo -e "\nChecking appendValue usage patterns:"
rg "appendValue\(" -B 1 -A 1 --type go
Length of output: 22635
binder/cookie.go (1)
26-26: LGTM: Clean refactoring to use appendValue
The change simplifies the cookie binding logic by delegating value handling to the centralized appendValue function, which is consistent with other binding types and supports the PR's goal of fixing embedded struct binding.
binder/query.go (2)
Line range hint 16-40: Consider adding test cases for complex query parameters
While the implementation looks solid, consider adding test cases that cover:
- Query parameters targeting embedded struct fields
- Nested array notation with embedded structs
- Case sensitivity handling in embedded struct field names
Let's check the existing test coverage:
#!/bin/bash
# Description: Verify test coverage for query parameter binding
# Look for query binding tests
echo "Checking query binding tests:"
rg "Test.*query.*bind" --type go -i
# Check for embedded struct test cases
echo "Checking embedded struct test cases:"
ast-grep --pattern 'func Test$_($_) {
$$$
type $_ struct {
$_
embedded $_
$$$
}
$$$
}'32-32: Verify the appendValue function implementation
The change looks good as it aligns with the PR objective to fix embedded struct binding. However, we should verify the implementation of the new appendValue function to ensure it properly handles all query parameter scenarios.
Let's verify the implementation and usage:
✅ Verification successful
Let me try different commands to verify the appendValue function and its usage.
Let me check the equalFieldType function and test coverage since it's used in appendValue.
Implementation verified and consistent across binders
The appendValue function is properly implemented in binder/mapping.go and is consistently used across all relevant binders (query, form, header, cookie). The implementation:
- Handles comma-separated values for slice fields
- Uses
equalFieldTypeto verify field types - Maintains consistent behavior across different binding types
- Is well integrated with the existing binding system
The change in query.go is safe and aligns with the overall binding architecture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify appendValue implementation and its usage across the codebase
# Check the appendValue implementation
echo "Checking appendValue implementation:"
ast-grep --pattern 'func appendValue($params) {
$$$
}'
# Verify consistent usage across other binding types
echo "Checking usage in other binders:"
rg "appendValue\(.*\)" --type go
Length of output: 298
Script:
#!/bin/bash
# Search for appendValue function definition
echo "Searching for appendValue function definition:"
rg "func appendValue" -A 10
# Search for files in binder directory
echo -e "\nListing files in binder directory:"
fd . "binder/"
# Check imports in query.go
echo -e "\nChecking imports in query.go:"
rg "^import" -A 10 "binder/query.go"
# Search for other binding implementations
echo -e "\nSearching for other binding implementations:"
rg "type.*Binding struct" --type go
Length of output: 1763
Script:
#!/bin/bash
# Search for equalFieldType implementation
echo "Searching for equalFieldType function:"
rg "func equalFieldType" -A 10
# Check test coverage for the new appendValue functionality
echo -e "\nSearching for related test cases:"
rg "func Test.*" -A 5 "binder/mapping_test.go"
# Check if appendValue is used consistently in other binders
echo -e "\nChecking appendValue usage in other binders:"
rg "appendValue\(" binder/{query,form,header,cookie}.go
Length of output: 1554
binder/form.go (2)
Line range hint 15-40: Verify embedded struct binding behavior
The changes look good, but since this PR specifically addresses embedded struct binding, we should verify the behavior with nested structs.
#!/bin/bash
# Description: Verify test coverage for embedded struct scenarios
# Check for embedded struct test cases
echo "Checking test coverage for embedded structs:"
rg -B 2 -A 2 "type.*struct.*{.*struct.*}" ./binder/
rg -B 2 -A 2 "embedded.*struct" ./binder/
# Look for related test cases
echo -e "\nChecking related test cases:"
rg -B 2 -A 2 "func Test.*Embedded" ./binder/32-32: Verify integration with other binding types
The change to use appendValue aligns with the broader refactoring effort across binding types. However, we should verify that this integration is consistent across all binding implementations.
✅ Verification successful
Integration with other binding types is consistent
The verification shows that appendValue is consistently integrated across all relevant binding types:
- Used identically in form.go, query.go, header.go, resp_header.go, and cookie.go
- All implementations follow the same pattern:
appendValue(data, v, out, k, b.Name()) - The function is centrally defined in mapping.go and shared across bindings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of appendValue across all binding types
# Check for appendValue usage across all binder files
echo "Checking appendValue usage across binding implementations:"
rg -A 2 "appendValue\(" ./binder/
# Verify all binding type implementations
echo -e "\nChecking all binding type implementations:"
ast-grep --pattern 'type $_Binding struct {}'
Length of output: 1122
Script:
#!/bin/bash
# Let's try a different approach to verify the integration
# Check for appendValue function definition and usage
echo "Checking appendValue function definition:"
rg -p "func appendValue" ./binder/
echo -e "\nChecking all usage of appendValue:"
rg -p "appendValue\(" ./binder/
# Check all binding type implementations
echo -e "\nChecking binding type implementations:"
fd -e go . ./binder/ --exec cat {} | rg -p "type.*Binding struct"
# Check for similar value handling patterns
echo -e "\nChecking value handling patterns:"
rg -p "\.Name\(\)" ./binder/
Length of output: 2115
binder/mapping_test.go (2)
13-16: LGTM: Good negative test cases
The basic type mismatch tests appropriately verify that equalFieldType returns false for invalid cases.
Line range hint 1-93: Verify test coverage for embedded structs across the codebase
Let's ensure that embedded struct binding is adequately tested somewhere in the codebase.
✅ Verification successful
Embedded struct binding is adequately tested in the codebase
The codebase has comprehensive test coverage for embedded structs:
- Embedded struct binding is tested in
Test_Bind_Body_Form_Embeddedwhich verifies both simple and slice fields - Nested structs with query parameters are tested through multiple test cases (Query1, Query2 structs)
- Similar patterns are tested for other binding types (header, cookie) ensuring consistent behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing tests of embedded structs in the test files
echo "Searching for embedded struct tests..."
rg -l "type.*struct.*{.*}.*struct.*{" --type go --glob "*_test.go"
# Search for struct embedding syntax in test files
echo "Searching for embedded fields in tests..."
rg "^[[:space:]]*[A-Z].*// embedded" --type go --glob "*_test.go"
Length of output: 443
Script:
#!/bin/bash
# Search for test files containing embedded struct patterns
echo "Searching for embedded struct tests..."
rg "type.+struct\s*{[^}]*\s+[A-Z][^}]*}" --type go --glob "*_test.go" -A 5
# Search for struct embedding in test files (looking for anonymous fields)
echo "Searching for embedded fields in tests..."
ast-grep --pattern 'type $_ struct {
$_
$_ struct {
$$$
}
$$$
}'
# Search for potential embedded struct test cases
echo "Searching for test functions that might involve embedded structs..."
rg "func Test.*Embed" --type go
Length of output: 2211
Script:
#!/bin/bash
# Let's check the content of the embedded struct test function
echo "Checking embedded struct test implementation..."
rg "func Test_Bind_Body_Form_Embedded" -A 30 bind_test.go
# Let's also check if there are any nested struct tests with query parameters
echo "Checking nested struct query parameter tests..."
rg "func.*Query.*Nested" -A 5 bind_test.go
Length of output: 1434
bind_test.go (2)
1094-1128: Well-structured test for embedded struct binding.
The test case is well-designed and covers:
- Embedded struct binding
- Multiple field types (string, []string)
- Both embedded and non-embedded fields
- Proper form parameter naming
1123-1124:
Investigate potential issue with multiple value binding.
The comment indicates that only one value is taken when multiple values are provided for OtherString. This might be a bug in the binding logic that needs to be investigated.
| Name string | ||
| Address string `query:"address"` | ||
| Age int `query:"AGE"` | ||
| } | ||
| require.True(t, equalFieldType(&user, reflect.String, "name")) | ||
| require.True(t, equalFieldType(&user, reflect.String, "Name")) | ||
| require.True(t, equalFieldType(&user, reflect.String, "address")) | ||
| require.True(t, equalFieldType(&user, reflect.String, "Address")) | ||
| require.True(t, equalFieldType(&user, reflect.Int, "AGE")) | ||
| require.True(t, equalFieldType(&user, reflect.Int, "age")) | ||
| require.True(t, equalFieldType(&user, reflect.String, "name", "query")) | ||
| require.True(t, equalFieldType(&user, reflect.String, "Name", "query")) | ||
| require.True(t, equalFieldType(&user, reflect.String, "address", "query")) | ||
| require.True(t, equalFieldType(&user, reflect.String, "Address", "query")) | ||
| require.True(t, equalFieldType(&user, reflect.Int, "AGE", "query")) | ||
| require.True(t, equalFieldType(&user, reflect.Int, "age", "query")) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for embedded structs
While the existing test cases provide good coverage for field names, tags, and case sensitivity, they don't cover the embedded struct binding that this PR aims to fix.
Consider adding test cases like:
type Address struct {
Street string `query:"street"`
}
type User struct {
Address // embedded struct
Name string `query:"name"`
}
var user User
require.True(t, equalFieldType(&user, reflect.String, "street", "query"))
require.True(t, equalFieldType(&user, reflect.String, "Street", "query"))This will ensure the fix for embedded struct binding is properly tested.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3203 +/- ##
==========================================
+ Coverage 82.79% 82.97% +0.17%
==========================================
Files 114 114
Lines 11151 11129 -22
==========================================
+ Hits 9233 9234 +1
+ Misses 1519 1496 -23
Partials 399 399
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@michal-laskowski Run |
|
@michal-laskowski Does this PR fixes #3159 ? |
|
@gaby if a fix is needed, write how it should work and I will make the changes. I am not sure what the expected behavior is. IMHO - we should not split input values. I don't know how v2 was working, i use only v3. Consider this : // go test -run Test_Bind_Body_Form_WithArray
func Test_Bind_Body_Form_WithArray(b *testing.T) {
var err error
app := New()
c := app.AcquireCtx(&fasthttp.RequestCtx{})
type Demo struct {
Item []string `json:"item" form:"item"`
Description []string `json:"description" form:"description"`
}
//form got 3 values one with comma
//?? do we expect 3 values on output or 4?
body := []byte("item=item1&item=item2&item=item3&description=description1%2C%20with comma.&description=description2&description=description3&q=%2C")
c.Request().SetBody(body)
c.Request().Header.SetContentType(MIMEApplicationForm)
c.Request().Header.SetContentLength(len(body))
d := new(Demo)
err = c.Bind().Body(d)
require.NoError(b, err)
require.Equal(b, []string{"item1", "item2", "item3"}, d.Item)
require.Equal(b, []string{"description1, with comma.", "description2", "description3"}, d.Description)
}
// go test -run Test_Bind_Body_Form_WithArray_SingleValueInForm
func Test_Bind_Body_Form_WithArray_SingleValueInForm(b *testing.T) {
var err error
app := New()
c := app.AcquireCtx(&fasthttp.RequestCtx{})
type Demo struct {
Item []string `json:"item" form:"item"`
Description []string `json:"description" form:"description"`
}
//form got 1 valu with comma
//?? do we expect 1 value on output or 2?
body := []byte("item=item1&item=item2&item=item3&description=description1%2C%20with comma.")
c.Request().SetBody(body)
c.Request().Header.SetContentType(MIMEApplicationForm)
c.Request().Header.SetContentLength(len(body))
d := new(Demo)
err = c.Bind().Body(d)
require.NoError(b, err)
require.Equal(b, []string{"item1", "item2", "item3"}, d.Item)
require.Equal(b, []string{"description1", " with comma."}, d.Description)
} |
|
@coderabbitai please update the summary with the implemented fixes and changes to make the pull requests more understandable for everyone |
|
Tip For best results, initiate chat on the files or code changes. Here's an updated summary of the pull request to make it more understandable for everyone, including the implemented fixes and changes, and taking into account the linked issues: 🩹 Fix: Embedded Struct Binding This pull request addresses issues with the binding of embedded structs within the Fiber framework. It enhances the binding logic to properly handle embedded fields across various binding methods, ensuring that data from requests are correctly mapped to embedded structs in handlers. Key Changes:
Related Issues:
Additional Notes:
I hope this updated summary clarifies the purpose and changes of the pull request. Let me know if there's anything else I can assist with. |
|
@michal-laskowski pls fix the lint issues with the |
|
we can ignore the benchmark problem its still the problem with the same named tests |
|
@michal-laskowski thanks for the pull request
|
|
@michal-laskowski only the test and linting are still missing |
|
@ReneWerner87 Look at test in |
|
@michal-laskowski okay thanks for the hint, I had overlooked it can you do a test like it is done in other common frameworks @coderabbitai how do the other rest frameworks behave in these cases with the commas? please make a list for the mentioned and other popular ones and give us the most common method for the behavior on the web |
|
Tip For best results, initiate chat on the files or code changes. Certainly! Here's how the mentioned frameworks handle commas in parameters during binding: Express.js (Node.js): In Express.js, when parsing query parameters or form data, commas in parameter values are treated as part of the string and are not split automatically. Multiple parameters with the same name are collected into an array. Example: // Query string: ?item=item1,item2&item=item3
app.get('/', (req, res) => {
console.log(req.query.item);
// Output: ['item1,item2', 'item3']
});If you need to split values on commas, you would need to manually process them in your handler. Gin (Go): Gin does not split parameter values on commas by default. Commas are considered part of the parameter value. Multiple parameters with the same name are bound into slices. Example: type Demo struct {
Item []string `form:"item"`
}
// Form data: item=item1,item2&item=item3
// After binding, Demo.Item = ["item1,item2", "item3"]To split values on commas, additional parsing is required after binding. Django (Python): Django treats commas as part of the parameter value. Multiple parameters with the same name are collected into a list. Example: # Query string: ?item=item1,item2&item=item3
def view(request):
items = request.GET.getlist('item')
# items = ['item1,item2', 'item3']Manual processing is needed to split values on commas. Laravel (PHP): In Laravel, commas in parameter values are not split automatically. Multiple parameters with the same name are retrieved as an array. Example: // Query string: ?item=item1,item2&item=item3
$items = $request->input('item');
// $items = ['item1,item2', 'item3']You would need to explode the string on commas if splitting is desired. Spring Boot (Java): Spring Boot treats commas within parameter values as part of the string. Multiple parameters with the same name map to arrays or lists. Example: @GetMapping("/items")
public void getItems(@RequestParam List<String> item) {
// item = ["item1,item2", "item3"]
}To split values on commas, custom logic or converters are necessary. Other Popular Frameworks:
Common Behavior: Across these frameworks, the common behavior is:
Conclusion: The standard practice on the web is not to split parameter values on commas during the binding process. If splitting is required, it should be handled explicitly in the application logic after binding. I hope this helps clarify how other frameworks handle commas in parameters! |
|
so the logic should be to leave the commas untouched @michal-laskowski does that help with the finalization ? |
|
#3223 (comment) |
|
@michal-laskowski This PR fixes #2859, right? |
Description
Fixed embedded struct binding.
Type of change
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.