From 7cc4f23db9f0f7ecceddbadc787fc9c2aa874d5b Mon Sep 17 00:00:00 2001 From: fabianlindfors Date: Fri, 4 Feb 2022 17:47:39 +0100 Subject: [PATCH] Fix changing nullability not working with alter_column The existing test to set a column as NOT NULL didn't actually check that the constraint was added, which has also been fixed. I also added a new test for the opposite change, making a NOT NULL column nullable. The README has also been updated with a new example for setting a column as NOT NULL. --- README.md | 17 +++++- src/migrations/alter_column.rs | 2 +- tests/alter_column.rs | 101 ++++++++++++++++++++++++++++++++- 3 files changed, 117 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 511f1b7..6a9fb2e 100644 --- a/README.md +++ b/README.md @@ -351,7 +351,7 @@ up = "data['path']['to']['value'] #>> '{}'" The `alter_column` action enables many different changes to an existing column, for example renaming, changing type and changing existing values. -When performing more complex changes than a rename, `up` and `down` must be provided. These should be SQL expressions which determine how to transform between the new and old version of the column. Inside those expressions, you can reference the current column value by the column name. +When performing more complex changes than a rename, `up` and `down` should be provided. These should be SQL expressions which determine how to transform between the new and old version of the column. Inside those expressions, you can reference the current column value by the column name. *Example: rename `last_name` column on `users` table to `family_name`* @@ -395,6 +395,21 @@ down = "index - 1" # Decrement to revert for old schema name = "index" ``` +*Example: make `name` column not nullable`* + +```toml +[[actions]] +type = "alter_column" +table = "users" +column = "name" + +# Use "N/A" for any rows that currently have a NULL name +up = "COALESCE(name, 'N/A')" + + [actions.changes] + nullable = false +``` + *Example: change default value of `created_at` column to current time* ```toml diff --git a/src/migrations/alter_column.rs b/src/migrations/alter_column.rs index f943800..e3d7bd1 100644 --- a/src/migrations/alter_column.rs +++ b/src/migrations/alter_column.rs @@ -187,7 +187,7 @@ impl Action for AlterColumn { // This constraint is set as NOT VALID so it doesn't apply to existing rows and // the existing rows don't need to be scanned under an exclusive lock. // Thanks to this, we can set the full column as NOT NULL later with minimal locking. - if !column.nullable { + if !self.changes.nullable.unwrap_or(column.nullable) { let query = format!( r#" ALTER TABLE "{table}" diff --git a/tests/alter_column.rs b/tests/alter_column.rs index 2dad0e4..256d88f 100644 --- a/tests/alter_column.rs +++ b/tests/alter_column.rs @@ -113,7 +113,6 @@ fn alter_column_set_not_null() { table = "users" column = "name" up = "COALESCE(name, 'TEST_DEFAULT_VALUE')" - down = "name" [actions.changes] nullable = false @@ -159,6 +158,106 @@ fn alter_column_set_not_null() { .query_one("SELECT name from users WHERE id = 4", &[]) .unwrap(); assert_eq!("Jane Doe", result.get::<_, &str>("name")); + + // Ensure NULL can't be inserted using the new schema + let result = new_db.simple_query("INSERT INTO users (id, name) VALUES (5, NULL)"); + assert!(result.is_err(), "expected insert to fail"); + }); + + test.after_completion(|db| { + // Ensure NULL can't be inserted + let result = db.simple_query("INSERT INTO users (id, name) VALUES (5, NULL)"); + assert!(result.is_err(), "expected insert to fail"); + }); + + test.after_abort(|db| { + // Ensure NULL can be inserted + let result = db.simple_query("INSERT INTO users (id, name) VALUES (5, NULL)"); + assert!(result.is_ok(), "expected insert to succeed"); + }); + + test.run(); +} + +#[test] +fn alter_column_set_nullable() { + let mut test = Test::new("Set column nullable"); + + test.first_migration( + r#" + name = "create_user_table" + + [[actions]] + type = "create_table" + name = "users" + primary_key = ["id"] + + [[actions.columns]] + name = "id" + type = "INTEGER" + + [[actions.columns]] + name = "name" + type = "TEXT" + nullable = false + "#, + ); + + test.second_migration( + r#" + name = "set_name_nullable" + + [[actions]] + type = "alter_column" + table = "users" + column = "name" + down = "COALESCE(name, 'TEST_DEFAULT_VALUE')" + + [actions.changes] + nullable = true + "#, + ); + + test.after_first(|db| { + // Insert a test user + db.simple_query( + " + INSERT INTO users (id, name) VALUES + (1, 'John Doe') + ", + ) + .unwrap(); + }); + + test.intermediate(|old_db, new_db| { + // Insert data using new schema and make sure the old schema gets correct values + new_db + .simple_query("INSERT INTO users (id, name) VALUES (2, NULL)") + .unwrap(); + let result = old_db + .query_one("SELECT name from users WHERE id = 2", &[]) + .unwrap(); + assert_eq!("TEST_DEFAULT_VALUE", result.get::<_, &str>("name")); + + // Ensure NULL can't be inserted using the old schema + let result = old_db.simple_query("INSERT INTO users (id, name) VALUES (3, NULL)"); + assert!(result.is_err(), "expected insert to fail"); + + // Ensure NULL can be inserted using the new schema + let result = new_db.simple_query("INSERT INTO users (id, name) VALUES (4, NULL)"); + assert!(result.is_ok(), "expected insert to succeed"); + }); + + test.after_completion(|db| { + // Ensure NULL can be inserted + let result = db.simple_query("INSERT INTO users (id, name) VALUES (5, NULL)"); + assert!(result.is_ok(), "expected insert to succeed"); + }); + + test.after_abort(|db| { + // Ensure NULL can't be inserted + let result = db.simple_query("INSERT INTO users (id, name) VALUES (5, NULL)"); + assert!(result.is_err(), "expected insert to fail"); }); test.run();