Fixed #20581 -- Added support for deferrable unique constraints.

[LilyFoote]

This builds upon #10271, extending the UniqueConstraint class to support deferring constraint checks.
https://code.djangoproject.com/ticket/20581

[auvipy]

is it ready for review?

[LilyFoote]

@auvipy Yes.

[LilyFoote]

@auvipy I've synced this with master. I've also fixed a bug in set_constraints, clarified the interaction between condition and defer with a ValueError, added Oracle support and added a first draft of documentation. I'd appreciate it if you could review again.

[auvipy]

OK I will

[LilyFoote]

I haven't tested the Oracle support, because I don't have Oracle configured locally. I think it should work, from my reading of the docs, but it's possible something isn't quite right.

[auvipy]

looks great overall.

[auvipy]

looks great to merge.

[LilyFoote]

@carltongibson @felixxm I'd like to land this today if possible. I think the main thing it needs is release notes. Is there anything else I need to do?

[felixxm]

IMO it should work on Oracle, can you enable supports_deferrable_unique_constraints? Do you know how it looks like on MySQL?

[LilyFoote]

As far as I can tell, MySQL and MariaDB do not support deferrable constraints. I've added a small release note and enabled this for Oracle.

[felixxm]

@Ian-Foote Thanks 👍 We have some failures on Oracle (with proposed changes):

• test_add_deferrable_unique_constraint (migrations.test_operations.OperationTests),
• test_create_model_with_deferrable_unique_constraint (migrations.test_operations.OperationTests),
• test_remove_deferrable_unique_constraint (migrations.test_operations.OperationTests), and
  raise

django.db.utils.IntegrityError: ORA-02091: transaction rolled back
ORA-00001: unique constraint (DJANGOTEST.DEFERRABLE_PINK_CONSTRAINT) violated

Also I think we should add a system checks for databases that don't support defferable, like we added for condition (ticket-31351).

[felixxm]

@Ian-Foote Thanks for updates 👍 I pushed small edits and:

• added deferable to the UniqueConstraint.__repr__() (with tests),
• added deferable to the UniqueConstraint.__eq__() (with tests),
• avoided creating/removing deferable unique constraints if not supported.

I'm not convinced that we need Deferrable enum, it's an advance flag (like index_type in ExclusionConstraint) and IMO we can handle it in the same way, as a string. If you agree I can push required changes.

I'm still fighting with migrations.test_operations 👎

Please don't use force-push if you will do any changes, it's harder to review such changes.

[ngnpope]

+1 on enum removal. For the simple case of two single-word strings we end up with an extra object to import...

[meshy]

I think it's better to keep the enum. It avoids the possibility of a typo. There is precident with things like passing on_delete=CASCADE to ForeignKeys.

Edit: or, if not an enum, at least an importable constant would be good.

[LilyFoote]

I originally used importable strings instead of the Enum and changed it in response to #10338 (comment). See also this discussion: #10338 (comment).

At this point, I prefer the Enum over the other two options:

• Importable string constants
• Two boolean flags

[ngnpope]

@meshy django.db.models.deletion.CASCADE & co. are not enums, but functions that actually define the logic for how to handle the deletion behaviour. Note that they are executed application-side and not enforced on the database - see #8661 for further work in that area - so they are not strings that are passed down to the database.

It avoids the possibility of a typo.

That can be easily be handled by raising a ValueError if a valid string is not provided in the same way that is done for Deferrable currently:

-        if deferrable is not None and not isinstance(deferrable, Deferrable):
+        if deferrable is not None and deferrable not in {'deferred', 'immediate'}:

Removing the enum avoids having to import it in various places, including an inline import in the _deferrable_constraint_sql() method.

Edit: or, if not an enum, at least an importable constant would be good.

That would be worse than an enum - you'd now need two constants and it wouldn't prevent use of an arbitrary string anyway as we would still have to validate the values as there would be no type checking.

Whether an enum or multiple constants, this also requires additional documentation of these objects/constants rather than just stating the two accepted string values for the deferrable argument in the documentation.

[ngnpope]

@Ian-Foote Sorry - we posted simultaneously.

At this point, I prefer the Enum over the other two options:

As pointed out by @felixxm, there is the third option is to just use the string values 'deferred' and 'immediate' and validate them in the constructor. Constants for the string values doesn't provide any guarantees that they will be used appropriately - you only need to look at f1664a2, e7033e0, and 1e0dcd6 - you still need to check the value is valid.

Don't get me wrong - enums do have benefits, I'm just not sure that it is worth it in this instance when there are only two possible values, and when that means importing Deferrable all over the place.

Anyway, that's just my 2¢ and I'll leave it at that. I see that @charettes was for the enum and am happy to go with his wisdom.

[LilyFoote]

Good point that I misunderstood @felixxm's suggestion. I think I still prefer the Enum slightly, but I don't feel as strongly as compared to the other two options.

[charettes]

I don't have a strong feeling either way, using an Enum felt more natural to me. I'd say that ExclusionConstraint.index_type is a bit different to me because PostgreSQL allows a ton different index type to used and it's even possible to define your own index type in extensions. It's not possible to do the same thing with constraint defer-ability and if we ever implement ForeignKeyConstraint we'll be able to reuse this constant.

[LilyFoote]

I already reuse Deferrable in ExclusionConstraint: #12704

[felixxm]

OK, let's stay with Enum, I will continue tomorrow.

[felixxm]

@Ian-Foote Many thanks for your hard work and patience 🚀 It took several DjangoCon's 😉

I pushed final edits and fixed tests on Oracle.

[felixxm]

oragis failure is not related with this patch, it's caused by recent changes in Jenkins configuration.

[charettes]

Congrats @Ian-Foote and thank you for your continued efforts 🙇 🎉 !