Opened 4 years ago
Last modified 4 months ago
#32263 assigned Bug
squashmigrations produces incorrect result with a RenameModel on a ForeignKey target.
Reported by: | InvalidInterrupt | Owned by: | Bhuvnesh |
---|---|---|---|
Component: | Migrations | Version: | 3.1 |
Severity: | Normal | Keywords: | |
Cc: | InvalidInterrupt, Bhuvnesh, David Wobrock | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
Given a list of operations:
operations = [ migrations.CreateModel( name="Author", fields=[ ("id", models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), ] ), migrations.CreateModel( name="Book", fields=[ ("id", models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), ("author", models.ForeignKey(on_delete=models.deletion.PROTECT, to="testapp.Author")), ] ), migrations.RenameModel("Author", "Person"), migrations.AddField(model_name="person", name="birthdate", field=models.DateField()), ]
squashmigrations produces the result:
operations = [ migrations.CreateModel( name='Book', fields=[ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), ('author', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to='testapp.author')), ], ), migrations.CreateModel( name='Person', fields=[ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), ('birthdate', models.DateField()), ], ), ]
There are two apparent problems with the result:
- The model with the
ForeignKey
field is created before the target of theForeignKey
- The
to
parameter of theForeignKey
is not updated with the new model name (the finalAddField
operation is not necessary to cause this problem)
I believe this is caused by CreateModel.reduce()
allowing optimization though operations that reference models which the model being created has a relationship to. Similar effects are likely to be caused by other migration patterns as well (I think Tim Graham may have found one here but as far as I can tell that's not the problem originally reported in that ticket)
Attachments (2)
Change History (21)
comment:1 by , 4 years ago
Description: | modified (diff) |
---|
by , 4 years ago
Attachment: | ticket_32263_1.tar.gz added |
---|
comment:2 by , 4 years ago
Triage Stage: | Unreviewed → Accepted |
---|
Thanks for the report. Agreed, we have two separate issues that can be fixed separately. I attached sample projects:
There are two apparent problems with the result:
- The model with the
ForeignKey
field is created before the target of theForeignKey
- The
to
parameter of theForeignKey
is not updated with the new model name (the finalAddField
operation is not necessary to cause this problem)
comment:3 by , 4 years ago
Summary: | squashmigrations produces incorrect result when a RenameModel operation is performed on a ForeignKey target → squashmigrations produces incorrect result with a RenameModel on a ForeignKey target. |
---|
comment:4 by , 4 years ago
You'll have to change the order of your save and assignment operations, or do the extra assignment. it might help
comment:5 by , 4 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 4 years ago
Owner: | removed |
---|
I have played with this for a while but I am not sure if this can be solved without a larger scale code change.
follow-up: 9 comment:7 by , 4 years ago
Feels like it's the same class of issue as #32256 but for RenameModel
instead of RenameField
(7c18b22e2fa70aa8dcfadb33beb17933abdf7ee4)
comment:8 by , 2 years ago
Owner: | set to |
---|
comment:9 by , 2 years ago
Replying to Simon Charette:
Feels like it's the same class of issue as #32256 but for
RenameModel
instead ofRenameField
(7c18b22e2fa70aa8dcfadb33beb17933abdf7ee4)
Tried doing the same with RenameModel.reduce()
dosen't seem to be working might have to look out for something else.
comment:10 by , 2 years ago
Cc: | added |
---|
comment:11 by , 2 years ago
Tried running around the files, the first problem that was the creation of model with ForeignKey
before the target of ForeignKey
seems to be a case for Sample Project 2 provided by Mariusz Felisiak. For the second problem it seems that the RenameModel
misses the functionality where it needs to change the to
parameter of the ForeignKey
model. The RenameModel.reduce()
doesn't seems to be getting called here.
comment:12 by , 2 years ago
After the squashed migration is created if we run makemigrations
once again it creates a new migrations file as such:
class Migration(migrations.Migration): dependencies = [ ("test_one", "0001_squashed_0003_person_birthdate"), ] operations = [ migrations.AlterField( model_name="book", name="author", field=models.ForeignKey( on_delete=django.db.models.deletion.PROTECT, to="test_one.person" ), ), ]
so the to
field automatically gets referenced to the right table in the new migrations file but still it won't work cause of the previous sqaushed migration.
comment:13 by , 2 years ago
A regression test for this ticket would look something like this:
def test_squashmigrations_rename_foreign_key(self): out = io.StringIO() with self.temporary_migration_module( module="migrations.test_migrations_rename" ) as migration_dir: call_command( "squashmigrations", "migrations", "0003", interactive=False, stdout=out, no_color=True, ) squashed_migration_file = os.path.join( migration_dir, "0001_squashed_0003_third.py" ) with open(squashed_migration_file, encoding="utf-8") as fp: content = fp.read() self.assertIn('to="migrations.person"',content) self.assertTrue(os.path.exists(squashed_migration_file)) self.assertEqual( out.getvalue(), f"Will squash the following migrations:\n" f" - 0001_initial\n" f" - 0002_second\n" f" - 0003_third\n" f"Optimizing...\n" f" Optimized from 4 operations to 2 operations.\n" f"Created new squashed migration {squashed_migration_file}\n" f" You should commit this migration but leave the old ones in place;\n" f" the new migration will be used for new installs. Once you are sure\n" f" all instances of the codebase have applied the migrations you " f"squashed,\n" f" you can delete them.\n", )
By running this test I found out that the base migration objects created are of CreateModel
so we need to somehow change the functionality of this section of CreateModel.reduce()
:
def reduce(self,operation,app_label): ... elif ( isinstance(operation, RenameModel) and self.name_lower == operation.old_name_lower ): return [ CreateModel( operation.new_name, fields=self.fields, options=self.options, bases=self.bases, managers=self.managers, ), ]
to something like:
def reduce(self,operation,app_label): ... elif ( isinstance(operation, RenameModel) and self.name_lower == operation.old_name_lower ): return operation.reduce(operation,app_label)
and changing the functionality of RenameModel.reduce()
. I hope I'm clear.
comment:14 by , 20 months ago
I tried to deal with this problem as follows. In the CreateModel.reduce function, when executed with a RenameModel operation, I check if the CreateModel operation has any fields that may have been impacted by the RenameModel operation, because in this scenario, these fields need to be updated for this renamed model.
def reduce(self, operation, app_label): ... elif impacted_fields := [ (_, field) for _, field in self.fields if field.remote_field and field.remote_field.model == f'{app_label}.{operation.old_name_lower}' ]: not_impacted_fields = [ (_, field) for (_, field) in self.fields if (_, field) not in impacted_fields ] fixed_fields = [] for (_, impacted_field) in impacted_fields: name, path, args, kwargs = impacted_field.deconstruct() kwargs['to'] = f'{app_label}.{operation.new_name_lower}' impacted_field = impacted_field.__class__(*args, **kwargs) fixed_fields.append((_, impacted_field)) return [ CreateModel( self.name, fields=not_impacted_fields + fixed_fields, bases=self.bases, managers=self.managers, ), ] ...
However, this approach did not work for the following reason. When the CreateModel operation is executed with its respective RenameModel, both are reduced to the new CreateModel operation. Then, some checks are made to decide whether this new resulting operation will be added to the list of new operations. But with this new reduction that I created, the other CreateModel operations, the ones that reference the renamed model, are also reduced, and this breaks the verification described below.
... for i, operation in enumerate(operations): right = True # Should we reduce on the right or on the left. # Compare it to each operation after it for j, other in enumerate(operations[i + 1 :]): result = operation.reduce(other, app_label) if isinstance(result, list): in_between = operations[i + 1 : i + j + 1] if right: new_operations.extend(in_between) new_operations.extend(result) # ↓↓↓ This condition fails (I didn't understand what is being checked here) elif all(op.reduce(other, app_label) is True for op in in_between): # Perform a left reduction if all of the in-between # operations can optimize through other. new_operations.extend(result) new_operations.extend(in_between) else: # <<<<<<<< New operation `result` is not added to new operations list # Otherwise keep trying. new_operations.append(operation) break ...
Now I am stuck, trying to think of a way to work around this problem.
Do any of you have any suggestions?
comment:15 by , 14 months ago
Has patch: | set |
---|---|
Owner: | changed from | to
PR
Thanks to Durval Carvalho for a basic idea.
comment:16 by , 13 months ago
Cc: | added |
---|
follow-up: 18 comment:17 by , 9 months ago
Is there anything I can do to help move this forward? The PR seems to handle this case, and create the correct optimization, and the author responded to the last comment on the PR. Is it being held up on the question about multiple apps?
comment:18 by , 9 months ago
Replying to bcail:
Is there anything I can do to help move this forward? The PR seems to handle this case, and create the correct optimization, and the author responded to the last comment on the PR. Is it being held up on the question about multiple apps?
It's waiting for a review, you can review it if you want to help.
comment:19 by , 4 months ago
Patch needs improvement: | set |
---|
Sample project 1.