fix(polymorphism): relation name disambiguation #1107
Conversation
WalkthroughWalkthroughThe changes focus on enhancing the schema validation and relation naming within the ZenStack framework to address issues related to relation fields in models, especially when dealing with base types and delegate models. Key updates include improved error messaging for Prisma generation failures, introduction of functions for attribute argument retrieval and relation naming disambiguation, and updated logic for handling relations inherited from delegate base models. Changes
Assessment against linked issues
Related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- packages/schema/src/language-server/validator/datamodel-validator.ts (1 hunks)
- packages/schema/src/plugins/enhancer/enhance/index.ts (1 hunks)
- packages/schema/src/plugins/prisma/schema-generator.ts (4 hunks)
- packages/schema/src/utils/ast-utils.ts (2 hunks)
- tests/integration/tests/enhancements/with-delegate/issue-1100.test.ts (1 hunks)
Additional comments: 13
tests/integration/tests/enhancements/with-delegate/issue-1100.test.ts (2)
- 3-34: The test case
missing opposite relationcorrectly simulates the scenario described in issue The generated prisma failed to compile if both the relation field of entity and it's base type exsit in one model #1100 by defining a Prisma schema with modelsUser,Content,Post, andImage. It then usesloadModelWithErrorto assert that the expected error message is returned. This is a good practice for testing specific error conditions and ensures that the schema validation logic is working as intended.- 37-68: The test case
successvalidates that a corrected schema, which includes an opposite relation field (authorinPostmodel), successfully loads without errors usingloadSchema. This test effectively confirms that the issue addressed by the pull request has been resolved. It's a good practice to include both negative and positive test cases to ensure comprehensive coverage.packages/schema/src/utils/ast-utils.ts (3)
- 19-19: The import statement correctly includes
isDelegateModelalongsideisFromStdlib, which is necessary for the new functionality introduced to handle delegate models. Ensuring that all required functions are imported is crucial for the correct operation of the utility functions.- 210-214: The modification to
getModelFieldsWithBasesto include anincludeDelegateparameter with a default value oftrueis a thoughtful addition. This allows callers to specify whether delegate model fields should be included in the results, providing flexibility in handling different scenarios. The implementation ensures backward compatibility by defaulting to including delegate model fields.- 218-225: The addition of the
includeDelegateparameter togetRecursiveBasesand the conditional check usingisDelegateModelto optionally skip delegate models based on theincludeDelegateflag is logically sound. This change enhances the function's versatility in handling model inheritance, especially in complex scenarios involving delegate models. It's a good practice to make such functions more configurable to accommodate various use cases.packages/schema/src/plugins/enhancer/enhance/index.ts (3)
- 136-136: The updated error message in the
generateLogicalPrismafunction now includes the specific file causing the failure, which is a significant improvement for debugging purposes. Providing detailed error messages can greatly assist developers in identifying and resolving issues more efficiently.- 133-139: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [50-50]
The use of
path.joinwith potentially user-controlled input (e.g.,outDir,logicalPrismaFile,prismaClientOutDir) could lead to path traversal vulnerabilities. However, in this context, the inputs to these functions are derived from internal configurations or generated paths, not directly from user input. It's important to ensure that any externally influenced paths are properly sanitized or validated to prevent exploitation. For future-proofing, consider adding checks or sanitization steps if these inputs could be influenced by user-controlled data.Also applies to: 58-58, 66-66, 114-114, 140-140, 152-152, 167-167
- 133-139: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [333-335]
The dynamic construction of regular expressions using
delegateModelsanddelegateInfocould potentially lead to Regular Expression Denial-of-Service (ReDoS) vulnerabilities if the input is controlled by an attacker. However, in this context, the input to these regular expressions comes from the model definitions, which are not user-controlled at runtime. It's important to be cautious with dynamic regular expressions, especially when dealing with untrusted input. Consider adding input validation or using static patterns when possible to mitigate ReDoS risks.Also applies to: 356-358
packages/schema/src/language-server/validator/datamodel-validator.ts (1)
- 244-244: The addition of the
falseargument to thegetModelFieldsWithBasesfunction call within thevalidateRelationFieldmethod is a targeted change to refine the filtering logic for opposite fields. This adjustment ensures that delegate model fields are excluded from consideration when identifying opposite relation fields, aligning with the broader goal of addressing relation field disambiguation issues. It's a precise and effective way to address the specific problem outlined in issue The generated prisma failed to compile if both the relation field of entity and it's base type exsit in one model #1100.packages/schema/src/plugins/prisma/schema-generator.ts (4)
- 37-37: The addition of the
getAttributeArgfunction is a good enhancement. It likely serves to retrieve specific arguments from attributes, which can be particularly useful in the context of this PR for handling relation naming disambiguation. However, ensure that this function is used consistently across the file and that its error handling is robust, especially in cases where the attribute or argument might not exist.- 304-305: The introduction of the
nameRelationsInheritedFromDelegatemethod is a key part of the solution to the relation naming disambiguation issue. This method's placement within thegenerateModelfunction suggests it's applied to each model during schema generation, which aligns with the PR objectives. It's crucial that this method accurately identifies relations inherited from delegate base models and names them appropriately to avoid conflicts.- 429-430: Using field names as relation names for disambiguation is a clever approach to solve the naming conflict issue. However, it's important to ensure that these generated names remain unique across the schema. Consider edge cases where multiple relations might result in the same generated name, leading to a new type of conflict.
Verification successful
The verification process did not find any duplicate relation names within the
.prismafiles in the repository. This suggests that the approach of using field names as relation names for disambiguation, as implemented, does not currently lead to conflicts. Therefore, the concern raised in the review comment does not manifest as an issue based on the available evidence.* 461-501: The logic within `nameRelationsInheritedFromDelegate` for naming relations inherited from delegate base models appears sound. It's good to see that it checks for the existence of a relation attribute before attempting to modify it, which should prevent runtime errors. However, ensure that the generated relation names do not clash with existing field names or relation names within the same model or across different models. This could potentially introduce a new set of disambiguation issues.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that generated relation names are unique across the schema # This script is a conceptual example and might need adjustments to fit the actual schema structure rg --type prisma '@relation' | awk '{print $3}' | sort | uniq -dLength of output: 95
Script:
#!/bin/bash # Adjusted script to verify that generated relation names are unique across the schema # This script explicitly targets .prisma files and attempts to accurately extract relation names rg '@relation' '*.prisma' | awk -F'[()]' '{print $2}' | tr ',' '\n' | awk -F'=' '{print $2}' | tr -d ' ' | sort | uniq -dLength of output: 166
Script:
#!/bin/bash # Script to confirm the presence of .prisma files in the repository fd '.prisma$'Length of output: 208
Script:
#!/bin/bash # Script to verify that generated relation names are unique across the schema # This script uses fd to find .prisma files and then searches within these files for @relation directives fd '.prisma$' | xargs rg '@relation' | awk -F'[()]' '{print $2}' | tr ',' '\n' | awk -F'=' '{print $2}' | tr -d ' ' | sort | uniq -dLength of output: 129
Fixes #1100
Summary by CodeRabbit