Altering the null attribute of a GeneratedField should raise when making migrations

[Jacob Walls]

The migration layer attempts to detect if a GeneratedField has been altered in a SQL-affecting way so that it can throw an exception:

        if modifying_generated_field:
            raise ValueError(
                f"Modifying GeneratedFields is not supported - the field {new_field} "
                "must be removed and re-added with the new definition."
            )

However, altering the null kwarg, e.g. from False to True is not detected by the above check, but does have an incidence on the DDL that gets emitted, so a migration gets emitted that can do wacky things in the database, see ticket:37348#comment:6.

I propose to fix the modifying_generated_field logic so that it fathoms changes in the null attribute and raises.

This will be a breaking change but most likely a welcome one (removing dangerous operations from your migrations).

Rough test:

tests/migrations/test_operations.py

@@ -6646 +6646 @@
             tests.append(
                 ("test_igfc_4", generated_1, generated_3),
             )
+        generated_4 = models.GeneratedField(
+            expression=F("pink") + F("pink") + F("pink"),
+            output_field=models.IntegerField(),
+            db_persist=db_persist,
+            null=True,
+        )
+        tests.append(("test_igfc_5", generated_2, generated_4))
         for app_label, add_field, alter_field in tests:
             project_state = self.set_up_test_model(app_label)
             operations = [

AssertionError: ValueError not raised

[Md. Saikat Islam]

I’ve reproduced this issue and agree with the approach proposed in the ticket. I’ve also prepared a patch with a regression test for the null attribute change.

I’d be happy to work on this ticket and raise a PR once the ticket is accepted.

[Natalia Bidart]

Thanks Jacob. I agree the current behavior is a bug, so I'll Accept on that basis. But I have concerns about raising if modifying_generated_field holds:

1. The proposed check is in the schema editor, therefore makemigrations still generates the AlterField which is conceptually incorrect.
2. (Arguably worse) The error surfaces on migrate, which is "too late": people who already have such an AlterField in their migration history would need to edit it, and a deploy is a bad moment to find that out.
3. Now that we merged the revert of W225 in #37348, users who removed null=True to silence W225 need to add it back. Making a generated column nullable again is just dropping the NOT NULL constraint (ish), but under this proposal I think Django would refuse that and tell users to drop the column and add it again, which for a stored column means recomputing every row, and loses any untracked index or constraint.
4. This proposal is very backward incompatible, since existing AlterField migrations changing null on generated fields have been applied successfully (when nullable or without NULL rows). Raising now would break migrating those projects (fully functional now) from scratch, including test databases.

So I think the root issue is that column creation and alteration disagree: _iter_column_sql() never emits NOT NULL for a generated column given this guards:

        if field.generated:
            generated_sql, generated_params = self._column_generated_sql(field)
            params.extend(generated_params)
            yield generated_sql
        elif not null:
            yield "NOT NULL"

while _alter_field() does. That's also why the suggested workaround doesn't help, since removing and re-adding the field produces a nullable column no matter what null says. So the question to settle first is whether null should affect the DDL of a GeneratedField:

• If it shouldn't, the alteration should be skipped for generated fields which matches creation.
• If it should, creation needs to emit NOT NULL as well.

[Jacob Walls]

I see! I wrongly assumed the existing check was invoked somehow during makemigrations, which would have solved 1, 2, and 4. Oops.

So the question to settle first is whether null should affect the DDL of a GeneratedField

I guess we need to assess the cross-db feasibility of that and the backward compatibility implications before pursuing that?

[Md. Saikat Islam]

I tested this locally by creating migrations and reproducing the issue.

class Pony(models.Model):
    pink = models.IntegerField(default=3)
    modified_pink = models.GeneratedField(
        expression=F("pink") + F("pink"),
        output_field=models.IntegerField(),
        db_persist=True,
    )

This results in the following SQLite schema:

CREATE TABLE IF NOT EXISTS "__main___pony" (
    "id" integer NOT NULL PRIMARY KEY AUTOINCREMENT,
    "pink" integer NOT NULL,
    "modified_pink" integer GENERATED ALWAYS AS (("pink" + "pink")) STORED
);

There is no NOT NULL constraint on modified_pink, regardless of the default null=False.

So, perhaps we should not consider null=True/null=False changes when generating migrations for GeneratedField. Since null=False currently has no effect on the generated column's DDL, ignoring changes to null could avoid generating an unnecessary AlterField.

I think this could also preserve backward compatibility with existing migrations that alter null on GeneratedField.

Does this approach make sense?

[Md. Saikat Islam]

Looking forward to hearing your thoughts on this PR: ​https://github.com/django/django/pull/21988