[Django] #36168: migration plan in the context of squashmigrations has too many leaves

[Django]

#36168: migration plan in the context of squashmigrations has too many leaves
-------------------------------------+-------------------------------------
Reporter: Klaas van Schelven | Type:
| Uncategorized
Status: new | Component:
| Migrations
Version: 5.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
-------------------------------------+-------------------------------------
I managed to debug my way to the broken part of the code, but did not
manage to find a nice clean reproduction. Hence the following report
starts with broken code and only then explains the problem, which is the
unusual order.

[https://github.com/django/django/blob/b1324a680add78de24c763911d0eefa19b9263bc/django/db/migrations/executor.py#L49
here's the broken code]

This is wrong because it mutates the state on the `MigrationExecutor`,
i.e. in certain conditions a different graph (without the pruning of
`replace_migrations`) is set "globally" on the `MigrationExecutor`.

But `migration_plan` (the function of which the broken code is part) is
called _twice_ in at least some flows. In particular: first as part of
`migrate`, and then as part of `_create_project_state`. This means that,
for flows where the broken code is called in the first call to
`migration_plan` (for presumably good reasons, as per the comment above
it), the construction of the second plan uses a graph that is too large,
causing a failure in some cases.

This fails in the following combination of conditions:

1. a project with squashed migrations in more than one app
2. where the squashed migrations do not have follow-up migrations (i.e.
when the graph is not simplified for replacements, both the squashing
migration and the last migration it replaces show up as leaf nodes)
3. explicit migration to a replaced migration from the command line.

because of the explicit migration to a replaced migration (3) the graph is
updated in the "wrong code" as per the comment. Then, when trying to
`_create_project_state` the graph contains leaf nodes for both the
original and squashed paths, which means that some things in the project-
state-creation happen doubly, which fails.

I have encountered this while working on [https://www.bugsink.com/
Bugsink, a self-hosted Error Tracker], on
[https://github.com/bugsink/bugsink/tree/0b42d3ff1e0344a79bf3784c7cf2d68ea0d20a29
this commit]. I have not been able to distill a more clean PoC that
actually exhibits failure though.

Replacing the mutation with something that leaves the loader untouched
(ugly version below) fixes the problem though.

{{{
old_loader = self.loader
self.loader = MigrationLoader(self.connection)
self.loader.replace_migrations = False
self.loader.build_graph()
result = self.migration_plan(targets,
clean_start=clean_start)
self.loader = old_loader
return result
}}}

[https://code.djangoproject.com/ticket/36166#ticket this ticket may or may
not be related]
--
Ticket URL: <https://code.djangoproject.com/ticket/36168>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36168: migration plan in the context of squashmigrations has too many leaves

------------------------------------+--------------------------------------
Reporter: Klaas van Schelven | Owner: (none)
Type: Uncategorized | Status: closed
Component: Migrations | Version: 5.1
Severity: Normal | Resolution: needsinfo

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

------------------------------------+--------------------------------------
Changes (by Sarah Boyce):

* resolution: => needsinfo
* status: new => closed

Comment:

Hi Klaas, thank you for your efforts into debugging further into the
migrations
I will close this until there is either a test to demonstrate undesired
behavior or a project to replicate (showing an issue different to #36166)
I would avoid going too low-level when creating a ticket as we can go into
this depth in PRs/ticket comments when resolving issues
--
Ticket URL: <https://code.djangoproject.com/ticket/36168#comment:1>

[Django]

#36168: migration plan in the context of squashmigrations has too many leaves
------------------------------------+--------------------------------------
Reporter: Klaas van Schelven | Owner: (none)
Type: Uncategorized | Status: closed
Component: Migrations | Version: 5.1
Severity: Normal | Resolution: needsinfo
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 Klaas van Schelven):

ok, finally found a clean reproducer!

https://github.com/vanschelven/squashwithrename

{{{
(freshdjango) klaas@pop-os:~/dev/squashwithrename$ rm db.sqlite3
(freshdjango) klaas@pop-os:~/dev/squashwithrename$ python manage.py
migrate
Operations to perform:
Apply all migrations: admin, auth, contenttypes, sessions, squashme,
triggerfailingcode
Running migrations:
Applying contenttypes.0001_initial... OK
Applying auth.0001_initial... OK
Applying admin.0001_initial... OK
Applying admin.0002_logentry_remove_auto_add... OK
Applying admin.0003_logentry_add_action_flag_choices... OK
Applying contenttypes.0002_remove_content_type_name... OK
Applying auth.0002_alter_permission_name_max_length... OK
Applying auth.0003_alter_user_email_max_length... OK
Applying auth.0004_alter_user_username_opts... OK
Applying auth.0005_alter_user_last_login_null... OK
Applying auth.0006_require_contenttypes_0002... OK
Applying auth.0007_alter_validators_add_error_messages... OK
Applying auth.0008_alter_user_username_max_length... OK
Applying auth.0009_alter_user_last_name_max_length... OK
Applying auth.0010_alter_group_name_max_length... OK
Applying auth.0011_update_proxy_permissions... OK
Applying auth.0012_alter_user_first_name_max_length... OK
Applying sessions.0001_initial... OK
Applying squashme.0001_initial... OK
Applying
squashme.0002_rename_name_foo_rename_squashed_0003_foo_another_field... OK
Applying triggerfailingcode.0001_squashed_0002_baz_baz... OK
(freshdjango) klaas@pop-os:~/dev/squashwithrename$ python manage.py
migrate triggerfailingcode 0001_initial
Operations to perform:
Target specific migration: 0001_initial, from triggerfailingcode
Traceback (most recent call last):
File "/tmp/freshdjango/lib/python3.10/site-
packages/django/db/migrations/state.py", line 297, in rename_field
found = fields.pop(old_name)
KeyError: 'name'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
File "/mnt/datacrypt/dev/squashwithrename/manage.py", line 22, in
<module>
main()
File "/mnt/datacrypt/dev/squashwithrename/manage.py", line 18, in main
execute_from_command_line(sys.argv)
File "/tmp/freshdjango/lib/python3.10/site-
packages/django/core/management/__init__.py", line 442, in
execute_from_command_line
utility.execute()
File "/tmp/freshdjango/lib/python3.10/site-
packages/django/core/management/__init__.py", line 436, in execute
self.fetch_command(subcommand).run_from_argv(self.argv)
File "/tmp/freshdjango/lib/python3.10/site-
packages/django/core/management/base.py", line 413, in run_from_argv
self.execute(*args, **cmd_options)
File "/tmp/freshdjango/lib/python3.10/site-
packages/django/core/management/base.py", line 459, in execute
output = self.handle(*args, **options)
File "/tmp/freshdjango/lib/python3.10/site-
packages/django/core/management/base.py", line 107, in wrapper
res = handle_func(*args, **kwargs)
File "/tmp/freshdjango/lib/python3.10/site-
packages/django/core/management/commands/migrate.py", line 302, in handle
pre_migrate_state =
executor._create_project_state(with_applied_migrations=True)
File "/tmp/freshdjango/lib/python3.10/site-
packages/django/db/migrations/executor.py", line 91, in
_create_project_state
migration.mutate_state(state, preserve=False)
File "/tmp/freshdjango/lib/python3.10/site-
packages/django/db/migrations/migration.py", line 91, in mutate_state
operation.state_forwards(self.app_label, new_state)
File "/tmp/freshdjango/lib/python3.10/site-
packages/django/db/migrations/operations/fields.py", line 303, in
state_forwards
state.rename_field(
File "/tmp/freshdjango/lib/python3.10/site-
packages/django/db/migrations/state.py", line 299, in rename_field
raise FieldDoesNotExist(
django.core.exceptions.FieldDoesNotExist: squashme.foo has no field named
'name'
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36168#comment:2>

[Django]

#36168: migration plan in the context of squashmigrations has too many leaves
------------------------------------+--------------------------------------
Reporter: Klaas van Schelven | Owner: (none)
Type: Uncategorized | Status: closed
Component: Migrations | Version: 5.1
Severity: Normal | Resolution: needsinfo
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 Klaas van Schelven):

The reason this was somewhat hard to reproduce cleanly is the following:

Per my original report:

> the graph contains leaf nodes for both the original and squashed paths,
which means that some things in the project-state-creation happen doubly,
which fails.

''however'', there's also
[https://github.com/django/django/blob/1330cb570519170bb4397b4fb02c7e3e0657855a/django/db/migrations/state.py#L120
this code], which is the adding of a model to a project's state. That code
does not bother to check whether a given model already exist, which means
that the things happening doubly doesn't trigger an error if both paths
start with creating a model, for the simple reason that the model is
simply overwritten.

That might actually be a separate bug...
--
Ticket URL: <https://code.djangoproject.com/ticket/36168#comment:3>

[Django]

#36168: migration plan in the context of squashmigrations has too many leaves
------------------------------------+--------------------------------------
Reporter: Klaas van Schelven | Owner: (none)
Type: Uncategorized | Status: closed
Component: Migrations | Version: 5.1
Severity: Normal | Resolution: needsinfo
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 Sarah Boyce):

I've downloaded the project and ran the commands. It migrates without
error for me on both Django 5.1 and main 🤔
--
Ticket URL: <https://code.djangoproject.com/ticket/36168#comment:4>