🩹 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
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}, }) } ... })
| if val.CanInterface() { | ||
| p.Add(name, fmt.Sprintf("%#v", val.Interface())) | ||
| } |
There was a problem hiding this comment.
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:
func SetValWithStruct(p WithStruct, tagName string, v any) {
...
setVal = func(name string, val reflect.Value) {
switch val.Kind() {
...
default:
+ if val.Kind() == reflect.Func || val.Kind() == reflect.Map {
+ return
+ }
if val.CanInterface() {
p.Add(name, fmt.Sprintf("%#v", val.Interface()))
}
}
}
...
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@ReneWerner87 My design involves writing all types that can be converted into an interface using fmt.Sprintf("%#v", val.Interface()). Do you have any thoughts on this design?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| // test SetValWithStruct via QueryParam struct. | ||
| 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 | ||
| TFunc func() | ||
| TBool bool | ||
| TMap map[int]int |
There was a problem hiding this comment.
🛠️ Refactor suggestion
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 and may cause serialization issues or unexpected behavior. It is recommended to remove these fields from the test to focus on supported and serializable types.
Apply this diff to remove unsupported types from the test struct:
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
- TFunc func()
TBool bool
- TMap map[int]int
}📝 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.
| // test SetValWithStruct via QueryParam struct. | |
| 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 | |
| TFunc func() | |
| TBool bool | |
| TMap map[int]int | |
| // test SetValWithStruct via QueryParam struct. | |
| 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 |
| TUint: 5, | ||
| TString: "string", | ||
| TFloat: 3.1, | ||
| TComplex: 3 + 4i, | ||
| TBool: false, | ||
| TSlice: []string{"foo", "bar"}, | ||
| TIntSlice: []int{1, 2}, | ||
| TIntSlice: []int{0, 1, 2}, | ||
| TFunc: func() {}, | ||
| TMap: map[int]int{1: 2}, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust test cases to reflect supported types only
The test case includes unsupported types like functions and maps. These types should be excluded from the test to prevent misleading results and to focus on verifying the behavior with supported types.
Apply this diff to adjust the test case:
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},
})📝 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.
| TUint: 5, | |
| TString: "string", | |
| TFloat: 3.1, | |
| TComplex: 3 + 4i, | |
| TBool: false, | |
| TSlice: []string{"foo", "bar"}, | |
| TIntSlice: []int{1, 2}, | |
| TIntSlice: []int{0, 1, 2}, | |
| TFunc: func() {}, | |
| TMap: map[int]int{1: 2}, | |
| TUint: 5, | |
| TString: "string", | |
| TFloat: 3.1, | |
| TComplex: 3 + 4i, | |
| TBool: false, | |
| TSlice: []string{"foo", "bar"}, | |
| TIntSlice: []int{0, 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()))
}| // SetValWithStruct sets values using a struct. | ||
| // `p` is a structure that implements the WithStruct interface. | ||
| // The field name is specified by `tagName`. | ||
| // `v` is a struct containing some data. | ||
| // Note: This method supports all values that can be converted to an interface. |
There was a problem hiding this comment.
Potential misalignment with issue #3167: Zero values are now being set
The changes to SetValWithStruct now cause zero values to be included in parameters. However, the linked issue #3167 specifies that zero values should be skipped consistently across all contexts, including slices. This modification may not align with the expected behavior outlined in the issue. Please revisit the implementation to ensure it meets the desired functionality of skipping zero values.
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