Backporting ticket 25548 into 1.9.x

[James Bennett]

About two months ago, ticket #25548[1] was opened, pointing out that

on an invalid form submission, FormView was calling form validation

methods twice (what was actually happening was that the original form

instance was discarded, a new one constructed with the same data, and

the first access to its errors or cleaned_data would run validation on

the new instance).

As noted on the ticket, that was fixed in master in e171a83[2], and

the ticket was closed about one month ago.

However, the fix was not backported into the (at-the-time unreleased)

1.9.x branch, and so this bug is present in Django 1.9, as I

discovered while investigating a bug report submitted to

django-registration.

I asked around on IRC and nobody present seemed to know why it wasn't

backported at the time of the commit. Obviously the bug was not

treated as a release blocker for 1.9, and does not cause a security

issue, crash or data loss, which means an argument can be made that

our policies don't support backporting this fix to 1.9.

Acknowledging that, I'd still like to argue for it to be backported to

1.9, for three primary reasons:

1. This is a performance regression, and potentially a significant

   performance regression. Form validation can be quite complex, and

   very often involves performing database queries; depending on the

   form, it may also involve accessing other services over a

   network. If Django 1.9 effectively causes that work to be doubled

   on invalid submissions, that's a strong argument against using

   Django 1.9. We shouldn't be in the business of incentivizing people

   not to use the latest release.

2. While an argument can be made that it doesn't *technically* violate

   any documented guarantees, the way in which Django's form system is

   implemented and designed carries with it a strong implication that

   validation methods are only called once per form submission. Many

   developers work under that implication, and rely on it when

   designing their form validation schemes. We should not break that

   without heavily warning and documenting it, which we haven't done.

3. Fear of the unknown. The history of the Web is littered with cases

   where, despite the fact that best practices involved writing

   idempotent code, large numbers of people didn't write idempotent

   code and bad things happened the instant it was accessed in ways

   which assumed idempotence (for example: URL prefetching features

   accidentally triggering data loss or data modification due to Web

   services not being idempotent on GET). While I can't off the top of

   my head come up with any realistic example where a non-idempotent

   form validation method would cause a crash, security issue or data

   loss, experience has shown that I am not imaginative enough to

   figure out what people will do in the real world. We should not be

   inviting those kinds of hard-to-imagine problems by not backporting

   this fix.

So I'd like to propose that e171a83 be backported into the 1.9.x

branch, to be included in the first bugfix release of the 1.9 series.

Thoughts?

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

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

[Aymeric Augustin]

> On 6 déc. 2015, at 10:49, James Bennett <ubern...@gmail.com> wrote:
>
> Thoughts?

I don’t think anyone ever prevented a core dev who wanted to backport a commit
from doing so — unless it carried a risk of backwards incompatibility, which
doesn’t appear to be the case here.

--
Aymeric.

[Shai Berger]

Not to my knowledge also, but we did have a policy change, about two years
ago, against arbitrary backports at a core-dev's judgment (which was the norm
until then). So, I find it very proper that James has brought this up on the
list.

That said, the reasoning for backporting seems sound to me, so +1.

Shai.

[Marc Tamlyn]

Agreed the reasoning is sound. This is a major bug as far as I'm concerned and I'd like to see it backported. Thanks for bringing it up.

Marc

[Tim Graham]

I've done the backport. I think the only reason it wasn't done initially is that there's no indication on the ticket that the issue was a regression.

[Anssi Kääriäinen]

On Mon, Dec 7, 2015 at 3:20 PM, Tim Graham <timog...@gmail.com> wrote:
> I've done the backport. I think the only reason it wasn't done initially is
> that there's no indication on the ticket that the issue was a regression.

For some reason we don't mention backporting of regression fixes in
https://docs.djangoproject.com/en/1.9/internals/release-process/.
Should we add an explicit bullet point for that?

- Anssi

[Tim Graham]

Done in https://github.com/django/django/commit/9c835990ea2911c06a877296fbc25157c1302e1d -- thanks for pointing it out.

[James Bennett]

Thanks to everyone for the quick consensus, and to Tim for backporting it and fixing the docs before I could get around to it :)

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" 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/54d1599b-caca-4379-b017-657c8b6b4cae%40googlegroups.com.

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

[charettes]

I confirm this is the reason it wasn't backported in the first place.