#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 , 9 months ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
comment:2 by , 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 , 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
follow-up: 5 comment:4 by , 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).
comment:5 by , 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 withoutfield_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 🙂
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:
Edit: Feel free to raise a discussion on the Django forum if you disagree: https://www.djangoproject.com/community/