#34166 closed New feature (wontfix)
UniqueConstraint with condition not validated if the condition field is not editable.
Reported by: | Márton Salomváry | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 4.1 |
Severity: | Normal | Keywords: | model validation unique constraint condition |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Given a model with a UniqueConstraint that has a condition with a field that is not editable
when I try to save this model with an (admin) form with an invalid value for the UniqueConstraint
I expect to get a (Django) validation error.
What I get instead is a database-level integrity error.
If the condition field is editable, I do get a validation error (although a form level one, not a field level one, which might be another bug).
If there is no condition on the field, I do get the expected field level validation error.
Example:
class Thing(models.Model): name = models.CharField(max_length=10) status = models.CharField(max_length=10, blank=True, editable=False) class Meta: constraints = [ models.UniqueConstraint( fields=["name"], name="thing_unique_name", condition=~Q(status="archived"), ) ]
Small test case repo: https://github.com/salomvary/django_partial_constraint_bug
(To reproduce, go to http://127.0.0.1:8000/admin/thing/thing/add/ and try to add two Things with the same name.)
I've tried the latest dev version of Django, which has same problem.
Attachments (2)
Change History (6)
by , 2 years ago
Attachment: | Screen Shot 2022-11-16 at 16.16.17.png added |
---|
comment:1 by , 2 years ago
The problem seems to be that the non-editable field is passed down as part of excludes
, which is right, we do not want to validate the value of the non-editable field.
However this is not about validating the value of the non-editable field, but conditionally validating the value of a *different* field, if the non-editable field's value matches the condition.
The documentation on the semantics of excludes
seems to confirm this:
https://docs.djangoproject.com/en/4.1/ref/models/instances/#django.db.models.Model.full_clean
"ModelForm uses this argument to exclude fields that aren’t present on your form from being validated since any errors raised could not be corrected by the user."
I'm a little confused however because I found a test that seems to specifically verify this (but there is no explanation for the "why" or "for what purpose").
https://github.com/django/django/blob/e14d08cd894e9d91cb5d9f44ba7532c1a223f458/tests/constraints/tests.py#L698-L703
I've also done a bit of Git archeology and found this conversation around the same problem which ends with "That's a suitable workaround, but I feel like it should not be necessary." https://github.com/django/django/pull/14625#pullrequestreview-1074071130
Would a patch be accepted that makes the workaround unnecessary? Something like this? https://github.com/django/django/compare/main...salomvary:django:ticket_34166?expand=1
follow-up: 4 comment:2 by , 2 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Summary: | UniqueConstraint with condition not validated if the condition field is not editable → UniqueConstraint with condition not validated if the condition field is not editable. |
Type: | Bug → New feature |
Thanks for the ticket. This is the documented behavior for forms and it was discussed shortly in the original PR (as you've already noticed).
Unfortunately, changing this would be backward incompatible so please first start a discussion on the DevelopersMailingList, where you'll reach a wider audience and see what other think, and follow the guidelines with regards to requesting features.
comment:3 by , 2 years ago
For your project, defining model form subclass for ModelAdmin.get_form implementing clean validating the non-editable field would be the way to go.
comment:4 by , 2 years ago
Replying to Mariusz Felisiak:
Thanks for the ticket. This is the documented behavior for forms and it was discussed shortly in the original PR (as you've already noticed).
Unfortunately, changing this would be backward incompatible so please first start a discussion on the DevelopersMailingList, where you'll reach a wider audience and see what other think, and follow the guidelines with regards to requesting features.
I understand, but I'm not sure I have the courage and time to try to sell a breaking change :)
Replying to Carlton Gibson:
For your project, defining model form subclass for ModelAdmin.get_form implementing clean validating the non-editable field would be the way to go.
I ended up adding this to the model as a simple workaround (and a handcrafted violation_error_message
on the UniqueConstraint
):
def clean(self): conditional_constraints = {"thing_unique_name"} for constraint in self._meta.constraints: if constraint.name in conditional_constraints: constraint.validate(self.__class__, self)
Expected error