chore(policy): more test cases and update #246
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces a FromRelationContext to propagate and accumulate parent updates across nested relation operations, refactors connect/disconnect/create flows to record parentUpdates instead of immediate parent writes, adjusts NotFoundError targeting, updates dialect import paths and method visibility, and adds/reorganizes policy and client tests. README text formatting updated. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
✨ Finishing touches
🧪 Generate unit tests
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 |
|
Claude finished @ymc9's task —— View job Pull Request ReviewReviewing PR #246: "chore(policy): more test cases and update" Review Checklist
SummaryThis PR adds extensive test coverage for ZenStack's policy engine and fixes several important bugs in relation handling within CRUD operations. The changes are well-structured and improve both reliability and test coverage. Code Quality & Best Practices ✅Positive aspects:
Bug Fixes Identified ✅The changes in
Test Coverage Improvements ✅Excellent additions:
Performance Considerations ✅No performance concerns identified:
Security Assessment ✅Strong security posture:
Potential Issues & RecommendationsMinor Issues:
Recommendations:
Files Analysis
|
There was a problem hiding this comment.
Pull Request Overview
This PR focuses on expanding test coverage for policy-based CRUD operations by adding comprehensive test cases across multiple scenarios and fixing some edge cases.
- Restructures update tests into logical groups (scalar conditions, relation conditions, nested updates, relation manipulations)
- Adds extensive test coverage for relation-based policy conditions including one-to-one and one-to-many relationships
- Fixes error handling in relation connect/disconnect operations to properly throw NotFoundError
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/runtime/test/policy/crud/update.test.ts | Reorganizes tests into nested describe blocks and adds comprehensive relation condition and manipulation tests |
| packages/runtime/test/policy/crud/create.test.ts | Adds test cases for nested create operations with policy validation |
| packages/runtime/src/client/crud/operations/base.ts | Fixes error handling in relation operations to properly throw NotFoundError when updates fail |
| README.md | Updates project status from alpha to beta phase |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime/src/client/crud/operations/base.ts (1)
284-307: Avoid BigInt/Number pitfalls and false NotFound on parent updatenumAffectedRows can be bigint/number/undefined depending on dialect. Convert before comparisons.
Apply this diff:
- const result = await this.executeQuery(kysely, query, 'update'); - if (!result.numAffectedRows) { + const result = await this.executeQuery(kysely, query, 'update'); + const affected = Number(result.numAffectedRows ?? 0); + if (affected === 0) { throw new NotFoundError(fromRelation.model); }
🧹 Nitpick comments (8)
README.md (1)
24-24: LGTM: status notice updated to betaDoc-only change looks good. Minor nit: consider adding an approximate timeline or criteria for exiting beta to set expectations.
packages/runtime/test/policy/crud/create.test.ts (1)
244-275: Add a negative case for mismatched auth on nested create (non-owner side)To harden this, add one more assertion where auth.id differs from the nested User.id (should be rejected by policy).
Apply this diff to extend the test:
@@ ).resolves.toMatchObject({ id: 1, user: { id: 1, }, }); + + // negative: auth mismatch should be rejected + await expect( + db + .$setAuth({ id: 2 }) + .profile.create({ data: { id: 2, user: { create: { id: 1 } } } }), + ).toBeRejectedByPolicy();packages/runtime/test/policy/crud/update.test.ts (6)
5-5: Typo: “Scala” → “Scalar”.Rename the group for clarity.
-describe('Scala condition tests', () => { +describe('Scalar condition tests', () => {
102-125: Minor: also assert state unchanged after denied auth.After the first rejected update with auth x:0, optionally verify the record remains x:1 for completeness.
await db.foo.create({ data: { id: 1, x: 1 } }); await expect(db.$setAuth({ x: 0 }).foo.update({ where: { id: 1 }, data: { x: 2 } })).toBeRejectedNotFound(); + await expect(db.foo.findUnique({ where: { id: 1 } })).resolves.toMatchObject({ x: 1 });
159-187: Test name mismatch: this is the non-owner side case.Here User holds the FK; rename to avoid confusion with the previous test.
-it('works with to-one relation check owner side', async () => { +it('works with to-one relation check non-owner side', async () => {
431-504: Behavior check: disconnect is silently ignored when nested target update is disallowed.Line 489 comment expects no-op rather than rejection. Is this divergence (ignore vs reject) intentional for owner-side disconnect when the related model denies update? If yes, consider adding a short comment explaining the policy rationale to future readers.
506-578: Add post-rejection assertion to ensure relation unchanged.After the rejected disconnect (Lines 563-569), assert the relation still exists.
await expect( db.user.update({ where: { id: 2 }, data: { profile: { disconnect: true } }, include: { profile: true }, }), ).toBeRejectedNotFound(); + await expect( + db.user.findUnique({ where: { id: 2 }, include: { profile: true } }) + ).resolves.toMatchObject({ profile: { id: 2 } });
581-583: Track placeholders or convert to TODOs.Either add TODO comments with issue links or remove the empty suites to reduce noise. I can help scaffold upsert/update-many tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)packages/runtime/src/client/crud/operations/base.ts(11 hunks)packages/runtime/test/policy/crud/create.test.ts(1 hunks)packages/runtime/test/policy/crud/update.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/runtime/test/policy/crud/update.test.tspackages/runtime/src/client/crud/operations/base.tspackages/runtime/test/policy/crud/create.test.ts
🧬 Code graph analysis (3)
packages/runtime/test/policy/crud/update.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/src/client/crud/operations/base.ts (3)
packages/runtime/src/client/errors.ts (1)
NotFoundError(27-31)packages/sdk/src/schema/schema.ts (1)
GetModels(108-108)packages/runtime/src/client/query-utils.ts (1)
getManyToManyRelation(238-274)
packages/runtime/test/policy/crud/create.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: claude-review
- GitHub Check: build-test (20.x)
🔇 Additional comments (13)
packages/runtime/test/policy/crud/create.test.ts (1)
210-242: Good coverage for nested create on the owner sideThe auth gating and include shape checks look solid.
packages/runtime/src/client/crud/operations/base.ts (1)
1906-1973: Nice: clearer NotFound target via deleteFromModelUsing deleteFromModel improves error specificity across m2m/non-m2m paths. Count-based validation also already normalizes to number.
packages/runtime/test/policy/crud/update.test.ts (11)
6-29: LGTM: scalar field check flow is correct.Covers deny/allow paths and QB row-count; assertions look solid.
31-47: LGTM: this.x member check behaves as expected.
49-62: LGTM: default deny is enforced.
64-79: LGTM: deny rule precedence validated.
81-100: LGTM: mixed allow/deny logic validated (edge at x==1 blocked).
128-157: LGTM: to-one relation check (owner side where Profile holds FK).
189-232: LGTM: to-many “some” semantics covered across 0/1/≥2 posts.
234-286: Confirm vacuous truth semantics for “all”.You allow updates when there are no posts (Line 257). Please confirm this “all over empty set = true” behavior is intended for policy evaluation.
288-340: Confirm vacuous truth semantics for “none”.You allow updates when there are no posts (Line 311). Verify “none over empty set = true” is the desired rule.
343-385: LGTM: nested update via owner side blocked by target rule; include shape verified.
387-428: LGTM: nested update via non-owner side mirrors owner-side behavior.
|
Claude finished @ymc9's task —— View job Pull Request ReviewReviewing PR #246: "chore(policy): more test cases and update" Review Checklist
SummaryThis PR adds extensive test coverage for ZenStack's policy engine and fixes several important bugs in relation handling within CRUD operations. The changes are well-structured and improve both reliability and test coverage. Code Quality & Best Practices ✅Positive aspects:
Bug Fixes Identified ✅The changes in
Test Coverage Improvements ✅Excellent additions:
Performance Considerations ✅No performance concerns identified:
Security Assessment ✅Strong security posture:
Potential Issues & RecommendationsMinor Issues:
Recommendations:
Files Analysis
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/runtime/src/client/crud/operations/base.ts (2)
1589-1596: Normalize numAffectedRows; avoid false NotFound on no-op updatesDrivers may return bigint/undefined or 0 when values don’t change. Normalize before comparing; treat 0 carefully to prevent spurious NotFound.
- const updateResult = await this.executeQuery(kysely, query, 'connect'); - - // validate connect result - if (!updateResult.numAffectedRows || _data.length > updateResult.numAffectedRows) { - // some entities were not connected - throw new NotFoundError(model); - } + const updateResult = await this.executeQuery(kysely, query, 'connect'); + const affected = Number(updateResult.numAffectedRows ?? 0); + if (affected === 0 || _data.length > affected) { + // some entities may not have been connected (or no-op) + throw new NotFoundError(model); + }Run to find other direct numAffectedRows comparisons:
#!/bin/bash rg -nP -C2 'numAffectedRows\s*[<>=]|!\s*[\w.]*numAffectedRows' packages/runtime/src
1837-1843: Also normalize numAffectedRows in setRelation connect validationSame issue as connectRelation; compare using a normalized number.
- const r = await this.executeQuery(kysely, query, 'connect'); - - // validate result - if (!r.numAffectedRows || _data.length > r.numAffectedRows) { + const r = await this.executeQuery(kysely, query, 'connect'); + const affected = Number(r.numAffectedRows ?? 0); + if (affected === 0 || _data.length > affected) { // some entities were not connected throw new NotFoundError(model); }
🧹 Nitpick comments (6)
packages/runtime/test/client-api/update.test.ts (1)
1846-1856: Assert state after no-op disconnectStrengthen the test by asserting the relation remains unchanged (null) instead of only resolving truthy.
Apply this diff:
- await expect( - client.profile.update({ - where: { id: profile.id }, - data: { - user: { - disconnect: { id: '2' }, - }, - }, - }), - ).toResolveTruthy(); + await expect( + client.profile.update({ + where: { id: profile.id }, + data: { + user: { + disconnect: { id: '2' }, + }, + }, + include: { user: true }, + }), + ).resolves.toMatchObject({ user: null });packages/runtime/src/client/crud/dialects/base-dialect.ts (1)
1107-1131: Make logical combinators short-circuit to reduce SQL node sizePublic exposure looks fine. Add quick checks for true/false to avoid building unnecessary AND/OR trees, especially on deeply nested filters.
Apply this diff:
- and(eb: ExpressionBuilder<any, any>, ...args: Expression<SqlBool>[]) { + and(eb: ExpressionBuilder<any, any>, ...args: Expression<SqlBool>[]) { + if (args.some((arg) => this.isFalse(arg))) { + return this.false(eb); + } const nonTrueArgs = args.filter((arg) => !this.isTrue(arg)); if (nonTrueArgs.length === 0) { return this.true(eb); } else if (nonTrueArgs.length === 1) { return nonTrueArgs[0]!; } else { return eb.and(nonTrueArgs); } } - or(eb: ExpressionBuilder<any, any>, ...args: Expression<SqlBool>[]) { + or(eb: ExpressionBuilder<any, any>, ...args: Expression<SqlBool>[]) { + if (args.some((arg) => this.isTrue(arg))) { + return this.true(eb); + } const nonFalseArgs = args.filter((arg) => !this.isFalse(arg)); if (nonFalseArgs.length === 0) { return this.false(eb); } else if (nonFalseArgs.length === 1) { return nonFalseArgs[0]!; } else { return eb.or(nonFalseArgs); } } not(eb: ExpressionBuilder<any, any>, ...args: Expression<SqlBool>[]) { return eb.not(this.and(eb, ...args)); }packages/runtime/test/policy/crud/update.test.ts (1)
159-187: Duplicate test name — clarify owner vs non-owner caseBoth tests are titled “works with to-one relation check owner side”. Rename this one to disambiguate.
- it('works with to-one relation check owner side', async () => { + it('works with to-one relation check owner side (FK on parent)', async () => {packages/runtime/src/client/crud/operations/base.ts (3)
68-78: Tighten FromRelationContext typingsids is always a bag of PK fields; prefer a precise shape over any.
export type FromRelationContext<Schema extends SchemaDef> = { // the model where the relation field is defined model: GetModels<Schema>; // the relation field name field: string; // the parent entity's id fields and values - ids: any; + ids: Record<string, unknown>; // for relations owned by model, record the parent updates needed after the relation is processed - parentUpdates: Record<string, unknown>; + parentUpdates: Record<string, unknown>; };
1024-1038: Guard against conflicting parentUpdates vs explicit field updatesMerging parentUpdates into updateFields can silently override user-specified updates to FK fields. Detect and fail fast to avoid surprising writes.
- if (Object.keys(parentUpdates).length > 0) { - // merge field updates propagated from nested relation processing - Object.assign(updateFields, parentUpdates); - } + if (Object.keys(parentUpdates).length > 0) { + // avoid silent override of explicit updates + const conflicts = Object.keys(parentUpdates).filter( + (k) => k in updateFields && updateFields[k] !== parentUpdates[k], + ); + if (conflicts.length > 0) { + throw new QueryError(`Conflicting updates on fields: ${conflicts.join(', ')}`); + } + Object.assign(updateFields, parentUpdates); + }
1464-1494: Consider wrapping nested relation ops and parent update in one transactionconnect/disconnect/create/delete across relations plus the final parent update can span multiple statements. Use safeTransaction at the update() caller to ensure atomicity and prevent partial state if an intermediate step fails.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/runtime/src/client/crud/dialects/base-dialect.ts(3 hunks)packages/runtime/src/client/crud/dialects/index.ts(1 hunks)packages/runtime/src/client/crud/dialects/postgresql.ts(1 hunks)packages/runtime/src/client/crud/dialects/sqlite.ts(1 hunks)packages/runtime/src/client/crud/operations/base.ts(20 hunks)packages/runtime/src/client/options.ts(1 hunks)packages/runtime/src/plugins/policy/expression-transformer.ts(1 hunks)packages/runtime/src/plugins/policy/policy-handler.ts(2 hunks)packages/runtime/src/plugins/policy/utils.ts(1 hunks)packages/runtime/test/client-api/update.test.ts(2 hunks)packages/runtime/test/policy/crud/update.test.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/runtime/src/plugins/policy/expression-transformer.ts
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/runtime/src/client/crud/dialects/sqlite.tspackages/runtime/src/client/crud/dialects/postgresql.tspackages/runtime/src/client/options.tspackages/runtime/src/plugins/policy/utils.tspackages/runtime/src/client/crud/dialects/index.tspackages/runtime/test/client-api/update.test.tspackages/runtime/src/client/crud/dialects/base-dialect.tspackages/runtime/src/plugins/policy/policy-handler.tspackages/runtime/src/client/crud/operations/base.tspackages/runtime/test/policy/crud/update.test.ts
🧬 Code graph analysis (2)
packages/runtime/src/client/crud/operations/base.ts (5)
packages/sdk/src/schema/schema.ts (2)
SchemaDef(10-18)GetModels(108-108)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/runtime/src/client/errors.ts (1)
NotFoundError(27-31)packages/runtime/src/utils/object-utils.ts (1)
fieldsToSelectObject(11-13)packages/runtime/src/client/query-utils.ts (1)
getManyToManyRelation(238-274)
packages/runtime/test/policy/crud/update.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: claude-review
- GitHub Check: build-test (20.x)
🔇 Additional comments (11)
packages/runtime/test/client-api/update.test.ts (1)
1818-1832: LGTM: added no-op disconnect when filter doesn’t matchThe scenario accurately guards against unintended disconnects and validates include shape.
packages/runtime/src/plugins/policy/utils.ts (1)
15-15: Import path alignment to base-dialectConsistent with the repo-wide migration. No behavior change.
packages/runtime/src/client/crud/dialects/postgresql.ts (1)
23-23: Import path updated to base-dialectLooks correct and consistent with other dialects.
packages/runtime/src/client/crud/dialects/sqlite.ts (1)
23-23: Import path updated to base-dialectConsistent with the rest of the changes.
packages/runtime/src/client/crud/dialects/index.ts (1)
4-4: Import type path updateMatches the new module location; factory remains unchanged.
packages/runtime/src/client/options.ts (1)
5-5: Type import path updateAligned with base-dialect; no API/signature changes.
packages/runtime/src/plugins/policy/policy-handler.ts (2)
34-34: Import path updated to base-dialectConsistent with other files; no functional impact.
183-188: Alias VALUES subquery for portabilityAliasing the VALUES-derived table fixes dialect differences (notably SQLite requires an alias). Good call.
Please run the policy pre-create tests against both SQLite and PostgreSQL to confirm consistent behavior of the aliased VALUES subquery.
packages/runtime/test/policy/crud/update.test.ts (2)
129-157: LGTM: solid coverage for to-one owner-side policyThe scenario and assertions align with the intended owner-side permission semantics.
234-286: LGTM: to-many all/none/some quantifiers behave as expectedThe combination of initial, mixed, and all-true cases exercises policy evaluation well.
packages/runtime/src/client/crud/operations/base.ts (1)
1679-1716: LGTM: owner-side disconnect records parent FK nullification safelyThe read-then-check pattern before recording parentUpdates avoids unnecessary writes when already disconnected.
Summary by CodeRabbit
Documentation
Bug Fixes
Tests