fix(delegate): self relation support #1821
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications 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: 3
🧹 Outside diff range and nitpick comments (6)
tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts (3)
1411-1441: LGTM! Consider adding negative test cases.The test case effectively validates one-to-one self-referential relationships between different concrete types (Person and Organization). The implementation correctly tests both sides of the relationship.
Consider adding the following test scenarios to improve coverage:
- Verify that the
@uniqueconstraint onsuccessorIdprevents multiple users from having the same successor- Test the behavior when attempting to create circular references
1443-1474: LGTM! Consider expanding test coverage.The test case effectively validates one-to-many self-referential relationships between different concrete types. The implementation correctly tests both sides of the relationship.
Consider adding the following test scenarios to improve coverage:
- Test with multiple children to verify the one-to-many relationship works correctly
- Verify cascade delete behavior when a parent is deleted
- Test updating parent relationship (changing parent)
1476-1506: LGTM! Consider adding more complex scenarios.The test case effectively validates many-to-many self-referential relationships between different concrete types. The implementation correctly tests both sides of the relationship.
Consider adding the following test scenarios to improve coverage:
- Test with multiple followers and following relationships
- Test disconnecting/removing relationships
- Test querying users by their follower/following count
packages/schema/src/plugins/prisma/schema-generator.ts (3)
420-422: Fix inconsistent indentation for better code readability.The indentation around lines 420 to 422 is inconsistent, which may lead to confusion. Proper indentation is essential for readability, especially in nested conditional statements. Please adjust the indentation to accurately reflect the code structure.
466-469: Correct indentation to match code structure.Lines 466 to 469 exhibit inconsistent indentation. Ensuring consistent indentation improves the maintainability and readability of the code. Align the closing braces with their corresponding opening statements.
Line range hint
626-726: Add comments to clarify complex logic in relation handling.The logic between lines 626 and 726 involves intricate steps for managing relations inherited from delegate models, including conditionally generating foreign key fields and handling self-relations. Adding explanatory comments within this block can aid future maintainability and help others understand the reasoning behind these operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/schema/src/plugins/prisma/schema-generator.ts(10 hunks)tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts(1 hunks)
🔇 Additional comments (1)
packages/schema/src/plugins/prisma/schema-generator.ts (1)
506-506:
Implement the code to fix the field name as indicated by the comment.
At line 506, the comment // fix its name suggests that there should be code to modify the addedFkField name, but the implementation is missing. Include the necessary code to adjust the field name accordingly.
Apply this diff to fix the field name:
// fix its name
+addedFkField.name = this.truncate(`${delegateModel.name}_${addedFkField.name}`);Likely invalid or redundant comment.
| // physical: generate relation fields on base models linking to concrete models | ||
| this.generateDelegateRelationForBase(model, decl); | ||
|
|
||
| // generate reverse relation fields on concrete models | ||
| // physical: generate reverse relation fields on concrete models | ||
| this.generateDelegateRelationForConcrete(model, decl); | ||
|
|
||
| // expand relations on other models that reference delegated models to concrete models | ||
| // logical: expand relations on other models that reference delegated models to concrete models | ||
| this.expandPolymorphicRelations(model, decl); | ||
|
|
||
| // name relations inherited from delegate base models for disambiguation | ||
| this.nameRelationsInheritedFromDelegate(model, decl); | ||
| // logical: ensure relations inherited from delegate models | ||
| this.ensureRelationsInheritedFromDelegate(model, decl); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor method calls based on mode to improve efficiency and readability.
The methods generateDelegateRelationForBase, generateDelegateRelationForConcrete, expandPolymorphicRelations, and ensureRelationsInheritedFromDelegate are called within the generateModel method regardless of the current mode. Each method internally checks this.mode before executing any logic.
To enhance performance and code clarity, consider conditionally invoking these methods based on the current mode. This approach avoids unnecessary function calls and makes the code flow more explicit.
| addedFkField.attributes = addedFkField.attributes.filter( | ||
| (attr) => !('name' in attr && attr.name === '@unique') | ||
| ); | ||
| const uniqueAttr = addedFkField.addAttribute('@unique'); | ||
| const constraintName = this.truncate(`${delegateModel.name}_${addedFkField.name}_${concreteModel.name}_unique`); | ||
| uniqueAttr.args.push(new PrismaAttributeArg('map', new AttributeArgValue('String', constraintName))); | ||
|
|
There was a problem hiding this comment.
Conditionally add 'map' argument based on provider support for named constraints.
In lines 498 to 504, you add a @unique attribute with a 'map' argument to specify a constraint name. However, not all database providers support named constraints. To prevent potential errors, check this.supportNamedConstraints before adding the 'map' argument.
Apply this diff to conditionally include the 'map' argument:
const uniqueAttr = addedFkField.addAttribute('@unique');
const constraintName = this.truncate(`${delegateModel.name}_${addedFkField.name}_${concreteModel.name}_unique`);
-if (this.supportNamedConstraints) {
+if (this.supportNamedConstraints) {
uniqueAttr.args.push(new PrismaAttributeArg('map', new PrismaAttributeArgValue('String', constraintName)));
+}Committable suggestion was skipped due to low confidence.
| delegateModel: DataModel, | ||
| concreteModel: DataModel, |
There was a problem hiding this comment.
Incomplete method signature: missing method name and access modifier.
The method definition at lines 484-485 appears incomplete. The method name and access modifier (e.g., private) are missing, which would result in a syntax error. Ensure the method signature includes all necessary components.
Apply this diff to fix the method signature:
- delegateModel: DataModel,
- concreteModel: DataModel,
- origForeignKey: DataModelField
+ private replicateForeignKey(
+ delegateModel: DataModel,
+ concreteModel: DataModel,
+ origForeignKey: DataModelField
+ ) {
+ // method implementation
+ }Committable suggestion was skipped due to low confidence.
fixes #1764