Nullable unique constraints not reinstated correctly

[sparrowt]

Problem

Unique constraints which are now implemented using a filtered UNIQUE INDEX (to get desired behaviour for multiple NULLs) are not correctly reinstated after an AlterField e.g. if the column type is changed.

This happens in at least these cases:

1. after changing the type of a field which has null=True and unique=True
2. after changing the type of a field involved in a unique_together

The constraint is reinstated using ALTER TABLE ... ADD CONSTRAINT instead of CREATE UNIQUE INDEX ... WHERE x is NOT NULL

This means the unique constraint is no longer getting the ANSI-compliant NULL behaviour which was recently introduced by PR #1 (for single columns) and PR #24 (for unique_together) to allow for multiple NULL values.

Symptoms

If there are already multiple NULL values in the table, the migration will fail because reinstating the over-strict constraint fails with an error like this:

django.db.utils.IntegrityError: ('23000', "[23000] [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]The CREATE UNIQUE INDEX statement terminated because a duplicate key was found for the object name 'dbo.myapp_foomodel' and the index name 'myapp_foomodel_field_a_d7c02f64_uniq'. The duplicate key value is (<NULL>). (1505) (SQLExecDirectW); [23000] [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Could not create constraint or index. See previous errors. (1750); [23000] [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]The statement has been terminated. (3621)")

(note: despite the error message saying CREATE UNIQUE INDEX this error is thrown during execution of ALTER TABLE ... ADD CONSTRAINT - presumably because internally in MSSQL this results in running CREATE UNIQUE INDEX without any conditions/filter)

Or if there aren't multiple NULLs then the migration may 'succeed' but:

• have the wrong runtime behaviour with multiple NULLs
• potentially fail on a future migration step - for example in the 2nd case with unique_together, if in future an AlterUniqueTogether tries to add/remove a field, it will first try to delete the existing constraint as if it were a UNIQUE INDEX (which it should be) but actually it's a CONSTRAINT so the later migration will fail like this:

django.db.utils.ProgrammingError: ('42000', "[42000] [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]An explicit DROP INDEX is not allowed on index 'bar_baz_deadbeef_uniq'. It is being used for UNIQUE KEY constraint enforcement. (3723) (SQLExecDirectW)")

Reproduction steps

I can reproduce this in a minimal Django project with using:

Django==2.2.10
django-mssql-backend==2.8.0
pyodbc==4.0.30
pytz==2019.3
sqlparse==0.3.0

1. Add the following model and run makemigrations and then migrate

class FooModel(models.Model):
    field_a = models.CharField(max_length=10, null=True, unique=True)

2. Create 2 rows which both have field_a as NULL
3. Edit max_length to say 15, makemigrations again and run migrate
4. This explodes with an error as in the first example above

To repro the 2nd example:

1. Add field_x = models.CharField(max_length=10) which is in a unique_together with at least 1 other field e.g. field_y
2. Then do AlterField on field_x to change it to max_length=15 (this will succeed if there are no rows in the table)
3. Then do AlterUniqueTogether to add a 3rd field field_z to the unique_together
4. This explodes with an error like the second example above

Cause

As far as I can tell the issue is that #24 changed the way such unique indexes were implemented, but did not ensure that _alter_field reinstates them in the same way, after they are temporarily dropped in order to make changes to the field.

Proposed solution

In _alter_field under # Restore an index, SQL Server requires explicit restoration, the handling of unique=True/unique_together needs to reinstate filtered UNIQUE INDEX rather than a CONSTRAINT in the relevant cases.

I am working on a PR for this.

[sparrowt]

Proposed fix here: #47

[zxj17815]

I had this problem when I updated or saved

('23000', '[23000] [Microsoft] [ODBC Driver 13 for SQL Server] [SQL Server] violated the PRIMARY KEY constraint "KZ98_CGJY_PR". You cannot insert duplicate keys in the object "dbo. @ Z98_CGJY". Duplicate key values Is (4742). (2627) (SQLExecDirectW) ')

[CCattermoul]

I don't think that they are initially created correctly either, on an alter field operation. If I alter a OneToOneField to be Nullable multiple Nulls are still not allowed, including with the proposed fix above. New additions that are initially Nullable are ok.

This is easily re-creatable by altering an existing non-null, unique, field. I found it whilst upgrading Django-oauth-toolkit from 1.0.0 to 1.2.0, where a null access_token is now allowed on refresh_token, as part of switch to Django 2.2.

It looks like the original UNIQUE constraint is not removed when the replacement UNIQUE INDEX is created, if using the alter field operation to get to unique & null.

[CCattermoul]

Apologies if these should be separate issues but they seem related. Un-applying an AddField migration operation with unique & nullable also fails - it tries to remove the name of the UNIQUE INDEX from the constraints, failing the migration reverse. The issue is not actually with the migration reverse. Further tests show the same behaviour on removal of unique & nullable field when forward migrating.

This real world migration dot_1_1 demonstrates the issues both here (reversing the source_refresh_token addition) and my previous post (altering the access_token field to accept null) Note I have already patched dot for issue 41, and other custom migrations for unique/null etc in our main app. It will be great if we don't need these in the future now all these issues are being addressed. Thanks!

Update: If you try to reverse the migration a second time, after the initial failure, it succeeds.

[sparrowt]

@CCattermoul I haven't quite understood your issue, but although somewhat related it sounds different to the precise issue described (and fixed) in this issue, so it would be best if you file a separate issue with a clear and minimal reproduction.