Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces Zod v4 schema generation as the default output format, featuring object shape spreads for embedded structs and specialized builders for string formats, while providing a legacy v3 compatibility mode and an updated golden-file-based testing suite. Feedback from the review highlights critical runtime and compatibility issues: the implementation of self-referential types using get accessors will likely trigger ReferenceError or TypeError during schema construction, and several emitted helpers—such as z.email(), z.uuid(), and z.partialRecord—are not standard Zod APIs, which would cause failures for users of the official library.
String schema generation: - Replace chunk-based approach with semantic validators (stringValidator) that separate parsing from rendering - Remove stringSchemaParts struct — renderStringSchema returns string directly - Add renderV3Chain, renderV4Chain, renderV4FormatBase for version-specific output - Skip redundant .min(1) from required when format validators are present (they already reject empty strings), except base64/hex in v4 which accept empty - Panic on impossible combinations: format+union (email,ip) and multiple formats (email,url) - Add AddTypeWithName for registering anonymous structs with custom names Tests: - Add buildValidatorConverter and assertValidators shared helpers for table-driven golden file tests with dynamic structs - Consolidate TestStringValidations from 30+ subtests into table-driven test (2 golden files) - Consolidate TestNumberValidations, TestMapWithValidations, TestConvertSliceWithValidations into table-driven tests - Consolidate TestFormatValidators: all 25 format + 2 union tags tested bare and with required modifier (8 golden files) - Add test for dive,oneof on slices - Add tests for format+union panic and multiple formats panic - Remove 24 redundant individual format tests from TestStringValidations - Remove 4 redundant tests from TestZodV4Defaults - Golden files reduced from 171 to 75 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Security:
- Add escapeJSString() to escape backslashes and double quotes in
generated JS string literals (contains, startswith, endswith, eq, ne,
oneof). Prevents broken output from struct tag values with special chars.
Refactors:
- Replace lastFieldSelfRef side-channel flag with convertResult struct
that explicitly returns {text, selfRef} from convertType(). Public
ConvertType API unchanged via thin wrapper.
- Extract renderChain() with shared cases from renderV3Chain/renderV4Chain,
eliminating ~80 lines of duplication.
- Simplify parseStringValidators: replace ~90 lines of switch cases with
knownStringTags map lookup.
- Extract regex rendering into maps (regexChainMap, unicodeRegexChainMap,
v3FormatRegexMap) with renderRegex()/renderUnicodeRegex() helpers.
Bug fixes:
- Fix v4 embedded field ordering: spreads now come before named fields
so named fields override embedded ones (last key wins in JS), matching
Go's shadowing semantics. Previously spreads came after, causing
embedded fields to override named fields incorrectly.
- Panic on non-integer gt=/lt= arguments instead of silently using 0.
Tests:
- Add field shadowing test (named field overrides embedded field, v3+v4)
- Add escapeJSString unit tests
- Add gt/lt non-integer panic tests
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use version-aware dispatch in enum branch (renderV3Chain for v3,
renderChain for v4) instead of always calling renderV3Chain.
Defensive change — no current behavioral difference.
- Replace magic number rawPart[6:] with rawPart[len("oneof="):]
- Use strings.ReplaceAll instead of strings.Replace with count -1
- Panic in renderV4FormatBase default case instead of returning ""
to catch future omissions when adding format tags
- Validate numeric arguments for len/min/max/gte/lte string validators
using requireIntArg helper (same treatment as gt/lt)
- Remove renderV4Chain wrapper — inline renderChain at all call sites
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove unreachable len(vals) == 0 panic in oneof handler — the case guard already requires valValue != "" and FindAllString always matches - Separate escapeJSString and requireIntArg doc comments with blank line Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use json.Marshal in escapeJSString for complete JS string escaping
(newlines, control characters, not just quotes and backslashes)
- Pass reflect.TypeOf("") instead of reflect.TypeOf(0) to custom tag
handlers in string validation context
- Replace string concatenation with strings.Builder in renderStringSchema
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Review Summary
Well-structured migration after many review rounds. The major issues from prior reviews (JS string escaping via json.Marshal, convertResult struct, shared renderChain, requireIntArg/requireNumericArg validation, renderV4FormatBase panic, getValidateValues guards, WithIgnoreTags tests, boolean enum fix) have all been addressed.
Architecture highlights:
- Clean v3/v4 split with
WithZodV3()opt-in parseStringValidators/renderStringSchemaseparation is a solid improvement over the original single-pass approach- Golden file + docker type-check + vitest runtime tests provide strong end-to-end confidence
assertSchemahelper elegantly handles version-specific vs shared golden files
Remaining items (all low):
- Potential panic in
getTypeNameWithGenericson empty generic type arg - Empty if-branch in
getValidateCurrentcould be more explicit
No high or critical issues found. The security posture is good — string interpolation paths are properly escaped, numeric arguments are validated, and invalid tag combinations panic with clear messages.
| typeArgs := strings.Split(name[typeArgsIdx+1:len(name)-1], ",") | ||
| for _, arg := range typeArgs { | ||
| sb.WriteString(strings.ToTitle(arg[:1])) // Capitalize first letter | ||
| sb.WriteString(strings.ToUpper(arg[:1])) // Capitalize first letter |
There was a problem hiding this comment.
Low: Potential panic on empty generic type argument
arg[:1] will panic with index out of range if arg is an empty string. This can happen if a generic type name has malformed syntax like Type[,T], where splitting on comma produces an empty element.
| sb.WriteString(strings.ToUpper(arg[:1])) // Capitalize first letter | |
| for _, arg := range typeArgs { | |
| if len(arg) == 0 { | |
| continue | |
| } | |
| sb.WriteString(strings.ToUpper(arg[:1])) // Capitalize first letter | |
| sb.WriteString(arg[1:]) | |
| } |
| return schema as { safeParse: (input: unknown) => any }; | ||
| } | ||
|
|
||
| describe(`Golden file runtime tests (zod@${currentZodVersion})`, () => { |
There was a problem hiding this comment.
Praise: Excellent test infrastructure
The golden file + docker type-check + vitest runtime test combination is a really solid approach. Dynamic schema import with version-aware skipping ensures both zod v3 and v4 outputs are validated end-to-end. The cases.ts table-driven tests complement the golden file snapshots well.
Summary
Migrate zen to support Zod v4 as the default output, with v3 available via
WithZodV3(). Includes a refactor of string schema generation and a new test infrastructure.Zod v4 output changes
Format validators use
.check()patternz.string().check(z.email())instead of deprecatedz.string().email(). Handles transform ordering correctly (e.g.,z.string().trim().check(z.email())trims before validating). See colinhacks/zod#4642IP union
ip/ip_addrtags producez.union([z.ipv4(), z.ipv6()])with constraints distributed to each arm.Self-referential getter pattern
get field() { return Schema; }instead ofz.lazy(() => Schema).Embedded struct spreads
...Schema.shapeinstead of.merge(Schema). Spreads are placed before named fields so that Go's field shadowing semantics are preserved (last key wins in JS).Partial records
z.partialRecord()for enum-keyed maps.Redundant
.min(1)removedFormat validators already reject empty strings, so
requiredtag's.min(1)is omitted. Exception:base64/hexadecimalin v4 which accept empty strings.Invalid combinations panic
email,ip(format + union) andemail,url(multiple formats) panic instead of producing broken outputoneof/boolean) combined with non-enum validators (e.g.oneof=a b,contains=x) panics —oneoffully constrains the valuedive,keyswithout matchingendkeyspanics with a clear messagegt=abc) panic viarequireIntArg/requireNumericArgString schema generation refactor
The original
validateStringbuilt Zod output directly using astrings.Builder— mixing parsing and rendering in one pass with no intermediate representation. This made v3/v4 branching difficult and led to edge cases with tag ordering.parseStringValidators— pure parser producing[]stringValidatorviaknownStringTagsmap lookuprenderStringSchema— version-aware renderer that classifies validators, validates combinations, and renders the complete schema in one passrenderChain— shared rendering for version-independent validatorsrenderV3Chain— v3-specific format chains (.email(),.ip(),.regex())renderV4FormatBase— v4 format builders (z.email(),z.uuid(), etc.)renderRegex()helpersBug fixes (pre-existing)
preprocessValidationTagPartnow passes actualreflect.Typeto custom tag handlers instead of alwaysreflect.TypeOf("")strings.ToTitle(deprecated) replaced withstrings.ToUppergetValidateValuesguards against missingendkeysand barediveSecurity
escapeJSString()escapes\and"in generated JS string literalslen/min/max/gt/gte/lt/lteNew public API
WithZodV3() Opt— opt into Zod v3 outputAddTypeWithName(input, name)— register anonymous structs with custom namesTest infrastructure
github.com/xorcare/golden—make test-updateto regeneratemake docker-test) — typechecks all golden files with tsc against zod@3 and zod@4, then runs vitest runtime tests against both versionsassertValidators+buildValidatorConverterusing dynamic structsTest plan
🤖 Generated with Claude Code