feat: "create" access policy implementation #242
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. WalkthroughApplies defaults (including generator and Json defaults) during create, enforces create-rule restrictions against non-owned relation references, shifts internal alias handling from strings to AST IdentifierNodes, rewrites create-policy evaluation to use a VALUES-derived synthetic table and removes per-entity thisEntity context, updates plugin hook args, adds ExpressionUtils.not, and adds/adjusts related tests and helpers. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1){packages,samples,tests}/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)packages/runtime/src/client/query-utils.ts (4)
⏰ 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). (1)
🔇 Additional comments (2)
✨ 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 Review: Create Policy ImplementationI've completed a comprehensive review of this PR implementing "create" access policy functionality. Here's my detailed feedback: Review Tasks:
🎯 Implementation SummaryThis PR implements comprehensive support for "create" access policies in ZenStack, adding proper policy enforcement during entity creation operations. The implementation spans language validation, runtime policy handling, and comprehensive test coverage. ✅ Strengths1. Robust Policy Handler Implementation (
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements access policy support for the "create" operation, enabling the framework to evaluate policy rules when creating new entities. The implementation includes both pre-creation validation and comprehensive testing to ensure policies are correctly enforced.
- Added comprehensive validation for "create" policy rules including field checks and auth comparisons
- Enhanced expression transformers to handle auth comparisons with model identity fields
- Expanded test coverage with new test files for create operations and reference equality scenarios
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk/src/model-utils.ts | Added isTypeDef import to support type definitions in auth declarations |
| packages/runtime/test/utils.ts | Added test logger utility function for debugging query logs |
| packages/runtime/test/policy/ref-equality.test.ts | New test file for reference equality scenarios with composite keys |
| packages/runtime/test/policy/deep-nested.test.ts | Updated test to fix relation mapping and add cascade delete validation |
| packages/runtime/test/policy/crud/dumb-rules.test.ts | New test file for basic create policy validation with literal conditions |
| packages/runtime/test/policy/crud/create.test.ts | New comprehensive test file for create operation policy enforcement |
| packages/runtime/test/plugin/on-kysely-query.test.ts | Updated plugin API to use client instead of kysely parameter |
| packages/runtime/src/schema/expression.ts | Added not utility function for logical negation operations |
| packages/runtime/src/plugins/policy/policy-handler.ts | Major implementation of create policy enforcement with pre-creation validation |
| packages/runtime/src/plugins/policy/expression-transformer.ts | Enhanced auth comparison handling and removed deprecated thisEntity logic |
| packages/runtime/src/client/query-utils.ts | Extended field resolution to support both models and type definitions |
| packages/runtime/src/client/plugin.ts | Removed deprecated kysely parameter from plugin API |
| packages/runtime/src/client/executor/zenstack-query-executor.ts | Updated to match new plugin API without kysely parameter |
| packages/runtime/src/client/executor/name-mapper.ts | Improved type safety for alias handling in query name mapping |
| packages/runtime/src/client/executor/kysely-utils.ts | Simplified alias stripping utility by removing unused invariant |
| packages/runtime/src/client/crud/validator.ts | Removed redundant model existence check |
| packages/runtime/src/client/crud/operations/base.ts | Enhanced value filling to include default values during creation |
| packages/language/src/validators/attribute-application-validator.ts | Added validation for JSON defaults and non-owned relation restrictions in create rules |
| TODO.md | Added Global omit feature to the todo list |
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 (2)
packages/runtime/src/plugins/policy/expression-transformer.ts (1)
233-236: Fix “every (^)" and “none (!)” collection predicate semantics.Current logic negates the predicate for "!" instead of "^", making "!" behave like "every". Correct mapping is:
- some (?) => count(P) > 0
- none (!) => count(P) == 0
- every (^) => count(!P) == 0
Apply this diff:
- if (expr.op === '!') { - predicateFilter = logicalNot(predicateFilter); - } + if (expr.op === '^') { + // for "every", count items that do NOT satisfy the predicate + predicateFilter = logicalNot(predicateFilter); + }Also applies to: 239-243
packages/runtime/src/plugins/policy/policy-handler.ts (1)
74-90: Wrap pre-create check + mutation + read-back in a transaction.Without a transaction, an insert that later fails read-back policy can leave persisted rows. The TODO should be addressed to ensure atomicity.
I can provide a minimal transactional flow using your existing ProceedKyselyQueryFunction hook if you expose a transaction wrapper. Want me to draft it?
Also applies to: 92-107, 109-110
🧹 Nitpick comments (17)
TODO.md (1)
86-86: Fix markdownlint MD007 (ul-indent) for the new bulletmarkdownlint flags Line 86: expected indent 4, found 8. If “Global omit” is intended as a sibling of “Misc” (ORM-level item), outdent to 4 spaces. If it’s a child under “Misc”, either reformat the whole subsection to satisfy the rule or suppress MD007 for this line.
Option A — make it ORM-level (4-space indent):
- - [ ] Global omit + - [ ] Global omitOption B — keep as child under “Misc”, but silence the lint just for this line:
+ <!-- markdownlint-disable-next-line MD007 --> - [ ] Global omitCan you confirm the intended hierarchy? I’ll adjust the diff accordingly.
packages/runtime/src/schema/expression.ts (1)
91-93: Add explicit return type for the new helperDeclare the return type for API clarity and stronger contracts.
- not: (expr: Expression) => { + not: (expr: Expression): UnaryExpression => { return ExpressionUtils.unary('!', expr); },packages/runtime/test/utils.ts (1)
196-198: Guard against non-query log eventsKysely may emit non-query events; avoid accessing undefined e.query.
-export function testLogger(e: LogEvent) { - console.log(e.query.sql, e.query.parameters); -} +export function testLogger(e: LogEvent) { + if ((e as any).query) { + console.log((e as any).query.sql, (e as any).query.parameters); + } +}packages/runtime/src/client/plugin.ts (1)
189-189: Align proceed return type with callbackUse UnknownRow instead of any for type safety and consistency with OnKyselyQueryCallback.
-export type ProceedKyselyQueryFunction = (query: RootOperationNode) => Promise<QueryResult<any>>; +export type ProceedKyselyQueryFunction = (query: RootOperationNode) => Promise<QueryResult<UnknownRow>>;packages/runtime/test/policy/deep-nested.test.ts (1)
619-621: Good coverage of cascade visibility; consider also asserting M1 cascade on M2 deleteThe added assertions and the explicit delete of M2 validate cascading to M4. Optionally, add a scenario where an M1 still references M2 when deleting M2 and assert M1 is also removed (since M1.m2 uses onDelete: Cascade).
@@ - await expect(db.m2.findMany()).resolves.toHaveLength(1); - await expect(db.m4.findMany()).resolves.toHaveLength(1); + await expect(db.m2.findMany()).resolves.toHaveLength(1); + await expect(db.m4.findMany()).resolves.toHaveLength(1); + + // Optional: verify M1 also cascades when deleting M2 with an existing reference + await db.m1.create({ data: { myId: 'cascade-m1', m2: { connect: { id: 1 } } } }); + await expect(db.m1.findUnique({ where: { myId: 'cascade-m1' } })).resolves.toBeTruthy(); @@ - await db.m2.delete({ where: { id: 1 } }); + await db.m2.delete({ where: { id: 1 } }); await expect(db.m4.findMany()).resolves.toHaveLength(0); + // Optional: M1 referencing M2 should be deleted by cascade + await expect(db.m1.findUnique({ where: { myId: 'cascade-m1' } })).resolves.toBeNull();Also applies to: 632-632, 636-640
packages/language/src/validators/attribute-application-validator.ts (2)
157-173: JSON @default currently accepts only string literalsIf this is intentional, LGTM. If not, consider allowing object/array literal expressions (e.g., ObjectExpr/ArrayExpr) as defaults for Json, validating they’re JSON-safe. This avoids double-quoting and escaping in schema files.
196-255: Auth()-scoped member access may not be skipped reliablyisAuthOrAuthMemberAccess(node) on a ReferenceExpr won’t detect cases like auth().foo where the ReferenceExpr is the member of a MemberAccessExpr. Check the container MemberAccessExpr instead.
Apply this diff:
@@ -import { +import { ArrayExpr, Attribute, AttributeArg, AttributeParam, DataField, DataFieldAttribute, DataModelAttribute, InternalAttribute, ReferenceExpr, isArrayExpr, isAttribute, isDataField, isDataModel, isEnum, + isMemberAccessExpr, isReferenceExpr, isTypeDef, } from '../generated/ast'; @@ - if (isAuthOrAuthMemberAccess(node)) { + // skip references coming from auth() chains, e.g. auth().foo + if (isMemberAccessExpr(node.$container) && isAuthOrAuthMemberAccess(node.$container)) { // field reference is from auth() or access from auth(), not a relation query return false; }Also, M:N relations (no owner) will be flagged by design; confirm that’s desired UX.
packages/runtime/test/policy/crud/dumb-rules.test.ts (1)
37-40: Consider parameterizing the four create checks to reduce duplicationUse a table-driven case with it.each to keep the test compact and easier to extend.
Example:
const cases: Array<[keyof typeof db, boolean]> = [['a', true], ['b', false], ['c', true], ['d', false]]; for (const [model, allowed] of cases) { const op = db[model].create({ data: { x: 0 } }); if (allowed) await expect(op).resolves.toMatchObject({ x: 0 }); else await expect(op).toBeRejectedByPolicy(); }packages/runtime/test/policy/ref-equality.test.ts (1)
16-23: Add a relation-based create assertion for Post to cover author equalityThis file defines Post’s rule but doesn’t exercise it. Add one positive and one negative case to lock behavior.
Example additions:
// negative: mismatched auth -> denied await expect( db.$setAuth({ id1: 9, id2: 9 }).post.create({ data: { title: 't', authorId1: 1, authorId2: 2 }, }), ).toBeRejectedByPolicy(); // positive: matching auth -> allowed await expect( db.$setAuth({ id1: 1, id2: 2 }).post.create({ data: { title: 't', authorId1: 1, authorId2: 2 }, }), ).resolves.toMatchObject({ authorId1: 1, authorId2: 2 });Also applies to: 34-39
packages/runtime/src/client/executor/kysely-utils.ts (1)
1-1: Make the new intent explicit with a typed signatureAdd an explicit return type and a type-only import to avoid inference drift and document the contract.
-import { type OperationNode, AliasNode } from 'kysely'; +import { type OperationNode, type IdentifierNode, AliasNode } from 'kysely'; @@ -export function stripAlias(node: OperationNode) { +export function stripAlias( + node: OperationNode, +): { alias: IdentifierNode | undefined; node: OperationNode } { if (AliasNode.is(node)) { return { alias: node.alias, node: node.node }; } else { return { alias: undefined, node }; } }Also applies to: 6-12
packages/runtime/test/policy/crud/create.test.ts (1)
151-168: Auth type appears to be used without a TypeDefThis suite references
auth().idbut doesn't declare atype Auth { ... @@auth }. Other tests do, but this one relies on implicit shape, which may break if the transformer now requires TypeDef-backed resolution.Consider adding an explicit Auth TypeDef to this test block:
model User { id Int @id profile Profile? @@allow('all', true) } +type Auth { + id Int + @@auth +} + model Profile {packages/runtime/src/client/crud/operations/base.ts (3)
728-761: Avoid drift: reuse the same default-filling logic in the createMany fallbackThe fallback re-implements part of the default logic and skips generator/updatedAt/Json-string parsing. Reuse
fillGeneratedAndDefaultValuesto keep behavior identical and reduce maintenance.Apply:
- for (const item of createData) { - if (Object.keys(item).length === allPassedFields.length) { - continue; - } - for (const field of allPassedFields) { - if (!(field in item)) { - const fieldDef = this.requireField(model, field); - if ( - fieldDef.default !== undefined && - fieldDef.default !== null && - typeof fieldDef.default !== 'object' - ) { - item[field] = this.dialect.transformPrimitive( - fieldDef.default, - fieldDef.type as BuiltinType, - !!fieldDef.array, - ); - } - } - } - } + createData = createData.map((item) => this.fillGeneratedAndDefaultValues(modelDef, item));
844-876: JSON default parsing should be guarded
JSON.parsecan throw if a schema’s Json default is malformed. Catch and surface aQueryErrorwith field context to avoid opaque crashes.Apply:
- if (fieldDef.type === 'Json' && typeof value === 'string') { - // Schema uses JSON string for default value of Json fields - value = JSON.parse(value); - } + if (fieldDef.type === 'Json' && typeof value === 'string') { + // Schema uses JSON string for default value of Json fields + try { + value = JSON.parse(value); + } catch (err) { + throw new QueryError( + `Invalid JSON default for field "${field}" on model "${modelDef.name}"`, + err, + ); + } + }
878-898: UUID v7 detection is brittleMatching
uuid(7)only by numeric literal7is fragile. Consider accepting string'v7'too, or validating via a dedicated generator kind to avoid accidental collisions.Example:
- .with('uuid', () => - defaultValue.args?.[0] && - ExpressionUtils.isLiteral(defaultValue.args?.[0]) && - defaultValue.args[0].value === 7 + .with('uuid', () => + defaultValue.args?.[0] && + ExpressionUtils.isLiteral(defaultValue.args?.[0]) && + (defaultValue.args[0].value === 7 || defaultValue.args[0].value === 'v7') ? uuid.v7() : uuid.v4(), )packages/runtime/src/client/executor/name-mapper.ts (2)
25-25: Tighten alias typing or guard at use sites.Alias is widened to OperationNode, but AliasNode.create expects an IdentifierNode. Add a guard in wrapAlias (and similar call sites) to avoid accidentally passing a non-identifier alias.
Apply this diff:
- private wrapAlias<T extends OperationNode>(node: T, alias: OperationNode | undefined) { - return alias ? AliasNode.create(node, alias) : node; - } + private wrapAlias<T extends OperationNode>(node: T, alias: OperationNode | undefined) { + return IdentifierNode.is(alias) ? AliasNode.create(node, alias) : node; + }Also applies to: 305-307, 384-384, 390-390
123-123: Remove redundant alias null-checks.Inside an if (scope.alias) block, re-checking scope.alias is redundant; simplify for readability.
Apply this diff:
- if (scope.alias && IdentifierNode.is(scope.alias) && scope.alias.name === mappedTableName) { + if (IdentifierNode.is(scope.alias) && scope.alias.name === mappedTableName) {- if (scope.alias && IdentifierNode.is(scope.alias) && scope.alias.name === qualifier) { + if (IdentifierNode.is(scope.alias) && scope.alias.name === qualifier) {Also applies to: 251-251
packages/runtime/src/plugins/policy/expression-transformer.ts (1)
544-556: De-duplicate logical-not helpers.Both logicalNot (from utils) and buildLogicalNot exist; use one consistently (prefer logicalNot or ExpressionUtils.not) and drop the other.
Also applies to: 234-235
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
TODO.md(1 hunks)packages/language/src/validators/attribute-application-validator.ts(3 hunks)packages/runtime/src/client/crud/operations/base.ts(4 hunks)packages/runtime/src/client/crud/validator.ts(2 hunks)packages/runtime/src/client/executor/kysely-utils.ts(1 hunks)packages/runtime/src/client/executor/name-mapper.ts(9 hunks)packages/runtime/src/client/executor/zenstack-query-executor.ts(0 hunks)packages/runtime/src/client/plugin.ts(1 hunks)packages/runtime/src/client/query-utils.ts(2 hunks)packages/runtime/src/plugins/policy/expression-transformer.ts(10 hunks)packages/runtime/src/plugins/policy/policy-handler.ts(5 hunks)packages/runtime/src/schema/expression.ts(1 hunks)packages/runtime/test/plugin/on-kysely-query.test.ts(1 hunks)packages/runtime/test/policy/crud/create.test.ts(1 hunks)packages/runtime/test/policy/crud/dumb-rules.test.ts(1 hunks)packages/runtime/test/policy/deep-nested.test.ts(4 hunks)packages/runtime/test/policy/ref-equality.test.ts(1 hunks)packages/runtime/test/utils.ts(2 hunks)packages/sdk/src/model-utils.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/runtime/src/client/executor/zenstack-query-executor.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/schema/expression.tspackages/runtime/test/policy/crud/dumb-rules.test.tspackages/runtime/test/policy/ref-equality.test.tspackages/runtime/src/client/crud/operations/base.tspackages/runtime/src/client/crud/validator.tspackages/runtime/test/utils.tspackages/runtime/test/policy/crud/create.test.tspackages/sdk/src/model-utils.tspackages/runtime/src/client/query-utils.tspackages/runtime/src/client/plugin.tspackages/runtime/test/plugin/on-kysely-query.test.tspackages/runtime/src/client/executor/name-mapper.tspackages/language/src/validators/attribute-application-validator.tspackages/runtime/src/client/executor/kysely-utils.tspackages/runtime/src/plugins/policy/expression-transformer.tspackages/runtime/test/policy/deep-nested.test.tspackages/runtime/src/plugins/policy/policy-handler.ts
🧬 Code graph analysis (12)
packages/runtime/src/schema/expression.ts (1)
packages/sdk/src/schema/expression.ts (1)
Expression(1-10)
packages/runtime/test/policy/crud/dumb-rules.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/test/policy/ref-equality.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/src/client/crud/operations/base.ts (1)
packages/sdk/src/schema/schema.ts (2)
ModelDef(20-36)BuiltinType(84-94)
packages/runtime/src/client/crud/validator.ts (2)
packages/runtime/src/client/crud/operations/base.ts (1)
requireModel(109-111)packages/runtime/src/client/query-utils.ts (1)
requireModel(24-30)
packages/runtime/test/policy/crud/create.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/sdk/src/model-utils.ts (1)
packages/language/src/generated/ast.ts (2)
isDataModel(391-393)isTypeDef(737-739)
packages/runtime/src/client/query-utils.ts (3)
packages/runtime/src/client/crud/operations/base.ts (3)
schema(86-88)requireField(117-119)getModel(113-115)packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/runtime/src/client/errors.ts (1)
QueryError(13-17)
packages/runtime/src/client/executor/name-mapper.ts (2)
packages/runtime/src/client/crud/operations/base.ts (1)
requireModel(109-111)packages/runtime/src/client/query-utils.ts (1)
requireModel(24-30)
packages/language/src/validators/attribute-application-validator.ts (3)
packages/sdk/src/schema/schema.ts (1)
AttributeApplication(38-41)packages/language/src/generated/ast.ts (8)
isDataField(344-346)Expression(115-115)Expression(117-117)isDataModel(391-393)DataField(333-340)DataField(342-342)BinaryExpr(256-262)BinaryExpr(264-264)packages/language/src/utils.ts (5)
getStringLiteral(72-74)isDataFieldReference(138-140)isRelationshipField(145-147)isAuthOrAuthMemberAccess(130-132)isCollectionPredicate(454-456)
packages/runtime/src/plugins/policy/expression-transformer.ts (7)
packages/sdk/src/schema/expression.ts (2)
BinaryExpression(39-44)Expression(1-10)packages/runtime/src/client/errors.ts (1)
QueryError(13-17)packages/runtime/src/schema/expression.ts (1)
ExpressionUtils(16-120)packages/runtime/src/client/query-utils.ts (2)
getModel(16-18)requireField(37-55)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/sdk/src/schema/schema.ts (2)
FieldDef(59-74)GetModels(108-108)packages/runtime/src/plugins/policy/utils.ts (1)
conjunction(49-65)
packages/runtime/src/plugins/policy/policy-handler.ts (4)
packages/runtime/src/client/query-utils.ts (1)
requireModel(24-30)packages/sdk/src/schema/schema.ts (1)
GetModels(108-108)packages/runtime/src/client/contract.ts (1)
CRUD(214-214)packages/runtime/src/plugins/policy/utils.ts (1)
falseNode(28-30)
🪛 markdownlint-cli2 (0.17.2)
TODO.md
86-86: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
🔇 Additional comments (22)
packages/sdk/src/model-utils.ts (1)
5-5: Confirmed explicit return type and TypeDef coverageReturn type is now
DataModel | TypeDef | undefinedandAttributeTargetalready includesTypeDeffor bothgetAttribute/hasAttribute.packages/runtime/test/utils.ts (1)
6-6: Type-only import LGTMUsing a type-only import for LogEvent is correct and keeps the runtime bundle clean.
packages/runtime/src/client/plugin.ts (1)
182-187: Confirmargs.kyselyremoval across code and docs
Scripts didn’t find any in-repo usages, but please manually verify that all downstream plugins and markdown documentation have been updated to no longer referenceargs.kysely.packages/runtime/test/policy/deep-nested.test.ts (1)
10-11: 1:1 relation ownership switch looks correctMoving the FK to M1 with onDelete: Cascade and making M2.m1 optional is consistent with a 1:1 where M1 references M2. Deleting M2 should cascade to M1; deleting M1 should not affect M2. Tests below reflect this.
Also applies to: 23-23
packages/language/src/validators/attribute-application-validator.ts (2)
3-3: Imports alignment looks goodNew symbols match the usages below.
Also applies to: 11-11, 17-17, 25-27
189-194: Create/all policy: non-owned relation guard—good additionThe early check for the second arg and delegating to expression scan is correct.
packages/runtime/src/client/crud/validator.ts (2)
25-25: Import cleanup: centralized model errorsDropping QueryError here is fine since requireModel throws consistently.
281-281: Verify no residual manual “Model … not found” throws
Quick grep to catch any leftover hardcoded errors:rg 'Model ".+" not found in schema' -g '*.ts' --ignore-dir node_modules rg 'throw new QueryError' -g '*.ts' --ignore-dir node_modulesRun tests to confirm QueryError behavior still matches expected.
packages/runtime/test/plugin/on-kysely-query.test.ts (1)
87-99: Approve hook arg rename and resolve migration
Verified no remaining{ kysely }references in allonKyselyQuerycallbacks across the codebase.packages/runtime/test/policy/crud/dumb-rules.test.ts (1)
5-41: LGTM: solid coverage for boolean create rulesThe test correctly exercises allow/deny paths and uses minimal assertions.
packages/runtime/test/policy/ref-equality.test.ts (1)
28-39: LGTM: auth() reference equality over composite key worksUnauthenticated create is denied; matching auth allows create as expected.
packages/runtime/src/client/executor/kysely-utils.ts (1)
6-12: No action needed for stripAlias alias usage
Verified that all call sites treataliasas anOperationNode(e.g., passed towrapAlias/withScope) and none perform string operations on it.packages/runtime/test/policy/crud/create.test.ts (2)
94-106: Cross-dialect behavior for explicit insert into autoincrement fieldCreating with
{ id: 1 }on an@default(autoincrement())field passes here. Some databases disallow or require special permissions for explicit values on identity/serial columns. Ensure the test harness/dialects under CI uniformly support this.If not guaranteed, gate this assertion by dialect or add a separate case that asserts rejection when explicit id is not allowed.
4-201: Good coverage of create-policy pathsSolid scenarios: scalar/this checks, default-deny, allow/deny mix, defaults, db-generated, non-owned relation rejection, auth(), owned to-one with create/connect/set/include. This should catch most regressions in the create path.
packages/runtime/src/client/crud/operations/base.ts (1)
356-363: Applying generated and default values before insertSwitching to
fillGeneratedAndDefaultValuesfor create aligns defaults/generators (including updatedAt/auth/json) with policy evaluation and insert payloads. Good improvement.packages/runtime/src/client/executor/name-mapper.ts (2)
361-365: Alias propagation strategy LGTM.Defaulting finalAlias and scope.alias to the model name preserves user-facing semantics while enabling mapped table names. Looks correct.
225-232: Good preservation of original field names via aliases.Expanding select-all and re-aliasing mapped columns back to original names is correct and consistent.
Also applies to: 448-451
packages/runtime/src/plugins/policy/expression-transformer.ts (2)
383-407: this-based member access and relation traversal LGTM.Receiver handling for this, forward/backward traversal over relations, and aliasing of nested selections look coherent.
Also applies to: 410-417, 430-463, 521-526
355-357: Call argument field resolution LGTM.Switching to eb.ref for field args is appropriate and avoids coupling to this-entity internals.
packages/runtime/src/plugins/policy/policy-handler.ts (3)
149-151: Pre-create policy should consider runtime defaults.You fill missing fields with nulls. If the runtime applies defaults before insert, policy should evaluate against the final values (after defaults). Otherwise legitimate creates can be rejected.
Would you like me to wire the default application into enforcePreCreatePolicyForOne so allValues reflect defaults?
Also applies to: 167-178
181-189: Derived-table policy check LGTM.Building a single-row SELECT over VALUES and applying the policy filter with COUNT(1) > 0 is sound. Column aliasing and outer alias to model name are consistent.
Also applies to: 191-209, 212-214
359-373: Policy combination logic LGTM.or(allows) and and(!denies) composition matches expected semantics.
|
Claude finished @ymc9's task —— View job Pull Request Review: Create Policy ImplementationI've completed a comprehensive review of this PR implementing "create" access policy functionality. Here's my detailed feedback: Review Tasks:
🎯 Implementation SummaryThis PR implements comprehensive support for "create" access policies in ZenStack, adding proper policy enforcement during entity creation operations. The implementation spans language validation, runtime policy handling, and comprehensive test coverage. ✅ Strengths1. Robust Policy Handler Implementation (
|
|
Claude finished @ymc9's task —— View job Pull Request Review: Create Policy ImplementationI've completed a comprehensive review of this PR implementing "create" access policy functionality. Here's my detailed feedback: Review Tasks:
🎯 Implementation SummaryThis PR implements comprehensive support for "create" access policies in ZenStack, adding proper policy enforcement during entity creation operations. The implementation spans language validation, runtime policy handling, and comprehensive test coverage. ✅ Strengths1. Robust Policy Handler Implementation (
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/runtime/src/plugins/policy/expression-transformer.ts (1)
254-256: Good: error message now references context.model (not this)This addresses the prior review note about incorrect model name in the error message.
🧹 Nitpick comments (6)
TODO.md (1)
86-89: Fix list indentation (markdownlint MD007).Indent child bullets by 4 spaces under "Misc" to satisfy MD007.
- - [ ] Global omit - - [ ] DbNull vs JsonNull - - [ ] Migrate to tsdown - - [ ] @default validation + - [ ] Global omit + - [ ] DbNull vs JsonNull + - [ ] Migrate to tsdown + - [ ] @default validationpackages/language/src/validators/attribute-application-validator.ts (2)
178-237: Non-owned relation detection is solid; minor refactor and a typo fix suggested.
- Extract owner-side check for readability and reuse.
- Fix comment typo “the the”.
- // check if the the node is a reference inside a collection predicate scope by auth access, + // check if the node is a reference inside a collection predicate scope by auth access, @@ - const relationAttr = field.attributes.find((attr) => attr.decl.ref?.name === '@relation'); - if (!relationAttr) { - // no "@relation", not owner side of the relation, match - return true; - } - - if (!relationAttr.args.some((arg) => arg.name === 'fields')) { - // no "fields" argument, can't be owner side of the relation, match - return true; - } - - return false; + // violation if the field is not owner-side + return !this.isOwnerSideRelationField(field);Add this helper within the class (outside the changed hunk):
private isOwnerSideRelationField(field: DataField): boolean { const relationAttr = field.attributes.find((a) => a.decl.ref?.name === '@relation'); return !!relationAttr && relationAttr.args.some((arg) => arg.name === 'fields'); }
157-157: Track plugin-registrable validation with an issue.Two TODOs note the same need. Consider opening/linking a tracking issue and referencing it here.
Also applies to: 239-239
packages/runtime/src/plugins/policy/expression-transformer.ts (3)
356-357: Qualify function-arg field refs with table/alias to avoid ambiguityUnqualified eb.ref(arg.field) can break under joins/derived tables. Qualify with context alias or model.
- return eb.ref(arg.field); + return eb.ref(`${context.alias ?? context.model}.${arg.field}`);
476-478: Use QueryError for invalid multi-member auth() accessKeep error typing consistent with other policy errors.
- if (expr.members.length !== 1) { - throw new Error(`Only single member access is supported`); - } + if (expr.members.length !== 1) { + throw new QueryError(`Only single member access is supported for auth()`); + }
83-88: Non-null assertion on authType is safe but unnecessary after invariantYou already throw if schema.authType is missing. Keeping ! is fine; consider returning this.schema.authType without ! if you prefer stricter typing via a local const after the check.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
TODO.md(1 hunks)packages/language/src/validators/attribute-application-validator.ts(3 hunks)packages/runtime/src/client/crud/operations/base.ts(4 hunks)packages/runtime/src/plugins/policy/expression-transformer.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/runtime/src/client/crud/operations/base.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/language/src/validators/attribute-application-validator.tspackages/runtime/src/plugins/policy/expression-transformer.ts
🧬 Code graph analysis (1)
packages/runtime/src/plugins/policy/expression-transformer.ts (5)
packages/runtime/src/client/errors.ts (1)
QueryError(13-17)packages/runtime/src/schema/expression.ts (1)
ExpressionUtils(16-120)packages/runtime/src/client/query-utils.ts (2)
getModel(16-18)requireField(37-55)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/runtime/src/plugins/policy/utils.ts (1)
conjunction(49-65)
🪛 markdownlint-cli2 (0.17.2)
TODO.md
86-86: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
87-87: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
88-88: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
89-89: Unordered list indentation
Expected: 4; Actual: 8
(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 (4)
packages/language/src/validators/attribute-application-validator.ts (2)
3-3: Imports look correct and are all used.Type-only and util imports align with later usage.
Also applies to: 11-11, 17-17, 25-27
171-176: Good: enforce non-owned relation restriction for create/all.Hooking the check only when an expression exists avoids unnecessary work.
packages/runtime/src/plugins/policy/expression-transformer.ts (2)
239-243: Confirm '^' semanticsBoth '!' and '^' map to count == 0. If '^' is intended to mean XOR vs "none," this is likely incorrect; if '^' is a synonym for "none," it's fine.
252-297: Confirm scalar-ID comparisons forauth()in policy DSL
Static analysis of the TS files didn’t reveal any instances ofauth().<idField> == someIdField(e.g.auth().id == userId), but your policy definitions aren’t in the repo code. Please manually verify that no scalar-ID comparisons againstauth()exist in your policies—or implement the optional fast-path for single-ID models as suggested.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests