[Django] #37351: Altering the null attribute of a GeneratedField should raise when making migrations

[Django]

#37351: Altering the null attribute of a GeneratedField should raise when making
migrations
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Type: Bug
Status: new | Component: Database
| layer (models, ORM)
Version: 6.1 | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
The migration layer attempts to detect if a `GeneratedField` has been
altered in a SQL-affecting way so that it can throw an exception:

{{{#!py
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:
{{{#!diff
diff --git a/tests/migrations/test_operations.py
b/tests/migrations/test_operations.py
index 9bbf1249e9..907c255e22 100644
--- a/tests/migrations/test_operations.py
+++ b/tests/migrations/test_operations.py
@@ -6646,6 +6646,13 @@ class OperationTests(OperationTestBase):
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 = [
}}}
{{{#!py
AssertionError: ValueError not raised
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37351>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#37351: Altering the null attribute of a GeneratedField should raise when making
migrations
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 6.1
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/37351#comment:1>

[Django]

#37351: Altering the null attribute of a GeneratedField should raise when making
migrations
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 6.1
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Natalia Bidart):

* stage: Unreviewed => Accepted

Comment:

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:

{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/37351#comment:2>

[Django]

#37351: Altering the null attribute of a GeneratedField should raise when making
migrations
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 6.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/37351#comment:3>

[Django]

#37351: Altering the null attribute of a GeneratedField should raise when making
migrations
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 6.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/37351#comment:4>

[Django]

#37351: Altering the null attribute of a GeneratedField should raise when making
migrations
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: Md.
| Saikat Islam
Type: Bug | Status: assigned

Component: Database layer | Version: 6.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Md. Saikat Islam):

* has_patch: 0 => 1
* needs_tests: 0 => 1
* owner: (none) => Md. Saikat Islam
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/37351#comment:5>

[Django]

#37351: Altering the null attribute of a GeneratedField should raise when making
migrations
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Md.
| Saikat Islam
Type: Bug | Status: assigned
Component: Database layer | Version: 6.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Md. Saikat Islam):

Looking forward to hearing your thoughts on this PR:
https://github.com/django/django/pull/21988
--
Ticket URL: <https://code.djangoproject.com/ticket/37351#comment:6>