Skip to content

fix: quote SQL identifiers in migration generation #9

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rekurt merged 1 commit into from
Apr 7, 2026
Merged

fix: quote SQL identifiers in migration generation #9

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
158 changes: 118 additions & 40 deletions src/migration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ pub fn generate_migration(diff: &SchemaDiff, dialect: SqlDialect) -> Vec<Migrati
statements.push(MigrationStatement {
sql: format!(
"ALTER TABLE {} DROP COLUMN {};",
table_diff.table_name, col.name
quote_ident(&table_diff.table_name, dialect),
quote_ident(&col.name, dialect)
),
warnings,
});
Expand All @@ -51,7 +52,7 @@ pub fn generate_migration(diff: &SchemaDiff, dialect: SqlDialect) -> Vec<Migrati
// Phase 3: DROP TABLEs
for table in &diff.removed_tables {
statements.push(MigrationStatement {
sql: format!("DROP TABLE {};", table.name),
sql: format!("DROP TABLE {};", quote_ident(&table.name, dialect)),
warnings: vec![format!(
"Dropping table '{}' will permanently delete all data.",
table.name
Expand All @@ -62,14 +63,14 @@ pub fn generate_migration(diff: &SchemaDiff, dialect: SqlDialect) -> Vec<Migrati
// Phase 4: CREATE TABLEs
for table in &diff.added_tables {
statements.push(MigrationStatement {
sql: create_table_sql(table),
sql: create_table_sql(table, dialect),
warnings: Vec::new(),
});

// Indexes for new table
for idx in table.indexes.values() {
statements.push(MigrationStatement {
sql: create_index_sql(idx),
sql: create_index_sql(idx, dialect),
warnings: Vec::new(),
});
}
Expand All @@ -82,8 +83,8 @@ pub fn generate_migration(diff: &SchemaDiff, dialect: SqlDialect) -> Vec<Migrati
statements.push(MigrationStatement {
sql: format!(
"ALTER TABLE {} ADD COLUMN {};",
table_diff.table_name,
col.definition()
quote_ident(&table_diff.table_name, dialect),
column_definition_sql(col, dialect)
),
warnings,
});
Expand All @@ -100,7 +101,7 @@ pub fn generate_migration(diff: &SchemaDiff, dialect: SqlDialect) -> Vec<Migrati
for table_diff in &diff.modified_tables {
for idx in &table_diff.added_indexes {
statements.push(MigrationStatement {
sql: create_index_sql(idx),
sql: create_index_sql(idx, dialect),
warnings: vec![
"Consider using CREATE INDEX CONCURRENTLY to avoid locking the table."
.to_string(),
Expand All @@ -119,28 +120,61 @@ pub struct MigrationStatement {
pub warnings: Vec<String>,
}

fn create_table_sql(table: &Table) -> String {
fn quote_ident(ident: &str, dialect: SqlDialect) -> String {
match dialect {
SqlDialect::MySql => format!("`{}`", ident.replace('`', "``")),
_ => format!("\"{}\"", ident.replace('"', "\"\"")),
}
}

fn column_definition_sql(col: &Column, dialect: SqlDialect) -> String {
let mut def = format!("{} {}", quote_ident(&col.name, dialect), col.data_type);
if !col.is_nullable {
def.push_str(" NOT NULL");
}
if let Some(ref default) = col.default {
def.push_str(&format!(" DEFAULT {default}"));
}
def
}

fn create_table_sql(table: &Table, dialect: SqlDialect) -> String {
let columns: Vec<String> = table
.columns
.values()
.map(|c| format!(" {}", c.definition()))
.map(|c| format!(" {}", column_definition_sql(c, dialect)))
.collect();
format!("CREATE TABLE {} (\n{}\n);", table.name, columns.join(",\n"))
format!(
"CREATE TABLE {} (\n{}\n);",
quote_ident(&table.name, dialect),
columns.join(",\n")
)
}

fn create_index_sql(idx: &Index) -> String {
fn create_index_sql(idx: &Index, dialect: SqlDialect) -> String {
let unique = if idx.is_unique { "UNIQUE " } else { "" };
let cols = idx.columns.join(", ");
let cols = idx
.columns
.iter()
.map(|c| quote_ident(c, dialect))
.collect::<Vec<_>>()
Comment on lines +157 to +160

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid quoting full index clauses in CREATE INDEX

Quoting every entry in idx.columns breaks non-trivial index definitions because these strings are not always bare identifiers. The PostgreSQL loader extracts raw clauses from pg_indexes.indexdef (for example created_at DESC, lower(email), or already-quoted names), and wrapping each clause with quote_ident turns them into invalid or semantically different SQL ("created_at DESC", "lower(email)", etc.). This means migrations generated for expression/sorted indexes can no longer be applied.

Useful? React with 👍 / 👎.

.join(", ");
format!(
"CREATE {unique}INDEX {} ON {}({});",
idx.name, idx.table_name, cols
quote_ident(&idx.name, dialect),
quote_ident(&idx.table_name, dialect),
cols
)
}

fn drop_index_sql(idx: &Index, dialect: SqlDialect) -> String {
match dialect {
SqlDialect::MySql => format!("DROP INDEX {} ON {};", idx.name, idx.table_name),
_ => format!("DROP INDEX {};", idx.name),
SqlDialect::MySql => format!(
"DROP INDEX {} ON {};",
quote_ident(&idx.name, dialect),
quote_ident(&idx.table_name, dialect)
),
_ => format!("DROP INDEX {};", quote_ident(&idx.name, dialect)),
}
}

Expand Down Expand Up @@ -177,7 +211,11 @@ fn generate_column_alterations(
if old.data_type != new.data_type {
match dialect {
SqlDialect::MySql => stmts.push(MigrationStatement {
sql: format!("ALTER TABLE {table} MODIFY COLUMN {};", new.definition()),
sql: format!(
"ALTER TABLE {} MODIFY COLUMN {};",
quote_ident(table, dialect),
column_definition_sql(new, dialect)
),
warnings: vec![format!(
"Changing column type from '{}' to '{}' may require a table rewrite \
and table lock.",
Expand All @@ -186,8 +224,9 @@ fn generate_column_alterations(
}),
SqlDialect::Sqlite => stmts.push(MigrationStatement {
sql: format!(
"-- manual migration required for type change on {table}.{}",
new.name
"-- manual migration required for type change on {}.{}",
quote_ident(table, dialect),
quote_ident(&new.name, dialect)
),
warnings: vec![format!(
"SQLite does not support ALTER COLUMN TYPE directly for '{}'. \
Expand All @@ -197,8 +236,10 @@ fn generate_column_alterations(
}),
_ => stmts.push(MigrationStatement {
sql: format!(
"ALTER TABLE {table} ALTER COLUMN {} TYPE {};",
new.name, new.data_type
"ALTER TABLE {} ALTER COLUMN {} TYPE {};",
quote_ident(table, dialect),
quote_ident(&new.name, dialect),
new.data_type
),
warnings: vec![format!(
"Changing column type from '{}' to '{}' may require a table rewrite \
Expand Down Expand Up @@ -227,15 +268,20 @@ fn generate_column_alterations(
)
};
stmts.push(MigrationStatement {
sql: format!("ALTER TABLE {table} MODIFY COLUMN {};", new.definition()),
sql: format!(
"ALTER TABLE {} MODIFY COLUMN {};",
quote_ident(table, dialect),
column_definition_sql(new, dialect)
),
warnings: vec![warning],
});
}
SqlDialect::Sqlite => {
stmts.push(MigrationStatement {
sql: format!(
"-- manual migration required for nullability change on {table}.{}",
new.name
"-- manual migration required for nullability change on {}.{}",
quote_ident(table, dialect),
quote_ident(&new.name, dialect)
),
warnings: vec![format!(
"SQLite does not support ALTER COLUMN nullability directly for '{}'. \
Expand All @@ -248,16 +294,18 @@ fn generate_column_alterations(
if new.is_nullable {
stmts.push(MigrationStatement {
sql: format!(
"ALTER TABLE {table} ALTER COLUMN {} DROP NOT NULL;",
new.name
"ALTER TABLE {} ALTER COLUMN {} DROP NOT NULL;",
quote_ident(table, dialect),
quote_ident(&new.name, dialect)
),
warnings: Vec::new(),
});
} else {
stmts.push(MigrationStatement {
sql: format!(
"ALTER TABLE {table} ALTER COLUMN {} SET NOT NULL;",
new.name
"ALTER TABLE {} ALTER COLUMN {} SET NOT NULL;",
quote_ident(table, dialect),
quote_ident(&new.name, dialect)
),
warnings: vec![format!(
"SET NOT NULL on '{}' will scan the entire table to verify no NULLs exist. \
Expand All @@ -276,17 +324,19 @@ fn generate_column_alterations(
Some(default) => {
stmts.push(MigrationStatement {
sql: format!(
"ALTER TABLE {table} ALTER COLUMN {} SET DEFAULT {default};",
new.name
"ALTER TABLE {} ALTER COLUMN {} SET DEFAULT {default};",
quote_ident(table, dialect),
quote_ident(&new.name, dialect)
),
warnings: Vec::new(),
});
}
None => {
stmts.push(MigrationStatement {
sql: format!(
"ALTER TABLE {table} ALTER COLUMN {} DROP DEFAULT;",
new.name
"ALTER TABLE {} ALTER COLUMN {} DROP DEFAULT;",
quote_ident(table, dialect),
quote_ident(&new.name, dialect)
),
warnings: Vec::new(),
});
Expand Down Expand Up @@ -335,7 +385,7 @@ mod tests {
assert_eq!(stmts.len(), 1);
assert_eq!(
stmts[0].sql,
"ALTER TABLE users ADD COLUMN email varchar(255) NOT NULL;"
"ALTER TABLE \"users\" ADD COLUMN \"email\" varchar(255) NOT NULL;"
);
assert!(!stmts[0].warnings.is_empty()); // NOT NULL without DEFAULT warning
}
Expand All @@ -360,7 +410,10 @@ mod tests {
let stmts = generate_migration(&diff, SqlDialect::Postgres);

assert_eq!(stmts.len(), 1);
assert_eq!(stmts[0].sql, "ALTER TABLE users DROP COLUMN old_field;");
assert_eq!(
stmts[0].sql,
"ALTER TABLE \"users\" DROP COLUMN \"old_field\";"
);
assert!(stmts[0].warnings[0].contains("destructive"));
}

Expand All @@ -380,9 +433,9 @@ mod tests {
let stmts = generate_migration(&diff, SqlDialect::Postgres);

assert_eq!(stmts.len(), 1);
assert!(stmts[0].sql.starts_with("CREATE TABLE orders"));
assert!(stmts[0].sql.contains("id serial NOT NULL"));
assert!(stmts[0].sql.contains("total numeric(10,2) NOT NULL"));
assert!(stmts[0].sql.starts_with("CREATE TABLE \"orders\""));
assert!(stmts[0].sql.contains("\"id\" serial NOT NULL"));
assert!(stmts[0].sql.contains("\"total\" numeric(10,2) NOT NULL"));
}

#[test]
Expand All @@ -405,7 +458,7 @@ mod tests {
assert_eq!(stmts.len(), 1);
assert_eq!(
stmts[0].sql,
"ALTER TABLE users ALTER COLUMN email TYPE varchar(255);"
"ALTER TABLE \"users\" ALTER COLUMN \"email\" TYPE varchar(255);"
);
}

Expand Down Expand Up @@ -436,7 +489,10 @@ mod tests {
let stmts = generate_migration(&diff, SqlDialect::Postgres);

assert_eq!(stmts.len(), 1);
assert_eq!(stmts[0].sql, "CREATE INDEX idx_orders_id ON orders(id);");
assert_eq!(
stmts[0].sql,
"CREATE INDEX \"idx_orders_id\" ON \"orders\"(\"id\");"
);
}

#[test]
Expand Down Expand Up @@ -519,7 +575,7 @@ mod tests {
let diff = diff_schemas(&left, &right);
let stmts = generate_migration(&diff, SqlDialect::MySql);

assert_eq!(stmts[0].sql, "DROP INDEX idx_orders_id ON orders;");
assert_eq!(stmts[0].sql, "DROP INDEX `idx_orders_id` ON `orders`;");
}

#[test]
Expand Down Expand Up @@ -563,7 +619,7 @@ mod tests {
assert_eq!(stmts.len(), 1);
assert_eq!(
stmts[0].sql,
"ALTER TABLE users MODIFY COLUMN email varchar(255) NOT NULL;"
"ALTER TABLE `users` MODIFY COLUMN `email` varchar(255) NOT NULL;"
);
}

Expand All @@ -590,4 +646,26 @@ mod tests {
.starts_with("-- manual migration required for nullability change"));
assert!(stmts[0].warnings[0].contains("does not support ALTER COLUMN nullability"));
}

#[test]
fn postgres_identifiers_are_quoted_and_escaped() {
let left = Schema::new();
let mut right = Schema::new();
let mut t = Table::new("users\"; DROP TABLE payments; --");
t.columns.insert(
"email\"; DELETE FROM users; --".into(),
col("email\"; DELETE FROM users; --", "text", true, None),
);
right.tables.insert(t.name.clone(), t);

let diff = diff_schemas(&left, &right);
let stmts = generate_migration(&diff, SqlDialect::Postgres);

assert!(stmts[0]
.sql
.starts_with("CREATE TABLE \"users\"\"; DROP TABLE payments; --\""));
assert!(stmts[0]
.sql
.contains("\"email\"\"; DELETE FROM users; --\" text"));
}
}