Merge pull request #251 from rust-lang/fix-migrations

Fix column migrations without `DEFAULT` clause and add a test for it

Author: Kobzol

Cargo.lock

@@ -63,6 +63,7 @@ tracing-test = "0.2.4"
 regex = "1.10.4"
 parking_lot = "0.12.3"
 thread_local = "1.1.8"
+sqlparser = { version = "0.55", features = ["visitor"] }
 
 [profile.release]
 debug = 1

Cargo.toml

@@ -104,6 +104,11 @@ Having the database up and running, with the `DATABASE_URL` set, is required to
 Make sure to also run `cargo sqlx migrate run` to apply the migrations to the database.
 
 ### Updating the DB schema
+
+> [!CAUTION]
+> When adding a new `NOT NULL` column, always specify the `DEFAULT` value that will be backfilled
+> during the migration! Otherwise, the migration might break the deployed bors service.
+
 1) Generate a new migration
     ```console
     $ cargo sqlx migrate add <new-migration>

docs/development.md

@@ -1,2 +1,3 @@
 -- Add up migration script here
-ALTER TABLE pull_request ADD COLUMN base_branch TEXT NOT NULL;
+ALTER TABLE pull_request
+    ADD COLUMN base_branch TEXT NOT NULL DEFAULT 'master';

migrations/20250313013946_add_base_branch_to_pr.up.sql

@@ -1,2 +1,3 @@
 -- Add up migration script here
-ALTER TABLE pull_request ADD COLUMN status TEXT NOT NULL;
+ALTER TABLE pull_request
+    ADD COLUMN status TEXT NOT NULL DEFAULT 'open';

migrations/20250315140840_add_merge_to_pr.down.sql renamed to migrations/20250315140840_add_merge_state_to_pr.down.sql

@@ -0,0 +1,117 @@
+use itertools::Itertools;
+use sqlparser::ast::{
+    AlterColumnOperation, AlterTableOperation, ColumnOption, Ident, ObjectName, Statement, Visit,
+    Visitor,
+};
+use sqlparser::dialect::PostgreSqlDialect;
+use sqlparser::parser::Parser;
+use std::collections::{BTreeSet, HashSet};
+use std::ffi::OsStr;
+use std::ops::ControlFlow;
+use std::path::PathBuf;
+
+struct CheckNotNullWithoutDefault {
+    error: Option<String>,
+    columns_set_to_not_null: HashSet<(ObjectName, Ident)>,
+    columns_set_default_value: HashSet<(ObjectName, Ident)>,
+}
+
+impl Visitor for CheckNotNullWithoutDefault {
+    type Break = ();
+
+    fn pre_visit_statement(&mut self, statement: &Statement) -> ControlFlow<Self::Break> {
+        if let Statement::AlterTable {
+            operations, name, ..
+        } = statement
+        {
+            for op in operations {
+                match op {
+                    AlterTableOperation::AddColumn { column_def, .. } => {
+                        let has_not_null = column_def
+                            .options
+                            .iter()
+                            .any(|option| option.option == ColumnOption::NotNull);
+                        let has_default = column_def
+                            .options
+                            .iter()
+                            .any(|option| matches!(option.option, ColumnOption::Default(_)));
+                        if has_not_null && !has_default {
+                            self.error = Some(format!(
+                                "Column `{name}.{}` is NOT NULL, but no DEFAULT value was configured!",
+                                column_def.name
+                            ));
+                            return ControlFlow::Break(());
+                        }
+                    }
+                    AlterTableOperation::AlterColumn { column_name, op } => match op {
+                        AlterColumnOperation::SetNotNull => {
+                            self.columns_set_to_not_null
+                                .insert((name.clone(), column_name.clone()));
+                        }
+                        AlterColumnOperation::SetDefault { .. } => {
+                            self.columns_set_default_value
+                                .insert((name.clone(), column_name.clone()));
+                        }
+                        _ => {}
+                    },
+                    _ => {}
+                }
+            }
+        }
+        ControlFlow::Continue(())
+    }
+}
+
+impl CheckNotNullWithoutDefault {
+    fn compute_error(self) -> Option<String> {
+        if let Some(error) = self.error {
+            return Some(error);
+        }
+
+        let missing_default = self
+            .columns_set_to_not_null
+            .difference(&self.columns_set_default_value)
+            .collect::<BTreeSet<_>>();
+        if !missing_default.is_empty() {
+            return Some(format!(
+                "Column(s) {} were modified to NOT NULL, but no DEFAULT value was set for them",
+                missing_default.iter().map(|v| format!("{v:?}")).join(",")
+            ));
+        }
+
+        None
+    }
+}
+
+/// Check that there is no migration that would add a NOT NULL column (or make an existing column
+/// NOT NULL) without also providing a DEFAULT value.
+#[test]
+fn check_non_null_column_without_default() {
+    let root = env!("CARGO_MANIFEST_DIR");
+    let migrations = PathBuf::from(root).join("migrations");
+    for file in std::fs::read_dir(migrations).unwrap() {
+        let file = file.unwrap();
+        if file.path().extension() == Some(OsStr::new("sql")) {
+            let contents =
+                std::fs::read_to_string(&file.path()).expect("cannot read migration file");
+
+            let ast = Parser::parse_sql(&PostgreSqlDialect {}, &contents).expect(&format!(
+                "Cannot parse migration {} as SQLL",
+                file.path().display()
+            ));
+            let mut visitor = CheckNotNullWithoutDefault {
+                error: None,
+                columns_set_to_not_null: Default::default(),
+                columns_set_default_value: Default::default(),
+            };
+            ast.visit(&mut visitor);
+
+            if let Some(error) = visitor.compute_error() {
+                panic!(
+                    "Migration {} contains error: {error}",
+                    file.path().display()
+                );
+            }
+        }
+    }
+}