fix(policy): only include field selections for evaluating field-level policies if the corresponding fields are selected #1979
Conversation
… policies if the corresponding fields are selected fixes #1978
📝 WalkthroughWalkthroughThis pull request updates the field-level read policy logic in the PolicyUtil class by modifying the getFieldReadCheckSelector method to accept a new fieldSelection parameter and conditionally merge selectors only for specified fields. Additionally, injectReadCheckSelect now forwards the select argument. A new regression test suite in tests/.../issue-1978.test.ts has been added to validate that the secret field on the User model is conditionally accessible, thereby aligning the implementation with the expectations described in Issue #1978. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant I as injectReadCheckSelect
participant G as getFieldReadCheckSelector
C->>I: Initiate read operation with select arguments
I->>G: Call getFieldReadCheckSelector(model, fieldSelection)
G->>G: Iterate over field definitions
G->>G: Check if field is present in fieldSelection
G->>I: Return merged selectors for selected fields
I->>C: Return adjusted read query
Assessment against linked issues
Possibly related PRs
Tip 🌐 Web search-backed reviews and chat
✨ 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 (
|
![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-1978.test.ts (2)
25-30: Consider adding more test cases.While the current test data covers basic scenarios (user with published post and user without posts), consider adding edge cases:
- User with multiple posts (some published, some not)
- User with unpublished post
32-39: Add assertions for field-level policy behavior.The test could be more comprehensive by:
- Verifying behavior when updating the
publishedflag- Testing the behavior when querying multiple users
Here's a suggested addition:
+ // Test behavior when updating post status + await db.post.update({ where: { id: 1 }, data: { published: false } }); + await expect(db.user.findFirst({ where: { id: 1 } })).resolves.not.toHaveProperty('secret'); + + // Test querying multiple users + const users = await db.user.findMany({}); + expect(users).toHaveLength(2); + expect(users[0].secret).toBeUndefined(); + expect(users[1].secret).toBeUndefined();packages/runtime/src/enhancements/node/policy/policy-utils.ts (1)
1319-1327: Optimize field selection merging.The current implementation merges selectors for each field individually. Consider using a more efficient approach for multiple fields:
- for (const [field, def] of Object.entries(fieldLevel)) { - if (!fieldSelection || fieldSelection[field]) { - // field is selected, merge the field-level selector - if (def.entityChecker?.selector) { - result = deepmerge(result, def.entityChecker.selector); - } - if (def.overrideEntityChecker?.selector) { - result = deepmerge(result, def.overrideEntityChecker.selector); - } - } - } + const selectedFields = Object.entries(fieldLevel) + .filter(([field]) => !fieldSelection || fieldSelection[field]); + + if (selectedFields.length > 0) { + const selectors = selectedFields.flatMap(([_, def]) => [ + def.entityChecker?.selector, + def.overrideEntityChecker?.selector + ]).filter(Boolean); + + if (selectors.length > 0) { + result = deepmerge.all([result, ...selectors]); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/runtime/src/enhancements/node/policy/policy-utils.ts(2 hunks)tests/regression/tests/issue-1978.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: OSSAR-Scan
- GitHub Check: build-test (20.x)
- GitHub Check: dependency-review
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
tests/regression/tests/issue-1978.test.ts (1)
5-23: LGTM! Clear and well-structured schema definition.The schema effectively defines the test case with:
- User model with a conditional
secretfield- Post model with a
publishedflag- Clear relationship between User and Post models
packages/runtime/src/enhancements/node/policy/policy-utils.ts (1)
1314-1314: LGTM! Improved method signature with explicit field selection.The addition of the
fieldSelectionparameter makes the method's behavior more explicit and controlled.
This improves performance by not selecting unnecessary fields (especially relations).
fixes #1978