fix(orm, zod): allow null in inferred type of required Json fields (#2647)#2649
fix(orm, zod): allow null in inferred type of required Json fields (#2647)#2649
Conversation
…2647) A required Json column can hold a JSON `null`, so the inferred model type must include `null`. Widen `TypeMap.Json` in @zenstackhq/orm and `JsonValue` in @zenstackhq/zod to align with Prisma's read semantics (the runtime zod schema already accepted null). Filter and mutation types are unaffected — they reference `JsonValue` from @zenstackhq/orm directly, which stays narrow, so explicit `JsonNull`/`DbNull` markers continue to be required. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR updates JSON value type definitions to permit null at the top level for required ChangesJSON Type Nullability Alignment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
Claude finished @ymc9's task —— View job PR Review: JSON Null Inference FixReviewing the changes to fix null inference in required Json fields... Todo List
Review SummaryThis PR successfully addresses issue #2647 by properly aligning the TypeScript type inference for required Json fields with their actual runtime behavior. The changes are minimal, focused, and well-tested. ✅ Code Quality & Best PracticesExcellent implementation - The changes follow the existing codebase patterns perfectly:
🔍 Type Safety AnalysisStrong type safety maintained:
🧪 Test CoverageComprehensive test coverage with the new regression test: // Tests all three critical aspects:
expectTypeOf<null>().toExtend<Test['metaData']>(); // ORM type inference
expectTypeOf<null>().toExtend<Test['metaData']>(); // Zod type inference
expect(_schema.safeParse({ metaData: null }).success).toBe(true); // Runtime behaviorThe test correctly validates that:
🔐 Security ConsiderationsNo security concerns identified:
⚡ Performance ConsiderationsZero performance impact:
🎯 Backward CompatibilityExcellent backward compatibility:
🔗 Integration PointsWell-integrated with existing systems:
Recommendation: ✅ APPROVEThis is a high-quality fix that:
The implementation demonstrates strong understanding of TypeScript's type system and careful consideration of the broader codebase architecture. |
Summary
Jsoncolumn can hold a JSONnull, but the inferred model type didn't includenull— Prisma'sJsonValuedoes. WidenTypeMap.Jsonin@zenstackhq/ormandJsonValuein@zenstackhq/zodso reads exposenullcorrectly.JsonValuefrom@zenstackhq/ormdirectly (which stays narrow), so explicitJsonNull/DbNullmarkers continue to be required there.@zenstackhq/zodalready acceptednull; this just aligns the inferred type with runtime behavior.Fixes #2647.
Test plan
tests/regression/test/issue-2647covers (a) ORM-inferred type allowsnull, (b) zod-inferred type allowsnull, (c) zod runtime parse acceptsnull.zenstack/zodpackage generates incompatible types for JsonValue #2550 regression tests still pass (NonNullable<>strips the newnullcleanly).@zenstackhq/zodsuite (336 tests), ORM unit tests, and Json e2e tests (json-filter,type-coverage) all pass.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests