fix(cli): quote identifiers in generated migrations - #29
Open
xico42 wants to merge 2 commits into
Open
Conversation
The migration generator interpolated every identifier with %s, so table, column, index, constraint and referenced-table names all reached the emitted DDL unquoted. The default schema names (`users`, `sessions`, ...) are lowercase and non-reserved, which is why this went unnoticed; it is reachable only through WithUserTableName and its siblings, where a caller naturally picks names like `user` or `order`. Both are reserved in PostgreSQL and MySQL, so `CREATE TABLE IF NOT EXISTS user` is a syntax error and the migration cannot be applied at all. The quieter half of the bug is worse. adapters/sql quotes every identifier it emits, so unquoted DDL does not merely look different — it creates a different name. A table configured as `AppUser` is folded to `appuser` by the database, while the adapter goes on asking for "AppUser": the migration succeeds, the service starts, and every query fails against a relation that does not exist. Quoting therefore belongs to the driver rather than to a shared helper, since PostgreSQL and MySQL disagree on the quote character. PostgreSQL defers to pgx's Identifier.Sanitize — already a dependency here, and it strips NUL bytes on top of doubling embedded quotes. MySQL and MariaDB double backticks by hand, because go-sql-driver/mysql exports no equivalent. Neither lives on baseDriver: a method there could not reach the outer type's implementation, so PostgreSQL would silently fall back to the generic one. The generator had no tests at all, and the two halves of the library were never exercised together — adapters/sql builds its fixture tables from hand-written, correctly quoted DDL, so the generator's output never reached the adapter that must later query it. The tests added here pin every affected statement for both drivers, using reserved words throughout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A down migration for a table that already exists emitted the index drops and the ALTER TABLE joined by newlines alone. DropColumnSQL and DropForeignKeySQL are fragments of that ALTER TABLE and correctly carry no terminator, but DropIndexSQL is a statement in its own right, so the two ran together as `DROP INDEX IF EXISTS "x" ALTER TABLE "y" ...` — rejected outright with a syntax error at or near "ALTER". Reachable whenever a diff adds an index alongside a column or a foreign key, which is what an ordinary schema change on a live table looks like. It cost nothing until someone rolled back, which is the worst moment to discover a migration does not parse. The semicolon is added at the call site rather than inside DropIndexSQL, so the driver methods stay uniformly fragment-shaped and only the code assembling statements decides how they are separated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
xico42
force-pushed
the
fix/quote-generated-ddl-identifiers
branch
from
August 8, 2026 22:14
c23007d to
462fd34
Compare
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
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.
The problem
cmd/limen's migration generator interpolates every identifier with%s. Table, column, index, constraint and referenced-table names all reach the emitted DDL unquoted — there is no quoting helper anywhere in the module.The default schema names (
users,sessions,accounts, ...) are lowercase and non-reserved, which is why this has gone unnoticed. It is reachable only throughWithUserTableNameand its siblings, where a caller naturally picks a singular name to match their project's convention:useris reserved in PostgreSQL (pg_get_keywords()classes itR) and in MySQL, so the generated migration is:The migration cannot be applied at all, and there is no way to work around it short of hand-editing generated SQL or picking a different table name.
The quieter half
adapters/sqlquotes every identifier it emits (quoteIdent, which also doubles embedded quote chars). So unquoted DDL does not merely look different from what the adapter expects — it creates a different name.A table configured as
AppUseris folded toappuserby the database, while the adapter goes on asking for"AppUser". The migration succeeds, the service starts, and every query fails against a relation that does not exist. That failure mode is strictly worse than the reserved-word one, because nothing fails at migration time.The fix
QuoteIdentifier(name string) stringon theDriverinterface, wired through the eleven interpolation sites inmigration_generator.goand the fiveDrop*SQLhelpers.joinCustomStringSliceis replaced by aquoteAllthat quotes each element — it existed only to build unquoted column lists.Quoting belongs to the driver rather than to a shared helper, since the dialects disagree on the quote character:
pgx.Identifier{name}.Sanitize(). pgx is already this driver's connection library and needs no new dependency (go.mod/go.sumare unchanged); beyond doubling embedded quotes it also strips NUL bytes, which PostgreSQL rejects outright.go-sql-driver/mysqlexports no equivalent. Backticks are accepted in everysql_mode, includingANSI_QUOTES.Neither implementation lives on
baseDriver: a method there cannot reach the outer type'sQuoteIdentifier, since Go embedding does not dispatch back, so PostgreSQL would silently fall back to the generic implementation.Second commit: unterminated
DROP INDEXFound while testing the above. A down migration for a table that already exists emitted the index drops and the
ALTER TABLEjoined by newlines alone:DropColumnSQLandDropForeignKeySQLare fragments of thatALTER TABLEand correctly carry no terminator, butDropIndexSQLis a statement in its own right. Reachable whenever a diff adds an index alongside a column or foreign key — an ordinary schema change on a live table. It costs nothing until someone rolls back, which is the worst moment to discover a migration does not parse.The semicolon is added at the call site rather than inside
DropIndexSQL, so the driver methods stay uniformly fragment-shaped.Tests
migration_generator.gohad no test coverage at all — the four existing tests incmd/limenall exercisemigration.go. More significantly, the two halves of the library were never tested together:adapters/sql/adapter_test.gobuilds its fixture tables from hand-written, correctly quoted DDL, so the generator's output never reaches the adapter that must later query it. That seam is where this bug lived.19 tests added, pinning every affected statement for both drivers using reserved words throughout (
user,order,select,index,table). All were written failing first against the unfixed generator.Verification
Beyond the unit tests, I executed the generated DDL against real engines using those same reserved names:
adapters/sqlemitsSHOW CREATE TABLEconfirms names stored verbatim; round-trip passesAlso confirmed backticks survive
sql_mode='ANSI_QUOTES', and that the full workspace suite passes under CI's owngo test workacross all modules.Compatibility
information_schema.columns.WithUserTableName("public.users")previously worked as schema-qualified and now quote to a single identifier. This matchesadapters/sql, which would have quoted it the same way and failed at runtime regardless — the generator and the adapter now agree, which is the point.SQLite is unaffected:
adapters/sqlsupports it at runtime but the CLI registers no SQLite driver, so it generates no DDL.