feat(studio): deterministic Drizzle ↔ SQL converters (#162) - #225
Merged
Conversation
Two-pane converter mounted as a third tab in the cockpit: input editor on the left, read-only output on the right, with a direction control, dialect picker, swap (carries the output into the input) and copy. Conversion is debounced at 300ms and re-runs on direction/dialect change; failures render as a ConvertError list in the output pane and warnings as a dismissable strip. State/dispatch logic is a pure module (converter-state) covered by vitest. Wired against the contract types via placeholder converters — the single import site in use-converter carries the swap TODO.
Implements the contract's DrizzleToSql for both surfaces: - SCHEMA: parse-drizzle-schema → SchemaIR → DDL, reusing the emitters from migration/generate-sql (quoting, SQL_TYPES, SERIAL/AUTOINCREMENT, SQLite inline FKs) rather than copying them; buildCreateTableSql/createIndexSql/ addForeignKeySql/quote are now exported and buildCreateTableSql takes an optional inline-FK list (existing callers unchanged). - QUERY: static TypeScript-AST parse (never executes user code) into the contract's QueryIR, emitted by the standalone converters/emit-sql module so round-trip tests can pivot through the IR. Surface is auto-detected when unpinned; unsupported constructs come back as ok:false with a named unsupported-construct error and nothing ever throws.
Implements the contract's SqlToDrizzle direction with a hand-rolled lexer
and recursive-descent parser — no new runtime dependency. The supported
grammar is small and closed, and the contract's promises (exact error
codes, a 1-based line, byte-identical output for the same input) are
precisely what a general SQL parser would not give us.
SCHEMA surface: a DDL script (CREATE TABLE, CREATE [UNIQUE] INDEX,
ALTER TABLE … ADD) parses into the frozen SchemaIR and emits idiomatic
pgTable/mysqlTable/sqliteTable declarations — camelCase properties with
explicit column names only when they differ, .primaryKey()/.notNull()/
.default()/.defaultNow()/.unique()/.references(), and a config callback
for composite keys, composite FKs and indexes. Referenced tables are
declared before the tables that reference them. The type map is the
deliberate inverse of SQL_TYPES (migration/generate-sql) and
BUILDER_TYPES (parsers/drizzle), so parseDrizzleSchema of the output
reproduces the schema the DDL described.
QUERY surface: a single DML statement parses into the contract's QueryIR
and emits a db.select()/insert()/update()/delete() chain with the
drizzle-orm operator imports it needs. Placeholders ($1/?/:name) become
QueryValue params and emit as bare identifiers; RETURNING * uses the
{ column: '*' } sentinel shared with the Drizzle → SQL direction.
Anything outside the surface (HAVING, window functions, CTEs, UNION,
aggregates or aliases in the select list, subqueries outside EXISTS)
returns ok:false with unsupported-construct naming it. Lossy but
severable mappings (unknown types, CHECK constraints, partial-index
predicates, dropped aliases) convert with a warning instead.
…#162) - swap mock-converters for convertDrizzleToSql/convertSqlToDrizzle in use-converter.ts and delete the mocks - round-trip suite over both fixture sets: sql→drizzle→sql byte-exact, drizzle→sql→drizzle byte-exact for queries and IR-equal for schemas (SchemaIR sorts tables/columns, so DDL cannot preserve declaration order) - parse-drizzle-schema: read primaryKey({ autoIncrement: true }) and timestamp({ withTimezone: true }) - generate-sql typeToken: carry typeParams for vector and override dialect default widths (mysql VARCHAR(255) no longer swallows explicit lengths)
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @remcostoeten, your pull request is larger than the review limit of 150000 diff characters
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds deterministic Drizzle ↔ SQL schema and query conversion for PostgreSQL, MySQL, and SQLite, integrates it into the ORM cockpit, and adds fixtures, round-trip validation, UI state tests, and documentation. ChangesConverter contracts and parsing primitives
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ConverterPanel
participant useConverter
participant DrizzleSqlConverter
participant SqlDrizzleConverter
participant ConverterEditor
User->>ConverterPanel: enter source and choose direction/dialect
ConverterPanel->>useConverter: dispatch input or direction change
useConverter->>DrizzleSqlConverter: convertDrizzleToSql(source, options)
useConverter->>SqlDrizzleConverter: convertSqlToDrizzle(source, options)
DrizzleSqlConverter-->>useConverter: output, warnings, or errors
SqlDrizzleConverter-->>useConverter: output, warnings, or errors
useConverter-->>ConverterPanel: conversion result
ConverterPanel->>ConverterEditor: render output or errors
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
packages/studio/src/features/orm-cockpit/converters/drizzle-to-sql.ts (2)
71-89: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRe-parses the same source a second time.
convertDrizzleToSqlalready builtfilefromsourceat line 53;convertSchemaparses it again at line 84. Pass the existingts.SourceFilethrough instead — this runs on every debounced keystroke in the converter UI.♻️ Proposed refactor
- return surface === 'schema' ? convertSchema(source, options) : convertQuery(file, options) + return surface === 'schema' + ? convertSchema(source, file, options) + : convertQuery(file, options) } -function convertSchema(source: string, options: ConvertOptions): ConvertResult { +function convertSchema( + source: string, + file: ts.SourceFile, + options: ConvertOptions +): ConvertResult { const parsed = parseDrizzleSchema([{ path: INPUT_PATH, text: source }], options.dialect) if (parsed.ir.tables.length === 0) { return failure({ code: 'parse-error', message: 'no Drizzle table definitions were found', }) } const warnings = [...parsed.warnings] - const file = ts.createSourceFile(INPUT_PATH, source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS) const ir = resolveForeignKeyTables(parsed.ir, tableNameByVar(file), warnings)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/orm-cockpit/converters/drizzle-to-sql.ts` around lines 71 - 89, Update the convertDrizzleToSql-to-convertSchema flow to pass the already-created ts.SourceFile into convertSchema, then use that instance for tableNameByVar instead of creating another source file from source. Remove the redundant ts.createSourceFile call inside convertSchema while preserving schema parsing and output behavior.
119-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid relying on internal
SourceFile.parseDiagnostics.
parseDiagnosticsis an internal compiler property; future TypeScript versions can rename or drop it without notice. Use the public diagnostics APIs instead, or add a fallback/version pin comment if this is intentionally compiler-version-specific.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/orm-cockpit/converters/drizzle-to-sql.ts` around lines 119 - 131, Update firstSyntaxError to obtain parse diagnostics through TypeScript’s public diagnostics API instead of reading the internal SourceFile.parseDiagnostics property. Preserve the existing first-diagnostic selection and ConvertError construction, including message flattening and one-based line reporting.packages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-typemap.ts (2)
13-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
TEntryduplicatesTTypeMapping.Same shape declared twice; alias it instead so the two can't drift.
-type TEntry = { builder: string; type: NormalizedType; note?: string } +type TEntry = TTypeMapping🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-typemap.ts` around lines 13 - 21, Replace the duplicate TEntry object type with an alias to TTypeMapping in the SQL-to-Drizzle type mapping definitions. Preserve the existing builder, type, and optional note shape while ensuring TEntry stays synchronized with TTypeMapping.
182-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe MySQL
CHAR(36)special case is a no-op.
MYSQL.CHARis already{ builder: 'char', type: 'varchar' }, so lines 187-189 return exactly what the table lookup below would return. Either drop the branch or make it actually differ (e.g. map to auuid-ish note).♻️ Drop the redundant branch
if (dialect === 'mysql' && token === 'TINYINT' && args[0] === '1') { return { builder: 'boolean', type: 'bool' } } - if (dialect === 'mysql' && token === 'CHAR' && args[0] === '36') { - return { builder: 'char', type: 'varchar' } - } const entry = TABLES[dialect][token]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-typemap.ts` around lines 182 - 197, Remove the redundant MySQL CHAR(36) special-case branch from mapSqlType, since TABLES[dialect][token] already returns the same mapping. Preserve the existing TINYINT(1) boolean handling and fallback table lookup behavior.packages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-emit-schema.ts (1)
92-96: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueTables absent from
plan.tableOrderare silently dropped.
sequenceis derived purely fromdeclarationOrder, so any table present inir.tablesbut missing fromtableOrdernever reachesemitted. The current DDL parser always produces both in lockstep, butemitSchemais exported and takes an arbitraryTSchemaPlan, so this fails silently rather than loudly.🛡️ Append tables missing from the declaration order
const byName = new Map(tables.map((table) => [table.name, table])) - const sequence = declarationOrder.filter((name) => byName.has(name)) + const declared = declarationOrder.filter((name) => byName.has(name)) + const sequence = [ + ...declared, + ...tables.map((table) => table.name).filter((name) => !declared.includes(name)) + ]Note:
emitTable(Line 128-135) has the same shape — columns missing fromplan.columnOrderare filtered out of the emitted table.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-emit-schema.ts` around lines 92 - 96, Update orderTables to append every table from tables that is absent from declarationOrder after processing the declared sequence, preserving the existing order for explicitly declared tables and ensuring none are silently dropped. Apply the same fallback in emitTable so columns missing from plan.columnOrder are also emitted, using the existing table and column symbols.packages/studio/src/features/orm-cockpit/converters/emit-sql.ts (2)
136-144: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMySQL gets a
RETURNINGclause with no warning.The SQL→Drizzle direction flags this (
sql-to-drizzle-dml.tsLine 474-478); this direction emitsRETURNING …into MySQL SQL silently. Threadwarningsthrough and mirror the message so both directions behave the same.♻️ Warn on MySQL RETURNING
-function returningSql(returning: ColumnRef[], dialect: Dialect): string { +function returningSql(returning: ColumnRef[], dialect: Dialect, warnings: string[]): string { if (returning.length === 0) { return '' } + if (dialect === 'mysql') { + warnings.push('MySQL has no RETURNING clause; the emitted statement will not run on MySQL.') + } if (returning.length === 1 && returning[0].column === '*') { return ' RETURNING *' }Update the three call sites (Lines 114, 125, 133) to pass
warnings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/orm-cockpit/converters/emit-sql.ts` around lines 136 - 144, Update returningSql and its three call sites to accept and pass the existing warnings collection, then emit the same MySQL RETURNING warning used by sql-to-drizzle-dml.ts before generating any RETURNING clause. Preserve current SQL output for supported dialects and empty or wildcard returning selections.
234-236: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winIdentifier quoting doesn't escape the quote character.
The lexer decodes
""/``pairs back to a single character, so an identifier legitimately containing `"` or` `` round-trips into a broken (and, for a hand-pasted schema, injectable) quoted identifier. Double it on the way out.🛡️ Escape by doubling
function quote(name: string, dialect: Dialect): string { - return dialect === 'mysql' ? `\`${name}\`` : `"${name}"` + return dialect === 'mysql' + ? `\`${name.replace(/`/g, '``')}\`` + : `"${name.replace(/"/g, '""')}"` }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/orm-cockpit/converters/emit-sql.ts` around lines 234 - 236, Update quote to escape embedded identifier delimiters by doubling the active quote character before wrapping the name: double backticks for MySQL and double double-quotes for other dialects. Preserve the existing dialect-specific delimiters and ensure identifiers containing quote characters round-trip safely.packages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-dml.ts (1)
713-729: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
unknown table or aliasalways reports line 1.
TRefSitedrops the source line, so this parse error points at the top of the file regardless of where the bad qualifier is — the one diagnostic in this module that doesn't honour the contract's line promise.parseColumnRefalready has the line in hand.♻️ Carry the line through `TRefSite`
type TRefSite = { ref: ColumnRef /** 0 for the outer statement, 1+ inside an EXISTS subquery. */ depth: number + line: number }function parseColumnRef(stream: TokenStream, state: TParseState): ColumnRef { let qualifier: string | undefined + const line = stream.line let column = stream.identifier() @@ - state.refs.push({ ref, depth: state.depth }) + state.refs.push({ ref, depth: state.depth, line })if (resolved === undefined) { - parseError(`unknown table or alias "${site.ref.table}"`, 1) + parseError(`unknown table or alias "${site.ref.table}"`, site.line) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-dml.ts` around lines 713 - 729, Carry the source line from parseColumnRef into each TRefSite record, then use that stored line when resolveRefs reports an unknown table or alias instead of the hardcoded line 1. Update the TRefSite type and all construction sites consistently while preserving existing reference resolution behavior.packages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-ddl.ts (1)
713-723: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSilently dropping a same-named index can hide a real definition.
The dedupe is right for the
UNIQUEcolumn constraint →CREATE UNIQUE INDEXoverlap, but it also swallows an index that shares a name yet declares different columns/uniqueness. A warning keeps the drop visible.♻️ Warn when the existing definition differs
const existing = draft.indexes.find(function (index) { return index.name === name }) if (existing) { + if (existing.unique !== unique || existing.columns.join(',') !== columns.join(',')) { + warnings.push( + `line ${line}: index "${name}" is already defined on "${tableName}" with different columns; the CREATE INDEX definition was dropped` + ) + } return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-ddl.ts` around lines 713 - 723, Update the duplicate-index handling in the parser around the existing index lookup: retain the silent return only when the existing index has the same columns and uniqueness, and emit a warning when its definition differs before returning. Use the existing parsing/logging warning mechanism and preserve the current deduplication behavior for equivalent UNIQUE constraint overlaps.__tests__/packages/studio/orm-cockpit/converters/round-trip.test.ts (1)
18-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant destructure-then-reassign of
rawType.
{ rawType: _rawType, ...column }stripsrawTypeonly to immediately add it back as''on the next line. This can be simplified to a single spread.🧹 Proposed simplification
- columns: table.columns.map(({ rawType: _rawType, ...column }) => ({ - ...column, - rawType: '', - })), + columns: table.columns.map((column) => ({ + ...column, + rawType: '', + })),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/packages/studio/orm-cockpit/converters/round-trip.test.ts` around lines 18 - 23, In the columns mapping within the round-trip test, simplify the object construction by removing the redundant rawType destructuring and spreading the column directly before overriding rawType with an empty string. Preserve all other column properties and the existing tables mapping behavior.__tests__/packages/studio/orm-cockpit/converters/fixtures/drizzle-to-sql-fixtures.ts (1)
3-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnify the two near-duplicate fixture types.
ConverterFixtureandTConverterFixturedescribe overlapping fixture shapes but use inconsistent naming conventions and importDialectfrom two different modules, with no shared root type.
__tests__/packages/studio/orm-cockpit/converters/fixtures/drizzle-to-sql-fixtures.ts#L3-L8: extract a shared base fixture type (or extend it) instead of redefiningConverterFixturelocally.__tests__/packages/studio/orm-cockpit/converters/fixtures/sql-to-drizzle-fixtures.ts#L10-L20: renameTConverterFixtureto drop the HungarianTprefix and align itsDialectimport path with the shared type/module.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/packages/studio/orm-cockpit/converters/fixtures/drizzle-to-sql-fixtures.ts` around lines 3 - 8, Unify the duplicate fixture types by extracting or reusing a shared base fixture type in __tests__/packages/studio/orm-cockpit/converters/fixtures/drizzle-to-sql-fixtures.ts:3-8, then update __tests__/packages/studio/orm-cockpit/converters/fixtures/sql-to-drizzle-fixtures.ts:10-20 to rename TConverterFixture without the T prefix and use the shared type/module’s Dialect import; update all references to the renamed type.packages/studio/src/features/orm-cockpit/components/converter-state.ts (1)
39-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep
ConversionRequestunexported.
ConversionRequestis only referenced insideconverter-state.ts, so exporting it violates the guideline to export TypeScript types only when they are consumed outside their defining module.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/orm-cockpit/components/converter-state.ts` at line 39, Remove the export modifier from the ConversionRequest type declaration in converter-state.ts, keeping the type available for internal references within that module only.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/studio/src/features/orm-cockpit/converters/drizzle-to-sql-query.ts`:
- Around line 583-594: Update parseComparisonValue to treat a two-part property
access as a column reference only when its root identifier is present in
ctx.symbols; otherwise route it through parseValue so unknown roots such as
session.role become bound parameters. Preserve the existing join-style handling
for known table roots, and add the requested warning for query-only snippets if
verbatim identifier behavior must remain supported.
In `@packages/studio/src/features/orm-cockpit/converters/emit-sql.ts`:
- Around line 82-88: Update the SQL emission logic in the query converter so
OFFSET without LIMIT produces dialect-valid SQL for MySQL and SQLite by adding
the required sentinel LIMIT before the OFFSET. Preserve the existing explicit
query.limit behavior and Postgres output, and keep the current LIMIT/OFFSET
ordering.
In
`@packages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-naming.ts`:
- Around line 3-41: Expand the RESERVED set used by tableVarName and
isSafeIdentifier to include strict-mode and ES2017+ disallowed identifiers: let,
static, implements, interface, package, private, protected, public, and await.
Keep the existing reserved-word handling unchanged so generated identifiers such
as SQL table names never produce invalid strict-mode variable declarations.
---
Nitpick comments:
In
`@__tests__/packages/studio/orm-cockpit/converters/fixtures/drizzle-to-sql-fixtures.ts`:
- Around line 3-8: Unify the duplicate fixture types by extracting or reusing a
shared base fixture type in
__tests__/packages/studio/orm-cockpit/converters/fixtures/drizzle-to-sql-fixtures.ts:3-8,
then update
__tests__/packages/studio/orm-cockpit/converters/fixtures/sql-to-drizzle-fixtures.ts:10-20
to rename TConverterFixture without the T prefix and use the shared
type/module’s Dialect import; update all references to the renamed type.
In `@__tests__/packages/studio/orm-cockpit/converters/round-trip.test.ts`:
- Around line 18-23: In the columns mapping within the round-trip test, simplify
the object construction by removing the redundant rawType destructuring and
spreading the column directly before overriding rawType with an empty string.
Preserve all other column properties and the existing tables mapping behavior.
In `@packages/studio/src/features/orm-cockpit/components/converter-state.ts`:
- Line 39: Remove the export modifier from the ConversionRequest type
declaration in converter-state.ts, keeping the type available for internal
references within that module only.
In `@packages/studio/src/features/orm-cockpit/converters/drizzle-to-sql.ts`:
- Around line 71-89: Update the convertDrizzleToSql-to-convertSchema flow to
pass the already-created ts.SourceFile into convertSchema, then use that
instance for tableNameByVar instead of creating another source file from source.
Remove the redundant ts.createSourceFile call inside convertSchema while
preserving schema parsing and output behavior.
- Around line 119-131: Update firstSyntaxError to obtain parse diagnostics
through TypeScript’s public diagnostics API instead of reading the internal
SourceFile.parseDiagnostics property. Preserve the existing first-diagnostic
selection and ConvertError construction, including message flattening and
one-based line reporting.
In `@packages/studio/src/features/orm-cockpit/converters/emit-sql.ts`:
- Around line 136-144: Update returningSql and its three call sites to accept
and pass the existing warnings collection, then emit the same MySQL RETURNING
warning used by sql-to-drizzle-dml.ts before generating any RETURNING clause.
Preserve current SQL output for supported dialects and empty or wildcard
returning selections.
- Around line 234-236: Update quote to escape embedded identifier delimiters by
doubling the active quote character before wrapping the name: double backticks
for MySQL and double double-quotes for other dialects. Preserve the existing
dialect-specific delimiters and ensure identifiers containing quote characters
round-trip safely.
In `@packages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-ddl.ts`:
- Around line 713-723: Update the duplicate-index handling in the parser around
the existing index lookup: retain the silent return only when the existing index
has the same columns and uniqueness, and emit a warning when its definition
differs before returning. Use the existing parsing/logging warning mechanism and
preserve the current deduplication behavior for equivalent UNIQUE constraint
overlaps.
In `@packages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-dml.ts`:
- Around line 713-729: Carry the source line from parseColumnRef into each
TRefSite record, then use that stored line when resolveRefs reports an unknown
table or alias instead of the hardcoded line 1. Update the TRefSite type and all
construction sites consistently while preserving existing reference resolution
behavior.
In
`@packages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-emit-schema.ts`:
- Around line 92-96: Update orderTables to append every table from tables that
is absent from declarationOrder after processing the declared sequence,
preserving the existing order for explicitly declared tables and ensuring none
are silently dropped. Apply the same fallback in emitTable so columns missing
from plan.columnOrder are also emitted, using the existing table and column
symbols.
In
`@packages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-typemap.ts`:
- Around line 13-21: Replace the duplicate TEntry object type with an alias to
TTypeMapping in the SQL-to-Drizzle type mapping definitions. Preserve the
existing builder, type, and optional note shape while ensuring TEntry stays
synchronized with TTypeMapping.
- Around line 182-197: Remove the redundant MySQL CHAR(36) special-case branch
from mapSqlType, since TABLES[dialect][token] already returns the same mapping.
Preserve the existing TINYINT(1) boolean handling and fallback table lookup
behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d143a9d-f912-4d64-a4df-f3f3004b3c62
📒 Files selected for processing (31)
__tests__/packages/studio/orm-cockpit/converters/drizzle-to-sql.test.ts__tests__/packages/studio/orm-cockpit/converters/fixtures/drizzle-to-sql-fixtures.ts__tests__/packages/studio/orm-cockpit/converters/fixtures/sql-to-drizzle-fixtures.ts__tests__/packages/studio/orm-cockpit/converters/round-trip.test.ts__tests__/packages/studio/orm-cockpit/converters/sql-to-drizzle.test.ts__tests__/packages/studio/orm-cockpit/converters/ui-converter-state.test.tsdocs/features/drizzle-sql-converter.mdxdocs/features/index.mdxdocs/features/meta.jsondocs/meta.jsonpackages/studio/src/features/orm-cockpit/components/converter-editor.tsxpackages/studio/src/features/orm-cockpit/components/converter-panel.tsxpackages/studio/src/features/orm-cockpit/components/converter-state.tspackages/studio/src/features/orm-cockpit/components/orm-cockpit-panel.tsxpackages/studio/src/features/orm-cockpit/components/use-converter.tspackages/studio/src/features/orm-cockpit/converters/contract.tspackages/studio/src/features/orm-cockpit/converters/drizzle-to-sql-query.tspackages/studio/src/features/orm-cockpit/converters/drizzle-to-sql-schema.tspackages/studio/src/features/orm-cockpit/converters/drizzle-to-sql-symbols.tspackages/studio/src/features/orm-cockpit/converters/drizzle-to-sql.tspackages/studio/src/features/orm-cockpit/converters/emit-sql.tspackages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-ddl.tspackages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-dml.tspackages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-emit-query.tspackages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-emit-schema.tspackages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-lexer.tspackages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-naming.tspackages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-typemap.tspackages/studio/src/features/orm-cockpit/converters/sql-to-drizzle.tspackages/studio/src/features/orm-cockpit/migration/generate-sql.tspackages/studio/src/features/orm-cockpit/parsers/drizzle/parse-drizzle-schema.ts
Comment on lines
+583
to
+594
| function parseComparisonValue( | ||
| arg: ts.Expression | undefined, | ||
| ctx: TCtx, | ||
| ): QueryValue | { kind: 'column'; ref: ColumnRef } | null { | ||
| // On the right of a comparison, `table.column` is the join-style column | ||
| // reference; deeper accesses (`req.body.id`) are runtime values. | ||
| if (arg && ts.isPropertyAccessExpression(arg) && ts.isIdentifier(arg.expression)) { | ||
| const ref = parseColumnRef(arg, ctx) | ||
| return ref === null ? null : { kind: 'column', ref } | ||
| } | ||
| return parseValue(arg, ctx) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Any x.y on the right of a comparison becomes a column reference, even when x is not a known table.
eq(users.status, session.role) parses session.role as a column ref (parseColumnRef falls back to { table: 'session', column: 'role' } at lines 608-610), so the emitted SQL compares against a non-existent session.role column instead of binding a param. Gating on ctx.symbols keeps the join-style case working while sending unknown roots down the param path.
🛠️ Proposed fix
if (arg && ts.isPropertyAccessExpression(arg) && ts.isIdentifier(arg.expression)) {
+ if (!ctx.symbols.has(arg.expression.text)) {
+ return parseValue(arg, ctx)
+ }
const ref = parseColumnRef(arg, ctx)
return ref === null ? null : { kind: 'column', ref }
}Note this changes query-only snippets (no schema in scope): they would now emit params rather than column refs, so pair it with a warning if the verbatim-identifier behaviour is intended there.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function parseComparisonValue( | |
| arg: ts.Expression | undefined, | |
| ctx: TCtx, | |
| ): QueryValue | { kind: 'column'; ref: ColumnRef } | null { | |
| // On the right of a comparison, `table.column` is the join-style column | |
| // reference; deeper accesses (`req.body.id`) are runtime values. | |
| if (arg && ts.isPropertyAccessExpression(arg) && ts.isIdentifier(arg.expression)) { | |
| const ref = parseColumnRef(arg, ctx) | |
| return ref === null ? null : { kind: 'column', ref } | |
| } | |
| return parseValue(arg, ctx) | |
| } | |
| function parseComparisonValue( | |
| arg: ts.Expression | undefined, | |
| ctx: TCtx, | |
| ): QueryValue | { kind: 'column'; ref: ColumnRef } | null { | |
| // On the right of a comparison, `table.column` is the join-style column | |
| // reference; deeper accesses (`req.body.id`) are runtime values. | |
| if (arg && ts.isPropertyAccessExpression(arg) && ts.isIdentifier(arg.expression)) { | |
| if (!ctx.symbols.has(arg.expression.text)) { | |
| return parseValue(arg, ctx) | |
| } | |
| const ref = parseColumnRef(arg, ctx) | |
| return ref === null ? null : { kind: 'column', ref } | |
| } | |
| return parseValue(arg, ctx) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/studio/src/features/orm-cockpit/converters/drizzle-to-sql-query.ts`
around lines 583 - 594, Update parseComparisonValue to treat a two-part property
access as a column reference only when its root identifier is present in
ctx.symbols; otherwise route it through parseValue so unknown roots such as
session.role become bound parameters. Preserve the existing join-style handling
for known table roots, and add the requested warning for query-only snippets if
verbatim identifier behavior must remain supported.
Comment on lines
+82
to
+88
| if (query.limit !== undefined) { | ||
| parts.push(`LIMIT ${query.limit}`) | ||
| } | ||
| if (query.offset !== undefined) { | ||
| parts.push(`OFFSET ${query.offset}`) | ||
| } | ||
| return `${parts.join(' ')};` |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
OFFSET without LIMIT emits invalid SQL on MySQL and SQLite.
Both dialects reject a bare OFFSET; only Postgres accepts it. The SQL→Drizzle direction already warns about this (sql-to-drizzle-dml.ts Line 352-354), but this emitter will happily produce SELECT … OFFSET 5; that fails at execution time.
🐛 Emit a sentinel LIMIT for the dialects that require one
if (query.limit !== undefined) {
parts.push(`LIMIT ${query.limit}`)
+ } else if (query.offset !== undefined && dialect !== 'postgres') {
+ warnings.push(
+ `${dialect} requires a LIMIT before OFFSET; emitted a sentinel LIMIT to keep the statement runnable.`
+ )
+ parts.push(dialect === 'sqlite' ? 'LIMIT -1' : 'LIMIT 18446744073709551615')
}
if (query.offset !== undefined) {
parts.push(`OFFSET ${query.offset}`)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (query.limit !== undefined) { | |
| parts.push(`LIMIT ${query.limit}`) | |
| } | |
| if (query.offset !== undefined) { | |
| parts.push(`OFFSET ${query.offset}`) | |
| } | |
| return `${parts.join(' ')};` | |
| if (query.limit !== undefined) { | |
| parts.push(`LIMIT ${query.limit}`) | |
| } else if (query.offset !== undefined && dialect !== 'postgres') { | |
| warnings.push( | |
| `${dialect} requires a LIMIT before OFFSET; emitted a sentinel LIMIT to keep the statement runnable.` | |
| ) | |
| parts.push(dialect === 'sqlite' ? 'LIMIT -1' : 'LIMIT 18446744073709551615') | |
| } | |
| if (query.offset !== undefined) { | |
| parts.push(`OFFSET ${query.offset}`) | |
| } | |
| return `${parts.join(' ')};` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/studio/src/features/orm-cockpit/converters/emit-sql.ts` around lines
82 - 88, Update the SQL emission logic in the query converter so OFFSET without
LIMIT produces dialect-valid SQL for MySQL and SQLite by adding the required
sentinel LIMIT before the OFFSET. Preserve the existing explicit query.limit
behavior and Postgres output, and keep the current LIMIT/OFFSET ordering.
Comment on lines
+3
to
+41
| const RESERVED = new Set([ | ||
| 'break', | ||
| 'case', | ||
| 'catch', | ||
| 'class', | ||
| 'const', | ||
| 'continue', | ||
| 'debugger', | ||
| 'default', | ||
| 'delete', | ||
| 'do', | ||
| 'else', | ||
| 'enum', | ||
| 'export', | ||
| 'extends', | ||
| 'false', | ||
| 'finally', | ||
| 'for', | ||
| 'function', | ||
| 'if', | ||
| 'import', | ||
| 'in', | ||
| 'instanceof', | ||
| 'new', | ||
| 'null', | ||
| 'return', | ||
| 'super', | ||
| 'switch', | ||
| 'this', | ||
| 'throw', | ||
| 'true', | ||
| 'try', | ||
| 'typeof', | ||
| 'var', | ||
| 'void', | ||
| 'while', | ||
| 'with', | ||
| 'yield' | ||
| ]) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
RESERVED misses strict-mode/ES2017+ reserved words.
Emitted modules are strict mode, so let, static, implements, interface, package, private, protected, public, await are all invalid variable names there too. A table named public or interface (both plausible in SQL) would emit const public = pgTable(...), which does not parse. tableVarName and isSafeIdentifier both depend on this set.
🛠️ Proposed additions
'instanceof',
+ 'interface',
+ 'let',
'new',
'null',
+ 'package',
+ 'private',
+ 'protected',
+ 'public',
'return',
+ 'static',
'super',Also consider adding 'await' and 'implements'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/studio/src/features/orm-cockpit/converters/sql-to-drizzle-naming.ts`
around lines 3 - 41, Expand the RESERVED set used by tableVarName and
isSafeIdentifier to include strict-mode and ES2017+ disallowed identifiers: let,
static, implements, interface, package, private, protected, public, and await.
Keep the existing reserved-word handling unchanged so generated identifiers such
as SQL table names never produce invalid strict-mode variable declarations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #162.
Deterministic, non-AI Drizzle ↔ SQL conversion in both directions, surfaced as a new Converter tab in the ORM cockpit. No LLM calls, no new runtime dependencies; same input + options ⇒ byte-identical output.
Shape
Everything hangs off a small shared contract (
converters/contract.ts):ConvertResult(discriminated ok/errors withunsupported-construct/parse-errorcodes, never throws), aDialectparameter (postgres / mysql / sqlite), and two surfaces auto-detected from the input:SchemaIR. Drizzle→SQL reusesparseDrizzleSchemaplus the DDL emission rules already inmigration/generate-sql.ts(now exported instead of duplicated). SQL→Drizzle is a hand-rolled DDL parser (CREATE TABLE / CREATE INDEX / ALTER ADD) emitting idiomatic drizzle code — camelCase keys,.references(() => …), config-callback composite PKs/FKs/indexes, FK-topological table order.QueryIR. select (4 join kinds, where, groupBy, orderBy, limit/offset), insert (multi-row, returning), update, delete, with exactly the operator set the Drizzle LSP completes (eq…notExists). Static TS-AST parsing on one side, a hand-rolled SQL tokenizer/parser on the other (node-sql-parser was evaluated and rejected: ~1 MB in the Tauri bundle, generic errors, dialect-AST translation cost).UI
Third tab in the ORM cockpit panel (offline — no connection/link gating): two resizable Monaco panes, direction + dialect pickers, swap (carries output into input), copy via the existing
useClipboard, inline error list with line numbers, dismissable warnings strip, 300 ms debounce.Round-trip guarantees (tested)
SchemaIRsorts tables/columns, so DDL can't preserve declaration order — spelling normalizes, semantics don't).Making these hold surfaced three real pre-existing gaps, fixed here:
parseDrizzleSchemaignored.primaryKey({ autoIncrement: true })andtimestamp({ withTimezone: true }).generate-sqldroppedvectordimensions and let MySQL's defaultVARCHAR(255)swallow explicit lengths (one fixture had baked that bug in; corrected).Known limitations (deliberate, reported not hidden)
unsupported-construct, not a guess.RETURNING *uses a[{ column: '*' }]sentinel inQueryIR; params render as:name.BYTEA/ mysql binary degrade totext()with an explicit warning (drizzle-orm 0.45 ships no builder for them).Gates
bunx vitest run __tests__/packages/studio— 22 files, 413 tests green (152 new).packages/studio+apps/desktoptypecheck clean; desktopvite buildsucceeds.Built by three parallel Opus subagents (Drizzle→SQL, SQL→Drizzle, UI) against the contract, then integrated: mocks swapped for the real converters, round-trip suite added on the shared fixtures.
Summary by CodeRabbit
New Features
Documentation
Tests