From ee17bb8a67a9e7e688da6e6f4b3be1b3a69c09b0 Mon Sep 17 00:00:00 2001 From: Jeremy Bowman Date: Mon, 9 Apr 2018 13:35:06 -0400 Subject: [PATCH] Fixed #29193 -- Prevented unnecessary foreign key drops when altering a unique field. Stopped dropping and recreating foreign key constraints on other fields in the same table as the one which is actually being altered in an AlterField operation. Regression in c3e0adcad8d8ba94b33cabd137056166ed36dae0. --- AUTHORS | 1 + django/db/backends/base/schema.py | 22 +++++++++++++++++--- docs/releases/1.11.13.txt | 4 +++- docs/releases/2.0.5.txt | 4 ++++ tests/schema/tests.py | 34 +++++++++++++++++++++++++++++++ 5 files changed, 61 insertions(+), 4 deletions(-) diff --git a/AUTHORS b/AUTHORS index bfba0c0523..909149137d 100644 --- a/AUTHORS +++ b/AUTHORS @@ -382,6 +382,7 @@ answer newbie questions, and generally made Django that much better: Jensen Cochran Jeong-Min Lee Jérémie Blaser + Jeremy Bowman Jeremy Carbaugh Jeremy Dunck Jeremy Lainé diff --git a/django/db/backends/base/schema.py b/django/db/backends/base/schema.py index bd07385672..cf18e7653e 100644 --- a/django/db/backends/base/schema.py +++ b/django/db/backends/base/schema.py @@ -14,12 +14,28 @@ from django.utils.encoding import force_bytes logger = logging.getLogger('django.db.backends.schema') +def _is_relevant_relation(relation, altered_field): + """ + When altering the given field, must constraints on its model from the given + relation be temporarily dropped? + """ + field = relation.field + if field.many_to_many: + # M2M reverse field + return False + if altered_field.primary_key and field.to_fields == [None]: + # Foreign key constraint on the primary key, which is being altered. + return True + # Is the constraint targeting the field being altered? + return altered_field.name in field.to_fields + + def _related_non_m2m_objects(old_field, new_field): # Filter out m2m objects from reverse relations. # Return (old_relation, new_relation) tuples. return zip( - (obj for obj in old_field.model._meta.related_objects if not obj.field.many_to_many), - (obj for obj in new_field.model._meta.related_objects if not obj.field.many_to_many) + (obj for obj in old_field.model._meta.related_objects if _is_relevant_relation(obj, old_field)), + (obj for obj in new_field.model._meta.related_objects if _is_relevant_relation(obj, new_field)) ) @@ -731,7 +747,7 @@ class BaseDatabaseSchemaEditor: # Rebuild FKs that pointed to us if we previously had to drop them if drop_foreign_keys: for rel in new_field.model._meta.related_objects: - if not rel.many_to_many and rel.field.db_constraint: + if _is_relevant_relation(rel, new_field) and rel.field.db_constraint: self.execute(self._create_fk_sql(rel.related_model, rel.field, "_fk")) # Does it have check constraints we need to add? if old_db_params['check'] != new_db_params['check'] and new_db_params['check']: diff --git a/docs/releases/1.11.13.txt b/docs/releases/1.11.13.txt index 8dbd651652..b9fd3329ef 100644 --- a/docs/releases/1.11.13.txt +++ b/docs/releases/1.11.13.txt @@ -9,4 +9,6 @@ Django 1.11.13 fixes several bugs in 1.11.12. Bugfixes ======== -* ... +* Fixed a regression in Django 1.11.8 where altering a field with a unique + constraint may drop and rebuild more foreign keys than necessary + (:ticket:`29193`). diff --git a/docs/releases/2.0.5.txt b/docs/releases/2.0.5.txt index c7a7a84d9e..c88135aeb9 100644 --- a/docs/releases/2.0.5.txt +++ b/docs/releases/2.0.5.txt @@ -11,3 +11,7 @@ Bugfixes * Corrected the import paths that ``inspectdb`` generates for ``django.contrib.postgres`` fields (:ticket:`29307`). + +* Fixed a regression in Django 1.11.8 where altering a field with a unique + constraint may drop and rebuild more foreign keys than necessary + (:ticket:`29193`). diff --git a/tests/schema/tests.py b/tests/schema/tests.py index 12c33a8433..b535a834f9 100644 --- a/tests/schema/tests.py +++ b/tests/schema/tests.py @@ -1545,6 +1545,40 @@ class SchemaTests(TransactionTestCase): TagUniqueRename.objects.create(title="bar", slug2="foo") Tag.objects.all().delete() + @isolate_apps('schema') + @unittest.skipIf(connection.vendor == 'sqlite', 'SQLite naively remakes the table on field alteration.') + @skipUnlessDBFeature('supports_foreign_keys') + def test_unique_no_unnecessary_fk_drops(self): + """ + If AlterField isn't selective about dropping foreign key constraints + when modifying a field with a unique constraint, the AlterField + incorrectly drops and recreates the Book.author foreign key even though + it doesn't restrict the field being changed (#29193). + """ + class Author(Model): + name = CharField(max_length=254, unique=True) + + class Meta: + app_label = 'schema' + + class Book(Model): + author = ForeignKey(Author, CASCADE) + + class Meta: + app_label = 'schema' + + with connection.schema_editor() as editor: + editor.create_model(Author) + editor.create_model(Book) + new_field = CharField(max_length=255, unique=True) + new_field.model = Author + new_field.set_attributes_from_name('name') + with patch_logger('django.db.backends.schema', 'debug') as logger_calls: + with connection.schema_editor() as editor: + editor.alter_field(Author, Author._meta.get_field('name'), new_field) + # One SQL statement is executed to alter the field. + self.assertEqual(len(logger_calls), 1) + @isolate_apps('schema') @unittest.skipIf(connection.vendor == 'sqlite', 'SQLite remakes the table on field alteration.') def test_unique_and_reverse_m2m(self):