- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 107
fix: make sure fields inherited from abstract base models are properly recognized as id #1130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…y recognized as id
WalkthroughWalkthroughThis update involves refining the logic for identifying ID fields in the SDK's utility functions and introducing a regression test for a specific issue. The changes aim to ensure accurate ID field recognition, particularly in scenarios involving inherited fields or unique situations, and to validate the resolution of a regression related to compound primary keys in models derived from abstract 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- packages/sdk/src/utils.ts (2 hunks)
- tests/integration/tests/regression/issue-1129.test.ts (1 hunks)
Additional comments: 4
packages/sdk/src/utils.ts (4)
- 221-223: The comment explains the rationale behind using field names instead of references for matching fields, which is crucial for handling inherited fields. This change aligns with the PR objectives and addresses the core issue effectively.
- 228-228: The logic to include fields specified in the model-level
@@id
attribute by comparing their names is correct and necessary for resolving the issue with compound primary keys.- 240-240: Ensuring that the first field with
@unique
can be used as an ID by comparing names is a sensible approach, especially when no@id
or@@id
attributes are present.- 245-245: Similarly, the logic to include fields specified in the model-level
@@unique
attribute by comparing their names is appropriate and helps in correctly identifying ID fields in complex scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tests/integration/tests/regression/issue-1129.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/integration/tests/regression/issue-1129.test.ts
Fixes #1129
Summary by CodeRabbit