Add support for custom validation tags #14
Conversation
hi-rai
requested review from
AndrianBdn,
ElenaGrasovskaya,
gornostay25,
ramilamparo and
satvik007
WalkthroughThis pull request updates the converter initialization approach, replacing direct calls to Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Converter
participant OptionHandler
Client->>Converter: NewConverterWithOpts(opts...)
Note right of Converter: Process options (custom types, tags, ignore tags)
Converter->>OptionHandler: Apply configuration
OptionHandler-->>Converter: Options applied
Client->>Converter: Convert struct to Zod schema
Converter->>Client: Return schema with validations
sequenceDiagram
participant Test
participant Converter
participant CustomTagHandler
Test->>Converter: Initialize with custom tag options
Converter->>CustomTagHandler: Retrieve handler for custom tags
CustomTagHandler-->>Converter: Provide validation rules
Converter->>Test: Generate Zod schema
Test->>Test: Assert schema correctness
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 (
|
|
The changes are required for improving SortField validations as mentioned in https://github.com/Hypersequent/tms-issues/issues/1300 |
![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 (9)
zod.go (7)
15-21: Consider validating or documentingprefixconstraints.If conflicting prefixes are passed in different calls, unexpected naming collisions might occur.
377-449: Large block inconvertStructand related validations.This block is quite extensive. Consider splitting it into smaller helper functions for maintainability.
582-619: Array and slice validation expanded.Logic is correct but quite lengthy; consider modularizing repeated validation patterns.
726-805:convertMaphandles map validations thoroughly.Suggestion: unify or standardize the refinement error messages so they follow a similar wording style.
Line range hint
890-966:validateNumbercovers many numeric validations.Consider extracting repeated logic (e.g., parse integer steps) into a helper for cleaner code.
Line range hint
972-1170:validateStringparallelsvalidateNumber.Similar refactoring could reduce duplication and improve readability.
Line range hint
2137-2184: AddedTestCustomTagtest coverage.Excellent demonstration of how custom tag handlers can extend the validation logic. Consider including a negative test case to ensure refinements catch errors properly.
custom/optional/optional_test.go (1)
13-16: LGTM! Consider adding a test case for invalid usage.The changes correctly implement the new options-based configuration system. The test demonstrates proper initialization of the converter with custom type handlers.
Consider adding a negative test case to verify that the converter properly handles invalid optional types or missing handlers.
README.md (1)
297-300: Enhance decimal type handler with validation.The current implementation could be improved by:
- Handling validation tags
- Adding refinements for valid decimal strings
Consider this enhanced implementation:
customTypeHandlers := map[string]zen.CustomFn{ "github.com/shopspring/decimal.Decimal": func (c *zen.Converter, t reflect.Type, v string, indent int) string { - // Shopspring's decimal type serialises to a string. - return "z.string()" + // Validate decimal string format and handle validation tags + base := "z.string().refine((val) => /^-?\\d*\\.?\\d+$/.test(val), 'Invalid decimal format')" + if v != "" { + return base + "." + v + } + return base }, }🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
297-297: Hard tabs
Column: 1(MD010, no-hard-tabs)
298-298: Hard tabs
Column: 1(MD010, no-hard-tabs)
299-299: Hard tabs
Column: 1(MD010, no-hard-tabs)
300-300: Hard tabs
Column: 1(MD010, no-hard-tabs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(3 hunks)custom/decimal/decimal_test.go(1 hunks)custom/optional/optional_test.go(1 hunks)zod.go(17 hunks)zod_test.go(6 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
221-221: Hard tabs
Column: 1
(MD010, no-hard-tabs)
222-222: Hard tabs
Column: 1
(MD010, no-hard-tabs)
226-226: Hard tabs
Column: 1
(MD010, no-hard-tabs)
227-227: Hard tabs
Column: 1
(MD010, no-hard-tabs)
228-228: Hard tabs
Column: 1
(MD010, no-hard-tabs)
229-229: Hard tabs
Column: 1
(MD010, no-hard-tabs)
230-230: Hard tabs
Column: 1
(MD010, no-hard-tabs)
231-231: Hard tabs
Column: 1
(MD010, no-hard-tabs)
235-235: Hard tabs
Column: 1
(MD010, no-hard-tabs)
236-236: Hard tabs
Column: 1
(MD010, no-hard-tabs)
237-237: Hard tabs
Column: 1
(MD010, no-hard-tabs)
238-238: Hard tabs
Column: 1
(MD010, no-hard-tabs)
239-239: Hard tabs
Column: 1
(MD010, no-hard-tabs)
240-240: Hard tabs
Column: 1
(MD010, no-hard-tabs)
241-241: Hard tabs
Column: 1
(MD010, no-hard-tabs)
242-242: Hard tabs
Column: 1
(MD010, no-hard-tabs)
243-243: Hard tabs
Column: 1
(MD010, no-hard-tabs)
244-244: Hard tabs
Column: 1
(MD010, no-hard-tabs)
245-245: Hard tabs
Column: 1
(MD010, no-hard-tabs)
246-246: Hard tabs
Column: 1
(MD010, no-hard-tabs)
247-247: Hard tabs
Column: 1
(MD010, no-hard-tabs)
297-297: Hard tabs
Column: 1
(MD010, no-hard-tabs)
298-298: Hard tabs
Column: 1
(MD010, no-hard-tabs)
299-299: Hard tabs
Column: 1
(MD010, no-hard-tabs)
300-300: Hard tabs
Column: 1
(MD010, no-hard-tabs)
306-306: Hard tabs
Column: 1
(MD010, no-hard-tabs)
314-314: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (19)
zod.go (15)
12-14: IntroduceOpttype for extensible converter configuration.This is a clean solution for passing behavior modifications into the converter.
22-31:WithCustomTypesapproach is straightforward.No issues found. The iteration over the provided map is an effective way to register custom types.
33-42:WithCustomTagsaligns well with the existing extension mechanism.No specific concerns here. The reusability of
CustomFnfor tag-based logic is a nice touch.
44-51: Confirm ignoring tags behaves as expected.Ensure that ignoring a tag doesn’t mask critical validations. Consider clarifying in docs that unknown, unignored tags panic.
52-63: Ensure thread safety inNewConverter.If multiple goroutines call methods on the same converter instance, we might need synchronization around shared maps (
customTypes,customTags, etc.).
70-70: No functional changes in doc comment.
112-113: Enhanced flexibility with variadic opts inStructToZodSchema.This is beneficial for passing multiple configuration options. Looks great.
156-162: Added new fields (prefix,customTypes,customTags,ignoreTags) toConverter.They make the internal state more explicit. No issues detected.
345-345: Refactor check for custom types is concise.Looks good. Just ensure
customTypesis not modified concurrently without a lock, if concurrency is a concern.
521-521: Detecting custom types inconvertField.No concerns. The logic for
isCustomis straightforward.
557-557: Reused pattern for detecting custom types ingetTypeField.No issues; consistent usage.
621-666: Robust validation switch for arrays.Double-check off-by-one logic for “gt/lt” transformations. Also watch out for panic messages being descriptive enough.
671-679: Completes array length checks for fixed-length arrays.Implementation looks consistent.
878-878: Ignore tag check is straightforward.No concerns here.
Line range hint
2136-2136: No functional changes on this blank line.custom/decimal/decimal_test.go (1)
14-17: Refactored to useWithCustomTypesfor converter initialization.This approach is consistent with the new options-based initialization in
zod.go.zod_test.go (2)
6-6: Importedstringspackage.No issues; presumably used for test logic or helper methods.
2137-2184: Comprehensive example inTestCustomTag.Demonstrates usage of multiple custom tag handlers. The
.extend()usage is especially helpful for advanced validations.README.md (1)
5-5: LGTM! Good addition of the reference link.Adding the link to go-validator repository improves documentation by providing direct access to the underlying validation library.
* Introduce converter options and use it in NewConverter * Add new option to specify handlers for custom validation tags * Add support for custom validation tags in all types * Improve ignore tags check for all types
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
🧹 Nitpick comments (5)
notes (5)
1-2: Consider merging top-level conversion logic for maintainability.
BothconvertStructTopLevelandgetTypehave similar responsibilities: handling the reflection logic required to process a type. If these grow in complexity, consider abstracting shared logic into a helper function to reduce duplication.
4-5: Extract common parts of struct conversion.
convertStructandgetTypeStructappear to serve complementary purposes (converting vs. retrieving type info). In the future, if they expand, watch for opportunities to factor out common reflection or error handling code for improved readability.
7-8: Ensure consistent field handling and reduce potential duplication.
convertFieldvs.getTypeFieldmight introduce duplicated logic when applying custom validation or optional checks. Consider consolidating repeated checks (e.g., optional, nullable, or anonymous logic) in a shared helper to avoid divergence.
27-30: Add clarifying comment about purpose ofentry.
Sinceentrytracks anorderanddata, consider adding a short doc comment indicating its role in conversion output ordering (e.g., “Represents a single unit of generated output with a defined insertion order”).
37-38: Enhance documentation forCustomFn.
CustomFnis a powerful extension point. A short comment specifying how each parameter is used (e.g., reflect.Type = the current type, string = the name, etc.) would clarify usage for future developers.
🛑 Comments failed to post (3)
.github/workflows/ci.yml (2)
24-24:
⚠️ Potential issueInvalid Go version specified.
The specified Go version
1.23.5does not exist yet. The latest stable version of Go is in the 1.21.x series.- go-version: '^1.23.5' + go-version: '^1.21.5'Also applies to: 26-26
41-41: 🛠️ Refactor suggestion
Ensure consistent golangci-lint versions.
The golangci-lint version
v1.63in the CI workflow should match exactly with the versionv1.63.4specified in the Makefile to ensure consistent behavior.- version: v1.63 + version: v1.63.4📝 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.version: v1.63.4notes (1)
16-17: 🛠️ Refactor suggestion
Handle advanced string/number validation.
The currentvalidateStringandvalidateNumberplaceholders suggest potential for deeper rule sets (e.g., regex patterns for strings or range checks for numbers). Consider introducing an extensible mechanism for advanced validations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
zod_test.go (2)
2152-2166: Consider enhancing validation patterns.While the implementation is solid, consider these improvements:
- Make the identifier regex pattern more descriptive with a named constant
- Add length validation for the identifier
- Add range validation for pageParams (e.g., maximum allowed page size)
customTagHandlers := map[string]CustomFn{ "identifier": func(c *Converter, t reflect.Type, validate string, i int) string { - return ".refine((val) => !val || /^[a-z0-9_]*$/.test(val), 'Invalid search identifier')" + const identifierPattern = "^[a-z0-9_]{1,64}$" + return fmt.Sprintf(".refine((val) => !val || new RegExp('%s').test(val), 'Invalid search identifier: must be 1-64 characters of lowercase letters, numbers, or underscores')", identifierPattern) }, "pageParams": func(c *Converter, t reflect.Type, validate string, i int) string { - return ".refine((val) => !val.start || !val.end || val.start < val.end, 'Start should be less than end')" + return `.refine((val) => !val.start || !val.end || val.start < val.end, 'Start should be less than end')` + + `.refine((val) => !val.end || val.end <= 1000, 'Maximum page size exceeded')` },
2168-2183: Add test coverage for edge cases.Consider adding test cases for:
- Empty/invalid sort fields
- Negative page numbers
- Maximum page size limits
- Special characters in identifiers
func TestCustomTagEdgeCases(t *testing.T) { type RequestEdgeCases struct { SortParams `validate:"sortFields="` // Empty sort fields PaginationParams struct { Start *int `json:"start,omitempty" validate:"omitempty,gt=-1"` // Negative pages End *int `json:"end,omitempty" validate:"omitempty,gt=0,lte=1000"` // Max page size } `validate:"pageParams"` Search *string `json:"search,omitempty" validate:"identifier"` // Special chars } // Add assertions here }README.md (1)
215-328: Fix markdown formatting issues.The documentation content is clear and well-structured, but there are formatting inconsistencies with tabs vs spaces. Convert all tabs to spaces for consistent formatting.
- "identifier": func(c *zen.Converter, t reflect.Type, validate string, indent int) string { + "identifier": func(c *zen.Converter, t reflect.Type, validate string, indent int) string {🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
221-221: Hard tabs
Column: 1(MD010, no-hard-tabs)
222-222: Hard tabs
Column: 1(MD010, no-hard-tabs)
226-226: Hard tabs
Column: 1(MD010, no-hard-tabs)
227-227: Hard tabs
Column: 1(MD010, no-hard-tabs)
228-228: Hard tabs
Column: 1(MD010, no-hard-tabs)
229-229: Hard tabs
Column: 1(MD010, no-hard-tabs)
230-230: Hard tabs
Column: 1(MD010, no-hard-tabs)
231-231: Hard tabs
Column: 1(MD010, no-hard-tabs)
235-235: Hard tabs
Column: 1(MD010, no-hard-tabs)
236-236: Hard tabs
Column: 1(MD010, no-hard-tabs)
237-237: Hard tabs
Column: 1(MD010, no-hard-tabs)
238-238: Hard tabs
Column: 1(MD010, no-hard-tabs)
239-239: Hard tabs
Column: 1(MD010, no-hard-tabs)
240-240: Hard tabs
Column: 1(MD010, no-hard-tabs)
241-241: Hard tabs
Column: 1(MD010, no-hard-tabs)
242-242: Hard tabs
Column: 1(MD010, no-hard-tabs)
243-243: Hard tabs
Column: 1(MD010, no-hard-tabs)
244-244: Hard tabs
Column: 1(MD010, no-hard-tabs)
245-245: Hard tabs
Column: 1(MD010, no-hard-tabs)
246-246: Hard tabs
Column: 1(MD010, no-hard-tabs)
247-247: Hard tabs
Column: 1(MD010, no-hard-tabs)
297-297: Hard tabs
Column: 1(MD010, no-hard-tabs)
298-298: Hard tabs
Column: 1(MD010, no-hard-tabs)
299-299: Hard tabs
Column: 1(MD010, no-hard-tabs)
300-300: Hard tabs
Column: 1(MD010, no-hard-tabs)
306-306: Hard tabs
Column: 1(MD010, no-hard-tabs)
314-314: Hard tabs
Column: 1(MD010, no-hard-tabs)
zod.go (1)
Line range hint
878-885: Consider enhancing error handling for ignored tags.The current implementation silently ignores unknown tags. Consider adding logging or validation to help catch mistyped tag names.
func (c *Converter) checkIsIgnored(part string) bool { + // Create a map for O(1) lookup + ignoredTags := make(map[string]bool) + for _, tag := range c.ignoreTags { + ignoredTags[tag] = true + } + + isIgnored := ignoredTags[part] + if isIgnored { + // Optional: Log ignored tags to help catch mistakes + log.Printf("Ignoring validation tag: %s", part) + } + return isIgnored }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.workis excluded by!**/*.work
📒 Files selected for processing (11)
.github/workflows/ci.yml(2 hunks).golangci.yml(1 hunks)Makefile(1 hunks)README.md(3 hunks)custom/decimal/decimal_test.go(1 hunks)custom/decimal/go.mod(1 hunks)custom/optional/go.mod(1 hunks)custom/optional/optional_test.go(1 hunks)go.mod(1 hunks)zod.go(17 hunks)zod_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- go.mod
- custom/decimal/go.mod
- Makefile
- .golangci.yml
- custom/optional/optional_test.go
- custom/decimal/decimal_test.go
- .github/workflows/ci.yml
- custom/optional/go.mod
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
221-221: Hard tabs
Column: 1
(MD010, no-hard-tabs)
222-222: Hard tabs
Column: 1
(MD010, no-hard-tabs)
226-226: Hard tabs
Column: 1
(MD010, no-hard-tabs)
227-227: Hard tabs
Column: 1
(MD010, no-hard-tabs)
228-228: Hard tabs
Column: 1
(MD010, no-hard-tabs)
229-229: Hard tabs
Column: 1
(MD010, no-hard-tabs)
230-230: Hard tabs
Column: 1
(MD010, no-hard-tabs)
231-231: Hard tabs
Column: 1
(MD010, no-hard-tabs)
235-235: Hard tabs
Column: 1
(MD010, no-hard-tabs)
236-236: Hard tabs
Column: 1
(MD010, no-hard-tabs)
237-237: Hard tabs
Column: 1
(MD010, no-hard-tabs)
238-238: Hard tabs
Column: 1
(MD010, no-hard-tabs)
239-239: Hard tabs
Column: 1
(MD010, no-hard-tabs)
240-240: Hard tabs
Column: 1
(MD010, no-hard-tabs)
241-241: Hard tabs
Column: 1
(MD010, no-hard-tabs)
242-242: Hard tabs
Column: 1
(MD010, no-hard-tabs)
243-243: Hard tabs
Column: 1
(MD010, no-hard-tabs)
244-244: Hard tabs
Column: 1
(MD010, no-hard-tabs)
245-245: Hard tabs
Column: 1
(MD010, no-hard-tabs)
246-246: Hard tabs
Column: 1
(MD010, no-hard-tabs)
247-247: Hard tabs
Column: 1
(MD010, no-hard-tabs)
297-297: Hard tabs
Column: 1
(MD010, no-hard-tabs)
298-298: Hard tabs
Column: 1
(MD010, no-hard-tabs)
299-299: Hard tabs
Column: 1
(MD010, no-hard-tabs)
300-300: Hard tabs
Column: 1
(MD010, no-hard-tabs)
306-306: Hard tabs
Column: 1
(MD010, no-hard-tabs)
314-314: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (2)
zod_test.go (1)
2137-2184: Well-structured test implementation for custom validation tags!The test provides good coverage of the new custom tag functionality:
- Validates search identifiers with regex
- Ensures proper page parameter validation
- Implements sort field validation using enums
- Handles optional fields correctly
zod.go (1)
12-50: Well-structured option pattern implementation!The option pattern implementation is clean and extensible. Each option function is well-documented and has a single responsibility.
| "sortFields": func(c *zen.Converter, t reflect.Type, validate string, indent int) string { | ||
| sortFields := strings.Split(validate, " ") | ||
| for i := range sortFields { | ||
| sortFields[i] = fmt.Sprintf("'%s'", sortFields[i]) | ||
| } | ||
| return fmt.Sprintf(".extend({field: z.enum([%s])})", strings.Join(sortFields, ", ")) | ||
| }, |
There was a problem hiding this comment.
Seems quite useful for building advanced use cases.
Does go-validator just ignore these custom tags? If thats the case it may be better to use a new tag for it (zod:"identifier" or zen:"identifier"), to make it clear this validation won't run at go level.
There was a problem hiding this comment.
This is intended for two cases
- User wants to overwrite the default behavior of zen for a tag supported out of the box
- User wants support for some go-validator tag not supported by zen out of the box, or when user has added their own custom go-validator tag
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
README.md (1)
239-239:⚠️ Potential issueFix logical error in pageParams validation.
This validation logic was previously identified as incorrect. The current implementation allows
startto be greater thanend.Apply this fix:
- return ".refine((val) => !val.start || !val.end || val.start < val.end, 'Start should be less than end')" + return ".refine((val) => !val.start || !val.end || val.start <= val.end, 'Start should be less than or equal to end')"🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
239-239: Hard tabs
Column: 1(MD010, no-hard-tabs)
🧹 Nitpick comments (40)
notes (9)
4-5: Provide function bodies or documentation.
convertStructandgetTypeStructcurrently lack implementations. If they are intentionally unimplemented, please add explanatory comments describing their expected usage and future implementation plans.
10-11: Add slice/array conversion logic or docstrings.
convertSliceAndArrayandgetTypeSliceAndArrayare placeholders. Insert the necessary reflection-based logic or at least stub out the approach to guide other developers.
16-17: Provide string/number validation logic.
validateStringandvalidateNumberappear to be core validation hooks, but they have no bodies. Ensure you either provide or reference validation logic here.
19-25: Document theConverterstruct.The
Converterstruct fields (e.g.,prefix,outputs,custom) are not explained. Adding doc comments or usage notes will help maintainers understand how these fields factor into code generation.
27-31: Consider clarifyingentrytype usage.This struct likely holds schema data with an ordering index. Brief doc comments could clarify how and where ordering is used.
32-36: Document themetatype.The
NameandSelfReffields suggest cycle detection for nested types, but there’s no explanation on usage patterns. Adding references or doc comments will aid future contributors.
37-38: Improve clarity aroundCustomFn.This function signature’s parameters and usage are non-trivial. Consider explaining each parameter (especially the strings) for maintainability.
40-45: Optional: use sort.Slice instead of byOrder.While byOrder works, using Go’s
sort.Slicemight be simpler and more succinct. If you keep byOrder, consider doc comments explaining your approach to ordering.
50-50: Clarify the purpose ofExport().
Exportoften returns code or strings describing schemas. Since there’s no implementation here, consider adding either a docstring or a TODO for consistency.zod.go (29)
12-13: Use consistent doc comments for exported entities.The
Opttype is exported but only briefly documented. Ensure all exported constructs follow a uniform documentation style for clarity.
33-42: Ensure custom tag functions do not conflict with ignore or built-in tags.
WithCustomTagsmerges user-provided tag handlers. If a user-supplied tag duplicates a built-in tag, your code might panic or produce unexpected results. Consider adding safe-guards.
52-67: Consider deprecating the old converter function or merging updates.
NewConverterWithOptsis a more flexible initialization path. Ensure existing calls toNewConverterare redirected or clearly deprecated to avoid confusion.
125-126: Use consistent naming conventions.
StructToZodSchemanow acceptsopts ...Optbut returns only astring. Consider a naming pattern likeConvertStructWithOptsor similar, for clarity.
413-430: Ensure unknown validations throw a consistent error message.Panics are triggered for unrecognized tags. This is correct but might hamper debugging in large codebases. Provide more context (e.g., field name, struct name).
495-529: Refactor: unify generate-then-merge approach.Your typed field conversion merges optional, nullable metadata with reflection logic. Consider a single function that composes these calls for consistent handling across all field types, avoiding duplication.
566-603: Improve slice/array validation structure.This chunk includes branching for arrays vs slices, plus large switch cases for min/max/len. Consider mapping these validations to a simpler DSL or helper to reduce complexity.
653-681: Type-check map key reflection.Coercing date or number keys is correct but can open corner cases (e.g., negative zeros, invalid date strings). If that’s undesired, log or handle such edge cases.
683-734: Extract repeated refine logic.Many lines add
.refine(...)statements for map sizes. Consolidating them into a helper function (e.g.,addMapSizeRefines(map, min, max)) can reduce duplication.
807-813: Confirm handling of ignore tags.You compare each part to
c.ignoreTags. If we truly want to skip them, ensure partial matches or duplicates are also handled as expected.
818-829: Enforce consistent error messages for unknown numeric validations.“unknown validation” panic is correct. However, mention which field or reflect type, to speed up debugging if a dev is uncertain about the failing code path.
856-866: Ensure user sees the reason behind the.refine((val) => val !== 0).In some code paths, “required” leads to
.refine((val) => val !== 0)on numbers. Possibly add a message “Number cannot be 0 if required” for clarity.
923-981: Expand coverage for advanced string validations.One-of, len, min, max, etc. are well handled. For more advanced validations, consider an extension or plugin mechanism if user code requires more robust or custom patterns.
992-1043: Consider generalizing your approach to UUID validations.You have separate tags for
uuid,uuid3,uuid4, etc. This might be simplified with a single tag parameter or advanced pattern matching, e.g., “uuid(version=3)”.
1048-1054: Fail gracefully if no recognized values appear afteruuid_rfc4122.In case the developer tries
uuid_rfc4122=someValue, it might incorrectly parse. Currently, a panic might happen, but a user-friendly error or fallback might be better.
1080-1088: Caution withjsonrefine approach.You refine by calling
JSON.parse. If the user’s environment or content is invalid JSON, they get “false” with minimal context. Logging or returning an error might help debug.
1117-1118: Recheck logic foromitemptyslices/maps.At line 1117, if it’s optional, it’s no longer nullable. This is correct per the code’s approach, but confirm it meets user expectations for zero-length vs. null vs. omitted.
1163-1166: Include reason in doc foromitzero.
omitzerois recognized similarly toomitempty. Document this, so others know it prevents null export for zero-value slices/maps.
1184-1187: Surface cycle detection details.Panic is thrown for self-referential cycles. Possibly add a message about which struct triggered the error to help debugging.
1198-1205: Capitalize generics carefully.
getTypeNameWithGenericscapitalizes each type param’s first letter. This can cause collisions if the generics were already uppercase. Add a check or doc note that all generic type arg names become Title-cased.
1274-1278: Validate direct maps or slices with empty tags.In lines 1274–1278, we see
mapWithStruct: z.record(...). Confirm that untagged fields with default reflect logic produce correct results. Possibly add a test for untagged nested maps.
1313-1321: Append doc references to combined merges.At line 1320-1321,
.merge(...)is used multiple times. Newcomers might not realize it merges Zod schemas. Add inline doc or references to official Zod docs.
1377-1381: Simplify multiple.refinecalls.Lines 1377–1381 layer multiple refines. A single
.refinewith combined conditions or a helper function might declutter the code and unify messages.
1421-1422: Consider upstream documentation for “Map too large” or “wrong size.”Your user-facing error strings seem helpful. Make sure docs or a readme clarify that these are thrown by
zodrefinements at runtime.
1459-1460: Confirmdiveusage for nested validations.You handle nested map validations with
divetags. If multipledivelevels are used, it can become complex. Ensure your test coverage includes multi-level nested maps or slices.
1481-1482: Confirm multi-levelkeysusage.At lines 1481–1482, you apply
z.record(z.string().min(3), z.string().max(4))for keys and values. If developers nest more keys afterendkeys, ensure your logic merges them properly.
1641-1645: Evaluate potential collisions in advanced interface usage.You map all interface pointers to
z.any(). If user code wants tighter checks (like shape constraints), provide a mechanism or note it in docs.
1778-1802: Check behavior for multiple calls toConvertSlice.You add types
Zip{}, Whim{}intoConvertSlicewithNewConverterWithOpts(). If multiple slices are passed consecutively, ensure no overwriting or missed merges in the final output.
1979-1983: Carefully handle thedecimalcustom type.Mapping decimals to
z.string()might limit numeric usage. If decimals need numeric comparisons, consider returningz.string().regex(...)or a custom refine. Check user expectations.zod_test.go (1)
2136-2184: Expand custom tag test coverage.
TestCustomTagverifies one scenario withsortFields,identifier, andpageParams. Consider adding negative test cases (e.g., invalid sort field or start >= end) to ensure error paths work correctly.README.md (1)
287-287: Clarify the example to avoid confusion.The example uses "identifier" as an ignored tag, but this same tag is used as a custom tag in the previous section. Consider using a different tag name in this example to avoid confusion.
-opt := zen.WithIgnoreTags("identifier") +opt := zen.WithIgnoreTags("custom_format")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(4 hunks)custom/decimal/decimal_test.go(1 hunks)custom/optional/optional_test.go(1 hunks)notes(1 hunks)zod.go(20 hunks)zod_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- custom/optional/optional_test.go
- custom/decimal/decimal_test.go
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
117-117: Hard tabs
Column: 1
(MD010, no-hard-tabs)
221-221: Hard tabs
Column: 1
(MD010, no-hard-tabs)
222-222: Hard tabs
Column: 1
(MD010, no-hard-tabs)
226-226: Hard tabs
Column: 1
(MD010, no-hard-tabs)
227-227: Hard tabs
Column: 1
(MD010, no-hard-tabs)
228-228: Hard tabs
Column: 1
(MD010, no-hard-tabs)
229-229: Hard tabs
Column: 1
(MD010, no-hard-tabs)
230-230: Hard tabs
Column: 1
(MD010, no-hard-tabs)
231-231: Hard tabs
Column: 1
(MD010, no-hard-tabs)
235-235: Hard tabs
Column: 1
(MD010, no-hard-tabs)
236-236: Hard tabs
Column: 1
(MD010, no-hard-tabs)
237-237: Hard tabs
Column: 1
(MD010, no-hard-tabs)
238-238: Hard tabs
Column: 1
(MD010, no-hard-tabs)
239-239: Hard tabs
Column: 1
(MD010, no-hard-tabs)
240-240: Hard tabs
Column: 1
(MD010, no-hard-tabs)
241-241: Hard tabs
Column: 1
(MD010, no-hard-tabs)
242-242: Hard tabs
Column: 1
(MD010, no-hard-tabs)
243-243: Hard tabs
Column: 1
(MD010, no-hard-tabs)
244-244: Hard tabs
Column: 1
(MD010, no-hard-tabs)
245-245: Hard tabs
Column: 1
(MD010, no-hard-tabs)
246-246: Hard tabs
Column: 1
(MD010, no-hard-tabs)
247-247: Hard tabs
Column: 1
(MD010, no-hard-tabs)
297-297: Hard tabs
Column: 1
(MD010, no-hard-tabs)
298-298: Hard tabs
Column: 1
(MD010, no-hard-tabs)
299-299: Hard tabs
Column: 1
(MD010, no-hard-tabs)
300-300: Hard tabs
Column: 1
(MD010, no-hard-tabs)
306-306: Hard tabs
Column: 1
(MD010, no-hard-tabs)
314-314: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (22)
notes (4)
1-2: Implement or document these top-level struct conversion functions.Both
convertStructTopLevelandgetTypeare declared without bodies. If they are placeholders, consider adding implementation logic or stub documentation to clarify their intended behavior.
7-8: Consider adding implementations or docs for field conversion functions.
convertFieldandgetTypeFieldare declared without bodies, making it unclear how fields are processed. Document or implement these functions to avoid confusion.
13-14: Implement or clarify map conversion methods.
convertMapandgetTypeMapare declared but have no implementations. Document or implement your intended approach for map handling.
46-49: Confirm these top-level converter methods are intentionally empty.
AddType,Convert,ConvertSlice,ConvertTypedo not show any body or return statements, which is unusual. Ensure these are either incomplete stubs or meant to be embedded from another partial file.zod.go (16)
15-20: Confirm prefix usage and possible collisions.Using a single
prefixmight cause naming conflicts if multiple prefixes are set by different calls. Consider adding validations/logging or clarifying that only one prefix is valid at a time.
22-31: Check thatcustomTypesmerges are intentional.When merging the custom map in
WithCustomTypes, keys from subsequent calls might overwrite earlier ones. If partial merging is intended, consider warnings or fallback logic.
72-73: Check for backward compatibility.Previously,
NewConvertermight not have allocatedoutputsorcustomTypes. This is a breaking change if user code relied on nil maps. Document or handle potential breakage gracefully.
358-358: Safeguard map lookups inhandleCustomType.
c.customTypes[fullName]is retrieved without any fallback. If thefullNameis subject to reflection anomalies, consider defensive checks or logs.
629-641: Return type clarity.
convertSliceAndArrayreturns"z.<type>().array()"but sometimes also adds refine calls. Confirm the final string is always valid. Complex combos might require additional parentheses or chaining checks.
840-848: Validate presence of a valid number set for.oneof.You parse the values and build a refinement list. Maybe confirm each parsed token is an integer. If a developer uses a string or float by mistake, the error might be cryptic.
1102-1104: Guard pointer logic for string or interface fields.You check pointer emptiness in
isNullable. For complex pointer-of-pointer or interface-of-pointer scenarios, ensure consistent nullability across all passes.
1264-1267: Ensure pointer or interface usage withjson:",omitempty"is tested thoroughly.Potentially, repeated calls might cause unexpected behavior if multiple fields are pointers or interfaces with the same tag. Add tests or doc clarifications.
1280-1283: Double-check optional + .nullable() usage.Setting both “optional()” and “nullable()” is sometimes redundant. Confirm that merging these does not produce unexpected union types in TypeScript.
1296-1299: Test or handle large nested maps.If your reflection approach must handle extremely nested maps, consider performance testing or short-circuit checks to avoid potential stack blowouts.
[performance]
1408-1411: Confirm partial coverage for map boundaries.
.refine((val) => Object.keys(val).length > 1, 'Map too small')is correct forgt=1. Double-check negative or zero boundary cases if user passes invalid data.
1432-1433: Inconsistent approach tolt=1vs.lte=1.One passes
.length < 1, the other.length <= 1. Ensure you handle negative or zero boundary cases consistently across map vs slice validations.
1470-1471: Check array transformation ordering when merging validations.You refine the map, then call
.array(). If a user expects the array constraints first, consider verifying that the final chain order is correct in TypeScript.
1547-1563: Carefully parse leftover validations afterkeys/endkeys.Your logic strips out certain segments from the validate string. Double-check that any leftover validations are properly applied to values or the map itself.
1723-1734: Add a test for partial constraints withMetadataLength.We see multiple refines for map size. Consider a test that verifies partial constraints (e.g., only min=1) to confirm it doesn’t break logic for user-supplied constraints.
2168-2183: Specify defaults or fallback for unhandled custom tags.At lines 2168–2183, you merge a new custom tag for
sortFieldsand others. If unrecognized fields are present, consider error feedback or ignoring them. Currently, unknown tags might panic upstream.README.md (2)
5-5: LGTM! Documentation improvements enhance clarity.The changes improve documentation by:
- Adding a helpful reference link to go-validator
- Consistently updating the converter initialization method across examples
Also applies to: 37-37, 117-117
291-328: LGTM! Well-documented custom types implementation.The section provides clear examples and comprehensive documentation for implementing custom type handlers.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
297-297: Hard tabs
Column: 1(MD010, no-hard-tabs)
298-298: Hard tabs
Column: 1(MD010, no-hard-tabs)
299-299: Hard tabs
Column: 1(MD010, no-hard-tabs)
300-300: Hard tabs
Column: 1(MD010, no-hard-tabs)
306-306: Hard tabs
Column: 1(MD010, no-hard-tabs)
314-314: Hard tabs
Column: 1(MD010, no-hard-tabs)
| var validateStr strings.Builder | ||
| var refines []string | ||
| name := typeName(t) | ||
| parts := strings.Split(validate, ",") | ||
|
|
||
| if name == "" { | ||
| // Handle fields with non-defined types - these are inline. | ||
| return c.convertStruct(t, indent) | ||
| validateStr.WriteString(c.convertStruct(t, indent)) | ||
| } else if name == "Time" { | ||
| var validateStr string | ||
| if validate != "" { | ||
| // We compare with both the zero value from go and the zero value that zod coerces to | ||
| if validate == "required" { | ||
| validateStr = ".refine((val) => val.getTime() !== new Date('0001-01-01T00:00:00Z').getTime() && val.getTime() !== new Date(0).getTime(), 'Invalid date')" | ||
| } | ||
| } | ||
| // timestamps are to be coerced to date by zod. JSON.parse only serializes to string | ||
| return "z.coerce.date()" + validateStr | ||
| validateStr.WriteString("z.coerce.date()") | ||
| } else { | ||
| if c.stack[len(c.stack)-1].name == name { | ||
| c.stack[len(c.stack)-1].selfRef = true | ||
| return fmt.Sprintf("z.lazy(() => %s)", schemaName(c.prefix, name)) | ||
| validateStr.WriteString(fmt.Sprintf("z.lazy(() => %s)", schemaName(c.prefix, name))) | ||
| } else { | ||
| // throws panic if there is a cycle | ||
| detectCycle(name, c.stack) | ||
| c.addSchema(name, c.convertStructTopLevel(t)) | ||
| validateStr.WriteString(schemaName(c.prefix, name)) | ||
| } | ||
| // throws panic if there is a cycle | ||
| detectCycle(name, c.stack) | ||
| c.addSchema(name, c.convertStructTopLevel(t)) | ||
| return schemaName(c.prefix, name) | ||
| } | ||
|
|
||
| for _, part := range parts { | ||
| valName, _, done := c.preprocessValidationTagPart(part, &refines, &validateStr) | ||
| if done { | ||
| continue | ||
| } | ||
|
|
||
| switch valName { | ||
| case "required": | ||
| if name == "Time" { | ||
| // We compare with both the zero value from go and the zero value that zod coerces to | ||
| refines = append(refines, ".refine((val) => val.getTime() !== new Date('0001-01-01T00:00:00Z').getTime() && val.getTime() !== new Date(0).getTime(), 'Invalid date')") | ||
| } | ||
| default: | ||
| panic(fmt.Sprintf("unknown validation: %s", part)) | ||
| } | ||
| } | ||
|
|
||
| for _, refine := range refines { | ||
| validateStr.WriteString(refine) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Break out validation code for readability.
This block handles struct-level validations, cycle detection, lazy schemas, and more. Consider splitting logic into smaller functions (e.g., applyRefineTags, applyRequiredTag).
| // Adds tags which should be ignored. Any unrecognized tag (which is also | ||
| // not ignored) results in panic. | ||
| func WithIgnoreTags(ignores ...string) Opt { | ||
| return func(c *Converter) { | ||
| c.ignoreTags = append(c.ignoreTags, ignores...) | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate ignoreTags usage.
Appending ignore tags at runtime can hide errors. If ignoring unknown tags is optional, consider a mechanism to notify the user that a tag was skipped or if it might cause schema mismatches.
- c.ignoreTags = append(c.ignoreTags, ignores...)
+ // Possibly log or track ignored tags for debugging.
+ for _, ig := range ignores {
+ // log the ignore
+ c.ignoreTags = append(c.ignoreTags, ig)
+ }📝 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.
| // Adds tags which should be ignored. Any unrecognized tag (which is also | |
| // not ignored) results in panic. | |
| func WithIgnoreTags(ignores ...string) Opt { | |
| return func(c *Converter) { | |
| c.ignoreTags = append(c.ignoreTags, ignores...) | |
| } | |
| } | |
| // Adds tags which should be ignored. Any unrecognized tag (which is also | |
| // not ignored) results in panic. | |
| func WithIgnoreTags(ignores ...string) Opt { | |
| return func(c *Converter) { | |
| // Possibly log or track ignored tags for debugging. | |
| for _, ig := range ignores { | |
| // log the ignore | |
| c.ignoreTags = append(c.ignoreTags, ig) | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
zod.go (2)
1048-1051: Enhance error messages for validation parsing.The current error message for invalid validation format could be more helpful.
Consider this improvement:
- panic(fmt.Sprintf("invalid validation: %s", part)) + panic(fmt.Sprintf("invalid validation format for '%s': expected name=value or name", part))
385-430: Consider using custom error types instead of panics.The current implementation uses panics extensively for validation errors. Consider introducing custom error types for better error handling and recovery.
Example implementation:
// Custom error types type ValidationError struct { Tag string Value string Msg string } func (e *ValidationError) Error() string { return fmt.Sprintf("validation error for tag '%s' with value '%s': %s", e.Tag, e.Value, e.Msg) } // Example usage in preprocessValidationTagPart func (c *Converter) preprocessValidationTagPart(...) (..., error) { if idx == 0 || idx == len(part)-1 { return "", "", false, &ValidationError{ Tag: part, Msg: "invalid format, expected name=value or name", } } // ... }Also applies to: 560-631, 677-729, 810-1077
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zod.go(20 hunks)
🔇 Additional comments (3)
zod.go (3)
12-75: Well-structured implementation of the options pattern!The implementation follows Go best practices with:
- Clear documentation for each option
- Type-safe implementation using functional options pattern
- Proper initialization of maps and slices
163-171: Clean separation of concerns in Converter struct!The restructuring improves maintainability by:
- Separating different types of customization (types vs tags)
- Maintaining proper field initialization
- Using idiomatic Go field naming
1042-1077: Well-structured validation preprocessing!The new preprocessValidationTagPart function effectively:
- Reduces code duplication
- Handles custom tags cleanly
- Separates refinements logically
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
zod_test.go (3)
2137-2184: Consider enhancing test coverage with negative test cases.The test case effectively validates the custom tag handlers, but could be strengthened by:
- Adding negative test cases to verify validation failures
- Testing edge cases in pagination validation (e.g., equal values, zero values)
- Testing invalid sort field values
Here's an example of how to add negative test cases:
func TestCustomTag(t *testing.T) { + // Existing positive test case type SortParams struct { Order *string `json:"order,omitempty" validate:"omitempty,oneof=asc desc"` Field *string `json:"field,omitempty"` } // ... rest of the positive test case ... + // Negative test cases + t.Run("invalid_search", func(t *testing.T) { + schema := NewConverterWithOpts(WithCustomTags(customTagHandlers)).Convert(Request{}) + assert.Contains(t, schema, "Invalid search identifier") + }) + + t.Run("invalid_pagination", func(t *testing.T) { + schema := NewConverterWithOpts(WithCustomTags(customTagHandlers)).Convert(Request{}) + assert.Contains(t, schema, "Start should be less than end") + }) + + t.Run("invalid_sort_field", func(t *testing.T) { + schema := NewConverterWithOpts(WithCustomTags(customTagHandlers)).Convert(Request{}) + assert.Contains(t, schema, "title") + }) }
2153-2155: Consider strengthening the identifier validation regex.The current regex pattern
^[a-z0-9_]*$allows empty strings. Consider:
- Requiring at least one character
- Adding restrictions on consecutive underscores
- Preventing underscore at the start/end
Here's a more robust pattern:
- return ".refine((val) => !val || /^[a-z0-9_]*$/.test(val), 'Invalid search identifier')" + return ".refine((val) => !val || /^[a-z][a-z0-9]*(?:_[a-z0-9]+)*$/.test(val), 'Invalid search identifier')"
2156-2158: Enhance pagination validation with additional checks.The current validation only checks if start is less than end. Consider adding:
- Maximum range limit
- Minimum value checks
- Validation for equal values
Here's an enhanced validation:
- return ".refine((val) => !val.start || !val.end || val.start < val.end, 'Start should be less than end')" + return `.refine((val) => { + if (!val.start || !val.end) return true; + if (val.start >= val.end) return false; + if (val.end - val.start > 1000) return false; + return true; + }, 'Invalid pagination range: start must be less than end and range must not exceed 1000')`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zod_test.go(5 hunks)
🔇 Additional comments (1)
zod_test.go (1)
1778-1779: LGTM! Consistent use of options-based initialization.The change from
NewConvertertoNewConverterWithOptsaligns with the PR objectives to support custom validation tags through configurable options.Also applies to: 2088-2089
Summary by CodeRabbit
Documentation
New Features
Chores