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

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

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

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

testapp/migrations/0004_test_issue45_unique_type_change_part1.py → 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(

testapp/migrations/0005_test_issue45_unique_type_change_part2.py → 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

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

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

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

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')),
+            ],
+        ),
+    ]

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',
+        ),
+    ]

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)
 

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

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