Regression in ValidationError in 1.7

[Russell Keith-Magee]

Hi all,

I've just done some testing of 1.7 against my 'day job' code base. The good news is that everything has pretty much worked out of the box without any problems.

However, I did find one regression, which I've opened as #22915 [1]. Full details are on the ticket; the short version is that the API for Validation.update_error_dict() has changed in very subtle way. The change was introduced by [2], which introduced Form.add_error() [3].

The catch here is that ValidationError is definitely stable API, but update_error_dict isn't documented - so it isn't clear to me whether this should be considered a regression in a stable API that must be fixed, or a "backwards compatibility gotcha" that is worth some documentation, but not a code change.

I caught the problem running the test suite for my "day job app". I'm doing some moderately complex form error handling, and in the `clean()` method for a form, I'm raising an error against a specific field on the field. This appears to be the exact use case that the `Form.add_error()` API is designed to satisfy, so the regression has been introduced by adding a formal API for something that people (including myself) would have been doing previously in an ad-hoc fashion. 

The question here is whether we need to continue to support the ad-hoc usage in a backwards compatible way.

Thoughts?

Yours,

Russ Magee %-)

[1] https://code.djangoproject.com/ticket/22915

[2] https://github.com/django/django/commit/f563c339ca2eed81706ab17726c79a6f00d7c553

[3] https://code.djangoproject.com/ticket/20867

[Wim Feijen]

Hi Russ,

Good point and thanks for testing! For me, this should definitely be listed in the release notes and we should provide people a hint of how to fix it as well.

IMO we do not need to support a non-documented feature.

Wim

[Curtis Maloney]

Am I reading this right as "people used to commonly solve this problem by using an internal API, but now we have a public one... AND the old internal API is now changed"?

If so, the solution seems obvious -- document that it's time to move the the official solution :)

--

Curtis

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/841bccb0-3251-47e1-bc53-27413d052506%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Russell Keith-Magee]

On Fri, Jun 27, 2014 at 7:35 PM, Curtis Maloney <cur...@acommoncreative.com> wrote:

Am I reading this right as "people used to commonly solve this problem by using an internal API, but now we have a public one... AND the old internal API is now changed"?

If so, the solution seems obvious -- document that it's time to move the the official solution :)

Broadly speaking, I agree. However, my hesitation in categorically agreeing stems from the fact that ValidationError *is* public API. Or, at least, the constructor is, - but it seems a little absurd that we formally document a way for people to construct an object, but not to support any of the ways that they might have *used* that object.

Yours,

Russ Magee %-)

[Tim Graham]

Russ, could you include a code snippet of what no longer works? I think there is a documented solution that involves modifying Form._errors (see https://docs.djangoproject.com/en/1.6/ref/forms/validation/#django.forms.Form.clean), but I'm not sure if you're doing that or something different. The 1.7 version of that example now uses Form.add_error().

[Russell Keith-Magee]

Hi Tim,

My use case in practice is effectively this - in the clean() method for a form that has a 'code' field, plus a number of others:

    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']

        ...

It's not quite the same as the example in the docs, because validate_code might return an error with a field *other* than code (i.e., code is valid, but as a result of the new value in code, another field isn't).

Russ %-)

--

You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/d87963aa-ee2c-47d8-a9f1-2eb5b78a6c0b%40googlegroups.com.

[Carl Meyer]

I don't think there's any absurdity. The documented and supported way to
"use" ValidationError has always been to simply raise it from a clean()
or clean_field() method, in which case Django itself catches it. Calling
other undocumented methods on it it is clearly making use of unsupported
internal API.

I think Curtis is right, there is no problem with breaking use of the
undocumented API and documenting the new public API to achieve the same
thing.

Carl

[Russell Keith-Magee]

Hi Carl,

On Thu, Jul 3, 2014 at 5:33 AM, Carl Meyer <ca...@oddbird.net> wrote:

On 06/27/2014 06:41 AM, Russell Keith-Magee wrote:
>
> On Fri, Jun 27, 2014 at 7:35 PM, Curtis Maloney

> <cur...@acommoncreative.com <mailto:cur...@acommoncreative.com>> wrote:
>
>     Am I reading this right as "people used to commonly solve this
>     problem by using an internal API, but now we have a public one...
>     AND the old internal API is now changed"?
>
>     If so, the solution seems obvious -- document that it's time to move
>     the the official solution :)
>
>
> Broadly speaking, I agree. However, my hesitation in categorically
> agreeing stems from the fact that ValidationError *is* public API. Or,
> at least, the constructor is, - but it seems a little absurd that we
> formally document a way for people to construct an object, but not to
> support any of the ways that they might have *used* that object.

I don't think there's any absurdity. The documented and supported way to
"use" ValidationError has always been to simply raise it from a clean()
or clean_field() method, in which case Django itself catches it. Calling
other undocumented methods on it it is clearly making use of unsupported
internal API.

Sure - I can appreciate that argument. However, on the other side - the use case I've described isn't completely left field, and a github code search shows that update_error_dict() is being used in the wild for exactly that reason. 

That said - the discussion on the ticket and related PRs indicates that restructuring necessary to make form.add_error() work mean that there probably isn't a path that will allow backwards compatibility, so the argument is moot at this point. The new add_error() API is much nicer, and it isn't that hard to convert update_error_dict() usage into add_error() usage, so I suppose we'll have to live with this upgrade wart.

Yours,

Russ Magee %-)