Fix #45 correctly reinstate nullable unique constraints (#47)

Ensure that the unique/unique_together constraint is
reinstated exactly the same as the one that was dropped
earlier in `_alter_field`

This fixes both cases described on the bug report, that
is when `AlterField` modifies a column which is:
 1) individually `unique=True` as well as `null=True`
 2) in a `unique_together` where at least 1 column is nullable

Author: sparrowt

sql_server/pyodbc/schema.py

@@ -454,21 +454,30 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type,
         # 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]))
-        # Restore an index, SQL Server requires explicit restoration
+
+        # Restore indexes & unique constraints deleted above, SQL Server requires explicit restoration
         if (old_type != new_type or (old_field.null and not new_field.null)) and (
             old_field.column == new_field.column
         ):
-            unique_columns = []
+            # Restore unique constraints
+            # Note: if nullable they are implemented via an explicit filtered UNIQUE INDEX (not CONSTRAINT)
+            # in order to get ANSI-compliant NULL behaviour (i.e. NULL != NULL, multiple are allowed)
             if old_field.unique and new_field.unique:
-                unique_columns.append([old_field.column])
+                if new_field.null:
+                    self.execute(
+                        self._create_index_sql(
+                            model, [old_field], sql=self.sql_create_unique_null, suffix="_uniq"
+                        )
+                    )
+                else:
+                    self.execute(self._create_unique_sql(model, columns=[old_field.column]))
             else:
                 for fields in model._meta.unique_together:
                     columns = [model._meta.get_field(field).column for field in fields]
                     if old_field.column in columns:
-                        unique_columns.append(columns)
-            if unique_columns:
-                for columns in unique_columns:
-                    self.execute(self._create_unique_sql(model, columns))
+                        condition = ' AND '.join(["[%s] IS NOT NULL" % col for col in columns])
+                        self.execute(self._create_unique_sql(model, columns, condition=condition))
+            # Restore indexes
             index_columns = []
             if old_field.db_index and new_field.db_index:
                 index_columns.append([old_field])

testapp/migrations/0002_test_unique_nullable_part1.py

@@ -8,6 +8,7 @@ class Migration(migrations.Migration):
     ]
 
     operations = [
+        # Issue #38 test prep
         # 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,6 +8,7 @@ class Migration(migrations.Migration):
     ]
 
     operations = [
+        # Issue #38 test
         # 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

@@ -0,0 +1,32 @@
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('testapp', '0003_test_unique_nullable_part2'),
+    ]
+
+    # Issue #45 test prep
+    operations = [
+        # for case 1:
+        migrations.AddField(
+            model_name='testuniquenullablemodel',
+            name='x',
+            field=models.CharField(max_length=10, null=True, unique=True),
+        ),
+
+        # for case 2:
+        migrations.CreateModel(
+            name='TestNullableUniqueTogetherModel',
+            fields=[
+                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('a', models.CharField(max_length=50, null=True)),
+                ('b', models.CharField(max_length=50)),
+                ('c', models.CharField(max_length=50)),
+            ],
+            options={
+                'unique_together': {('a', 'b')},
+            },
+        ),
+    ]

testapp/migrations/0005_test_issue45_unique_type_change_part2.py

@@ -0,0 +1,33 @@
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('testapp', '0004_test_issue45_unique_type_change_part1'),
+    ]
+
+    # Issue #45 test
+    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
+        # (see also the specific unit test which checks that multiple rows with NULL are allowed)
+        migrations.AlterField(
+            model_name='testuniquenullablemodel',
+            name='x',
+            field=models.CharField(max_length=11, null=True, unique=True),
+        ),
+
+        # Case 2: the filtered UNIQUE INDEX implementing the partially nullable `unique_together` constraint
+        # should be correctly reinstated after this column type change
+        migrations.AlterField(
+            model_name='testnullableuniquetogethermodel',
+            name='a',
+            field=models.CharField(max_length=51, null=True),
+        ),
+        # ...similarly adding another field to the `unique_together` should preserve the constraint correctly
+        migrations.AlterUniqueTogether(
+            name='testnullableuniquetogethermodel',
+            unique_together={('a', 'b', 'c')},
+        ),
+    ]

testapp/models.py

@@ -44,7 +44,23 @@ def __str__(self):
 
 
 class TestUniqueNullableModel(models.Model):
+    # Issue #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)
+    # 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)
+
+
+class TestNullableUniqueTogetherModel(models.Model):
+    class Meta:
+        unique_together = (('a', 'b', 'c'),)
+
+    # Issue #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)

testapp/tests/test_constraints.py

@@ -0,0 +1,54 @@
+from django.db.utils import IntegrityError
+from django.test import TestCase, skipUnlessDBFeature
+
+from ..models import (
+    Author, Editor, Post,
+    TestUniqueNullableModel, TestNullableUniqueTogetherModel,
+)
+
+
+@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
+
+        # Allowed
+        TestUniqueNullableModel.objects.create(x=None, test_field='randomness')
+        TestUniqueNullableModel.objects.create(x=None, test_field='doesntmatter')
+
+        # Disallowed
+        TestUniqueNullableModel.objects.create(x="foo", test_field='irrelevant')
+        with self.assertRaises(IntegrityError):
+            TestUniqueNullableModel.objects.create(x="foo", test_field='nonsense')
+
+
+@skipUnlessDBFeature('supports_partially_nullable_unique_constraints')
+class TestPartiallyNullableUniqueTogether(TestCase):
+    def test_partially_nullable(self):
+        # Check basic behaviour of `unique_together` where at least 1 of the columns is nullable
+
+        # It should be possible to have 2 rows both with NULL `alt_editor`
+        author = Author.objects.create(name="author")
+        Post.objects.create(title="foo", author=author)
+        Post.objects.create(title="foo", author=author)
+
+        # But `unique_together` is still enforced for non-NULL values
+        editor = Editor.objects.create(name="editor")
+        Post.objects.create(title="foo", author=author, alt_editor=editor)
+        with self.assertRaises(IntegrityError):
+            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
+
+        # Multiple rows with a=NULL are considered different
+        TestNullableUniqueTogetherModel.objects.create(a=None, b='bbb', c='ccc')
+        TestNullableUniqueTogetherModel.objects.create(a=None, b='bbb', c='ccc')
+
+        # Uniqueness still enforced for non-NULL values
+        TestNullableUniqueTogetherModel.objects.create(a='aaa', b='bbb', c='ccc')
+        with self.assertRaises(IntegrityError):
+            TestNullableUniqueTogetherModel.objects.create(a='aaa', b='bbb', c='ccc')

testapp/tests/test_expressions.py

@@ -3,10 +3,9 @@
 from django import VERSION
 from django.db.models import IntegerField
 from django.db.models.expressions import Case, Exists, OuterRef, Subquery, Value, When
-from django.db.utils import IntegrityError
-from django.test import TestCase, skipUnlessDBFeature
+from django.test import TestCase
 
-from ..models import Author, Comment, Editor, Post
+from ..models import Author, Comment, Post
 
 DJANGO3 = VERSION[0] >= 3
 
@@ -52,16 +51,3 @@ def test_order_by_exists(self):
 
         authors_by_posts = Author.objects.order_by(Exists(Post.objects.filter(author=OuterRef('pk'))).asc())
         self.assertSequenceEqual(authors_by_posts, [author_without_posts, self.author])
-
-
-@skipUnlessDBFeature('supports_partially_nullable_unique_constraints')
-class TestPartiallyNullableUniqueTogether(TestCase):
-    def test_partially_nullable(self):
-        author = Author.objects.create(name="author")
-        Post.objects.create(title="foo", author=author)
-        Post.objects.create(title="foo", author=author)
-
-        editor = Editor.objects.create(name="editor")
-        Post.objects.create(title="foo", author=author, alt_editor=editor)
-        with self.assertRaises(IntegrityError):
-            Post.objects.create(title="foo", author=author, alt_editor=editor)