feat(policy): support read filtering for update with "from" and delete with "using" #253
Conversation
WalkthroughAdds guarded, cached resolved-type checks to binary expression validation; consolidates and applies read-policy filters across FROMs and JOINs (including UPDATE/DELETE joins) via new compile/createPolicyFilter helpers and a transformJoin override; switches several ID-field helpers to requireIdFields; adds tests for relation _count under read policies. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 Code Review for PR #253Review Tasks
Code Quality and Best Practices ✅Strengths:
Areas for improvement:
Implementation Analysis ✅Core Changes:
Technical correctness:
Security Assessment ✅Security strengths:
Security considerations:
Performance Considerations ✅Performance impact:
Test Coverage ✅Test quality:
Test scenarios covered:
Potential Issues 🔍Minor concerns:
Recommendations
Overall Assessment ⭐This is a high-quality implementation that:
The code is ready to merge with only minor documentation improvements suggested. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for read filtering policies in SQL update operations that use "from" clauses and delete operations that use "using" clauses. The feature ensures that when update/delete queries reference additional tables through joins, read policies for those referenced tables are properly enforced.
- Extends policy filtering to handle joined tables in update and delete operations
- Adds comprehensive test coverage for the new functionality including query builder scenarios
- Fixes a type safety issue in expression validation for optional resolved types
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/runtime/src/plugins/policy/policy-handler.ts | Core implementation adding policy filtering for joins in update/delete operations |
| packages/runtime/test/policy/crud/read.test.ts | Comprehensive test suite covering new policy filtering scenarios |
| packages/language/src/validators/expression-validator.ts | Type safety improvements for optional resolved types |
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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/language/src/validators/expression-validator.ts(2 hunks)packages/runtime/src/plugins/policy/policy-handler.ts(7 hunks)packages/runtime/test/policy/crud/read.test.ts(1 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/language/src/validators/expression-validator.tspackages/runtime/test/policy/crud/read.test.tspackages/runtime/src/plugins/policy/policy-handler.ts
🧬 Code graph analysis (2)
packages/runtime/test/policy/crud/read.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/src/plugins/policy/policy-handler.ts (1)
packages/runtime/src/plugins/policy/utils.ts (1)
conjunction(49-67)
⏰ 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)
- GitHub Check: build-test (20.x)
🔇 Additional comments (17)
packages/language/src/validators/expression-validator.ts (2)
112-115: Good guard against unresolved types for left operand.Prevents false positives when type inference fails.
122-125: Good guard against unresolved types for right operand.Consistent with the left‑side check.
packages/runtime/test/policy/crud/read.test.ts (9)
248-283: Nice: relation _count respects child read policy.Covers the to‑many counting path accurately.
285-308: Count API coverage looks good.Validates both
_alland field‑level counts under policy.
310-343: Aggregate honors read policy.Solid coverage for count/sum/avg/min/max.
345-393: GroupBy under policy: good expectations.Verifies grouped aggregates filter correctly.
396-430: Query builder simple selects: good baseline checks.Exercises aliasing and nested subquery paths.
432-492: Join path is well covered.Ensures joined reads respect both sides’ policies.
494-537: Implicit cross join constrained to PostgreSQL – good call.Prevents dialect variance; case looks correct.
539-580: UPDATE … FROM: behavior validated.Confirms join‑side read policy gates updates.
582-613: DELETE … USING: behavior validated.Confirms join‑side read policy gates deletes.
packages/runtime/src/plugins/policy/policy-handler.ts (6)
11-11: JoinNode import OK.Required for the new join transformation.
142-151: Select WHERE merge is correct.Transforming WHERE first and then merging per‑FROM policy filter avoids double‑processing.
229-246: UPDATE with FROM: correctly merges join-side filters.Ensures only policy-visible joined rows can drive updates.
248-265: DELETE with USING: correctly merges join-side filters.Symmetric to update path; looks good.
554-572: FROM/tables policy filter helpers look right.Alias extraction + conjunction is clean.
573-588: compilePolicyCondition refactor looks good.Clearer naming; parameters passed appropriately.
|
Claude finished @ymc9's task —— View job Code Review for PR #253Review Tasks
Code Quality and Best Practices ✅Strengths:
Areas for improvement:
Implementation Analysis ✅Core Changes:
Technical correctness:
Security Assessment ✅Security strengths:
Security considerations:
Performance Considerations ✅Performance impact:
Test Coverage ✅Test quality:
Test scenarios covered:
Potential Issues 🔍Minor concerns:
Recommendations
Overall Assessment ⭐This is a high-quality implementation that:
The code is ready to merge with only minor documentation improvements suggested. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/language/src/validators/expression-validator.ts (1)
134-139: Bug: DateTime incompatibility still triggers when the other side is 'Any'/unresolved.This can reintroduce false errors; only enforce the DateTime-only rule when both sides are resolved scalars and neither is 'Any'.
Apply this diff:
- // DateTime comparison is only allowed between two DateTime values - if (leftResolvedDecl === 'DateTime' && rightResolvedDecl && rightResolvedDecl !== 'DateTime') { - accept('error', 'incompatible operand types', { - node: expr, - }); - } else if (rightResolvedDecl === 'DateTime' && leftResolvedDecl && leftResolvedDecl !== 'DateTime') { - accept('error', 'incompatible operand types', { - node: expr, - }); - } + // DateTime comparison is only enforced when both sides are resolved scalars (excluding 'Any') + const leftScalar = typeof leftResolvedDecl === 'string' ? leftResolvedDecl : undefined; + const rightScalar = typeof rightResolvedDecl === 'string' ? rightResolvedDecl : undefined; + if (leftScalar && rightScalar && leftScalar !== 'Any' && rightScalar !== 'Any') { + if ((leftScalar === 'DateTime') !== (rightScalar === 'DateTime')) { + accept('error', 'incompatible operand types', { node: expr }); + } + }
🧹 Nitpick comments (1)
packages/language/src/validators/expression-validator.ts (1)
115-116: Minor readability DRY-up for symmetric operand checks.The two symmetric “invalid operand type” checks are correct. Consider extracting a tiny helper or a local predicate to reduce duplication and make intent clearer.
Apply if you like:
- if ( - leftResolvedDecl && - (typeof leftResolvedDecl !== 'string' || !supportedShapes.includes(leftResolvedDecl)) - ) { - accept('error', `invalid operand type for "${expr.operator}" operator`, { - node: expr.left, - }); - return; - } - if ( - rightResolvedDecl && - (typeof rightResolvedDecl !== 'string' || !supportedShapes.includes(rightResolvedDecl)) - ) { - accept('error', `invalid operand type for "${expr.operator}" operator`, { - node: expr.right, - }); - return; - } + const invalidNode = + leftResolvedDecl && + (typeof leftResolvedDecl !== 'string' || !supportedShapes.includes(leftResolvedDecl)) + ? expr.left + : rightResolvedDecl && + (typeof rightResolvedDecl !== 'string' || !supportedShapes.includes(rightResolvedDecl)) + ? expr.right + : undefined; + if (invalidNode) { + accept('error', `invalid operand type for "${expr.operator}" operator`, { node: invalidNode }); + return; + }Also applies to: 124-125
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/language/src/validators/expression-validator.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/language/src/validators/expression-validator.ts
🔇 Additional comments (1)
packages/language/src/validators/expression-validator.ts (1)
111-113: LGTM: caching resolved decls reduces repeated lookups.The introduction of leftResolvedDecl/rightResolvedDecl is a good cleanup and avoids repeated property access.
|
Claude finished @ymc9's task —— View job Code Review for PR #253Review Tasks
Code Quality and Best Practices ✅Strengths:
Areas for improvement:
Implementation Analysis ✅Core Changes:
Technical correctness:
Security Assessment ✅Security strengths:
Security considerations:
Performance Considerations ✅Performance impact:
Test Coverage ✅Test quality:
Test scenarios covered:
Potential Issues 🔍Minor concerns:
Recommendations
Overall Assessment ⭐This is a high-quality implementation that:
The code is ready to merge with only minor documentation improvements suggested. |
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/plugins/policy/policy-handler.ts (1)
451-467: Also require ID fields when building read-back conditions.
mapon possibly-undefinedidFieldswill crash.- const idFields = getIdFields(this.client.$schema, table); + const idFields = requireIdFields(this.client.$schema, table);
♻️ Duplicate comments (1)
packages/runtime/src/plugins/policy/policy-handler.ts (1)
163-182: Join: nested subquery with alias-propagated policy — resolves prior alias issue.You now pass the extracted alias into
buildPolicyFilter, fixing the earlier predicate qualification bug.Optional micro‑opt: avoid rewriting the join when the filter is constant TRUE.
- const filter = this.buildPolicyFilter(table.model, table.alias, 'read'); + const filter = this.buildPolicyFilter(table.model, table.alias, 'read'); + // If always-true, keep original join to reduce nesting + if (isTrueNode?.(filter)) { + return super.transformJoin(node); + }(Import
isTrueNodefrom./utilsif available.)
🧹 Nitpick comments (2)
packages/runtime/src/plugins/policy/policy-handler.ts (2)
142-151: Select: policy filter merged for all FROM tables — consider skipping no-op TRUE.Works as intended. Micro‑opt: if the combined filter reduces to a constant TRUE, skip creating a
WHEREto avoidWHERE (true).If
isTrueNodeis available:- if (policyFilter) { + if (policyFilter && !isTrueNode(policyFilter)) { whereNode = WhereNode.create( whereNode?.where ? conjunction(this.dialect, [whereNode.where, policyFilter]) : policyFilter, ); }
61-109: Non-transactional pre/post checks can cause partial effects.When
mutationRequiresTransactionis true, pre-check, mutation, and post-check still run outside a transaction; if post-check fails, the mutation has already committed. Track as known debt or gate behind a flag.I can sketch a transaction-wrapped flow compatible with your
ProceedKyselyQueryFunctionif desired.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/runtime/src/client/query-utils.ts(1 hunks)packages/runtime/src/plugins/policy/policy-handler.ts(14 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/src/client/query-utils.tspackages/runtime/src/plugins/policy/policy-handler.ts
🧬 Code graph analysis (1)
packages/runtime/src/plugins/policy/policy-handler.ts (5)
packages/runtime/src/plugins/policy/utils.ts (2)
conjunction(49-67)getTableName(144-156)packages/runtime/src/client/query-utils.ts (1)
getIdFields(57-60)packages/runtime/src/plugins/policy/column-collector.ts (1)
ColumnCollector(7-21)packages/runtime/src/client/plugin.ts (1)
ProceedKyselyQueryFunction(189-189)packages/runtime/src/client/errors.ts (1)
QueryError(13-17)
⏰ 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 (9)
packages/runtime/src/plugins/policy/policy-handler.ts (9)
11-11: Imports updated correctly for join handling and errors.
JoinNodeandQueryErroradditions look right.Also applies to: 35-35
232-241: Update: merges join tables’ read-policy into WHERE — good.Policy for
UPDATE ... FROMis conjoined with the base update policy; ordering is fine.
251-260: Delete: merges USING tables’ read-policy into WHERE — good.Correctly conjoins join filters for
DELETE ... USING.
285-289: Pre-create policy path updates look good.Aliases and value unwrapping changes are consistent.
436-444: Read-back selection uses TableNode — good.Using
TableNode.create(mutationModel)is clearer; policy filter applied separately.
471-488: Mutation model extraction: better errors; confirm DELETE multi-using stance.Throwing
QueryErroris appropriate. Please confirm that restricting delete’sfrom.fromsto exactly one is intended and documented. Postgres allows multipleUSINGtables, which you handle vianode.using, so the constraint is likely safe.
542-560: Table/alias extraction looks correct.Identifier alias handling and recursive unwrap are fine.
562-568: FROM/USING policy aggregation helper — good.Conjoining per-table filters with extracted aliases is the right approach. Skips subqueries cleanly by returning
undefined.Also applies to: 569-580
581-596: compilePolicyCondition via ExpressionTransformer — good.Centralizing condition compilation here simplifies callers.
|
Claude finished @ymc9's task —— View job Code Review for PR #253Review Tasks
Code Quality and Best Practices ✅Strengths:
Areas for improvement:
Implementation Analysis ✅Core Changes:
Technical correctness:
Security Assessment ✅Security strengths:
Security considerations:
Performance Considerations ✅Performance impact:
Test Coverage ✅Test quality:
Test scenarios covered:
Potential Issues 🔍Minor concerns:
Recommendations
Overall Assessment ⭐This is a high-quality implementation that:
The code is ready to merge with only minor documentation improvements suggested. |
There was a problem hiding this comment.
Actionable comments posted: 0
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/plugins/policy/expression-transformer.ts (1)
199-214: Fix relation normalization to use the related model's id fieldWhen normalizing relation-field comparisons the code currently calls requireIdFields(this.schema, context.model) (parent model). It must read ID fields from the relation field's target model (fieldDef.type) instead.
File: packages/runtime/src/plugins/policy/expression-transformer.ts
Lines: 199-214- // if relation fields are used directly in comparison, it can only be compared with null, - // so we normalize the args with the id field (use the first id field if multiple) + // if relation fields are used directly in comparison, it can only be compared with null, + // so we normalize the args with the related model's id field (use the first id field if multiple) let normalizedLeft: Expression = expr.left; if (this.isRelationField(expr.left, context.model)) { invariant(ExpressionUtils.isNull(expr.right), 'only null comparison is supported for relation field'); - const idFields = requireIdFields(this.schema, context.model); - normalizedLeft = this.makeOrAppendMember(normalizedLeft, idFields[0]!); + const leftFieldDef = this.getFieldDefFromFieldRef(expr.left, context.model)!; + const leftIdFields = requireIdFields(this.schema, leftFieldDef.type); + normalizedLeft = this.makeOrAppendMember(normalizedLeft, leftIdFields[0]!); } let normalizedRight: Expression = expr.right; if (this.isRelationField(expr.right, context.model)) { invariant(ExpressionUtils.isNull(expr.left), 'only null comparison is supported for relation field'); - const idFields = requireIdFields(this.schema, context.model); - normalizedRight = this.makeOrAppendMember(normalizedRight, idFields[0]!); + const rightFieldDef = this.getFieldDefFromFieldRef(expr.right, context.model)!; + const rightIdFields = requireIdFields(this.schema, rightFieldDef.type); + normalizedRight = this.makeOrAppendMember(normalizedRight, rightIdFields[0]!); }Add/extend a test where parent model’s ID name differs from the related model’s (e.g., parent uses
uuid, related usesid) and assertrelation == nullcompiles/works correctly.
♻️ Duplicate comments (4)
packages/runtime/src/client/query-utils.ts (2)
233-236: Switch to requireIdFields for default order: good tighteningUsing
requireIdFieldshere prevents silent runtime surprises when IDs are missing.
320-323: extractIdFields hardened with requireIdFields: goodThis avoids passing
undefinedtoextractFields.packages/runtime/src/plugins/policy/policy-handler.ts (2)
219-221: Returning IDs now requires ID fields — resolvedSwitching to
requireIdFieldsaddresses the prior undefined hazard.
276-281: onlyReturningId: guard via requireIdFields — resolvedPrevents false positives when IDs are missing; good.
🧹 Nitpick comments (6)
packages/runtime/src/client/crud/dialects/postgresql.ts (1)
20-22: m2m join filter now uses requireIdFields: correct and saferThe invariants ensure single-ID per side before dereferencing
[0].Consider extracting the repeated m2m join-filter building into a shared helper in
BaseCrudDialectto reduce duplication across dialects.Also applies to: 160-173
packages/runtime/src/client/crud/dialects/sqlite.ts (1)
20-22: m2m join filter hardened with requireIdFields: LGTMMatches the Postgres path; invariants gate single-ID assumption.
Same helper extraction suggestion as Postgres to keep the dialects in sync.
Also applies to: 216-229
packages/runtime/src/client/crud/dialects/base-dialect.ts (2)
369-374: Many‑to‑many: enforce single‑column IDs — consider clearer error type/messageThe new invariants are good. However,
invariant(...)collapses messages in production, which hurts DX. Prefer throwing a typed error (InternalError/QueryError) with model/relation context so users see actionable messages in prod.Example:
- invariant(modelIdFields.length === 1, 'many-to-many relation must have exactly one id field'); + if (modelIdFields.length !== 1) { + throw new InternalError(`Many-to-many relation "${model}.${field}" requires exactly one ID field on "${model}".`); + } - invariant(relationIdFields.length === 1, 'many-to-many relation must have exactly one id field'); + if (relationIdFields.length !== 1) { + throw new InternalError(`Many-to-many relation "${model}.${field}" requires exactly one ID field on "${relationModel}".`); + }
375-386: M2M join condition: prefer EXISTS over IN for planner friendliness
IN (SELECT ...)can be less optimal for large join tables.EXISTSoften produces better plans and avoids large intermediate lists.High‑level shape:
- WHERE EXISTS (
SELECT 1 FROM joinTable jt
WHERE jt.otherFk = relation.id AND jt.parentFk = model.id
)If you want, I can draft a Kysely‑fluent version aligned with your EB helpers.
packages/runtime/src/plugins/policy/policy-handler.ts (2)
163-182: Preserve ON‑clause transformations in join overrideThis override replaces the join target but doesn’t transform
node.on, so any downstream transformer logic won’t run on the ON tree. Transform it explicitly; avoid callingsuper.transformJointo prevent re‑applying filters to the nested subquery.Apply:
protected override transformJoin(node: JoinNode) { const table = this.extractTableName(node.table); if (!table) { return super.transformJoin(node); } const filter = this.buildPolicyFilter(table.model, table.alias, 'read'); const nestedSelect: SelectQueryNode = { kind: 'SelectQueryNode', from: FromNode.create([node.table]), selections: [SelectionNode.createSelectAll()], where: WhereNode.create(filter), }; - return { - ...node, - table: AliasNode.create(ParensNode.create(nestedSelect), IdentifierNode.create(table.alias ?? table.model)), - }; + return { + ...node, + on: node.on ? (this.transformNode(node.on) as typeof node.on) : undefined, + table: AliasNode.create( + ParensNode.create(nestedSelect), + IdentifierNode.create(table.alias ?? table.model), + ), + }; }
542-554: Alias extraction: consider centralizing and expanding casesYou’re re‑implementing table/alias extraction. Consider extending a shared helper to also cover
ReferenceNode/TableExpressionvariants where applicable, so future join forms are handled uniformly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/runtime/src/client/crud/dialects/base-dialect.ts(3 hunks)packages/runtime/src/client/crud/dialects/postgresql.ts(2 hunks)packages/runtime/src/client/crud/dialects/sqlite.ts(2 hunks)packages/runtime/src/client/crud/operations/base.ts(10 hunks)packages/runtime/src/client/query-utils.ts(3 hunks)packages/runtime/src/plugins/policy/expression-transformer.ts(2 hunks)packages/runtime/src/plugins/policy/policy-handler.ts(15 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/src/client/crud/dialects/sqlite.tspackages/runtime/src/client/crud/dialects/postgresql.tspackages/runtime/src/plugins/policy/expression-transformer.tspackages/runtime/src/client/query-utils.tspackages/runtime/src/client/crud/dialects/base-dialect.tspackages/runtime/src/client/crud/operations/base.tspackages/runtime/src/plugins/policy/policy-handler.ts
🧬 Code graph analysis (7)
packages/runtime/src/client/crud/dialects/sqlite.ts (1)
packages/runtime/src/client/query-utils.ts (1)
requireIdFields(62-69)
packages/runtime/src/client/crud/dialects/postgresql.ts (1)
packages/runtime/src/client/query-utils.ts (1)
requireIdFields(62-69)
packages/runtime/src/plugins/policy/expression-transformer.ts (3)
packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/runtime/src/schema/expression.ts (1)
ExpressionUtils(16-120)packages/runtime/src/client/query-utils.ts (1)
requireIdFields(62-69)
packages/runtime/src/client/query-utils.ts (2)
packages/runtime/src/client/crud/operations/base.ts (2)
getModel(118-120)schema(91-93)samples/blog/zenstack/schema.ts (1)
schema(9-230)
packages/runtime/src/client/crud/dialects/base-dialect.ts (2)
packages/runtime/src/client/query-utils.ts (1)
requireIdFields(62-69)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
packages/runtime/src/client/crud/operations/base.ts (1)
packages/runtime/src/client/query-utils.ts (1)
requireIdFields(62-69)
packages/runtime/src/plugins/policy/policy-handler.ts (6)
packages/runtime/src/plugins/policy/utils.ts (2)
conjunction(49-67)getTableName(144-156)packages/runtime/src/client/query-utils.ts (1)
requireIdFields(62-69)packages/runtime/src/plugins/policy/column-collector.ts (1)
ColumnCollector(7-21)packages/runtime/src/client/plugin.ts (1)
ProceedKyselyQueryFunction(189-189)packages/sdk/src/schema/schema.ts (1)
GetModels(108-108)packages/runtime/src/client/errors.ts (1)
QueryError(13-17)
⏰ 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 (12)
packages/runtime/src/plugins/policy/expression-transformer.ts (1)
28-29: Import switch to requireIdFields aligns with stricter runtime guaranteesGood consistency with the broader migration.
packages/runtime/src/client/crud/operations/base.ts (1)
42-44: Consistent migration to requireIdFields across CRUD paths — LGTM
- No unsafe chained uses or extractFields(..., getIdFields(...)) found; only remaining call is getIdValues in packages/runtime/src/client/query-utils.ts (around lines 166–168) and it already throws when idFields is missing.
packages/runtime/src/client/query-utils.ts (1)
58-60: getIdFields stays “soft”; callers migrated to requireIdFields — verifiedOnly remaining usage is getIdValues (packages/runtime/src/client/query-utils.ts:166–168) which explicitly checks for missing idFields and throws; all other hot paths call requireIdFields.
packages/runtime/src/client/crud/dialects/base-dialect.ts (1)
1019-1026: Delegate join now uses required ID fields — LGTMSwitch to
requireIdFieldsremoves undefined cases; loop join for composite keys remains correct.packages/runtime/src/plugins/policy/policy-handler.ts (8)
142-151: Select: merge FROM‑table policy filters into WHERE — LGTMCombining transformed WHERE with aggregated FROM filters is correct and preserves original predicates.
191-203: Upsert: updateWhere now respects alias‑scoped update policy — LGTM
232-241: Update WITH FROM: correctly conjoins join tables’ read filters — LGTMThis ensures updates only touch rows join‑readable under policy.
251-260: Delete WITH USING: correctly conjoins join tables’ read filters — LGTMMirrors the UPDATE path; looks good.
436-444: Read‑back select: ID disjunction built with required IDs — LGTMPolicy is applied post‑mutation; ID predicate construction is correct.
Also applies to: 451-452
471-488: getMutationModel: explicit errors and alias propagation — LGTMClear errors for malformed UPDATE/DELETE; alias carry‑through is important for policy compilation.
562-568: FROM/USING aggregation helpers — LGTMReducing to a conjunction of per‑table read filters is clean and composable.
Also applies to: 569-579
512-519: Policy condition compilation path — LGTMCentralizing via
compilePolicyConditionaligns with the new ExpressionTransformer flow and keeps alias/model context intact.Also applies to: 581-596
Summary by CodeRabbit
New Features
Bug Fixes
Tests