fix(orm): properly handle literal array values in policy evaluation for postgres#628
Conversation
…or postgres fixes #595
|
Claude finished @ymc9's task —— View job Code Review Complete ✅I'll review this pull request that fixes issue #595 regarding PostgreSQL literal array value handling in policy evaluation. Review Todo List:
Code Review FeedbackUnderstanding the IssueThe issue was that literal array values in policy expressions (like ✅ Positive Aspects
🔍 Code Quality Issues
🐛 Potential BugIn return this.dialect
.buildArrayValue(value.map((v) => new ExpressionWrapper(this.transformValue(v, type))))
.toOperationNode();Issue: Passing the array's Fix: Consider element type detection: // Get element type for arrays
const elementType = type.endsWith('[]') ? type.slice(0, -2) : type;
return this.dialect
.buildArrayValue(value.map((v) => new ExpressionWrapper(this.transformValue(v, elementType))))
.toOperationNode();⚡ Performance Considerations
🔒 Security Assessment
🧪 Test Coverage
📋 Minor Improvements
🎯 Recommendations
Overall Assessment: ✅ APPROVE with Minor FixesThis is a solid fix that properly addresses the PostgreSQL array handling issue. The core architecture change (delegating to dialect-specific array building) is correct and well-implemented. The main concern is the type propagation bug for array elements, which should be addressed before merging. The test coverage is excellent and the changes maintain backward compatibility. Once the type issue is fixed, this PR will be ready to merge. |
📝 WalkthroughWalkthroughThis pull request refactors array handling across the ORM and schema systems to be dialect-aware and type-explicit. It replaces generic array builders with dialect-specific implementations, introduces explicit type parameters for array expressions, and updates three array-related functions ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The refactoring spans multiple interconnected systems (dialects, expressions, code generation) with logic density in array handling and dialect polymorphism. However, ~40 generated schema files follow a homogeneous pattern (adding type parameters), reducing per-file review burden. Core implementation files require careful scrutiny for SQL generation correctness and type mapping logic. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #595 where array-related functions (has, hasSome) in access policies fail on PostgreSQL. The root cause was that enum values and array expressions were being improperly converted to SQL, resulting in malformed array literals and unsupported expression errors.
Changes:
- Introduced a new
EnumExpressiontype to properly represent enum values in the schema expression system - Replaced direct PostgreSQL array operator usage with dialect-specific methods that correctly construct array values
- Added proper handling for enum type and value name mapping in the query name mapper
Reviewed changes
Copilot reviewed 32 out of 34 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/schema/src/expression.ts | Added new EnumExpression type to represent enum values |
| packages/schema/src/expression-utils.ts | Added enum() helper function and isEnum() type guard |
| packages/sdk/src/ts-schema-generator.ts | Changed to generate enum expressions instead of string literals for enum field references |
| packages/plugins/policy/src/expression-transformer.ts | Added handlers for enum and array expressions, simplified call argument transformation |
| packages/plugins/policy/src/policy-handler.ts | Updated to use new buildArrayValue method |
| packages/plugins/policy/src/expression-evaluator.ts | Added evaluation support for enum expressions |
| packages/orm/src/client/functions.ts | Refactored has, hasEvery, hasSome to use dialect methods |
| packages/orm/src/client/crud/dialects/*.ts | Implemented array operation methods for each dialect |
| packages/orm/src/client/executor/name-mapper.ts | Added enum type and value mapping support for CAST nodes |
| packages/orm/src/client/helpers/schema-db-pusher.ts | Updated to use mapped enum names |
| tests/regression/test/issue-595.test.ts | Comprehensive regression test covering the reported issue |
| Multiple schema files | Regenerated to use ExpressionUtils.enum() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Claude finished @ymc9's task —— View job Claude Code Review Complete ✅ Review Todo List:
Code Review Feedback✅ Issue UnderstandingThis PR successfully fixes PostgreSQL array literal handling in policy evaluation. The issue was that enum arrays in expressions like ✅ Architecture & Design QualityExcellent Design Decisions:
🔍 Code Quality AssessmentStrengths:
Minor Issues Identified:
⚡ Performance ImplicationsPositive:
Minor Concerns:
🔒 Security Assessment✅ No security concerns identified
🧪 Test Coverage✅ Excellent test coverage
💡 RecommendationsMust Fix:
Should Consider:
Nice to Have:
📋 Dialect Implementation Review
🎯 Overall Assessment: ✅ APPROVE with Minor FixThis is a solid, well-architected fix that properly addresses the PostgreSQL array handling issue. The core design of delegating to dialect-specific array building is excellent and maintains the abstraction boundaries properly. The main concern is the type propagation bug for array elements, which should be addressed before merging. Once fixed, this PR significantly improves PostgreSQL enum array handling while maintaining compatibility across all supported databases. Impact: This fix enables proper policy evaluation with enum arrays on PostgreSQL, resolving a critical functional gap in the ORM's authorization system. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/plugins/policy/src/expression-transformer.ts (1)
165-186: Remove@ts-expect-errorfrom production handlers.These handler signatures can be typed explicitly to avoid
@ts-expect-error. This keeps the policy transformer type-safe and aligned with repo standards.♻️ Suggested refactor
-import { - ExpressionUtils, - type ArrayExpression, - type CallExpression, - type Expression, - type FieldExpression, - type SchemaDef, -} from '@zenstackhq/orm/schema'; +import { + ExpressionUtils, + type ArrayExpression, + type CallExpression, + type EnumExpression, + type Expression, + type FieldExpression, + type SchemaDef, +} from '@zenstackhq/orm/schema'; `@expr`('literal') -// `@ts-expect-error` -private _literal(expr: LiteralExpression) { +private _literal(expr: LiteralExpression, _context: ExpressionTransformerContext): OperationNode { return this.transformValue( expr.value, typeof expr.value === 'string' ? 'String' : typeof expr.value === 'boolean' ? 'Boolean' : 'Int', ); } `@expr`('enum') -// `@ts-expect-error` -private _enum(expr: { type: string; value: string }) { +private _enum(expr: EnumExpression, _context: ExpressionTransformerContext): OperationNode { return this.dialect.castEnum(this.eb.val(expr.value), expr.type).toOperationNode(); } `@expr`('array') -// `@ts-expect-error` -private _array(expr: ArrayExpression, context: ExpressionTransformerContext) { +private _array(expr: ArrayExpression, context: ExpressionTransformerContext): OperationNode { return this.dialect .buildArrayValue(expr.items.map((item) => new ExpressionWrapper(this.transform(item, context)))) .toOperationNode(); }Based on learnings: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
packages/orm/src/client/executor/name-mapper.ts (1)
263-300: PropagatequeryIdin the non-table update branch.
The early return drops per-query context wheninnerTableisn’t aTableNode.🔧 Proposed fix
- if (!innerTable || !TableNode.is(innerTable)) { - return super.transformUpdateQuery(node); - } + if (!innerTable || !TableNode.is(innerTable)) { + return super.transformUpdateQuery(node, queryId); + }
🤖 Fix all issues with AI agents
In `@packages/orm/src/client/crud/dialects/mysql.ts`:
- Around line 231-233: Replace the plain Error thrown in buildArrayValue with
the standardized helper by throwing createNotSupportedError instead; locate the
override buildArrayValue(_values: Expression<unknown>[]):
AliasableExpression<number> in the MySQL dialect and change the throw new
Error('MySQL does not support array value') to throw
createNotSupportedError('array values') (or the project's existing not-supported
message) so it matches other not-supported array helpers and centralizes error
handling.
In `@packages/orm/src/client/crud/dialects/sqlite.ts`:
- Around line 450-452: Replace the generic thrown Error in the override
buildArrayValue method with a call to createNotSupportedError so the "array
values not supported" case uses the same not-supported error type as other array
helpers; locate the buildArrayValue method and change the throw new
Error('SQLite does not support array values') to return or throw (matching
surrounding helpers) createNotSupportedError('array values') or the same message
used elsewhere to preserve consistent error typing.
In `@packages/orm/src/client/executor/name-mapper.ts`:
- Around line 332-367: In transformCast (name-mapper.ts) replace the
unconditional invariant(parameters.length === 1) with a guard that only enters
the enum-mapping branch when node.dataType.parameters.length === 1 and the other
checks (RawNode.is, ReferenceNode.is, ColumnNode.is) succeed; i.e., wrap the
existing enum-detection/mapping logic in an if that checks parameters.length ===
1 (and keep existing ReferenceNode/ColumnNode checks) so RAW types like
varchar(255) or decimal(10,2) don't throw, while preserving behavior of
mapEnumTypeName, getEnum and subsequent ValueNode / CastNode handling.
In `@packages/sdk/src/ts-schema-generator.ts`:
- Line 38: Remove the unused type import "ResolvedType" from the import list in
ts-schema-generator.ts; find the import statement that includes "type
ResolvedType" and delete that symbol so the file no longer imports ResolvedType
(leave other imported symbols intact).
|
Claude finished @ymc9's task —— View job Claude Code Review Complete ✅ Code Review FeedbackUnderstanding the IssueThe issue was that literal array values in policy expressions (like ✅ Positive Aspects
🔍 Code Quality Issues
🐛 Potential BugIn return this.dialect
.buildArrayValue(value.map((v) => new ExpressionWrapper(this.transformValue(v, type))))
.toOperationNode();Issue: Passing the array's Fix: Consider element type detection: // Get element type for arrays
const elementType = type.endsWith('[]') ? type.slice(0, -2) : type;
return this.dialect
.buildArrayValue(value.map((v) => new ExpressionWrapper(this.transformValue(v, elementType))))
.toOperationNode();⚡ Performance Considerations
🔒 Security Assessment
🧪 Test Coverage
📋 Minor Improvements
🎯 Recommendations
Overall Assessment: ✅ APPROVE with Minor FixesThis is a solid fix that properly addresses the PostgreSQL array handling issue. The core architecture change (delegating to dialect-specific array building) is correct and well-implemented. The main concern is the type propagation bug for array elements, which should be addressed before merging. The test coverage is excellent and the changes maintain backward compatibility. Once the type issue is fixed, this PR will be ready to merge. |
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/orm/src/client/executor/name-mapper.ts (1)
268-271: MissingqueryIdargument in early return.Line 270 calls
super.transformUpdateQuery(node)without passingqueryId, while line 265 correctly passes it. This breaks the per-query context threading for update queries with non-table nodes.Proposed fix
const { alias, node: innerTable } = stripAlias(node.table); if (!innerTable || !TableNode.is(innerTable)) { - return super.transformUpdateQuery(node); + return super.transformUpdateQuery(node, queryId); }
🧹 Nitpick comments (1)
packages/orm/src/client/executor/name-mapper.ts (1)
343-344: Remove unnecessary non-null assertion.The
!assertion ongetEnum(...)is misleading since the very next line checksif (enumDef). Removing it makes the code intent clearer.Suggested change
- const enumDef = getEnum(this.schema, castTypeName)!; - if (enumDef) { + const enumDef = getEnum(this.schema, castTypeName); + if (enumDef) {
|
Claude finished @ymc9's task —— View job Claude Code Review Complete ✅ Review Todo List:
Code Review Feedback✅ Issue UnderstandingThis PR successfully fixes PostgreSQL array literal handling in policy evaluation. The issue was that enum arrays in expressions like ✅ Architecture & Design QualityExcellent Design Decisions:
🔍 Code Quality AssessmentStrengths:
Minor Issues Identified:
⚡ Performance ImplicationsPositive:
Minor Concerns:
🔒 Security Assessment✅ No security concerns identified
🧪 Test Coverage✅ Excellent test coverage
💡 RecommendationsMust Fix:
Should Consider:
Nice to Have:
📋 Dialect Implementation Review
🎯 Overall Assessment: ✅ APPROVE with Minor FixThis is a solid, well-architected fix that properly addresses the PostgreSQL array handling issue. The core design of delegating to dialect-specific array building is excellent and maintains the abstraction boundaries properly. The main concern is the type propagation bug for array elements, which should be addressed before merging. Once fixed, this PR significantly improves PostgreSQL enum array handling while maintaining compatibility across all supported databases. Impact: This fix enables proper policy evaluation with enum arrays on PostgreSQL, resolving a critical functional gap in the ORM's authorization system. |
|
Claude finished @ymc9's task —— View job Claude Code Review Complete ✅ Review Todo List:
Code Review Feedback✅ Issue UnderstandingThis PR successfully fixes PostgreSQL array literal handling in policy evaluation. The issue was that enum arrays in expressions like ✅ Architecture & Design QualityExcellent Design Decisions:
🔍 Code Quality AssessmentStrengths:
Minor Issues Identified:
⚡ Performance ImplicationsPositive:
Minor Concerns:
🔒 Security Assessment✅ No security concerns identified
🧪 Test Coverage✅ Excellent test coverage
💡 RecommendationsMust Fix:
Should Consider:
Nice to Have:
📋 Dialect Implementation Review
🎯 Overall Assessment: ✅ APPROVE with Minor FixThis is a solid, well-architected fix that properly addresses the PostgreSQL array handling issue. The core design of delegating to dialect-specific array building is excellent and maintains the abstraction boundaries properly. The main concern is the type propagation bug for array elements, which should be addressed before merging. Once fixed, this PR significantly improves PostgreSQL enum array handling while maintaining compatibility across all supported databases. Impact: This fix enables proper policy evaluation with enum arrays on PostgreSQL, resolving a critical functional gap in the ORM's authorization system. |
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 (2)
packages/orm/src/client/executor/name-mapper.ts (1)
261-269: PropagatequeryIdin the non-TableNode update path.Line 268 drops
queryIdwhen returning from the non-TableNode branch, inconsistent with all other early returns in this method (lines 263, 275) and the pattern across other transform overrides (transformSelectQuery, transformInsertQuery, transformDeleteQuery, etc.). This loss of context can break per-query mapping.🛠️ Suggested fix
- return super.transformUpdateQuery(node); + return super.transformUpdateQuery(node, queryId);packages/plugins/policy/src/expression-transformer.ts (1)
550-569: MySQL dialect returns Expression objects fromtransformInputthat require special handling.MySQL's
transformInputmethod returns Expression objects (viathis.eb.cast()) when handlingJsonNullClassinstances and JSON type transformations (lines 72 and 112). The current code at lines 564-569 treats these as simple primitives and wraps them withValueNode.createImmediate(), which serializes the object instead of converting it to an operation node.Detect expression objects by checking for the
toOperationNodemethod and return their operation node directly:🛠️ Suggested fix
- if (typeof transformed !== 'string') { - // simple non-string primitives can be immediate values - return ValueNode.createImmediate(transformed); - } else { - return ValueNode.create(transformed); - } + if (typeof transformed === 'string') { + return ValueNode.create(transformed); + } + if (transformed && typeof transformed === 'object' && 'toOperationNode' in transformed) { + return (transformed as Expression<unknown>).toOperationNode(); + } + // simple non-string primitives can be immediate values + return ValueNode.createImmediate(transformed);
fixes #595
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.