fix(delegate): base fields not properly wrapped inside relation layer when injected from policies #1736
Conversation
… when injected from policies Fixes #1734
|
Caution Review failedThe pull request is closed. WalkthroughWalkthroughThe pull request introduces several updates across multiple files, including enhancements to the changelog, a version increment in 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: 3
Outside diff range and nitpick comments (2)
packages/ide/jetbrains/CHANGELOG.md (2)
4-7: LGTM! Consider enhancing the description.The fix addresses the issue mentioned in the PR objectives regarding base fields not being properly wrapped. This is a crucial update that resolves the problem with accessing fields defined in a base model.
Consider rephrasing the entry for clarity:
- - ZModel validation issues when accessing fields defined in a base model from `future().` or `this.`. + - Fixed ZModel validation issues when accessing fields defined in a base model using `future().` or `this.`.
21-29: LGTM! New feature and bug fix.The addition of the
check()policy rule function and the fix for formatting schemas withUnsupportedtype are valuable improvements.Consider rephrasing the second entry for better wording:
- - Fixed the issue with formatting schemas containing `Unsupported` type. + - Resolved the issue with formatting schemas containing `Unsupported` type.Tools
LanguageTool
[style] ~29-~29: Consider using a different verb for a more formal wording.
Context: ...` policy rule function. ### Fixed - Fixed the issue with formatting schemas conta...(FIX_RESOLVE)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (13)
package.jsonis excluded by!**/*.jsonpackages/ide/jetbrains/package.jsonis excluded by!**/*.jsonpackages/language/package.jsonis excluded by!**/*.jsonpackages/misc/redwood/package.jsonis excluded by!**/*.jsonpackages/plugins/openapi/package.jsonis excluded by!**/*.jsonpackages/plugins/swr/package.jsonis excluded by!**/*.jsonpackages/plugins/tanstack-query/package.jsonis excluded by!**/*.jsonpackages/plugins/trpc/package.jsonis excluded by!**/*.jsonpackages/runtime/package.jsonis excluded by!**/*.jsonpackages/schema/package.jsonis excluded by!**/*.jsonpackages/sdk/package.jsonis excluded by!**/*.jsonpackages/server/package.jsonis excluded by!**/*.jsonpackages/testtools/package.jsonis excluded by!**/*.json
Files selected for processing (4)
- packages/ide/jetbrains/CHANGELOG.md (1 hunks)
- packages/ide/jetbrains/build.gradle.kts (1 hunks)
- packages/runtime/src/enhancements/node/delegate.ts (4 hunks)
- tests/regression/tests/issue-1734.test.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/ide/jetbrains/build.gradle.kts
Additional context used
LanguageTool
packages/ide/jetbrains/CHANGELOG.md
[style] ~29-~29: Consider using a different verb for a more formal wording.
Context: ...` policy rule function. ### Fixed - Fixed the issue with formatting schemas conta...(FIX_RESOLVE)
Additional comments not posted (12)
packages/ide/jetbrains/CHANGELOG.md (8)
9-13: LGTM! New feature addition.The new
pathparameter for the@@validateattribute is a valuable addition, allowing for more precise error reporting.
15-19: LGTM! Important update to UUID functionality.The update to the
uuid()function to support new UUID version features from Prisma is a valuable improvement.
31-35: LGTM! Significant feature addition.The support for comparing fields from different models in mutation policy rules is a powerful enhancement that increases the flexibility of the system.
37-42: LGTM! Important updates to improve functionality.Both changes in this version are valuable improvements:
- The support for using ZModel type names as model field names increases flexibility in schema design.
- The update to
auth()resolution to include all reachable schema files enhances the system's ability to handle complex schema structures.
44-48: LGTM! Major version release.The release of ZenStack V2 is a significant milestone. It would be helpful to provide more details about the major changes or improvements in this version.
50-58: LGTM! Relevant fixes and feature additions.Both changes in this version are valuable:
- The support for complex usage of the
@@indexattribute enhances the system's indexing capabilities.- The fix for ZModel validation issues related to model inheritance is particularly relevant to the current PR objectives, as it addresses problems in the inheritance system, which is related to the base fields issue being fixed in the current PR.
Line range hint
60-64: LGTM! Useful developer feature.The addition of auto-completion support inside attributes is a valuable improvement that enhances the developer experience.
Tools
LanguageTool
[style] ~29-~29: Consider using a different verb for a more formal wording.
Context: ...` policy rule function. ### Fixed - Fixed the issue with formatting schemas conta...(FIX_RESOLVE)
Line range hint
1-64: Overall, the changelog is well-maintained and relevant.The changelog effectively documents the changes across multiple versions, including the fix mentioned in the PR objectives. The entries are clear, concise, and provide valuable information about new features, improvements, and bug fixes. The unreleased section directly addresses the issue with base fields not being properly wrapped, which aligns with the PR objectives.
Tools
LanguageTool
[style] ~29-~29: Consider using a different verb for a more formal wording.
Context: ...` policy rule function. ### Fixed - Fixed the issue with formatting schemas conta...(FIX_RESOLVE)
packages/runtime/src/enhancements/node/delegate.ts (4)
236-236: Method Signature Updated CorrectlyThe addition of the
includeConcreteFieldsparameter with a default value ensures backward compatibility with existing method calls.
261-263: Conditional Inclusion of Concrete Fields Implemented ProperlyThe conditional check for
includeConcreteFieldsbefore invokinginjectConcreteIncludeRecursivelyappropriately controls the inclusion of concrete fields based on the parameter's value.
349-350: Properly Generating Include Payload for Sub-Model RelationsThe introduction of
createConcreteRelationIncludePayloadenhances the clarity and modularity of the code by separating the creation of the include payload for sub-model relations.
372-372: Verify Recursive Call Does Not Introduce Side EffectsCalling
buildSelectIncludeHierarchyrecursively withincludeConcreteFieldsset tofalsecould potentially skip necessary concrete fields in certain cases. Verify that all required fields are included in the final hierarchy, and the recursive call behaves as expected without unintended omissions.Run the following script to check for any missing fields in the hierarchy:
Verification successful
✅ Verified: Recursive Call Does Not Introduce Side Effects
No instances of
buildSelectIncludeHierarchyare called withincludeConcreteFieldsset tofalse. All necessary concrete fields are included in the final hierarchy as expected.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all necessary fields are included in the hierarchy. # Test: Search for calls to buildSelectIncludeHierarchy with includeConcreteFields set to false. # Expect: Ensure that this does not cause any required fields to be omitted. rg --type ts --fixed-strings 'await this.buildSelectIncludeHierarchy' -A 3 | grep 'includeConcreteFields: false'Length of output: 114
Script:
#!/bin/bash # Description: Retrieve all calls to buildSelectIncludeHierarchy and inspect the includeConcreteFields argument. # Use ast-grep to find all call expressions of buildSelectIncludeHierarchy and print the third argument. ast-grep --lang typescript --pattern 'await this.buildSelectIncludeHierarchy($_, $_, $_)' --print-pattern | grep 'false'Length of output: 341
Script:
#!/bin/bash # Description: Retrieve all calls to buildSelectIncludeHierarchy and inspect the includeConcreteFields argument. # Use ripgrep to find all calls to buildSelectIncludeHierarchy and extract the includeConcreteFields parameter. rg --type ts 'await\s+this\.buildSelectIncludeHierarchy\s*\(([^)]*)\)' -o --only-matching | grep 'includeConcreteFields\s*:\s*false'Length of output: 134
Script:
#!/bin/bash # Description: Retrieve all calls to buildSelectIncludeHierarchy and inspect the includeConcreteFields argument. # Use ripgrep to find all calls to buildSelectIncludeHierarchy and extract the includeConcreteFields parameter if it exists rg --type ts 'await\s+this\.buildSelectIncludeHierarchy\s*\(([^)]*)\)' -o --only-matching | \ grep -E 'includeConcreteFields\s*:\s*(true|false)' || echo "No buildSelectIncludeHierarchy calls with includeConcreteFields parameter found."Length of output: 315
Fixes #1734