[2.0.x] Fixed #28849 -- Fixed referenced table and column rename on SQLite.

Thanks Ramiro for the input and Tim for the review.

Backport of 095c1aaa from master

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:

django/db/backends/base/features.py

@@ -167,6 +167,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
 

django/db/backends/sqlite3/features.py

@@ -24,6 +24,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

django/db/backends/sqlite3/schema.py

@@ -6,6 +6,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):
@@ -59,6 +61,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
@@ -182,8 +236,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)
 

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
 -------------
 

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')

tests/backends/sqlite/tests.py

@@ -5,11 +5,14 @@ import unittest
 from django.core.exceptions import ImproperlyConfigured
 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, skipUnlessDBFeature,
 )
+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')
@@ -71,6 +74,44 @@ class Tests(TestCase):
                 creation._get_test_db_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):

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):
         """

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"