From 8f76939f54924b01b41d4242e71ca98eb35964f2 Mon Sep 17 00:00:00 2001 From: Jeremy Bowman Date: Mon, 9 Apr 2018 13:35:06 -0400 Subject: [PATCH] [1.11.x] 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. Backport of ee17bb8a67a9e7e688da6e6f4b3be1b3a69c09b0 from master --- AUTHORS | 1 + django/db/backends/base/schema.py | 22 +++++++++++++++++--- docs/releases/1.11.13.txt | 4 +++- tests/schema/tests.py | 34 +++++++++++++++++++++++++++++++ 4 files changed, 57 insertions(+), 4 deletions(-) diff --git a/AUTHORS b/AUTHORS index 9d27cd039b..4f92fd3ef7 100644 --- a/AUTHORS +++ b/AUTHORS @@ -365,6 +365,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 8802476c07..efef3e0f3c 100644 --- a/django/db/backends/base/schema.py +++ b/django/db/backends/base/schema.py @@ -11,12 +11,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): # Filters out m2m objects from reverse relations. # Returns (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)) ) @@ -780,7 +796,7 @@ class BaseDatabaseSchemaEditor(object): # 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/tests/schema/tests.py b/tests/schema/tests.py index 36daccb19b..1f8f97188c 100644 --- a/tests/schema/tests.py +++ b/tests/schema/tests.py @@ -1392,6 +1392,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):