fix(orm): use raw alias for join keys when PK/FK has field access policy#2675
fix(orm): use raw alias for join keys when PK/FK has field access policy#2675lsmith77 wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds detection and selection of raw column aliases for PK/FK fields with conditional read policies, updates join-pair construction and lateral/SQLite join filters to use those raw aliases, and includes regression tests validating three ChangesFix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
022e5dd to
c109d4a
Compare
c109d4a to
531e8d1
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/regression/test/issue-2674.test.ts (1)
148-148: ⚡ Quick winAssert FK redaction across all included rows.
Checking only
comments[0]can miss partial leaks. Assert every included comment haspostId === nullin this scenario.♻️ Proposed test tightening
- expect(userResult?.comments[0]?.postId).toBeNull(); + expect(userResult?.comments.every((comment) => comment.postId === null)).toBe(true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/regression/test/issue-2674.test.ts` at line 148, The test currently only asserts the first included comment's postId is null via expect(userResult?.comments[0]?.postId).toBeNull(); and can miss partial leaks—update the assertion to verify every included comment has postId === null by iterating over userResult?.comments (or using an array matcher) and asserting each comment.postId is null; reference the userResult variable and comments property in the test (replace the single-index check with a forEach/map-based assertion that fails if any comment has a non-null postId).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/regression/test/issue-2674.test.ts`:
- Line 148: The test currently only asserts the first included comment's postId
is null via expect(userResult?.comments[0]?.postId).toBeNull(); and can miss
partial leaks—update the assertion to verify every included comment has postId
=== null by iterating over userResult?.comments (or using an array matcher) and
asserting each comment.postId is null; reference the userResult variable and
comments property in the test (replace the single-index check with a
forEach/map-based assertion that fails if any comment has a non-null postId).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ecac85c-bd93-4695-aaac-a2c298cfbec0
📒 Files selected for processing (5)
packages/orm/src/client/crud/dialects/lateral-join-dialect-base.tspackages/orm/src/client/crud/dialects/sqlite.tspackages/orm/src/client/query-utils.tspackages/plugins/policy/src/policy-handler.tstests/regression/test/issue-2674.test.ts
3f814a8 to
b682b06
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/regression/test/issue-2674.test.ts`:
- Around line 59-103: Add a positive-control assertion that the ADMIN principal
can resolve the relation before the USER checks: after creating post and comment
with db.$setAuth(admin) and before the USER query, call
db.$setAuth(admin).comment.findUnique({ where: { id: comment.id }, include: {
post: true } }) (or reuse an adminResult variable) and assert adminResult?.post
is not null (and optionally adminResult?.postId equals post.id); then proceed
with the existing userResult assertions so failures that affect all roles are
caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 817b55b5-eea1-47c4-af67-6414aacd2233
📒 Files selected for processing (5)
packages/orm/src/client/crud/dialects/lateral-join-dialect-base.tspackages/orm/src/client/crud/dialects/sqlite.tspackages/orm/src/client/query-utils.tspackages/plugins/policy/src/policy-handler.tstests/regression/test/issue-2674.test.ts
b682b06 to
2345079
Compare
fix #2674
When a PK or FK field carries a
@denyor@allowfield-level policy, thepolicy handler wraps it in
CASE WHEN … THEN NULL. This causedincludetosilently return empty arrays because join conditions evaluated as
fk = NULL.Fix: Two-part:
The policy handler emits a
__zs_raw_<field>alias (the raw column value)alongside the potentially-nulled expression for any PK or FK field that has
a field access policy.
Join condition builders (
buildJoinPairs, SQLite'sbuildRelationJoinFilter,M2M path in the lateral-join dialect) use the raw alias selectively:
ownedByModel = false): raw alias on both sides — the child'sFK may be denied to hide which parent it belongs to, but the parent must
still be able to fetch its children.
ownedByModel = true): raw alias only on the relation's PKside; the FK side stays as a plain ref. Denying the FK is designed to hide
the relation entirely (preserves pre-existing behavior verified by
field-level-policy.test.ts).Regression tests added in
tests/regression/test/issue-2674.test.ts.Note:
externalIdMappingin the issue title is a REST API option unrelated tothe fix; the root cause is purely at the ORM layer.
Summary by CodeRabbit
Bug Fixes
New Features
Tests