Regression in behaviour of ValidationError.update_error_dict()

[Russell Keith-Magee]

The behaviour of ValidationError.update_error_dict() has have changed since 1.6.

The problem was introduced by rf563c339ca2eed81706ab17726c79a6f00d7c553, which was a fix for #20867, adding the Form.add_error() method.

Under 1.6.:

In [1]: from django.core.exceptions import ValidationError

In [2]: from django.forms.util import ErrorList

In [3]: v = ValidationError({'fieldname': ErrorList(['This is an error.'])})

In [4]: errors = ErrorList()

In [5]: new_errors = v.update_error_dict(errors)

In [6]: print new_errors
{'fieldname': [u'This is an error.']}

Under 1.7rc1:

In [1]: from django.core.exceptions import ValidationError

In [2]: from django.forms.util import ErrorList

In [3]: v = ValidationError({'fieldname': ErrorList(['This is an error.'])})

In [4]: errors = ErrorList()

In [5]: new_errors = v.update_error_dict(errors)

In [6]: print new_errors
{'fieldname': [ValidationError([u'This is an error.'])]}

That is - the error dictionary returned by update_error_dict now returns a list of ValidationErrors, not a list of error messages. You get similar problems if the original ValidationError contains a list of strings:

In [3]: v = ValidationError({'fieldname': ['This is an error.']})

or just a single standalone string:

In [3]: v = ValidationError({'fieldname': 'This is an error.'})

In each case, Django 1.6 returns the content that was provided; Django 1.7 returns a ValidationError containing a list of errors.

[Russell Keith-Magee]

I'm marking this as a release blocker - but I'm not 100% sure that it actually is.

I'll open a discussion on django-dev with details.

[Russell Keith-Magee]

Django-dev discussion: ​https://groups.google.com/d/msg/django-developers/oN1Q_oMYRVw/sp2bDdNyx0MJ

[Anssi Kääriäinen]

The change isn't in the update_error_dict() method. It is deeper in the structure of ValidationError.error_dict. The code intentionally forces the error_dict to contain only ValidationError instances. Thus, it seems it will be hard to support the 1.6 way of doing things.

[loic84]

Indeed ValidationError now works as a vehicle for other ValidationError instances. A ValidationError can represent a single error, a list of errors, or a dict of errors. Since ValidationError is used in so many contexts and must accept a wide range of inputs (e.g. a simple string), the constructor normalizes everything to ValidationError instances. The public accessors message_dict and messages assure backwards compatibility (although in read-only mode).

This was made necessary so that we never lose track of actual ValidationError instances so we can retrieve their metadata at any time (error code, non-interpolated error message, and message params). This gave us niceties such as as_json() and as_data(), the possibility to "identify" what error has happened thanks to error codes, and the ability to rewrite any error messages.

Now, update_error_dict is IMO a bit of a wart in the ValidationError class. Ideally it'd be replaced by a standalone function only for internal use. Maybe what we could do, is fake the old behavior of update_error_dict() and hit it with a deprecation, that way we clean up the API while avoiding the subtle change of behavior for people who were using this private API. Similarly I was planning to deprecate message_dict and messages at some point (see ​https://github.com/loic/django/compare/validation_error_deprecations) since there is now a better option which works for both list and dict: iterating over the ValidationError yields the error messages when it's a list, and a (field, messages) tuple when it's a dict, although that part is obviously too late for 1.7.

[Andrew Godwin]

Of course, update_error_dict isn't actually documented, so this isn't strictly a regression; it's just internal behaviour changing (though the same used to go for _meta and if we changed that there would be uproar)

That said, if we land a patch for this and push back the release, there is another fix I would like to land (for a different issue that's not really an issue but would make things smoother anyway), so I'm not against adding a fix. Faking the behaviour and sticking in deprecation works for me.

[loic84]

I started looking at a deprecation but it looks like people have been using it (​https://github.com/search?q=update_error_dict&type=Code) and I feel bad taking it away unless we have a decent replacement for it. I'm not sure what would be a decent alternative, one option would be to have a __add__/__radd__ method allowing to merge ValidationErrors with dict/list.

@russellm, in which context has it been an issue that update_error_dict returned instances of ValidationError? If the handling mimics what core does, i.e. gather errors in a dict then raise them all in a new ValidationError, then it should work as expected, even if the dict contains a mix of ValidationError and simple error strings.

[Russell Keith-Magee]

Replying to loic84:

I started looking at a deprecation but it looks like people have been using it (​https://github.com/search?q=update_error_dict&type=Code) and I feel bad taking it away unless we have a decent replacement for it. I'm not sure what would be a decent alternative, one option would be to have a __add__/__radd__ method allowing to merge ValidationErrors with dict/list.

@russellm, in which context has it been an issue that update_error_dict returned instances of ValidationError? If the handling mimics what core does, i.e. gather errors in a dict then raise them all in a new ValidationError, then it should work as expected, even if the dict contains a mix of ValidationError and simple error strings.

My specific use case looks like this:

    def clean(self):
        ...
        try:
            validate_code(self.instance, code)
        except ValidationError as e:
            self._errors = e.update_error_dict(self._errors)
            del cleaned_data['code']

From a look at the Github code search you provided, most of the uses in the wild are exactly the same (although many uses don't do the deletion from cleaned_data). It's not hard to adapt to the new API, but then you lose any 1.6/1.7 cross-compatibility (since add_error doesn't exist in 1.6, and update_error_dict doesn't behave consistently across versions).

[loic84]

I'm still investigating but the problem here is not so much that update_error_dict returns instances of ValidationError, but rather that instances of ErrorList are stripped when they go through the ValidationError constructor. This issue arises because ErrorList is the container that assures backwards compatibility.

Even if we made update_error_dict return strings in place of ValidationError we would still end up with plain lists in place of ErrorList in form._error when using the self._errors = e.update_error_dict(self._errors) pattern.

[loic84]

One solution would be to recommend in the release notes that ErrorDict should be composed of ErrorList.

See ​https://gist.github.com/adf447623b4e25c97c7e.

[loic84]

Made a PR with a documentation fix: ​https://github.com/django/django/pull/2868

It also contains a test case that shows how to deal with the situation in a way that should be compatible with both Django 1.6 and 1.7; although it's obviously better going forward to use the new public API than to rely on outdated private APIs.

Russ does it look acceptable to you?

[Russell Keith-Magee]

I'm not sure I see any real improvement here. The conversion on line 771 of the test_forms.py of the PR is where the "fix" really lies - that's the line that is converting the ValidationError containing errors into an ErrorList that is actually printable to the user. The three line fix represented by lines 769-771 of test_forms.py isn't required in Django 1.6; but it's mandatory in 1.7. If you omit those three lines, the test on line 780-784 (the one that actually checks the content of form.errors) will fail in exactly the way that I'm seeing in production.

That means that there is still a backwards incompatibility - anyone using update_error_dict will still need to make a code change, except this time, they're making a change as a workaround, rather than making a change to use the new, officially supported add_error() API.

[loic84]

This is an incompatible change (now documented as such) that was made necessary to provide many features; this "fix" is only about providing an upgrade path to people that used private APIs. update_error_dict is a good candidate for deprecation and even _error is now a private API in Django 1.7 (see b72b85af153848ef5f1f07f5c1e5a81e3563b015), so clearly the way forward is to use add_error(); maybe it should be more prominent in the feature list of Django 1.7?

Since people that need both Django <= 6 *and* Django 1.7 compatibility in the same codebase (3rd party apps?) can't use add_error() I tried to come up with a workaround they can use to make it work cross version. Since this is all happening due to changes in private APIs I believe this is an acceptable compromise.

[loic84]

I'm reasonably happy with the shape of the patch at ​https://github.com/django/django/pull/2868.

Russ, is it good enough to close this ticket?

[Russell Keith-Magee]

@loic84 - Other than the fact that it doesn't fix my problem grumble grumble :-)

Seriously - Looks good to me. Ship it!

[Loic Bistuer <loic.bistuer@…>]

In 1966054febbb96b713db27513617eabdbd70957b:

Fixed #22915 -- Document backward incompatible changes in the ValidationError constructor.

This patch also fixes update_error_dict to better handle the use case described
in this ticket, previously the type of the provided container could be lost in
some conditions.

Thanks Russell Keith-Magee for the report and Tim Graham for review.

[Loic Bistuer <loic.bistuer@…>]

In b68c7a5abbb737c54dff91e56e0e56e67cd3f718:

[1.7.x] Fixed #22915 -- Document backward incompatible changes in the ValidationError constructor.

This patch also fixes update_error_dict to better handle the use case described
in this ticket, previously the type of the provided container could be lost in
some conditions.

Thanks Russell Keith-Magee for the report and Tim Graham for review.

Backport of eb7df6b8d7 from master