Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#20199 closed New feature (fixed)

Allow ModelForm to override error messages defined in model fields.

Reported by: loic84 Owned by: loic84
Component: Forms Version: dev
Severity: Normal Keywords:
Cc: bmispelon@…, timograham@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (19)

comment:1 by Claude Paroz, 11 years ago

Triage Stage: UnreviewedAccepted

comment:2 by loic84, 11 years ago

Has patch: set

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

comment:3 by Baptiste Mispelon, 11 years ago

Cc: bmispelon@… added

comment:4 by loic84, 11 years ago

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.

comment:5 by loic84, 11 years ago

Owner: changed from nobody to loic84
Status: newassigned

in reply to:  4 ; comment:6 by Claude Paroz, 11 years ago

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?

in reply to:  6 comment:7 by loic84, 11 years ago

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?

comment:9 by Tim Graham, 11 years ago

Cc: timograham@… added

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

comment:10 by Jannis Leidel, 11 years ago

-0

comment:11 by Jannis Leidel, 11 years ago

Needs documentation: set
Patch needs improvement: set

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.

comment:12 by Tim Graham, 11 years ago

Needs documentation: unset
Patch needs improvement: unset

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

comment:13 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In ee77d4b25360a9fc050c32769a334fd69a011a63:

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

comment:14 by Loic Bistuer <loic.bistuer@…>, 11 years ago

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.

comment:15 by Loic Bistuer <loic.bistuer@…>, 10 years ago

In f563c339ca2eed81706ab17726c79a6f00d7c553:

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

Refs #20199 #16986.

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

comment:16 by ispivey@…, 10 years ago

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.

comment:17 by loic84, 10 years ago

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.

comment:18 by ispivey@…, 10 years ago

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!

comment:19 by Tim Graham <timograham@…>, 10 years ago

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.

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