feat(orm): mysql support #616
Conversation
|
Claude finished @ymc9's task —— View job PR Review: MySQL Support ImplementationReviewing this substantial PR that adds MySQL support to ZenStack ORM. This is a significant feature addition with extensive changes across the codebase. Todo List
🔍 Comprehensive Review Results✅ Security & SQL Safety - Excellent
✅ Code Quality - High Standard
✅ Feature Completeness - ComprehensiveCore MySQL Features Implemented:
Advanced Features:
✅ Test Coverage - Thorough
|
|
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. 📝 WalkthroughWalkthroughAdds comprehensive MySQL support: new MySQL CRUD dialect and lateral-join base, provider capability flags and type gating, transformInput/transformOutput plumbing, non-RETURNING read-back strategies, CI/test matrix and test-suite adaptations, and wide-ranging dialect-aware code and test updates. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 adds MySQL as a supported ORM provider and updates the runtime, dialect layer, and test infrastructure to account for MySQL-specific SQL/feature differences (notably lack of RETURNING, collation/case-sensitivity, and differing DDL/quoting).
Changes:
- Introduces a MySQL CRUD dialect and wires provider selection throughout the ORM/runtime.
- Updates policy enforcement, mutation hooks, and executor/read-back logic to work without
RETURNING. - Expands CI/test tooling and updates many tests for MySQL compatibility or skips unsupported operations.
Reviewed changes
Copilot reviewed 77 out of 78 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/regression/test/v2-migrated/issue-1894.test.ts | Uses created record’s id instead of assuming id: 0 works across providers. |
| tests/regression/test/v2-migrated/issue-1681.test.ts | Skips createManyAndReturn assertion for MySQL. |
| tests/regression/test/v2-migrated/issue-1576.test.ts | Adds MySQL fallback path since createManyAndReturn isn’t supported. |
| tests/e2e/package.json | Adds ts-pattern dependency for new provider branching. |
| tests/e2e/orm/query-builder/query-builder.test.ts | Normalizes DateTime string format for MySQL compatibility. |
| tests/e2e/orm/policy/policy-functions.test.ts | Accounts for MySQL case-insensitive string comparison defaults. |
| tests/e2e/orm/policy/nonexistent-models.test.ts | Uses provider-specific DDL for creating tables with correct quoting/types. |
| tests/e2e/orm/policy/migrated/view.test.ts | Uses provider-specific CREATE VIEW syntax; errors on unsupported providers. |
| tests/e2e/orm/policy/migrated/update-many-and-return.test.ts | Skips MySQL where updateManyAndReturn isn’t supported; enables additional coverage on other DBs. |
| tests/e2e/orm/policy/migrated/toplevel-operations.test.ts | Adjusts expectations for MySQL limitations around ID updates under post-update policies. |
| tests/e2e/orm/policy/migrated/nested-to-one.test.ts | Adjusts/blocks MySQL behavior for nested updates that mutate IDs. |
| tests/e2e/orm/policy/migrated/multi-id-fields.test.ts | Adds MySQL-specific behavior for post-update policy checks with ID updates. |
| tests/e2e/orm/policy/migrated/deep-nested.test.ts | Broadens error matching to include MySQL duplicate/constraint text. |
| tests/e2e/orm/policy/migrated/current-operation.test.ts | Adjusts expected policy outcome for MySQL case-insensitive comparisons. |
| tests/e2e/orm/policy/migrated/current-model.test.ts | Same as above for current-model tests and casing variants. |
| tests/e2e/orm/policy/migrated/create-many-and-return.test.ts | Skips MySQL since createManyAndReturn isn’t supported. |
| tests/e2e/orm/policy/migrated/auth.test.ts | Skips MySQL-only unsupported returning operations; broadens constraint message matching. |
| tests/e2e/orm/policy/crud/update.test.ts | Adds MySQL-specific upsert/on-duplicate behavior and relaxes constraint matching. |
| tests/e2e/orm/policy/crud/read.test.ts | Forces PostgreSQL provider for features only supported there (FROM). |
| tests/e2e/orm/policy/crud/create.test.ts | Removes .returningAll() dependence to work on providers without RETURNING. |
| tests/e2e/orm/plugin-infra/on-kysely-query.test.ts | Makes inserted DateTime values MySQL-compatible and simplifies insert node creation. |
| tests/e2e/orm/plugin-infra/ext-query-args.test.ts | Skips createManyAndReturn/updateManyAndReturn expectations for MySQL. |
| tests/e2e/orm/plugin-infra/entity-mutation-hooks.test.ts | Adds assertion for beforeMutationEntities availability in after hooks. |
| tests/e2e/orm/client-api/update.test.ts | Broadens constraint matching for MySQL error text. |
| tests/e2e/orm/client-api/update-many.test.ts | Skips updateManyAndReturn on MySQL due to missing RETURNING. |
| tests/e2e/orm/client-api/type-coverage.test.ts | Uses usePrismaPush and narrows array-type test to PostgreSQL. |
| tests/e2e/orm/client-api/relation/self-relation.test.ts | Import reordering only. |
| tests/e2e/orm/client-api/raw-query.test.ts | Adds provider-specific quoting and SQL variants for MySQL. |
| tests/e2e/orm/client-api/name-mapping.test.ts | Skips/adjusts RETURNING-dependent assertions on MySQL. |
| tests/e2e/orm/client-api/mixin.test.ts | Broadens constraint matching for MySQL error text. |
| tests/e2e/orm/client-api/json-filter.test.ts | Strengthens assertion for created JSON content. |
| tests/e2e/orm/client-api/find.test.ts | Extends distinct-not-supported branch to MySQL as well. |
| tests/e2e/orm/client-api/error-handling.test.ts | Adds MySQL error code mapping and broadens message matching. |
| tests/e2e/orm/client-api/delegate.test.ts | Skips createManyAndReturn/updateManyAndReturn on MySQL; broadens constraint matching. |
| tests/e2e/orm/client-api/create-many-and-return.test.ts | Skips MySQL since createManyAndReturn isn’t supported. |
| tests/e2e/orm/client-api/computed-fields.test.ts | Changes computed expression to be compatible across providers and skips MySQL returning test. |
| pnpm-workspace.yaml | Adds mysql2 to catalog for workspace consumption. |
| pnpm-lock.yaml | Locks mysql2 and ts-pattern additions and updated transitive graph. |
| packages/testtools/src/schema.ts | Extends schema generation helpers to support MySQL datasource prelude/placeholder replacement. |
| packages/testtools/src/client.ts | Adds MySQL test DB config, dialect creation, and DB provisioning logic. |
| packages/testtools/package.json | Adds mysql2 dependency for MySQL test dialect setup. |
| packages/server/test/api/rpc.test.ts | Formatting / type cast adjustments. |
| packages/server/test/api/rest.test.ts | Adjusts test data IDs and formatting. |
| packages/schema/src/schema.ts | Adds mysql to DataSourceProviderType. |
| packages/plugins/policy/src/utils.ts | Switches to transformInput API. |
| packages/plugins/policy/src/policy-handler.ts | Reworks post-update handling and adds MySQL onDuplicateKeyUpdate support. |
| packages/plugins/policy/src/functions.ts | Adjusts check function query structure. |
| packages/plugins/policy/src/expression-transformer.ts | Switches to transformInput API. |
| packages/orm/tsup.config.ts | Adds MySQL dialect entrypoint build. |
| packages/orm/src/dialects/mysql.ts | Exposes Kysely MysqlDialect from ORM package. |
| packages/orm/src/client/query-utils.ts | Adjusts default orderBy typing and removes unused type import. |
| packages/orm/src/client/plugin.ts | Adds beforeMutationEntities to after-mutation hook args. |
| packages/orm/src/client/helpers/schema-db-pusher.ts | Adds MySQL DDL/type/default handling for test-only schema pushing. |
| packages/orm/src/client/functions.ts | Makes text matching/escaping logic more cross-dialect friendly. |
| packages/orm/src/client/executor/zenstack-query-executor.ts | Adds MySQL-aware mutation hook read-back logic and normalizes mutation results. |
| packages/orm/src/client/executor/name-mapper.ts | Uses dialect-aware casting for name-mapping transformations. |
| packages/orm/src/client/crud/operations/update.ts | Forces read-back for dialects without RETURNING. |
| packages/orm/src/client/crud/operations/group-by.ts | Uses dialect casting and passes take into orderBy builder. |
| packages/orm/src/client/crud/operations/delete.ts | Uses numAffectedRows for correctness on non-returning dialects. |
| packages/orm/src/client/crud/operations/create.ts | Wraps createMany in a transaction. |
| packages/orm/src/client/crud/operations/count.ts | Uses dialect casting for counts. |
| packages/orm/src/client/crud/operations/base.ts | Adds MySQL fallbacks for create/update/delete and expands insert ignore handling. |
| packages/orm/src/client/crud/operations/aggregate.ts | Uses dialect casting and passes take into orderBy builder. |
| packages/orm/src/client/crud/dialects/sqlite.ts | Introduces capability flags, casting helpers, and VALUES-table helper. |
| packages/orm/src/client/crud/dialects/postgresql.ts | Introduces capability flags, casting helpers, and VALUES-table helper. |
| packages/orm/src/client/crud/dialects/mysql.ts | Adds new MySQL CRUD dialect implementation. |
| packages/orm/src/client/crud/dialects/index.ts | Registers MySQL dialect in dialect factory. |
| packages/orm/src/client/crud/dialects/base-dialect.ts | Adds capability flags, transformInput, casting hooks, and VALUES-table interface. |
| packages/orm/src/client/crud-types.ts | Makes distinct type available only for providers that support it. |
| packages/orm/src/client/contract.ts | Removes createManyAndReturn/updateManyAndReturn from MySQL model contract. |
| packages/orm/src/client/client-impl.ts | Throws explicit not-supported errors for MySQL *AndReturn operations. |
| packages/orm/package.json | Adds mysql dialect export and mysql2 optional peer dependency. |
| packages/language/src/validators/datamodel-validator.ts | Removes sqlite-specific list-type validation in favor of centralized validation. |
| packages/language/src/document.ts | Adds provider-aware validation for list types after import merge. |
| packages/language/src/constants.ts | Adds MySQL to supported providers; defines providers supporting list types. |
| package.json | Adds test:mysql and includes it in test:all. |
| TODO.md | Marks MySQL and custom procedures as completed. |
| .github/workflows/build-test.yml | Adds MySQL service and runs matrix tests against mysql provider. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/e2e/orm/client-api/delegate.test.ts (1)
179-187: Make error matching resilient whencauseis absent.The predicate assumes
e.cause.messageexists; some providers surface the message one.messageinstead, which can make the matcher throw for the wrong reason.💡 Suggested hardening
- ).rejects.toSatisfy((e) => e.cause.message.toLowerCase().match(/(constraint)|(duplicate)/i)); + ).rejects.toSatisfy((e) => { + const msg = String(e?.cause?.message ?? e?.message ?? ''); + return /(constraint|duplicate)/i.test(msg); + });packages/orm/src/client/helpers/schema-db-pusher.ts (1)
327-332: Array type handling incompatible with RawBuilder.When
mapFieldTypereturns aRawBuilder(e.g., for MySQL'svarchar(255)), the array branch at line 329 attempts to use it in a template string, which will produce incorrect SQL like[object Object][].🐛 Proposed fix
if (fieldDef.array) { // Kysely doesn't support array type natively - return sql.raw(`${result}[]`); + if (typeof result === 'string') { + return sql.raw(`${result}[]`); + } else { + // For RawBuilder types, we need to handle this differently + // Note: MySQL doesn't support array types natively anyway + throw new Error(`Array types are not supported for provider ${this.schema.provider.type}`); + } } else { return result as ColumnDataType; }packages/orm/src/client/crud/dialects/base-dialect.ts (1)
734-752: Fix JSON null filtering for Postgres (current predicate collapses to false).
transformInput(null, 'Json', false)returns SQL NULL for Postgres, which makeslhs = NULL AND lhs IS NOT NULLimpossible and degrades AnyNull to DbNull only.🐛 Proposed fix
- } else if (value instanceof JsonNullClass) { - return this.eb.and([ - this.eb(lhs, '=', this.transformInput(null, 'Json', false)), - this.eb(lhs, 'is not', null), - ]); - } else if (value instanceof AnyNullClass) { - // AnyNull matches both DB NULL and JSON null - return this.eb.or([this.eb(lhs, 'is', null), this.eb(lhs, '=', this.transformInput(null, 'Json', false))]); + } else if (value instanceof JsonNullClass) { + const jsonNull = this.transformInput(new JsonNullClass(), 'Json', false); + return this.eb.and([this.eb(lhs, '=', jsonNull), this.eb(lhs, 'is not', null)]); + } else if (value instanceof AnyNullClass) { + // AnyNull matches both DB NULL and JSON null + const jsonNull = this.transformInput(new JsonNullClass(), 'Json', false); + return this.eb.or([this.eb(lhs, 'is', null), this.eb(lhs, '=', jsonNull)]);
🤖 Fix all issues with AI agents
In `@packages/orm/src/client/crud/dialects/mysql.ts`:
- Around line 668-694: The code contains duplicate handling for the "_count"
aggregate: the initial if that checks ['_count','_avg','_sum','_min','_max']
already processes "_count" and continues, making the switch case for '_count'
unreachable; remove the redundant switch case block for '_count' (the case
labeled '_count' that uses eb.fn.count and buildFieldRef) so only the
aggregate-handling branch (which calls aggregate(...)) remains, leaving other
switch cases intact.
- Around line 528-531: The buildArrayLiteralSQL implementations are vulnerable
because string elements are interpolated without escaping single quotes; update
the MySQL override buildArrayLiteralSQL and the PostgreSQL counterpart (the
override buildArrayLiteralSQL in postgresql.ts) to properly escape string values
before interpolation (e.g., replace single quote characters ' with doubled ''),
and ensure strings are wrapped in single quotes after escaping; leave non-string
values untouched and preserve current array formatting (MySQL JSON_ARRAY(...)
and Postgres ARRAY[...] or ARRAY[...]::type) so only the string-escaping logic
is added to the existing functions.
In `@packages/orm/src/client/crud/operations/base.ts`:
- Around line 2534-2537: There is a duplicate, unreachable check of
this.dialect.supportsReturning that returns { needReadBack: true,
selectedFields: undefined }; remove that redundant block from the method in
base.ts (the one using supportsReturning/needReadBack/selectedFields) so the
earlier check (lines ~2524–2527) is the single source of truth; ensure no other
logic relies on this second return before deleting it.
- Around line 1292-1296: The current check uses updateResult.numAffectedRows to
decide whether to set updatedEntity = null which can be false-negative on MySQL
when rows matched but no values changed; update the logic in the update flow
(around executeQuery / updateQuery handling) to: if updateResult.numAffectedRows
is undefined or zero, either (a) prefer updateResult.numUpdatedRows when
present, or (b) perform the read-back query unconditionally (or if you already
fetched thisEntity earlier, use that presence to determine existence) and only
set updatedEntity = null when the read-back confirms the row is missing; ensure
references to executeQuery, updateQuery, numAffectedRows, numUpdatedRows,
updatedEntity and thisEntity are updated accordingly.
In `@packages/orm/src/client/helpers/schema-db-pusher.ts`:
- Line 298: The current sql.raw call interpolates enumValues directly (return
sql.raw(`enum(${enumValues.map((v) => `'${v}'`).join(', ')})`)), which allows
quotes/injection; fix by using parameterized bindings or proper escaping: build
a placeholder list like enum(${enumValues.map(() => '?').join(',')}) and call
sql.raw with that string and the enumValues as bindings (or use your SQL
builder's literal/escape helper) so values are passed as parameters instead of
directly injected into the raw SQL; update the sql.raw invocation that
references enumValues to use these bindings.
In `@packages/testtools/src/client.ts`:
- Around line 289-295: The cleanup for the MySQL callback-style pool is using
mysqlPool.end() which is not promise-aware; update the teardown in the provider
=== 'mysql' branch to call the promise-wrapped end via mysqlPool.promise().end()
so the await actually waits for the pool to close (keep the existing use of
mysqlPool.promise().query(...) for queries and change only the final end() call
on the mysqlPool created by createMysqlPool with TEST_MYSQL_CONFIG, ensuring
dbName invariant handling remains unchanged).
In `@tests/e2e/orm/client-api/raw-query.test.ts`:
- Around line 20-22: The comparison in function ref uses an unnecessary and
unsafe cast ('mysql' as any); remove the as any cast and compare directly to the
string literal so TypeScript uses the existing DataSourceProviderType on
client.$schema.provider.type (i.e., update the conditional in ref to use
client.$schema.provider.type === 'mysql' and keep the sql.raw branches intact).
In `@tests/e2e/orm/plugin-infra/ext-query-args.test.ts`:
- Line 126: Remove the unnecessary type assertion in the isMySql check: change
the comparison that currently uses ('mysql' as any) to a plain 'mysql' string
literal so the expression reads like const isMySql = db.$schema.provider.type
=== 'mysql'; update the code around the isMySql variable (referencing
db.$schema.provider.type and isMySql) to match other tests and production usage
without the cast.
♻️ Duplicate comments (1)
packages/server/test/api/rpc.test.ts (1)
257-270: Sameas anypattern as above.Same observation as line 170 - the type assertion works around a type mismatch. Consider a unified fix if addressing the underlying type definitions.
🧹 Nitpick comments (32)
tests/e2e/orm/policy/policy-functions.test.ts (1)
17-19: Optional: DRY up repeated provider check.The
['sqlite', 'mysql'].includes(...)branch is repeated three times. Consider extracting a small helper to keep the test intent focused and reduce duplication.♻️ Possible refactor
describe('policy functions tests', () => { + const isCaseInsensitiveProvider = (db: { $schema: { provider: { type: string } } }) => + ['sqlite', 'mysql'].includes(db.$schema.provider.type); + it('supports contains case-sensitive', async () => { @@ - if (['sqlite', 'mysql'].includes(db.$schema.provider.type)) { + if (isCaseInsensitiveProvider(db)) { // sqlite and mysql are always case-insensitive await expect(db.foo.create({ data: { string: 'Acd' } })).toResolveTruthy(); } else { @@ - if (['sqlite', 'mysql'].includes(db.$schema.provider.type)) { + if (isCaseInsensitiveProvider(db)) { // sqlite and mysql are always case-insensitive await expect(db.foo.create({ data: { string: 'Acd' } })).toResolveTruthy(); } else { @@ - if (['sqlite', 'mysql'].includes(db.$schema.provider.type)) { + if (isCaseInsensitiveProvider(db)) { // sqlite and mysql are always case-insensitive await expect(db.$setAuth({ id: 'user1', name: 'Abc' }).foo.create({ data: {} })).toResolveTruthy(); } else {Also applies to: 54-56, 96-98
tests/e2e/orm/client-api/mixin.test.ts (1)
78-78: Redundant case handling and prefertest()overmatch()for boolean assertions.The code applies
.toLowerCase()and then uses thei(case-insensitive) flag in the regex — only one is needed. Additionally,toSatisfyexpects a boolean, soRegExp.test()is more appropriate thanString.match().✨ Suggested cleanup
- ).rejects.toSatisfy((e) => e.cause.message.toLowerCase().match(/(constraint)|(duplicate)/i)); + ).rejects.toSatisfy((e) => /constraint|duplicate/i.test(e.cause.message));tests/e2e/orm/client-api/create-many-and-return.test.ts (2)
18-21: Consider usingit.skipIffor better test reporting and remove the type assertion.The
as anycast suggests the type system doesn't recognize'mysql'as a valid provider type yet—this should be addressed at the type definition level as part of this PR's MySQL support.Additionally, using an early
returnmakes skipped tests appear as "passed" in test reports. Vitest'sit.skipIfprovides clearer visibility:♻️ Suggested refactor using vitest's skipIf
- it('works with toplevel createManyAndReturn', async () => { - if (client.$schema.provider.type === ('mysql' as any)) { - // mysql doesn't support createManyAndReturn - return; - } + it.skipIf(process.env.TEST_PROVIDER === 'mysql')( + 'works with toplevel createManyAndReturn', + async () => {Alternatively, if the runtime check is required, consider extracting a helper:
const isMysql = () => client.$schema.provider.type === 'mysql';This would require updating the provider type definitions to include
'mysql'as a valid literal.
67-70: Duplicated guard pattern—consider consolidating.Same concerns as the earlier guard. Since the entire test suite is for
createManyAndReturn, consider wrapping the wholedescribeblock conditionally or usingdescribe.skipIfto avoid repeating this guard in every test case.tests/e2e/orm/client-api/update.test.ts (1)
1053-1053: Minor: Redundant case handling in regex assertion.The
iflag already makes the regex case-insensitive, so.toLowerCase()is unnecessary. Additionally, the parentheses in the regex pattern can be removed for simplicity.Suggested simplification
- ).rejects.toSatisfy((e) => e.cause.message.toLowerCase().match(/(constraint)|(duplicate)/i)); + ).rejects.toSatisfy((e) => e.cause.message.match(/constraint|duplicate/i));packages/server/test/api/rpc.test.ts (1)
161-170: Consider addressing the underlying type mismatch.The
as anyassertion bypasses type checking on the procedures configuration. While acceptable in test code, this could mask legitimate type errors if the procedure handler signatures evolve.If the type mismatch stems from recent changes to
createPolicyTestClient's type definitions, consider updating the type definitions to accommodate procedure options, or adding a more specific type annotation.tests/e2e/orm/query-builder/query-builder.test.ts (1)
6-7: Consider parameterizing this test for MySQL coverage.The timestamp format changes suggest MySQL compatibility considerations, but the test is hardcoded to SQLite. For a MySQL support PR, consider parameterizing the schema provider to validate query builder functionality against MySQL as well.
-describe('Client API tests', () => { - const schema = getSchema('sqlite'); +describe.each(['sqlite', 'mysql'] as const)('Client API tests (%s)', (provider) => { + const schema = getSchema(provider);As per coding guidelines, E2E tests should validate real-world schema compatibility.
tests/regression/test/v2-migrated/issue-1894.test.ts (1)
45-46: Good fix for MySQL compatibility; consider removing explicitid: 0.Using
r.idinstead of a hardcoded value correctly handles MySQL's autoincrement behavior. However, passingid: 0explicitly is still problematic since MySQL treats 0 for autoincrement columns as "generate next value" rather than inserting literal 0. For consistent cross-database behavior, consider letting the database generate the id:Suggested change
- const r = await db.a.create({ data: { id: 0 } }); + const r = await db.a.create({ data: {} }); await expect(db.c.create({ data: { a: { connect: { id: r.id } } } })).toResolveTruthy();tests/e2e/orm/plugin-infra/on-kysely-query.test.ts (1)
96-103: Timestamp format workaround is acceptable but could be more robust.The
replace('Z', '+00:00')approach works for MySQL 8.0+ but has some caveats:
- Depends on MySQL server accepting ISO 8601 with timezone offset in
DATETIME/TIMESTAMPcolumns- Behavior may vary with MySQL's
sql_modesettingsConsider using a database-agnostic format without timezone suffix:
const now = new Date().toISOString().slice(0, 23).replace('T', ' '); // 'YYYY-MM-DD HH:mm:ss.sss'Or leverage the ORM's
now()function if compatible with raw query builder context (per relevant snippet infunctions.ts).The inline query builder usage with
toOperationNode()is the correct pattern for building Kysely operations within hooks.tests/e2e/orm/client-api/name-mapping.test.ts (4)
88-89: Consider extracting a shared helper or reusing this variable across tests.The
as anycast suggests'mysql'isn't yet a recognized provider type literal. This is fine during the transition, but the same check is repeated inline in other test functions (lines 398, 427) rather than reusing a shared pattern.For consistency and maintainability, consider either:
- Extracting a helper function at the describe block level, or
- Updating the
SchemaTypeor provider type definition to include'mysql'so the cast becomes unnecessary.Example: shared helper at describe level
describe('Name mapping tests', () => { let db: ClientContract<SchemaType>; const isMysql = () => db.$schema.provider.type === ('mysql' as any); // ... use isMysql() throughout tests });
105-114: MySQL branch lacks verification of inserted data.The non-MySQL path asserts that the returned data matches expected values (id, email, role), but the MySQL branch only inserts without verifying the result. Consider adding a follow-up select to confirm the inserted values, ensuring equivalent test coverage for MySQL.
Proposed enhancement
} else { // mysql doesn't support returning, simply insert await db.$qb .insertInto('User') .values({ email: '[email protected]', role: 'ADMIN', }) .executeTakeFirst(); + + // Verify the insert by selecting the record + await expect( + db.$qb.selectFrom('User').selectAll().where('email', '=', '[email protected]').executeTakeFirst(), + ).resolves.toMatchObject({ + id: expect.any(Number), + email: '[email protected]', + role: 'ADMIN', + }); }
125-152: MySQL skips these test scenarios entirely.These tests validate the
returningclause with qualified column names ('User.id','User.email') andreturningAll(). Since MySQL doesn't support RETURNING, the entire block is skipped, leaving no equivalent validation for MySQL.If the goal is only to test RETURNING clause behavior, this is acceptable. However, if you also want to verify that name mapping works correctly for MySQL inserts, consider adding a MySQL-specific path that performs the insert and verifies via a follow-up select.
398-411: Inconsistent conditional pattern compared to other tests.The
'works with create'and'works with delete'tests haveelsebranches for MySQL that perform the operation withoutreturning. This test only uses anif (!mysql)guard with noelsebranch, creating inconsistency.For consistency and coverage, consider adding a MySQL path that performs the update and verifies via select, similar to the pattern used elsewhere.
Suggested pattern for consistency
if (db.$schema.provider.type !== ('mysql' as any)) { // ... existing non-MySQL tests ... + } else { + // MySQL doesn't support returning, verify via select + await db.$qb + .updateTable('User') + .set({ email: (eb) => eb.fn('upper', [eb.ref('email')]), role: 'USER' }) + .where('email', '=', '[email protected]') + .execute(); + + await expect( + db.$qb.selectFrom('User').select(['email', 'role']).where('email', '=', '[email protected]').executeTakeFirst(), + ).resolves.toMatchObject({ email: '[email protected]', role: 'USER' }); }tests/e2e/orm/policy/migrated/toplevel-operations.test.ts (1)
169-200: Add a post‑force‑update assertion for MySQL.After the forced update, the test doesn’t verify that the record actually moved to
id: '2'with the expected value. Adding a quick assertion makes the MySQL branch more robust and prevents silent regressions that could affect the subsequent upsert expectations.🧪 Suggested tweak
// force update await db.$unuseAll().model.update({ where: { id: '1' }, data: { id: '2', value: 3, }, }); + await expect( + db.$unuseAll().model.findUnique({ where: { id: '2' } }), + ).resolves.toMatchObject({ id: '2', value: 3 }); }packages/orm/src/client/crud/operations/base.ts (4)
516-534: Potential issue with composite primary keys containing auto-increment field.When a model has a composite primary key where one field is auto-increment and others are not, the current logic iterates through all ID fields and assigns
insertResult.insertIdto the first auto-increment field it finds, then falls back toupdatedDatafor others. This should work correctly, but consider adding a comment explaining this behavior for maintainability.Also,
insertResult.insertIdmay be abigintin MySQL. Ensure downstream code handles bothnumberandbiginttypes correctly.
1013-1027: Fallback to sequential creates for non-RETURNING dialects is correct but may have performance implications.For polymorphic models on MySQL, this falls back to N individual
createcalls instead of a single batch insert. This is necessary for correctness but could be slow for large batches. Consider documenting this limitation or logging a debug message when this path is taken.
1649-1657: Potential race condition between SELECT and UPDATE for non-RETURNING dialects.The code selects records to update, then executes the update, then queries them back. In a concurrent environment without an explicit transaction, another process could modify or delete these records between steps.
Since
updateManyis already withinsafeTransactionin most call paths, this may be acceptable. However, consider adding a comment documenting this assumption, or ensuring the transaction isolation level is sufficient.
1668-1681: Consider query efficiency for large batch updates without RETURNING.Building an OR of AND conditions for each updated record could generate very large queries when updating many rows. For example, updating 1000 records with 2 ID fields generates 2000+ conditions.
Consider using
INwith tuple comparison for databases that support it, or adding a reasonable limit with pagination for very large updates.packages/plugins/policy/src/policy-handler.ts (2)
288-288: Use the class-level expression builder for consistency.Line 288 creates a new
expressionBuilderinstance, but the class already has a reusablethis.ebfield (line 69). Consider usingthis.ebfor consistency.♻️ Suggested change
- const eb = expressionBuilder<any, any>(); - - // before update table is joined if fields from `before()` are used in post-update policies - const needsBeforeUpdateJoin = !!beforeUpdateInfo?.fields; + // before update table is joined if fields from `before()` are used in post-update policies + const needsBeforeUpdateJoin = !!beforeUpdateInfo?.fields;Then replace all occurrences of
ebwiththis.ebin this method (lines 304-318).
1343-1383: Consider explicit handling for AndNode and OrNode for code clarity.The
stripTableReferenceshelper relies on generic traversal to handleAndNodeandOrNode, which are actively produced bybuildPolicyFilterthroughconjunction()anddisjunction()utilities (lines 1078–1110). While the generic object traversal (lines 1366–1379) should correctly process these nodes via the'kind' in valuecheck (since all Kysely OperationNode types have akindproperty), explicitly handlingAndNodeandOrNodewould improve code maintainability and make the intent clearer:if (AndNode.is(node)) { return AndNode.create( this.stripTableReferences(node.left), this.stripTableReferences(node.right), ); } if (OrNode.is(node)) { return OrNode.create( this.stripTableReferences(node.left), this.stripTableReferences(node.right), ); }Add these checks before the generic traversal fallback to make the handling explicit and future-proof.
tests/e2e/orm/client-api/raw-query.test.ts (1)
5-7: Avoid shadowing thesqlhelper nameThe local
const sql = ...in two tests shadows the top-levelsqlimport used byref, which makes the flow a bit harder to scan. Renaming the local variable improves clarity.♻️ Suggested rename
- const sql = match(client.$schema.provider.type as DataSourceProviderType) + const statement = match(client.$schema.provider.type as DataSourceProviderType) .with('postgresql', () => `UPDATE "User" SET "email" = $1 WHERE "id" = $2`) .with('mysql', () => 'UPDATE `User` SET `email` = ? WHERE `id` = ?') .with('sqlite', () => 'UPDATE "User" SET "email" = ? WHERE "id" = ?') .exhaustive(); - await expect(client.$executeRawUnsafe(sql, '[email protected]', '1')).resolves.toBe(1); + await expect(client.$executeRawUnsafe(statement, '[email protected]', '1')).resolves.toBe(1);- const sql = match(client.$schema.provider.type as DataSourceProviderType) + const statement = match(client.$schema.provider.type as DataSourceProviderType) .with('postgresql', () => `SELECT "User"."id", "User"."email" FROM "User" WHERE "User"."id" = $1`) .with('mysql', () => 'SELECT `User`.`id`, `User`.`email` FROM `User` WHERE `User`.`id` = ?') .with('sqlite', () => 'SELECT "User"."id", "User"."email" FROM "User" WHERE "User"."id" = ?') .exhaustive(); - const users = await client.$queryRawUnsafe<{ id: string; email: string }[]>(sql, '1'); + const users = await client.$queryRawUnsafe<{ id: string; email: string }[]>(statement, '1');Also applies to: 46-50, 78-82
packages/testtools/package.json (1)
42-42: Consider consistency with other database drivers.
mysql2is added as a direct dependency, whilepgandbetter-sqlite3are peer dependencies. This inconsistency may cause confusion about the intended dependency model for database drivers in test tooling.If MySQL support is optional like PostgreSQL and SQLite, consider moving
mysql2topeerDependenciesfor consistency.Suggested change
"dependencies": { ... "kysely": "catalog:", - "mysql2": "catalog:", "prisma": "catalog:", ... }, "peerDependencies": { "better-sqlite3": "catalog:", + "mysql2": "catalog:", "pg": "catalog:" },Also applies to: 47-50
tests/regression/test/v2-migrated/issue-1681.test.ts (1)
28-31: Appropriate guard for MySQL's lack of RETURNING support.The conditional correctly gates
createManyAndReturnsince MySQL doesn't support theRETURNINGclause. This preserves test coverage for other providers while avoiding failures on MySQL.For improved test output visibility, consider using Vitest's
skipIfto make it clear in test results when this assertion is skipped:Alternative using test.skipIf for better test reporting
it.skipIf(db.$schema.provider.type === 'mysql')( 'createManyAndReturn with auth default', async () => { const r = await authDb.post.createManyAndReturn({ data: [{ title: 'Post2' }] }); expect(r[0].authorId).toBe(user.id); } );Note: This would require restructuring the test since
dbis created inside the test. The current inline approach is pragmatic given the test structure.tests/e2e/orm/policy/nonexistent-models.test.ts (1)
18-26: Consider explicitly checking for 'mysql' instead of using else.The current else branch catches any non-PostgreSQL/SQLite provider, which could silently succeed for future unsupported providers. Consider making the MySQL case explicit for clarity and safety.
♻️ Suggested improvement
- if (['postgresql', 'sqlite'].includes(dbRaw.$schema.provider.type)) { + const provider = dbRaw.$schema.provider.type; + if (provider === 'postgresql' || provider === 'sqlite') { await dbRaw.$executeRawUnsafe( `CREATE TABLE "Bar" ("id" TEXT PRIMARY KEY, "string" TEXT, "fooId" TEXT, FOREIGN KEY ("fooId") REFERENCES "Foo" ("id"));`, ); - } else { + } else if (provider === 'mysql') { await dbRaw.$executeRawUnsafe( `CREATE TABLE Bar (id VARCHAR(191) PRIMARY KEY, string VARCHAR(191), fooId VARCHAR(191), FOREIGN KEY (fooId) REFERENCES Foo (id));`, ); + } else { + throw new Error(`Unsupported provider: ${provider}`); }tests/e2e/orm/policy/migrated/view.test.ts (1)
39-45: Provider-specific view creation is correctly implemented.The branching handles PostgreSQL/SQLite's double-quoted identifiers and MySQL's unquoted identifiers appropriately. The fallback error for unsupported providers is good defensive practice.
Optional: Consider using backticks for MySQL identifiers
For consistency and safety with reserved words, you could use backticks in the MySQL version:
- await rawDb.$executeRaw`CREATE VIEW UserInfo as select User.id, User.name, User.email, User.id as userId, count(Post.id) as postCount from User left join Post on User.id = Post.authorId group by User.id;`; + await rawDb.$executeRaw`CREATE VIEW \`UserInfo\` as select \`User\`.\`id\`, \`User\`.\`name\`, \`User\`.\`email\`, \`User\`.\`id\` as \`userId\`, count(\`Post\`.\`id\`) as \`postCount\` from \`User\` left join \`Post\` on \`User\`.\`id\` = \`Post\`.\`authorId\` group by \`User\`.\`id\`;`;However, the current unquoted form works fine for this test's schema.
tests/e2e/orm/policy/crud/read.test.ts (1)
635-641: Guard appears redundant given explicit provider setting.The test specifies
{ provider: 'postgresql' }at line 635, so the check at lines 638-641 should never trigger unless the test infrastructure ignores the provider option. While the guard is defensive and harmless, consider whether it's necessary.If the intention is to allow this test to be skipped when running against a non-PostgreSQL database in CI (e.g., via environment variable override), this pattern makes sense. Otherwise, the guard could be removed since the provider is explicitly set.
packages/testtools/src/schema.ts (1)
43-47: Consider usingDataSourceProviderTypefor consistency.The
providerparameter uses an inline union type'sqlite' | 'postgresql' | 'mysql'instead of the importedDataSourceProviderType. This could cause drift if the canonical type is updated elsewhere.Suggested fix
function replacePlaceholders( schemaText: string, - provider: 'sqlite' | 'postgresql' | 'mysql', + provider: DataSourceProviderType, dbUrl: string | undefined, ) {packages/orm/src/client/helpers/schema-db-pusher.ts (2)
273-302: Duplicate enum value extraction logic.The logic for extracting enum values (handling
@mapattributes) is duplicated betweenpush()(lines 30-48) andmapFieldType()(lines 281-297). Consider extracting this into a shared helper method.♻️ Suggested refactor
+ private getEnumValues(enumDef: { fields?: Record<string, any>; values?: Record<string, string> }): string[] { + if (enumDef.fields) { + return Object.values(enumDef.fields).map((f) => { + const mapAttr = f.attributes?.find((a: any) => a.name === '@map'); + if (!mapAttr || !mapAttr.args?.[0]) { + return f.name; + } else { + const mappedName = ExpressionUtils.getLiteralValue(mapAttr.args[0].value); + invariant( + mappedName && typeof mappedName === 'string', + `Invalid `@map` attribute for enum field ${f.name}`, + ); + return mappedName; + } + }); + } else { + return Object.values(enumDef.values!); + } + }Then use
this.getEnumValues(enumDef)in both locations.
65-67: Consider making provider list a constant for maintainability.The provider check in
providerSupportsNativeEnumuses an inline array. As more providers are added, consider using a shared constant.packages/language/src/document.ts (1)
256-267: Consider consolidating with the SDK'sgetDataSourceProvider.A similar helper exists in
packages/sdk/src/ts-schema-generator.tsthat usesinvariantto throw on missing values. This version returnsundefinedinstead, which is appropriate here since validation has already ensured the datasource exists. The difference in error handling is intentional, but consider documenting this distinction or extracting a shared utility with configurable error behavior.packages/language/src/constants.ts (1)
37-41: Consider adding aconstassertion for stricter typing.Adding
as constwould provide stricter type inference, allowing consumers to use the literal types rather thanstring[].💡 Optional type improvement
/** * Database providers that support list field types. */ -export const DB_PROVIDERS_SUPPORTING_LIST_TYPE = ['postgresql']; +export const DB_PROVIDERS_SUPPORTING_LIST_TYPE = ['postgresql'] as const;tests/e2e/orm/policy/migrated/multi-id-fields.test.ts (1)
127-148: Consider extracting MySQL detection to a test helper.The pattern
db.$schema.provider.type === 'mysql'is repeated multiple times. Consider extracting to a helper function for consistency and maintainability.💡 Example helper extraction
// In test utilities function isMySql(db: PolicyClient): boolean { return db.$schema.provider.type === 'mysql'; } // Or as a property on the test client const mysql = isMySql(db);
|
Claude finished @ymc9's task —— View job PR Review: MySQL Support ImplementationReviewing this substantial PR that adds MySQL support to ZenStack ORM. This is a significant feature addition with extensive changes across the codebase. Todo List
🔍 Comprehensive Review Results✅ Security & SQL Safety - Excellent
✅ Code Quality - High Standard
✅ Feature Completeness - ComprehensiveCore MySQL Features Implemented:
Advanced Features:
✅ Test Coverage - Thorough
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/orm/src/client/crud/operations/delete.ts`:
- Around line 35-38: The check for affected rows uses a falsy test on
deleteResult.numAffectedRows which treats 0n and undefined the same; change the
logic in the block that calls this.delete (around deleteResult handling) to use
nullish coalescing: compute an effectiveAffected = deleteResult.numAffectedRows
?? deleteResult.rows?.length and throw createNotFoundError(this.model) only if
effectiveAffected is 0 (or falsy after explicit numeric/coercion handling),
ensuring you fall back to deleteResult.rows.length when numAffectedRows is
undefined.
♻️ Duplicate comments (1)
packages/testtools/src/client.ts (1)
281-296:await mysqlPool.end()doesn’t await on callback pools.Line 294 uses the callback-style pool from
mysql2, whereend()is callback-based; theawaitwon’t actually wait for teardown. UsemysqlPool.promise().end()for reliable cleanup.🐛 Proposed fix
- await mysqlPool.end(); + await mysqlPool.promise().end();In mysql2's callback pool API (createPool from 'mysql2'), is end() callback-only and should promise().end() be used to await cleanup?
|
Claude finished @ymc9's task —— View job PR Review: MySQL Support ImplementationReviewing this substantial PR that adds MySQL support to ZenStack ORM. This is a significant feature addition with extensive changes across the codebase. Todo List
🔍 Comprehensive Review Results✅ Security & SQL Safety - Excellent
✅ Code Quality - High Standard
✅ Feature Completeness - ComprehensiveCore MySQL Features Implemented:
Advanced Features:
✅ Test Coverage - Thorough
|
|
Claude encountered an error —— View job PR Review: MySQL Support Implementation
|
|
Claude finished @ymc9's task —— View job PR Review: MySQL Support ImplementationReviewing this substantial PR that adds MySQL support to ZenStack ORM. This is a significant feature addition with extensive changes across the codebase. Todo List
🔍 Comprehensive Review Results✅ Security & SQL Safety - Excellent
✅ Code Quality - High Standard
✅ Feature Completeness - ComprehensiveCore MySQL Features Implemented:
Advanced Features:
✅ Test Coverage - Thorough
|
|
Claude finished @ymc9's task —— View job PR Review: MySQL Support ImplementationReviewing this substantial PR that adds MySQL support to ZenStack ORM. This is a significant feature addition with extensive changes across the codebase. Todo List
🔍 Comprehensive Review Results✅ Security & SQL Safety - Excellent
✅ Code Quality - High Standard
✅ Feature Completeness - ComprehensiveCore MySQL Features Implemented:
Advanced Features:
✅ Test Coverage - Thorough
|
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 (1)
packages/orm/src/client/crud/operations/base.ts (1)
1592-1594: Replacets-expect-errorwith explicit tuple typing forbuildIdFieldRefsspread operator.The
buildIdFieldRefs()method returns an array with dynamic length, buteb.refTuple()requires a tuple type. Replace the type suppressions with a type assertion to satisfy TypeScript's tuple requirements.Suggested fix
- eb.refTuple( - // `@ts-expect-error` - ...this.buildIdFieldRefs(kysely, model), - ), + eb.refTuple( + ...(this.buildIdFieldRefs(kysely, model) as [any, ...any[]]), + ),Apply this change to all three locations: lines 1592-1594, 1636-1638, and 2351-2353.
🤖 Fix all issues with AI agents
In `@packages/orm/src/client/crud/dialects/mysql.ts`:
- Around line 85-91: transformInput currently throws for any array when
Array.isArray(value) even if forArrayField is true, causing list field
create/update failures in MySQL; update the array handling in transformInput (in
packages/orm/src/client/crud/dialects/mysql.ts) so that when forArrayField is
true you return the array value unchanged (allow list-field processing
upstream), only stringify when type === 'Json' and forArrayField is false, and
keep throwing createNotSupportedError for other array cases.
In `@packages/orm/src/client/crud/operations/base.ts`:
- Around line 971-979: The thrown error for unsupported createManyAndReturn
should be a not-supported error, not createInternalError; replace the
createInternalError(...) call in the createManyAndReturn fallback with the
appropriate not-supported error factory (e.g., createNotSupportedError or
createUnsupportedError) and ensure the message still includes
this.dialect.provider and guidance about using multiple create() calls or
querying after insertion; also add or update the import for the chosen
not-supported error helper where base.ts currently references
createInternalError.
- Around line 1358-1363: The code assumes readUnique(...) returns an entity and
then accesses fromEntity[fk], which will throw if fromEntity is null; update the
block around the call to this.readUnique in the nested update/filter handling to
check if fromEntity is null and, if so, throw a proper not-found error (e.g., a
domain NotFoundError or the project's standard not-found helper) with context
about fromRelation.model and fromRelation.ids before iterating keyPairs and
assigning parentWhere[pk] = fromEntity[fk]; this prevents a TypeError and
surfaces a clear not-found error for the missing parent row.
- Around line 1625-1681: The pre-read and final read-back currently selectAll(),
leaking unrequested columns; change the pre-read select (the selectQuery built
before recordsToUpdate) to only select ID fields using buildIdFieldRefs(kysely,
model) / requireIdFields(this.schema, model) so you only fetch identifiers, and
change the final resultQuery (the query after computing updatedIds) to select
only the requested fieldsToReturn plus the ID fields (ensure ID fields are
always included) instead of selectAll(); update both the normal and
shouldFallbackToIdFilter branches (places using selectAll() and
buildIdFieldRefs) and keep using executeQuery for execution.
♻️ Duplicate comments (2)
packages/orm/src/client/crud/operations/base.ts (1)
1292-1296: MySQL UPDATE may report 0 affected rows even when the row exists.Line 1292 uses
numAffectedRowsto decide if the row exists. MySQL returns 0 when values are unchanged, which can incorrectly produceupdatedEntity = null. Please adjust to read back unconditionally or use a more reliable indicator.packages/orm/src/client/crud/dialects/mysql.ts (1)
451-463: Guard against empty rows in VALUES table.
rows.length === 0will produce invalid SQL. Add the same guard used in Postgres/SQLite.🧯 Suggested guard
override buildValuesTableSelect(fields: FieldDef[], rows: unknown[][]) { + if (rows.length === 0) { + throw createInvalidInputError('At least one row is required to build values table'); + } const cols = rows[0]?.length ?? 0;
🧹 Nitpick comments (5)
tests/regression/package.json (2)
9-12: Provider-specific scripts assume prior build.The main
testscript runstest:generate && tscbefore vitest, but the provider-specific scripts (test:sqlite,test:postgresql,test:mysql) run vitest directly. This is likely intentional for running tests against different databases after a single build, but could trip up developers who run them in isolation.Consider documenting this expectation or adding a
test:allscript that chains the provider tests after the build step (the AI summary suggests this may have been intended).
10-12: Consider cross-platform compatibility if Windows support is planned.The current scripts use Unix-style environment variable syntax (
VAR=value command) which will fail on Windows. The project currently runs CI only on Linux, but if Windows development support is needed in the future, consider using thecross-envpackage or a similar cross-platform solution.packages/plugins/policy/src/policy-handler.ts (2)
287-287: Usethis.ebfor consistency.This line creates a new
expressionBuilderinstance, but the class already has a centralizedthis.ebfield (line 68). Using the existing instance improves consistency.Suggested fix
- const eb = expressionBuilder<any, any>(); - - // before update table is joined if fields from `before()` are used in post-update policies - const needsBeforeUpdateJoin = !!beforeUpdateInfo?.fields; + // before update table is joined if fields from `before()` are used in post-update policies + const needsBeforeUpdateJoin = !!beforeUpdateInfo?.fields;Then replace
ebusages withthis.ebon lines 303-317.
1344-1384: Consider explicit handling for additional node types.The
stripTableReferenceshelper handlesReferenceNode,BinaryOperationNode, andParensNodeexplicitly, with a generic fallback for others. For improved reliability and maintainability, consider adding explicit cases for commonly used logical nodes (AndNode,OrNode,UnaryOperationNode):Suggested additions
if (ParensNode.is(node)) { return ParensNode.create(this.stripTableReferences(node.node)); } + + // Handle logical operators explicitly + if (AndNode.is(node)) { + return AndNode.create( + this.stripTableReferences(node.left), + this.stripTableReferences(node.right), + ); + } + + if (OrNode.is(node)) { + return OrNode.create( + this.stripTableReferences(node.left), + this.stripTableReferences(node.right), + ); + } + + if (UnaryOperationNode.is(node)) { + return UnaryOperationNode.create( + node.operator, + this.stripTableReferences(node.operand), + ); + } // For other node types, recursively process their children if they have anyYou would need to add
AndNode,OrNode, andUnaryOperationNodeto the imports fromkysely.packages/orm/src/client/crud/dialects/base-dialect.ts (1)
995-1098: Clarify whytakeis threaded throughbuildOrderBy.The base implementation doesn’t use it yet; a brief comment would prevent future confusion.
|
Claude finished @ymc9's task —— View job PR Review: MySQL Support ImplementationReviewing this substantial PR that adds MySQL support to ZenStack ORM. This is a significant feature addition with extensive changes across the codebase. Todo List
🔍 Comprehensive Review Results✅ Security & SQL Safety - Excellent
✅ Code Quality - High Standard
✅ Feature Completeness - ComprehensiveCore MySQL Features Implemented:
Advanced Features:
✅ Test Coverage - Thorough
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/orm/src/client/crud/dialects/mysql.ts`:
- Around line 276-288: The buildJsonArrayExistsPredicate implementation uses
MySQL JSON_TABLE which requires MySQL >= 8.0.4; update the code in
buildJsonArrayExistsPredicate to either assert/document that minimum server
version is 8.0.4+ or add a fallback path for older servers (for example, use
JSON_SEARCH/JSON_CONTAINS-based predicates or an IN-subquery approach) and
branch based on a detected/declared serverVersion flag; ensure you reference
buildJsonArrayExistsPredicate and the JSON_TABLE usage so callers know the
requirement or fallback is active.
In `@packages/orm/src/client/crud/operations/base.ts`:
- Around line 1633-1638: The code currently uses "@ts-expect-error" to suppress
type errors when spreading the result of buildIdFieldRefs into refTuple (seen
where selectQuery = selectQuery.where((eb) => eb(
eb.refTuple(...this.buildIdFieldRefs(kysely, model), ...) )) and two other
similar call sites); remove the `@ts-expect-error` and instead cast the result to
any at the call site (e.g., ...(this.buildIdFieldRefs(kysely, model) as any)) so
TypeScript no longer needs the suppression; apply this same change for all three
occurrences referencing buildIdFieldRefs being spread into refTuple.
♻️ Duplicate comments (5)
packages/orm/src/client/crud/operations/base.ts (3)
1292-1330: MySQL no‑op UPDATEs can be misclassified as “not found.”MySQL can return
numAffectedRows = 0when the row matches but values are unchanged, which makes this path setupdatedEntity = nulland later throwNotFound. That’s a false negative since you already loadedthisEntity. Consider always doing the read‑back (or usingthisEntitywhen read‑back is unnecessary) regardless of affected rows.🐛 Proposed fix (always read back on non‑RETURNING path)
- const updateResult = await this.executeQuery(kysely, updateQuery, 'update'); - if (!updateResult.numAffectedRows) { - // no rows updated - updatedEntity = null; - } else { - // collect id field/values from the original filter - const idFields = requireIdFields(this.schema, model); - const filterIdValues: any = {}; - for (const key of idFields) { - if (combinedWhere[key] !== undefined && typeof combinedWhere[key] !== 'object') { - filterIdValues[key] = combinedWhere[key]; - } - } - - // check if we are updating any id fields - const updatingIdFields = idFields.some((idField) => idField in updateFields); - - if (Object.keys(filterIdValues).length === idFields.length && !updatingIdFields) { - // if we have all id fields in the original filter and ids are not being updated, - // we can simply return the id values as the update result - updatedEntity = filterIdValues; - } else { - // otherwise we need to re-query the updated entity - - // replace id fields in the filter with updated values if they are being updated - const readFilter: any = { ...combinedWhere }; - for (const idField of idFields) { - if (idField in updateFields && updateFields[idField] !== undefined) { - // if id fields are being updated, use the new values - readFilter[idField] = updateFields[idField]; - } - } - const selectQuery = kysely - .selectFrom(model) - .select(fieldsToReturn as any) - .where(() => this.dialect.buildFilter(model, model, readFilter)); - updatedEntity = await this.executeQueryTakeFirst(kysely, selectQuery, 'update'); - } - } + await this.executeQuery(kysely, updateQuery, 'update'); + // MySQL can report 0 affected rows when values are unchanged; still read back. + // collect id field/values from the original filter + const idFields = requireIdFields(this.schema, model); + const filterIdValues: any = {}; + for (const key of idFields) { + if (combinedWhere[key] !== undefined && typeof combinedWhere[key] !== 'object') { + filterIdValues[key] = combinedWhere[key]; + } + } + + // check if we are updating any id fields + const updatingIdFields = idFields.some((idField) => idField in updateFields); + + if (Object.keys(filterIdValues).length === idFields.length && !updatingIdFields) { + updatedEntity = filterIdValues; + } else { + const readFilter: any = { ...combinedWhere }; + for (const idField of idFields) { + if (idField in updateFields && updateFields[idField] !== undefined) { + readFilter[idField] = updateFields[idField]; + } + } + const selectQuery = kysely + .selectFrom(model) + .select(fieldsToReturn as any) + .where(() => this.dialect.buildFilter(model, model, readFilter)); + updatedEntity = await this.executeQueryTakeFirst(kysely, selectQuery, 'update'); + }MySQL UPDATE affected rows behavior when SET assigns existing values; how does affected rows differ from matched rows?
1358-1363: Guard against null parent in nested update filters.
readUniquecan returnnull; dereferencingfromEntity[fk]will throw a TypeError. Convert this to a proper not‑found error.🛡️ Suggested guard
if (ownedByModel) { const fromEntity = await this.readUnique(kysely, fromRelation.model, { where: fromRelation.ids, }); + if (!fromEntity) { + throw createNotFoundError(fromRelation.model); + } for (const { fk, pk } of keyPairs) { parentWhere[pk] = fromEntity[fk]; } } else {
1625-1627: Non‑RETURNING updateMany should respectfieldsToReturn.The fallback path selects and returns all columns. This can leak fields callers didn’t request. Prefer selecting only IDs in the pre‑read, then
fieldsToReturn(plus IDs) in the final read‑back.💡 Proposed adjustment
- let selectQuery = kysely.selectFrom(model).selectAll(); + const idFields = requireIdFields(this.schema, model); + let selectQuery = kysely.selectFrom(model).select(idFields as any); ... - const resultQuery = kysely - .selectFrom(model) - .selectAll() + const resultQuery = kysely + .selectFrom(model) + .select(Array.from(new Set([...idFields, ...(fieldsToReturn ?? [])])) as any)Also applies to: 1669-1672
packages/orm/src/client/crud/dialects/mysql.ts (2)
62-91: Array inputs for list fields still throw.
WhenforArrayFieldis true and the field is a non-Json list, theArray.isArraybranch throws, which will make create/update on list fields fail.
451-475: Guard against emptyrowsin VALUES table generation.
Withrows.length === 0, the SQL can become invalid whenfields.length === 0. Add an explicit empty-rows check and throw a validation error.
🧹 Nitpick comments (1)
packages/orm/src/client/crud/dialects/mysql.ts (1)
295-297: Resolve the TODO for@db.xtype mapping.
Happy to help draft the mapping logic if you want to address it in this PR or spin it into a follow-up task.
|
Claude finished @ymc9's task —— View job PR Review: MySQL Support ImplementationReviewing this substantial PR that adds MySQL support to ZenStack ORM. This is a significant feature addition with extensive changes across the codebase. Todo List
🔍 Comprehensive Review Results✅ Security & SQL Safety - Excellent
✅ Code Quality - High Standard
✅ Feature Completeness - ComprehensiveCore MySQL Features Implemented:
Advanced Features:
✅ Test Coverage - Thorough
|
|
Claude finished @ymc9's task —— View job PR Review: MySQL Support ImplementationReviewing this substantial PR that adds MySQL support to ZenStack ORM. This is a significant feature addition with extensive changes across the codebase. Todo List
🔍 Comprehensive Review Results✅ Security & SQL Safety - Excellent
✅ Code Quality - High Standard
✅ Feature Completeness - ComprehensiveCore MySQL Features Implemented:
Advanced Features:
✅ Test Coverage - Thorough
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/orm/src/client/executor/zenstack-query-executor.ts`:
- Around line 675-684: The ensureProperQueryResult method currently fills
numAffectedRows for mutations using BigInt(result.rows.length), which is
incorrect for non-RETURNING mutations where rows is empty (e.g., MySQL); update
ensureProperQueryResult (and its use of isMutationNode) to only infer
numAffectedRows from result.rows when result.rows.length > 0, otherwise leave
numAffectedRows as undefined so getInsertIds can correctly detect single-row
inserts via the query node; ensure the inferred value is still converted to
BigInt when used.
In `@packages/plugins/policy/src/policy-handler.ts`:
- Around line 868-891: The values table used for pre-create policy checks
currently includes non-physical fields and should exclude computed/inherited
fields; update the allFields construction (the
Object.values(QueryUtils.requireModel(this.client.$schema, model).fields) step
used before calling this.dialect.buildValuesTableSelect) to filter out
definitions where def.computed or def.inherited (or whatever flag your schema
uses for virtual fields) is true, so buildValuesTableSelect(allFields,
[allValues]) and the subsequent preCreateCheck/selectFrom logic only use real
columns when evaluating buildPolicyFilter for 'create'.
- Around line 1344-1361: TableReferenceStripper currently strips qualifiers from
every ReferenceNode; narrow it so only references to the mutation table are
stripped: add a constructor parameter or property to TableReferenceStripper
(e.g., mutationTableName) and in transformReference check that node.column is a
ColumnNode and that its table identifier matches mutationTableName before
returning a stripped ReferenceNode; otherwise call
super.transformReference(node) so references in joins/subqueries remain
qualified. Update stripTableReferences to pass the mutation table name into new
TableReferenceStripper(...).
♻️ Duplicate comments (3)
packages/orm/src/client/crud/dialects/mysql.ts (3)
87-93: List fields cannot be created or updated in MySQL.When
forArrayFieldis true (for list fields likeString[],Int[]), the type parameter is the element type (e.g.,'String'), not'Json'. The current condition only allows arrays whentype === 'Json', causing all list field operations to throw.Suggested fix
} else if (Array.isArray(value)) { - if (type === 'Json') { - // type-def arrays reach here + if (forArrayField) { + // list fields are stored as JSON in MySQL + return this.eb.cast(this.eb.val(JSON.stringify(value)), 'json'); + } else if (type === 'Json') { + // scalar Json field storing an array return JSON.stringify(value); } else { throw createNotSupportedError(`MySQL does not support array literals`); }
277-289: Document MySQL 8.0.4+ requirement for JSON_TABLE.
JSON_TABLEis only available in MySQL 8.0.4+. Users on MySQL 5.7 will encounter runtime errors when this code path is executed (e.g., when usingsomefilters on JSON arrays). Consider documenting the minimum MySQL version requirement or providing a fallback usingJSON_SEARCH/JSON_CONTAINS.
330-342: Add explicit guard for emptyrowsarray.When
rowsis empty,colsbecomes0. Iffieldsis non-empty, the error message "Number of fields must match number of columns in each row" is misleading. If both are empty, invalid SQL is generated. Add an explicit check for clarity.Suggested fix
override buildValuesTableSelect(fields: FieldDef[], rows: unknown[][]) { + if (rows.length === 0) { + throw createInvalidInputError('Cannot build VALUES table with no rows'); + } + const cols = rows[0]?.length ?? 0;
|
Claude finished @ymc9's task —— View job PR Review: MySQL Support ImplementationReviewing this substantial PR that adds MySQL support to ZenStack ORM. This is a significant feature addition with extensive changes across the codebase. Todo List
🔍 Comprehensive Review Results✅ Security & SQL Safety - Excellent
✅ Code Quality - High Standard
✅ Feature Completeness - ComprehensiveCore MySQL Features Implemented:
Advanced Features:
✅ Test Coverage - Thorough
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/plugins/policy/src/policy-handler.ts`:
- Around line 1342-1345: In stripTableReferences, the local parameter is
modelName but the code calls new TableReferenceStripper().strip(node, tableName)
causing a runtime error; update the call in the stripTableReferences method to
pass modelName (i.e., call TableReferenceStripper.strip(node, modelName)) so the
OperationNode stripping uses the correct variable and not an undefined
tableName.
🧹 Nitpick comments (3)
packages/plugins/policy/src/policy-handler.ts (3)
287-287: Use the class-levelthis.ebinstead of creating a local instance.A local
expressionBuilderis created here, but the class already hasthis.ebas a private field. This is inconsistent with the rest of the refactoring in this PR.♻️ Suggested fix
- const eb = expressionBuilder<any, any>(); - // before update table is joined if fields from `before()` are used in post-update policies const needsBeforeUpdateJoin = !!beforeUpdateInfo?.fields;Then replace usages of
ebwiththis.ebin the subsequent code (lines 303-317).
1356-1362: Add defensive check fornode.tabletype before accessing nested properties.The access pattern
node.table?.table.identifier.nameassumesnode.tableis aTableNode, but if it's anAliasNodeor other type, this could fail. Consider usingTableNode.is()for type safety, consistent with patterns used elsewhere in this file.♻️ Suggested fix
protected override transformReference(node: ReferenceNode) { - if (ColumnNode.is(node.column) && node.table?.table.identifier.name === this.tableName) { + if ( + ColumnNode.is(node.column) && + node.table && + TableNode.is(node.table) && + node.table.table.identifier.name === this.tableName + ) { // strip the table part return ReferenceNode.create(this.transformNode(node.column)); } return super.transformReference(node); }
1364-1365: Misplaced region comment.The
//#endregion`` comment on line 1364 appears after theTableReferenceStripperclass but likely belongs inside the `PolicyHandler` class (before line 1346) to properly close the "helpers" region.♻️ Suggested fix
private stripTableReferences(node: OperationNode, modelName: string): OperationNode { return new TableReferenceStripper().strip(node, modelName); } + + // `#endregion` } class TableReferenceStripper extends OperationNodeTransformer { // ... - - // `#endregion` }
|
Claude finished @ymc9's task —— View job PR Review: MySQL Support Implementation
|
Summary by CodeRabbit
New Features
Behavior Changes
Tests
Chores
Public Types
✏️ Tip: You can customize this high-level summary in your review settings.