From dcdd219ee1e062dc6189f382e0298e0adf5d5ddf Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Fri, 24 Nov 2017 03:05:56 -0500 Subject: [PATCH] Fixed #25817 -- Made RenameField repoint to_field/to_fields references. Also updated the autodetector to assume the RenameField operation will perform the required repointing. --- django/db/migrations/autodetector.py | 18 +++++ django/db/migrations/operations/fields.py | 32 +++++++- tests/migrations/test_autodetector.py | 96 +++++++++++++++++++++++ tests/migrations/test_operations.py | 28 +++++++ 4 files changed, 171 insertions(+), 3 deletions(-) diff --git a/django/db/migrations/autodetector.py b/django/db/migrations/autodetector.py index 1dce17be6e..0ccb726b3d 100644 --- a/django/db/migrations/autodetector.py +++ b/django/db/migrations/autodetector.py @@ -895,6 +895,24 @@ class MigrationAutodetector: ) if rename_key in self.renamed_models: new_field.remote_field.model = old_field.remote_field.model + # Handle ForeignKey which can only have a single to_field. + remote_field_name = getattr(new_field.remote_field, 'field_name', None) + if remote_field_name: + to_field_rename_key = rename_key + (remote_field_name,) + if to_field_rename_key in self.renamed_fields: + new_field.remote_field.field_name = old_field.remote_field.field_name + # Handle ForeignObjects which can have multiple from_fields/to_fields. + from_fields = getattr(new_field, 'from_fields', None) + if from_fields: + from_rename_key = (app_label, model_name) + new_field.from_fields = tuple([ + self.renamed_fields.get(from_rename_key + (from_field,), from_field) + for from_field in from_fields + ]) + new_field.to_fields = tuple([ + self.renamed_fields.get(rename_key + (to_field,), to_field) + for to_field in new_field.to_fields + ]) if hasattr(new_field, "remote_field") and getattr(new_field.remote_field, "through", None): rename_key = ( new_field.remote_field.through._meta.app_label, diff --git a/django/db/migrations/operations/fields.py b/django/db/migrations/operations/fields.py index d103e5c127..96b0045744 100644 --- a/django/db/migrations/operations/fields.py +++ b/django/db/migrations/operations/fields.py @@ -270,8 +270,9 @@ class RenameField(FieldOperation): model_state = state.models[app_label, self.model_name_lower] # Rename the field fields = model_state.fields + found = False for index, (name, field) in enumerate(fields): - if name == self.old_name: + if not found and name == self.old_name: fields[index] = (self.new_name, field) # Delay rendering of relationships if it's not a relational # field and not referenced by a foreign key. @@ -279,8 +280,15 @@ class RenameField(FieldOperation): not field.is_relation and not is_referenced_by_foreign_key(state, self.model_name_lower, field, self.name) ) - break - else: + found = True + # Fix from_fields to refer to the new field. + from_fields = getattr(field, 'from_fields', None) + if from_fields: + field.from_fields = tuple([ + self.new_name if from_field_name == self.old_name else from_field_name + for from_field_name in from_fields + ]) + if not found: raise FieldDoesNotExist( "%s.%s has no field named '%s'" % (app_label, self.model_name, self.old_name) ) @@ -292,6 +300,24 @@ class RenameField(FieldOperation): [self.new_name if n == self.old_name else n for n in together] for together in options[option] ] + # Fix to_fields to refer to the new field. + model_tuple = app_label, self.model_name_lower + for (model_app_label, model_name), model_state in state.models.items(): + for index, (name, field) in enumerate(model_state.fields): + remote_field = field.remote_field + if remote_field: + remote_model_tuple = self._get_model_tuple( + remote_field.model, model_app_label, model_name + ) + if remote_model_tuple == model_tuple: + if getattr(remote_field, 'field_name', None) == self.old_name: + remote_field.field_name = self.new_name + to_fields = getattr(field, 'to_fields', None) + if to_fields: + field.to_fields = tuple([ + self.new_name if to_field_name == self.old_name else to_field_name + for to_field_name in to_fields + ]) state.reload_model(app_label, self.model_name_lower, delay=delay) def database_forwards(self, app_label, schema_editor, from_state, to_state): diff --git a/tests/migrations/test_autodetector.py b/tests/migrations/test_autodetector.py index bc828203f8..43c3a1941a 100644 --- a/tests/migrations/test_autodetector.py +++ b/tests/migrations/test_autodetector.py @@ -821,6 +821,102 @@ class AutodetectorTests(TestCase): self.assertOperationTypes(changes, 'testapp', 0, ["RenameField"]) self.assertOperationAttributes(changes, 'testapp', 0, 0, old_name="name", new_name="names") + def test_rename_field_foreign_key_to_field(self): + before = [ + ModelState('app', 'Foo', [ + ('id', models.AutoField(primary_key=True)), + ('field', models.IntegerField(unique=True)), + ]), + ModelState('app', 'Bar', [ + ('id', models.AutoField(primary_key=True)), + ('foo', models.ForeignKey('app.Foo', models.CASCADE, to_field='field')), + ]), + ] + after = [ + ModelState('app', 'Foo', [ + ('id', models.AutoField(primary_key=True)), + ('renamed_field', models.IntegerField(unique=True)), + ]), + ModelState('app', 'Bar', [ + ('id', models.AutoField(primary_key=True)), + ('foo', models.ForeignKey('app.Foo', models.CASCADE, to_field='renamed_field')), + ]), + ] + changes = self.get_changes(before, after, MigrationQuestioner({'ask_rename': True})) + # Right number/type of migrations? + self.assertNumberMigrations(changes, 'app', 1) + self.assertOperationTypes(changes, 'app', 0, ['RenameField']) + self.assertOperationAttributes(changes, 'app', 0, 0, old_name='field', new_name='renamed_field') + + def test_rename_foreign_object_fields(self): + fields = ('first', 'second') + renamed_fields = ('first_renamed', 'second_renamed') + before = [ + ModelState('app', 'Foo', [ + ('id', models.AutoField(primary_key=True)), + ('first', models.IntegerField()), + ('second', models.IntegerField()), + ], options={'unique_together': {fields}}), + ModelState('app', 'Bar', [ + ('id', models.AutoField(primary_key=True)), + ('first', models.IntegerField()), + ('second', models.IntegerField()), + ('foo', models.ForeignObject( + 'app.Foo', models.CASCADE, from_fields=fields, to_fields=fields, + )), + ]), + ] + # Case 1: to_fields renames. + after = [ + ModelState('app', 'Foo', [ + ('id', models.AutoField(primary_key=True)), + ('first_renamed', models.IntegerField()), + ('second_renamed', models.IntegerField()), + ], options={'unique_together': {renamed_fields}}), + ModelState('app', 'Bar', [ + ('id', models.AutoField(primary_key=True)), + ('first', models.IntegerField()), + ('second', models.IntegerField()), + ('foo', models.ForeignObject( + 'app.Foo', models.CASCADE, from_fields=fields, to_fields=renamed_fields, + )), + ]), + ] + changes = self.get_changes(before, after, MigrationQuestioner({'ask_rename': True})) + self.assertNumberMigrations(changes, 'app', 1) + self.assertOperationTypes(changes, 'app', 0, ['RenameField', 'RenameField', 'AlterUniqueTogether']) + self.assertOperationAttributes( + changes, 'app', 0, 0, model_name='foo', old_name='first', new_name='first_renamed', + ) + self.assertOperationAttributes( + changes, 'app', 0, 1, model_name='foo', old_name='second', new_name='second_renamed', + ) + # Case 2: from_fields renames. + after = [ + ModelState('app', 'Foo', [ + ('id', models.AutoField(primary_key=True)), + ('first', models.IntegerField()), + ('second', models.IntegerField()), + ], options={'unique_together': {fields}}), + ModelState('app', 'Bar', [ + ('id', models.AutoField(primary_key=True)), + ('first_renamed', models.IntegerField()), + ('second_renamed', models.IntegerField()), + ('foo', models.ForeignObject( + 'app.Foo', models.CASCADE, from_fields=renamed_fields, to_fields=fields, + )), + ]), + ] + changes = self.get_changes(before, after, MigrationQuestioner({'ask_rename': True})) + self.assertNumberMigrations(changes, 'app', 1) + self.assertOperationTypes(changes, 'app', 0, ['RenameField', 'RenameField']) + self.assertOperationAttributes( + changes, 'app', 0, 0, model_name='bar', old_name='first', new_name='first_renamed', + ) + self.assertOperationAttributes( + changes, 'app', 0, 1, model_name='bar', old_name='second', new_name='second_renamed', + ) + def test_rename_model(self): """Tests autodetection of renamed models.""" changes = self.get_changes( diff --git a/tests/migrations/test_operations.py b/tests/migrations/test_operations.py index fb429e19cf..b5c9fb1b3d 100644 --- a/tests/migrations/test_operations.py +++ b/tests/migrations/test_operations.py @@ -1455,6 +1455,34 @@ class OperationTests(OperationTestBase): with self.assertRaisesMessage(FieldDoesNotExist, "app.model has no field named 'field'"): migrations.RenameField('model', 'field', 'new_field').state_forwards('app', state) + def test_rename_referenced_field_state_forward(self): + state = ProjectState() + state.add_model(ModelState('app', 'Model', [ + ('id', models.AutoField(primary_key=True)), + ('field', models.IntegerField(unique=True)), + ])) + state.add_model(ModelState('app', 'OtherModel', [ + ('id', models.AutoField(primary_key=True)), + ('fk', models.ForeignKey('Model', models.CASCADE, to_field='field')), + ('fo', models.ForeignObject('Model', models.CASCADE, from_fields=('fk',), to_fields=('field',))), + ])) + operation = migrations.RenameField('Model', 'field', 'renamed') + new_state = state.clone() + operation.state_forwards('app', new_state) + self.assertEqual(new_state.models['app', 'othermodel'].fields[1][1].remote_field.field_name, 'renamed') + self.assertEqual(new_state.models['app', 'othermodel'].fields[1][1].from_fields, ['self']) + self.assertEqual(new_state.models['app', 'othermodel'].fields[1][1].to_fields, ('renamed',)) + self.assertEqual(new_state.models['app', 'othermodel'].fields[2][1].from_fields, ('fk',)) + self.assertEqual(new_state.models['app', 'othermodel'].fields[2][1].to_fields, ('renamed',)) + operation = migrations.RenameField('OtherModel', 'fk', 'renamed_fk') + new_state = state.clone() + operation.state_forwards('app', new_state) + self.assertEqual(new_state.models['app', 'othermodel'].fields[1][1].remote_field.field_name, 'renamed') + self.assertEqual(new_state.models['app', 'othermodel'].fields[1][1].from_fields, ('self',)) + self.assertEqual(new_state.models['app', 'othermodel'].fields[1][1].to_fields, ('renamed',)) + self.assertEqual(new_state.models['app', 'othermodel'].fields[2][1].from_fields, ('renamed_fk',)) + self.assertEqual(new_state.models['app', 'othermodel'].fields[2][1].to_fields, ('renamed',)) + def test_alter_unique_together(self): """ Tests the AlterUniqueTogether operation.