Code

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#12896 closed (fixed)

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

Reported by: lukeplant 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: UI/UX:

Description

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 = form.save()
    if obj.val != orig_val:
        do_something()

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 lukeplant 4 years ago.
Patch to tests to demonstrate problem.

Download all attachments as: .zip

Change History (8)

Changed 4 years ago by lukeplant

Patch to tests to demonstrate problem.

comment:1 Changed 4 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 4 years ago by russellm

  • Component changed from Uncategorized to Forms
  • milestone set to 1.2
  • Triage Stage changed from Unreviewed to Design 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 4 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 4 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 4 years ago by jkocherhans

  • Owner changed from nobody to jkocherhans
  • Status changed from new to assigned

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 4 years ago by jkocherhans

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:7 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.