Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#33346 closed Bug (fixed)

assertFormsetError() crashes on formset named "form".

Reported by: OutOfFocus4 Owned by: Baptiste Mispelon
Component: Testing framework Version: 4.0
Severity: Release blocker Keywords:
Cc: David Smith, Baptiste Mispelon Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (1)

example.zip (6.4 KB ) - added by OutOfFocus4 3 years ago.
Basic Django application to demonstrate the bug. Just install Django 4 and run "manage.py test"

Download all attachments as: .zip

Change History (10)

by OutOfFocus4, 3 years ago

Attachment: example.zip added

Basic Django application to demonstrate the bug. Just install Django 4 and run "manage.py test"

comment:1 by Mariusz Felisiak, 3 years ago

Cc: David Smith added
Component: FormsTesting framework
Severity: NormalRelease blocker
Summary: Form rendering in Django 4 causes AttributeError during testingassertFormsetError() crashes on formset named "form".

Thanks for the report, it's niche but we should definitely fix this bug in assertFormsetError().

Regression in 456466d932830b096d39806e291fe23ec5ed38d5.

comment:2 by Mariusz Felisiak, 3 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Baptiste Mispelon, 3 years ago

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).

in reply to:  3 ; comment:4 by Mariusz Felisiak, 3 years ago

Replying to 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.

in reply to:  4 comment:5 by Baptiste Mispelon, 3 years ago

Owner: changed from nobody to Baptiste Mispelon
Status: newassigned

Replying to 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 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.

comment:6 by Baptiste Mispelon, 3 years ago

Cc: Baptiste Mispelon added
Has patch: set

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.
I went with hasattr(context[formset], 'forms') because the isinstance check broke some admin tests (InlineAdminFormSet is not a subclass of BaseFormset 😿 ).

Last edited 3 years ago by Baptiste Mispelon (previous) (diff)

comment:7 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In cb383753:

Fixed #33346 -- Fixed SimpleTestCase.assertFormsetError() crash on a formset named "form".

Thanks OutOfFocus4 for the report.

Regression in 456466d932830b096d39806e291fe23ec5ed38d5.

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 15031852:

[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.

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