From 9356f63a99957f01c14a9788617428a172a29fcb Mon Sep 17 00:00:00 2001 From: Jon Dufresne Date: Wed, 22 Jun 2016 21:46:11 -0700 Subject: [PATCH] Fixed #25317, #26090 -- Fixed swapping combinations of unique and db_index during migrations. --- django/db/backends/base/schema.py | 27 ++- django/db/backends/postgresql/schema.py | 5 +- tests/schema/tests.py | 216 +++++++++++++++++++++++- 3 files changed, 236 insertions(+), 12 deletions(-) diff --git a/django/db/backends/base/schema.py b/django/db/backends/base/schema.py index b11892dbeb..63775c981b 100644 --- a/django/db/backends/base/schema.py +++ b/django/db/backends/base/schema.py @@ -547,9 +547,16 @@ class BaseDatabaseSchemaEditor(object): for fk_name in rel_fk_names: self.execute(self._delete_constraint_sql(self.sql_delete_fk, new_rel.related_model, fk_name)) # Removed an index? (no strict check, as multiple indexes are possible) - if (old_field.db_index and not new_field.db_index and - not old_field.unique and not - (not new_field.unique and old_field.unique)): + # Remove indexes if db_index switched to False or a unique constraint + # will now be used in lieu of an index. The following lines from the + # truth table show all True cases; the rest are False: + # + # old_field.db_index | old_field.unique | new_field.db_index | new_field.unique + # ------------------------------------------------------------------------------ + # True | False | False | False + # True | False | False | True + # True | False | True | True + if old_field.db_index and not old_field.unique and (not new_field.db_index or new_field.unique): # Find the index for this field index_names = self._constraint_names(model, [old_field.column], index=True) for index_name in index_names: @@ -688,10 +695,16 @@ class BaseDatabaseSchemaEditor(object): old_field.primary_key and not new_field.primary_key and new_field.unique ): self.execute(self._create_unique_sql(model, [new_field.column])) - # Added an index? - if (not old_field.db_index and new_field.db_index and - not new_field.unique and not - (not old_field.unique and new_field.unique)): + # Added an index? Add an index if db_index switched to True or a unique + # constraint will no longer be used in lieu of an index. The following + # lines from the truth table show all True cases; the rest are False: + # + # old_field.db_index | old_field.unique | new_field.db_index | new_field.unique + # ------------------------------------------------------------------------------ + # False | False | True | False + # False | True | True | False + # True | True | True | False + if (not old_field.db_index or old_field.unique) and new_field.db_index and not new_field.unique: self.execute(self._create_index_sql(model, [new_field])) # Type alteration on primary key? Then we need to alter the column # referring to us. diff --git a/django/db/backends/postgresql/schema.py b/django/db/backends/postgresql/schema.py index cd9e6cfd5e..5faaea0672 100644 --- a/django/db/backends/postgresql/schema.py +++ b/django/db/backends/postgresql/schema.py @@ -111,13 +111,14 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): new_db_params, strict, ) # Added an index? Create any PostgreSQL-specific indexes. - if not old_field.db_index and not old_field.unique and (new_field.db_index or new_field.unique): + if ((not (old_field.db_index or old_field.unique) and new_field.db_index) or + (not old_field.unique and new_field.unique)): like_index_statement = self._create_like_index_sql(model, new_field) if like_index_statement is not None: self.execute(like_index_statement) # Removed an index? Drop any PostgreSQL-specific indexes. - if (old_field.db_index or old_field.unique) and not (new_field.db_index or new_field.unique): + if old_field.unique and not (new_field.db_index or new_field.unique): index_to_remove = self._create_index_name(model, [old_field.column], suffix='_like') index_names = self._constraint_names(model, [old_field.column], index=True) for index_name in index_names: diff --git a/tests/schema/tests.py b/tests/schema/tests.py index a721abfe66..ae579d5f33 100644 --- a/tests/schema/tests.py +++ b/tests/schema/tests.py @@ -7,7 +7,7 @@ from django.db import ( DatabaseError, IntegrityError, OperationalError, connection, ) from django.db.models import Model -from django.db.models.deletion import CASCADE +from django.db.models.deletion import CASCADE, PROTECT from django.db.models.fields import ( AutoField, BigIntegerField, BinaryField, BooleanField, CharField, DateField, DateTimeField, IntegerField, PositiveIntegerField, SlugField, @@ -136,6 +136,26 @@ class SchemaTests(TransactionTestCase): database_default = cast_function(database_default) self.assertEqual(database_default, expected_default) + def get_constraints_count(self, table, column, fk_to): + """ + Return a dict with keys 'fks', 'uniques, and 'indexes' indicating the + number of foreign keys, unique constraints, and indexes on + `table`.`column`. The `fk_to` argument is a 2-tuple specifying the + expected foreign key relationship's (table, column). + """ + with connection.cursor() as cursor: + constraints = connection.introspection.get_constraints(cursor, table) + counts = {'fks': 0, 'uniques': 0, 'indexes': 0} + for c in constraints.values(): + if c['columns'] == [column]: + if c['foreign_key'] == fk_to: + counts['fks'] += 1 + if c['unique']: + counts['uniques'] += 1 + elif c['index']: + counts['indexes'] += 1 + return counts + # Tests def test_creation_deletion(self): """ @@ -826,6 +846,120 @@ class SchemaTests(TransactionTestCase): author_is_fk = True self.assertTrue(author_is_fk, "No FK constraint for author_id found") + def test_alter_field_fk_to_o2o(self): + with connection.schema_editor() as editor: + editor.create_model(Author) + editor.create_model(Book) + expected_fks = 1 if connection.features.supports_foreign_keys else 0 + + # Check the index is right to begin with. + counts = self.get_constraints_count( + Book._meta.db_table, + Book._meta.get_field('author').column, + (Author._meta.db_table, Author._meta.pk.column), + ) + self.assertEqual(counts, {'fks': expected_fks, 'uniques': 0, 'indexes': 1}) + + old_field = Book._meta.get_field('author') + new_field = OneToOneField(Author, CASCADE) + new_field.set_attributes_from_name('author') + with connection.schema_editor() as editor: + editor.alter_field(Book, old_field, new_field, strict=True) + + counts = self.get_constraints_count( + Book._meta.db_table, + Book._meta.get_field('author').column, + (Author._meta.db_table, Author._meta.pk.column), + ) + # The index on ForeignKey is replaced with a unique constraint for OneToOneField. + self.assertEqual(counts, {'fks': expected_fks, 'uniques': 1, 'indexes': 0}) + + def test_alter_field_fk_keeps_index(self): + with connection.schema_editor() as editor: + editor.create_model(Author) + editor.create_model(Book) + expected_fks = 1 if connection.features.supports_foreign_keys else 0 + + # Check the index is right to begin with. + counts = self.get_constraints_count( + Book._meta.db_table, + Book._meta.get_field('author').column, + (Author._meta.db_table, Author._meta.pk.column), + ) + self.assertEqual(counts, {'fks': expected_fks, 'uniques': 0, 'indexes': 1}) + + old_field = Book._meta.get_field('author') + # on_delete changed from CASCADE. + new_field = ForeignKey(Author, PROTECT) + new_field.set_attributes_from_name('author') + with connection.schema_editor() as editor: + editor.alter_field(Book, old_field, new_field, strict=True) + + counts = self.get_constraints_count( + Book._meta.db_table, + Book._meta.get_field('author').column, + (Author._meta.db_table, Author._meta.pk.column), + ) + # The index remains. + self.assertEqual(counts, {'fks': expected_fks, 'uniques': 0, 'indexes': 1}) + + def test_alter_field_o2o_to_fk(self): + with connection.schema_editor() as editor: + editor.create_model(Author) + editor.create_model(BookWithO2O) + expected_fks = 1 if connection.features.supports_foreign_keys else 0 + + # Check the unique constraint is right to begin with. + counts = self.get_constraints_count( + BookWithO2O._meta.db_table, + BookWithO2O._meta.get_field('author').column, + (Author._meta.db_table, Author._meta.pk.column), + ) + self.assertEqual(counts, {'fks': expected_fks, 'uniques': 1, 'indexes': 0}) + + old_field = BookWithO2O._meta.get_field('author') + new_field = ForeignKey(Author, CASCADE) + new_field.set_attributes_from_name('author') + with connection.schema_editor() as editor: + editor.alter_field(BookWithO2O, old_field, new_field, strict=True) + + counts = self.get_constraints_count( + BookWithO2O._meta.db_table, + BookWithO2O._meta.get_field('author').column, + (Author._meta.db_table, Author._meta.pk.column), + ) + # The unique constraint on OneToOneField is replaced with an index for ForeignKey. + self.assertEqual(counts, {'fks': expected_fks, 'uniques': 0, 'indexes': 1}) + + def test_alter_field_o2o_keeps_unique(self): + with connection.schema_editor() as editor: + editor.create_model(Author) + editor.create_model(BookWithO2O) + expected_fks = 1 if connection.features.supports_foreign_keys else 0 + + # Check the unique constraint is right to begin with. + counts = self.get_constraints_count( + BookWithO2O._meta.db_table, + BookWithO2O._meta.get_field('author').column, + (Author._meta.db_table, Author._meta.pk.column), + ) + self.assertEqual(counts, {'fks': expected_fks, 'uniques': 1, 'indexes': 0}) + + old_field = BookWithO2O._meta.get_field('author') + # on_delete changed from CASCADE. + new_field = OneToOneField(Author, PROTECT) + new_field.set_attributes_from_name('author') + with connection.schema_editor() as editor: + editor.alter_field(BookWithO2O, old_field, new_field, strict=True) + + counts = self.get_constraints_count( + BookWithO2O._meta.db_table, + BookWithO2O._meta.get_field('author').column, + (Author._meta.db_table, Author._meta.pk.column), + ) + # The unique constraint remains. + self.assertEqual(counts, {'fks': expected_fks, 'uniques': 1, 'indexes': 0}) + def test_alter_db_table_case(self): # Create the table with connection.schema_editor() as editor: @@ -1794,6 +1928,27 @@ class SchemaTests(TransactionTestCase): editor.alter_field(Author, new_field, old_field, strict=True) self.assertEqual(self.get_constraints_for_column(Author, 'name'), []) + @unittest.skipUnless(connection.vendor == 'postgresql', "PostgreSQL specific") + def test_alter_field_add_unique_to_charfield(self): + # Create the table and verify no initial indexes. + with connection.schema_editor() as editor: + editor.create_model(Author) + self.assertEqual(self.get_constraints_for_column(Author, 'name'), []) + # Alter to add unique=True and create 2 indexes. + old_field = Author._meta.get_field('name') + new_field = CharField(max_length=255, unique=True) + new_field.set_attributes_from_name('name') + with connection.schema_editor() as editor: + editor.alter_field(Author, old_field, new_field, strict=True) + self.assertEqual( + self.get_constraints_for_column(Author, 'name'), + ['schema_author_name_1fbc5617_like', 'schema_author_name_1fbc5617_uniq'] + ) + # Remove unique=True to drop both indexes. + with connection.schema_editor() as editor: + editor.alter_field(Author, new_field, old_field, strict=True) + self.assertEqual(self.get_constraints_for_column(Author, 'name'), []) + @unittest.skipUnless(connection.vendor == 'postgresql', "PostgreSQL specific") def test_alter_field_add_index_to_textfield(self): # Create the table and verify no initial indexes. @@ -1824,7 +1979,7 @@ class SchemaTests(TransactionTestCase): self.get_constraints_for_column(BookWithoutAuthor, 'title'), ['schema_book_d5d3db17', 'schema_book_title_2dfb2dff_like'] ) - # Alter to add unique=True (should add 1 index) + # Alter to add unique=True (should replace the index) old_field = BookWithoutAuthor._meta.get_field('title') new_field = CharField(max_length=100, db_index=True, unique=True) new_field.set_attributes_from_name('title') @@ -1832,7 +1987,7 @@ class SchemaTests(TransactionTestCase): editor.alter_field(BookWithoutAuthor, old_field, new_field, strict=True) self.assertEqual( self.get_constraints_for_column(BookWithoutAuthor, 'title'), - ['schema_book_d5d3db17', 'schema_book_title_2dfb2dff_like', 'schema_book_title_2dfb2dff_uniq'] + ['schema_book_title_2dfb2dff_like', 'schema_book_title_2dfb2dff_uniq'] ) # Alter to remove unique=True (should drop unique index) new_field2 = CharField(max_length=100, db_index=True) @@ -1844,6 +1999,61 @@ class SchemaTests(TransactionTestCase): ['schema_book_d5d3db17', 'schema_book_title_2dfb2dff_like'] ) + @unittest.skipUnless(connection.vendor == 'postgresql', "PostgreSQL specific") + def test_alter_field_remove_unique_and_db_index_from_charfield(self): + # Create the table and verify initial indexes. + with connection.schema_editor() as editor: + editor.create_model(BookWithoutAuthor) + self.assertEqual( + self.get_constraints_for_column(BookWithoutAuthor, 'title'), + ['schema_book_d5d3db17', 'schema_book_title_2dfb2dff_like'] + ) + # Alter to add unique=True (should replace the index) + old_field = BookWithoutAuthor._meta.get_field('title') + new_field = CharField(max_length=100, db_index=True, unique=True) + new_field.set_attributes_from_name('title') + with connection.schema_editor() as editor: + editor.alter_field(BookWithoutAuthor, old_field, new_field, strict=True) + self.assertEqual( + self.get_constraints_for_column(BookWithoutAuthor, 'title'), + ['schema_book_title_2dfb2dff_like', 'schema_book_title_2dfb2dff_uniq'] + ) + # Alter to remove both unique=True and db_index=True (should drop all indexes) + new_field2 = CharField(max_length=100) + new_field2.set_attributes_from_name('title') + with connection.schema_editor() as editor: + editor.alter_field(BookWithoutAuthor, new_field, new_field2, strict=True) + self.assertEqual(self.get_constraints_for_column(BookWithoutAuthor, 'title'), []) + + @unittest.skipUnless(connection.vendor == 'postgresql', "PostgreSQL specific") + def test_alter_field_swap_unique_and_db_index_with_charfield(self): + # Create the table and verify initial indexes. + with connection.schema_editor() as editor: + editor.create_model(BookWithoutAuthor) + self.assertEqual( + self.get_constraints_for_column(BookWithoutAuthor, 'title'), + ['schema_book_d5d3db17', 'schema_book_title_2dfb2dff_like'] + ) + # Alter to set unique=True and remove db_index=True (should replace the index) + old_field = BookWithoutAuthor._meta.get_field('title') + new_field = CharField(max_length=100, unique=True) + new_field.set_attributes_from_name('title') + with connection.schema_editor() as editor: + editor.alter_field(BookWithoutAuthor, old_field, new_field, strict=True) + self.assertEqual( + self.get_constraints_for_column(BookWithoutAuthor, 'title'), + ['schema_book_title_2dfb2dff_like', 'schema_book_title_2dfb2dff_uniq'] + ) + # Alter to set db_index=True and remove unique=True (should restore index) + new_field2 = CharField(max_length=100, db_index=True) + new_field2.set_attributes_from_name('title') + with connection.schema_editor() as editor: + editor.alter_field(BookWithoutAuthor, new_field, new_field2, strict=True) + self.assertEqual( + self.get_constraints_for_column(BookWithoutAuthor, 'title'), + ['schema_book_d5d3db17', 'schema_book_title_2dfb2dff_like'] + ) + @unittest.skipUnless(connection.vendor == 'postgresql', "PostgreSQL specific") def test_alter_field_add_db_index_to_charfield_with_unique(self): # Create the table and verify initial indexes.