Allow ModelForm to override error messages defined in model fields.

[loic84]

Currently error messages generated by model fields bypass error messages defined in ModelForm fields and leak to the end user.

This issue was brough up on the ML: ​https://groups.google.com/d/msg/django-developers/KlmdlR59vuc/e1HQpAwMYhoJ

After discussion with carljm on IRC, we came to the conclusion that allowing such override would be an improvement.

[loic84]

To address this issue we need to bubble up error codes and error params which are currently lost before they reach the ModelForm.

The fact that we don't keep track of ValidationError instances is actually a broader problem than this ModelForm issue.

I've attempted a refactor of ValidationError that aims to address that. It's a tricky issue because of backward compatibility, but the proposed solution should remain compatible. It keeps track of the individual instances of ValidationError while keeping the previous API of ValidationError.message_dict and ValidationError.messages.

The Django test suite currently pass without errors.

​https://github.com/loic/django/compare/ticket20199

[loic84]

The refactor of ValidationError is more or less complete.

I've also made the necessary changes to ModelForm to enable overriding messages when provided with sufficient information.

Currently only ValidationError from model validators can take advantage of the new system because these are the only exceptions where an error code is provided and where the message params are not coerced into the final message.

Most other validation errors are raised using the following pattern:

raise ValidationError(self.error_messages['null'])

In order to override such messages we need to know their code, so these would have to be rewritten as follows:

raise ValidationError(self.error_messages['null'], code='null')

I think we should take advantage of this refactor to also make params available to people who rewrite error messages.

Given the following error message:

'invalid_choice': _('Value %r is not a valid choice.')

Currently raised as follows:

msg = self.error_messages['invalid_choice'] % value
raise ValidationError(msg)

We should write:

raise ValidationError(self.error_messages['invalid_choice'], code='invalid_choice', params={'choice': value})

There is a catch, to do that, we need to rewrite the messages to use mapping keys instead of positional formatting. I believe it's a good move, as not everyone want to display values in their messages and that wouldn't be possible with positional formatting.

It's a big task to update every occurrence of ValidationError, so I would like confirmation by a core dev that this would be accepted before I start.

[Claude Paroz]

Replying to loic84:

There is a catch, to do that, we need to rewrite the messages to use mapping keys instead of positional formatting. I believe it's a good move, as not everyone want to display values in their messages and that wouldn't be possible with positional formatting.

I'm all in favour of this change. Could you check the patch on #17840 and tell me if it goes in the direction you suggest?

[loic84]

Replying to claudep:

I'm all in favour of this change. Could you check the patch on #17840 and tell me if it goes in the direction you suggest?

It is in the right direction, but these are just a subset of the error messages that need editing. Most of the others live in django.db.models.fields.__init__.py.

The changes to default_error_messages is exactly what needs to happen throughout the system, but the part that raises the exception would have to be rewritten to provide the error code and error params to the ValidationError constructor.

So maybe this ticket should take over?

[loic84]

PR ​https://github.com/django/django/pull/1019

[Tim Graham]

Updated PR to merge cleanly: ​https://github.com/django/django/pull/1247

[Jannis Leidel]

-0

[Jannis Leidel]

Thinking about it a bit more, I'm still not thrilled this change in behavior would be introduced but can't come up with a better approach than storing stuff in the ValidationError. This definitely needs a hands-on explanation in the docs how to use this in the case where you want to override an unwanted error message from a 3rd party app form.

[Tim Graham]

Updated pull request with documentation. Looks good to me, but wouldn't mind one more set of eyes before it gets committed.

[Tim Graham <timograham@…>]

In ee77d4b25360a9fc050c32769a334fd69a011a63:

Fixed #20199 -- Allow ModelForm fields to override error_messages from model fields

[Loic Bistuer <loic.bistuer@…>]

In 71093d22b62548fc2667468f1ae9e28f4fef30db:

Fixed #16986 -- Model.clean() can report errors on individual fields.

This commit provides the tests for this issue but the actual problem was solved
by the ValidationError refactor in f34cfec and ee77d4b.

Refs #20199.

[Loic Bistuer <loic.bistuer@…>]

In f563c339ca2eed81706ab17726c79a6f00d7c553:

Fixed #20867 -- Added the Form.add_error() method.

Refs #20199 #16986.

Thanks @akaariai, @bmispelon, @mjtamlyn, @timgraham for the reviews.

[ispivey@…]

The changes in the initial refactor commit appear to have caused a regression for us when upgrading to 1.6.

If you look at the unit test that was changed in tests/validators/tests.py in ​https://github.com/django/django/commit/f34cfec0fa1243b4a3b9865b961a2360f211f0d8 , it used to be possible to create a ValidatonError by passing a dict of string values like

ValidationError({ 'field_name': 'error' })

instead of a dict of list values like

ValidationError({'field_name': ['error',]}) .

Now, however, if you pass a string instead of a list, ValidationError treats the string as a list and thus returns an error dictionary like:

{
    "first": [
        "F", 
        "i", 
        "r", 
        "s", 
        "t", 
        " ", 
        "P", 
        "r", 
        "o", 
        "b", 
        "l", 
        "e", 
        "m"
    ]
}

As a result, some of our error messages now have a lot of commas in them.

In reading the docs in more detail, I realize we weren't following the best practice, which is to directly set self._errors instead of passing a dictionary to the ValidationError constructor. So we'll go fix our code, but I wanted to let Loic know in case others were possibly depending on this old behavior.

[loic84]

I remember discussing this with a core dev on IRC at the time, if I remember well, we concluded that this actually was a mistake, as it only worked as the consequence of reduce(operator.add, message.values()) in conjunction with strings being iterables, and that it doesn't work well with more than one error message; for instance {'first': 'First Problem', 'second': 'Second Problem'} would lead to a ValidationError.message attribute of 'Second ProblemFirst Problem'. Also the ValidationError(dict) construct is a private API, it was never documented.

FWIW [f563c339] in Django 1.7 further tightened the ValidationError constructor, and {'first': 'First Problem', 'second': 'Second Problem'} would work properly there. That commit also introduces the Form.add_error() API which may solve your original problem of adding error to a specific field.

[ispivey@…]

Makes sense. After looking over the docs, we guessed we'd fallen into using a private API. Thanks for the response and the heads-up re: 1.7 changes!

[Tim Graham <timograham@…>]

In 8847a0c601e4261823b1726b2db73eec2ac17940:

Fixed #16192 -- Made unique error messages in ModelForm customizable.

Overriding the error messages now works for both unique fields, unique_together
and unique_for_date.

This patch changed the overriding logic to allow customizing NON_FIELD_ERRORS
since previously only fields' errors were customizable.

Refs #20199.

Thanks leahculver for the suggestion.