Add EachWithOptions for Validating Arrays of Objects#12
Conversation
WalkthroughThe changes enhance the validator package by adding a new function Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant EachWithOptions
participant ElementValidator
participant StructConverter
Caller->>EachWithOptions: Invoke validator(slice/array)
EachWithOptions->>EachWithOptions: Check for nil, type, and empty slice/array
alt Element is present
EachWithOptions->>ElementValidator: Process element at index i
alt Element is a struct
ElementValidator->>StructConverter: Convert struct to map
StructConverter-->>ElementValidator: Return converted map
else Element is map
ElementValidator-->>EachWithOptions: Use element as is
end
ElementValidator->>EachWithOptions: Validate with provided options
else Invalid input
EachWithOptions-->>Caller: Return error immediately
end
EachWithOptions-->>Caller: Return final validation result
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for "github.com/gin-gonic/gin"" Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
validator/validator.go (1)
88-111: StructToMap implementation looks good but could handle edge cases better.The function correctly converts structs to maps using reflection and respects JSON tags. It also properly checks for exported fields. However, it doesn't handle nil or non-struct inputs.
Consider improving robustness with these modifications:
// StructToMap converts a struct to a map[string]interface{} for validation func StructToMap(obj interface{}) map[string]interface{} { + if obj == nil { + return make(map[string]interface{}) + } + result := make(map[string]interface{}) v := reflect.ValueOf(obj) + + // Handle non-struct types + if v.Kind() != reflect.Struct { + return result + } + t := reflect.TypeOf(obj) for i := range v.NumField() { field := t.Field(i) // Use the json tag if present, otherwise fall back to field name jsonTag := field.Tag.Get("json") key := field.Name if jsonTag != "" { // Split on "," to handle options like "omitempty" if parts := strings.Split(jsonTag, ","); len(parts) > 0 { key = parts[0] } } // Only process exported fields if field.PkgPath == "" { // PkgPath is empty for exported fields result[key] = v.Field(i).Interface() } } return result }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
validator/range.go(1 hunks)validator/range_test.go(1 hunks)validator/validator.go(3 hunks)
🔇 Additional comments (2)
validator/range.go (1)
113-142: Well-implemented function with thorough error handling.The new
EachWithOptionsfunction provides robust validation for array elements with comprehensive error handling. It handles nil values, type checking, and provides clear error messages with element indices.One minor suggestion to improve performance:
// EachWithOptions applies a set of validation options to each element in a slice or array, returning the first error func EachWithOptions(options []ValidationOption) ValidatorFunc { return func(value interface{}) error { if value == nil { return fmt.Errorf("value must be a non-nil slice or array") } v := reflect.ValueOf(value) if v.Kind() != reflect.Slice && v.Kind() != reflect.Array { return fmt.Errorf("value must be a slice or array, got %T", value) } if v.Len() == 0 { return nil } + // Pre-allocate capacity for nestedBody to avoid multiple reallocations + nestedBodies := make([]map[string]interface{}, 0, v.Len()) + for i := 0; i < v.Len(); i++ { elem := v.Index(i).Interface() nestedBody, ok := elem.(map[string]interface{}) if !ok { if reflect.TypeOf(elem).Kind() == reflect.Struct { nestedBody = StructToMap(elem) } else { return fmt.Errorf("element at index %d must be an object, got %T", i, elem) } } if err := Validate(nestedBody, options); err != nil { return fmt.Errorf("element at index %d: %v", i, err) } + nestedBodies = append(nestedBodies, nestedBody) } return nil } }This is an optional optimization that could be useful if you're handling large arrays.
validator/range_test.go (1)
148-257: Comprehensive test coverage with good edge cases.The test function thoroughly covers various scenarios including happy paths, error cases, and edge conditions. The test cases for mixed-case JSON keys, empty arrays, and struct handling are particularly valuable.
Consider adding one additional test case to verify handling of deeply nested structures:
{ name: "json with mixed case keys", input: []interface{}{ map[string]interface{}{ "name": "ahmed", "Name": 123, "value": "test", }, }, expected: nil, }, + { + name: "nested structures", + input: []interface{}{ + map[string]interface{}{ + "name": "parent", + "value": map[string]interface{}{ + "name": "child", + "value": "nested", + }, + }, + }, + expected: nil, + }, }This would help ensure that the validation works correctly with complex nested data structures.
| // Check if the field is required but missing *before* running validators | ||
| if !option.IsOptional && !exists { | ||
| return fmt.Errorf("%s is required", option.Key) | ||
| } |
There was a problem hiding this comment.
Code duplication concern.
This new validation check for required fields is duplicated at lines 61-63. Having multiple checks for the same condition can lead to confusion and maintenance issues.
// Validate checks the request body against the validation options and returns the first error.
func Validate(body map[string]interface{}, options []ValidationOption) error {
for _, option := range options {
value, exists := body[option.Key]
// Skip validation if the field is optional and not present
if option.IsOptional && !exists {
continue
}
- // Check if the field is required but missing *before* running validators
- if !option.IsOptional && !exists {
- return fmt.Errorf("%s is required", option.Key)
- }
-
// Apply transformations
if exists {
for _, transformer := range option.Transformers {
value = transformer(value)
}
body[option.Key] = value // Update the body with the transformed value
}
// Run all validators for the field (if it exists)
for _, validator := range option.Validators {
if err := validator.Func(value); err != nil {
return fmt.Errorf("%s", validator.Message)
}
}
// Check if the field is required but missing
if !option.IsOptional && !exists {
return fmt.Errorf("'%s' is required", option.Key)
}Either keep only one of the checks or make their purposes clear with improved comments if they serve different purposes.
📝 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.
| // Check if the field is required but missing *before* running validators | |
| if !option.IsOptional && !exists { | |
| return fmt.Errorf("%s is required", option.Key) | |
| } | |
| // Validate checks the request body against the validation options and returns the first error. | |
| func Validate(body map[string]interface{}, options []ValidationOption) error { | |
| for _, option := range options { | |
| value, exists := body[option.Key] | |
| // Skip validation if the field is optional and not present | |
| if option.IsOptional && !exists { | |
| continue | |
| } | |
| // Apply transformations | |
| if exists { | |
| for _, transformer := range option.Transformers { | |
| value = transformer(value) | |
| } | |
| body[option.Key] = value // Update the body with the transformed value | |
| } | |
| // Run all validators for the field (if it exists) | |
| for _, validator := range option.Validators { | |
| if err := validator.Func(value); err != nil { | |
| return fmt.Errorf("%s", validator.Message) | |
| } | |
| } | |
| // Check if the field is required but missing | |
| if !option.IsOptional && !exists { | |
| return fmt.Errorf("'%s' is required", option.Key) | |
| } | |
| } | |
| return nil | |
| } |
1 participant
Changes:
Motivation:
Summary by CodeRabbit
New Features
Tests