Raise error on trying to create unique constraints with unsupported conditions

[davidjb]

This was originally raised at wagtail/wagtail#6393 -- Wagtail currently has a UniqueConstraint with a condition that isn't supported within SQL Server databases, being an OR condition where SQL Server only supports AND (see the create index filter_predicate syntax).

This PR handles a situation where an unsupportable UniqueConstraint is trying to be added to the database and, following other parts of the schema editor class, raises an error. Firstly, it would be helpful to know whether crashing out is the best strategy (compared to just skipping/ignoring & logging a warning for the unsupportable constraint).

Likewise, being new and unfamiliar with Django's database and migration internals, I would like to add tests to this PR, but I'm unsure how one tests an unsupportable migration (especially one that only applies to when the database backend is SQL Server) - as adding it normally to the testapp migrations would break migrations running normally against both sqlite and SQL Server.

So this PR is more of an discussion at this point until someone feels it's able to be merged. Let me know your thoughts, thanks!

[aceofwings]

Django comes with several test helpers. I would start by creating a Migration class such as the one below with your constrain defintions. Maybe this will help and you can edit this to fit your needs.

from django.db import migrations, models
from django.test.testcases import TransactionTestCase
from django.db.migrations.state import  ProjectState
from django.db.migrations.migration import Migration
from django.db import (
    IntegrityError, connection, migrations, models, transaction,
)
class Migration(migrations.Migration):
    initial = True
    operations = [
        migrations.CreateModel(
            name='UUIDModel',
            fields=[
                ('id', models.UUIDField(default=uuid.uuid4, editable=False, primary_key=True, serialize=False)),
                ('other',  models.UUIDField(null=True)),
            ],
        ),
        migrations.AlterField(
            model_name='UUIDModel',
            name='other',
            field=models.UUIDField(null=False,default=uuid.uuid4),
        ),
    ]

Test Case as Follows

class TestOperations(TransactionTestCase):
    def test_alter_uuid_field(self):
        app_label = "test_alter_uuid_field"
        migration = Migration('name', app_label)
        with connection.schema_editor(atomic=True) as editor:
            return migration.apply(ProjectState(), editor)

[davidjb]

Thank you @aceofwings - very detailed and super helpful! From poking around in Django's source, I was actually pretty close but wasn't using the common imports you highlighted or using the appropriate TransactionTestCase class. I've pushed tests as part of the PR now.

So the remaining question is whether raising an exception is the best strategy. By doing so, this explicitly requires incompatible code to be rewritten to support this database backend but at the same time prevents an incompatible index from being present in the code at all (such as where multiple database engines are being supported by a common library or where an index can't otherwise be rewritten). Skipping/ignoring & logging a warning for the unsupportable constraint would be most compatible, allowing the schema's creation, but might affect an application if it's not prepared for the index not to exist.

Thoughts?

[aceofwings]

Maybe raising the error would be the most appropriate. I would rather have a migration fail than to continue to with the assumption that the schema exists and the migration applied. If you do go with the silent/log route it may be worth while to expand the warning on application start up. Decision is really yours at the end of the day.

[davidjb]

Thanks @aceofwings - I really appreciate it. On balance, I agree this seems the best strategy, particularly when future migrations and code will be created in Django projects on the assumption that a constraint exists (e.g. dropping or modification -- how to handle that if the constraint had never been created?).

In that case, my PR is done, tests added & CI passes for all environments. Let me know if there's anything additional to be added.

[aceofwings]

Have you head back from Microsoft on there fork?

[davidjb]

All quiet over at microsoft/mssql-django#9 - a few positive reactions but no reply at this stage. They’ve responded to other issues I’ve opened so will keep on following up.

[sparrowt]

Seems to be in now, so I guess this can be closed?