fix(orm): fix PostgreSQL type mismatch when @db.Uuid fields used in policy expressions#2532
fix(orm): fix PostgreSQL type mismatch when @db.Uuid fields used in policy expressions#2532
Conversation
…olicy expressions (#2394) Two related bugs where `@db.Uuid` fields caused "operator does not exist: uuid = text" errors: 1. `before()` in post-update policies: VALUES table was casting UUID ID fields to `text` instead of `uuid`, breaking the `$before` JOIN condition. 2. Policy expressions comparing a `@db.Uuid` column against a plain `String` field or an `auth()` value (which always arrives as a text parameter). Fix (1): `getSqlType` in the PostgreSQL dialect now checks `@db.*` field attributes first, returning the exact native type (e.g. `uuid`) rather than the generic ZModel base type (e.g. `text`). Fix (2): A new `buildComparison` dialect hook lets the PostgreSQL dialect detect when one operand carries a `@db.*` native type override and the other doesn't, and casts the native-typed side back to its base ZModel SQL type so the comparison types align. The policy plugin calls this hook instead of building the `BinaryOperationNode` directly, keeping all database-specific logic in the dialect layer. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job PR Review Complete ✅Reviewing PR #2532: PostgreSQL UUID type mismatch fix Code Quality & Best Practices ⭐Excellent work overall! The implementation follows clean coding principles:
Technical Implementation 🔧PostgreSQL Dialect (
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBase dialect gained a concrete comparison builder; PostgreSQL dialect maps Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/orm/src/client/crud/dialects/postgresql.ts`:
- Around line 455-474: The current buildComparison only checks presence of `@db`.*
attributes; instead resolve the actual SQL type names for left and right
(extract the `@db`.* attribute value when present, e.g.
attribute.name.slice('@db.'.length), and fallback to
this.getSqlType(fieldDef.type) for base types) and compare those resolved SQL
type strings; if they differ and at least one side used a `@db`.* override, cast
the overridden side back to its base SQL type using this.eb.cast(...,
sql.raw(this.getSqlType(...))) before calling super.buildComparison so
mismatched native/native or native/implicit comparisons (e.g. uuid vs text) are
normalized.
In `@packages/plugins/policy/src/expression-transformer.ts`:
- Around line 268-274: The comparison lookup must use the correct type depending
on the operand context: change the two calls to
getFieldDefFromFieldRef(normalizedLeft, context.modelOrType) and
getFieldDefFromFieldRef(normalizedRight, context.modelOrType) so they resolve
against context.thisType when the ref is a "this" reference (e.g. this.foo) and
keep using context.modelOrType for direct fields and for before().foo
expressions; update the logic around normalizedLeft/normalizedRight detection
(look for "this" prefix or before() pattern) and pass the appropriate type into
getFieldDefFromFieldRef before calling dialect.buildComparison/ExpressionWrapper
so the FieldDef preserves native-type info for buildComparison.
In `@tests/regression/test/issue-2394.test.ts`:
- Around line 30-46: The test currently only compares two fields; add a
regression test that compares the `@db.Uuid` field to an auth() value so
transformAuthBinary's path is exercised: change the model policy to use
@@allow('all', id == auth()) (reference @@allow and auth()), use
createPolicyTestClient to create the test DB client (reference
createPolicyTestClient), generate a UUID (reference randomUUID/newId) and ensure
the create/update calls supply the same auth value in the request/session
context the test harness expects so the auth() side is typed as the native UUID
and the create/update assertions (db.foo.create and db.foo.update) still resolve
truthily; this will catch regressions where auth() loses native-type metadata
(reference transformAuthBinary).
- Around line 22-27: Uncomment the deny-path assertion so the test verifies the
post-update deny behavior: re-enable the lines that call expect(db.item.update({
where: { id: updated.id }, data: { status: 'published' } })).rejects.toThrow();
(i.e., keep the earlier successful update via db.item.update to 'published',
then call db.item.update again with the same status and assert it throws) to
ensure the policy check that compares before().status to the new status is
actually exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 723b55a4-db83-4d5e-94c5-f95221aa692c
📒 Files selected for processing (4)
packages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud/dialects/postgresql.tspackages/plugins/policy/src/expression-transformer.tstests/regression/test/issue-2394.test.ts
…ison Instead of casting based solely on presence of @db.* attributes, resolve the effective SQL type for each side (native type from @db.* or base ZModel type as fallback), then only cast when the types actually differ. This avoids unnecessary casts for @db.Text vs String (both `text`) and correctly handles two differing native types (e.g. @db.Uuid vs @db.Citext). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job PR Review: PostgreSQL UUID Type Mismatch FixReviewing PR #2532 that fixes PostgreSQL type mismatches when Todo List
Overall Assessment ⭐This is a well-architected solution to a specific PostgreSQL type system issue. The implementation demonstrates strong understanding of database dialect differences and follows good separation of concerns by keeping PostgreSQL-specific logic isolated. Code Quality & Best Practices ✅Excellent Architecture
Strong Implementation Details
Technical Deep Dive 🔧PostgreSQL Dialect Changes (
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/orm/src/client/crud/dialects/postgresql.ts (1)
455-466: Edge case: unmapped@db.*attributes returnsqlType: undefinedwithhasDbOverride: true.If a
@db.*attribute exists but isn't indbAttributeToSqlTypeMap(e.g.,@db.Money), line 463 returns{ sqlType: undefined, hasDbOverride: true }. This works in practice—buildComparisonwill cast the field to its base ZModel type—but the combination ofundefinedtype with a true override flag is semantically confusing.Consider adding a fallback or a comment clarifying this behavior:
📝 Optional: clarify fallback for unmapped attributes
private resolveFieldSqlType(fieldDef: FieldDef | undefined): { sqlType: string | undefined; hasDbOverride: boolean } { if (!fieldDef) { return { sqlType: undefined, hasDbOverride: false }; } const dbAttr = fieldDef.attributes?.find((a) => a.name.startsWith('@db.')); if (dbAttr) { - return { sqlType: PostgresCrudDialect.dbAttributeToSqlTypeMap[dbAttr.name], hasDbOverride: true }; + const mappedType = PostgresCrudDialect.dbAttributeToSqlTypeMap[dbAttr.name]; + // If the `@db`.* attribute isn't in our map, fall back to base ZModel type + // but still mark hasDbOverride so buildComparison knows to cast if needed + return { sqlType: mappedType ?? this.getSqlType(fieldDef.type), hasDbOverride: true }; } return { sqlType: this.getSqlType(fieldDef.type), hasDbOverride: false }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/crud/dialects/postgresql.ts` around lines 455 - 466, The resolveFieldSqlType function currently sets hasDbOverride=true even when a `@db`.* attribute isn't mapped, which is confusing; update resolveFieldSqlType so that when dbAttr exists but PostgresCrudDialect.dbAttributeToSqlTypeMap[dbAttr.name] is undefined you fall back to this.getSqlType(fieldDef.type) and return hasDbOverride:false (i.e., treat unmapped attributes as no override), keeping references to resolveFieldSqlType, PostgresCrudDialect.dbAttributeToSqlTypeMap, getSqlType and FieldDef so the change is made in the same method and keeps buildComparison behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/orm/src/client/crud/dialects/postgresql.ts`:
- Around line 455-466: The resolveFieldSqlType function currently sets
hasDbOverride=true even when a `@db`.* attribute isn't mapped, which is confusing;
update resolveFieldSqlType so that when dbAttr exists but
PostgresCrudDialect.dbAttributeToSqlTypeMap[dbAttr.name] is undefined you fall
back to this.getSqlType(fieldDef.type) and return hasDbOverride:false (i.e.,
treat unmapped attributes as no override), keeping references to
resolveFieldSqlType, PostgresCrudDialect.dbAttributeToSqlTypeMap, getSqlType and
FieldDef so the change is made in the same method and keeps buildComparison
behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee7a05b9-b9bf-495a-afc8-74e33e4ddc01
📒 Files selected for processing (1)
packages/orm/src/client/crud/dialects/postgresql.ts
Exercises the transformAuthBinary path where `id == auth()` expands to `id == auth().id`. The auth value is always emitted as a text parameter regardless of the auth model's @db.Uuid attribute, so this catches regressions where the uuid/text type mismatch is reintroduced. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job PR Review: PostgreSQL UUID Type Mismatch FixReviewing PR #2532 that fixes PostgreSQL type mismatches when Todo List
Overall Assessment ⭐This is a well-architected solution to a specific PostgreSQL type system issue. The implementation demonstrates strong understanding of database dialect differences and follows good separation of concerns by keeping PostgreSQL-specific logic isolated. Code Quality & Best Practices ✅Excellent Architecture
Strong Implementation Details
Technical Deep Dive 🔧PostgreSQL Dialect Changes (
|
|
Claude finished @ymc9's task —— View job PR Review: PostgreSQL UUID Type Mismatch FixReviewing PR #2532 that fixes PostgreSQL type mismatches when Todo List
Overall Assessment ⭐This is a well-architected solution to a specific PostgreSQL type system issue. The implementation demonstrates strong understanding of database dialect differences and follows good separation of concerns by keeping PostgreSQL-specific logic isolated. Code Quality & Best Practices ✅Excellent Architecture
Strong Implementation Details
Technical Deep Dive 🔧PostgreSQL Dialect Changes (
|
… casting Extended getFieldDefFromFieldRef to handle: - `this.foo` references resolving against thisType instead of modelOrType, so native-type info is preserved in collection-predicate contexts where the two differ. - Single- and multi-hop relation field access (e.g. `owner.tag`, `org.owner.token`): walk the relation chain and return the terminal field's FieldDef so buildComparison can detect @db.* type mismatches and cast accordingly (e.g. "operator does not exist: uuid = text"). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job PR Review: PostgreSQL UUID Type Mismatch FixReviewing PR #2532 that fixes PostgreSQL type mismatches when Todo List
Overall Assessment ⭐This is a well-architected solution to a specific PostgreSQL type system issue. The implementation demonstrates strong understanding of database dialect differences and follows good separation of concerns by keeping PostgreSQL-specific logic isolated. Code Quality & Best Practices ✅Excellent Architecture
Strong Implementation Details
Technical Deep Dive 🔧PostgreSQL Dialect Changes (
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/plugins/policy/src/expression-transformer.ts (1)
268-279:⚠️ Potential issue | 🟠 MajorField metadata still disappears for
before(), binding, and multi-hopthisoperands.Lines 270-279 only switch away from
modelOrTypefor directthis.*, and Lines 1005-1017 only walk chains when the receiver is a plain field.before().ownerId,binding.ownerId, andthis.owner.idstill reachbuildComparison(...)withundefinedFieldDefs, so PostgreSQL can still miss the native@db.*cast and reproduce the sameuuid = textfailure on those supported expression forms. Please resolve the receiver model from the full comparison context before walking the member chain.Also applies to: 996-1017
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/policy/src/expression-transformer.ts` around lines 268 - 279, The field metadata loss happens because you only switch to context.thisType for direct `this.*` receivers and only walk member chains when the immediate receiver is a plain field; update the comparison resolution to first resolve the root receiver and its model from the whole member chain (e.g., inspect the full normalizedLeft/normalizedRight member chain to find if the ultimate root is `this`, `binding`, `before`, or a model field) and then call getFieldDefFromFieldRef with that resolved model so buildComparison never receives undefined FieldDef; adjust the logic around ExpressionUtils.isMember/ExpressionUtils.isThis checks and the code that currently sets leftModel/rightModel (and the member-walking block around the 996-1017 area) to walk chains for multi-hop `this.owner.id`, `before().ownerId`, and `binding.ownerId` and derive the correct FieldDef.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/plugins/policy/src/expression-transformer.ts`:
- Around line 268-279: The field metadata loss happens because you only switch
to context.thisType for direct `this.*` receivers and only walk member chains
when the immediate receiver is a plain field; update the comparison resolution
to first resolve the root receiver and its model from the whole member chain
(e.g., inspect the full normalizedLeft/normalizedRight member chain to find if
the ultimate root is `this`, `binding`, `before`, or a model field) and then
call getFieldDefFromFieldRef with that resolved model so buildComparison never
receives undefined FieldDef; adjust the logic around
ExpressionUtils.isMember/ExpressionUtils.isThis checks and the code that
currently sets leftModel/rightModel (and the member-walking block around the
996-1017 area) to walk chains for multi-hop `this.owner.id`, `before().ownerId`,
and `binding.ownerId` and derive the correct FieldDef.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d204cd18-25b5-4d49-94f4-5eb204591dc0
📒 Files selected for processing (2)
packages/plugins/policy/src/expression-transformer.tstests/regression/test/issue-2394.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/regression/test/issue-2394.test.ts
…model string Move the this.foo → thisType / else → modelOrType selection inside getFieldDefFromFieldRef so callers don't need to compute a leftModel/rightModel intermediate before each call. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job PR Review: PostgreSQL UUID Type Mismatch FixReviewing PR #2532 that fixes PostgreSQL type mismatches when Todo List
Overall Assessment ⭐This is a well-architected solution to a specific PostgreSQL type system issue. The implementation demonstrates strong understanding of database dialect differences and follows good separation of concerns by keeping PostgreSQL-specific logic isolated. Code Quality & Best Practices ✅Excellent Architecture
Strong Implementation Details
Technical Deep Dive 🔧PostgreSQL Dialect Changes (
|
1 participant
Summary
Fixes #2394. Two related PostgreSQL errors ("operator does not exist: uuid = text") when
@db.Uuidfields are involved in policy expressions:before()in post-update policies: the$beforeVALUES table was casting UUID ID columns totextinstead ofuuid, causing the JOIN condition to fail.@db.Uuidcolumn against a plainStringfield or anauth()value (which always arrives as a text parameter) caused a type mismatch.Changes
packages/orm/src/client/crud/dialects/postgresql.tsgetSqlTypenow checks@db.*field attributes first, returning the exact native SQL type (e.g.uuid) instead of the generic ZModel base type (text) — fixes the VALUES table cast forbefore().buildComparisonoverride detects when one operand has a@db.*native type and the other doesn't, and casts the native-typed side back to its base ZModel SQL type to align comparison types.packages/orm/src/client/crud/dialects/base-dialect.tsbuildComparisonvirtual method with a default passthrough implementation for other dialects (SQLite, MySQL) where this is not an issue.packages/plugins/policy/src/expression-transformer.ts_binarynow routes non-null comparisons throughdialect.buildComparison, keeping all database-specific casting logic inside the dialect layer.Test plan
tests/regression/test/issue-2394.test.ts— two new PostgreSQL regression tests covering both fixed scenarios🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor
Tests