Opened 10 years ago

Closed 7 years ago

#22229 closed Bug (fixed)

Invalid data can cause InlineFormSet.is_valid to throw ValueError

Reported by: anonymous Owned by: Jake Rothenbuhler
Component: Forms Version: 1.6
Severity: Normal Keywords: InlineFormSet
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The InlineFormSet's foreign key field ("<input name='myformset-0-id' ...>") seems to assume that its data is valid for all of the initial data. Thus, if it is altered, the database layer will fail to convert it and throw an exception, instead of reporting that the form is invalid. For an integer foreign key, this takes form of a ValueError thrown by the int function.

I've created a test demonstrating the issue here: https://gist.github.com/ColonelThirtyTwo/edbc575b10b068397dc7

Change History (6)

comment:1 by Tim Graham, 10 years ago

Triage Stage: UnreviewedAccepted

On master/1.7, the test code throws a ValidationError instead, but I agree this probably should cause is_valid() simply to return False.

Traceback (most recent call last):
  File "/home/tim/code/mysite/test/tests.py", line 150, in test_django_formset_int_error
    formset.is_valid()
  File "/home/tim/code/django/django/forms/formsets.py", line 302, in is_valid
    self.errors
  File "/home/tim/code/django/django/forms/formsets.py", line 276, in errors
    self.full_clean()
  File "/home/tim/code/django/django/forms/formsets.py", line 324, in full_clean
    form = self.forms[i]
  File "/home/tim/code/django/django/utils/functional.py", line 55, in __get__
    res = instance.__dict__[self.func.__name__] = self.func(instance)
  File "/home/tim/code/django/django/forms/formsets.py", line 141, in forms
    forms = [self._construct_form(i) for i in xrange(self.total_form_count())]
  File "/home/tim/code/django/django/forms/models.py", line 869, in _construct_form
    form = super(BaseInlineFormSet, self)._construct_form(i, **kwargs)
  File "/home/tim/code/django/django/forms/models.py", line 586, in _construct_form
    pk = to_python(pk)
  File "/home/tim/code/django/django/db/models/fields/__init__.py", line 894, in to_python
    params={'value': value},
ValidationError: [u"'' value must be an integer."]

comment:2 by Jake Rothenbuhler, 10 years ago

Owner: changed from nobody to Jake Rothenbuhler
Status: newassigned

comment:3 by Jake Rothenbuhler, 10 years ago

I'm not quite sure that we want to make is_valid() return False in this case. With that behavior, the template would end up displaying an error message saying something like: "Invalid primary key for form that should have an existing instance." The user, seeing this error message, would not be able to fix it because the primary key fields wouldn't be editable. Instead, I think the error should be thrown as it currently is, resulting in a 500 response. I can't think of any situations where this error would arise from anything other than a misconfigured form initialization or a mistake in the form rendering. Is there a valid use case for catching this error that I'm missing?

If we do want to throw the error, the current ValidationError is probably misleading. I think a ValueError would be more correct.

comment:4 by Jon Dufresne, 7 years ago

I'm not quite sure that we want to make is_valid() return False in this case.

Generally speaking, user input (even malformed user input) should never result in a 500 response. A 500 response indicates an error server side. I think Django should catch this malformed user input and respond appropriately. The HTTP status codes used to represent user error or input error are 4XX.

the template would end up displaying an error message saying something like: "Invalid primary key for form that should have an existing instance."

I agree that care will needed to present an error as useful as possible to the user, hopefully avoiding technical jargon. Given the nature of the input error, good phrasing may not be entirely possible. But I still think it shouldn't result in a 500 response.

comment:5 by Jon Dufresne, 7 years ago

Has patch: set

comment:6 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In d7881d20:

Fixed #22229 -- Added primary key validation to BaseModelFormSet._construct_form().

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