[Django] #36350: Custom field's db_check() not taken into account when deciding whether to recreate constraint.

[Django]

#36350: Custom field's db_check() not taken into account when deciding whether to
recreate constraint.
-------------------------------------+-------------------------------------
Reporter: Baptiste | Owner: (none)
Mispelon |
Type: Bug | Status: assigned
Component: Database | Version: 5.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
This is a regression introduced in
9953c804a9375956a542da94665662d306dff48d.

I have a custom field that implements `db_check()` in order to add a
custom db constraint:
{{{#!python
class CharChoiceField(models.CharField):
"""
A custom CharField that automatically creates a db constraint to
guarante
that the stored value respects the field's `choices`.
"""
@property
def non_db_attrs(self):
# Remove `choices` from non_db_attrs so that migrations that only
change
# choices still trigger a db operation and drop/create the
constraint.
attrs = super().non_db_attrs
return tuple({*attrs} - {"choices"})

def db_check(self, connection):
constraint = CheckConstraint(
condition=Q(**{f"{self.name}__in": dict(self.choices)}),
name="", # doesn't matter, Django will reassign one anyway
)
with connection.schema_editor() as schema_editor:
return constraint.get_check_sql(self.model, schema_editor)
}}}

This used to work well (in Django < 5.0): the constraint was added
alongside the field when the initial migration was run, and any changes to
the field's `choices` would trigger a new migration (that's standard
Django behavior) which when executed would drop/create the constraint with
the new value of `choices`.

However the commit I linked to at the beginning broke this: now any
changes to `choices` still creates a migration, but executing the
migration is now a no-op (which can be confirmed by checking with
`sqlmigrate`).

I'm not sure if `Field.db_check()` can be considered public API (it's not
documented), but I have a fix which is fairly simple (PR incoming) so I
hope you'll consider it.
--
Ticket URL: <https://code.djangoproject.com/ticket/36350>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36350: Custom field's db_check() not taken into account when deciding whether to
recreate constraint.
-------------------------------------+-------------------------------------

Reporter: Baptiste Mispelon | Owner: (none)
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Baptiste Mispelon):

* has_patch: 0 => 1
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
* needs_tests: 0 => 1

Comment:

Draft PR (missing tests and release notes) is here:
https://github.com/django/django/pull/19412
--
Ticket URL: <https://code.djangoproject.com/ticket/36350#comment:1>

[Django]

#36350: Custom field's db_check() not taken into account when deciding whether to
recreate constraint.
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: (none)
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Balazs Endresz):

* cc: Balazs Endresz (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/36350#comment:2>

[Django]

#36350: Custom field's db_check() not taken into account when deciding whether to
recreate constraint.
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: (none)
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Natalia Bidart):

Thanks Baptiste for the details and PR! Could you please provide a failing
test on `main` that demonstrates the regression? While this won’t be a
release blocker since 5.0 is EOL, the test would still be valuable.
--
Ticket URL: <https://code.djangoproject.com/ticket/36350#comment:3>

[Django]

#36350: Custom field's db_check() not taken into account when deciding whether to
recreate constraint.
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: (none)
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Natalia Bidart):

* cc: Mariusz Felisiak, Simon Charette (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/36350#comment:4>

[Django]

#36350: Custom field's db_check() not taken into account when deciding whether to
recreate constraint.
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: (none)
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak):

Replying to [ticket:36350 Baptiste Mispelon]:

> I'm not sure if `Field.db_check()` can be considered public API (it's
not documented), but I have a fix which is fairly simple (PR incoming) so
I hope you'll consider it.

Unfortunately, the default implementation of `db_check()` already
incorporates the column name in generated SQL, so using it will
reintroduced issue fixed by 9953c804a9375956a542da94665662d306dff48d.
--
Ticket URL: <https://code.djangoproject.com/ticket/36350#comment:5>

[Django]

#36350: Custom field's db_check() not taken into account when deciding whether to
recreate constraint.
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: (none)
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Baptiste Mispelon):

* has_patch: 1 => 0
* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
* needs_tests: 1 => 0

Comment:

Ah right, I didn't realize that my fix was just doing the `%`-formatting
twice and therefore just turning off the optimization that commit
9953c804a9375956a542da94665662d306dff48d was adding (which explains why CI
was failing on MariaDB). Maybe the fix is not as simple as I first thought
(it was too good to be `True`!).

I'm attaching a patch that adds a failing test (targeting `main`). I tried
to keep it simple but unfortunately it's still fairly verbose (it's quite
hard to test this type of code).
--
Ticket URL: <https://code.djangoproject.com/ticket/36350#comment:6>

[Django]

#36350: Custom field's db_check() not taken into account when deciding whether to
recreate constraint.
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: (none)
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Baptiste Mispelon):

* Attachment "ticket_36350_test.patch" added.

--
Ticket URL: <https://code.djangoproject.com/ticket/36350>

[Django]

#36350: Custom field's db_check() not taken into account when deciding whether to
recreate constraint.
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: (none)
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* cc: Lily Foote, Carlton Gibson (added)

Comment:

It looks like there isn't a simple fix here and this is not officially
supported

I have found a forum discussion around wanting to be able to define a
constraint for a custom field: https://forum.djangoproject.com/t
/dynamically-populate-constraints-in-field-contribute-to-class/22934/5
A number of folks seem to be in favor of the change (will add them in cc),
so I think we could have a "New Feature" ticket to add support for this
--
Ticket URL: <https://code.djangoproject.com/ticket/36350#comment:7>

[Django]

#36350: Custom field's db_check() not taken into account when deciding whether to
recreate constraint.
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: (none)

Type: New feature | Status: assigned

Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Carlton Gibson):

* stage: Unreviewed => Accepted
* type: Bug => New feature

Comment:

I agree this looks like a new feature. I'll accept on that basis. (Happy
if someone wants to reclassify.)
--
Ticket URL: <https://code.djangoproject.com/ticket/36350#comment:8>

[Django]

#36350: Custom field's db_check() not taken into account when deciding whether to
recreate constraint.
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: (none)
Type: New feature | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Pravin):

HI Can I work on this ?
--
Ticket URL: <https://code.djangoproject.com/ticket/36350#comment:9>