fix(delegate): delegate model's guards are not properly including concrete models #1932
Conversation
…crete models fixes #1930
|
Warning Rate limit exceeded@ymc9 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request introduces refactoring changes across several files in the schema and utilities packages. It focuses on centralizing and improving the handling of delegate models, discriminator fields, and policy generation. Key modifications include the addition of utility functions like 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: 0
🧹 Nitpick comments (3)
packages/schema/src/plugins/enhancer/policy/utils.ts (2)
314-316: Delegate guard routing logic.
UsingisDelegateModel(model)to switch to the delegate guard is a clear approach. If there's any plan to support field-level policies for delegates, consider addressing that here or in a separate helper.
452-505: Implementation ofgenerateDelegateQueryGuardFunction.
This new function correctly iterates through the concrete models, applies anANDblock containing (1) a discriminator check and (2) a reference to the concrete model’s guard. One corner case to consider is whenconcreteModels.length === 0—the function defaults to returningTRUE. Verify that this is desired behavior (i.e., for a delegate model with no subtypes). If you expect a strict denial for unknown subtypes or a fallback scenario, consider returningFALSEinstead. Otherwise, implementation details here are sound.packages/schema/src/utils/ast-utils.ts (1)
5-5: New utility functions:getConcreteModelsandgetDiscriminatorField.
Both functions logically support delegate handling. If a model isn’t a delegate,getConcreteModelsreturns an empty array—this is consistent. ForgetDiscriminatorField, consider logging a warning or throwing if multiple attributes are defined, ensuring you don’t silently pick the first one. Otherwise, these utilities appear to meet the delegate-based inheritance requirements.Also applies to: 19-19, 315-338
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/schema/src/plugins/enhancer/enhance/index.ts(3 hunks)packages/schema/src/plugins/enhancer/policy/expression-writer.ts(1 hunks)packages/schema/src/plugins/enhancer/policy/policy-guard-generator.ts(2 hunks)packages/schema/src/plugins/enhancer/policy/utils.ts(5 hunks)packages/schema/src/plugins/prisma/schema-generator.ts(2 hunks)packages/schema/src/utils/ast-utils.ts(2 hunks)tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts(1 hunks)tests/regression/tests/issue-1930.test.ts(1 hunks)
🔇 Additional comments (9)
packages/schema/src/plugins/enhancer/policy/utils.ts (1)
2-2: Imports and references look consistent.
These additions cleanly introduce isDelegateModel, lowerCaseFirst, getConcreteModels, and getDiscriminatorField. Ensure there are no circular dependencies with other modules. Otherwise, everything looks in order.
Also applies to: 18-18, 43-43, 46-52
packages/schema/src/plugins/enhancer/policy/policy-guard-generator.ts (1)
28-28: Conditional checks for delegate vs. non-delegate models.
By placing a top-level isDelegate check, the code gracefully bypasses the current model-level optimizations and defers guard generation to the utils.ts logic for delegate models. This properly addresses the need to include all concrete models in the guard logic. The sequence of constant guard checks remains correct for non-delegate models. Consider adding a small code comment clarifying scenarios when a delegate model has no policies defined (and how it's handled).
Also applies to: 458-495
tests/regression/tests/issue-1930.test.ts (1)
1-91: Comprehensive test for deleted/private article access.
The test thoroughly verifies creation, reading, and denial of access for deleted and private articles. Including coverage for partial reads or updates could further strengthen this regression test. Otherwise, this suite is a solid test that ensures correct guard behavior.
tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts (2)
575-613: Test for optional relation reading with a private sub-model.
The test confirms that the parent record acknowledges the child’s record at a delegate level (base type) while the policy enforces that it’s not visible to standard queries. This provides an essential coverage of “optional” relations for private items.
615-651: Test for required relation reading with a private sub-model.
Similarly, this ensures that a user’s required relation is hidden if the sub-model is private. The test coverage is robust for end-to-end policy validation of mandatory associations.
packages/schema/src/plugins/enhancer/policy/expression-writer.ts (1)
842-854: Ensure consistency when skipping delegation for postUpdate.
The new logic sets the policy to a constant false for postUpdate operations. If there's ever a use case where post-update checks need some sort of delegation, you'll need to revisit this line to allow custom validations. Otherwise, this is consistent and clear.
packages/schema/src/plugins/enhancer/enhance/index.ts (1)
47-47: Centralized utility usage for model discrimination.
The newly added import for getConcreteModels and getDiscriminatorField suggests that you are shifting toward a more centralized approach for retrieving model details. If this aligns with other refactors, it will help avoid duplicating logic across files.
packages/schema/src/plugins/prisma/schema-generator.ts (2)
60-60: Reusing the unified helper for model retrieval.
Importing getConcreteModels helps remove duplicated code that filters concrete models. This centralizes logic and improves maintainability.
324-324: Simplified delegate base model detection.
Replacing manual filtering logic with getConcreteModels(decl) clarifies your intent to retrieve only concrete models inheriting from the delegate. This change is a nice step toward keeping the codebase DRY.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/regression/tests/issue-1930.test.ts (2)
7-50: Add documentation comments to explain the model hierarchy and access control rules.The schema defines a complex hierarchy with delegate models and access control rules. Adding documentation comments would help other developers understand:
- The purpose of each model
- The relationship between models
- The access control rules and their implications
Add documentation comments like this:
+/** + * Represents an organization that owns entities + */ model Organization { id String @id @default(cuid()) entities Entity[] @@allow('all', true) } +/** + * Base model for all entities with common fields and access control + * @property entityType - Discriminator field for delegate models + */ model Entity { // ... rest of the model }
53-78: Improve test organization with describe blocks.The test implementation is thorough but could benefit from better organization using nested describe blocks to group related test cases.
Consider restructuring the test like this:
describe('issue 1930', () => { + describe('normal article', () => { it('can create and read contents', async () => { // ... existing test code for normal article }); + }); + + describe('deleted article', () => { it('cannot read contents', async () => { // ... existing test code for deleted article }); + }); });tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts (2)
579-618: Enhance test readability with better documentation and assertions.While the test effectively validates policy enforcement for optional relations, its purpose and expectations could be more clearly documented.
Consider these improvements:
- it('respects concrete policies when read as base optional relation', async () => { + it('should hide private post fields when accessed through optional base relation', async () => { const { enhance } = await loadSchema(`...`); const fullDb = enhance(undefined, { kinds: ['delegate'] }); await fullDb.user.create({ data: { id: 1 } }); await fullDb.post.create({ data: { title: 'Post1', private: true, user: { connect: { id: 1 } } } }); + // First verify the record exists with full access await expect(fullDb.user.findUnique({ where: { id: 1 }, include: { asset: true } })).resolves.toMatchObject({ asset: expect.objectContaining({ type: 'Post' }), }); const db = enhance(); const read = await db.user.findUnique({ where: { id: 1 }, include: { asset: true } }); - expect(read.asset).toBeTruthy(); - expect(read.asset.title).toBeUndefined(); + expect(read.asset).toBeTruthy(); // Asset should be accessible + expect(read.asset.type).toBe('Post'); // Base fields should be visible + expect(read.asset.title).toBeUndefined(); // Private post fields should be hidden });
620-657: Add error case validation and improve test documentation.The test effectively validates the happy path but could be more comprehensive.
Consider these improvements:
- it('respects concrete policies when read as base required relation', async () => { + it('should handle private posts correctly in required relations while preserving referential integrity', async () => { const { enhance } = await loadSchema(`...`); const fullDb = enhance(undefined, { kinds: ['delegate'] }); await fullDb.post.create({ data: { id: 1, title: 'Post1', private: true, user: { create: { id: 1 } } } }); + // Verify the record exists with full access await expect(fullDb.user.findUnique({ where: { id: 1 }, include: { asset: true } })).resolves.toMatchObject({ asset: expect.objectContaining({ type: 'Post' }), }); const db = enhance(); const read = await db.user.findUnique({ where: { id: 1 }, include: { asset: true } }); - expect(read).toBeTruthy(); - expect(read.asset.title).toBeUndefined(); + expect(read).toBeTruthy(); // User should be accessible + expect(read.asset).toBeTruthy(); // Required relation should be preserved + expect(read.asset.type).toBe('Post'); // Base fields should be visible + expect(read.asset.title).toBeUndefined(); // Private post fields should be hidden + + // Add error case validation + await expect( + db.user.create({ + data: { + id: 2, + asset: { + create: { + type: 'Post', + title: 'Post2', + private: true + } + } + } + }) + ).toResolveTruthy(); // Should succeed as creation is allowed });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/schema/src/plugins/enhancer/policy/policy-guard-generator.ts(1 hunks)packages/schema/src/plugins/enhancer/policy/utils.ts(1 hunks)tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts(2 hunks)tests/regression/tests/issue-1930.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/schema/src/plugins/enhancer/policy/utils.ts
- packages/schema/src/plugins/enhancer/policy/policy-guard-generator.ts
fixes #1930