🐛 [Bug]: client.SetValWithStruct set zero value if the field is slice

[ksw2000]

Bug Description

The function SetValWithStruct() sets values using structs. It skips zero values, such as 0 and false. However, if the fields are slices (excluding bool slices), all the values in the slices will be set.

How to Reproduce

To reproduce the bugs, we can simply add a test in client/request_test.go:

func TestSetValWithStructForSlices(t *testing.T) {
	p := &QueryParam{
		Args: fasthttp.AcquireArgs(),
	}
	SetValWithStruct(p, "param", struct {
		TZeroInt   int
		TInt       int
		TIntSlice  []int
		TBoolSlice []bool
	}{
		TZeroInt:   0,
		TInt:       1,
		TIntSlice:  []int{0, 1, 2},
		TBoolSlice: []bool{true, false},
	})

	fmt.Println(string(p.Peek("TZeroInt")))
	fmt.Println(string(p.Peek("TInt")))
	for _, v := range p.PeekMulti("TIntSlice") {
		fmt.Print(string(v), ", ")
	}
	fmt.Println()
	for _, v := range p.PeekMulti("TBoolSlice") {
		fmt.Print(string(v), ", ")
	}
	fmt.Println()
}

cd client
go test --run TestSetValWithStructForSlices -v

result:

1
0, 1, 2, 
true,

Expected Behavior

1
1, 2, 
true,

The SetValWithStruct() should skip 0 in the int slice too.

Fiber Version

latest

Code Snippet (optional)

No response

Checklist:

• [X] I agree to follow Fiber's Code of Conduct.
• [X] I have checked for existing issues that describe my problem prior to opening this one.
• [X] I understand that improperly formatted bug reports may be closed without explanation.

[welcome[bot]]

Thanks for opening your first issue here! 🎉 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

[JIeJaitt]

@ksw2000 Hi, are you sure SetValWithStruct() should also skip the 0's in the int slice, I don't think [1,2] and [0,1,2] are the same slice.

[ksw2000]

@JIeJaitt I'm not entirely sure, since there are two different logics for processing int slice and bool slice in the SetValWithStruct() function. In my opinion, there are absolutely bugs.

If the 0 in [0, 1, 2] should be stored. I think false in [true, false] should be stored too. That is to say, the expected behavior is changed to:

1
0, 1, 2, 
true, false,

SetValWithStruct does not consider zero values when processing slices. However, since it only stores true when processing bool slices, zero values are considered in bool slices.

https://github.com/gofiber/fiber/blob/7b3a36f22fc1166ceb9cb78cf69b3a2f95d077da/client/request.go#L948-L951

[JIeJaitt]

@ksw2000 I think what you said makes sense. There is indeed a problem here, which creates ambiguity for users. I think if possible, two functions are provided here. One is the normal SetValWithStruct() function that will store 0 and false, and the other is the SetValWithStructSpecial() function that will skip 0 and false values.

[ksw2000]

@JIeJaitt Based on its usage, I think the zero values in the slice might need to be removed, since SetValWithStruct treats the slice as a single key with multiple values. Perhaps we should consult maintainers. Besides, maybe we can use a tag to specify not to skip zero values instead.

[neowulf]

Hi folks, I'd like to take a stab at fixing this issue. What's the suggested fix?

[gaby]

@ksw2000 @JIeJaitt Thoughts?

[ksw2000]

@gaby I think the zero values in the slice might need to be removed, since SetValWithStruct treats the slice as a single key with multiple values. If you agree with my viewpoint, I can fix the bug.

[gaby]

@efectn Thoughts?

[JIeJaitt]

@gaby @ksw2000 @neowulf I think the meaning of skipping the 0 of int type and false of boolean type here is unclear. If we want to set a parameter through this method, don't we need 0 and false?

If I have a free product, I need to set its price to 0.0 and it is temporarily not sellable

func SubmitProductForm(client *Client, product ProductForm) (*Response, error) {
    req := client.AcquireRequest()
    defer client.ReleaseRequest(req)

    req.SetMethod(fiber.MethodPost)
    req.SetURL("https://api.example.com/products")
    req.SetFormDataWithStruct(product)

    return req.Send()
}


// SetFormDataWithStruct sets form data from a struct using Fiber's SetValWithStruct
func (r *Request) SetFormDataWithStruct(s interface{}) *Request {
    if r.formData == nil {
        r.formData = make(fiber.Map)
    }
    fiber.SetValWithStruct(r.formData, "form", s)
    return r
}

// use
product := ProductForm{
    Name:     "Free Sample",
    Price:    0.0, // price == 0, free product
    Quantity: 100,
    Sale: false
}
response, err := SubmitProductForm(client, product)

In summary, I believe there is no need to control whether to pass 0 and false or provide two functions through a single tag, as this is completely redundant and users will choose the parameter values they need to pass themselves.

[ksw2000]

@gaby , what do you think? I believe we should fix the bug first, and in order to maintain backward compatibility for SetValWithStruct, we should keep its original behavior. If you agree with my thoughts, I can create a pull request.

[efectn]

Hi @ksw2000 you can create a PR to fix the issue. You don't need to mind backward-compatibility since v3 hasn't been released yet. I also agree with you about the fact that both 0, false should be covered by SetValWithStruct