From 2d7eb376af89252b96fecb6fe27cd1a6d0ad5c9a Mon Sep 17 00:00:00 2001 From: James Oakley Date: Mon, 15 Apr 2024 22:12:57 -0400 Subject: [PATCH 1/3] [pg] Use ALTER TABLE instead of copies For the Postgres backend, use the appropriate `ALTER TABLE` command for changed columns instead of copying and recreating the table. This makes the generated sql more readable and also makes it easier to be correct with foreign key constraints, since the recreated table does not automatically replace the old one in constraints. --- butane_core/src/db/helper.rs | 4 +- butane_core/src/db/pg.rs | 154 +++++++++++++----- butane_core/src/db/sqlite.rs | 2 +- butane_core/src/lib.rs | 2 + .../pg_down.sql | 30 +--- .../pg_up.sql | 33 +--- .../getting_started/src/butane_migrations.rs | 4 +- examples/getting_started/tests/rollback.rs | 10 -- 8 files changed, 124 insertions(+), 115 deletions(-) diff --git a/butane_core/src/db/helper.rs b/butane_core/src/db/helper.rs index a9a9bd7d..b6b7123a 100644 --- a/butane_core/src/db/helper.rs +++ b/butane_core/src/db/helper.rs @@ -293,7 +293,7 @@ fn sql_column(col: query::Column, w: &mut impl Write) { .unwrap() } -pub fn sql_literal_value(val: SqlVal) -> Result { +pub fn sql_literal_value(val: &SqlVal) -> Result { use SqlVal::*; match val { SqlVal::Null => Ok("NULL".to_string()), @@ -307,6 +307,6 @@ pub fn sql_literal_value(val: SqlVal) -> Result { Json(val) => Ok(format!("{val}")), #[cfg(feature = "datetime")] Timestamp(ndt) => Ok(ndt.format("'%Y-%m-%dT%H:%M:%S%.f'").to_string()), - Custom(val) => Err(Error::LiteralForCustomUnsupported((*val).clone())), + Custom(val) => Err(Error::LiteralForCustomUnsupported(*(*val).clone())), } } diff --git a/butane_core/src/db/pg.rs b/butane_core/src/db/pg.rs index 8f57b0c6..fdcaec1c 100644 --- a/butane_core/src/db/pg.rs +++ b/butane_core/src/db/pg.rs @@ -553,7 +553,7 @@ fn sql_for_op(current: &mut ADB, op: &Operation) -> Result { Operation::RemoveTable(name) => Ok(drop_table(name)), Operation::AddColumn(tbl, col) => add_column(tbl, col), Operation::RemoveColumn(tbl, name) => Ok(remove_column(tbl, name)), - Operation::ChangeColumn(tbl, old, new) => change_column(current, tbl, old, Some(new)), + Operation::ChangeColumn(tbl, old, new) => change_column(current, tbl, old, new), } } @@ -662,7 +662,7 @@ fn add_column(tbl_name: &str, col: &AColumn) -> Result { "ALTER TABLE {} ADD COLUMN {} DEFAULT {};", helper::quote_reserved_word(tbl_name), define_column(col)?, - helper::sql_literal_value(default)? + helper::sql_literal_value(&default)? )]; if col.reference().is_some() { stmts.push(define_constraint(tbl_name, col)); @@ -679,31 +679,13 @@ fn remove_column(tbl_name: &str, name: &str) -> String { ) } -fn copy_table(old: &ATable, new: &ATable) -> String { - let column_names = new - .columns - .iter() - .map(|col| helper::quote_reserved_word(col.name())) - .collect::>>() - .join(", "); - format!( - "INSERT INTO {} SELECT {} FROM {};", - helper::quote_reserved_word(&new.name), - column_names, - helper::quote_reserved_word(&old.name) - ) -} - -fn tmp_table_name(name: &str) -> String { - format!("{name}__butane_tmp") -} - fn change_column( current: &mut ADB, tbl_name: &str, old: &AColumn, - new: Option<&AColumn>, + new: &AColumn, ) -> Result { + use helper::quote_reserved_word; let table = current.get_table(tbl_name); if table.is_none() { crate::warn!( @@ -713,28 +695,114 @@ fn change_column( ); return Ok(String::new()); } - let old_table = table.unwrap(); - let mut new_table = old_table.clone(); - new_table.name = tmp_table_name(&new_table.name); - match new { - Some(col) => new_table.replace_column(col.clone()), - None => new_table.remove_column(old.name()), - } - let mut stmts: Vec = vec![ - create_table(&new_table, false)?, - create_table_constraints(&new_table), - copy_table(old_table, &new_table), - drop_table(&old_table.name), - format!( - "ALTER TABLE {} RENAME TO {};", - helper::quote_reserved_word(&new_table.name), - helper::quote_reserved_word(tbl_name) - ), - ]; - stmts.retain(|stmt| !stmt.is_empty()); + + // Let's figure out what changed about the column + let mut stmts: Vec = Vec::new(); + if old.name() != new.name() { + // column rename + stmts.push(format!( + "ALTER TABLE {} RENAME COLUMN {} TO {};", + quote_reserved_word(tbl_name), + quote_reserved_word(old.name()), + quote_reserved_word(new.name()) + )); + } + if old.typeid()? != new.typeid()? { + // column type change + stmts.push(format!( + "ALTER TABLE {} ALTER COLUMN {} TYPE {};", + quote_reserved_word(tbl_name), + quote_reserved_word(old.name()), + col_sqltype(new)? + )); + } + if old.nullable() != new.nullable() { + stmts.push(format!( + "ALTER TABLE {} ALTER COLUMN {} {} NOT NULL;", + quote_reserved_word(tbl_name), + quote_reserved_word(old.name()), + if new.nullable() { "SET" } else { "DROP" } + )); + } + if old.is_pk() != new.is_pk() { + // Change to primary key + // Either way, drop the previous primary key + // Butane does not currently support composite primary keys + + if new.is_pk() { + // Drop the old primary key + stmts.push(format!( + "ALTER TABLE {} DROP CONSTRAINT IF EXISTS {}_pkey;", + quote_reserved_word(tbl_name), + tbl_name + )); + + // add the new primary key + stmts.push(format!( + "ALTER TABLE {} ADD PRIMARY KEY {};", + quote_reserved_word(tbl_name), + quote_reserved_word(old.name()) + )); + } else { + // this field is no longer the primary key. Butane requires a single primary key, + // so some other column must be the primary key now. It will drop the constraint when processed. + } + } + if old.unique() != new.unique() { + // Changed uniqueness constraint + if new.unique() { + stmts.push(format!( + "ALTER TABLE {} ADD UNIQUE ({});", + quote_reserved_word(tbl_name), + quote_reserved_word(old.name()) + )); + } else { + // Standard constraint naming scheme + stmts.push(format!( + "ALTER TABLE {} DROP CONSTRAINT {}_{}_key;", + quote_reserved_word(tbl_name), + tbl_name, + &old.name() + )); + } + } + + if old.default() != new.default() { + stmts.push(match new.default() { + None => format!( + "ALTER TABLE {} ALTER COLUMN {} DROP DEFAULT;", + quote_reserved_word(tbl_name), + quote_reserved_word(old.name()) + ), + Some(val) => format!( + "ALTER TABLE {} ALTER COLUMN {} SET DEFAULT {};", + quote_reserved_word(tbl_name), + quote_reserved_word(old.name()), + helper::sql_literal_value(val)? + ), + }); + } + + if old.reference() != new.reference() { + if old.reference().is_some() { + // Drop the old reference + stmts.push(format!( + "ALTER TABLE {} DROP CONSTRAINT {}_{}_fkey;", + quote_reserved_word(tbl_name), + tbl_name, + old.name() + )); + } + if new.reference().is_some() { + stmts.push(define_constraint(tbl_name, new)); + } + } + + if stmts.is_empty() { + return Err(crate::Error::PgChangedColumnNotProcessed); + } + let result = stmts.join("\n"); - new_table.name.clone_from(&old_table.name); - current.replace_table(new_table); Ok(result) } diff --git a/butane_core/src/db/sqlite.rs b/butane_core/src/db/sqlite.rs index 6238dd54..71a3c34f 100644 --- a/butane_core/src/db/sqlite.rs +++ b/butane_core/src/db/sqlite.rs @@ -621,7 +621,7 @@ fn add_column(tbl_name: &str, col: &AColumn) -> Result { "ALTER TABLE {} ADD COLUMN {} DEFAULT {};", helper::quote_reserved_word(tbl_name), define_column(col), - helper::sql_literal_value(default)? + helper::sql_literal_value(&default)? )) } diff --git a/butane_core/src/lib.rs b/butane_core/src/lib.rs index 7f073a0f..b58c3bac 100644 --- a/butane_core/src/lib.rs +++ b/butane_core/src/lib.rs @@ -229,6 +229,8 @@ pub enum Error { LiteralForCustomUnsupported(custom::SqlValCustom), #[error("This DataObject doesn't support determining whether it has been saved.")] SaveDeterminationNotSupported, + #[error("No column changes found for changed column. Bug in pg backend.")] + PgChangedColumnNotProcessed, #[error("(De)serialization error {0}")] SerdeJson(#[from] serde_json::Error), #[error("IO error {0}")] diff --git a/examples/getting_started/.butane/migrations/20240115_023841384_dbconstraints/pg_down.sql b/examples/getting_started/.butane/migrations/20240115_023841384_dbconstraints/pg_down.sql index b62d930f..1eb956aa 100644 --- a/examples/getting_started/.butane/migrations/20240115_023841384_dbconstraints/pg_down.sql +++ b/examples/getting_started/.butane/migrations/20240115_023841384_dbconstraints/pg_down.sql @@ -1,27 +1,3 @@ -CREATE TABLE Post__butane_tmp ( -id SERIAL NOT NULL PRIMARY KEY, -title TEXT NOT NULL, -body TEXT NOT NULL, -published BOOLEAN NOT NULL, -blog BIGINT NOT NULL, -byline TEXT , -likes INTEGER NOT NULL -); -INSERT INTO Post__butane_tmp SELECT id, title, body, published, blog, byline, likes FROM Post; -DROP TABLE Post; -ALTER TABLE Post__butane_tmp RENAME TO Post; -CREATE TABLE Post_tags_Many__butane_tmp ( -owner INTEGER NOT NULL, -has TEXT NOT NULL -); -ALTER TABLE Post_tags_Many__butane_tmp ADD FOREIGN KEY (owner) REFERENCES Post(id); -INSERT INTO Post_tags_Many__butane_tmp SELECT owner, has FROM Post_tags_Many; -DROP TABLE Post_tags_Many; -ALTER TABLE Post_tags_Many__butane_tmp RENAME TO Post_tags_Many; -CREATE TABLE Post_tags_Many__butane_tmp ( -owner INTEGER NOT NULL, -has TEXT NOT NULL -); -INSERT INTO Post_tags_Many__butane_tmp SELECT owner, has FROM Post_tags_Many; -DROP TABLE Post_tags_Many; -ALTER TABLE Post_tags_Many__butane_tmp RENAME TO Post_tags_Many; +ALTER TABLE Post DROP CONSTRAINT Post_blog_fkey; +ALTER TABLE Post_tags_Many DROP CONSTRAINT Post_tags_Many_has_fkey; +ALTER TABLE Post_tags_Many DROP CONSTRAINT Post_tags_Many_owner_fkey; diff --git a/examples/getting_started/.butane/migrations/20240115_023841384_dbconstraints/pg_up.sql b/examples/getting_started/.butane/migrations/20240115_023841384_dbconstraints/pg_up.sql index a023e1e0..773142e0 100644 --- a/examples/getting_started/.butane/migrations/20240115_023841384_dbconstraints/pg_up.sql +++ b/examples/getting_started/.butane/migrations/20240115_023841384_dbconstraints/pg_up.sql @@ -1,30 +1,3 @@ -CREATE TABLE Post__butane_tmp ( -id SERIAL NOT NULL PRIMARY KEY, -title TEXT NOT NULL, -body TEXT NOT NULL, -published BOOLEAN NOT NULL, -blog BIGINT NOT NULL, -byline TEXT , -likes INTEGER NOT NULL -); -ALTER TABLE Post__butane_tmp ADD FOREIGN KEY (blog) REFERENCES Blog(id); -INSERT INTO Post__butane_tmp SELECT id, title, body, published, blog, byline, likes FROM Post; -DROP TABLE Post; -ALTER TABLE Post__butane_tmp RENAME TO Post; -CREATE TABLE Post_tags_Many__butane_tmp ( -owner INTEGER NOT NULL, -has TEXT NOT NULL -); -ALTER TABLE Post_tags_Many__butane_tmp ADD FOREIGN KEY (has) REFERENCES Tag(tag); -INSERT INTO Post_tags_Many__butane_tmp SELECT owner, has FROM Post_tags_Many; -DROP TABLE Post_tags_Many; -ALTER TABLE Post_tags_Many__butane_tmp RENAME TO Post_tags_Many; -CREATE TABLE Post_tags_Many__butane_tmp ( -owner INTEGER NOT NULL, -has TEXT NOT NULL -); -ALTER TABLE Post_tags_Many__butane_tmp ADD FOREIGN KEY (owner) REFERENCES Post(id); -ALTER TABLE Post_tags_Many__butane_tmp ADD FOREIGN KEY (has) REFERENCES Tag(tag); -INSERT INTO Post_tags_Many__butane_tmp SELECT owner, has FROM Post_tags_Many; -DROP TABLE Post_tags_Many; -ALTER TABLE Post_tags_Many__butane_tmp RENAME TO Post_tags_Many; +ALTER TABLE Post ADD FOREIGN KEY (blog) REFERENCES Blog(id); +ALTER TABLE Post_tags_Many ADD FOREIGN KEY (has) REFERENCES Tag(tag); +ALTER TABLE Post_tags_Many ADD FOREIGN KEY (owner) REFERENCES Post(id); diff --git a/examples/getting_started/src/butane_migrations.rs b/examples/getting_started/src/butane_migrations.rs index 5c045ef9..f0e772ab 100644 --- a/examples/getting_started/src/butane_migrations.rs +++ b/examples/getting_started/src/butane_migrations.rs @@ -530,11 +530,11 @@ pub fn get_migrations() -> Result { }, "from": "20201229_171630604_likes", "up": { - "pg": "CREATE TABLE Post__butane_tmp (\nid SERIAL NOT NULL PRIMARY KEY,\ntitle TEXT NOT NULL,\nbody TEXT NOT NULL,\npublished BOOLEAN NOT NULL,\nblog BIGINT NOT NULL,\nbyline TEXT ,\nlikes INTEGER NOT NULL\n);\nALTER TABLE Post__butane_tmp ADD FOREIGN KEY (blog) REFERENCES Blog(id);\nINSERT INTO Post__butane_tmp SELECT id, title, body, published, blog, byline, likes FROM Post;\nDROP TABLE Post;\nALTER TABLE Post__butane_tmp RENAME TO Post;\nCREATE TABLE Post_tags_Many__butane_tmp (\nowner INTEGER NOT NULL,\nhas TEXT NOT NULL\n);\nALTER TABLE Post_tags_Many__butane_tmp ADD FOREIGN KEY (has) REFERENCES Tag(tag);\nINSERT INTO Post_tags_Many__butane_tmp SELECT owner, has FROM Post_tags_Many;\nDROP TABLE Post_tags_Many;\nALTER TABLE Post_tags_Many__butane_tmp RENAME TO Post_tags_Many;\nCREATE TABLE Post_tags_Many__butane_tmp (\nowner INTEGER NOT NULL,\nhas TEXT NOT NULL\n);\nALTER TABLE Post_tags_Many__butane_tmp ADD FOREIGN KEY (owner) REFERENCES Post(id);\nALTER TABLE Post_tags_Many__butane_tmp ADD FOREIGN KEY (has) REFERENCES Tag(tag);\nINSERT INTO Post_tags_Many__butane_tmp SELECT owner, has FROM Post_tags_Many;\nDROP TABLE Post_tags_Many;\nALTER TABLE Post_tags_Many__butane_tmp RENAME TO Post_tags_Many;\n", + "pg": "ALTER TABLE Post ADD FOREIGN KEY (blog) REFERENCES Blog(id);\nALTER TABLE Post_tags_Many ADD FOREIGN KEY (has) REFERENCES Tag(tag);\nALTER TABLE Post_tags_Many ADD FOREIGN KEY (owner) REFERENCES Post(id);\n", "sqlite": "CREATE TABLE Post__butane_tmp (\nid INTEGER NOT NULL PRIMARY KEY,\ntitle TEXT NOT NULL,\nbody TEXT NOT NULL,\npublished INTEGER NOT NULL,\nblog INTEGER NOT NULL,\nbyline TEXT,\nlikes INTEGER NOT NULL,\nFOREIGN KEY (blog) REFERENCES Blog(id)\n);\nINSERT INTO Post__butane_tmp SELECT id, title, body, published, blog, byline, likes FROM Post;\nDROP TABLE Post;\nALTER TABLE Post__butane_tmp RENAME TO Post;\nCREATE TABLE Post_tags_Many__butane_tmp (\nowner INTEGER NOT NULL,\nhas TEXT NOT NULL,\nFOREIGN KEY (has) REFERENCES Tag(tag)\n);\nINSERT INTO Post_tags_Many__butane_tmp SELECT owner, has FROM Post_tags_Many;\nDROP TABLE Post_tags_Many;\nALTER TABLE Post_tags_Many__butane_tmp RENAME TO Post_tags_Many;\nCREATE TABLE Post_tags_Many__butane_tmp (\nowner INTEGER NOT NULL,\nhas TEXT NOT NULL,\nFOREIGN KEY (owner) REFERENCES Post(id)\nFOREIGN KEY (has) REFERENCES Tag(tag)\n);\nINSERT INTO Post_tags_Many__butane_tmp SELECT owner, has FROM Post_tags_Many;\nDROP TABLE Post_tags_Many;\nALTER TABLE Post_tags_Many__butane_tmp RENAME TO Post_tags_Many;\n" }, "down": { - "pg": "CREATE TABLE Post__butane_tmp (\nid SERIAL NOT NULL PRIMARY KEY,\ntitle TEXT NOT NULL,\nbody TEXT NOT NULL,\npublished BOOLEAN NOT NULL,\nblog BIGINT NOT NULL,\nbyline TEXT ,\nlikes INTEGER NOT NULL\n);\nINSERT INTO Post__butane_tmp SELECT id, title, body, published, blog, byline, likes FROM Post;\nDROP TABLE Post;\nALTER TABLE Post__butane_tmp RENAME TO Post;\nCREATE TABLE Post_tags_Many__butane_tmp (\nowner INTEGER NOT NULL,\nhas TEXT NOT NULL\n);\nALTER TABLE Post_tags_Many__butane_tmp ADD FOREIGN KEY (owner) REFERENCES Post(id);\nINSERT INTO Post_tags_Many__butane_tmp SELECT owner, has FROM Post_tags_Many;\nDROP TABLE Post_tags_Many;\nALTER TABLE Post_tags_Many__butane_tmp RENAME TO Post_tags_Many;\nCREATE TABLE Post_tags_Many__butane_tmp (\nowner INTEGER NOT NULL,\nhas TEXT NOT NULL\n);\nINSERT INTO Post_tags_Many__butane_tmp SELECT owner, has FROM Post_tags_Many;\nDROP TABLE Post_tags_Many;\nALTER TABLE Post_tags_Many__butane_tmp RENAME TO Post_tags_Many;\n", + "pg": "ALTER TABLE Post DROP CONSTRAINT Post_blog_fkey;\nALTER TABLE Post_tags_Many DROP CONSTRAINT Post_tags_Many_has_fkey;\nALTER TABLE Post_tags_Many DROP CONSTRAINT Post_tags_Many_owner_fkey;\n", "sqlite": "CREATE TABLE Post__butane_tmp (\nid INTEGER NOT NULL PRIMARY KEY,\ntitle TEXT NOT NULL,\nbody TEXT NOT NULL,\npublished INTEGER NOT NULL,\nblog INTEGER NOT NULL,\nbyline TEXT,\nlikes INTEGER NOT NULL\n);\nINSERT INTO Post__butane_tmp SELECT id, title, body, published, blog, byline, likes FROM Post;\nDROP TABLE Post;\nALTER TABLE Post__butane_tmp RENAME TO Post;\nCREATE TABLE Post_tags_Many__butane_tmp (\nowner INTEGER NOT NULL,\nhas TEXT NOT NULL,\nFOREIGN KEY (owner) REFERENCES Post(id)\n);\nINSERT INTO Post_tags_Many__butane_tmp SELECT owner, has FROM Post_tags_Many;\nDROP TABLE Post_tags_Many;\nALTER TABLE Post_tags_Many__butane_tmp RENAME TO Post_tags_Many;\nCREATE TABLE Post_tags_Many__butane_tmp (\nowner INTEGER NOT NULL,\nhas TEXT NOT NULL\n);\nINSERT INTO Post_tags_Many__butane_tmp SELECT owner, has FROM Post_tags_Many;\nDROP TABLE Post_tags_Many;\nALTER TABLE Post_tags_Many__butane_tmp RENAME TO Post_tags_Many;\n" } } diff --git a/examples/getting_started/tests/rollback.rs b/examples/getting_started/tests/rollback.rs index 7932fc8b..19c250e0 100644 --- a/examples/getting_started/tests/rollback.rs +++ b/examples/getting_started/tests/rollback.rs @@ -50,16 +50,6 @@ fn migrate_and_rollback(mut connection: Connection) { // Rollback migrations. for migration in to_apply.iter().rev() { - if connection.backend_name() == "pg" - && migration.name() == "20240115_023841384_dbconstraints" - { - // migration 20240115_023841384_dbconstraints failed: Postgres error db error: - // ERROR: cannot drop table post because other objects depend on it - // DETAIL: constraint post_tags_many__butane_tmp_owner_fkey on table post_tags_many depends on table post - let err = migration.apply(&mut connection).unwrap_err(); - eprintln!("Migration {} failed: {err:?}", migration.name()); - return; - } migration .downgrade(&mut connection) .unwrap_or_else(|err| panic!("rollback of {} failed: {err}", migration.name())); From 788a3784e37ead8bb1c2008db47b3fdd96bf6344 Mon Sep 17 00:00:00 2001 From: James Oakley Date: Tue, 16 Apr 2024 21:36:19 -0400 Subject: [PATCH 2/3] Tests and fixes --- butane/tests/migration-tests.rs | 153 ++++++++++++++++++++++++++++++++ butane_core/src/db/pg.rs | 16 ++-- butane_core/src/lib.rs | 2 - 3 files changed, 159 insertions(+), 12 deletions(-) diff --git a/butane/tests/migration-tests.rs b/butane/tests/migration-tests.rs index 98738f13..50f42f0c 100644 --- a/butane/tests/migration-tests.rs +++ b/butane/tests/migration-tests.rs @@ -227,6 +227,50 @@ fn migration_add_field_with_default_pg() { ); } +#[cfg(feature = "pg")] +#[test] +fn migration_modify_field_pg() { + let (mut conn, _data) = pg_connection(); + // Not verifying rename right now because we don't detect it + // https://github.com/Electron100/butane/issues/89 + + migration_modify_field_type_change( + &mut conn, + "ALTER TABLE Foo ALTER COLUMN bar SET DATA TYPE BIGINT;", + "ALTER TABLE Foo ALTER COLUMN bar SET DATA TYPE INTEGER;", + ); + + migration_modify_field_nullability_change( + &mut conn, + "ALTER TABLE Foo ALTER COLUMN bar DROP NOT NULL;", + "ALTER TABLE Foo ALTER COLUMN bar SET NOT NULL;", + ); + + migration_modify_field_pkey_change( + &mut conn, + "ALTER TABLE Foo DROP CONSTRAINT IF EXISTS Foo_pkey;\nALTER TABLE Foo ADD PRIMARY KEY (baz);", + "ALTER TABLE Foo DROP CONSTRAINT IF EXISTS Foo_pkey;\nALTER TABLE Foo ADD PRIMARY KEY (bar);", + ); + + migration_modify_field_uniqueness_change( + &mut conn, + "ALTER TABLE Foo ADD UNIQUE (bar);", + "ALTER TABLE Foo DROP CONSTRAINT Foo_bar_key;", + ); + + migration_modify_field_default_added( + &mut conn, + "ALTER TABLE Foo ALTER COLUMN bar SET DEFAULT 42;", + "ALTER TABLE Foo ALTER COLUMN bar DROP DEFAULT;", + ); + + migration_modify_field_different_default( + &mut conn, + "ALTER TABLE Foo ALTER COLUMN bar SET DEFAULT 42;", + "ALTER TABLE Foo ALTER COLUMN bar SET DEFAULT 41;", + ); +} + #[cfg(feature = "sqlite")] #[test] fn migration_add_and_remove_field_sqlite() { @@ -302,6 +346,7 @@ fn test_migrate( let mut to_apply = ms.unapplied_migrations(conn).unwrap(); assert_eq!(to_apply.len(), 2); for m in &to_apply { + eprintln!("{}", m.up_sql(backends[0].name()).unwrap().unwrap()); m.apply(conn).unwrap(); } verify_sql(conn, &ms, expected_up_sql, expected_down_sql); @@ -371,6 +416,114 @@ fn migration_add_field_with_default(conn: &mut Connection, up_sql: &str, down_sq test_migrate(conn, init, v2, up_sql, down_sql); } +fn migration_modify_field_type_change(conn: &mut Connection, up_sql: &str, down_sql: &str) { + let init = quote! { + struct Foo { + id: i64, + bar: i32, + } + }; + + let v2 = quote! { + struct Foo { + id: i64, + bar: i64, + } + }; + test_migrate(conn, init, v2, up_sql, down_sql); +} + +fn migration_modify_field_nullability_change(conn: &mut Connection, up_sql: &str, down_sql: &str) { + let init = quote! { + struct Foo { + id: i64, + bar: i32, + } + }; + + let v2 = quote! { + struct Foo { + id: i64, + bar: Option, + } + }; + test_migrate(conn, init, v2, up_sql, down_sql); +} + +fn migration_modify_field_uniqueness_change(conn: &mut Connection, up_sql: &str, down_sql: &str) { + let init = quote! { + struct Foo { + id: i64, + bar: i32, + } + }; + + let v2 = quote! { + struct Foo { + id: i64, + #[unique] + bar: i32, + } + }; + test_migrate(conn, init, v2, up_sql, down_sql); +} + +fn migration_modify_field_pkey_change(conn: &mut Connection, up_sql: &str, down_sql: &str) { + let init = quote! { + struct Foo { + #[pk] + bar: i64, + baz: i32, + } + }; + + let v2 = quote! { + struct Foo { + bar: i64, + #[pk] + baz: i32 + } + }; + test_migrate(conn, init, v2, up_sql, down_sql); +} + +fn migration_modify_field_default_added(conn: &mut Connection, up_sql: &str, down_sql: &str) { + let init = quote! { + struct Foo { + id: i64, + bar: String, + } + }; + + let v2 = quote! { + struct Foo { + id: i64, + #[default=42] + bar: String, + } + }; + test_migrate(conn, init, v2, up_sql, down_sql); +} + +fn migration_modify_field_different_default(conn: &mut Connection, up_sql: &str, down_sql: &str) { + let init = quote! { + struct Foo { + id: i64, + #[default=41] + bar: String, + } + }; + + let v2 = quote! { + struct Foo { + id: i64, + #[default=42] + bar: String, + } + }; + test_migrate(conn, init, v2, up_sql, down_sql); +} + fn migration_add_and_remove_field(conn: &mut Connection, up_sql: &str, down_sql: &str) { let init = quote! { struct Foo { diff --git a/butane_core/src/db/pg.rs b/butane_core/src/db/pg.rs index fdcaec1c..d7b084bd 100644 --- a/butane_core/src/db/pg.rs +++ b/butane_core/src/db/pg.rs @@ -710,10 +710,10 @@ fn change_column( if old.typeid()? != new.typeid()? { // column type change stmts.push(format!( - "ALTER TABLE {} ALTER COLUMN {} TYPE {};", + "ALTER TABLE {} ALTER COLUMN {} SET DATA TYPE {};", quote_reserved_word(tbl_name), quote_reserved_word(old.name()), - col_sqltype(new)? + col_sqltype(new)?, )); } if old.nullable() != new.nullable() { @@ -721,7 +721,7 @@ fn change_column( "ALTER TABLE {} ALTER COLUMN {} {} NOT NULL;", quote_reserved_word(tbl_name), quote_reserved_word(old.name()), - if new.nullable() { "SET" } else { "DROP" } + if new.nullable() { "DROP" } else { "SET" } )); } if old.is_pk() != new.is_pk() { @@ -739,9 +739,9 @@ fn change_column( // add the new primary key stmts.push(format!( - "ALTER TABLE {} ADD PRIMARY KEY {};", + "ALTER TABLE {} ADD PRIMARY KEY ({});", quote_reserved_word(tbl_name), - quote_reserved_word(old.name()) + quote_reserved_word(new.name()) )); } else { // this field is no longer the primary key. Butane requires a single primary key, @@ -754,7 +754,7 @@ fn change_column( stmts.push(format!( "ALTER TABLE {} ADD UNIQUE ({});", quote_reserved_word(tbl_name), - quote_reserved_word(old.name()) + quote_reserved_word(new.name()) )); } else { // Standard constraint naming scheme @@ -798,10 +798,6 @@ fn change_column( } } - if stmts.is_empty() { - return Err(crate::Error::PgChangedColumnNotProcessed); - } - let result = stmts.join("\n"); Ok(result) } diff --git a/butane_core/src/lib.rs b/butane_core/src/lib.rs index b58c3bac..7f073a0f 100644 --- a/butane_core/src/lib.rs +++ b/butane_core/src/lib.rs @@ -229,8 +229,6 @@ pub enum Error { LiteralForCustomUnsupported(custom::SqlValCustom), #[error("This DataObject doesn't support determining whether it has been saved.")] SaveDeterminationNotSupported, - #[error("No column changes found for changed column. Bug in pg backend.")] - PgChangedColumnNotProcessed, #[error("(De)serialization error {0}")] SerdeJson(#[from] serde_json::Error), #[error("IO error {0}")] From 7629125e1467d917ed302c6414ec98c42f95b4d2 Mon Sep 17 00:00:00 2001 From: James Oakley Date: Wed, 17 Apr 2024 08:25:33 -0400 Subject: [PATCH 3/3] Remove leftover debugging print --- butane/tests/migration-tests.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/butane/tests/migration-tests.rs b/butane/tests/migration-tests.rs index 50f42f0c..864c2ffb 100644 --- a/butane/tests/migration-tests.rs +++ b/butane/tests/migration-tests.rs @@ -346,7 +346,6 @@ fn test_migrate( let mut to_apply = ms.unapplied_migrations(conn).unwrap(); assert_eq!(to_apply.len(), 2); for m in &to_apply { - eprintln!("{}", m.up_sql(backends[0].name()).unwrap().unwrap()); m.apply(conn).unwrap(); } verify_sql(conn, &ms, expected_up_sql, expected_down_sql);