Test and fix for unique index lost when renaming M2M field (#88)

sparrowt · web-flow · commit 853f5b5a9825 · 2022-01-25T10:45:08.000-08:00
* Fix ambiguous issue number references from previous fork Add absolute links in place of #NN which are no longer valid in this repo. * TDD: test which fails due to mssql-django/issues/86 * Don't drop unique indexes before M2M rename This method override was added by: ESSolutions/django-mssql-backend@08c2721 meaning that it dropped (but did not re-instate) any unique index on the M2M through table, before renaming that table. However MSSQL does not seem to require this, so rather than trying to re-create the correct indexes afterwards just don't drop them unnecessarily in the first place.
diff --git a/mssql/schema.py b/mssql/schema.py
@@ -225,14 +225,6 @@ def _model_indexes_sql(self, model):
                 output.append(index.create_sql(model, self))
         return output
 
-    def _alter_many_to_many(self, model, old_field, new_field, strict):
-        """Alter M2Ms to repoint their to= endpoints."""
-
-        for idx in self._constraint_names(old_field.remote_field.through, index=True, unique=True):
-            self.execute(self.sql_delete_index % {'name': idx, 'table': old_field.remote_field.through._meta.db_table})
-
-        return super()._alter_many_to_many(model, old_field, new_field, strict)
-
     def _db_table_constraint_names(self, db_table, column_names=None, unique=None,
                                    primary_key=None, index=None, foreign_key=None,
                                    check=None, type_=None, exclude=None):
diff --git a/testapp/migrations/0002_test_unique_nullable_part1.py b/testapp/migrations/0002_test_unique_nullable_part1.py
@@ -8,7 +8,7 @@ class Migration(migrations.Migration):
     ]
 
     operations = [
-        # Issue #38 test prep
+        # Prep test for issue https://github.com/ESSolutions/django-mssql-backend/issues/38
         # Create with a field that is unique *and* nullable so it is implemented with a filtered unique index.
         migrations.CreateModel(
             name='TestUniqueNullableModel',
diff --git a/testapp/migrations/0003_test_unique_nullable_part2.py b/testapp/migrations/0003_test_unique_nullable_part2.py
@@ -8,7 +8,7 @@ class Migration(migrations.Migration):
     ]
 
     operations = [
-        # Issue #38 test
+        # Run test for issue https://github.com/ESSolutions/django-mssql-backend/issues/38
         # Now remove the null=True to check this transition is correctly handled.
         migrations.AlterField(
             model_name='testuniquenullablemodel',
diff --git a/testapp/migrations/0004_test_unique_type_change_part1.py b/testapp/migrations/0004_test_unique_type_change_part1.py
@@ -7,7 +7,7 @@ class Migration(migrations.Migration):
         ('testapp', '0003_test_unique_nullable_part2'),
     ]
 
-    # Issue #45 test prep
+    # Prep test for issue https://github.com/ESSolutions/django-mssql-backend/issues/45
     operations = [
         # for case 1:
         migrations.AddField(
diff --git a/testapp/migrations/0005_test_unique_type_change_part2.py b/testapp/migrations/0005_test_unique_type_change_part2.py
@@ -4,10 +4,10 @@
 class Migration(migrations.Migration):
 
     dependencies = [
-        ('testapp', '0004_test_issue45_unique_type_change_part1'),
+        ('testapp', '0004_test_unique_type_change_part1'),
     ]
 
-    # Issue #45 test
+    # Run test for issue https://github.com/ESSolutions/django-mssql-backend/issues/45
     operations = [
         # Case 1: changing max_length changes the column type - the filtered UNIQUE INDEX which implements
         # the nullable unique constraint, should be correctly reinstated after this change of column type
diff --git a/testapp/migrations/0006_test_remove_onetoone_field_part1.py b/testapp/migrations/0006_test_remove_onetoone_field_part1.py
@@ -7,7 +7,7 @@
 class Migration(migrations.Migration):
 
     dependencies = [
-        ('testapp', '0005_test_issue45_unique_type_change_part2'),
+        ('testapp', '0005_test_unique_type_change_part2'),
     ]
 
     operations = [
diff --git a/testapp/migrations/0012_test_indexes_retained_part1.py b/testapp/migrations/0012_test_indexes_retained_part1.py
@@ -7,7 +7,7 @@ class Migration(migrations.Migration):
         ('testapp', '0011_test_unique_constraints'),
     ]
 
-    # Issue #58 test prep
+    # Prep test for issue https://github.com/ESSolutions/django-mssql-backend/issues/58
     operations = [
         migrations.CreateModel(
             name='TestIndexesRetained',
diff --git a/testapp/migrations/0013_test_indexes_retained_part2.py b/testapp/migrations/0013_test_indexes_retained_part2.py
@@ -7,7 +7,8 @@ class Migration(migrations.Migration):
         ('testapp', '0012_test_indexes_retained_part1'),
     ]
 
-    # Issue #58 test operations which should leave index intact
+    # Run test for issue https://github.com/ESSolutions/django-mssql-backend/issues/58
+    # where the following operations should leave indexes intact
     operations = [
         migrations.AlterField(
             model_name='testindexesretained',
diff --git a/testapp/migrations/0014_test_rename_m2mfield_part1.py b/testapp/migrations/0014_test_rename_m2mfield_part1.py
@@ -0,0 +1,26 @@
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('testapp', '0013_test_indexes_retained_part2'),
+    ]
+
+    operations = [
+        # Prep test for issue https://github.com/microsoft/mssql-django/issues/86
+        migrations.CreateModel(
+            name='M2MOtherModel',
+            fields=[
+                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('name', models.CharField(max_length=10)),
+            ],
+        ),
+        migrations.CreateModel(
+            name='TestRenameManyToManyFieldModel',
+            fields=[
+                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('others', models.ManyToManyField(to='testapp.M2MOtherModel')),
+            ],
+        ),
+    ]
diff --git a/testapp/migrations/0015_test_rename_m2mfield_part2.py b/testapp/migrations/0015_test_rename_m2mfield_part2.py
@@ -0,0 +1,19 @@
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('testapp', '0014_test_rename_m2mfield_part1'),
+    ]
+
+    operations = [
+        # Run test for issue https://github.com/microsoft/mssql-django/issues/86
+        # Must be in a separate migration so that the unique index was created
+        # (deferred after the previous migration) before we do the rename.
+        migrations.RenameField(
+            model_name='testrenamemanytomanyfieldmodel',
+            old_name='others',
+            new_name='others_renamed',
+        ),
+    ]
diff --git a/testapp/models.py b/testapp/models.py
@@ -48,13 +48,13 @@ def __str__(self):
 
 
 class TestUniqueNullableModel(models.Model):
-    # Issue #38:
+    # Issue https://github.com/ESSolutions/django-mssql-backend/issues/38:
     # This field started off as unique=True *and* null=True so it is implemented with a filtered unique index
     # Then it is made non-nullable by a subsequent migration, to check this is correctly handled (the index
     # should be dropped, then a normal unique constraint should be added, now that the column is not nullable)
     test_field = models.CharField(max_length=100, unique=True)
 
-    # Issue #45 (case 1)
+    # Issue https://github.com/ESSolutions/django-mssql-backend/issues/45 (case 1)
     # Field used for testing changing the 'type' of a field that's both unique & nullable
     x = models.CharField(max_length=11, null=True, unique=True)
 
@@ -63,22 +63,24 @@ class TestNullableUniqueTogetherModel(models.Model):
     class Meta:
         unique_together = (('a', 'b', 'c'),)
 
-    # Issue #45 (case 2)
+    # Issue https://github.com/ESSolutions/django-mssql-backend/issues/45 (case 2)
     # Fields used for testing changing the 'type of a field that is in a `unique_together`
     a = models.CharField(max_length=51, null=True)
     b = models.CharField(max_length=50)
     c = models.CharField(max_length=50)
 
 
 class TestRemoveOneToOneFieldModel(models.Model):
-    # Fields used for testing removing OneToOne field. Verifies that delete_unique does not try to remove
-    # indexes that have already been removed (Issue #51)
+    # Issue https://github.com/ESSolutions/django-mssql-backend/pull/51
+    # Fields used for testing removing OneToOne field. Verifies that delete_unique
+    # does not try to remove indexes that have already been removed
     # b = models.OneToOneField('self', on_delete=models.SET_NULL, null=True)
     a = models.CharField(max_length=50)
 
 
 class TestIndexesRetainedRenamed(models.Model):
-    # Issue #58 - in all these cases the column index should still exist afterwards
+    # Issue https://github.com/ESSolutions/django-mssql-backend/issues/58
+    # In all these cases the column index should still exist afterwards
     # case (a) `a` starts out not nullable, but then is changed to be nullable
     a = models.IntegerField(db_index=True, null=True)
     # case (b) column originally called `b` is renamed
@@ -87,6 +89,15 @@ class TestIndexesRetainedRenamed(models.Model):
     c = models.IntegerField(db_index=True)
 
 
+class M2MOtherModel(models.Model):
+    name = models.CharField(max_length=10)
+
+
+class TestRenameManyToManyFieldModel(models.Model):
+    # Issue https://github.com/microsoft/mssql-django/issues/86
+    others_renamed = models.ManyToManyField(M2MOtherModel)
+
+
 class Topping(models.Model):
     name = models.UUIDField(primary_key=True, default=uuid.uuid4)
 
diff --git a/testapp/tests/test_constraints.py b/testapp/tests/test_constraints.py
@@ -10,18 +10,21 @@
 from ..models import (
     Author,
     Editor,
+    M2MOtherModel,
     Post,
     TestUniqueNullableModel,
     TestNullableUniqueTogetherModel,
+    TestRenameManyToManyFieldModel,
 )
 
 
 @skipUnlessDBFeature('supports_nullable_unique_constraints')
 class TestNullableUniqueColumn(TestCase):
     def test_multiple_nulls(self):
-        # Issue #45 (case 1) - after field `x` has had its type changed, the filtered UNIQUE
-        # INDEX which is implementing the nullable unique constraint should still be correctly
-        # in place - i.e. allowing multiple NULLs but still enforcing uniqueness of non-NULLs
+        # Issue https://github.com/ESSolutions/django-mssql-backend/issues/45 (case 1)
+        # After field `x` has had its type changed, the filtered UNIQUE INDEX which is
+        # implementing the nullable unique constraint should still be correctly in place
+        # i.e. allowing multiple NULLs but still enforcing uniqueness of non-NULLs
 
         # Allowed
         TestUniqueNullableModel.objects.create(x=None, test_field='randomness')
@@ -50,8 +53,9 @@ def test_partially_nullable(self):
             Post.objects.create(title="foo", author=author, alt_editor=editor)
 
     def test_after_type_change(self):
-        # Issue #45 (case 2) - after one of the fields in the `unique_together` has had its
-        # type changed in a migration, the constraint should still be correctly enforced
+        # Issue https://github.com/ESSolutions/django-mssql-backend/issues/45 (case 2)
+        # After one of the fields in the `unique_together` has had its type changed
+        # in a migration, the constraint should still be correctly enforced
 
         # Multiple rows with a=NULL are considered different
         TestNullableUniqueTogetherModel.objects.create(a=None, b='bbb', c='ccc')
@@ -63,6 +67,21 @@ def test_after_type_change(self):
             TestNullableUniqueTogetherModel.objects.create(a='aaa', b='bbb', c='ccc')
 
 
+class TestRenameManyToManyField(TestCase):
+    def test_uniqueness_still_enforced_afterwards(self):
+        # Issue https://github.com/microsoft/mssql-django/issues/86
+        # Prep
+        thing1 = TestRenameManyToManyFieldModel.objects.create()
+        other1 = M2MOtherModel.objects.create(name='1')
+        other2 = M2MOtherModel.objects.create(name='2')
+        thing1.others_renamed.set([other1, other2])
+        # Check that the unique_together on the through table is still enforced
+        ThroughModel = TestRenameManyToManyFieldModel.others_renamed.through
+        with self.assertRaises(IntegrityError, msg='Through model fails to enforce uniqueness after m2m rename'):
+            # This should fail due to the unique_together because (thing1, other1) is already in the through table
+            ThroughModel.objects.create(testrenamemanytomanyfieldmodel=thing1, m2mothermodel=other1)
+
+
 class TestUniqueConstraints(TransactionTestCase):
     def test_unsupportable_unique_constraint(self):
         # Only execute tests when running against SQL Server
diff --git a/testapp/tests/test_indexes.py b/testapp/tests/test_indexes.py
@@ -8,8 +8,9 @@
 
 class TestIndexesRetained(TestCase):
     """
+    Issue https://github.com/ESSolutions/django-mssql-backend/issues/58
     Indexes dropped during a migration should be re-created afterwards
-    assuming the field still has `db_index=True` (issue #58)
+    assuming the field still has `db_index=True`
     """
 
     @classmethod
@@ -35,13 +36,13 @@ def _assert_index_exists(self, columns):
         )
 
     def test_field_made_nullable(self):
-        # Issue #58 case (a)
+        # case (a) of https://github.com/ESSolutions/django-mssql-backend/issues/58
         self._assert_index_exists({'a'})
 
     def test_field_renamed(self):
-        # Issue #58 case (b)
+        # case (b) of https://github.com/ESSolutions/django-mssql-backend/issues/58
         self._assert_index_exists({'b_renamed'})
 
     def test_table_renamed(self):
-        # Issue #58 case (c)
+        # case (c) of https://github.com/ESSolutions/django-mssql-backend/issues/58
         self._assert_index_exists({'c'})

Original file line number	Diff line number	Diff line change
`@@ -8,7 +8,7 @@ class Migration(migrations.Migration):`
`8`	`8`	`]`
`9`	`9`
`10`	`10`	`operations = [`
`11`		`- # Issue #38 test prep`
	`11`	`+ # Prep test for issue https://github.com/ESSolutions/django-mssql-backend/issues/38`
`12`	`12`	`# Create with a field that is unique and nullable so it is implemented with a filtered unique index.`
`13`	`13`	`migrations.CreateModel(`
`14`	`14`	`name='TestUniqueNullableModel',`
Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,7 @@ class Migration(migrations.Migration):`
`7`	`7`	`('testapp', '0003_test_unique_nullable_part2'),`
`8`	`8`	`]`
`9`	`9`
`10`		`- # Issue #45 test prep`
	`10`	`+ # Prep test for issue https://github.com/ESSolutions/django-mssql-backend/issues/45`
`11`	`11`	`operations = [`
`12`	`12`	`# for case 1:`
`13`	`13`	`migrations.AddField(`
Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,7 @@`
`7`	`7`	`class Migration(migrations.Migration):`
`8`	`8`
`9`	`9`	`dependencies = [`
`10`		`- ('testapp', '0005_test_issue45_unique_type_change_part2'),`
	`10`	`+ ('testapp', '0005_test_unique_type_change_part2'),`
`11`	`11`	`]`
`12`	`12`
`13`	`13`	`operations = [`
Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,7 @@ class Migration(migrations.Migration):`
`7`	`7`	`('testapp', '0011_test_unique_constraints'),`
`8`	`8`	`]`
`9`	`9`
`10`		`- # Issue #58 test prep`
	`10`	`+ # Prep test for issue https://github.com/ESSolutions/django-mssql-backend/issues/58`
`11`	`11`	`operations = [`
`12`	`12`	`migrations.CreateModel(`
`13`	`13`	`name='TestIndexesRetained',`
Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,8 @@ class Migration(migrations.Migration):`
`7`	`7`	`('testapp', '0012_test_indexes_retained_part1'),`
`8`	`8`	`]`
`9`	`9`
`10`		`- # Issue #58 test operations which should leave index intact`
	`10`	`+ # Run test for issue https://github.com/ESSolutions/django-mssql-backend/issues/58`
	`11`	`+ # where the following operations should leave indexes intact`
`11`	`12`	`operations = [`
`12`	`13`	`migrations.AlterField(`
`13`	`14`	`model_name='testindexesretained',`