[Django] #33346: Form rendering in Django 4 causes AttributeError during testing
Django
---------------------------------------+------------------------
Reporter: OutOfFocus4 | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 4.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------+------------------------
I started updating a project from Django 3 to Django 4. When I ran my unit
tests after the update, one of them failed with an AttributeError.
The full stack trace is
{{{
Traceback (most recent call last):
File "/private/tmp/example/example/tests.py", line 17, in test_formset
self.assertFormsetError(
File "/private/tmp/example/.venv/lib/python3.9/site-
packages/django/test/testcases.py", line 565, in assertFormsetError
if field in context[formset].forms[form_index].errors:
AttributeError: 'ManagementForm' object has no attribute 'forms'
}}}
It looks like rendering the ManagementForm of a formset adds a context to
response.context which contains a "form". Calling "assertFormsetError" to
check for errors in a formset called "form" will check the formset's
errors, but will also try to look at the errors in any "form" in all
contexts, including the ManagementForm.
--
Ticket URL: <https://code.djangoproject.com/ticket/33346>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: OutOfFocus4 | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 4.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by OutOfFocus4):
* Attachment "example.zip" added.
Basic Django application to demonstrate the bug. Just install Django 4 and
run "manage.py test"
Django
Reporter: OutOfFocus4 | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 4.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by OutOfFocus4):
* Attachment "example.zip" added.
Basic Django application to demonstrate the bug. Just install Django 4 and
run "manage.py test"
--
Django
Reporter: OutOfFocus4 | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 4.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by OutOfFocus4):
* Attachment "example.zip" removed.
Basic Django application to demonstrate the bug. Just install Django 4 and
run "manage.py test"
--
Django
-----------------------------------+--------------------------------------
Reporter: OutOfFocus4 | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* cc: David Smith (added)
* component: Forms => Testing framework
* severity: Normal => Release blocker
Comment:
Thanks for the report, it's niche but we should definitely fix this bug in
`assertFormsetError()`.
Regression in 456466d932830b096d39806e291fe23ec5ed38d5.
--
Ticket URL: <https://code.djangoproject.com/ticket/33346#comment:1>
Django
-----------------------------------+------------------------------------
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/33346#comment:2>
Django
-----------------------------------+------------------------------------
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Baptiste Mispelon):
I did some digging around `assertFormsetError` (and `assertFormError`)
while working on #33301 and came to the conclusion that they might be
fundamentally broken.
Both methods work by looking for a specific name in the context of
**each** template rendered during the execution of the view. When the name
is not found in the template, it is skipped. If the name is found then the
`context[name]` object is checked for the assertion.
The issue is that you cannot always control which templates are rendered
during your views, and especially what names those templates are using in
their respective contexts.
The situation is even worse now that Django uses templates to render
forms, formsets and widgets.
Restricting the search to the first context would probably fix most issues
like the one reported here, but it's not 100% correct (and backwards-
incompatible).
The only way out that I could think of would be to deprecate passing a
`response` to `assertFormError`/`assertFormsetError` in favor of passing
the form/formset object directly. I think it would be a more natural API
anway (I never understood why the assertions were based on the response to
begin with).
--
Ticket URL: <https://code.djangoproject.com/ticket/33346#comment:3>
Django
-----------------------------------+------------------------------------
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
Replying to [comment:3 Baptiste Mispelon]:
> ...
> Restricting the search to the first context would probably fix most
issues like the one reported here, but it's not 100% correct (and
backwards-incompatible).
What do you think about skipping context values that are not a `FormSet`
instance? or don't have the `forms` attribute. This should be backward
compatible.
>
> The only way out that I could think of would be to deprecate passing a
`response` to `assertFormError`/`assertFormsetError` in favor of passing
the form/formset object directly. I think it would be a more natural API
anway (I never understood why the assertions were based on the response to
begin with).
We cannot change or deprecate an existing and documented API as a part of
patch which is intended for backport. This can be discussed separately.
Personally, I like the idea.
--
Ticket URL: <https://code.djangoproject.com/ticket/33346#comment:4>
Django
Reporter: OutOfFocus4 | Owner: Baptiste
| Mispelon
Type: Bug | Status: assigned
Component: Testing framework | Version: 4.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Baptiste Mispelon):
* owner: nobody => Baptiste Mispelon
* status: new => assigned
Comment:
Replying to [comment:4 Mariusz Felisiak]:
> What do you think about skipping context values that are not a `FormSet`
instance? or don't have the `forms` attribute. This should be backward
compatible.
I haven't thought it through completely, but my gut feeling is that your
proposed fix would work for the reported regression but there might still
be corner cases that could fail. But those corner cases might have already
been broken so it's probably ok.
I'll start working on a PR, it'll be easier for me to think it through
with some concrete examples.
Replying to [comment:4 Mariusz Felisiak]:
> We cannot change or deprecate an existing and documented API as a part
of patch which is intended for backport. This can be discussed separately.
Personally, I like the idea.
Agreed, I'll open a separate ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/33346#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: OutOfFocus4 | Owner: Baptiste
| Mispelon
Type: Bug | Status: assigned
Component: Testing framework | Version: 4.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Baptiste Mispelon):
* cc: Baptiste Mispelon (added)
* has_patch: 0 => 1
Comment:
PR here: https://github.com/django/django/pull/15165
I went with the `isinstance` check wich seems a bit more robust since
`forms` seems like quite a common attribute name and might catch other
non-formset things.
--
Ticket URL: <https://code.djangoproject.com/ticket/33346#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: OutOfFocus4 | Owner: Baptiste
| Mispelon
Type: Bug | Status: assigned
Component: Testing framework | Version: 4.0
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33346#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: OutOfFocus4 | Owner: Baptiste
| Mispelon
Component: Testing framework | Version: 4.0
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"cb383753c0e0eb52306e1024d32a782549c27e61" cb383753]:
{{{
#!CommitTicketReference repository=""
revision="cb383753c0e0eb52306e1024d32a782549c27e61"
Fixed #33346 -- Fixed SimpleTestCase.assertFormsetError() crash on a
formset named "form".
Thanks OutOfFocus4 for the report.
Regression in 456466d932830b096d39806e291fe23ec5ed38d5.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33346#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: OutOfFocus4 | Owner: Baptiste
| Mispelon
Type: Bug | Status: closed
Component: Testing framework | Version: 4.0
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"15031852c5ef6b3713bb9766d918460ea46e254e" 15031852]:
{{{
#!CommitTicketReference repository=""
revision="15031852c5ef6b3713bb9766d918460ea46e254e"
[4.0.x] Fixed #33346 -- Fixed SimpleTestCase.assertFormsetError() crash on
a formset named "form".
Thanks OutOfFocus4 for the report.
Regression in 456466d932830b096d39806e291fe23ec5ed38d5.
Backport of cb383753c0e0eb52306e1024d32a782549c27e61 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33346#comment:9>