fix(orm): cast fields with @db.* native type annotations to base SQL type#2473
fix(orm): cast fields with @db.* native type annotations to base SQL type#2473
Conversation
…type When a field has a native database type annotation like @db.Uuid, PostgreSQL stores it as that native type. Comparing such a field with a plain String field (e.g., in policy expressions like `id == x`) causes a type mismatch error. This fix casts fields with @db.* attributes back to their ZModel base SQL type in fieldRef(), which affects both SELECT and WHERE clauses. - Add abstract `getSqlType()` to BaseCrudDialect with implementations in PostgreSQL, SQLite, and MySQL dialects - Add `hasNativeTypeAttribute()` helper to detect @db.* attributes - Modify `fieldRef()` to apply CAST for fields with native type annotations - Update policy expression transformer to use dialect's fieldRef() Fixes #2394 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job Code Review Completed ✅Reviewing this PR that fixes database UUID casting issues. Here's my analysis:
Overall Assessment: APPROVE ✅This is a well-designed fix for a legitimate PostgreSQL type compatibility issue. The implementation is clean, follows established patterns, and has appropriate test coverage. StrengthsArchitecture & Design:
Implementation Quality:
Test Coverage:
Technical ReviewType Mappings:
Performance Considerations:
Security:
Minor Observations
Problem SolvedThe fix addresses the core issue where PostgreSQL fields with This is a targeted, low-risk fix that solves a specific database compatibility issue without affecting the broader system architecture. Recommendation: MERGE ✅ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds dialect-aware SQL type mapping and native-type casting to CRUD dialects (BaseCrudDialect gains getSqlType and hasNativeTypeAttribute). Dialects implement getSqlType. Field references in policy expression transformer and policy handler now use dialect.fieldRef OperationNodes. Adds a regression test and updates test deps. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
fixes #2394 |
![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
🧹 Nitpick comments (1)
tests/regression/test/issue-2394.test.ts (1)
15-22: Assert the comparison result, not just that the query resolves.Line 22 will also pass on
[], so this regression only proves the query stopped throwing. It won't catch a broken cast that turnsid == xinto an always-false predicate.Suggested test tightening
- await db.$unuseAll().foo.create({ data: { x: uuid() } }); - await expect(db.foo.findMany()).toResolveTruthy(); + const id = uuid(); + await db.$unuseAll().foo.create({ data: { id, x: id } }); + await expect(db.foo.findMany()).resolves.toHaveLength(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/regression/test/issue-2394.test.ts` around lines 15 - 22, The test currently only asserts that db.foo.findMany() resolves (expect(db.foo.findMany()).toResolveTruthy()), which would pass for an empty array; instead await the query and assert the actual result contents or length to ensure the id match worked. Update the assertion after creating the record (using createPolicyTestClient and db.$unuseAll().foo.create) to await db.foo.findMany() and assert e.g. that the returned array length is 1 or that it equals the expected object (matching the created uuid), so the test fails if the cast turned the predicate always-false.
🤖 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/base-dialect.ts`:
- Around line 1374-1383: The cast target derivation must preserve array-ness for
list fields: when a FieldDef has a native DB type
(hasNativeTypeAttribute(fieldDef)) and fieldDef.isList (or equivalent list
indicator), append "[]" to the sqlType returned by getSqlType(fieldDef.type) so
you cast to e.g., "text[]" instead of "text"; apply this same FieldDef-aware
logic to the other duplicate cast site in the delegate-descendant selection path
(the block around the delegate selection at lines 1162-1168) so both uses (the
shown CAST branch using sql.ref/sql.raw and the delegate-descendant branch that
currently uses fieldDef.type only) consistently produce array-typed casts and
avoid feeding scalars into array operators.
In `@packages/orm/src/client/crud/dialects/mysql.ts`:
- Around line 321-335: getSqlType in the MySQL dialect returns column-definition
types that are invalid CAST targets and causes fieldRef() (in base-dialect.ts)
to emit invalid CAST(expr AS type) expressions; update
MySQLDialect.prototype.getSqlType to return CAST-friendly types: map enum/String
varchar(191) → CHAR(191) (preferably "CHAR(191) CHARACTER SET utf8mb4"), map
Boolean tinyint(1) → SIGNED (or UNSIGNED if the column is unsigned), map BigInt
bigint → SIGNED/UNSIGNED, and map Bytes longblob → BINARY (or a fixed-length
BINARY(N) if known); keep the existing valid returns (signed, double,
decimal(65,30), datetime(3), json) unchanged so fieldRef() produces valid CAST
targets.
In `@packages/plugins/policy/src/expression-transformer.ts`:
- Around line 909-914: The delegated-base-field branch currently returns a raw
column selection from buildDelegateBaseFieldSelect, causing mismatched DB types
for inherited fields; update buildDelegateBaseFieldSelect (or its return in
expression-transformer.ts) to use this.dialect.fieldRef(...) for the origin
model/column (e.g. this.dialect.fieldRef(fieldDef.originModel, column,
tableName, false).toOperationNode()) instead of a raw column node so delegated
base fields are cast the same way as same-model fields; keep
QueryUtils.requireField, dialect.fieldRef, and buildDelegateBaseFieldSelect as
the key symbols to locate and change.
---
Nitpick comments:
In `@tests/regression/test/issue-2394.test.ts`:
- Around line 15-22: The test currently only asserts that db.foo.findMany()
resolves (expect(db.foo.findMany()).toResolveTruthy()), which would pass for an
empty array; instead await the query and assert the actual result contents or
length to ensure the id match worked. Update the assertion after creating the
record (using createPolicyTestClient and db.$unuseAll().foo.create) to await
db.foo.findMany() and assert e.g. that the returned array length is 1 or that it
equals the expected object (matching the created uuid), so the test fails if the
cast turned the predicate always-false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 533bd452-d446-4f1b-8d4c-b0dd5ae32f5e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
packages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud/dialects/mysql.tspackages/orm/src/client/crud/dialects/postgresql.tspackages/orm/src/client/crud/dialects/sqlite.tspackages/plugins/policy/src/expression-transformer.tstests/regression/package.jsontests/regression/test/issue-2394.test.ts
…ansformer Use dialect.fieldRef() for the selection in buildDelegateBaseFieldSelect so inherited fields with @db.* attributes get the same CAST treatment as same-model fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job Code Review Completed ✅Reviewing this PR that fixes database UUID casting issues. Here's my analysis:
Overall Assessment: APPROVE ✅This is a well-designed fix for a legitimate PostgreSQL type compatibility issue. The implementation is clean, follows established patterns, and has appropriate test coverage. StrengthsArchitecture & Design:
Implementation Quality:
Test Coverage:
Technical ReviewType Mappings:
Performance Considerations:
Security:
Critical Issues Found
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/orm/src/client/crud/dialects/mysql.ts (1)
321-323:⚠️ Potential issue | 🟠 MajorUse a valid MySQL CAST target for enums.
Line 323 returns
varchar(191), andfieldRef()will splice that directly intoCAST(... AS ...). MySQL permitsCHAR[(N)]for string casts, notVARCHAR, and its own examples castENUMvalues withCAST(enum_col AS CHAR). This still leaves the enum/native-type path open to a runtime SQL error. (dev.mysql.com)Suggested fix
protected override getSqlType(zmodelType: string) { if (isEnum(this.schema, zmodelType)) { - return 'varchar(191)'; + return 'char character set utf8mb4'; } return match(zmodelType) .with('String', () => 'char')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/crud/dialects/mysql.ts` around lines 321 - 323, getSqlType currently returns 'varchar(191)' for enums which will be injected into CAST(... AS ...) and is invalid in MySQL; update the enum branch in getSqlType (packages/orm/src/client/crud/dialects/mysql.ts, method getSqlType and the isEnum check) to return a valid MySQL CAST target such as 'char(191)' (or 'char' if you prefer no length) and ensure any other code paths that map enum/native types to SQL types also use CHAR rather than VARCHAR so CAST(... AS CHAR) is produced.
🤖 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/orm/src/client/crud/dialects/mysql.ts`:
- Around line 321-323: getSqlType currently returns 'varchar(191)' for enums
which will be injected into CAST(... AS ...) and is invalid in MySQL; update the
enum branch in getSqlType (packages/orm/src/client/crud/dialects/mysql.ts,
method getSqlType and the isEnum check) to return a valid MySQL CAST target such
as 'char(191)' (or 'char' if you prefer no length) and ensure any other code
paths that map enum/native types to SQL types also use CHAR rather than VARCHAR
so CAST(... AS CHAR) is produced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2791dd81-4fad-4d25-9858-ad62ebb0f07c
📒 Files selected for processing (2)
packages/orm/src/client/crud/dialects/mysql.tspackages/plugins/policy/src/expression-transformer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/plugins/policy/src/expression-transformer.ts
When a field with a @db.* attribute is also an array, the CAST type should use the array form (e.g., `text[]` instead of `text`). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job Code Review Completed ✅Reviewing this PR that fixes database UUID casting issues. Here's my analysis:
Overall Assessment: APPROVE ✅This is a well-designed fix for a legitimate PostgreSQL type compatibility issue. The implementation is clean, follows established patterns, and has appropriate test coverage. StrengthsArchitecture & Design:
Implementation Quality:
Test Coverage:
Technical ReviewType Mappings:
Performance Considerations:
Security:
Critical Issues Found
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/base-dialect.ts`:
- Around line 1162-1174: The code currently builds descendant field refs
directly (using eb.ref / sql.ref) which bypasses the shared fieldRef() logic and
thus skips computed-field handling; replace the direct ref construction with a
call to fieldRef() for the descendant (e.g. obtain an expression via
this.fieldRef(subModel.name, field, { eb, sql }) or similar API used elsewhere),
then if castSqlType is present wrap that returned expression in a CAST to the
computed castType (CAST(<fieldRefExpr> AS <castType>)); keep requireField(),
hasNativeTypeAttribute(), getSqlType() checks but delegate actual field
expression construction to fieldRef so computed fields are correctly handled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e1dae5b9-ec60-4f11-b080-283462739a18
📒 Files selected for processing (1)
packages/orm/src/client/crud/dialects/base-dialect.ts
…ields The post-update policy check joins the current table with a VALUES table ($before) on id fields. When an id field has @db.Uuid, the raw column is uuid while the VALUES table casts to text, causing a type mismatch. Use dialect.fieldRef() for both sides of the join so the uuid column gets cast to text. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job Code Review Completed ✅Reviewing this PR that fixes database UUID casting issues. Here's my analysis:
Overall Assessment: APPROVE ✅This is a well-designed fix for a legitimate PostgreSQL type compatibility issue. The implementation is clean, follows established patterns, and has appropriate test coverage. StrengthsArchitecture & Design:
Implementation Quality:
Test Coverage:
Technical ReviewType Mappings:
Performance Considerations:
Security:
Critical Issues Found
|
|
Claude finished @ymc9's task —— View job Code Review Completed ✅Reviewing this PR that fixes database UUID casting issues. Here's my analysis:
Overall Assessment: APPROVE ✅This is a well-designed fix for a legitimate PostgreSQL type compatibility issue. The implementation is clean, follows established patterns, and has appropriate test coverage. StrengthsArchitecture & Design:
Implementation Quality:
Test Coverage:
Technical ReviewType Mappings:
Performance Considerations:
Security:
Critical Issues Found
|
1 participant
Summary
@db.Uuid, PostgreSQL stores it as that native type. Comparing such a field with a plainStringfield (e.g., in policy expressions like@@allow('all', id == x)) causes a type mismatch error (operator does not exist: uuid = text)@db.*attributes back to their ZModel base SQL type viafieldRef(), which affects both SELECT and WHERE clausesgetSqlType()toBaseCrudDialectwith implementations in all three dialect subclasses (PostgreSQL, SQLite, MySQL)Test plan
tests/regression/test/issue-2394.test.tspasses🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Chores