feat(policy): many-to-many and self-relation support #256
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. WalkthroughAdds a generic zip helper and re-exports it; introduces many-to-many (M2M) awareness across runtime (count queries, query-utils, validator, policy handler, and expression transformer); updates CRUD select/include/_count typings; expands policy-focused tests and test utilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 Reviewing PR: Many-to-Many and Self-Relation SupportI've completed a comprehensive review of this pull request that adds many-to-many and self-relation support to ZenStack's policy system. Here's my detailed feedback: Review Tasks
🎯 Overall AssessmentThis is a well-implemented feature that extends ZenStack's policy system to support many-to-many and self-relations. The implementation follows established patterns and includes comprehensive test coverage. ✅ Strengths1. Comprehensive Policy Handling
2. Solid Architecture
3. Excellent Test Coverage
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for many-to-many and self-relation support within the policy framework. The changes enhance the runtime to properly handle complex relational operations and filtering for many-to-many relationships and self-referential models.
Key changes include:
- Added policy filtering support for many-to-many join tables with proper permission inheritance
- Enhanced the schema generation to include model information for better policy evaluation
- Added comprehensive test coverage for self-relations and many-to-many filtering scenarios
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/testtools/src/schema.ts | Refactored plugin module loading and enhanced return type to include model information |
| packages/runtime/test/utils.ts | Updated test client creation to handle model information and improved schema loading |
| packages/runtime/test/policy/migrated/self-relation.test.ts | Added comprehensive tests for self-relation policy scenarios (one-to-one, one-to-many, many-to-many) |
| packages/runtime/test/policy/migrated/relation-one-to-one-filter.test.ts | Added tests for one-to-one relation filtering with policy enforcement |
| packages/runtime/test/policy/migrated/relation-one-to-many-filter.test.ts | Added tests for one-to-many relation filtering with policy enforcement |
| packages/runtime/test/policy/migrated/relation-many-to-many-filter.test.ts | Added tests for many-to-many relation filtering with policy enforcement |
| packages/runtime/src/plugins/policy/policy-handler.ts | Enhanced policy handler to support many-to-many join tables and improved value row processing |
| packages/runtime/src/client/query-utils.ts | Extended many-to-many relation metadata to include primary key information |
| packages/runtime/src/client/crud/validator.ts | Refactored count selection schema generation for better reusability |
| packages/runtime/src/client/crud/operations/base.ts | Fixed typo in comment |
| packages/runtime/src/client/crud/dialects/base-dialect.ts | Enhanced count query handling for many-to-many relations |
| packages/runtime/src/client/crud-types.ts | Improved type definitions for include/select operations with count support |
| packages/common-helpers/src/zip.ts | Added utility function for zipping arrays |
| packages/common-helpers/src/index.ts | Exported new zip utility |
| TODO.md | Updated task completion status |
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: 2
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-types.ts (1)
648-665: Sanity-check: findFirst currently disallows _count (AllowCount tied to Collection) — tests rely on _countFindArgs passes AllowCount = Collection so SelectIncludeOmit<..., Collection> disables _count for findFirst (Collection = false); repository tests call findFirst with include/select _count (e.g. packages/runtime/test/policy/migrated/relation-one-to-many-filter.test.ts:417, packages/runtime/test/policy/crud/read.test.ts:280). Change FindArgs to decouple AllowCount from Collection (e.g. use SelectIncludeOmit<Schema, Model, true> for single-item finds / align with FindUniqueArgs) or document & update tests.
🧹 Nitpick comments (13)
TODO.md (1)
102-103: Fix markdown list indentation (MD007).Indent nested list items by 2 spaces under “Access Policy” to satisfy markdownlint.
- - [x] Inject "on conflict do update" - - [x] `check` function + - [x] Inject "on conflict do update" + - [x] `check` functionpackages/testtools/src/schema.ts (1)
87-89: Reuse helper in-place to avoid path duplication.
generateTsSchemaInPlacestill globs the plugin paths directly. ReusegetPluginModules()to keep a single source of truth.- const pluginModelFiles = glob.sync(path.resolve(__dirname, '../../runtime/src/plugins/**/plugin.zmodel')); + const pluginModelFiles = getPluginModules();packages/runtime/test/policy/migrated/self-relation.test.ts (3)
4-4: Hyphenate suite title for consistency/readabilityConsider "Policy self-relation tests".
-describe('Policy self relations tests', () => { +describe('Policy self-relation tests', () => {
31-79: Add connect/connectOrCreate coverage for self one-to-oneYou cover nested creates; add cases for connect/connectOrCreate to ensure policy runs on non-create paths as well (and unique constraints don’t mask policy failures).
147-200: Add connect/connectOrCreate coverage for self many-to-manyInclude followedBy.connect, following.connectOrCreate, and mixed create/connect to exercise M2M join handling under policy.
packages/runtime/src/client/crud/validator.ts (1)
613-642: makeCountSelectionSchema: allow-list is good; consider forbidding empty select payloadsRight now
{ _count: { select: {} } }passes. If you want to avoid silent no-ops, refine to require at least one key.- z.strictObject({ - select: z.strictObject( + z.strictObject({ + select: z.strictObject( /* ... */ - ), + ).refine((v) => Object.keys(v).some((k) => v[k] !== undefined), '"_count.select" must specify at least one relation'), }),packages/runtime/src/client/crud-types.ts (1)
480-504: IncludeInput: _count gating matches runtime validatorTypes now expose _count only when there are to-many relations; good. Document AllowCount in a TSDoc to aid discoverability.
-export type IncludeInput< +/** + * IncludeInput + * @template AllowCount controls whether `_count` is allowed in this shape. + */ +export type IncludeInput<packages/common-helpers/src/zip.ts (2)
1-3: Fix docstring tenseUse “Zip/Zips” instead of “Zipped”.
-/** - * Zipped two arrays into an array of tuples. - */ +/** + * Zips two arrays into an array of tuples. + */
4-11: Broaden input types to readonly arrays; tiny perf tweakSupport readonly inputs; pre-size result array to avoid dynamic growth.
-export function zip<T, U>(arr1: T[], arr2: U[]): Array<[T, U]> { - const length = Math.min(arr1.length, arr2.length); - const result: Array<[T, U]> = []; - for (let i = 0; i < length; i++) { - result.push([arr1[i]!, arr2[i]!]); - } - return result; -} +export function zip<T, U>(arr1: readonly T[], arr2: readonly U[]): Array<[T, U]> { + const length = Math.min(arr1.length, arr2.length); + const result: Array<[T, U]> = new Array(length); + for (let i = 0; i < length; i++) { + result[i] = [arr1[i]!, arr2[i]!] as [T, U]; + } + return result; +}packages/runtime/src/client/query-utils.ts (1)
263-267: Error message clarity (nitpick)Consider clarifying the invariant messages to mention “implicit many-to-many” to reduce confusion for users hitting composite-ID schemas.
-invariant(modelIdFields.length === 1, 'Only single-field ID is supported for many-to-many relation'); +invariant(modelIdFields.length === 1, 'Only single-field ID is supported for implicit many-to-many relations'); -invariant(otherIdFields.length === 1, 'Only single-field ID is supported for many-to-many relation'); +invariant(otherIdFields.length === 1, 'Only single-field ID is supported for implicit many-to-many relations');packages/runtime/src/plugins/policy/policy-handler.ts (1)
216-233: Potential crash if inserting into M2M join table with RETURNING
transformInsertQueryandonlyReturningIdcallrequireIdFields(mutationModel). For implicit join tables, this throws. Ensure join-table inserts don’t request returning, or guard this path for join tables.- if (this.onlyReturningId(node)) { + if (!this.isManyToManyJoinTable(getTableName(node.into)) && this.onlyReturningId(node)) { return result; } else {And guard
onlyReturningIdsimilarly or early-return true for M2M join tables.packages/runtime/test/policy/migrated/relation-many-to-many-filter.test.ts (2)
58-63: Great: validates policy-aware _count for M2M (basic case)Solid coverage for
_countwithout awhere. Consider adding a case with_count: { select: { m2: { where: { value: { gt: 1 } } } } }to catch aliasing/joins in the M2M path.I can draft the additional test snippet if helpful.
27-49: Seeder uses hard-coded IDs across suites (minor)Reusing
"1"/"2"across tests is fine given isolated clients, but if tests are parallelized against a shared DB it can collide. Prefer randomized IDs or per-suite prefixes.Also applies to: 118-133, 199-214
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
TODO.md(1 hunks)packages/common-helpers/src/index.ts(1 hunks)packages/common-helpers/src/zip.ts(1 hunks)packages/runtime/src/client/crud-types.ts(8 hunks)packages/runtime/src/client/crud/dialects/base-dialect.ts(1 hunks)packages/runtime/src/client/crud/operations/base.ts(1 hunks)packages/runtime/src/client/crud/validator.ts(4 hunks)packages/runtime/src/client/query-utils.ts(2 hunks)packages/runtime/src/plugins/policy/policy-handler.ts(11 hunks)packages/runtime/test/policy/migrated/relation-many-to-many-filter.test.ts(1 hunks)packages/runtime/test/policy/migrated/relation-one-to-many-filter.test.ts(1 hunks)packages/runtime/test/policy/migrated/relation-one-to-one-filter.test.ts(1 hunks)packages/runtime/test/policy/migrated/self-relation.test.ts(1 hunks)packages/runtime/test/utils.ts(3 hunks)packages/testtools/src/schema.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/runtime/test/policy/migrated/relation-one-to-many-filter.test.tspackages/common-helpers/src/zip.tspackages/runtime/src/client/crud/operations/base.tspackages/common-helpers/src/index.tspackages/runtime/test/policy/migrated/relation-one-to-one-filter.test.tspackages/runtime/test/utils.tspackages/runtime/test/policy/migrated/self-relation.test.tspackages/runtime/test/policy/migrated/relation-many-to-many-filter.test.tspackages/runtime/src/client/crud/validator.tspackages/runtime/src/client/query-utils.tspackages/runtime/src/client/crud/dialects/base-dialect.tspackages/runtime/src/plugins/policy/policy-handler.tspackages/testtools/src/schema.tspackages/runtime/src/client/crud-types.ts
**/schema.ts
📄 CodeRabbit inference engine (CLAUDE.md)
The generated TypeScript schema should be named
schema.ts
Files:
packages/testtools/src/schema.ts
🧠 Learnings (2)
📚 Learning: 2025-09-04T12:38:14.150Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T12:38:14.150Z
Learning: Applies to **/schema.ts : The generated TypeScript schema should be named `schema.ts`
Applied to files:
packages/runtime/test/utils.tspackages/testtools/src/schema.ts
📚 Learning: 2025-09-04T12:38:14.150Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T12:38:14.150Z
Learning: Applies to **/schema.zmodel : Name ZModel schema files `schema.zmodel`
Applied to files:
packages/testtools/src/schema.ts
🧬 Code graph analysis (10)
packages/runtime/test/policy/migrated/relation-one-to-many-filter.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/test/policy/migrated/relation-one-to-one-filter.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/test/utils.ts (2)
packages/testtools/src/schema.ts (2)
generateTsSchema(32-63)getPluginModules(65-67)packages/language/src/index.ts (1)
loadDocument(21-132)
packages/runtime/test/policy/migrated/self-relation.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/test/policy/migrated/relation-many-to-many-filter.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/src/client/crud/validator.ts (1)
packages/sdk/src/schema/schema.ts (1)
ModelDef(20-36)
packages/runtime/src/client/query-utils.ts (3)
packages/runtime/test/schemas/typing/schema.ts (1)
schema(9-342)packages/runtime/test/schemas/basic/schema.ts (1)
schema(9-284)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
packages/runtime/src/client/crud/dialects/base-dialect.ts (1)
packages/runtime/src/client/query-utils.ts (2)
getManyToManyRelation(239-282)buildJoinPairs(214-232)
packages/runtime/src/plugins/policy/policy-handler.ts (4)
packages/sdk/src/schema/schema.ts (1)
GetModels(108-108)packages/runtime/src/client/plugin.ts (1)
ProceedKyselyQueryFunction(195-195)packages/runtime/src/client/query-utils.ts (4)
requireField(38-56)requireModel(25-31)getManyToManyRelation(239-282)requireIdFields(63-70)packages/common-helpers/src/zip.ts (1)
zip(4-11)
packages/runtime/src/client/crud-types.ts (1)
packages/sdk/src/schema/schema.ts (2)
SchemaDef(10-18)GetModels(108-108)
🪛 markdownlint-cli2 (0.17.2)
TODO.md
102-102: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
103-103: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ 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: build-test (20.x)
- GitHub Check: claude-review
🔇 Additional comments (17)
packages/runtime/src/client/crud/operations/base.ts (1)
478-482: Comment clarification LGTM.Wording update to “implicit m2m join table” improves accuracy; no behavioral change.
packages/testtools/src/schema.ts (1)
44-46: Good centralization of plugin module loading and returning the parsed model.Using
getPluginModules()forloadDocumentand returningmodelremoves duplication and helps downstream consumers.Also applies to: 62-63
packages/runtime/test/utils.ts (1)
102-108: Solid: model is carried through and loaded with plugin modules when needed.
- Capturing
generated.modelavoids re-parsing when using Prisma push.- Fallback to
loadDocument(path.join(workDir, 'schema.zmodel'), getPluginModules())is correct and robust.Also applies to: 149-158
packages/runtime/test/policy/migrated/relation-one-to-one-filter.test.ts (1)
4-35: LGTM: comprehensive coverage for is/isNot and direct object filters.Scenarios exercise deleted visibility and nested relation predicates well.
packages/runtime/test/policy/migrated/relation-one-to-many-filter.test.ts (2)
1-1: Confirm custom matchers exist (toResolveTruthy/toResolveFalsy).These assertions are used widely here. Ensure their matchers are registered in the test setup to avoid false positives/undefined matcher errors.
428-439: Verify nested_countshould be underinclude(notselect)Types moved
_countfrom SelectInput to IncludeInput — confirm whether nested selections must useincludeand update the test at packages/runtime/test/policy/migrated/relation-one-to-many-filter.test.ts:428-439.Suggested change:
- include: { m2: { select: { _count: true } } }
- include: { m2: { include: { _count: true } } }
Repository search returned no matches with the provided ripgrep — manual verification required.
packages/runtime/src/client/crud/validator.ts (3)
6-13: Import changes look correctAdding ModelDef aligns with the new helper’s signature.
695-699: Include: gated _count attachment is correctMirrors select-path behavior; consistent with types.
605-611: Select: gated _count attachment is correctgrep shows all select._count usages are in runtime implementation or tests; tests deliberately use @ts-expect-error for models without to-many relations (e.g. packages/runtime/test/client-api/find.test.ts:990–997). No consumer code was found relying on select._count for models without to-many relations.
packages/runtime/src/client/crud-types.ts (5)
455-456: Propagating AllowCount to include is sensibleWires select/include symmetry for _count.
679-682: CreateArgs: SelectInput change aligns with new IncludeInput-driven _countLooks good.
819-821: UpdateArgs: SelectInput change aligns with new IncludeInput-driven _countLooks good.
847-849: UpsertArgs: SelectInput change aligns with new IncludeInput-driven _countLooks good.
964-966: DeleteArgs: SelectInput change aligns with new IncludeInput-driven _countLooks good.
packages/common-helpers/src/index.ts (1)
7-7: Re-export of zip is finePublic surfacing matches downstream usage.
packages/runtime/src/plugins/policy/policy-handler.ts (1)
670-731: Fix M2M join-table policy: build boolean expressions (EXISTS / COUNT>0) instead of passing subqueries to eb.andLocation: packages/runtime/src/plugins/policy/policy-handler.ts (approx. lines 670–731). eb.and is being given SelectQueryBuilder instances (subqueries) — it expects boolean expressions. Replace each side's subquery with a boolean (e.g. COUNT(...)>0 or EXISTS(...)) and AND them.
Apply this diff:
- const queries: SelectQueryBuilder<any, any, any>[] = []; + const conditions: any[] = []; const eb = expressionBuilder<any, any>(); @@ - for (const [fk, entry] of zip(['A', 'B'], sortedRecords)) { + for (const [fk, entry] of zip(['A', 'B'], sortedRecords)) { const idFields = requireIdFields(this.client.$schema, entry.model); invariant( idFields.length === 1, 'only single-field id is supported for implicit many-to-many join table', ); - const policyFilter = this.buildPolicyFilter( + const policyFilter = this.buildPolicyFilter( entry.model as GetModels<Schema>, undefined, operation === 'read' ? 'read' : 'update', ); - const query = eb - .selectFrom(entry.model) - .whereRef(`${entry.model}.${idFields[0]}`, '=', `${alias ?? tableName}.${fk}`) - .select(new ExpressionWrapper(policyFilter).as(`$condition${fk}`)); - queries.push(query); + const sideOk = eb( + eb + .selectFrom(entry.model) + .whereRef(`${entry.model}.${idFields[0]}`, '=', `${alias ?? tableName}.${fk}`) + // embed the model-side policy filter explicitly + .where(() => new ExpressionWrapper(policyFilter)) + .select((eb1) => eb1.fn.count(eb1.lit(1)).as('$count')), + '>', + 0, + ); + conditions.push(sideOk); } - return eb.and(queries).toOperationNode(); + return eb.and(conditions).toOperationNode();Optional: use eb.exists(subquery) for EXISTS semantics. Add tests that attempt insert/connect/disconnect through an M2M relation with a policy that denies updates on one side and expect the operation to be rejected.
packages/runtime/src/client/query-utils.ts (1)
1-1: Approve — M2M metadata additions verified
Adding single-field ID invariants and exposing parentPKName/otherPKName is correct. The check found no destructuring of getManyToManyRelation; callers access the returned object (e.g. packages/runtime/src/client/crud/dialects/base-dialect.ts:1061 uses m2m.parentPKName).
|
Claude finished @ymc9's task —— View job Reviewing PR: Many-to-Many and Self-Relation SupportI've completed a comprehensive review of this pull request that adds many-to-many and self-relation support to ZenStack's policy system. Here's my detailed feedback: Review Tasks
🎯 Overall AssessmentThis is a well-implemented feature that extends ZenStack's policy system to support many-to-many and self-relations. The implementation follows established patterns and includes comprehensive test coverage. ✅ Strengths1. Comprehensive Policy Handling
2. Solid Architecture
3. Excellent Test Coverage
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/runtime/src/plugins/policy/policy-handler.ts (1)
74-99: Pre-create policy flow handles implicit M2M correctlySkips constant model checks for join tables and dispatches to a dedicated checker. This addresses the earlier failure mode with requireModel on non-model tables.
🧹 Nitpick comments (2)
packages/runtime/src/plugins/policy/expression-transformer.ts (1)
337-345: Simplify boolean handling in transformValueCurrent checks work. Minor tidy: use typeof value === 'boolean' and branch once.
Apply this diff:
- if (value === true) { - return trueNode(this.dialect); - } else if (value === false) { - return falseNode(this.dialect); - } else { - return ValueNode.create(this.dialect.transformPrimitive(value, type, false) ?? null); - } + if (typeof value === 'boolean') { + return value ? trueNode(this.dialect) : falseNode(this.dialect); + } + return ValueNode.create(this.dialect.transformPrimitive(value, type, false) ?? null);packages/runtime/src/plugins/policy/policy-handler.ts (1)
289-348: Type tightening: tableName should be string (join tables aren’t models)Parameter tableName is typed as GetModels but may be a non-model join table. Use string to avoid misleading types.
Apply this diff:
- private async enforcePreCreatePolicyForManyToManyJoinTable( - tableName: GetModels<Schema>, + private async enforcePreCreatePolicyForManyToManyJoinTable( + tableName: string,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/common-helpers/src/zip.ts(1 hunks)packages/runtime/src/client/crud/dialects/base-dialect.ts(1 hunks)packages/runtime/src/plugins/policy/expression-transformer.ts(2 hunks)packages/runtime/src/plugins/policy/policy-handler.ts(10 hunks)packages/runtime/test/policy/migrated/relation-many-to-many-filter.test.ts(1 hunks)packages/runtime/test/policy/migrated/self-relation.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/common-helpers/src/zip.ts
- packages/runtime/test/policy/migrated/relation-many-to-many-filter.test.ts
- packages/runtime/src/client/crud/dialects/base-dialect.ts
- packages/runtime/test/policy/migrated/self-relation.test.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/plugins/policy/policy-handler.tspackages/runtime/src/plugins/policy/expression-transformer.ts
🧬 Code graph analysis (2)
packages/runtime/src/plugins/policy/policy-handler.ts (5)
packages/runtime/src/plugins/policy/errors.ts (1)
RejectedByPolicyError(4-11)packages/sdk/src/schema/schema.ts (1)
GetModels(108-108)packages/runtime/src/client/plugin.ts (1)
ProceedKyselyQueryFunction(195-195)packages/runtime/src/client/query-utils.ts (4)
requireField(38-56)requireModel(25-31)getManyToManyRelation(239-282)requireIdFields(63-70)packages/common-helpers/src/zip.ts (1)
zip(4-11)
packages/runtime/src/plugins/policy/expression-transformer.ts (1)
packages/runtime/src/plugins/policy/utils.ts (2)
trueNode(21-23)falseNode(28-30)
⏰ 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: build-test (20.x)
- GitHub Check: claude-review
🔇 Additional comments (6)
packages/runtime/src/plugins/policy/expression-transformer.ts (1)
47-47: Import of falseNode is correctThis pairs with existing trueNode usage and enables consistent boolean literal handling.
packages/runtime/src/plugins/policy/policy-handler.ts (5)
260-287: Good: pre-create dispatcher branches M2M vs model pathsThe new enforcePreCreatePolicy signature and branching look correct and keep model logic unchanged.
577-583: M2M filter short‑circuit looks goodDelegating to getModelPolicyFilterForManyToManyJoinTable early is the right layering to avoid recursion on normal models.
703-738: resolveManyToManyJoinTable: solid detection and orderingCorrectly handles self-relations and enforces single-field IDs with clear errors.
740-745: Comment tweak LGTMClarifies that the join table is implicit.
672-701: Policy extraction unchanged in behaviorReads @@allow/@@deny and filters per operation. Looks consistent with prior behavior.
|
Claude finished @ymc9's task —— View job Reviewing PR: Many-to-Many and Self-Relation SupportI've completed a comprehensive review of this pull request that adds many-to-many and self-relation support to ZenStack's policy system. Here's my detailed feedback: Review Tasks
🎯 Overall AssessmentThis is a well-implemented feature that extends ZenStack's policy system to support many-to-many and self-relations. The implementation follows established patterns and includes comprehensive test coverage. ✅ Strengths1. Comprehensive Policy Handling
2. Solid Architecture
3. Excellent Test Coverage
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/runtime/src/plugins/policy/policy-handler.ts (2)
425-469: unwrapCreateValueRow still resolves fields for implicit M2M join tables (will throw)For ValueNode items, you still call requireField even when the target is an implicit M2M join table. Bypass field lookups/transforms and pass raw FK values.
Apply this diff:
- if (typeof item === 'object' && item && 'kind' in item) { - const fieldDef = requireField(this.client.$schema, model, fields[i]!); - invariant(item.kind === 'ValueNode', 'expecting a ValueNode'); - result.push({ - node: ValueNode.create( - this.dialect.transformPrimitive( - (item as ValueNode).value, - fieldDef.type as BuiltinType, - !!fieldDef.array, - ), - ), - raw: (item as ValueNode).value, - }); + if (typeof item === 'object' && item && 'kind' in item) { + invariant((item as any).kind === 'ValueNode', 'expecting a ValueNode'); + const v = (item as ValueNode).value; + if (isImplicitManyToManyJoinTable) { + // Join table: raw FKs, no model field defs. + result.push({ node: ValueNode.create(v), raw: v }); + } else { + const fieldDef = requireField(this.client.$schema, model, fields[i]!); + result.push({ + node: ValueNode.create( + this.dialect.transformPrimitive(v, fieldDef.type as BuiltinType, !!fieldDef.array), + ), + raw: v, + }); + }
750-787: Bug: M2M join-table filter builds AND over queries instead of boolean conditionsYou’re constructing SelectQueryBuilders and passing them to eb.and, and the policy filters are only selected, not enforced in WHERE. Use EXISTS(subquery WITH policy in WHERE) and AND those conditions.
Apply this diff:
- const aQuery = eb - .selectFrom(m2m.firstModel) - .whereRef(`${m2m.firstModel}.${m2m.firstIdField}`, '=', `${joinTable}.A`) - .select(() => - new ExpressionWrapper( - this.buildPolicyFilter(m2m.firstModel as GetModels<Schema>, undefined, checkForOperation), - ).as('$conditionA'), - ); - - const bQuery = eb - .selectFrom(m2m.secondModel) - .whereRef(`${m2m.secondModel}.${m2m.secondIdField}`, '=', `${joinTable}.B`) - .select(() => - new ExpressionWrapper( - this.buildPolicyFilter(m2m.secondModel as GetModels<Schema>, undefined, checkForOperation), - ).as('$conditionB'), - ); - - return eb.and([aQuery, bQuery]).toOperationNode(); + const aExists = eb.exists( + eb + .selectFrom(m2m.firstModel) + .select(eb.val(1).as('$t')) + .whereRef(`${m2m.firstModel}.${m2m.firstIdField}`, '=', `${joinTable}.A`) + .where(() => + new ExpressionWrapper( + this.buildPolicyFilter(m2m.firstModel as GetModels<Schema>, undefined, checkForOperation), + ), + ), + ); + const bExists = eb.exists( + eb + .selectFrom(m2m.secondModel) + .select(eb.val(1).as('$t')) + .whereRef(`${m2m.secondModel}.${m2m.secondIdField}`, '=', `${joinTable}.B`) + .where(() => + new ExpressionWrapper( + this.buildPolicyFilter(m2m.secondModel as GetModels<Schema>, undefined, checkForOperation), + ), + ), + ); + return eb.and([aExists, bExists]).toOperationNode();
🧹 Nitpick comments (4)
packages/runtime/test/policy/crud/create.test.ts (1)
276-322: M2M create policy tests: good coverageBoth implicit and named M2M create flows are exercised with positive/negative cases and usePrismaPush. Consider adding:
- connect and connectOrCreate cases
- a self-relation M2M variant
Also applies to: 324-369
packages/runtime/src/plugins/policy/policy-handler.ts (1)
288-347: Prefer EXISTS for pre-create M2M checksCurrent approach selects boolean projections. EXISTS subqueries in selections reduce ambiguity and align with the filter usage elsewhere.
- const filterA = this.buildPolicyFilter(m2m.firstModel as GetModels<Schema>, undefined, 'update'); - const queryA = eb - .selectFrom(m2m.firstModel) - .where(eb(eb.ref(`${m2m.firstModel}.${m2m.firstIdField}`), '=', aValue)) - .select(() => new ExpressionWrapper(filterA).as('$t')); + const filterA = this.buildPolicyFilter(m2m.firstModel as GetModels<Schema>, undefined, 'update'); + const aExists = eb.exists( + eb + .selectFrom(m2m.firstModel) + .select(eb.val(1).as('$t')) + .where(eb(eb.ref(`${m2m.firstModel}.${m2m.firstIdField}`), '=', aValue)) + .where(() => new ExpressionWrapper(filterA)), + ); @@ - const filterB = this.buildPolicyFilter(m2m.secondModel as GetModels<Schema>, undefined, 'update'); - const queryB = eb - .selectFrom(m2m.secondModel) - .where(eb(eb.ref(`${m2m.secondModel}.${m2m.secondIdField}`), '=', bValue)) - .select(() => new ExpressionWrapper(filterB).as('$t')); + const filterB = this.buildPolicyFilter(m2m.secondModel as GetModels<Schema>, undefined, 'update'); + const bExists = eb.exists( + eb + .selectFrom(m2m.secondModel) + .select(eb.val(1).as('$t')) + .where(eb(eb.ref(`${m2m.secondModel}.${m2m.secondIdField}`), '=', bValue)) + .where(() => new ExpressionWrapper(filterB)), + ); @@ - const queryNode: SelectQueryNode = { + const queryNode: SelectQueryNode = { kind: 'SelectQueryNode', selections: [ - SelectionNode.create(AliasNode.create(queryA.toOperationNode(), IdentifierNode.create('$conditionA'))), - SelectionNode.create(AliasNode.create(queryB.toOperationNode(), IdentifierNode.create('$conditionB'))), + SelectionNode.create(AliasNode.create(aExists.toOperationNode(), IdentifierNode.create('$conditionA'))), + SelectionNode.create(AliasNode.create(bExists.toOperationNode(), IdentifierNode.create('$conditionB'))), ], };packages/runtime/test/policy/crud/read.test.ts (1)
168-206: M2M read policy tests: solidImplicit and named M2M reads validate include filtering and _count. Consider adding a self-relation M2M read test.
Also applies to: 208-246
packages/runtime/test/policy/crud/update.test.ts (1)
342-381: M2M update gating via related policy: goodCovers both implicit and named relations and policy flips. Optional: add a self-relation M2M case.
Also applies to: 383-422
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/runtime/src/plugins/policy/expression-transformer.ts(5 hunks)packages/runtime/src/plugins/policy/policy-handler.ts(9 hunks)packages/runtime/test/policy/crud/create.test.ts(1 hunks)packages/runtime/test/policy/crud/read.test.ts(1 hunks)packages/runtime/test/policy/crud/update.test.ts(2 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/test/policy/crud/read.test.tspackages/runtime/test/policy/crud/create.test.tspackages/runtime/src/plugins/policy/policy-handler.tspackages/runtime/src/plugins/policy/expression-transformer.ts
🧬 Code graph analysis (5)
packages/runtime/test/policy/crud/update.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/test/policy/crud/read.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/test/policy/crud/create.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/src/plugins/policy/policy-handler.ts (6)
packages/runtime/src/plugins/policy/errors.ts (1)
RejectedByPolicyError(4-11)packages/sdk/src/schema/schema.ts (2)
GetModels(108-108)BuiltinType(84-94)packages/runtime/src/client/plugin.ts (1)
ProceedKyselyQueryFunction(195-195)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/runtime/src/client/query-utils.ts (4)
requireField(38-56)requireModel(25-31)getManyToManyRelation(239-282)requireIdFields(63-70)packages/runtime/src/plugins/policy/types.ts (1)
PolicyOperation(12-12)
packages/runtime/src/plugins/policy/expression-transformer.ts (2)
packages/runtime/src/plugins/policy/utils.ts (2)
trueNode(21-23)falseNode(28-30)packages/runtime/src/client/query-utils.ts (1)
getManyToManyRelation(239-282)
⏰ 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: build-test (20.x)
- GitHub Check: claude-review
🔇 Additional comments (3)
packages/runtime/src/plugins/policy/expression-transformer.ts (2)
343-351: Boolean literal emission is correctUsing trueNode/falseNode avoids dialect ambiguities for booleans. LGTM.
552-556: M2M relation access join path looks correctThe join correctly ties otherPK→join.FK and join.FK→parentPK. Please add/confirm tests for self-relation M2M (same model on both sides) to verify orderedFK logic.
Also applies to: 594-615
packages/runtime/test/policy/crud/update.test.ts (1)
974-1101: M2M relation manipulation under policy: thoroughCreate/connect/disconnect/set/delete scenarios are well covered, including non-effects when blocked. LGTM.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores