fix: don't set default value in nested writes when set through FK #1989
Conversation
Gabrola
commented
Feb 19, 2025
•
Gabrola
commented
Gabrola
force-pushed
the
fix/nested-write-default
branch
from
49fb100 to
7cf0e98
Compare
📝 WalkthroughWalkthroughThe pull request updates the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant DA as DefaultAuthHandler
participant SD as setDefaultValueForModelData
C->>DA: processCreatePayload(model, data, context)
DA->>SD: setDefaultValueForModelData(fieldInfo, model, data, authDefaultValue, context)
SD-->>DA: Return updated data
DA-->>C: Return final write payload
Possibly related PRs
✨ Finishing Touches
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 (
|
|
Thanks for making this PR @Gabrola ! I'll follow up here shortly. |
|
Hi @Gabrola , I checked the change and the location you changed is spot on. However, I think it's safer if we detect such condition by inspecting the nested creation path (and not just checking back link and foreign key mappings), because a model being created (in a nested context) may have multiple relations, and the parent context may not be the one that's related to the foreign key field with default value. model A {
c C?
}
model B {
c C?
}
model C {
a A
aId String @default(auth().id)
b B
bId String @default(auth().id)
}
// "bId" should still be set
db.a.create({ data: { c: {} } });I'm making some changes directly based on your PR. Do you mind taking a look later? |
- rely on the nested create path to determine the parent model context and detect if it's implicitly set the fk field
|
fixes #1997 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/regression/tests/issue-1997.test.ts (3)
82-107: Enhance test coverage for post creation with likes.The test case could be more comprehensive. Consider:
- Verifying all generated fields (post.id, likes[].id)
- Testing error cases (e.g., duplicate likes from the same user)
- Testing the unique constraint
@@unique([tenantId, userId, postId])Example test for duplicate likes:
await expect( db.post.create({ data: { likes: { createMany: { data: [ { userId: user.id, }, + // This should fail due to unique constraint + { + userId: user.id, + }, ], }, }, }, include: { likes: true, }, }) -).resolves.toMatchObject({ - authorId: user.id, - likes: [ - { - tenantId: tenant.id, - userId: user.id, - }, - ], -}); +).rejects.toThrow();
109-129: Make comment creation test more realistic.The current test creates an empty comment, which might not represent real-world usage. Consider:
- Adding meaningful comment data (e.g., content field)
- Verifying all generated fields
- Testing error cases
Example of a more realistic test:
await expect( db.post.create({ data: { comments: { createMany: { - data: [{}], + data: [{ + content: 'Test comment' + }], }, }, }, include: { comments: true, }, }) ).resolves.toMatchObject({ authorId: user.id, comments: [ { tenantId: tenant.id, + content: 'Test comment', + id: expect.any(String), }, ], });
7-69:❓ Verification inconclusive
Consider enhancing the schema design for better security and consistency.
- The
@@allow('all', true)policy is overly permissive. Consider implementing more granular access control.- The
Commentmodel lacks anauthorrelationship, unlike thePostmodel. This might make it difficult to track comment ownership.Let me help you verify if there are any existing models in the codebase that follow a more secure access control pattern:
🏁 Script executed:
#!/bin/bash # Search for models with more granular access control rg -U "model \w+[\s\S]*?@@allow\('.*?', (?!true)[^)]+\)" --type tsLength of output: 359
Security Model Adjustments Needed in Schema
- The models (User, Post, PostUserLikes, and Comment) use the permissive rule
@@allow('all', true), which could be a security risk. Consider implementing more granular access controls.- The Comment model does not include an author relationship, unlike the Post model. This omission can complicate tracking comment ownership.
- Note: The automated regex search for more granular access control produced errors due to unsupported look-arounds. Please verify manually or re-run with adjusted tools to confirm whether any models deviate from the permissive pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/runtime/src/enhancements/node/default-auth.ts(4 hunks)tests/regression/tests/issue-1997.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/runtime/src/enhancements/node/default-auth.ts
🔇 Additional comments (1)
tests/regression/tests/issue-1997.test.ts (1)
73-80: Well-structured test setup!The test data preparation follows best practices by:
- Creating parent entities (Tenant) before child entities (User)
- Properly setting up the authentication context with both user and tenant IDs
|
@ymc9 this is perfect! I wasn't super confident in my approach, so thank you for making it more robust! |
|
Thanks @Gabrola . Merging it now and will include the fix in the next patch release. |
