Making management forms for formsets optional

[Patryk Zawadzki]

Hi folks,

I've talked to Marc about fixing the case where a formset will raise
an uncaught ValidationError when instantiated with a missing or broken
management form. This has caused us great pain when dealing with
vulnerability scanners that tend to POST random crap to each endpoint
they find as it means formsets—unlike forms—are not safe to
instantiate with untrusted data.

Now wrapping each and every formset instance in a try/except block and
having to manually handle the fallback case is not fun. But as Marc
pointed out, forgetting a management form is one of the most common
pitfalls people run into when dealing with formsets.

Therefore I propose making the management form optional. The only case
that requires an explicit TOTAL_FORMS variable are forms that consist
of nothing but checkboxes (as unchecked checkboxes are not included in
POST data). In other cases (including all ModelFormSets as they always
contain a hidden PK field) we can deduct the number of submitted forms
from the data itself.

Ideally I would get rid of the management form entirely and document
that a form that is nothing but checkboxes is an unsupported case and
that a workaround would be to include an extra hidden field. Honza may
or may not kill me for suggesting that.

For now, I would like to make the formset optional and document the
cases where you need to include it. If included, it would take
precedence to keep the existing deployments working.

Thoughts?

--
Patryk Zawadzki
I solve problems.

[Patryk Zawadzki]

Playground: https://github.com/django/django/pull/4797

[Tomek Paczkowski]

I remember that Ruby on Rails solved problem with checkboxes with a hidden field that was added before each checkbox [1]

[1]: http://apidock.com/rails/ActionView/Helpers/FormHelper/check_box

[Florian Apolloner]

What about instead of trying to guess the forms from the input, just fix the one condition which causes the error and return 0 as totalformcount + an error marker to reraise the error in clean of the modelform?

On Friday, June 5, 2015 at 11:29:21 AM UTC+1, Patryk Zawadzki wrote:

[Marc Tamlyn]

I believe the complaint is mostly that an invalid formset will raise a 500 whereas an invalid form (should) never. There was a patch to display an error if not. My complaint is that this error can only occur if the dev has rendered the form by hand missing the management form. It's likely they would also miss the non form error output this way and so the end result is we display back empty forms to the user with no useful error event though they filled in values. This is hard to debug and bad ux - a 500 is better.

Equally, every time a security crawler posts an empty body to a page triggering a 500 is irritating noise. We don't report invalid forms as 500s either...

This brought up the question of whether the management form is needed at all.

Unrelated arguments: It also streamlines testing user formsets massively, and significantly reduces the complexity of js to add more forms.

Marc

--
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/de9e5a03-7f14-42e8-bf76-9405440ecf94%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Shai Berger]

Semi-devil's-advocate suggestion: Replace the management form with a
management field, a hidden field with

name = "__manage__%s" % (formsetname)

and

value="x"

Then instead of taking the number of forms from a field on a management form,
we can just take the accumulated length of the management field (which would be
returned as an array). This also offers a better solution to the checkboxes-
only-form problem, I think.

Do we use the management form for anything other than counting forms in the
formset?

Shai.

> > <https://groups.google.com/d/msgid/django-developers/de9e5a03-7f14-42e8-
> > bf76-9405440ecf94%40googlegroups.com?utm_medium=email&utm_source=footer>
> > .

[Patryk Zawadzki]

2015-06-07 10:18 GMT+02:00 Shai Berger <sh...@platonix.com>:
> Semi-devil's-advocate suggestion: Replace the management form with a
> management field, a hidden field with
>
> name = "__manage__%s" % (formsetname)
>
> and
>
> value="x"
>
> Then instead of taking the number of forms from a field on a management form,
> we can just take the accumulated length of the management field (which would be
> returned as an array). This also offers a better solution to the checkboxes-
> only-form problem, I think.

This is what Rails prefers but I think it is only added for checkbox fields.

> Do we use the management form for anything other than counting forms in the
> formset?

Yes, we currently have the concept of initial forms even for bound
formsets. This does not mirror form behaviour as bound forms don't
have the concept of initial data. I'd like to get rid of that as well
but Russell is not sure whether there are cases that require initial
forms to work correctly.

[Carl Meyer]

There definitely are a number of cases that require initial forms to
work correctly, as can be easily seen by simply grepping for
`initial_form_count`. It's especially heavily used by the
inline-formsets code.

The formsets code is complex and fragile in general, IMO. I've often had
the feeling that a rewrite could simplify it and make it more robust,
though it's hard to say for sure without actually attempting such a rewrite.

But I think such a project needs to be approached as a full rewrite,
probably first as a third-party module and then later (if it works out
well) considered for merge as a separate replacement module. It does not
seem likely to me that tugging on individual threads like "remove
management form" and "remove initial forms" will result in changes to
the current formset code that are both backwards-compatible and an
improvement in the design. I'm willing to be proven wrong, but so far
the current pull request only strengthens that feeling.

Also, it's important to note that many people (including the admin) are
using the management form fields in JS code to allow for dynamic
formsets, so backwards compatibility needs to take that into account as
well.

I think it will be much more productive to focus on minimalist changes
that address the problem of a missing management form causing a 500
error, without trying to deeply change the design of the formsets code.

Carl

[Carl Meyer]

On 06/07/2015 02:18 AM, Shai Berger wrote:
> Semi-devil's-advocate suggestion: Replace the management form with a
> management field, a hidden field with
>
> name = "__manage__%s" % (formsetname)
>
> and
>
> value="x"
>
> Then instead of taking the number of forms from a field on a management form,
> we can just take the accumulated length of the management field (which would be
> returned as an array). This also offers a better solution to the checkboxes-
> only-form problem, I think.

I think this is a promising germ of an idea for a better approach to
formsets. It would be tricky to make it backwards-compatible (and it
simply can't be backwards-compatible in terms of markup, meaning it
would break all the dynamic-formset JS out in the wild).

> Do we use the management form for anything other than counting forms in the
> formset?

We do - we also use it for counting "initial" vs "extra" (or new) forms.
However, your idea could easily be extended to cover that as well.

We also use it for communicating min-num and max-num to the client side,
for use by dynamic-formset JS. (Such JS usually also relies on the
total-form-count and initial-form-count values).

Carl

[Patryk Zawadzki]

2015-06-08 20:22 GMT+02:00 Carl Meyer <ca...@oddbird.net>:
> But I think such a project needs to be approached as a full rewrite,
> probably first as a third-party module and then later (if it works out
> well) considered for merge as a separate replacement module. It does not
> seem likely to me that tugging on individual threads like "remove
> management form" and "remove initial forms" will result in changes to
> the current formset code that are both backwards-compatible and an
> improvement in the design. I'm willing to be proven wrong, but so far
> the current pull request only strengthens that feeling.

This could very well be true. We could also try implementing a field
wrapping a subform and a field wrapping a multitude of fields. I am
not sure whether there would be an obvious way to render the resultin
form as HTML but it would mean that we'd end up only having a single
thing to deal with (forms) and would allow existing views to
transparently handle situations where more than one form is needed
(just create a form that consists of two subform fields).