fix: ignore empty form values in http request

[kevwan]

No description provided.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.56%. Comparing base (8690859) to head (5d2b734).
Report is 211 commits behind head on master.

Additional details and impacted files

Files with missing lines	Coverage Δ
rest/httpx/util.go	100.00% <100.00%> (ø)

... and 7 files with indirect coverage changes

[kevwan]

Changes Overview:

1. Modified rest/httpx/util.go:

   • Added logic to skip empty form values in the GetFormValues function
   • This change prevents empty strings from being included in the form values array

2. Modified rest/httpx/requests_test.go:

   • Added new test cases for handling empty values and array formats
   • Updated existing test expectations to match the new behavior

Detailed Review:

1. In rest/httpx/util.go:

for name, values := range r.Form {
    filtered := make([]string, 0, len(values))
    for _, v := range values {
+       if len(v) == 0 {
+           continue
+       }

✅ This change is good because:

• It filters out empty form values which can help reduce noise in request processing
• The implementation is simple and efficient
• It maintains the maxFormParamCount limit logic

2. In rest/httpx/requests_test.go, several test cases were added/modified:

a. New test case for slice with comma-separated values:

t.Run("slice with one value on array format", func(t *testing.T) {
    var v struct {
        Names string `form:"names"`
    }
    // ...
    assert.Equal(t, "1,2,3", v.Names)
})

✅ Good test coverage for comma-separated values

b. Modified behavior for empty values:

// Changed from asserting [""] to asserting empty
assert.Empty(t, v.Name)

// Changed from asserting ["", "1"] to asserting ["1"]
assert.ElementsMatch(t, []string{"1"}, v.Name)

✅ These changes are consistent with the new empty value filtering behavior

c. New test cases for integer arrays:

t.Run("slice with one value on integer array format", func(t *testing.T) {
    var v struct {
        Numbers []int `form:"numbers,optional"`
    }
    // ...
})

✅ Good addition of tests for:

• Empty number arrays
• Arrays with empty values mixed with valid numbers

Suggestions for Improvement:

1. Consider adding a comment in util.go explaining why empty values are filtered out
2. Consider adding test cases for other data types (float, bool) to ensure consistent behavior

Security Considerations:
✅ No security concerns identified - the changes don't introduce any potential security issues

Performance Impact:
✅ Minimal performance impact - the additional empty check is O(1) and reduces subsequent processing

Compatibility:
⚠️ This is a behavior change that might affect existing applications that rely on empty values being included. However, the change appears intentional and beneficial for most use cases.

Overall Assessment:
This PR represents a good improvement to the form value handling in go-zero. The changes are well-tested and implement a reasonable default behavior for handling empty form values. The code is clean and maintainable.

Recommendation: ✅ Approve
The changes are well-implemented and properly tested. The behavior change is reasonable and likely to be beneficial for most users.

[kevwan]

Code Review Summary:

1. Changes in rest/httpx/util.go:
   ✅ Added empty value filtering with clear documentation

// ignore empty values, especially for optional int parameters
// e.g. /api?ids=
// e.g. /api
// type Req struct {
//     IDs []int `form:"ids,optional"`
// }
if len(v) == 0 {
    continue
}

The added comments clearly explain the purpose and provide good examples of use cases. This is a beneficial improvement to the codebase.

2. Changes in rest/httpx/requests_test.go:
   ✅ Comprehensive test coverage added for multiple scenarios:

• String arrays with comma-separated values
• Empty value handling for integer arrays
• Empty value handling for float64 arrays
• Mixed empty and non-empty values

Test improvements include:

// Changed behavior:
- assert.ElementsMatch(t, []string{""}, v.Name)
+ assert.Empty(t, v.Name)

// And:
- assert.ElementsMatch(t, []string{"", "1"}, v.Name)
+ assert.ElementsMatch(t, []string{"1"}, v.Name)

Strong Points:

1. Well-documented changes with clear examples
2. Comprehensive test coverage for different data types (string, int, float64)
3. Consistent behavior across different numeric types
4. Proper handling of empty values in optional fields
5. Tests cover both single empty values and mixed empty/non-empty scenarios

Enhancement Suggestions:

1. Consider adding test cases for:
   • Boolean array types
   • Fields with custom types that implement encoding.TextUnmarshaler
2. Consider adding a constant or configuration option to make this behavior toggleable if needed for backward compatibility

Impact Analysis:

• 🔄 This is a behavior change that affects how empty values are handled
• ✅ The change is backward compatible for most use cases
• ✅ The change makes the behavior more intuitive for optional parameters
• ✅ Reduces potential issues with type conversion for numeric types

The PR looks ready to merge. The implementation is clean, well-tested, and properly documented. The behavior change is reasonable and improves the handling of form values, especially for optional numeric arrays.