diff --git a/sql_server/pyodbc/schema.py b/sql_server/pyodbc/schema.py index 2bd2dcc5..cf75b27b 100644 --- a/sql_server/pyodbc/schema.py +++ b/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]) diff --git a/testapp/migrations/0002_test_unique_nullable_part1.py b/testapp/migrations/0002_test_unique_nullable_part1.py index 1c0e48d2..33ab86a6 100644 --- a/testapp/migrations/0002_test_unique_nullable_part1.py +++ b/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', diff --git a/testapp/migrations/0003_test_unique_nullable_part2.py b/testapp/migrations/0003_test_unique_nullable_part2.py index d6fc61e0..ade35429 100644 --- a/testapp/migrations/0003_test_unique_nullable_part2.py +++ b/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', diff --git a/testapp/migrations/0004_test_issue45_unique_type_change_part1.py b/testapp/migrations/0004_test_issue45_unique_type_change_part1.py new file mode 100644 index 00000000..2f3b9fba --- /dev/null +++ b/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')}, + }, + ), + ] diff --git a/testapp/migrations/0005_test_issue45_unique_type_change_part2.py b/testapp/migrations/0005_test_issue45_unique_type_change_part2.py new file mode 100644 index 00000000..a938fe2a --- /dev/null +++ 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')}, + ), + ] diff --git a/testapp/models.py b/testapp/models.py index 503d81ce..7b993165 100644 --- a/testapp/models.py +++ b/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) diff --git a/testapp/tests/test_constraints.py b/testapp/tests/test_constraints.py new file mode 100644 index 00000000..523a2c85 --- /dev/null +++ b/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') diff --git a/testapp/tests/test_expressions.py b/testapp/tests/test_expressions.py index 720c542b..90623753 100644 --- a/testapp/tests/test_expressions.py +++ b/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)