Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#25548 closed Cleanup/optimization (fixed)

FormView.form_invalid() discards its form argument resulting in duplicated validation

Reported by: Antoine Catton Owned by: Andrew Artajos
Component: Generic views Version: master
Severity: Normal Keywords: performance FormView form_invalid
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description (last modified by Antoine Catton)

Let's consider this example:

class MyView(FormView):
    def form_valid(self, form):
        value = form.cleaned_data['value']
        try:
            do_action(value)
        except ValueError as e:
            form.add_error(None, e.message)
            return self.form_invalid(form)
        return super().form_valid(form)

The error message is ignored. The main reason being that the form in discarded in FormView.form_invalid(). Here's the current implementation on master:

    def form_invalid(self, form):
        """
        If the form is invalid, re-render the context data with the
        data-filled form and errors.
        """
        return self.render_to_response(self.get_context_data())

I propose to modify this implementation to:

    def form_invalid(self, form):
        return self.render_to_response(self.get_context_data(form=form))

since Form.get_context_data() is doing kwargs.setdefault('form', self.get_form()). I did that on my view, but it does feel this is something that belongs in Django.

Also, from my little understanding of the code, this also mean that if I have a form like this:

class MyForm(Form):
    def clean_value(self):
        value = self.cleaned_data['value']
        try:
            heavy_computation(value)
        except ValueError as e:
            raise ValidationError(e.message)
        return value

The heavy_computation() is going run twice.

Change History (15)

comment:1 Changed 4 years ago by Antoine Catton

Description: modified (diff)

comment:2 Changed 4 years ago by Simon Charette

Needs tests: set
Summary: Can't Form.add_error() in FormView.form_valid()FormView.form_invalid() discards its form argument resulting in duplicated validation
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

This makes a lot of sense.

comment:3 Changed 4 years ago by Artem Bashev

Owner: changed from nobody to Artem Bashev
Status: newassigned

comment:4 Changed 4 years ago by Artem Bashev

Owner: Artem Bashev deleted
Status: assignednew

comment:5 Changed 4 years ago by Andrew Artajos

Owner: set to Andrew Artajos
Status: newassigned

comment:6 Changed 4 years ago by Alex Morozov

@dudepare, if you don't mind, I can make a PR.

comment:7 Changed 4 years ago by Andrew Artajos

Sure go ahead, I'm curious to see the unittest for this.

comment:8 Changed 4 years ago by Alex Morozov

https://github.com/django/django/pull/5620
@dudepare, you mean there should be something tricky with a unittest? I feel like I've missed something big :).

comment:9 Changed 4 years ago by Simon Charette

Needs tests: unset
Patch needs improvement: set
Triage Stage: AcceptedReady for checkin

Provided patch LGTM pending a small docstring rephrasing.

comment:10 Changed 4 years ago by Alex Morozov

@charettes, fixed.

comment:11 Changed 4 years ago by Simon Charette <charette.s@…>

Resolution: fixed
Status: assignedclosed

In e171a83:

Fixed #25548 -- Prevented FormView.form_invalid() from discarding its form argument.

comment:12 Changed 4 years ago by Ilya Baryshev

Could this be backported to upcoming 1.9.1 please? This is a fix for regression in 1.9, so I hope this makes sense )

comment:13 Changed 4 years ago by Tim Graham

Yes, I'll backport now. I think the only reason it wasn't is that there's no obvious indication that this is a regression (introduced in #24643).

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

In 0154702a:

[1.9.x] Fixed #25548 -- Prevented FormView.form_invalid() from discarding its form argument.

Backport of e171a83b1516a3a2a393a5300527af1aab21c213 from master

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

In 285b08ab:

Refs #25548 -- Forwardported 1.9.1 release note.

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