Opened 3 weeks ago
Last modified 12 days ago
#37244 assigned Bug
Interaction between postgres ArrayField model field and ModelForm results in incomplete validation error messages
| Reported by: | Chris | Owned by: | farthestmage |
|---|---|---|---|
| Component: | contrib.postgres | Version: | 5.2 |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
The contrib.postgres model ArrayField uses the validator(s) of the base (model) field of the ArrayField to validate the items in the array. It catches the ValidationError exceptions, then prefixes them with its own prefix ("Item <x> in the array did not validate:") and re-raises this exception with the `"item_invalid"` code.
BaseModelForm collects these model validation errors in the _post_clean() method, and passes them to _update_errors(). This last method replaces the message of a validation error if the code of that validation error also is in the form field's error_messages dict, before adding it to the form errors.
The problem here is of course that the helpful message from the model (i.e. "Item <x> in the array did not validate: <reason from base field validator>") gets replaced by only the prefix: "Item <x> in the array did not validate:", and this is what is then rendered as the error in the form, with the entire reason for the validation failure no longer shown.
This is not usually a problem if the model field also has a corresponding custom form field which has the same validators, because these are run first and should catch the same problems before the model validators are tried, and these are not subject to the same replace-validation-message-from-model-with-the-one-from-the-form mechanics.
Change History (7)
comment:1 by , 3 weeks ago
comment:2 by , 3 weeks ago
Reproduced on Django 6.0.7. No Postgres server needed for this one, psycopg just has to be importable since nothing here touches the DB.
import django from django.conf import settings settings.configure( DATABASES={"default": {"ENGINE": "django.db.backends.sqlite3", "NAME": ":memory:"}}, INSTALLED_APPS=["django.contrib.contenttypes", "django.contrib.auth"], USE_TZ=True, ) django.setup() from django import forms from django.contrib.postgres.fields import ArrayField from django.core.validators import RegexValidator from django.db import models class Tag(models.Model): names = ArrayField(models.CharField( max_length=50, validators=[RegexValidator(r"^[a-z]+$", "Only lowercase letters are allowed.")], )) class Meta: app_label = "contenttypes" class TagForm(forms.ModelForm): class Meta: model = Tag fields = ["names"] try: Tag(names=["abc", "DEF"]).full_clean() except Exception as exc: print("model:", exc.message_dict["names"][0]) form = TagForm(data={"names": "abc,DEF"}) form.is_valid() print("form: ", form.errors.as_data()["names"][0].messages[0])
Output:
model: Item 2 in the array did not validate: Only lowercase letters are allowed. form: Item 2 in the array did not validate:
The trigger is a validator on the model's base_field. ArrayField.formfield() builds the form's base field with self.base_field.formfield(), which doesn't pass validators through:
model base_field validators: [RegexValidator, MaxLengthValidator] form base_field validators: [MaxLengthValidator, ProhibitNullCharactersValidator]
So the RegexValidator only runs during model validation, and that's the path _update_errors() rewrites. max_length does get propagated to the form field and gets caught earlier, which is probably why this doesn't come up often.
The error arriving at the form carries code='item_invalid' and params={'value': 'DEF', 'nth': 2}, so it hits the message.code in error_messages branch in BaseModelForm._update_errors(). SimpleArrayField.default_error_messages['item_invalid'] is just the prefix, so it replaces the model's full message.
comment:3 by , 3 weeks ago
| Component: | Forms → contrib.postgres |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
Replicated thank you! Example Django test below:
-
tests/postgres_tests/test_array.py
a b from django.contrib.admin.utils import display_for_field 9 9 from django.core import checks, exceptions, serializers, validators 10 10 from django.core.exceptions import FieldError 11 11 from django.core.management import call_command 12 from django.core.validators import MaxValueValidator 12 13 from django.db import IntegrityError, connection, models 13 14 from django.db.models import JSONNull 14 15 from django.db.models.expressions import Exists, F, OuterRef, RawSQL, Value … … class TestStringSerialization(PostgreSQLSimpleTestCase): 1069 1070 1070 1071 1071 1072 class TestValidation(PostgreSQLSimpleTestCase): 1073 def test_modelform_base_field_validator_error_message(self): 1074 class MyModel(PostgreSQLModel): 1075 field = ArrayField(models.IntegerField(validators=[MaxValueValidator(10)])) 1076 1077 class Form(forms.ModelForm): 1078 class Meta: 1079 model = MyModel 1080 fields = ("field",) 1081 1082 form = Form({"field": ["51", "1"]}) 1083 self.assertFalse(form.is_valid()) 1084 self.assertEqual( 1085 form.errors, 1086 { 1087 "array": [ 1088 "Item 1 in the array did not validate: " 1089 "Ensure this value is less than or equal to 10." 1090 ] 1091 }, 1092 ) 1093 1072 1094 def test_unbounded(self):
comment:4 by , 3 weeks ago
Diagnosis, if it helps confirm the direction above:
The composed message is correct when it leaves ArrayField.run_validators() —
prefix_validation_error() glues the prefix and the validator's own message
together and tags the result code="item_invalid". It is destroyed afterwards,
in BaseModelForm._update_errors():
for message in messages: if isinstance(message, ValidationError) and message.code in error_messages: message.message = error_messages[message.code]
SimpleArrayField.default_error_messages has an item_invalid key, so the code
matches and the composed message is replaced by the form field's value — which is
only the prefix. The override itself is intentional (form-level wording beats
model-level wording); it misfires here because item_invalid is a sentence
fragment meant for concatenation, not a standalone message.
This only bites when the failing validator is absent from the form's base field,
because form-field errors never pass through _update_errors(). That is the case
today: ArrayField.formfield() calls self.base_field.formfield() with no
arguments, so nothing is propagated.
Proposed fix, django/contrib/postgres/fields/array.py line 237:
"base_field": self.base_field.formfield( validators=self.base_field._validators ),
The error is then raised during SimpleArrayField.run_validators(), prefixed
correctly, and reaches add_error() without going near _update_errors().
Two notes on the details:
- It has to be
_validators(the explicitly-declared list), not thevalidatorscached_property. The property also carries validators Django adds automatically, which the corresponding form field re-adds on its own —CharFieldwould getMaxLengthValidatortwice,EmailFieldEmailValidatorandMaxLengthValidatortwice,DecimalFieldDecimalValidatortwice. Users would see the same error reported twice._validatorshas no overlap with what the form field builds for itself.
- When the base field has
choices,Field.formfield()drops any kwarg outside its whitelist,validatorsincluded, so the fix does not reach that case. No regression (TypedChoiceFieldenforces the choices), but the truncation would remain for a custom validator on a base field with choices.
One visible behaviour change worth a release note: the model-side loop raises on
the first bad item, whereas SimpleArrayField collects all of them. A form with
two invalid items currently shows one truncated error and would show two complete
ones.
This does not address the underlying prefix/code collision in _update_errors(),
which would still truncate an item_invalid error raised from a custom model-level
Field.validate(). That seems like a larger change to core forms code and probably
belongs in a separate ticket.
No test in tests/ currently declares validators on an ArrayField base field, so
_validators is empty throughout the existing suite and the change is a no-op there.
Yassin — you asked first; happy to leave this to you. If you have not started, I am
glad to put a PR together with Sarah's test case (note the expected dict key should
be the field name rather than "array").
comment:5 by , 3 weeks ago
| Owner: | set to |
|---|---|
| Status: | new → assigned |
comment:7 by , 12 days ago
| Has patch: | set |
|---|
If this gets triaged and accepted can I work on it if possible?
Thanks