diff --git a/cmd/limen/driver.go b/cmd/limen/driver.go index 077c794..b5a7d04 100644 --- a/cmd/limen/driver.go +++ b/cmd/limen/driver.go @@ -14,6 +14,10 @@ type Driver interface { IntrospectColumnsQuery(tableName string) (string, []any) IntrospectIndexesQuery(tableName string) (string, []any) IntrospectForeignKeysQuery(tableName string) (string, []any) + // QuoteIdentifier quotes a table, column, index or constraint name for this + // database, so that reserved words and case-sensitive names survive into + // the generated DDL. Must agree with how the runtime adapter quotes. + QuoteIdentifier(name string) string MapGoTypeToSQL(goType limen.ColumnType, isAutoIncrement bool) string MapSQLTypeToGoType(dataType string) limen.ColumnType GetAutoIncrementSuffix() string diff --git a/cmd/limen/driver_base.go b/cmd/limen/driver_base.go index fc649ca..37ef828 100644 --- a/cmd/limen/driver_base.go +++ b/cmd/limen/driver_base.go @@ -1,7 +1,6 @@ package main import ( - "fmt" "strings" "github.com/thecodearcher/limen" @@ -42,10 +41,6 @@ func (d *baseDriver) ParseForeignKeyRow(scan func(dest ...any) error) (limen.For }, nil } -func (d *baseDriver) DropColumnSQL(tableName, columnName string) string { - return fmt.Sprintf("DROP COLUMN %s", columnName) -} - func (d *baseDriver) ParseColumnRow(scan func(dest ...any) error) (limen.ColumnDefinition, error) { var colName string diff --git a/cmd/limen/driver_mysql.go b/cmd/limen/driver_mysql.go index 0e237d1..3b6937e 100644 --- a/cmd/limen/driver_mysql.go +++ b/cmd/limen/driver_mysql.go @@ -19,6 +19,24 @@ func NewMySQLDriver() Driver { return &mysqlDriver{} } +// mysqlQuoteChar is MySQL's and MariaDB's identifier quote. It is accepted in +// every sql_mode, including ANSI_QUOTES, where " also becomes an identifier +// quote. +const mysqlQuoteChar = "`" + +// QuoteIdentifier quotes a table, column, index or constraint name, doubling +// any embedded backtick so the result stays a single identifier. It mirrors +// adapters/sql's quoteIdent, since the generator and the runtime adapter must +// agree on the name a table actually has. +// +// Hand-rolled because go-sql-driver/mysql exports no equivalent of pgx's +// Identifier.Sanitize. +func (d *mysqlDriver) QuoteIdentifier(name string) string { + return mysqlQuoteChar + + strings.ReplaceAll(name, mysqlQuoteChar, mysqlQuoteChar+mysqlQuoteChar) + + mysqlQuoteChar +} + func (d *mysqlDriver) Name() string { return string(DriverMySQL) } @@ -133,9 +151,13 @@ func (d *mysqlDriver) FormatDefaultValue(defaultValue string) string { } func (d *mysqlDriver) DropIndexSQL(tableName, indexName string) string { - return fmt.Sprintf("DROP INDEX %s ON %s", indexName, tableName) + return fmt.Sprintf("DROP INDEX %s ON %s", d.QuoteIdentifier(indexName), d.QuoteIdentifier(tableName)) } func (d *mysqlDriver) DropForeignKeySQL(tableName, constraintName string) string { - return fmt.Sprintf("DROP FOREIGN KEY %s", constraintName) + return fmt.Sprintf("DROP FOREIGN KEY %s", d.QuoteIdentifier(constraintName)) +} + +func (d *mysqlDriver) DropColumnSQL(tableName, columnName string) string { + return fmt.Sprintf("DROP COLUMN %s", d.QuoteIdentifier(columnName)) } diff --git a/cmd/limen/driver_postgres.go b/cmd/limen/driver_postgres.go index c7f9cdb..d78fcc0 100644 --- a/cmd/limen/driver_postgres.go +++ b/cmd/limen/driver_postgres.go @@ -5,6 +5,7 @@ import ( "fmt" "strings" + "github.com/jackc/pgx/v5" _ "github.com/jackc/pgx/v5/stdlib" "github.com/thecodearcher/limen" @@ -19,6 +20,13 @@ func NewPostgresDriver() Driver { return &postgresDriver{} } +// QuoteIdentifier defers to pgx, which is already this driver's connection +// library, rather than re-deriving PostgreSQL's quoting rules. Beyond doubling +// embedded quotes it also strips NUL bytes, which PostgreSQL rejects outright. +func (d *postgresDriver) QuoteIdentifier(name string) string { + return pgx.Identifier{name}.Sanitize() +} + func (d *postgresDriver) Name() string { return string(DriverPostgres) } @@ -182,9 +190,13 @@ func (d *postgresDriver) FormatDefaultValue(defaultValue string) string { } func (d *postgresDriver) DropIndexSQL(tableName, indexName string) string { - return fmt.Sprintf("DROP INDEX IF EXISTS %s", indexName) + return fmt.Sprintf("DROP INDEX IF EXISTS %s", d.QuoteIdentifier(indexName)) } func (d *postgresDriver) DropForeignKeySQL(tableName, constraintName string) string { - return fmt.Sprintf("DROP CONSTRAINT %s", constraintName) + return fmt.Sprintf("DROP CONSTRAINT %s", d.QuoteIdentifier(constraintName)) +} + +func (d *postgresDriver) DropColumnSQL(tableName, columnName string) string { + return fmt.Sprintf("DROP COLUMN %s", d.QuoteIdentifier(columnName)) } diff --git a/cmd/limen/migration_generator.go b/cmd/limen/migration_generator.go index cf3f288..b389394 100644 --- a/cmd/limen/migration_generator.go +++ b/cmd/limen/migration_generator.go @@ -38,13 +38,28 @@ func (s *sqlMigrationGenerator) generateDownMigration(schema *limen.SchemaDefini if diff != nil && diff.HasChanges() { return s.generateAlterDownMigration(schema.GetTableName(), diff) } - return fmt.Sprintf("DROP TABLE IF EXISTS %s;", schema.GetTableName()), nil + return fmt.Sprintf("DROP TABLE IF EXISTS %s;", s.quote(string(schema.GetTableName()))), nil +} + +// quote quotes a single identifier for the target database. +func (s *sqlMigrationGenerator) quote(name string) string { + return s.driver.QuoteIdentifier(name) +} + +// quoteAll quotes each identifier and joins them with separator, for the +// column lists in PRIMARY KEY and CREATE INDEX. +func quoteAll[T ~string](s *sqlMigrationGenerator, names []T, separator string) string { + quoted := make([]string, len(names)) + for i := range names { + quoted[i] = s.quote(string(names[i])) + } + return strings.Join(quoted, separator) } func (s *sqlMigrationGenerator) generateCreateTable(schema *limen.SchemaDefinition) (string, error) { var buf strings.Builder - fmt.Fprintf(&buf, "CREATE TABLE IF NOT EXISTS %s (\n", schema.GetTableName()) + fmt.Fprintf(&buf, "CREATE TABLE IF NOT EXISTS %s (\n", s.quote(string(schema.GetTableName()))) columns := make([]string, 0, len(schema.Columns)) for _, field := range schema.Columns { @@ -61,7 +76,7 @@ func (s *sqlMigrationGenerator) generateCreateTable(schema *limen.SchemaDefiniti } if len(pkFields) > 0 { - fmt.Fprintf(&buf, ",\n PRIMARY KEY (%s)", strings.Join(pkFields, ", ")) + fmt.Fprintf(&buf, ",\n PRIMARY KEY (%s)", quoteAll(s, pkFields, ", ")) } for _, fk := range schema.ForeignKeys { @@ -107,7 +122,11 @@ func (s *sqlMigrationGenerator) generateAlterDownMigration(tableName limen.Schem statements := []string{} for _, idx := range diff.AddedIndexes { - dropSQL := s.driver.DropIndexSQL(string(tableName), idx.Name) + // DropIndexSQL is a standalone statement but carries no terminator, + // unlike DropColumnSQL and DropForeignKeySQL, which are fragments of + // the ALTER TABLE appended below. Without the semicolon the two run + // together into one statement the database rejects. + dropSQL := s.driver.DropIndexSQL(string(tableName), idx.Name) + ";" statements = append(statements, dropSQL) } @@ -135,7 +154,7 @@ func (s *sqlMigrationGenerator) generateUpAlterTableStatement(tableName limen.Sc var buf strings.Builder statements := []string{} - fmt.Fprintf(&buf, "ALTER TABLE %s\n", tableName) + fmt.Fprintf(&buf, "ALTER TABLE %s\n", s.quote(string(tableName))) for _, col := range diff.AddedColumns { colDef := s.generateColumnDefinition(&col) statements = append(statements, fmt.Sprintf("ADD COLUMN %s", colDef)) @@ -161,7 +180,7 @@ func (s *sqlMigrationGenerator) generateDownAlterTableStatement(tableName limen. var buf strings.Builder statements := []string{} - fmt.Fprintf(&buf, "ALTER TABLE %s\n", tableName) + fmt.Fprintf(&buf, "ALTER TABLE %s\n", s.quote(string(tableName))) for _, fk := range diff.AddedForeignKeys { dropOp := s.driver.DropForeignKeySQL(string(tableName), fk.Name) @@ -181,7 +200,7 @@ func (s *sqlMigrationGenerator) generateDownAlterTableStatement(tableName limen. } func (s *sqlMigrationGenerator) generateColumnDefinition(field *limen.ColumnDefinition) string { - parts := []string{field.Name} + parts := []string{s.quote(field.Name)} isAutoIncrement := field.LogicalField == limen.SchemaIDField && s.useAutoIncrementIDs @@ -214,10 +233,12 @@ func (s *sqlMigrationGenerator) generateForeignKeyStatement(fk *limen.ForeignKey if alterTable { fmt.Fprintf(&buf, "ADD CONSTRAINT %s FOREIGN KEY (%s) REFERENCES %s (%s)", - fk.Name, fk.Column, string(fk.ReferencedSchema), string(fk.ReferencedField)) + s.quote(fk.Name), s.quote(string(fk.Column)), + s.quote(string(fk.ReferencedSchema)), s.quote(string(fk.ReferencedField))) } else { fmt.Fprintf(&buf, ",\nCONSTRAINT %s FOREIGN KEY (%s) REFERENCES %s (%s)", - fk.Name, fk.Column, string(fk.ReferencedSchema), string(fk.ReferencedField)) + s.quote(fk.Name), s.quote(string(fk.Column)), + s.quote(string(fk.ReferencedSchema)), s.quote(string(fk.ReferencedField))) } if fk.OnDelete != "" { @@ -234,20 +255,9 @@ func (s *sqlMigrationGenerator) generateForeignKeyStatement(fk *limen.ForeignKey func (s *sqlMigrationGenerator) generateCreateIndexStatement(idx *limen.IndexDefinition, tableName limen.SchemaTableName) string { if idx.Unique { return fmt.Sprintf("CREATE UNIQUE INDEX %s ON %s (%s);", - idx.Name, tableName, joinCustomStringSlice(idx.Columns, ", ")) + s.quote(idx.Name), s.quote(string(tableName)), quoteAll(s, idx.Columns, ", ")) } return fmt.Sprintf("CREATE INDEX %s ON %s (%s);", - idx.Name, tableName, joinCustomStringSlice(idx.Columns, ", ")) -} - -func joinCustomStringSlice[T ~string](fields []T, separator string) string { - var joined string - for i := range fields { - joined += string(fields[i]) - if i < len(fields)-1 { - joined += separator - } - } - return joined + s.quote(idx.Name), s.quote(string(tableName)), quoteAll(s, idx.Columns, ", ")) } diff --git a/cmd/limen/migration_generator_test.go b/cmd/limen/migration_generator_test.go new file mode 100644 index 0000000..2a66cb2 --- /dev/null +++ b/cmd/limen/migration_generator_test.go @@ -0,0 +1,330 @@ +package main + +import ( + "strings" + "testing" + + "github.com/thecodearcher/limen" +) + +// The generator interpolates every identifier with %s, so table, column, index, +// constraint and referenced-table names all reach the emitted DDL unquoted. +// +// `user` and `order` are reserved words in both PostgreSQL and MySQL, and they +// are exactly the names a caller reaches for through WithUserTableName and +// friends — the default names (`users`, `sessions`, ...) happen to be safe, +// which is why this survives untested. +// +// Quoting is not cosmetic here: adapters/sql quotes every identifier at +// runtime (its quoteIdent doubles embedded quote chars), so DDL emitted +// unquoted disagrees with the queries that will later run against it. A +// reserved word fails loudly at migration time; a name needing case +// preservation is worse, since unquoted DDL folds to lowercase while the +// adapter's quoted query does not, and the mismatch only surfaces at runtime. + +func newTestGenerator(t *testing.T, driver Driver) *sqlMigrationGenerator { + t.Helper() + + gen, err := newSQLMigrationGenerator(driver, &cliConfig{}) + if err != nil { + t.Fatalf("newSQLMigrationGenerator() error = %v", err) + } + + return gen +} + +// reservedWordSchema is a minimal schema whose table and one of whose columns +// are reserved words. +func reservedWordSchema() *limen.SchemaDefinition { + return &limen.SchemaDefinition{ + TableName: "user", + Columns: []limen.ColumnDefinition{ + { + Name: "id", + LogicalField: limen.SchemaIDField, + Type: limen.ColumnTypeInt64, + IsPrimaryKey: true, + }, + { + Name: "order", + LogicalField: "order", + Type: limen.ColumnTypeString, + }, + }, + Indexes: []limen.IndexDefinition{ + { + Name: "idx_user_order", + Columns: []limen.SchemaField{"order"}, + Unique: true, + }, + }, + } +} + +func assertContainsAll(t *testing.T, got string, want []string) { + t.Helper() + + for _, w := range want { + if !strings.Contains(got, w) { + t.Errorf("generated SQL is missing %q\ngot:\n%s", w, got) + } + } +} + +func TestGenerateCreateTableQuotesIdentifiers(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + driver Driver + want []string + }{ + { + name: "postgres", + driver: NewPostgresDriver(), + want: []string{ + `CREATE TABLE IF NOT EXISTS "user" (`, + `"id" BIGINT`, + `"order" VARCHAR(255) NOT NULL`, + `PRIMARY KEY ("id")`, + `CREATE UNIQUE INDEX "idx_user_order" ON "user" ("order");`, + }, + }, + { + name: "mysql", + driver: NewMySQLDriver(), + want: []string{ + "CREATE TABLE IF NOT EXISTS `user` (", + "`id` BIGINT", + "PRIMARY KEY (`id`)", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got, err := newTestGenerator(t, tt.driver).generateCreateTable(reservedWordSchema()) + if err != nil { + t.Fatalf("generateCreateTable() error = %v", err) + } + + assertContainsAll(t, got, tt.want) + }) + } +} + +func TestGenerateDownMigrationQuotesTableName(t *testing.T) { + t.Parallel() + + got, err := newTestGenerator(t, NewPostgresDriver()).generateDownMigration(reservedWordSchema(), nil) + if err != nil { + t.Fatalf("generateDownMigration() error = %v", err) + } + + assertContainsAll(t, got, []string{`DROP TABLE IF EXISTS "user";`}) +} + +func TestGenerateCreateIndexStatementQuotesIdentifiers(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + unique bool + want string + }{ + { + name: "unique index", + unique: true, + want: `CREATE UNIQUE INDEX "idx_user_order" ON "user" ("order");`, + }, + { + name: "plain index", + unique: false, + want: `CREATE INDEX "idx_user_order" ON "user" ("order");`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + idx := limen.IndexDefinition{ + Name: "idx_user_order", + Columns: []limen.SchemaField{"order"}, + Unique: tt.unique, + } + + got := newTestGenerator(t, NewPostgresDriver()).generateCreateIndexStatement(&idx, "user") + + assertContainsAll(t, got, []string{tt.want}) + }) + } +} + +func TestGenerateForeignKeyStatementQuotesIdentifiers(t *testing.T) { + t.Parallel() + + fk := limen.ForeignKeyDefinition{ + Name: "fk_session_user", + Column: "user_id", + ReferencedSchema: "user", + ReferencedField: "id", + OnDelete: "CASCADE", + } + + tests := []struct { + name string + alterTable bool + want []string + }{ + { + name: "inside CREATE TABLE", + alterTable: false, + want: []string{ + `CONSTRAINT "fk_session_user" FOREIGN KEY ("user_id") REFERENCES "user" ("id")`, + }, + }, + { + name: "inside ALTER TABLE", + alterTable: true, + want: []string{ + `ADD CONSTRAINT "fk_session_user" FOREIGN KEY ("user_id") REFERENCES "user" ("id")`, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got := newTestGenerator(t, NewPostgresDriver()).generateForeignKeyStatement(&fk, tt.alterTable) + + assertContainsAll(t, got, tt.want) + }) + } +} + +// reservedWordDiff adds a reserved-word column, an index and a foreign key to +// an existing table, which is the path taken when the table already exists. +func reservedWordDiff() *schemaDiff { + return &schemaDiff{ + AddedColumns: []limen.ColumnDefinition{ + {Name: "order", LogicalField: "order", Type: limen.ColumnTypeString}, + }, + AddedIndexes: []limen.IndexDefinition{ + {Name: "idx_user_order", Columns: []limen.SchemaField{"order"}, Unique: true}, + }, + AddedForeignKeys: []limen.ForeignKeyDefinition{ + { + Name: "fk_user_order", + Column: "order", + ReferencedSchema: "order", + ReferencedField: "id", + }, + }, + } +} + +func TestGenerateUpMigrationForExistingTableQuotesIdentifiers(t *testing.T) { + t.Parallel() + + got, err := newTestGenerator(t, NewPostgresDriver()). + generateUpMigration(reservedWordSchema(), reservedWordDiff()) + if err != nil { + t.Fatalf("generateUpMigration() error = %v", err) + } + + assertContainsAll(t, got, []string{ + `ALTER TABLE "user"`, + `ADD COLUMN "order" VARCHAR(255) NOT NULL`, + `ADD CONSTRAINT "fk_user_order" FOREIGN KEY ("order") REFERENCES "order" ("id")`, + `CREATE UNIQUE INDEX "idx_user_order" ON "user" ("order");`, + }) +} + +func TestGenerateDownMigrationForExistingTableQuotesIdentifiers(t *testing.T) { + t.Parallel() + + got, err := newTestGenerator(t, NewPostgresDriver()). + generateDownMigration(reservedWordSchema(), reservedWordDiff()) + if err != nil { + t.Fatalf("generateDownMigration() error = %v", err) + } + + assertContainsAll(t, got, []string{ + `DROP INDEX IF EXISTS "idx_user_order"`, + `ALTER TABLE "user"`, + `DROP CONSTRAINT "fk_user_order"`, + `DROP COLUMN "order"`, + }) +} + +// A down migration that drops an index and alters the table emits both, and +// the two must not run together into one statement. The driver's DropIndexSQL +// carries no terminator of its own, unlike generateCreateIndexStatement on the +// up path, so the caller has to supply it. +func TestGenerateDownMigrationTerminatesEachStatement(t *testing.T) { + t.Parallel() + + got, err := newTestGenerator(t, NewPostgresDriver()). + generateDownMigration(reservedWordSchema(), reservedWordDiff()) + if err != nil { + t.Fatalf("generateDownMigration() error = %v", err) + } + + assertContainsAll(t, got, []string{`DROP INDEX IF EXISTS "idx_user_order";`}) + + for _, stmt := range strings.Split(got, ";") { + if strings.Contains(stmt, "DROP INDEX") && strings.Contains(stmt, "ALTER TABLE") { + t.Errorf("DROP INDEX and ALTER TABLE share one statement\ngot:\n%s", got) + } + } +} + +func TestDriverDropStatementsQuoteIdentifiers(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + got string + want string + }{ + { + name: "postgres drop index", + got: NewPostgresDriver().DropIndexSQL("user", "idx_user_order"), + want: `DROP INDEX IF EXISTS "idx_user_order"`, + }, + { + name: "postgres drop foreign key", + got: NewPostgresDriver().DropForeignKeySQL("user", "fk_user_order"), + want: `DROP CONSTRAINT "fk_user_order"`, + }, + { + name: "postgres drop column", + got: NewPostgresDriver().DropColumnSQL("user", "order"), + want: `DROP COLUMN "order"`, + }, + { + name: "mysql drop index", + got: NewMySQLDriver().DropIndexSQL("user", "idx_user_order"), + want: "DROP INDEX `idx_user_order` ON `user`", + }, + { + name: "mysql drop column", + got: NewMySQLDriver().DropColumnSQL("user", "order"), + want: "DROP COLUMN `order`", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + if !strings.Contains(tt.got, tt.want) { + t.Errorf("got %q, want it to contain %q", tt.got, tt.want) + } + }) + } +}