[Django] #22229: Invalid data can cause InlineFormSet.is_valid to throw ValueError
Django
----------------------------+---------------------------
Reporter: anonymous | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
Severity: Normal | Keywords: InlineFormSet
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+---------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/22229>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: anonymous | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
Keywords: InlineFormSet | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_docs: => 0
* needs_better_patch: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
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."]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22229#comment:1>
Django
Reporter: anonymous | Owner: jrothenbuhler
Type: Bug | Status: assigned
Component: Forms | Version: 1.6
Severity: Normal | Resolution:
Keywords: InlineFormSet | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jrothenbuhler):
* status: new => assigned
* owner: nobody => jrothenbuhler
--
Ticket URL: <https://code.djangoproject.com/ticket/22229#comment:2>
Django
Reporter: anonymous | Owner: jrothenbuhler
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: InlineFormSet | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jrothenbuhler):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22229#comment:3>
Django
Reporter: anonymous | Owner: Jake
| Rothenbuhler
Type: Bug | Status: assigned
Component: Forms | Version: 1.6
Severity: Normal | Resolution:
Keywords: InlineFormSet | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22229#comment:4>
Django
Reporter: anonymous | Owner: Jake
| Rothenbuhler
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: InlineFormSet | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jon Dufresne):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/8679 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/22229#comment:5>
Django
Reporter: anonymous | Owner: Jake
| Rothenbuhler
Component: Forms | Version: 1.6
Severity: Normal | Resolution: fixed
Keywords: InlineFormSet | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"d7881d2020a7337ed128eeef811ef1c1e549b481" d7881d20]:
{{{
#!CommitTicketReference repository=""
revision="d7881d2020a7337ed128eeef811ef1c1e549b481"
Fixed #22229 -- Added primary key validation to
BaseModelFormSet._construct_form().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22229#comment:6>