Bugfix for compound id create and update #1855
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
packages/plugins/openapi/tests/baseline/rest-3.0.0.baseline.yamlis excluded by!**/*.yamlpackages/plugins/openapi/tests/baseline/rest-3.1.0.baseline.yamlis excluded by!**/*.yaml
📒 Files selected for processing (2)
packages/plugins/openapi/src/rest-generator.ts(2 hunks)packages/server/tests/api/rest.test.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/server/tests/api/rest.test.ts
🔇 Additional comments (1)
packages/plugins/openapi/src/rest-generator.ts (1)
863-863: Approved: Updated field inclusion logic aligns with compound ID handling
The updated condition correctly includes all model fields when dealing with compound IDs (idFields.length > 1) in 'create' and 'read' operations, and excludes ID fields during 'update' operations. This change ensures that the OpenAPI schema accurately represents the requirements for compound ID handling in Prisma.
thomassnielsen
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/server/tests/api/rest.test.ts (1)
2009-2032: LGTM! Consider enhancing test coverage.The test case effectively validates the updating of compound IDs through relationship changes. The setup, execution, and assertions are well-structured and align with the PR objectives.
Consider adding additional assertions to verify:
- The old postLike record (1_user1) no longer exists
- The new postLike record (2_user1) exists with all attributes preserved
- The relationships are properly updated on both posts
expect(r.status).toBe(200); expect(r.body.data.id).toBe(`2${idDivider}user1`); + +// Verify old record is gone +const oldRecord = await prisma.postLike.findUnique({ + where: { postId_userId: { postId: 1, userId: 'user1' } } +}); +expect(oldRecord).toBeNull(); + +// Verify new record exists with preserved attributes +const newRecord = await prisma.postLike.findUnique({ + where: { postId_userId: { postId: 2, userId: 'user1' } } +}); +expect(newRecord).toMatchObject({ + superLike: false, + userId: 'user1', + postId: 2 +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/plugins/openapi/src/rest-generator.ts(2 hunks)packages/server/src/api/rest/index.ts(1 hunks)packages/server/tests/api/rest.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/plugins/openapi/src/rest-generator.ts
🔇 Additional comments (1)
packages/server/src/api/rest/index.ts (1)
962-962: Correct fix for handling compound IDs in single relationships.
The change from set to connect is the correct approach. When dealing with single relationships in Prisma, especially those involving compound IDs, using connect is more appropriate than set because:
connectestablishes a relationship with an existing record using its ID(s)- It aligns with Prisma's requirement of supplying compound ID components as separate attributes
- It maintains referential integrity by ensuring the related record exists
Let's verify the usage of compound IDs in relationship connections:
✅ Verification successful
Let me generate additional verification scripts to check the implementation of compound ID handling:
Based on the verification results, I can now provide the final response:
The change to connect is correct and properly handles compound IDs.
The implementation shows that:
-
makeIdConnectmethod properly handles both single and compound IDs:- For single IDs: Creates a simple connect object with the ID field
- For compound IDs: Creates a connect object with Prisma's compound ID format using
makePrismaIdKey
-
The switch from
settoconnectaligns with Prisma's requirements for establishing relationships, especially for compound IDs where individual components need to be supplied as separate attributes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other instances of relationship handling with compound IDs
# to ensure consistent usage of `connect` over `set`.
# Search for relationship handling patterns
rg -A 5 'connect:|set:'
# Search for compound ID handling
ast-grep --pattern 'makePrismaIdKey|makeCompoundId'
Length of output: 229737
Script:
#!/bin/bash
# Search for compound ID handling implementation and usage
rg -A 5 "makePrismaIdKey|makeCompoundId|makeIdConnect" packages/server/src/api/rest/index.ts
# Search for relationship handling patterns to understand the context
ast-grep --pattern 'makeIdConnect($_, $_)'
Length of output: 3120
When creating a record with a compound id, Prisma expects the parts of the compound id to be passed as attributes. This change reflects that.