fix(zod): don't include fields marked @ignore in schemas
#1881
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (4)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files, primarily enhancing the handling of fields marked with the Changes
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 (1)
packages/schema/src/plugins/zod/generator.ts (1)
466-467: Consider adding a comment explaining discriminator fields.While the implementation is correct, it would be helpful to add a comment explaining what discriminator fields are and why they need to be excluded from mutation schemas.
+// Discriminator fields are used for polymorphic relations and should be excluded from mutation schemas +// as they are managed by Prisma const delegateDiscriminatorFields = allFields.filter((field) => isDiscriminatorField(field));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/schema/src/plugins/zod/generator.ts(2 hunks)tests/integration/tests/plugins/zod.test.ts(1 hunks)
🔇 Additional comments (5)
packages/schema/src/plugins/zod/generator.ts (3)
399-400: LGTM! Clean implementation of ignored fields filtering.
The introduction of allFields effectively filters out fields marked with @ignore at the beginning, ensuring they're excluded from all generated schemas.
401-408: LGTM! Consistent usage of filtered fields for scalar field identification.
The scalar fields filtering logic correctly uses the filtered allFields, maintaining the existing conditions while ensuring ignored fields are excluded.
409-410: LGTM! Consistent usage of filtered fields for relations and foreign keys.
The relations and foreign key fields filtering correctly use the filtered allFields, maintaining consistency with the scalar fields implementation.
tests/integration/tests/plugins/zod.test.ts (2)
1084-1084: LGTM!
The blank line improves readability by separating test cases.
1085-1099: LGTM! Test case effectively validates @ignore functionality.
The test case is well-structured and effectively validates that:
- Fields marked with
@ignoreare excluded from the generated schemas - Both regular and Prisma-specific schemas handle ignored fields correctly
Let's verify that this is the only test case for @ignore functionality:
✅ Verification successful
Let me check the content of these files to understand the context of @ignore usage.
The test coverage for @ignore is complete but spans different use cases
The codebase has appropriate test coverage for both field-level @ignore attribute (in zod.test.ts) and model-level @@ignore attribute (in auth.test.ts). These are different use cases:
- Field-level
@ignoretested in the current PR:password String @ignore - Model-level
@@ignoretested in auth tests:@@ignoreat model level
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other tests related to @ignore functionality
# Expect: Only one test case should handle @ignore fields
rg -l "@ignore" "tests/integration/tests/"
Length of output: 146
Script:
#!/bin/bash
# Check the actual usage of @ignore in both files
rg -B2 -A2 "@ignore" "tests/integration/tests/plugins/zod.test.ts" "tests/integration/tests/enhancements/with-policy/auth.test.ts"
Length of output: 1585
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/schema/src/cli/cli-util.ts (1)
374-382: Consider enhancing robustness and documentationWhile the implementation is functional, consider these improvements:
- Add JSDoc comments to document:
- The function's purpose
- The in-place mutation behavior
- The
$allFieldsproperty's purpose- Add null checks for the fields array
- Consider memory implications of storing duplicate field arrays
Here's a suggested implementation with these improvements:
+/** + * Filters out fields marked with @ignore from data models while preserving + * all original fields in $allFields for reference. + * Note: This function modifies the model in-place. + * @param model The model to process + */ function filterIgnoredFields(model: Model) { model.declarations.forEach((decl) => { if (!isDataModel(decl)) { return; } + // Preserve all fields for reference decl.$allFields = [...(decl.fields ?? [])]; + // Filter out ignored fields decl.fields = (decl.fields ?? []).filter((f) => !hasAttribute(f, '@ignore')); }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/language/src/ast.ts(1 hunks)packages/schema/src/cli/cli-util.ts(3 hunks)tests/integration/tests/schema/todo.zmodel(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/integration/tests/schema/todo.zmodel
🔇 Additional comments (4)
packages/language/src/ast.ts (2)
Line range hint 8-17: LGTM! Well-structured type declarations
The new type declarations are well-documented and follow TypeScript best practices. The ResolvedShape and ResolvedType provide a robust foundation for type resolution in expressions.
61-65: Verify synchronization of $allFields with actual fields
The $allFields property is well-documented and supports the PR objective of handling @ignore fields. However, we should ensure there's a mechanism to keep this array synchronized with the actual fields.
Let's verify how $allFields is populated and maintained:
✅ Verification successful
Based on the search results, I can now generate the final response as we have found clear evidence of how $allFields is synchronized:
$allFields is properly synchronized with actual fields
The synchronization is handled in packages/schema/src/cli/cli-util.ts:
decl.$allFields = [...decl.fields];
decl.fields = decl.fields.filter((f) => !hasAttribute(f, '@ignore'));This shows that:
$allFieldsis populated with a copy of all fields before filteringfieldsis then filtered to exclude@ignorefields- The synchronization happens at the schema processing stage, ensuring consistency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to $allFields to understand its lifecycle
rg -t ts "\$allFields" --no-heading
# Look for field collection/processing logic
ast-grep --pattern 'function $_($$_) {
$$$
$allFields
$$$
}'
Length of output: 105
Script:
#!/bin/bash
# Search for references to $allFields with proper escaping and context
rg -t ts "\\\$allFields" -B 2 -A 2
# Look for field collection/processing logic using a broader pattern
ast-grep --pattern 'interface DataModel {
$$$
}'
# Search for field processing functions
rg -t ts "fields.*=" -B 2 -A 2
Length of output: 94936
packages/schema/src/cli/cli-util.ts (2)
120-122: LGTM! Appropriate placement of the filtering step
The filterIgnoredFields call is correctly placed after model relinking and before returning, ensuring all model processing is complete before filtering out ignored fields.
379-380: Verify type declarations for $allFields
The code adds a new $allFields property to data models. Let's ensure this property is properly typed in the AST definitions.
✅ Verification successful
$allFields property is properly typed in AST definitions
The property is correctly typed as $allFields?: DataModelField[] in the DataModel interface at packages/language/src/ast.ts, with appropriate JSDoc documentation indicating its purpose to store "All fields including those marked with @ignore".
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if $allFields is properly typed in AST definitions
# Look for DataModel interface/type definition
ast-grep --pattern 'interface DataModel {
$$$
}'
# Check for any existing $allFields declarations
rg '\$allFields'
Length of output: 866
No description provided.