From a6ba49779c52f0a1d5369257712eaf951faddf22 Mon Sep 17 00:00:00 2001 From: Tom Sparrow <793763+sparrowt@users.noreply.github.com> Date: Fri, 14 Jan 2022 10:48:01 +0000 Subject: [PATCH 1/3] Fix ambiguous issue number references from previous fork Add absolute links in place of #NN which are no longer valid in this repo. --- .../migrations/0002_test_unique_nullable_part1.py | 2 +- .../migrations/0003_test_unique_nullable_part2.py | 2 +- ...t1.py => 0004_test_unique_type_change_part1.py} | 2 +- ...t2.py => 0005_test_unique_type_change_part2.py} | 4 ++-- .../0006_test_remove_onetoone_field_part1.py | 2 +- .../migrations/0012_test_indexes_retained_part1.py | 2 +- .../migrations/0013_test_indexes_retained_part2.py | 3 ++- testapp/models.py | 14 ++++++++------ testapp/tests/test_constraints.py | 12 +++++++----- testapp/tests/test_indexes.py | 9 +++++---- 10 files changed, 29 insertions(+), 23 deletions(-) rename testapp/migrations/{0004_test_issue45_unique_type_change_part1.py => 0004_test_unique_type_change_part1.py} (91%) rename testapp/migrations/{0005_test_issue45_unique_type_change_part2.py => 0005_test_unique_type_change_part2.py} (89%) diff --git a/testapp/migrations/0002_test_unique_nullable_part1.py b/testapp/migrations/0002_test_unique_nullable_part1.py index 33ab86a6..911a5952 100644 --- 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 index ade35429..14280266 100644 --- 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_issue45_unique_type_change_part1.py b/testapp/migrations/0004_test_unique_type_change_part1.py similarity index 91% rename from testapp/migrations/0004_test_issue45_unique_type_change_part1.py rename to testapp/migrations/0004_test_unique_type_change_part1.py index 2f3b9fba..cb820669 100644 --- a/testapp/migrations/0004_test_issue45_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_issue45_unique_type_change_part2.py b/testapp/migrations/0005_test_unique_type_change_part2.py similarity index 89% rename from testapp/migrations/0005_test_issue45_unique_type_change_part2.py rename to testapp/migrations/0005_test_unique_type_change_part2.py index a938fe2a..47f48d49 100644 --- a/testapp/migrations/0005_test_issue45_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 index e7e61473..f3aaed73 100644 --- 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 index 13c34299..288e3dff 100644 --- 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 index 7d844c61..1fb53039 100644 --- 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/models.py b/testapp/models.py index 52890ea5..9ac0731b 100644 --- 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,7 +63,7 @@ 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) @@ -71,14 +71,16 @@ class Meta: 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 diff --git a/testapp/tests/test_constraints.py b/testapp/tests/test_constraints.py index 1030e9f8..ab6a094c 100644 --- a/testapp/tests/test_constraints.py +++ b/testapp/tests/test_constraints.py @@ -19,9 +19,10 @@ @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 +51,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') diff --git a/testapp/tests/test_indexes.py b/testapp/tests/test_indexes.py index 5639ae16..1d7a8f83 100644 --- 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'}) From d6d1e4a27984eba0a321614be1c16a9a832a9fc7 Mon Sep 17 00:00:00 2001 From: Tom Sparrow <793763+sparrowt@users.noreply.github.com> Date: Mon, 17 Jan 2022 09:39:53 +0000 Subject: [PATCH 2/3] TDD: test which fails due to mssql-django/issues/86 --- .../0014_test_rename_m2mfield_part1.py | 26 +++++++++++++++++++ .../0015_test_rename_m2mfield_part2.py | 19 ++++++++++++++ testapp/models.py | 9 +++++++ testapp/tests/test_constraints.py | 17 ++++++++++++ 4 files changed, 71 insertions(+) create mode 100644 testapp/migrations/0014_test_rename_m2mfield_part1.py create mode 100644 testapp/migrations/0015_test_rename_m2mfield_part2.py diff --git a/testapp/migrations/0014_test_rename_m2mfield_part1.py b/testapp/migrations/0014_test_rename_m2mfield_part1.py new file mode 100644 index 00000000..ca2cf969 --- /dev/null +++ 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 new file mode 100644 index 00000000..cfc60bb6 --- /dev/null +++ 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 index 9ac0731b..cec74519 100644 --- a/testapp/models.py +++ b/testapp/models.py @@ -89,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 index ab6a094c..dca4e456 100644 --- a/testapp/tests/test_constraints.py +++ b/testapp/tests/test_constraints.py @@ -10,9 +10,11 @@ from ..models import ( Author, Editor, + M2MOtherModel, Post, TestUniqueNullableModel, TestNullableUniqueTogetherModel, + TestRenameManyToManyFieldModel, ) @@ -65,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 From 3c8b0b168cb0b8e7adc6d22ff737a2f8a8ffbc60 Mon Sep 17 00:00:00 2001 From: Tom Sparrow <793763+sparrowt@users.noreply.github.com> Date: Mon, 24 Jan 2022 14:46:45 +0000 Subject: [PATCH 3/3] Don't drop unique indexes before M2M rename This method override was added by: https://github.com/ESSolutions/django-mssql-backend/pull/24/commits/08c2721a681062eccbb0c2cd33a9cf5661ec92f7 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. --- mssql/schema.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/mssql/schema.py b/mssql/schema.py index 8a28cca3..02f937ce 100644 --- 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):