fix: allow 'null' values for optional fields in custom types #1864
Conversation
📝 WalkthroughWalkthroughThe pull request introduces changes to the Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 0
🧹 Outside diff range and nitpick comments (3)
tests/regression/tests/issue-1857.test.ts (3)
7-16: Consider restricting access control in test schemaWhile the schema correctly tests the optional JSON field issue, the
@@allow('all', true)directive is overly permissive for a test case.Consider limiting the access to only the required operations:
- @@allow('all', true) + @@allow('create', true)
29-36: Enhance the example code with proper cleanupThe example code should include proper resource cleanup and error handling.
Consider updating the example:
async function main() { const prisma = new PrismaClient(); + try { await prisma.post.create({ data: { content: { type: 'foo', text: null } } }); + } finally { + await prisma.$disconnect(); + } }
43-43: Add more test cases for comprehensive validationWhile the current test case verifies the null value scenario, consider adding more cases to ensure robust validation.
Add test cases for:
// Additional test cases to consider zodSchemas.models.JSONContentSchema.parse({ type: 'foo', text: 'some text' }); // non-null case zodSchemas.models.JSONContentSchema.parse({ type: 'foo' }); // undefined case expect(() => zodSchemas.models.JSONContentSchema.parse({ type: 'foo', text: 123 }) ).toThrow(); // invalid type case
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/schema/src/plugins/enhancer/enhance/model-typedef-generator.ts(1 hunks)tests/regression/tests/issue-1857.test.ts(1 hunks)
🔇 Additional comments (4)
tests/regression/tests/issue-1857.test.ts (2)
1-4: LGTM! Well-structured test setup
The test is properly organized with clear naming that ties it to the issue being fixed.
1-45: Verify test coverage against issue requirements
The test correctly addresses the core issue of allowing null values for optional fields in custom types.
✅ Verification successful
Let me gather more information about the actual issue to ensure the test coverage aligns with the reported problem.
Test coverage correctly addresses the reported issue
The test case perfectly aligns with issue #1857's requirements by:
- Using the exact same
JSONContenttype structure with an optionaltextfield - Testing the specific scenario where
text: nullis provided - Validating through Zod schema parsing, which would catch any type incompatibility issues
- Using the same environment setup (PostgreSQL, JSON field)
The test effectively reproduces and verifies the fix for the reported TypeScript error where null was not properly handled for optional JSON fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the test matches the issue requirements
# Check if the issue number is referenced in other test files to avoid duplication
rg -l "issue.?1857" --type ts
# Verify the schema structure matches the issue description
rg -A 10 "type JSONContent" --type ts
Length of output: 858
Script:
#!/bin/bash
# Check the issue description and related discussions
gh issue view 1857 || true
# Look for any related changes or discussions in commit messages
git log --grep="1857" --pretty=format:"%h %s" || true
# Search for any other files that might be testing JSON content type
rg "JSONContent.*@json" --type ts
Length of output: 1501
packages/schema/src/plugins/enhancer/enhance/model-typedef-generator.ts (2)
20-24: LGTM! Clean implementation for nullable optional fields.
The implementation correctly makes optional fields nullable by appending | null to their types, which:
- Fixes the issue with optional JSON fields
- Maintains consistency with Prisma's behavior
- Follows TypeScript best practices for optional fields
20-24: Verify the impact on existing type definitions.
Since this changes how optional fields are typed, let's verify the impact on existing type definitions.
✅ Verification successful
Based on the code inspection, I can now provide a final response regarding the impact of the optional field typing changes.
The change is consistent with Prisma's behavior and doesn't require further verification.
The code change makes TypeDef field types consistent with Prisma's handling of optional fields, where optional fields are both optional (?) and nullable (| null). This is an intentional alignment with Prisma's type system behavior and doesn't introduce any breaking changes, as:
- The change is contained within the type definition generator
- It matches Prisma's existing behavior for optional fields
- The type definitions are generated files, and any regeneration will maintain consistency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all TypeDef declarations to assess the impact
# Look for type definitions with optional fields to understand the scope of changes
# Search for TypeDef declarations with optional fields
ast-grep --pattern 'type $name {
$$$
$field?: $type;
$$$
}'
# Search for any existing type definitions that might already include '| null'
# to ensure we're not creating duplicates
rg -l '\?: \w+ \| null;'
Length of output: 94
Script:
#!/bin/bash
# Let's try a different approach to find type definitions and their usage
# Find all TypeScript files that might contain type definitions
fd -e ts -x cat {} | grep -B2 -A2 "type.*{" || true
# Look for existing nullable type patterns
rg ":\s*\w+\s*\|\s*null" || true
# Look for optional field patterns
rg ":\s*\w+\?" || true
# Check the specific file for context
cat packages/schema/src/plugins/enhancer/enhance/model-typedef-generator.ts || true
Length of output: 174476
fixes #1857