🩹 fix: make SetValWithStruct set zero values and support more types #3167 #3227
Conversation
WalkthroughThe changes in this pull request modify the Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (4)client/request_test.go (4)
The additions of
The test assertions for
The addition of test assertions for zero values in Line range hint The benchmark tests have been appropriately updated to maintain parity with the regular tests, ensuring performance characteristics are measured for all new functionality. 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 (
|
ksw2000
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
client/request_test.go (1)
Line range hint
1468-1515: Exclude unsupported types from benchmark testThe benchmark test includes function and map types, which are not supported for serialization into parameters. Removing these types will make the benchmark more accurate and focused on measuring performance with valid data types.
Apply this diff to modify the benchmark test:
type args struct { //nolint:govet // Aligning the struct fields is not necessary. TString string TSlice []string TIntSlice []int `param:"int_slice"` unexport int TInt int TUint uint TFloat float64 TComplex complex128 TBool bool - TFunc func() - TMap map[int]int } b.Run("the struct should be applied", func(b *testing.B) { ... for i := 0; i < b.N; i++ { SetValWithStruct(p, "param", args{ unexport: 5, TInt: 5, TUint: 5, TString: "string", TFloat: 3.1, TComplex: 3 + 4i, TBool: false, TSlice: []string{"foo", "bar"}, TIntSlice: []int{0, 1, 2}, - TFunc: func() {}, - TMap: map[int]int{1: 2}, }) } ... })
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
client/request_test.go (2)
1382-1389: Improve test readability for zero value checksThe zero value test case is buried within multiple similar test blocks. Consider extracting zero value tests into a dedicated test case for better visibility and documentation of the fixed behavior.
+ t.Run("zero values should be set", func(t *testing.T) { + t.Parallel() + p := &QueryParam{ + Args: fasthttp.AcquireArgs(), + } + + SetValWithStruct(p, "param", args{ + TInt: 0, + TUint: 0, + TBool: false, + TIntSlice: []int{0}, + }) + + require.Equal(t, []byte("0"), p.Peek("TInt")) + require.Equal(t, []byte("0"), p.Peek("TUint")) + require.Equal(t, []byte("false"), p.Peek("TBool")) + require.Equal(t, []byte("0"), p.PeekMulti("int_slice")[0]) + })
Line range hint
1477-1524: Streamline benchmarks to focus on supported typesThe benchmark includes unsupported types (TFunc, TMap) which may skew performance metrics. Consider focusing benchmarks on the core supported types to get more accurate performance measurements.
type args struct { TString string TSlice []string TIntSlice []int `param:"int_slice"` unexport int TInt int TUint uint TFloat float64 TComplex complex128 TBool bool - TFunc func() - TMap map[int]int }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
client/request_test.go(6 hunks)
🔇 Additional comments (1)
client/request_test.go (1)
1320-1332: Remove unsupported types from test struct
Including function types (TFunc) and map types (TMap) in the test struct args is not meaningful, as these types are not typically serialized into parameters.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
client/request.go(4 hunks)
🔇 Additional comments (4)
client/request.go (4)
956-957: Support for unsigned integer types added
The addition correctly handles unsigned integer types using strconv.FormatUint, ensuring that uint, uint8, uint16, uint32, uint64, and uintptr are properly converted to strings.
958-959: Support for complex number types added
The inclusion of complex number types (complex64, complex128) and their handling using strconv.FormatComplex is correctly implemented. This allows complex numbers to be added as parameters.
963-964: Boolean false values are now properly handled
The addition of the else branch ensures that when a boolean value is false, it is explicitly added as "false" to the parameters. This change improves the consistency of boolean value handling.
975-977:
Avoid adding unsupported types like functions and maps to parameters
Including function types (func) and map types (map) as parameters may lead to unintended behavior or serialization issues. Functions cannot be serialized, and maps may not serialize in a predictable way suitable for query parameters or form data. Excluding these types will ensure that only supported and serializable types are added.
Apply this diff to exclude functions and maps from being added:
default:
+ if val.Kind() == reflect.Func || val.Kind() == reflect.Map {
+ return
+ }
if val.CanInterface() {
p.Add(name, fmt.Sprintf("%#v", val.Interface()))
}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3227 +/- ##
==========================================
+ Coverage 82.69% 82.73% +0.04%
==========================================
Files 115 115
Lines 11377 11383 +6
==========================================
+ Hits 9408 9418 +10
+ Misses 1561 1558 -3
+ Partials 408 407 -1
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: 0
🧹 Outside diff range and nitpick comments (1)
client/request.go (1)
932-939: Update documentation to reflect zero value handlingThe documentation should explicitly mention that the function now sets both zero and non-zero values, as this is a significant behavioral change mentioned in the PR objectives.
Add a note like:
// SetValWithStruct stores the fields of `v` into `p`. // `tagName` specifies the key used to store into `p`. If not specified, // the field name is used by default. // `v` is a struct or a pointer to a struct containing some data. +// All field values, including zero values, are stored. // Fields in `v` should be string, int, int8, int16, int32, int64, uint,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
client/request.go(2 hunks)client/request_test.go(6 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
client/request.go
[warning] 977-977: client/request.go#L977
Added line #L977 was not covered by tests
🔇 Additional comments (7)
client/request.go (3)
958-963: LGTM! Support for additional types added
The implementation correctly adds support for unsigned integers and complex numbers as specified in the PR objectives.
967-968: LGTM! Explicit handling of boolean false values
The implementation now explicitly sets "false" for boolean values, which aligns with the PR objective of handling zero values.
977-977:
Add test coverage for default case
The default case in the switch statement is not covered by tests.
Consider adding test cases that exercise the default case by including unsupported types in the test struct.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 977-977: client/request.go#L977
Added line #L977 was not covered by tests
client/request_test.go (4)
1320-1331: LGTM! Test struct updated with new types
The test struct has been properly updated to include the new supported types (uint and complex128).
1342-1348: LGTM! Comprehensive test values
The test values appropriately cover:
- New types (uint and complex)
- Zero values in slices (TIntSlice starting with 0)
- Boolean false values
1376-1384: LGTM! Zero value assertions added
The test now properly verifies that zero values (0) in slices are correctly handled.
1353-1357: LGTM! New type assertions added
The test properly verifies the handling of new types:
- uint values
- complex number formatting
|
benchmark errors are ok the other errors are caused by a non-updated bench which was also changed in master as we switched from go 1.22 to 1.23 |
Description
According to issue #3167, the
SetValWithStructfunction originally set only non-zero values, but it also set zero values in slices. After discussion, we have changed the behavior ofSetValWithStructto set both non-zero and zero values, starting from Fiber v3.Additionally, in this PR, I have made
SetValWithStructmore general by supporting more types, includinguint,complex, and others.Since I did not find any statements in the documentation describing that
SetValWithStructabout handling zero values, I did not update the documentation. If anyone finds any, I will make the necessary changes.Fixes #3167
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