Fix SaferRemoveFieldForeignKey for non null columns

Prior to this change, if the FK field being removed had a null=False set
up, this operation would fail:

  ValueError: Can't safely create a FK field with null=False

While this error is relevant when the FK manager is *creating* a new
column, it is not relevant when deleting one.

This commit changes that by skipping the nullability check when running
from SaferRemoveFieldForeignKey.

A note was dropped on the documentation to alert that the field will be
created with null=True when reversing the operation, since the
alternative wouldn't be safe.

Author: marcelofern

CHANGELOG.md

@@ -8,7 +8,11 @@ Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to
 
 ## [Unreleased]
 
-_No notable unreleased changes_
+### Fixed
+
+- Fixed a bug where using `SaferRemoveFieldForeignKey` on a field that had
+  null=False was raising an error. This shouldn't be the case as the
+  nullability of the field is not important when removing the FK field.
 
 ## [0.1.21] - 2025-07-07
 

docs/guides/migration_guides.rst

@@ -830,6 +830,8 @@ fixes those problems by:
 - Having a custom backward operation that will add the foreign key back
   without blocking any reads/writes. This is achieved through the same
   strategy of :ref:`SaferAddFieldForeignKey <safer_add_field_foreign_key>`.
+  Note that if the original column was non-nullable, the reversal will create
+  the field as nullable to avoid outages.
 
 .. _guide_how_to_use_safer_remove_field_foreign_key:
 

src/django_pg_migration_tools/operations.py

@@ -1156,8 +1156,9 @@ def __init__(
         column_name: str,
         field: models.ForeignKey[models.Model],
         unique: bool,
+        skip_null_check: bool = False,
     ) -> None:
-        if not field.null:
+        if not field.null and not skip_null_check:
             # Validate at initialisation, rather than wasting time later.
             raise ValueError("Can't safely create a FK field with null=False")
 
@@ -1486,6 +1487,7 @@ def database_forwards(
             column_name=self.name,
             field=field,
             unique=False,
+            skip_null_check=True,
         ).drop_fk_field()
 
     def database_backwards(
@@ -1508,6 +1510,7 @@ def database_backwards(
             column_name=self.name,
             field=field,
             unique=False,
+            skip_null_check=True,
         ).add_fk_field()
 
     def describe(self) -> str:

tests/django_pg_migration_tools/test_operations.py

@@ -22,6 +22,7 @@
     IntModelWithExplicitPK,
     ModelWithCheckConstraint,
     ModelWithForeignKey,
+    ModelWithNotNullForeignKey,
     NotNullIntFieldModel,
     NullFKFieldModel,
     NullIntFieldModel,
@@ -2586,6 +2587,135 @@ def test_operation(self):
                 AND convalidated IS TRUE;
         """)
 
+    @pytest.mark.django_db(transaction=True)
+    def test_when_column_not_null(self):
+        with connection.cursor() as cursor:
+            # Set the lock_timeout to check it has been returned to
+            # its original value once the fk index creation is completed by
+            # the reverse operation.
+            cursor.execute(_SET_LOCK_TIMEOUT)
+
+        project_state = ProjectState()
+        project_state.add_model(ModelState.from_model(IntModel))
+        project_state.add_model(ModelState.from_model(ModelWithNotNullForeignKey))
+        new_state = project_state.clone()
+        operation = operations.SaferRemoveFieldForeignKey(
+            model_name="modelwithnotnullforeignkey",
+            name="fk",
+        )
+
+        assert operation.describe() == (
+            "Remove field fk from modelwithnotnullforeignkey. Note: Using "
+            "django_pg_migration_tools SaferRemoveFieldForeignKey operation."
+        )
+
+        operation.state_forwards(self.app_label, new_state)
+        with connection.schema_editor(atomic=False, collect_sql=False) as editor:
+            with utils.CaptureQueriesContext(connection) as queries:
+                operation.database_forwards(
+                    self.app_label, editor, from_state=project_state, to_state=new_state
+                )
+
+        assert len(queries) == 2
+
+        assert queries[0]["sql"] == dedent("""
+            SELECT 1
+            FROM pg_catalog.pg_attribute
+            WHERE
+                attrelid = 'example_app_modelwithnotnullforeignkey'::regclass
+                AND attname = 'fk_id';
+        """)
+        assert queries[1]["sql"] == dedent("""
+            ALTER TABLE "example_app_modelwithnotnullforeignkey"
+            DROP COLUMN "fk_id";
+        """)
+
+        with connection.schema_editor(atomic=False, collect_sql=False) as editor:
+            with utils.CaptureQueriesContext(connection) as reverse_queries:
+                operation.database_backwards(
+                    self.app_label, editor, from_state=new_state, to_state=project_state
+                )
+
+        assert len(reverse_queries) == 9
+
+        assert reverse_queries[0]["sql"] == dedent("""
+            SELECT 1
+            FROM pg_catalog.pg_attribute
+            WHERE
+                attrelid = 'example_app_modelwithnotnullforeignkey'::regclass
+                AND attname = 'fk_id';
+        """)
+        assert reverse_queries[1]["sql"] == dedent("""
+            ALTER TABLE "example_app_modelwithnotnullforeignkey"
+            ADD COLUMN IF NOT EXISTS "fk_id"
+            integer NULL;
+        """)
+        assert reverse_queries[2]["sql"] == "SHOW lock_timeout;"
+        assert reverse_queries[3]["sql"] == "SET lock_timeout = '0';"
+        assert reverse_queries[4]["sql"] == dedent("""
+            SELECT relname
+            FROM pg_class, pg_index
+            WHERE (
+                pg_index.indisvalid = false
+                AND pg_index.indexrelid = pg_class.oid
+                AND relname = 'modelwithnotnullforeignkey_fk_id_idx'
+            );
+            """)
+        assert (
+            reverse_queries[5]["sql"]
+            == 'CREATE INDEX CONCURRENTLY IF NOT EXISTS "modelwithnotnullforeignkey_fk_id_idx" ON "example_app_modelwithnotnullforeignkey" ("fk_id");'
+        )
+        assert reverse_queries[6]["sql"] == "SET lock_timeout = '1s';"
+        assert reverse_queries[7]["sql"] == dedent("""
+            ALTER TABLE "example_app_modelwithnotnullforeignkey"
+            ADD CONSTRAINT "example_app_modelwithnotnullforeignkey_fk_id_fk" FOREIGN KEY ("fk_id")
+            REFERENCES "example_app_intmodel" ("id")
+            DEFERRABLE INITIALLY DEFERRED
+            NOT VALID;
+        """)
+        assert reverse_queries[8]["sql"] == dedent("""
+            ALTER TABLE "example_app_modelwithnotnullforeignkey"
+            VALIDATE CONSTRAINT "example_app_modelwithnotnullforeignkey_fk_id_fk";
+        """)
+
+        # Reversing again does nothing apart from checking that the FK is
+        # already there and the index/constraint are all good to go.
+        # This proves the OP is idempotent.
+        with connection.schema_editor(atomic=False, collect_sql=False) as editor:
+            with utils.CaptureQueriesContext(connection) as second_reverse_queries:
+                operation.database_backwards(
+                    self.app_label, editor, from_state=new_state, to_state=project_state
+                )
+        assert len(second_reverse_queries) == 4
+        assert second_reverse_queries[0]["sql"] == dedent("""
+            SELECT 1
+            FROM pg_catalog.pg_attribute
+            WHERE
+                attrelid = 'example_app_modelwithnotnullforeignkey'::regclass
+                AND attname = 'fk_id';
+        """)
+        assert second_reverse_queries[1]["sql"] == dedent("""
+            SELECT 1
+            FROM pg_class, pg_index
+            WHERE (
+                pg_index.indisvalid = true
+                AND pg_index.indexrelid = pg_class.oid
+                AND relname = 'modelwithnotnullforeignkey_fk_id_idx'
+            );
+        """)
+        assert second_reverse_queries[2]["sql"] == dedent("""
+            SELECT conname
+            FROM pg_catalog.pg_constraint
+            WHERE conname = 'example_app_modelwithnotnullforeignkey_fk_id_fk';
+        """)
+        assert second_reverse_queries[3]["sql"] == dedent("""
+            SELECT 1
+            FROM pg_catalog.pg_constraint
+            WHERE
+                conname = 'example_app_modelwithnotnullforeignkey_fk_id_fk'
+                AND convalidated IS TRUE;
+        """)
+
     @pytest.mark.django_db(transaction=True)
     def test_when_column_already_deleted(self):
         with connection.cursor() as cursor:

tests/example_app/models.py

@@ -31,6 +31,10 @@ class ModelWithForeignKey(models.Model):
     fk = models.ForeignKey(IntModel, null=True, on_delete=models.CASCADE)
 
 
+class ModelWithNotNullForeignKey(models.Model):
+    fk = models.ForeignKey(IntModel, null=False, on_delete=models.CASCADE)
+
+
 class CharModel(models.Model):
     char_field = models.CharField(default="char")