Move exclusion constraints to Meta.constraints.

[felixxm]

No description provided.

[felixxm]

DDL statement generated by the ORM:

ALTER TABLE "subscription_package"
ADD CONSTRAINT "subscription_package_prevent_overlaps"
EXCLUDE USING GIST (
    "subscription_id" WITH =,
    (TSTZRANGE("active_since", "active_until", '[)')) WITH &&
)
WHERE ("active_since" IS NOT NULL)

[felixxm]

DDL statement generated by the ORM:

ALTER TABLE "subscription_subscription"
ADD CONSTRAINT "subscription_subscription_prevent_overlaps"
EXCLUDE USING GIST (
     "account_id" WITH =,
     (TSTZRANGE("active_since", "active_until", '[)')) WITH &&
)
WHERE ("active_since" IS NOT NULL)

[lukasgraf]

Hey @felixxm, thanks for your contribution!

While we appreciate your contributions, please take a moment to check https://github.com/opengisch/QFieldCloud#before-considering-a-new-pr

When touching critical parts of the codebase, like migrations, maybe it's better to first talk to us about what change you have in mind, and what motivates it.

Retroactively changing migrations in Django can be quite dangerous, and in my opinion should only be done if there is a pressing need (if a migration is actually broken, and will lead to problems during deployment unless fixed).

Because if a historic migration that has already been executed on some deployments, but not others, is changed in a way that doesn't result in exactly the same database state in all conceivable circumstances, then you end up with diverging database state between deployments. Depending on which exact migration path they followed, and which version of that particular migration they executed.

So it seems to me that your change to the migration accomplishes this (and the explicit comments with the generated DDL definitely help to confirm that, thanks for those!), but I still would question whether this change is worth the risk.

Yes, it cleans up some use of raw SQL, but at the risk of causing a major headache in production. For a change like that, I would want to see more than "No description provided." as to what motivates it, and what risks / benefits there are to it.

But please don't be discouraged by my feedback! It's just that I would recommend going for changes first that are less critical, until we've had some more time to work together 😉

[felixxm]

Yes, it cleans up some use of raw SQL, but at the risk of causing a major headache in production. For a change like that, I would want to see more than "No description provided." as to what motivates it, and what risks / benefits there are to it.

The main motivation is to reduce the number of raw SQL queries to maintain. My proposal changes migrations "in-place", so it's a no-op change if migration is already deployed, when it's not, the new version is semantically identical to the previous one. Also, ExclusionConstraint() class was added in Django 3.1 so it's well established.

Another thing that we could benefit from is validation of constraints during model validation (Django 4.1+).

I don't see any risk in this change, but, of course the final decision is yours. It's only a suggestion.