Opened 18 months ago

Closed 18 months ago

Last modified 17 months ago

#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)

Screen Shot 2022-11-16 at 16.16.17.png (38.7 KB ) - added by Márton Salomváry 18 months ago.
Expected error
Screen Shot 2022-11-16 at 16.18.53.png (87.0 KB ) - added by Márton Salomváry 18 months ago.
Actual error

Download all attachments as: .zip

Change History (6)

by Márton Salomváry, 18 months ago

Expected error

by Márton Salomváry, 18 months ago

Actual error

comment:1 by Márton Salomváry, 18 months 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

Last edited 18 months ago by Márton Salomváry (previous) (diff)

comment:2 by Mariusz Felisiak, 18 months ago

Resolution: wontfix
Status: newclosed
Summary: UniqueConstraint with condition not validated if the condition field is not editableUniqueConstraint with condition not validated if the condition field is not editable.
Type: BugNew 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 Carlton Gibson, 17 months 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.

in reply to:  2 comment:4 by Márton Salomváry, 17 months 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)

Note: See TracTickets for help on using tickets.
Back to Top