Invalid data can cause InlineFormSet.is_valid to throw ValueError

[anonymous]

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

[Tim Graham]

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."]

[Jake Rothenbuhler]

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.

[Jon Dufresne]

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.

[Jon Dufresne]

​PR

[Tim Graham <timograham@…>]

In d7881d20:

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