Opened 11 days ago

Last modified 5 days ago

#28862 assigned Bug

Removing a field from index_together/unique_together and from the model generates a migration that crashes

Reported by: Artem Maslovskiy Owned by: Ramiro Morales
Component: Migrations Version: 1.9
Severity: Normal Keywords: models migrations
Cc: Artem Maslovskiy Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In Django 1.9.11, after deleting a model field and removing it from index_together attribute, the makemigrations command generates a broken migration code with RemoveField operation preceding AlterIndexTogether operation. That causes the following migrate command to raise an exception while trying to apply the generated migration.

Change History (10)

comment:1 Changed 11 days ago by Tim Graham

Summary: Error in auto-generated migrationRemoving a field from index_together and from the model generates a migration that crashes

Can you reproduce that issue with Django 2.0 or Django 1.11? It sounds familiar and may have been fixed in a newer version of Django (in general, we would always like bug reports to be confirmed against the latest version of Django or even Django master, if possible).

comment:2 Changed 11 days ago by Artem Maslovskiy

Cc: Artem Maslovskiy added

comment:3 Changed 11 days ago by Tomer Chachamu

We have a test that says the generated migrations are in said order:

https://github.com/django/django/blob/6c0042430e3618ce5c276d195d92a6b884daa3a3/tests/migrations/test_autodetector.py#L1323

The docstring is incorrect, it says "Removed fields will be removed after updating index/unique_together." but the test actually checks for "Removed fields will be removed before updating index/unique_together."

comment:4 Changed 6 days ago by Tim Graham

Component: Database layer (models, ORM)Migrations
Summary: Removing a field from index_together and from the model generates a migration that crashesRemoving a field from index_together/unique_together and from the model generates a migration that crashes
Triage Stage: UnreviewedAccepted

That test is from the ticket I was thinking of: #23614 (fixed in Django 1.7.2 and later). The comment isn't accurate because the order of operations changed in 5c9c1e029d139bd3d5213804af2ed9f317cd0b86 (Django 1.8). That change in ordering looks incorrect.

Last edited 5 days ago by Tim Graham (previous) (diff)

comment:5 Changed 5 days ago by Ramiro Morales

Owner: changed from nobody to Ramiro Morales
Status: newassigned
Version: 1.91.8

comment:6 Changed 5 days ago by Ramiro Morales

This is what I've found so far:

It seems the optimization introduced in e470f311d654267ec86f9a6325ec500345b9dff2 (later refactored in 49f4c9f4c61885189d136d7073c26ecc91b482b1) by which a sequence of schema migration operations like this:

- AlterIndexTogether(index_together=['title', 'author', 'newfield'] -> index_together=['title', 'author'])
- RemoveField('newfield')

which is correctly created by django.db.migrations.autodetector.MigrationAutodetector._build_migration_list()
gets swapped to

- RemoveField('newfield')
- AlterIndexTogether(index_together=['title', 'author', 'newfield'] -> index_together=['title', 'author'])

This is because the references_field() method of django.db.migrations.operations.AlterIndexTogether considers only the final set of index_together fields to conclude there is no overlap in fields affected by the two operations. This reasoning might be valid when reordering operation sequences which involve e.g. AddField But when it's interacting with RemoveField (and RenameField?) it needs to consider the _initial_ set of index_together fields instead.

if it did, then it would discover it can't optimize the AlterIndexTogether to be after the RemoveField which is the origin of the crash the OP reports.

Example is for Meta.index_together but affects also at least Meta.unique_together too. AFAICT fixing this might involve some non-trivial refactoring.

I'm open to confirmation/rebuttal and to ideas on how this could be solved.

Last edited 5 days ago by Ramiro Morales (previous) (diff)

comment:7 Changed 5 days ago by Ramiro Morales

Forgot to say this happens in django.db.migrations.autodetector.MigrationAutodetector._optimize_migrations()

comment:8 Changed 5 days ago by Markus Holtermann

Version: 1.81.9

A good find, Artem. Thank you!

The issue seems to come from e470f311d654267ec86f9a6325ec500345b9dff2 which is part of 1.9 release cycle, but not 1.8.

While the docstring on https://github.com/django/django/commit/e470f311d654267ec86f9a6325ec500345b9dff2#diff-c11e6432df7086eda3dfb9ab8e5b2839R1141 is clearly wrong, the test itself is still correct because the RemoveField operation doesn't touch any of the fields referred to in AlterUniqueTogether or AlterIndexTogether.

There is a test for the migration optimizer that shows a RemoveField operation after the *Together operation is not moved to the front if both involve the same field.

Adding this code though will make the test fail:

        self.assertOptimizesTo(
            [
                migrations.CreateModel("Foo", [
                    ("a", models.IntegerField()),
                    ("b", models.IntegerField()),
                ]),
                migrations.RemoveField("Foo", "b"),
                alter,
            ],
            [
                migrations.CreateModel("Foo", [
                    ("a", models.IntegerField()),
                    ("b", models.IntegerField()),
                ]),
                alter,
                migrations.RemoveField("Foo", "b"),
            ],
        )

which I believe is what you're experiencing. Maybe even this would be an expected behavior:

        self.assertOptimizesTo(
            [
                migrations.CreateModel("Foo", [
                    ("a", models.IntegerField()),
                    ("b", models.IntegerField()),
                ]),
                migrations.RemoveField("Foo", "b"),
                alter,
            ],
            [
                migrations.CreateModel("Foo", [
                    ("a", models.IntegerField()),
                ]),
            ],
        )

comment:9 in reply to:  8 Changed 5 days ago by Ramiro Morales

Replying to Markus Holtermann:

the test itself is still correct because the RemoveField operation doesn't touch any of the fields referred to in AlterUniqueTogether or AlterIndexTogether.

The point I tried to make in comment:6 is that this reasoning (and hence the asserts of the test case) is wrong because if the user removes a field from the model and from Meta.*_together (the test case scenario) then it's wrong to optimize the sequence to have te RemoveField first even it it doesn't mention any of the fields which will remain in *_together.

The asserts in the test case examine the order and type of parts of the generated migration and don't fail because no DDL code is executed against the DB at that point. The breakage happens at migration application time when it wants to execute the Alter*Together operation.

Last edited 5 days ago by Ramiro Morales (previous) (diff)

comment:10 Changed 5 days ago by Markus Holtermann

Now I understand, Ramiro. I concur with your evaluation that the initial assumption is wrong. I am as well surprised that this hasn't come up earlier / more frequently.

Another idea that came to mind was adding temporary dependencies in https://github.com/MarkusH/django/blob/8e352876c337332b45a72da8bbccad2830c7b1e0/django/db/migrations/autodetector.py#L1006-L1021

Note: See TracTickets for help on using tickets.
Back to Top