Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a new generic MapSchema[K,V] with validation/parsing/transform support, zconst.TypeMap constant, toZSS conversion for maps, i18n TypeMap entries for az/en/es/ja, and extensive unit tests for map behaviors and validations. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant MapSchema as MapSchema
participant KeySchema as KeySchema
participant ValueSchema as ValueSchema
participant Processors as Processors
participant Issues as ZogIssueList
Caller->>MapSchema: Validate/Parse(map input)
activate MapSchema
MapSchema->>KeySchema: validate/parse each key
KeySchema-->>MapSchema: key result / issue
MapSchema->>ValueSchema: validate/parse each value
ValueSchema-->>MapSchema: value result / issue
loop per-map-constraints
MapSchema->>Processors: run Required/Min/Max/Len/custom tests
Processors-->>MapSchema: test outcomes / issues
end
MapSchema-->>Issues: return aggregated ZogIssueList
deactivate MapSchema
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @maps.go:
- Around line 84-104: The defer calls inside the map iteration cause contexts to
pile up; for each key you create keySubCtx and subCtx then defer their Free(),
which delays freeing until function exit. Replace the deferred frees with
immediate Free() calls after validation: locate keySubCtx created with
ctx.NewValidateSchemaCtx in the loop (used with v.keySchema.validate) and call
keySubCtx.Free() right after v.keySchema.validate returns; likewise locate
subCtx created for v.valueSchema.validate (with subCtx.Path manipulation and
subCtx.Exit toggle) and call subCtx.Free() immediately after
v.valueSchema.validate and before continuing the loop. Ensure no other code
depends on those contexts after the immediate Free() calls.
- Around line 181-196: The loop defers subCtx.Free() causing accumulated
resources and may panic when V is an interface because reflect.TypeOf(zeroValue)
is nil; fix by (1) replacing defer subCtx.Free() with an explicit subCtx.Free()
call at the end of each iteration (after Pop and before using parsedValue) and
(2) avoid using reflect.TypeOf(zeroValue) — instead obtain the concrete value
type via v.valueSchema.getType(), guard against nil (e.g., if t==nil set t =
reflect.TypeOf((*interface{})(nil)).Elem()), then use reflect.New(t).Interface()
to allocate valuePtr so reflect.New never receives nil. Ensure you still call
v.valueSchema.process(subCtx) and check subCtx.Exit before setting the map.
🧹 Nitpick comments (1)
toZSS.go (1)
152-162: Shallow copy may be insufficient for nested values.The
deepCopyMapfunction performs a shallow copy - ifVcontains pointers, slices, or nested maps, those references are shared with the original. For ZSS serialization this is likely acceptable, but the function name "deepCopy" may be misleading.💡 Consider renaming for clarity
-// Helper function to deep copy a map for ZSS -func deepCopyMap[K comparable, V any](m map[K]V) any { +// Helper function to copy a map for ZSS (shallow copy of values) +func copyMapForZSS[K comparable, V any](m map[K]V) any {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
i18n/az/az.goi18n/en/en.goi18n/es/es.goi18n/ja/ja.gomaps.gomaps_test.gomaps_validate_test.gotoZSS.gozconst/consts.go
🧰 Additional context used
🧬 Code graph analysis (8)
maps_test.go (8)
maps.go (1)
EXPERIMENTAL_MAP(37-46)struct.go (2)
Struct(42-46)Shape(35-35)utils.go (1)
Issues(31-31)internals/IssueFormatting.go (1)
Flatten(10-20)tutils/testIssueMessages.go (1)
VerifyDefaultIssueMessages(15-33)zconst/consts.go (1)
TypeMap(29-29)time.go (1)
Time(37-45)pointers.go (1)
Ptr(31-35)
maps_validate_test.go (7)
maps.go (1)
EXPERIMENTAL_MAP(37-46)tutils/testIssueMessages.go (1)
VerifyDefaultIssueMessages(15-33)internals/IssueFormatting.go (1)
Flatten(10-20)zconst/consts.go (1)
ISSUE_KEY_ROOT(5-5)time.go (1)
Time(37-45)pointers.go (1)
Ptr(31-35)slices.go (1)
Slice(44-53)
toZSS.go (3)
maps.go (1)
MapSchema(15-21)pkgs/zss/core/zss_structures.go (1)
ZSSSchema(32-41)zconst/consts.go (1)
TypeMap(29-29)
i18n/ja/ja.go (1)
zconst/consts.go (7)
TypeMap(29-29)IssueCodeRequired(50-50)IssueCodeNotNil(54-54)IssueCodeMin(76-76)IssueCodeMax(80-80)IssueCodeLen(84-84)IssueCodeFallback(64-64)
i18n/en/en.go (1)
zconst/consts.go (7)
TypeMap(29-29)IssueCodeRequired(50-50)IssueCodeNotNil(54-54)IssueCodeMin(76-76)IssueCodeMax(80-80)IssueCodeLen(84-84)IssueCodeFallback(64-64)
i18n/es/es.go (1)
zconst/consts.go (7)
TypeMap(29-29)IssueCodeRequired(50-50)IssueCodeNotNil(54-54)IssueCodeMin(76-76)IssueCodeMax(80-80)IssueCodeLen(84-84)IssueCodeFallback(64-64)
maps.go (3)
zogSchema.go (3)
ComplexZogSchema(21-24)PrimitiveZogSchema(28-31)ZogSchema(11-17)internals/types.go (1)
ZogPrimitive(15-17)zconst/consts.go (2)
ZogType(21-21)TypeMap(29-29)
i18n/az/az.go (1)
zconst/consts.go (7)
TypeMap(29-29)IssueCodeRequired(50-50)IssueCodeNotNil(54-54)IssueCodeMin(76-76)IssueCodeMax(80-80)IssueCodeLen(84-84)IssueCodeFallback(64-64)
🔇 Additional comments (15)
zconst/consts.go (1)
29-29: LGTM!The new
TypeMapconstant follows the existing pattern and is appropriately positioned betweenTypeSliceandTypeStruct.maps.go (6)
12-31: LGTM!The
MapSchemastruct definition and interface compliance are well-structured. The no-opsetCoerceris appropriately documented.
35-46: LGTM!The constructor follows the established pattern for schema creation in the codebase.
48-64: LGTM!The
Validatemethod correctly manages resources with deferredFree()calls and follows the established validation pattern.
115-131: LGTM!The
Parsemethod correctly manages resources and follows the established parsing pattern.
209-239: LGTM!The modifier methods (
Transform,Required,Optional,Default) follow the established builder pattern consistently.
241-334: LGTM!The test methods (
Test,TestFunc,Min,Max,Len) and their helper functions are well-implemented and follow the existing patterns in the codebase.i18n/es/es.go (1)
81-88: LGTM!The Spanish translations for
TypeMapare grammatically correct and follow the existing structure used by other types.i18n/ja/ja.go (1)
82-89: LGTM!The Japanese translations for
TypeMapare appropriately structured and consistent with other type translations.i18n/az/az.go (1)
82-89: LGTM!The Azerbaijani translations for
TypeMapfollow the established pattern and are consistent with other type translations.toZSS.go (1)
136-150: LGTM!The
toZSS()method forMapSchemafollows the established pattern and appropriately represents the map structure with key/value child schemas.i18n/en/en.go (1)
82-89: LGTM!The English translations for
TypeMapare clear and consistent with other type translations.maps_validate_test.go (1)
1-344: Comprehensive test coverage for MapSchema validation.The test file provides thorough coverage of the new
EXPERIMENTAL_MAPvalidation functionality including:
- Required/optional/default behaviors
- Transform functions
- Length constraints (Len/Min/Max)
- Custom test functions
- Multiple key types (string, int64, uint, float64, bool)
- Multiple value types including complex nested types (pointers, slices)
- Value and key validation with proper error path assertions
maps_test.go (2)
12-684: Excellent test coverage for MapSchema parsing and composition.The test file provides comprehensive coverage including:
- Basic parsing and struct integration
- Required/optional/default behaviors
- Error handling with proper path assertions
- Transform functions
- Length constraints
- Multiple key/value type combinations with coercion
- Complex nested value types (slices of structs)
- Integration with Slice, Ptr, and Boxed schemas
The tests verify error paths correctly propagate through nested structures (e.g.,
[1]["c"]for slice of maps).
72-84: No issue here. TheRequired()andOptional()methods on*MapSchema[K, V]use a fluent interface pattern—they mutate the receiver in place and return it. On line 80,schema.Required().Optional()correctly chains these calls:Required()mutatesschema.requiredand returnsschema, thenOptional()is called on that same instance to mutate it again. The original schema variable is properly updated regardless of whether the chain result is reassigned. The test is correct as written.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds experimental map schema support to Zog, enabling validation and parsing of Go maps with typed keys and values. The implementation follows the existing pattern used for slices and structs, providing methods like Required(), Optional(), Default(), Min(), Max(), Len(), Transform(), and Test().
Key Changes
Core Implementation (maps.go - 334 lines)
- Introduces
MapSchema[K, V]with generic key and value types - Provides
EXPERIMENTAL_MAP()constructor for creating map schemas - Implements both
Parse()(for parsing input data) andValidate()(for validating existing maps) - Supports nested schemas: maps can contain structs, slices, or other maps as values
- Integrates with existing Zog features: works inside
Struct(),Slice(),Ptr(), andBoxed()schemas
Comprehensive Test Coverage (maps_test.go - 684 lines, maps_validate_test.go - 344 lines)
- Tests basic map operations, nested structures, and error handling
- Validates multiple key types: string, int, float64, bool, int64, uint
- Validates multiple value types: primitives, structs, slices, pointers, time.Time
- Tests integration with other schema types
ZSS Serialization Support (toZSS.go)
- Adds
toZSS()method for map schemas to support schema serialization - Includes helper for copying default map values
Internationalization (i18n files)
- Adds map-specific error messages in English, Spanish, Azerbaijani, and Japanese
- Messages cover: required, not nil, min/max/len entries, and fallback errors
Critical Issues Found
validate() and process() functions use defer statements inside loops (lines 91, 99, 186), causing contexts to accumulate in memory until function exit. For large maps, this can lead to significant memory consumption.
EXPERIMENTAL_MAP[string, any]) will panic at line 183 because reflect.TypeOf(nil) returns nil, causing reflect.New(nil) to panic.
Architectural Fit
The implementation follows Zog's established patterns:
- Generic type constraints match existing primitive schemas
- Method chaining API consistent with other schema types
- Error handling and context management mirrors slice/struct implementations
- Proper integration with the processor pipeline
Confidence Score: 1/5
- This PR has critical bugs that will cause memory leaks and runtime panics in production
- The defer-in-loop bug causes memory leaks for any map with multiple entries, and the interface type bug will cause panics for common use cases like map[string]any. These are not edge cases - they will affect normal usage patterns. The memory leak will impact all map validations and parsing operations.
- maps.go requires immediate fixes to the defer-in-loop issues (lines 91, 99, 186) and the interface type handling bug (line 183) before this can be safely merged
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| maps.go | 1/5 | New map schema implementation with critical memory leak bugs (defer in loops) and panic bug (interface type handling). Key/value validation errors lack proper path context. |
| maps_test.go | 4/5 | Comprehensive test coverage for map schema including basic operations, nested structures, various key/value types, and integration with other schemas (Slice, Ptr, Boxed). |
| maps_validate_test.go | 4/5 | Thorough validation test coverage for map schema including required/optional, defaults, transforms, size constraints, and various key/value type validations. |
| toZSS.go | 3/5 | Adds map schema serialization to ZSS format. Includes helper function with misleading name (deepCopyMap only does shallow copy). |
Sequence Diagram
sequenceDiagram
participant User
participant MapSchema
participant KeySchema
participant ValueSchema
participant Context
User->>MapSchema: Parse(data, &dest)
MapSchema->>MapSchema: Check if data is zero/nil
alt Data is nil and Required
MapSchema->>Context: AddIssue(required error)
else Data is nil and has Default
MapSchema->>MapSchema: Use default value
else Data is valid map
MapSchema->>MapSchema: Verify data is map type
MapSchema->>MapSchema: Create destination map
loop For each map entry
MapSchema->>KeySchema: Parse(keyData, &parsedKey)
alt Key parsing fails
KeySchema-->>MapSchema: Return errors
MapSchema->>Context: AddIssue(key errors)
else Key parsing succeeds
KeySchema-->>MapSchema: parsedKey
MapSchema->>ValueSchema: process(valueData, valuePtr)
ValueSchema->>ValueSchema: Validate and parse value
alt Value processing succeeds
ValueSchema-->>MapSchema: Parsed value
MapSchema->>MapSchema: Add to destination map
else Value processing fails
ValueSchema->>Context: AddIssue(value error)
end
end
end
MapSchema->>MapSchema: Set destination map
loop For each processor (Min/Max/Len/Transform)
MapSchema->>MapSchema: Execute processor
alt Processor fails
MapSchema->>Context: AddIssue(processor error)
end
end
end
MapSchema-->>User: Return errors (if any)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @toZSS.go:
- Line 152: The comment currently reads "deep copy" but the helper function in
toZSS.go actually performs a shallow copy of the map; update the comment to
accurately describe the behavior (e.g., "shallow copy" or "copies top-level keys
only") immediately above the helper function in toZSS.go so it correctly
reflects the implementation.
🧹 Nitpick comments (1)
toZSS.go (1)
153-162: Consider usingmaps.Clonefor cleaner implementation.Since the
mapspackage is already imported, you could simplify this function usingmaps.Clone(m), which is more idiomatic and concise.♻️ Proposed refactor using maps.Clone
Based on the documentation, maps.Clone performs a shallow clone with keys and values set using ordinary assignment, which matches your current implementation exactly.
// Helper function to shallow copy a map for ZSS func shallowCopyMap[K comparable, V any](m map[K]V) any { - if m == nil { - return nil - } - result := make(map[K]V, len(m)) - for k, v := range m { - result[k] = v - } - return result + return maps.Clone(m) }Note: maps.Clone preserves nil maps by returning nil, so the explicit nil check is unnecessary.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
maps.gotoZSS.go
🚧 Files skipped from review as they are similar to previous changes (1)
- maps.go
🧰 Additional context used
🧬 Code graph analysis (1)
toZSS.go (3)
maps.go (1)
MapSchema(15-21)pkgs/zss/core/zss_structures.go (1)
ZSSSchema(32-41)zconst/consts.go (1)
TypeMap(29-29)
🔇 Additional comments (1)
toZSS.go (1)
136-150: LGTM! MapSchema serialization follows existing patterns.The implementation correctly serializes MapSchema to ZSS format with appropriate handling of processors, required field, default value, and child schemas. The child map structure with "key" and "value" entries is a clean approach for representing map schemas.
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Overview
This PR introduces experimental map schema support to the Zog validation library, allowing users to validate, parse, transform, and apply constraints to maps with typed keys and values.
Key Changes
Core Implementation (maps.go)
- Added
MapSchema[K, V]struct supporting typed key-value pairs where keys must be primitive types (K ∈ ZogPrimitive) - Implemented
Parse()andValidate()methods following existing schema patterns (consistent with SliceSchema) - Supports map size constraints: Min(), Max(), Len()
- Supports transforms and custom tests via Test() and TestFunc()
- Supports defaults and required/optional behavior
- Properly handles context reuse and resource pooling
ZSS Integration (toZSS.go)
- Added
toZSS()method for MapSchema to support Zog Schema Specification serialization - Properly represents child schemas for keys and values in the "Child" map
Localization (i18n files)
- Added complete localization for 4 languages (English, Spanish, Azerbaijani, Japanese)
- All translations include messages for: Required, NotNil, Min, Max, Len, Fallback
Test Coverage (maps_test.go, maps_validate_test.go)
- 36 comprehensive test functions covering:
- Basic parsing with various key/value types
- Struct values and nested maps
- Optional/required behavior and defaults
- Transforms and custom validators
- Size constraints (Min, Max, Len)
- Multiple numeric key types (int, int64, uint, float64)
- Complex value types (bool, time, ptr, slice, nested structs)
- Integration with Slice, Ptr, and Boxed schemas
- Proper error reporting with path information
Design Notes
- Follows existing Zog patterns: consistent with SliceSchema and StructSchema
- Reuses contexts and pools for efficiency
- Collects all errors (not fail-fast) for better user feedback
- Partial parsing on error: entries with validation errors are skipped, but valid entries are included in result
- Type-safe: leverages Go generics for K and V types
Quality Assessment
- Implementation is solid and well-tested
- Code is consistent with existing codebase patterns
- Resource management is proper (context pooling)
- Localization is complete and consistent across all supported languages
Confidence Score: 5/5
- This PR is safe to merge with high confidence. The map schema implementation is well-designed, thoroughly tested, and follows established patterns in the codebase.
- Score of 5 reflects: (1) Comprehensive test coverage with 36+ test functions covering basic usage, edge cases, complex scenarios, and error handling; (2) Code consistency with existing patterns (SliceSchema, StructSchema); (3) Proper resource management and context pooling; (4) Complete localization across 4 languages; (5) Clean implementation following Zog's design principles; (6) No identified logical errors, memory leaks, or resource cleanup issues after thorough analysis.
- No files require special attention. All changes are additive and well-tested.
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| maps.go | 4/5 | MapSchema implementation for parsing and validating map key-value pairs. Implementation is solid with proper context management, error handling, and consistency with existing patterns (Slice/Struct schemas). No critical bugs found. |
| maps_test.go | 5/5 | Comprehensive test coverage for map schema including basic parsing, struct values, optional/required behavior, defaults, transforms, size constraints, multiple key/value types, and complex nested scenarios. Tests are well-structured. |
| maps_validate_test.go | 5/5 | Complete validation test suite covering required/optional maps, defaults, transforms, size constraints, custom tests, multiple validators, and various key/value types. Tests verify error reporting with path information. |
| toZSS.go | 4/5 | Added toZSS() method for MapSchema to support ZSS (Zog Schema Specification). Implementation follows existing patterns, properly handles child schemas for keys and values, and includes default value copying. |
Sequence Diagram
sequenceDiagram
participant User
participant MapSchema
participant KeySchema
participant ValueSchema
participant Processors
User->>MapSchema: Parse(data, &dest)
activate MapSchema
alt nil/zero value
MapSchema->>MapSchema: Apply default or check required
else valid data
MapSchema->>MapSchema: Verify input is map
MapSchema->>MapSchema: Create destination map
loop For each key-value pair
MapSchema->>KeySchema: process(keyData)
activate KeySchema
KeySchema->>KeySchema: Coerce/validate key
deactivate KeySchema
MapSchema->>ValueSchema: process(valueData)
activate ValueSchema
ValueSchema->>ValueSchema: Coerce/validate value
deactivate ValueSchema
alt Both key and value succeeded
MapSchema->>MapSchema: Add to destination map
else Key or value failed
MapSchema->>MapSchema: Skip entry
end
end
MapSchema->>MapSchema: Set destination to new map
MapSchema->>Processors: Run each processor
activate Processors
Processors->>Processors: Transform/validate map
deactivate Processors
end
deactivate MapSchema
MapSchema->>User: Return errors list
1 participant
Summary by CodeRabbit
New Features
Localization
Tests
✏️ Tip: You can customize this high-level summary in your review settings.