diff --git a/crates/squawk_linter/src/rules/adding_not_null_field.rs b/crates/squawk_linter/src/rules/adding_not_null_field.rs index 4cdb252b..dc3323a2 100644 --- a/crates/squawk_linter/src/rules/adding_not_null_field.rs +++ b/crates/squawk_linter/src/rules/adding_not_null_field.rs @@ -51,6 +51,9 @@ pub(crate) fn adding_not_null_field(ctx: &mut Linter, parse: &Parse) let mut not_null_constraints: HashMap = HashMap::new(); let mut validated_not_null_columns: HashSet = HashSet::new(); + // Tables where VALIDATE CONSTRAINT was seen without a matching ADD CONSTRAINT + // in the same file (cross-migration pattern). + let mut tables_with_external_validated_constraints: HashSet = HashSet::new(); for stmt in file.stmts() { if let ast::Stmt::AlterTable(alter_table) = stmt { @@ -87,10 +90,17 @@ pub(crate) fn adding_not_null_field(ctx: &mut Linter, parse: &Parse) if let Some(constraint_name) = validate_constraint .name_ref() .map(|x| Identifier::new(&x.text())) - && let Some(table_column) = not_null_constraints.get(&constraint_name) - && table_column.table == table { - validated_not_null_columns.insert(table_column.clone()); + if let Some(table_column) = not_null_constraints.get(&constraint_name) + && table_column.table == table + { + validated_not_null_columns.insert(table_column.clone()); + } else { + // Cross-migration pattern: the ADD CONSTRAINT ... NOT VALID + // was in a previous migration file. Track the table so that + // a subsequent SET NOT NULL is considered safe. + tables_with_external_validated_constraints.insert(table.clone()); + } } } // Step 3: Check that we're altering a validated constraint @@ -109,7 +119,9 @@ pub(crate) fn adding_not_null_field(ctx: &mut Linter, parse: &Parse) table: table.clone(), column, }; - if validated_not_null_columns.contains(&table_column) { + if validated_not_null_columns.contains(&table_column) + || tables_with_external_validated_constraints.contains(&table) + { continue; } } @@ -301,6 +313,65 @@ ALTER TABLE foo ALTER COLUMN bar SET NOT NULL; )); } + // Cross-migration pattern: ADD CONSTRAINT was in a previous migration file, + // so only VALIDATE CONSTRAINT + SET NOT NULL appear in this file. + #[test] + fn pg16_cross_migration_validate_then_set_not_null_ok() { + let sql = r#" +ALTER TABLE foo VALIDATE CONSTRAINT foo_bar_not_null; + +ALTER TABLE foo +ALTER COLUMN bar +SET NOT NULL; + +ALTER TABLE foo +DROP CONSTRAINT foo_bar_not_null; + "#; + lint_ok_with( + sql, + LinterSettings { + pg_version: "16".parse().expect("Invalid PostgreSQL version"), + ..Default::default() + }, + Rule::AddingNotNullableField, + ); + } + + #[test] + fn pg12_cross_migration_validate_then_set_not_null_ok() { + let sql = r#" +ALTER TABLE foo VALIDATE CONSTRAINT foo_bar_not_null; +ALTER TABLE foo ALTER COLUMN bar SET NOT NULL; +ALTER TABLE foo DROP CONSTRAINT foo_bar_not_null; + "#; + lint_ok_with( + sql, + LinterSettings { + pg_version: "12".parse().expect("Invalid PostgreSQL version"), + ..Default::default() + }, + Rule::AddingNotNullableField, + ); + } + + #[test] + fn pg11_cross_migration_validate_then_set_not_null_err() { + // PostgreSQL 11 doesn't support using CHECK constraint to skip table scan + let sql = r#" +ALTER TABLE foo VALIDATE CONSTRAINT foo_bar_not_null; +ALTER TABLE foo ALTER COLUMN bar SET NOT NULL; +ALTER TABLE foo DROP CONSTRAINT foo_bar_not_null; + "#; + assert_snapshot!(lint_errors_with( + sql, + LinterSettings { + pg_version: "11".parse().expect("Invalid PostgreSQL version"), + ..Default::default() + }, + Rule::AddingNotNullableField + )); + } + #[test] fn pg12_with_different_column_validated_err() { // Validated CHECK exists for a different column diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_not_null_field__test__pg11_cross_migration_validate_then_set_not_null_err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_not_null_field__test__pg11_cross_migration_validate_then_set_not_null_err.snap new file mode 100644 index 00000000..d189f536 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_not_null_field__test__pg11_cross_migration_validate_then_set_not_null_err.snap @@ -0,0 +1,11 @@ +--- +source: crates/squawk_linter/src/rules/adding_not_null_field.rs +assertion_line: 368 +expression: "lint_errors_with(sql, LinterSettings\n{\n pg_version: \"11\".parse().expect(\"Invalid PostgreSQL version\"),\n ..Default::default()\n}, Rule::AddingNotNullableField)" +--- +warning[adding-not-nullable-field]: Setting a column `NOT NULL` blocks reads while the table is scanned. + ╭▸ +3 │ ALTER TABLE foo ALTER COLUMN bar SET NOT NULL; + │ ━━━━━━━━━━━━ + │ + ╰ help: Make the field nullable and use a `CHECK` constraint instead.