#28862 closed Bug (fixed)
Removing a field from index_together/unique_together and from the model generates a migration that crashes
Reported by: | Artem Maslovskiy | Owned by: | Jeff |
---|---|---|---|
Component: | Migrations | Version: | 1.9 |
Severity: | Normal | Keywords: | models migrations |
Cc: | Artem Maslovskiy, Ed Morley | Triage Stage: | Accepted |
Has patch: | yes | 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 (29)
comment:1 by , 7 years ago
Summary: | Error in auto-generated migration → Removing a field from index_together and from the model generates a migration that crashes |
---|
comment:2 by , 7 years ago
Cc: | added |
---|
comment:3 by , 7 years ago
We have a test that says the generated migrations are in said order:
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 by , 7 years ago
Component: | Database layer (models, ORM) → Migrations |
---|---|
Summary: | Removing a field from index_together and from the model generates a migration that crashes → Removing a field from index_together/unique_together and from the model generates a migration that crashes |
Triage Stage: | Unreviewed → Accepted |
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.
comment:5 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Version: | 1.9 → 1.8 |
comment:6 by , 7 years ago
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.
comment:7 by , 7 years ago
Forgot to say this happens in django.db.migrations.autodetector.MigrationAutodetector._optimize_migrations()
follow-up: 9 comment:8 by , 7 years ago
Version: | 1.8 → 1.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 by , 7 years ago
Replying to Markus Holtermann:
the test itself is still correct because the
RemoveField
operation doesn't touch any of the fields referred to inAlterUniqueTogether
orAlterIndexTogether
.
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.
comment:10 by , 7 years ago
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
comment:13 by , 7 years ago
Cc: | added |
---|
comment:14 by , 6 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:15 by , 6 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:16 by , 6 years ago
Ramiro Morales pointed out I am working on another duplicate #29123. I'll take ownership of this as well.
comment:17 by , 6 years ago
Looking into overwriting the FieldRelatedOptionOperation
's reduce method to additionally:
1) check if the current operation is a RemoveField
(and maybe RenameField
, I need to confirm if it is also problematic)
2) if it is a RemoveField
operation, to look into the model (as it should exist prior to this migration) and if the field being removed is in unique_together
, index_together
, or order_with_respect_to
, to skip the operation.
This would correct the optimizer's erroneous migrations and I do not think would cause any other tests to fail.
I am still new to the codebase, could anyone point me in the right direction on how I can get the state of the model prior to the current migration? Using apps.config
's get_model()
is returning the state of the model from models.py
. I am trying to use ProjectState
at the moment. I plan to continue down this path, but please let me know if you can guide me to a more correct method of getting a model in the desired state.
comment:18 by , 6 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:19 by , 6 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:22 by , 6 years ago
I had a look at the issue and I came to the conclusion that the only solution for now is to disable AlterFooTogether
optimization when a RemoveField
on the same model is involved.
As mentioned by Ramiro we'd need to have access to the previous foo_together
value to determine whether or not the optimization can take place. This is a class of problem that also came up when working on #27768 where I had to disable an optimization from taking place because I didn't have the previous context of a RemoveField
.
I guess I'd be possible to have newly generated operation embed a such a previous state to allow the optimization to take place but I figured out what I believe is a clever way of working around this disabled optimization. By implementing CreateModel
/AlterFooOperation
reduction most of the negative side effect during migration squashing where the optimizer wouldn't have been able to perform a complete reduction are gone because the former is able to reduce RemoveField
operations to a new CreateModel
operation.
All of these ideas are implemented in this PR.
follow-up: 25 comment:24 by , 6 years ago
Hey Jeff, sorry from stealing this from you. I know you've invested a lot of effort in getting this fixed and I should have reached out to discuss why I think this a more appropriate solution.
comment:25 by , 6 years ago
I wish you had. I was still working on this last night and as a new contributor had invested a lot of time getting familiarized with all the migrations/fields/optimizer code to get out that initial PR. As someone new who wants to become a regular contributor I really would appreciate if you'd help me get this across the line instead of coming in and just doing it while I am still trying to figure out how to do it the way you suggested, instead of the way I did that seemed to work correctly.
Replying to Simon Charette:
Hey Jeff, sorry from stealing this from you. I know you've invested a lot of effort in getting this fixed and I should have reached out to discuss why I think this a more appropriate solution.
comment:26 by , 6 years ago
Take heart, Jeff, there are plenty of other tickets to use your new knowledge. Also, you can review Simon's patch. We have more people writing code than reviewing and we don't merge patches without review, so that's an important task. Thanks for your interest and effort.
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).