Using form validation to prevent deletion no longer works (after rev 10206/ticket #9587)

[Dan Tallis]

[I posted a usage question on the same subject to django-users a short
time ago, and Karen suggested I bring the issue here.]

Since rev 10206, if a form in a formset has validation errors but is
marked for deletion, the formset is_valid returns True (assuming there
are no other form errors). Prior to rev 10206 it would return False.
The change was on the grounds that "formset with can_delete shouldn't
require deleted forms to
validate" -- see ticket #9587 [1].

I understand the thinking behind the change, but I think I've hit an
unintended consequence: it's no longer possible (straightforwardly) to
use the validation mechanism to prevent the deletion.

My application behaviour (which worked pre-10206 but doesn't work from
10206) is as follows: There's a formset, with delete checkboxes and
some other fields in each form. The user may wish to delete some rows
(forms). On submit, the application decides whether the deletion(s)
can go ahead, based on some dynamic state. If any deletions are not
allowed, the user is presented with error messages alongside each form
explaining why not. This is implemented by having a clean() method on
the form that checked for cleaned_data['DELETE'], performed the check,
and if necessary raised a ValidationError with an error message along
the lines of "You cannot delete this right now because ...".

Prior to 10206, the formset is_valid returned False and the user saw
the error messages as I intended. From 10206 on, the formset is_valid
returns True (because validation errors on forms marked for deletion
"don't count") and so my view goes on to try to save the forms.

So, is this an unintended consequence? If so, please can it be backed
out pending further design discussion?

Regards

Dan

[1] http://code.djangoproject.com/ticket/9587

[Joseph Kocherhans]

On Sun, Apr 5, 2009 at 11:33 AM, Dan Tallis <dan.t...@gmail.com> wrote:

Prior to 10206, the formset is_valid returned False and the user saw
the error messages as I intended.  From 10206 on, the formset is_valid
returns True (because validation errors on forms marked for deletion
"don't count") and so my view goes on to try to save the forms.

So, is this an unintended consequence?  If so, please can it be backed
out pending further design discussion?

We can't really have it both ways. Either validation errors on a form stop deletion, or they don't. (Or there's a way to choose one or the other per formset, but I'd like to avoid that.) The new behavior is more correct in my view, and there should be a more specific way to veto deletion, but there's no way to do that without making people who rely on the pre-10206 implementation write more code. That said, I'm inclined to back it out for the 1.1 release. This has bitten at least one other person that I know of, and it wasn't in the beta, so it will likely bite others who aren't tracking trunk.

Joseph

[Alex Gaynor]

Sure we can have it both ways, how about a constructor option, "validate_deletions".  With the current(that is no validation) behavior as the default.

Alex

--
"I disapprove of what you say, but I will defend to the death your right to say it." --Voltaire
"The people's good is the highest law."--Cicero

[Malcolm Tredinnick]

On Sun, 2009-04-05 at 18:03 -0400, Alex Gaynor wrote:
[...]

> Sure we can have it both ways, how about a constructor option,
> "validate_deletions". With the current(that is no validation)
> behavior as the default.

No, we really can't. Having an option for every single possibility isn't
a good idea. It leads to option explosion and a very necessary
implementation ("happens to be technically possible" doesn't mean
"actually possible in practice", since it's not practical).

There's usually one predominantly correct behaviour in situations like
this. the fact that some people sometimes don't want that isn't a reason
to add Yet Another Option -- after all, they're not obligated to use
formsets or the admin or anything like that, so they already have a
choice to just use plain Python if they really want something else. In
this case, it sounds like people are using an implementation side-effect
to do something unintended and whilst Joseph's efforts to work with that
for a few weeks longer are admirable, long-term it's a problem that we
should solve with a proper API if the use-case is reasonable (which it
seems like it does). Preserving the undesired side-effect isn't a good
technical approach.

I agree with Joseph here that validation errors shouldn't stop deletion,
since you've said you want to *delete* the form and the data no longer
has a meaning. Data that is about to cease to exist shouldn't play a
role in validation -- it's un-data; wouldn't voom is you put 50,000
volts through it, etc.

The optional piece here is allowing for a form to be prevented from
deletion in the first place, not post hoc.

Malcolm

[Alex Gaynor]

On Sun, Apr 5, 2009 at 10:39 PM, Malcolm Tredinnick <mal...@pointy-stick.com> wrote:

On Sun, 2009-04-05 at 18:03 -0400, Alex Gaynor wrote:
[...]

> Sure we can have it both ways, how about a constructor option,
> "validate_deletions".  With the current(that is no validation)
> behavior as the default.

No, we really can't. Having an option for every single possibility isn't
a good idea. It leads to option explosion and a very necessary
implementation ("happens to be technically possible" doesn't mean
"actually possible in practice", since it's not practical).

To be clear(since I most certainly wasn't before).  I'm not necessarily advocating that idea, merely saying that it's a false dichotomy to say he have to pick only one way, we merely need to provide the default behavior.
 

There's usually one predominantly correct behaviour in situations like
this. the fact that some people sometimes don't want that isn't a reason
to add Yet Another Option -- after all, they're not obligated to use
formsets or the admin or anything like that, so they already have a
choice to just use plain Python if they really want something else. In
this case, it sounds like people are using an implementation side-effect
to do something unintended and whilst Joseph's efforts to work with that
for a few weeks longer are admirable, long-term it's a problem that we
should solve with a proper API if the use-case is reasonable (which it
seems like it does). Preserving the undesired side-effect isn't a good
technical approach.

I agree with Joseph here that validation errors shouldn't stop deletion,
since you've said you want to *delete* the form and the data no longer
has a meaning. Data that is about to cease to exist shouldn't play a
role in validation -- it's un-data; wouldn't voom is you put 50,000
volts through it, etc.

The optional piece here is allowing for a form to be prevented from
deletion in the first place, not post hoc.

Malcolm

To me the simplest solution would be to pull some of that logic out of is_valid into a "should_delete" method or something that's intended to be subclassed if the user doesn't want the delete doesn't validate behavior by default.

[Malcolm Tredinnick]

On Sun, 2009-04-05 at 22:44 -0400, Alex Gaynor wrote:
[...]
>

> To be clear(since I most certainly wasn't before). I'm not
> necessarily advocating that idea, merely saying that it's a false
> dichotomy to say he have to pick only one way, we merely need to
> provide the default behavior.

No, it's not a false dichotomy. At some level of technical discourse
(and we're certainly there on this list), it's natural to exclude the
obviously poor technical options as already read. This was one of those
cases where providing the option is a fairly clear poor choice (since
it's symptom patching and not making things any better, not problem
solving).

Malcolm

[Dan Tallis]

It's not clear to me from the thread so far whether there has been a
decision to back out 10206, but, to add some more evidence, I would
point out that it has also made it rather more awkward to process
formsets: You can no longer iterate over form.formsets (e.g. in a save
method) and consider cleaned_data, since it might not be present.
Unless I've missed something, there's deleted_forms, but no analogous
non_deleted_forms (which would help, but may still require quite a lot
of applications to change their approach).

Also the documentation [1] includes code that will no longer work in general:

>>> [form.cleaned_data for form in formset.deleted_forms]

(may fail because form.cleaned_data will not be present if the form
had 'delete' set but other field-level validation errors).

Thinking about possible ways forward, one observation that occurs to
me is that I think there's a significant semantic distinction between
field validation and form-level (non-field) validation that applies in
this case: Field validation errors on forms with delete should not
cause formset is_valid to return False but perhaps form-level errors
should...?

Regards

Dan

[1] http://docs.djangoproject.com/en/dev/topics/forms/formsets/#dealing-with-ordering-and-deletion-of-forms

[Joseph Kocherhans]

On Mon, Apr 6, 2009 at 3:09 AM, Dan Tallis <dan.t...@gmail.com> wrote:

It's not clear to me from the thread so far whether there has been a
decision to back out 10206

There hasn't been a decision. I'm still thinking about it.

 

but, to add some more evidence, I would
point out that it has also made it rather more awkward to process
formsets: You can no longer iterate over form.formsets (e.g. in a save
method) and consider cleaned_data, since it might not be present.
Unless I've missed something, there's deleted_forms, but no analogous
non_deleted_forms (which would help, but may still require quite a lot
of applications to change their approach).

It was never possible to iterate over every form and blindly check form.cleaned_data because it has never existed for invalid forms. The only difference is that you can no longer expect cleaned_data to exist for every form if formset.is_valid() returns True. You can still iterate over every form, check form.is_valid(), and decide not to check form.cleaned_data there. The change hasn't deleted form errors, they just don't prevent the entire formset from being valid anymore.

 

Also the documentation [1] includes code that will no longer work in general:

>>> [form.cleaned_data for form in formset.deleted_forms]

(may fail because form.cleaned_data will not be present if the form
had 'delete' set but other field-level validation errors).

Ah. I'll need to change that at some point. Thank you for pointing it out.

 

Thinking about possible ways forward, one observation that occurs to
me is that I think there's a significant semantic distinction between
field validation and form-level (non-field) validation that applies in
this case:  Field validation errors on forms with delete should not
cause formset is_valid to return False but perhaps form-level errors
should...?

Ugh. Please no. This is the kind of subtlety that has been so terrible to deal with in other languages. What Malcolm said still applies. Deleted data is not data. Form validation is the wrong place to veto deletion. It is an entirely separate thing. The only reason I'm considering rolling this back instead of adding hook for deletion is because there likely won't be another beta release, and we're very close to releasing 1.1. The fix probably deserves a little more time and eyeballs than it would get. Rolling back is just delaying the 3 lines of code you'll probably have to add once there's a proper way to veto deletion.

I'll post to this thread if and when I decide to roll back the change. It's spread across at least 4 different commits on different days. Rolling it back is more than a 5 minute job.

Joseph