Opened 2 years ago

Closed 2 years ago

Last modified 18 months 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: master
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 Changed 2 years ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 2 years ago by loic84

  • 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 Changed 2 years ago by bmispelon

  • Cc bmispelon@… added

comment:4 follow-up: Changed 2 years ago by 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.

comment:5 Changed 2 years ago by loic84

  • Owner changed from nobody to loic84
  • Status changed from new to assigned

comment:6 in reply to: ↑ 4 ; follow-up: Changed 2 years ago by claudep

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?

comment:7 in reply to: ↑ 6 Changed 2 years ago by 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?

comment:9 Changed 2 years ago by timo

  • Cc timograham@… added

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

comment:10 Changed 2 years ago by jezdez

-0

comment:11 Changed 2 years ago by jezdez

  • 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 Changed 2 years ago by timo

  • 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 Changed 2 years ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In ee77d4b25360a9fc050c32769a334fd69a011a63:

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

comment:14 Changed 2 years ago by 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.

comment:15 Changed 20 months ago by 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.

comment:16 Changed 19 months ago by 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.

comment:17 Changed 19 months ago by 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.

comment:18 Changed 19 months ago by 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!

comment:19 Changed 18 months ago by 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.

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