Improve working with entities related to entities that uses compound ids #1801
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
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 (1)
packages/server/tests/api/rest.test.ts (1)
1778-1802: Consider adding more comprehensive assertions.While the test successfully verifies the creation of a
PostLikeInfoentity related to aPostLikewith compound ID, consider adding assertions to verify:
- The created entity's attributes (text field)
- The relationship to the PostLike entity
- The proper handling of the compound ID in the response
Apply this diff to enhance the test:
expect(r.status).toBe(201); + expect(r.body.data).toMatchObject({ + type: 'postLikeInfo', + attributes: { + text: 'LikeInfo1', + postId: 1, + userId: 'user1' + }, + relationships: { + postLike: { + data: { type: 'postLike', id: `1${idDivider}user1` } + } + } + });
📜 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 (3)
- packages/plugins/openapi/src/rest-generator.ts (1 hunks)
- packages/server/src/api/rest/index.ts (5 hunks)
- packages/server/tests/api/rest.test.ts (2 hunks)
🧰 Additional context used
🪛 Biome
packages/server/src/api/rest/index.ts
[error] 1292-1292: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
🔇 Additional comments (4)
packages/plugins/openapi/src/rest-generator.ts (1)
850-852: LGTM! Consider enhancing the documentation.The implementation correctly handles compound IDs by exposing each component as separate fields during read operations. This aligns with the PR objectives.
Consider expanding the comment to better explain the rationale:
-// For compound ids, each component is also exposed as a separate fields for read operations +// For compound ids (multiple @id fields), each component is exposed as a separate field during read operations. +// This is necessary because the client needs access to individual ID components for proper validation and +// relationship handling, while create/update operations handle the compound ID as a single entity.Let's verify the test coverage for this change:
✅ Verification successful
Based on the shell script results, I can now generate the final response since I have sufficient information about the test coverage for compound ID handling.
Implementation is well-tested, documentation suggestion stands
The implementation is thoroughly tested across multiple test files:
packages/plugins/openapi/tests/openapi-restful.test.tsdirectly tests the OpenAPI generation for compound IDs withexposes individual fields from a compound id as attributes- Integration tests in
packages/server/tests/api/rest.test.tsverify the end-to-end behavior with multiple test cases for compound IDs- Additional test coverage exists in regression tests (
issue-1243.test.ts) and integration tests (multi-id-fields.test.ts)The original documentation suggestion remains valuable as it helps explain the implementation rationale, but the code change itself is well-tested and correctly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for compound ID handling in OpenAPI generation # Test: Look for test files related to OpenAPI generation fd -e test.ts -e spec.ts openapi # Test: Search for compound ID test cases rg -A 5 "compound.*id|multiple.*id" --type tsLength of output: 11021
packages/server/tests/api/rest.test.ts (1)
77-87: LGTM! Well-structured schema changes.The new
PostLikeInfomodel and its relation toPostLikeare properly defined with:
- Appropriate field types and naming conventions
- Correct compound key reference using both
postIdanduserIdfieldspackages/server/src/api/rest/index.ts (2)
894-894:⚠️ Potential issueCheck for undefined
parsed.data.data.idinprocessRelationshipCRUDThere is a potential for
parsed.data.data.idto be undefined in theprocessRelationshipCRUDmethod, which could cause runtime errors. Please add a check to ensure it is defined before using it.Apply this diff to handle undefined
parsed.data.data.id:updateArgs.data = { [relationship]: { + connect: parsed.data.data.id + ? this.makeIdConnect(relationInfo.idFields, parsed.data.data.id) + : undefined, }, }; + if (!parsed.data.data.id) { + return this.makeError('invalidRelationData', `Missing id in relationship data for ${relationship}`); + }Likely invalid or redundant comment.
772-783: Ensure attributes are correctly cleaned for compound IDsWhen removing attributes that are part of compound ID relationships, make sure that only the intended attributes are deleted from the
attributesobject to prevent unintended side effects.Run the following script to verify which attributes are being deleted:
✅ Verification successful
Let me gather more context about the compound ID relationships and type mapping to better understand the attribute deletion logic.
Based on the comprehensive context gathered, I can now provide a final response about the attribute deletion verification.
The attribute deletion logic for compound IDs is correctly implemented
The code is working as intended because:
- The attribute deletion only occurs for relationships with compound IDs (checked via
typeInfo.idFields.length > 1)- The deletion is properly scoped to fields that are part of the compound ID relationship, as verified by:
- The
typeInfois correctly mapped from the model metadata- The
idFieldsarray contains only the legitimate compound ID fields for that relationship- The deletion is performed using the exact field names from the relationship's ID fields
The attribute cleanup is necessary because these fields are handled differently in Prisma's connection logic for compound IDs, and leaving them in the attributes object would cause conflicts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: List attributes being deleted for compound ID relationships # Expected: Only attributes corresponding to compound ID fields are deleted grep -E 'delete attributes\[' packages/server/src/api/rest/index.ts -A 5Length of output: 196
Script:
#!/bin/bash # Let's examine the typeMap structure and compound ID handling # First, let's look for typeMap initialization and structure ast-grep --pattern 'typeMap = { $$$ }' # Also search for any references to compound IDs or multiple ID fields rg "idFields" packages/server/src/api/rest/index.ts -B 2 -A 2 # Look for the broader context of the relationship handling rg "relationships" packages/server/src/api/rest/index.ts -B 5 -A 5Length of output: 18181
- use zod schema that contains only non-relation fields to validate mutation payload - make sure id fields are always included in the base zod schema (even if they're also FK fields)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/schema/src/plugins/zod/generator.ts (2 hunks)
- packages/server/src/api/rest/index.ts (5 hunks)
🧰 Additional context used
🪛 Biome
packages/server/src/api/rest/index.ts
[error] 1269-1269: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
🔇 Additional comments (4)
packages/schema/src/plugins/zod/generator.ts (2)
13-13: LGTM: Import addition for ID field handling.The addition of
isIdFieldimport is necessary for the enhanced ID field handling in schema generation.
295-298: LGTM: Enhanced ID field handling in schema generation.The modification to include ID fields explicitly in the scalar fields filter is a good improvement. This ensures that ID fields are always included in the schema generation, which is crucial for proper handling of compound IDs, even when they are part of foreign key relationships.
The condition ordering is correct:
- ID fields are checked first to ensure they're always included
- Regular fields are filtered to exclude data models and foreign keys
packages/server/src/api/rest/index.ts (2)
723-725: LGTM: Good separation of validation concerns.The schema name pattern clearly indicates that only scalar (non-relation) fields are validated, which aligns well with the PR's objective to improve validation for entities with compound IDs.
781-783: Add null checks before using makeIdConnect.While the relationship handling is improved, we should validate the data before using makeIdConnect to prevent runtime errors.
#!/bin/bash # Description: Check if similar validation exists elsewhere in the codebase # Test: Search for null checks on relationship data. Expect: Similar validation patterns rg "data\.data\.id"Also applies to: 790-790
|
Hi @thomassnielsen , I've changed to use the "scalar" schema to validate which doesn't include FK fields. Also had to make a fix in the zod generation part to make sure ID fields are always included in the schema even they're also FK fields. May different corner combinations 😄. Please take a look, thanks! |
|
Nice! I'm not familiar with the zod generator, but this looks reasonable to me 👍 |
Cool. I'm merging it now. |
makeIdConnect)