Opened 9 months ago

Closed 9 months ago

Last modified 9 months ago

#35367 closed Bug (wontfix)

Multi-field constraint are not correctly handled when a form has one field marked as read-only

Reported by: David Owned by: nobody
Component: Forms Version: 4.2
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When a model defines a constraint which relates two or more fields and one of those field is excluded from the related model form the validation provide a false positive and then the form tries to store an invalid record on the database, raising an IntegrityError.

Here is a code sample which reflects the situation

# models.py
from django.db import models
from django.forms import modelform_factory


class MyModel(models.Model):
    field_1 = models.IntegerField(null=True)
    field_2 = models.IntegerField()

    class Meta:
        constraints = [
            models.CheckConstraint(
                name="mycheck",
                check=models.Q(field_1__lt=models.F("field_2")) | models.Q(field_1__isnull=True),
            )
        ]  


# this is like it would be done in admin if 'field_1' is set a readonly
Form = modelform_factory(MyModel, fields=['field_2'], exclude=['field_1'])
# this is ok for validation: 10 < 20
instance = MyModel(field_1=10, field_2=20)
# this does not respect the constraint: 10 > 5
form_instance = Form({'field_2': 5}, instance=instance)
form.is_valid()
# > True
form.save()
# > IntegrityError

This is somehow similar and related to #12627

Change History (5)

comment:1 by David Sanders, 9 months ago

Resolution: wontfix
Status: newclosed

Form & model validation are separate concerns though & I'd argue this is desired behaviour. In fact the code explicitly leaves out the constraint validation because it includes field_1.

If you want the constraint to be respected then you'd need to add an additional line to validate the model instance in its entirety:

if form.is_valid():
    instance = form.save(commit=False)
    instance.full_clean()  # full_clean() within form.is_valid() excludes field_1
    instance.save()

Edit: Feel free to raise a discussion on the Django forum if you disagree: https://www.djangoproject.com/community/

Last edited 9 months ago by David Sanders (previous) (diff)

comment:2 by Sarah Boyce, 9 months ago

Hi David, to reiterate what David Sanders said, this is expected behaviour and documented. See: https://docs.djangoproject.com/en/5.0/ref/models/instances/#validating-objects
In particular:

When you use a ModelForm, the call to is_valid() will perform these validation steps for all the fields that are included on the form. See the ModelForm documentation for more information. 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.

You need to call full_clean() as you have excluded a field required for validation purposes: https://docs.djangoproject.com/en/5.0/ref/models/instances/#django.db.models.Model.validate_constraints

comment:3 by David, 9 months ago

I undestand that this may be a design choice, the part which worries me is that when the form creation is delegated to ModelAdmin this behaviour is masked.

I came across this problem with an admin like:

@admin.register(MyModel)
class MyModelAdmin(admin.ModelAdmin):
    readonly_fields = ('field_1',)

My expectation was to get warned before changing the other field that the constraint was going to be violated rather than getting an error.

Currently there is no warning in the admin documentation which refers to possible problems which may be caused by read only fields and data validation performed by the ORM:
https://docs.djangoproject.com/en/5.0/ref/contrib/admin/#django.contrib.admin.ModelAdmin.readonly_fields

comment:4 by David, 9 months ago

Maybe an other way to express what I belive is: if there is any constraint which involves more than one field it is not correct to allow excluding a subset of those fields from validation while keeping the others in the validation workflow.

With the provided code sample the validation of field_1 cannot be done without field_2 (and vice-versa).

in reply to:  4 comment:5 by Sarah Boyce, 9 months ago

I think #12627 covers your previous comment about readonly_fields in the admin.

Replying to David:

Maybe an other way to express what I belive is: if there is any constraint which involves more than one field it is not correct to allow excluding a subset of those fields from validation while keeping the others in the validation workflow.

With the provided code sample the validation of field_1 cannot be done without field_2 (and vice-versa).

If you want to discuss changing the design, you should propose this in the Django forum and see if you can get agreement from the community for this change. As this would impact a lot of existing projects, we need strong community agreement after which we can accept a ticket 🙂

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