fix: required rule fails for int/uint values equal to 0#311
fix: required rule fails for int/uint values equal to 0#311inhere merged 1 commit intogookit:masterfrom
Conversation
Pull Request Test Coverage Report for Build 17487708601Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a validation bug where the required rule incorrectly failed for numeric fields with zero values (0 for int/uint types). The fix ensures that zero is treated as a valid value for required validation rules, addressing issue #302.
Key changes:
- Added
isIgnoreableZeroNumerichelper method to identify zero-valued numeric fields that should be considered valid - Updated all required-related validation methods to use this helper, allowing zero values for numeric types to pass validation
- Added comprehensive test coverage for the fix
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| validators.go | Added isIgnoreableZeroNumeric helper and updated all required validation methods to handle numeric zero values correctly |
| issues_test.go | Added regression test TestIssue_302 and fixed test isolation issue in existing test |
| func (v *Validation) isIgnoreableZeroNumeric(field string) bool { | ||
| if v.data != nil && v.data.Type() == sourceMap { | ||
| if val, ok := v.data.Get(field); ok { | ||
| k := reflect.ValueOf(val).Kind() | ||
| return k >= reflect.Int && k <= reflect.Float64 | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
The logic incorrectly returns true for all numeric types regardless of their actual value. It should check if the value is specifically zero. Currently, any numeric value (e.g., 42, -5, 3.14) would be considered 'ignoreable zero numeric', which is incorrect.
| return val != nil && !IsEmpty(val) || v.isIgnoreableZeroNumeric(sourceField) | ||
| } | ||
| } else if Enum(dstVal, args) { | ||
| return val != nil && !IsEmpty(val) | ||
| return val != nil && !IsEmpty(val) || v.isIgnoreableZeroNumeric(sourceField) |
There was a problem hiding this comment.
Operator precedence issue: the || has lower precedence than &&, so this expression is evaluated as (val != nil && !IsEmpty(val)) || v.isIgnoreableZeroNumeric(sourceField). This means the helper method result is returned regardless of the val conditions, which may not be the intended logic. Consider adding parentheses for clarity: return (val != nil && !IsEmpty(val)) || v.isIgnoreableZeroNumeric(sourceField)
| } | ||
| } else if Enum(dstVal, args) { | ||
| return val != nil && !IsEmpty(val) | ||
| return val != nil && !IsEmpty(val) || v.isIgnoreableZeroNumeric(sourceField) |
There was a problem hiding this comment.
The same logical pattern !IsEmpty(val) || v.isIgnoreableZeroNumeric(sourceField) is repeated across multiple validation methods. Consider extracting this into a helper method like isValidRequiredValue(sourceField, val) to reduce duplication and improve maintainability.
| if dstVal, has, _ := v.tryGet(dstField); has { | ||
| if !Enum(dstVal, values) { | ||
| return !IsEmpty(val) | ||
| return !IsEmpty(val) || v.isIgnoreableZeroNumeric(sourceField) |
There was a problem hiding this comment.
The same logical pattern !IsEmpty(val) || v.isIgnoreableZeroNumeric(sourceField) is repeated across multiple validation methods. Consider extracting this into a helper method like isValidRequiredValue(sourceField, val) to reduce duplication and improve maintainability.
| for _, field := range fields { | ||
| if _, has, zero := v.tryGet(field); has && !zero { | ||
| return !IsEmpty(val) | ||
| return !IsEmpty(val) || v.isIgnoreableZeroNumeric(sourceField) |
There was a problem hiding this comment.
The same logical pattern !IsEmpty(val) || v.isIgnoreableZeroNumeric(sourceField) is repeated across multiple validation methods. Consider extracting this into a helper method like isValidRequiredValue(sourceField, val) to reduce duplication and improve maintainability.
|
|
||
| // all fields exist | ||
| return !IsEmpty(val) | ||
| return !IsEmpty(val) || v.isIgnoreableZeroNumeric(sourceField) |
There was a problem hiding this comment.
The same logical pattern !IsEmpty(val) || v.isIgnoreableZeroNumeric(sourceField) is repeated across multiple validation methods. Consider extracting this into a helper method like isValidRequiredValue(sourceField, val) to reduce duplication and improve maintainability.
| for _, field := range fields { | ||
| if _, has, zero := v.tryGet(field); !has || zero { | ||
| return !IsEmpty(val) | ||
| return !IsEmpty(val) || v.isIgnoreableZeroNumeric(sourceField) |
There was a problem hiding this comment.
The same logical pattern !IsEmpty(val) || v.isIgnoreableZeroNumeric(sourceField) is repeated across multiple validation methods. Consider extracting this into a helper method like isValidRequiredValue(sourceField, val) to reduce duplication and improve maintainability.
|
|
||
| // all fields not exists, required | ||
| return !IsEmpty(val) | ||
| return !IsEmpty(val) || v.isIgnoreableZeroNumeric(sourceField) |
There was a problem hiding this comment.
The same logical pattern !IsEmpty(val) || v.isIgnoreableZeroNumeric(sourceField) is repeated across multiple validation methods. Consider extracting this into a helper method like isValidRequiredValue(sourceField, val) to reduce duplication and improve maintainability.
close #302
This PR addresses an issue with the validation of required rules for numeric fields, specifically ensuring that zero values for int/uint types are handled correctly as valid input in certain required scenarios. The main changes involve updating the logic of required-related validation methods and adding a new helper to identify zero numeric values that should be considered valid.
Validation logic improvements for numeric zero values:
isIgnoreableZeroNumericto theValidationstruct to detect if a field is a numeric type with a zero value, allowing such values to be considered valid for required rules.Required,RequiredIf,RequiredUnless,RequiredWith,RequiredWithAll,RequiredWithout, andRequiredWithoutAllmethods to useisIgnoreableZeroNumeric, ensuring that zero values for numeric types are not incorrectly treated as missing or empty. [1] [2] [3] [4] [5] [6] [7] [8] [9]Testing enhancements:
TestIssue_302to verify that required rules correctly accept zero values for int/uint fields, preventing regression on this behavior.TestIssues_I36T2Bto ensure a fresh map is used in validation, improving test reliability.