Opened 7 years ago

Closed 6 years ago

Last modified 4 years ago

#8171 closed (wontfix)

Invalid formset when form is missing from POST data

Reported by: James Chua <james_027@…> Owned by: brosner
Component: Forms Version: master
Severity: Keywords: formsets
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description (last modified by brosner)

A forms that doesn't have ordered prefix will not be process. On the sample the formset is expecting to process choices-0, choices-1, and choices-2 forms while choices-4, and choices-5 forms will be ignore but it should be process because the 3 forms we are referring here are the choices-0, choices-4, choices-5 and it should return False on formset.is_valid() since the choices-4 is invalid while choices-5 is considered valid since it is blank. As a result the formset is valid because it treats blank forms as valid which are choices-1 and choices-2.

>>> data = {
...     'choices-TOTAL_FORMS': '3', # the number of forms rendered
...     'choices-INITIAL_FORMS': '0', # the number of forms with initial data
...     'choices-0-choice': 'Calexico',
...     'choices-0-votes': '100',
...     'choices-4-choice': 'The Decemberists',
...     'choices-4-votes': '', # missing value
...     'choices-5-choice': '',
...     'choices-5-votes': '',
... }

>>> formset = ChoiceFormSet(data, auto_id=False, prefix='choices')
>>> formset.is_valid()
True

Change History (13)

comment:1 Changed 7 years ago by James Chua <james_027@…>

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to brosner
  • Patch needs improvement unset

comment:2 Changed 7 years ago by brosner

  • Description modified (diff)

Fixed description formatting.

comment:3 Changed 7 years ago by jacob

  • milestone set to 1.0
  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 7 years ago by brosner

Ok, I have looked a bit more deeply into this problem. The test case provided above is slightly misleading. The data provided is invalid data and the reporter is expecting this to fail. When in reality that isn't a good test case. Here is an improved case clearly showing the problem:

# Handle when forms are missing in the POST data ##############################
# See http://code.djangoproject.com/ticket/8171 ###############################

>>> ChoiceFormSet = formset_factory(Choice)

>>> data = {
...     'choices-TOTAL_FORMS': '3', # the number of forms rendered
...     'choices-INITIAL_FORMS': '0', # the number of forms with initial data
...     'choices-0-choice': 'Calexico',
...     'choices-0-votes': '100',
...     # choices-1, choices-2, choices-3 are missing. Possibly due to some
...     # Javascript add/delete of the elements.
...     'choices-4-choice': 'The Decemberists',
...     'choices-4-votes': '200',
...     'choices-5-choice': '',
...     'choices-5-votes': '',
... }

>>> formset = ChoiceFormSet(data, auto_id=False, prefix='choices')
>>> formset.is_valid()
True
>>> [form.cleaned_data for form in formset.forms]
[{'votes': 100, 'choice': u'Calexico'}, {'votes': 200, 'choice': 'The Decemberists'}, {}]

comment:5 Changed 7 years ago by brosner

Oh and here is the output:

$ ./runtests.py --settings=toolbox.settings forms
======================================================================
FAIL: Doctest: regressiontests.forms.tests.__test__.formset_tests
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/brian/Development/django-svn/django/test/_doctest.py", line 2180, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for regressiontests.forms.tests.__test__.formset_tests
  File "/Users/brian/Development/django-svn/tests/regressiontests/forms/tests.py", line unknown line number, in formset_tests

----------------------------------------------------------------------
File "/Users/brian/Development/django-svn/tests/regressiontests/forms/tests.py", line ?, in regressiontests.forms.tests.__test__.formset_tests
Failed example:
    [form.cleaned_data for form in formset.forms]
Expected:
    [{'votes': 100, 'choice': u'Calexico'}, {'votes': 200, 'choice': 'The Decemberists'}, {}]
Got:
    [{'votes': 100, 'choice': u'Calexico'}, {}, {}]


----------------------------------------------------------------------
Ran 32 tests in 3.444s

FAILED (failures=1)

comment:6 follow-up: Changed 7 years ago by brosner

Ok. I have thought this through and got input from Joseph. We have concluded, don't do this. It is expected that all forms are present in the POST data regardless. If you are using Javascript to provide some interface where the user can add/remove extra/initial data forms then you need to properly ensure the ones removed are properly marked for deletion via choices-N-DELETE. I will leave this open for the pending documentation change indicating this.

comment:7 Changed 7 years ago by jacob

  • Resolution set to wontfix
  • Status changed from new to closed

wontfix - see #8649.

comment:8 Changed 7 years ago by veena

  • Resolution wontfix deleted
  • Status changed from closed to reopened

There is a bug which perhaps relate to this ticket.

When you set form-TOTAL_FORMS for example to 5 but send less than 5 forms then formset.is_valid() goes buggy True.
formsets.forms contain 5 forms. So when you iterate through forms then you go through 5 cycles. Each form.is_valid() is same buggy True (?!) and form.cleaned_data is empty dictionary in each missing forms (that's how I discover it. KeyError on cleaned_data dictionary.) form.errors is empty for missing forms.

comment:9 Changed 6 years ago by gwilson

  • milestone changed from 1.0 to 1.2

comment:10 in reply to: ↑ 6 Changed 6 years ago by anonymous

Replying to brosner:

Ok. I have thought this through and got input from Joseph. We have concluded, don't do this. It is expected that all forms are present in the POST data regardless. If you are using Javascript to provide some interface where the user can add/remove extra/initial data forms then you need to properly ensure the ones removed are properly marked for deletion via choices-N-DELETE. I will leave this open for the pending documentation change indicating this.

I absolutely agree. But missing forms should be threated as not valid, because even form variables aren't sended. See it in James code:

[{'votes': 100, 'choice': u'Calexico'}, {}, {}]

That's where problem is. It's probably not clear in James Chua description. To more clear see my 8th comment comment:8

comment:11 Changed 6 years ago by jacob

  • Resolution set to wontfix
  • Status changed from reopened to closed

Again, don't do this. See #8649.

comment:12 Changed 5 years ago by veena

  • milestone changed from 1.2 to 1.3

It's not upon to me to do it or not. Via HTTP you can receive any possible combination of data in POST, can't you? In the case of comment:8 django says: "that's a valid formset" but it isn't.

comment:13 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

Note: See TracTickets for help on using tickets.
Back to Top