fix(delegate): policies inherited from delegate base models are not injected into proper hierarchy #1776
Conversation
…njected into proper hierarchy Fixes #1770
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily enhance the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
📜 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: 0
🧹 Outside diff range and nitpick comments (4)
tests/regression/tests/issue-1770.test.ts (1)
46-48: Test logic addresses the core issue, but could be more comprehensive.The test correctly attempts to query resources, which is the primary concern in issue #1770. However, consider the following improvements:
- Add data setup to create some resources before querying.
- Verify the content of the query results, not just that it resolves.
- Test different scenarios, such as querying with different user contexts.
Consider expanding the test like this:
const db = enhance(); // Setup test data await db.user.create({ data: { id: 'user1' } }); await db.organization.create({ data: { id: 'org1', users: { create: { userId: 'user1' } }, resources: { create: { name: 'Resource 1' } } } }); // Test as authenticated user const result = await db.resource.findMany(); expect(result).toHaveLength(1); expect(result[0].name).toBe('Resource 1'); // Test as unauthenticated user const unauthenticatedDb = enhance(undefined); await expect(unauthenticatedDb.resource.findMany()).rejects.toThrow();This expansion would provide more comprehensive coverage of the issue.
tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts (1)
487-532: LGTM! Consider adding an explanatory comment.The new test case effectively validates the policy interactions between the base Asset model and the derived Post model. It covers crucial scenarios such as policy rejection and successful creation.
Consider adding a brief comment before the test case to explain its purpose and the specific scenario it's testing. This would enhance the readability and maintainability of the test suite. For example:
// This test case verifies that policies from both the base Asset model and the derived Post model // are correctly applied when performing operations on Post entities through the Asset interface. it('respects sub model policies when queried from a base: case 3', async () => { // ... (existing test code) });packages/schema/src/utils/ast-utils.ts (1)
120-126: Consider adding tests to verify the new inheritance resolution logicThe updated assignment of
clone.$inheritedFromimproves how inherited fields are resolved in the AST cloning process. Adding unit tests or integration tests to cover various inheritance scenarios can help ensure that this change behaves as expected and prevent future regressions.packages/runtime/src/enhancements/node/delegate.ts (1)
369-370: Reminder to Address the TODO Comment on Redundant Policy Rule InjectionThere's a TODO comment indicating that both the delegate base's policy rules and the concrete model's rules are being injected, which may be redundant. Resolving this can improve performance and prevent potential conflicts.
Would you like assistance in refactoring the code to eliminate this redundancy? I can help provide a solution or open a GitHub issue to track this task.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/runtime/src/enhancements/node/delegate.ts (1 hunks)
- packages/schema/src/utils/ast-utils.ts (1 hunks)
- tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts (1 hunks)
- tests/regression/tests/issue-1770.test.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
tests/regression/tests/issue-1770.test.ts (3)
1-1: LGTM: Import statement is correct.The import of
loadSchemafrom '@zenstackhq/testtools' is appropriate for this test file.
3-4: LGTM: Test suite structure is well-defined.The test suite is correctly structured using Jest conventions. It's appropriately labeled with the issue number, and the test case is correctly marked as asynchronous.
5-44: Schema structure aligns with issue #1770.The schema definition accurately represents the polymorphic model structure described in issue #1770. Key points:
- The abstract
BaseAuthmodel with its authorization rules.- The
Resourcemodel extendingBaseAuthand using the@@delegateattribute.- The
Personnelmodel extendingResource.This structure should allow for testing the policy injection in the proper hierarchy.
To ensure the schema aligns with the issue description, let's verify the presence of key models and attributes:
✅ Verification successful
Schema verification successful for issue #1770.
All key models, attributes, and authorization rules are present as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the presence of key models and attributes in the schema # Test 1: Check for BaseAuth, Resource, and Personnel models echo "Checking for key models:" rg -q 'model BaseAuth|model Resource|model Personnel' tests/regression/tests/issue-1770.test.ts && echo "Key models found" || echo "Key models missing" # Test 2: Check for @@delegate attribute echo "Checking for @@delegate attribute:" rg -q '@@delegate' tests/regression/tests/issue-1770.test.ts && echo "@@delegate attribute found" || echo "@@delegate attribute missing" # Test 3: Check for authorization rule echo "Checking for authorization rule:" rg -q '@@allow' tests/regression/tests/issue-1770.test.ts && echo "Authorization rule found" || echo "Authorization rule missing"Length of output: 594
packages/runtime/src/enhancements/node/delegate.ts (1)
374-375: Confirmed Correct Merging of Select/Include Hierarchy into ResultThe code correctly builds the select/include hierarchy with
buildSelectIncludeHierarchyand merges it into theresult. This ensures that the proper hierarchy is maintained during processing.
Fixes #1770