Opened 7 hours ago

Closed 4 hours ago

#36668 closed Bug (invalid)

ModelForm validation skips UniqueConstraint if its condition refers to a field not present on the form

Reported by: Baptiste Mispelon Owned by:
Component: Database layer (models, ORM) Version: 5.2
Severity: Normal Keywords: modelform, constraint, uniqueconstraint
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

(sorry for this mouthful of a title 😁)

For starters, I'm not sure if this is a bug or a feature request but it feels like a bug to me so I went with that. Similarly, I'm unsure if this is a ModelForm issue or a UniqueConstraint one and went with the latter.

Now for the actual issue, consider this code (see attached patch for an actual testcase [1] ):

from django import forms
from django.db import models


class Page(models.Model):
    pass


class Revision(models.Model):
    page = models.ForeignKey(Page, on_delete=models.CASCADE)
    status = models.IntegerField(default=1)

    class Meta:
        constraints = [
            models.UniqueConstraint(
                name="unique_page_status_1",
                fields=["page"],
                condition=models.Q(status=1),
            )
        ]


class RevisionForm(forms.ModelForm):
    class Meta:
        model = Revision
        fields = ["page"]


page = Page.objects.create()
page.revision_set.create()

form = RevisionForm(data={"page": page.pk})
# I would explain the form to be invalid because the revision that would be
# created by saving the form would fail against the constraint.
# But that's not what happens:
assert not form.is_valid()  # fails

(I've tested as far back as 4.1 and they all exhibit the same issue.)

I've investigated a little and here's what I think is happening: the ModelForm ends up calling UniqueConstraint.validate() and passing it an exclude list that contains status (because status is a field on the model, but not on the form). In turn, the logic in UniqueConstraint.validate() notices that status is present in its condition, and so decides to skip that validation.

I think that last step where UniqueConstraint.validate() skips fields used in the constraint's condition if they're listed in exclude is a bug. I think this behavior makes sense for CheckConstraint, but not for UniqueConstraint. In my mind UniqueConstraint.validate() should only check self.fields and ignore self.condition.

[1] Once the patch is applied with git apply ticket-xxx.diff you can run it with uv run --with-editable=. --with-requirements=tests/requirements/py3.txt python tests/runtests.py aaaaaa

Attachments (1)

ticket-36668-tests.diff (1.9 KB ) - added by Baptiste Mispelon 6 hours ago.

Download all attachments as: .zip

Change History (3)

by Baptiste Mispelon, 6 hours ago

Attachment: ticket-36668-tests.diff added

comment:1 by Simon Charette, 5 hours ago

This was discussed on the forum extensively so ​I'll drop a link here.

The TL;DR is by design as unique_together has always worked this way (if a unique constraint refers to any excluded fields then it's not validated) so we followed the same approach for Meta.constraints.

comment:2 by Jacob Walls, 4 hours ago

Resolution: β†’ invalid
Status: new β†’ closed

Happy to look at a documentation improvement here -- the closest thing I could find was (emphasis added):

You should only need to call a model’s full_clean() method if you plan to handle validation errors yourself, or if you have excluded fields from the ModelForm that require validation.

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