Opened 10 years ago

Closed 10 years ago

Last modified 9 years ago

#12896 closed (fixed)

ModelForm.is_valid() and ModelForm.errors have backwards-incompatible side effects since model validation

Reported by: Luke Plant Owned by: jkocherhans
Component: Forms Version: 1.1
Severity: Keywords:
Cc: Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


Since r12098 (model validation merge), ModelForm.is_valid() and ModelForm.errors have the side effect of modifying the bound instance.

Attached is a (rather lazily written) patch to the tests which demonstrates the problem - it ought to pass, and does pass before r12098 (noting that the patch doesn't apply cleanly before r12098), but not after.

This is a significant backwards incompatibility, but rather subtle too - in my case it was caught only due to fairly extensive tests in one of my projects. It is also quite counter-intuitive that either form.errors or form.is_valid should have any side effects. At the very least this needs to be clearly documented. I'm guessing it might be tricky to fix, given earlier problems surrounding the order of events in model validation.

For your information, my use case is detecting that a change has occurred to a certain field:

if form.is_valid():
    orig_val = obj.val
    obj =
    if obj.val != orig_val:

The side effects of form.is_valid() mean that do_something() is no longer called.

Attachments (1)

form_is_valid_side_effects_test.diff (665 bytes) - added by Luke Plant 10 years ago.
Patch to tests to demonstrate problem.

Download all attachments as: .zip

Change History (8)

Changed 10 years ago by Luke Plant

Patch to tests to demonstrate problem.

comment:1 Changed 10 years ago by Alex Gaynor

I'm pretty sure this is intended behavior now (it broke one of my projects as well, but it was easily resolved).

comment:2 Changed 10 years ago by Russell Keith-Magee

Component: UncategorizedForms
milestone: 1.2
Triage Stage: UnreviewedDesign decision needed

We need to decide on this one way or the other for 1.2 - if this has changed, we need to make sure the change is intentional.

comment:3 Changed 10 years ago by jkocherhans

I'm going to think about this more, but I don't see a way around it without extensively altering things or copying the instance. Form.errors and Form.is_valid() have had side effects for as long as I can remember, they trigger validation. The side effects are more extensive now, but side effects of those calls in general aren't anything new. I think the solution is just to document it in the backwards compatibility notes, but I'll try to track Honza down at PyCon to chat about it.

comment:4 Changed 10 years ago by Jacob

I'll leave the final call to Joseph, but I think he's right and this should just be noted in the docs.

comment:5 Changed 10 years ago by jkocherhans

Owner: changed from nobody to jkocherhans
Status: newassigned

Model.full_clean() modifies the model in-place. I hope no one will argue that's the wrong thing to do. So, to fix this I see a couple of solutions, both of which I think are unacceptable.

  1. Copy the instance. Copying could be an expensive operation, and if the developer really wants to do that, they can just explicitly do it themselves and pass the copy to the ModeForm constructor.
  1. Change the internals of Model.full_clean() to optionally not modify the model in place, and change ModelForm to use that option. I think this option is just needlessly confusing.

I'll add a warning to the release notes and to the ModelForm docs.

comment:6 Changed 10 years ago by jkocherhans

Resolution: fixed
Status: assignedclosed

(In [12693]) Fixed #12896. Documented the new side-effects of ModelForm validation.

comment:7 Changed 9 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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