Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#26018 closed Bug (fixed)

FormMixin.get_context_data calls get_form improperly after form_invalid

Reported by: Chris Cogdon Owned by: Chris Cogdon
Component: Generic views Version: 1.9
Severity: Normal Keywords:
Cc: 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

In the fix for ticket #25548, the validated (and failed) form is now passed in as a parameter to get_context_data. In that code, self.get_form() is called unnecessarily, which will likely cause a performance and side-effect penalty equivalent or worse to validating the form twice.

Solution is to use the "if x not in d" pattern instead of setdefault.

I'll create a pull request.

Change History (6)

comment:1 Changed 4 years ago by Chris Cogdon

Owner: changed from nobody to Chris Cogdon
Status: newassigned

comment:2 Changed 4 years ago by Chris Cogdon

Has patch: set
Triage Stage: UnreviewedAccepted

Pull request: https://github.com/django/django/pull/5900

Release notes for 1.9.1 updated. Docs compile without error. Test case added. All tests pass under Sqlite.

comment:3 Changed 4 years ago by Chris Cogdon

Note: I added a call-counter stub to the AuthorUpdate view. Ideally I'd use the "mock" library to do this kind of thing, as that would also let me check the call-count after form_valid, rather than only being able to test if res.context['view'] was present.

Last edited 4 years ago by Chris Cogdon (previous) (diff)

comment:4 Changed 4 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

Thanks, I'll merge the release note with the one for #25548 since it seems this issue isn't present in 1.9 but rather introduced in the fix for that ticket as you mentioned.

By the way, we have automated checks on the pull request for docs building without error and tests passing, so there's no need to mention all that here.

comment:5 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In e429c51:

Fixed #26018 -- Prevented unecessary get_form() call in FormMixin.get_context_data().

Changed "dict.setdefault" to "if x in dict" pattern so that get_form() would not
be called unnecessarily, specifically in the case where FormMixin.form_invalid()
calls get_context_data() with the current form.

comment:6 Changed 4 years ago by Tim Graham <timograham@…>

In 8202ce45:

[1.9.x] Fixed #26018 -- Prevented unecessary get_form() call in FormMixin.get_context_data().

Changed "dict.setdefault" to "if x in dict" pattern so that get_form() would not
be called unnecessarily, specifically in the case where FormMixin.form_invalid()
calls get_context_data() with the current form.

Backport of e429c5186ceed81c4627165518e0c70c58e69595 from master

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