diff --git a/django/contrib/gis/db/backends/spatialite/schema.py b/django/contrib/gis/db/backends/spatialite/schema.py index 6f4e3380db..93ea754488 100644 --- a/django/contrib/gis/db/backends/spatialite/schema.py +++ b/django/contrib/gis/db/backends/spatialite/schema.py @@ -123,7 +123,7 @@ class SpatialiteSchemaEditor(DatabaseSchemaEditor): else: super().remove_field(model, field) - def alter_db_table(self, model, old_db_table, new_db_table): + def alter_db_table(self, model, old_db_table, new_db_table, disable_constraints=True): from django.contrib.gis.db.models.fields import GeometryField # Remove geometry-ness from temp table for field in model._meta.local_fields: @@ -135,7 +135,7 @@ class SpatialiteSchemaEditor(DatabaseSchemaEditor): } ) # Alter table - super().alter_db_table(model, old_db_table, new_db_table) + super().alter_db_table(model, old_db_table, new_db_table, disable_constraints) # Repoint any straggler names for geom_table in self.geometry_tables: try: diff --git a/django/db/backends/base/features.py b/django/db/backends/base/features.py index 3706e12db1..cde66be422 100644 --- a/django/db/backends/base/features.py +++ b/django/db/backends/base/features.py @@ -164,6 +164,9 @@ class BaseDatabaseFeatures: # Can we roll back DDL in a transaction? can_rollback_ddl = False + # Does it support operations requiring references rename in a transaction? + supports_atomic_references_rename = True + # Can we issue more than one ALTER COLUMN clause in an ALTER TABLE? supports_combined_alters = False diff --git a/django/db/backends/sqlite3/features.py b/django/db/backends/sqlite3/features.py index 1c3d2a7378..7563edf1c0 100644 --- a/django/db/backends/sqlite3/features.py +++ b/django/db/backends/sqlite3/features.py @@ -22,6 +22,7 @@ class DatabaseFeatures(BaseDatabaseFeatures): supports_transactions = True atomic_transactions = False can_rollback_ddl = True + supports_atomic_references_rename = False supports_paramstyle_pyformat = False supports_sequence_reset = False can_clone_databases = True diff --git a/django/db/backends/sqlite3/schema.py b/django/db/backends/sqlite3/schema.py index 71f6c1c303..c2950ffd2e 100644 --- a/django/db/backends/sqlite3/schema.py +++ b/django/db/backends/sqlite3/schema.py @@ -5,6 +5,8 @@ from decimal import Decimal from django.apps.registry import Apps from django.db.backends.base.schema import BaseDatabaseSchemaEditor from django.db.backends.ddl_references import Statement +from django.db.transaction import atomic +from django.db.utils import NotSupportedError class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): @@ -53,6 +55,58 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): else: raise ValueError("Cannot quote parameter value %r of type %s" % (value, type(value))) + def alter_db_table(self, model, old_db_table, new_db_table, disable_constraints=True): + if model._meta.related_objects and disable_constraints: + if self.connection.in_atomic_block: + raise NotSupportedError(( + 'Renaming the %r table while in a transaction is not ' + 'supported on SQLite because it would break referential ' + 'integrity. Try adding `atomic = False` to the Migration class.' + ) % old_db_table) + self.connection.enable_constraint_checking() + super().alter_db_table(model, old_db_table, new_db_table) + self.connection.disable_constraint_checking() + else: + super().alter_db_table(model, old_db_table, new_db_table) + + def alter_field(self, model, old_field, new_field, strict=False): + old_field_name = old_field.name + if (new_field.name != old_field_name and + any(r.field_name == old_field.name for r in model._meta.related_objects)): + if self.connection.in_atomic_block: + raise NotSupportedError(( + 'Renaming the %r.%r column while in a transaction is not ' + 'supported on SQLite because it would break referential ' + 'integrity. Try adding `atomic = False` to the Migration class.' + ) % (model._meta.db_table, old_field_name)) + with atomic(self.connection.alias): + super().alter_field(model, old_field, new_field, strict=strict) + # Follow SQLite's documented procedure for performing changes + # that don't affect the on-disk content. + # https://sqlite.org/lang_altertable.html#otheralter + with self.connection.cursor() as cursor: + schema_version = cursor.execute('PRAGMA schema_version').fetchone()[0] + cursor.execute('PRAGMA writable_schema = 1') + table_name = model._meta.db_table + references_template = ' REFERENCES "%s" ("%%s") ' % table_name + old_column_name = old_field.get_attname_column()[1] + new_column_name = new_field.get_attname_column()[1] + search = references_template % old_column_name + replacement = references_template % new_column_name + cursor.execute('UPDATE sqlite_master SET sql = replace(sql, %s, %s)', (search, replacement)) + cursor.execute('PRAGMA schema_version = %d' % (schema_version + 1)) + cursor.execute('PRAGMA writable_schema = 0') + # The integrity check will raise an exception and rollback + # the transaction if the sqlite_master updates corrupt the + # database. + cursor.execute('PRAGMA integrity_check') + # Perform a VACUUM to refresh the database representation from + # the sqlite_master table. + with self.connection.cursor() as cursor: + cursor.execute('VACUUM') + else: + super().alter_field(model, old_field, new_field, strict=strict) + def _remake_table(self, model, create_field=None, delete_field=None, alter_field=None): """ Shortcut to transform a model from old_model into new_model @@ -176,8 +230,10 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): with altered_table_name(model, model._meta.db_table + "__old"): # Rename the old table to make way for the new - self.alter_db_table(model, temp_model._meta.db_table, model._meta.db_table) - + self.alter_db_table( + model, temp_model._meta.db_table, model._meta.db_table, + disable_constraints=False, + ) # Create a new table with the updated schema. self.create_model(temp_model) diff --git a/docs/releases/2.0.txt b/docs/releases/2.0.txt index 904597a033..34068a7a05 100644 --- a/docs/releases/2.0.txt +++ b/docs/releases/2.0.txt @@ -534,6 +534,13 @@ rebuild tables using a script similar to this:: This script hasn't received extensive testing and needs adaption for various cases such as multiple databases. Feel free to contribute improvements. +In addition, because of a table alteration limitation of SQLite, it's prohibited +to perform :class:`~django.db.migrations.operations.RenameModel` and +:class:`~django.db.migrations.operations.RenameField` operations on models or +fields referenced by other models in a transaction. In order to allow migrations +containing these operations to be applied, you must set the +``Migration.atomic`` attribute to ``False``. + Miscellaneous ------------- diff --git a/tests/backends/models.py b/tests/backends/models.py index 6277e52525..1fa8d44e63 100644 --- a/tests/backends/models.py +++ b/tests/backends/models.py @@ -103,3 +103,11 @@ class ObjectReference(models.Model): class RawData(models.Model): raw_data = models.BinaryField() + + +class Author(models.Model): + name = models.CharField(max_length=255, unique=True) + + +class Book(models.Model): + author = models.ForeignKey(Author, models.CASCADE, to_field='name') diff --git a/tests/backends/sqlite/tests.py b/tests/backends/sqlite/tests.py index 0fbb139278..c82ed1667d 100644 --- a/tests/backends/sqlite/tests.py +++ b/tests/backends/sqlite/tests.py @@ -4,10 +4,12 @@ import unittest from django.db import connection from django.db.models import Avg, StdDev, Sum, Variance +from django.db.models.fields import CharField from django.db.utils import NotSupportedError from django.test import TestCase, TransactionTestCase, override_settings +from django.test.utils import isolate_apps -from ..models import Item, Object, Square +from ..models import Author, Item, Object, Square @unittest.skipUnless(connection.vendor == 'sqlite', 'SQLite tests') @@ -58,6 +60,44 @@ class Tests(TestCase): self.assertEqual(creation._get_test_db_name(), creation.connection.settings_dict['TEST']['NAME']) +@unittest.skipUnless(connection.vendor == 'sqlite', 'SQLite tests') +@isolate_apps('backends') +class SchemaTests(TransactionTestCase): + + available_apps = ['backends'] + + def test_field_rename_inside_atomic_block(self): + """ + NotImplementedError is raised when a model field rename is attempted + inside an atomic block. + """ + new_field = CharField(max_length=255, unique=True) + new_field.set_attributes_from_name('renamed') + msg = ( + "Renaming the 'backends_author'.'name' column while in a " + "transaction is not supported on SQLite because it would break " + "referential integrity. Try adding `atomic = False` to the " + "Migration class." + ) + with self.assertRaisesMessage(NotSupportedError, msg): + with connection.schema_editor(atomic=True) as editor: + editor.alter_field(Author, Author._meta.get_field('name'), new_field) + + def test_table_rename_inside_atomic_block(self): + """ + NotImplementedError is raised when a table rename is attempted inside + an atomic block. + """ + msg = ( + "Renaming the 'backends_author' table while in a transaction is " + "not supported on SQLite because it would break referential " + "integrity. Try adding `atomic = False` to the Migration class." + ) + with self.assertRaisesMessage(NotSupportedError, msg): + with connection.schema_editor(atomic=True) as editor: + editor.alter_db_table(Author, "backends_author", "renamed_table") + + @unittest.skipUnless(connection.vendor == 'sqlite', 'Test only for SQLite') @override_settings(DEBUG=True) class LastExecutedQueryTest(TestCase): diff --git a/tests/migrations/test_operations.py b/tests/migrations/test_operations.py index 02f8713650..2e24047d6d 100644 --- a/tests/migrations/test_operations.py +++ b/tests/migrations/test_operations.py @@ -28,16 +28,16 @@ class OperationTestBase(MigrationTestBase): Common functions to help test operations. """ - def apply_operations(self, app_label, project_state, operations): + def apply_operations(self, app_label, project_state, operations, atomic=True): migration = Migration('name', app_label) migration.operations = operations - with connection.schema_editor() as editor: + with connection.schema_editor(atomic=atomic) as editor: return migration.apply(project_state, editor) - def unapply_operations(self, app_label, project_state, operations): + def unapply_operations(self, app_label, project_state, operations, atomic=True): migration = Migration('name', app_label) migration.operations = operations - with connection.schema_editor() as editor: + with connection.schema_editor(atomic=atomic) as editor: return migration.unapply(project_state, editor) def make_test_state(self, app_label, operation, **kwargs): @@ -561,7 +561,8 @@ class OperationTests(OperationTestBase): self.assertFKNotExists("test_rnmo_rider", ["pony_id"], ("test_rnmo_horse", "id")) # Migrate forwards new_state = project_state.clone() - new_state = self.apply_operations("test_rnmo", new_state, [operation]) + atomic_rename = connection.features.supports_atomic_references_rename + new_state = self.apply_operations("test_rnmo", new_state, [operation], atomic=atomic_rename) # Test new state and database self.assertNotIn(("test_rnmo", "pony"), new_state.models) self.assertIn(("test_rnmo", "horse"), new_state.models) @@ -573,7 +574,7 @@ class OperationTests(OperationTestBase): self.assertFKNotExists("test_rnmo_rider", ["pony_id"], ("test_rnmo_pony", "id")) self.assertFKExists("test_rnmo_rider", ["pony_id"], ("test_rnmo_horse", "id")) # Migrate backwards - original_state = self.unapply_operations("test_rnmo", project_state, [operation]) + original_state = self.unapply_operations("test_rnmo", project_state, [operation], atomic=atomic_rename) # Test original state and database self.assertIn(("test_rnmo", "pony"), original_state.models) self.assertNotIn(("test_rnmo", "horse"), original_state.models) @@ -634,7 +635,8 @@ class OperationTests(OperationTestBase): if connection.features.supports_foreign_keys: self.assertFKExists("test_rmwsrf_rider", ["friend_id"], ("test_rmwsrf_rider", "id")) self.assertFKNotExists("test_rmwsrf_rider", ["friend_id"], ("test_rmwsrf_horserider", "id")) - with connection.schema_editor() as editor: + atomic_rename = connection.features.supports_atomic_references_rename + with connection.schema_editor(atomic=atomic_rename) as editor: operation.database_forwards("test_rmwsrf", editor, project_state, new_state) self.assertTableNotExists("test_rmwsrf_rider") self.assertTableExists("test_rmwsrf_horserider") @@ -642,7 +644,7 @@ class OperationTests(OperationTestBase): self.assertFKNotExists("test_rmwsrf_horserider", ["friend_id"], ("test_rmwsrf_rider", "id")) self.assertFKExists("test_rmwsrf_horserider", ["friend_id"], ("test_rmwsrf_horserider", "id")) # And test reversal - with connection.schema_editor() as editor: + with connection.schema_editor(atomic=atomic_rename) as editor: operation.database_backwards("test_rmwsrf", editor, new_state, project_state) self.assertTableExists("test_rmwsrf_rider") self.assertTableNotExists("test_rmwsrf_horserider") @@ -675,7 +677,7 @@ class OperationTests(OperationTestBase): # and the foreign key on rider points to pony, not shetland pony self.assertFKExists("test_rmwsc_rider", ["pony_id"], ("test_rmwsc_pony", "id")) self.assertFKNotExists("test_rmwsc_rider", ["pony_id"], ("test_rmwsc_shetlandpony", "id")) - with connection.schema_editor() as editor: + with connection.schema_editor(atomic=connection.features.supports_atomic_references_rename) as editor: operation.database_forwards("test_rmwsc", editor, project_state, new_state) # Now we have a little horse table, not shetland pony self.assertTableNotExists("test_rmwsc_shetlandpony") @@ -696,7 +698,7 @@ class OperationTests(OperationTestBase): ]) project_state = self.apply_operations(app_label, project_state, operations=[ migrations.RenameModel("ReflexivePony", "ReflexivePony2"), - ]) + ], atomic=connection.features.supports_atomic_references_rename) Pony = project_state.apps.get_model(app_label, "ReflexivePony2") pony = Pony.objects.create() pony.ponies.add(pony) @@ -749,7 +751,7 @@ class OperationTests(OperationTestBase): project_state = self.apply_operations(app_label, project_state, operations=[ migrations.RenameModel("Rider", "Rider2"), - ]) + ], atomic=connection.features.supports_atomic_references_rename) Pony = project_state.apps.get_model(app_label, "Pony") Rider = project_state.apps.get_model(app_label, "Rider2") pony = Pony.objects.create() @@ -1397,7 +1399,7 @@ class OperationTests(OperationTestBase): project_state = self.apply_operations(app_label, project_state, operations=[ migrations.RenameField('Rider', 'id', 'id2'), migrations.AlterField('Pony', 'id', models.CharField(primary_key=True, max_length=99)), - ]) + ], atomic=connection.features.supports_atomic_references_rename) def test_rename_field(self): """ diff --git a/tests/schema/tests.py b/tests/schema/tests.py index 66a54b1ce4..10c8b9780b 100644 --- a/tests/schema/tests.py +++ b/tests/schema/tests.py @@ -173,7 +173,7 @@ class SchemaTests(TransactionTestCase): index_orders = constraints[index]['orders'] self.assertTrue(all(val == expected for val, expected in zip(index_orders, order))) - def assertForeignKeyExists(self, model, column, expected_fk_table): + def assertForeignKeyExists(self, model, column, expected_fk_table, field='id'): """ Fail if the FK constraint on `model.Meta.db_table`.`column` to `expected_fk_table`.id doesn't exist. @@ -184,7 +184,7 @@ class SchemaTests(TransactionTestCase): if details['columns'] == [column] and details['foreign_key']: constraint_fk = details['foreign_key'] break - self.assertEqual(constraint_fk, (expected_fk_table, 'id')) + self.assertEqual(constraint_fk, (expected_fk_table, field)) def assertForeignKeyNotExists(self, model, column, expected_fk_table): with self.assertRaises(AssertionError): @@ -1147,6 +1147,30 @@ class SchemaTests(TransactionTestCase): self.assertEqual(columns['display_name'][0], "CharField") self.assertNotIn("name", columns) + @isolate_apps('schema') + def test_rename_referenced_field(self): + class Author(Model): + name = CharField(max_length=255, unique=True) + + class Meta: + app_label = 'schema' + + class Book(Model): + author = ForeignKey(Author, CASCADE, to_field='name') + + 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.set_attributes_from_name('renamed') + with connection.schema_editor(atomic=connection.features.supports_atomic_references_rename) as editor: + editor.alter_field(Author, Author._meta.get_field('name'), new_field) + # Ensure the foreign key reference was updated. + self.assertForeignKeyExists(Book, 'author_id', 'schema_author', 'renamed') + @skipIfDBFeature('interprets_empty_strings_as_nulls') def test_rename_keep_null_status(self): """ @@ -1625,25 +1649,41 @@ class SchemaTests(TransactionTestCase): ), ) + @isolate_apps('schema') def test_db_table(self): """ Tests renaming of the table """ - # Create the table + class Author(Model): + name = CharField(max_length=255) + + class Meta: + app_label = 'schema' + + class Book(Model): + author = ForeignKey(Author, CASCADE) + + class Meta: + app_label = 'schema' + + # Create the table and one referring it. with connection.schema_editor() as editor: editor.create_model(Author) + editor.create_model(Book) # Ensure the table is there to begin with columns = self.column_classes(Author) self.assertEqual(columns['name'][0], "CharField") # Alter the table - with connection.schema_editor() as editor: + with connection.schema_editor(atomic=connection.features.supports_atomic_references_rename) as editor: editor.alter_db_table(Author, "schema_author", "schema_otherauthor") # Ensure the table is there afterwards Author._meta.db_table = "schema_otherauthor" columns = self.column_classes(Author) self.assertEqual(columns['name'][0], "CharField") + # Ensure the foreign key reference was updated + self.assertForeignKeyExists(Book, "author_id", "schema_otherauthor") # Alter the table again - with connection.schema_editor() as editor: + with connection.schema_editor(atomic=connection.features.supports_atomic_references_rename) as editor: editor.alter_db_table(Author, "schema_otherauthor", "schema_author") # Ensure the table is still there Author._meta.db_table = "schema_author"