Code

Opened 7 years ago

Closed 21 months ago

#5524 closed Bug (fixed)

Cleaned form data should not be deleted if other data is invalid.

Reported by: __hawkeye__ Owned by: charettes
Component: Forms Version: master
Severity: Normal Keywords:
Cc: hv@…, simon@…, charette.s@… 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

Currently, when validating a newforms Form object (.full_clean()), the cleaned_data attribute is deleted if any of the fields on the form are invalid.

Data that has been cleaned should be retained, rather than deleted.

Some sample use cases are:

  1. Allowing partial previews (discussed in #5153)
  2. Maintain file-uploads across submits of a form
  3. Prevent having to perform expensive operations twice (hash the value and compare on resubmit)

Discussed here http://groups.google.com/group/django-developers/browse_thread/thread/a23807d32cc3042d/58a4ea6a2e3e8a77.

Attachments (7)

ticket_5524__revision_6369.diff (903 bytes) - added by __hawkeye__ 7 years ago.
cleaned_data is persisted for fields that do not have validation errors. (corrected typo in previous)
ticket_5524__rev_6822.diff (11.4 KB) - added by __hawkeye__ 6 years ago.
Now with docs and tests.
ticket_5524__rev_6917.diff (5.1 KB) - added by __hawkeye__ 6 years ago.
5524-new.diff (7.0 KB) - added by claudep 3 years ago.
Refresh the patch
5524-new2.diff (7.9 KB) - added by claudep 2 years ago.
Added release notes section
5524-new-3.diff (12.5 KB) - added by aaugustin 2 years ago.
5524-new-4.patch (13.7 KB) - added by charettes 21 months ago.
Doc update and unicode_literals related changes

Download all attachments as: .zip

Change History (33)

comment:1 Changed 7 years ago by __hawkeye__

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

Changed 7 years ago by __hawkeye__

cleaned_data is persisted for fields that do not have validation errors. (corrected typo in previous)

comment:2 Changed 7 years ago by ubernostrum

  • Triage Stage changed from Unreviewed to Accepted

Changed 6 years ago by __hawkeye__

Now with docs and tests.

comment:3 Changed 6 years ago by __hawkeye__

  • Needs documentation unset
  • Needs tests unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:4 follow-up: Changed 6 years ago by mtredinnick

  • Triage Stage changed from Ready for checkin to Accepted

Is the change to the _data() method really necessary?

I'd tend to lean towards displaying the data exactly as the user entered it when we are redisplaying the form after an error. This part of the patch changes what the user entered to the normalised form, which may be more confusing than helpful. I'm not going to reject it out of hand in case there's a subtlety I'm missing, but more explanation required for that part, please.

comment:5 in reply to: ↑ 4 Changed 6 years ago by __hawkeye__

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

Replying to mtredinnick:

Is the change to the _data() method really necessary?

I'd tend to lean towards displaying the data exactly as the user entered it when we are redisplaying the form after an error.


You're absolutely right on this. The use-case I'm using as a reference is #2 from the ticket description (preserving file uploads across submits). In my own solution to that problem, I had to hack data to pull the information from the normalization process. Looking more closely, this was the wrong way of handling things since it has unintended (and potentially harmful/confusing) results.

I'm attaching a new patch which introduces a cleaned_data property to the BoundField class. This means that normalized data will be easily accessible, but the behavior of _data() will remain unchanged.

What do you think of this approach?

I should have thought about this when I had to go changing so many tests.

Changed 6 years ago by __hawkeye__

comment:6 Changed 6 years ago by __hawkeye__

  • milestone set to 1.0

Spoke with Malcolm, adding 1.0 milestone.... he doesn't think it's likely to be backwards incompatible, but we should give it a bit more thought.

comment:7 Changed 6 years ago by mtredinnick

  • milestone changed from 1.0 to post-1.0
  • Summary changed from [patch] Cleaned form data should not be deleted if other data is invalid. to Cleaned form data should not be deleted if other data is invalid.

Pushing to after 1.0 because it adds features. We've just got too much to do to nurse the bugs that might be caused by even little additions at the moment. Sorry, Ben.

comment:8 Changed 5 years ago by guettli

  • Cc hv@… added

comment:9 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:10 Changed 4 years ago by copelco

this ticket is somewhat related to #10828

comment:11 Changed 3 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to Bug

comment:12 Changed 3 years ago by julien

  • Easy pickings unset
  • Patch needs improvement set

The tests would need to be rewritten using unittests since this is now Django's preferred way.

Changed 3 years ago by claudep

Refresh the patch

comment:13 Changed 3 years ago by claudep

  • Owner __hawkeye__ deleted
  • Patch needs improvement unset
  • Status changed from assigned to new
  • UI/UX unset

I just attached a new patch for current trunk. I didn't include the cleaned_data for the bound field, as it might be a separate issue, if I understand it well. Comments welcome.

comment:14 Changed 3 years ago by guettli

I applied 5524-new.diff and all of my 280 unittests passed. I read the patch and think it can be committed.

comment:15 follow-up: Changed 3 years ago by ris

I often find myself using the pattern

form = BananaForm ( request.GET )

if not form.is_valid ():
	# probably very APIFRAGILE, but we need the remnants of the cleaned_data
	# containing the bits of form that did validate.
	form.cleaned_data = {}
	form._clean_fields ()
	form = BananaForm ( initial = form.cleaned_data )

so I can supply arbitrary pre-filled-out form elements in a GET string to a view that only usually POSTs to itself. Doing it this way allows just the supplied (and I suppose valid) elements to be filled in without nasty error messages for e.g. required fields that weren't.

I would love to be able to remove such flaky bits from my code.

I would have personally suggested moving/renaming cleaned_data to something else (partial_cleaned_data?) when overall validation fails rather than just not deleting it. Some existing code may be implicitly relying on the existence of cleaned_data to signify the form as being valid.

comment:16 in reply to: ↑ 15 Changed 3 years ago by claudep

Replying to ris:
(...)

Some existing code may be implicitly relying on the existence of cleaned_data to signify the form as being valid.

The docs clearly specify that this behaviour may change in the future, so they have been warned (sorry for them).

comment:17 Changed 2 years ago by aaugustin

  • Patch needs improvement set

This change should be explained in the release notes. Besides this the patch looks pretty good.

Changed 2 years ago by claudep

Added release notes section

comment:18 Changed 2 years ago by aaugustin

  • Owner set to aaugustin

comment:19 Changed 2 years ago by simon29

  • Cc simon@… added

comment:20 Changed 2 years ago by aaugustin

  • Owner changed from aaugustin to nobody
  • Patch needs improvement unset

I'm uploading a new patch with the following changes:

  • updated patch against latest revision of trunk
  • kept cleaned_data when a modelform fails a uniqueness check or a date uniqueness check (and I'm not totally confident on these changes)
  • replaced a hasattr(form, 'cleaned_data') with form.is_valid() in the formtools tests
  • updated docs/ref/forms/validation.txt to account for this change
  • rephrased the docs slightly

Changed 2 years ago by aaugustin

comment:21 Changed 2 years ago by guettli

I applied 5524-new-3.diff and all of my 296 unittests passed. I read the patch and think it can be committed. But the release notes should be updated, too.

comment:22 Changed 21 months ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:23 Changed 21 months ago by charettes

  • Cc charette.s@… added
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

The documentation should be changed to reflect that it has been changed in 1.5 and not 1.4

comment:24 Changed 21 months ago by charettes

  • Owner changed from nobody to charettes

I'll update the patch.

Changed 21 months ago by charettes

Doc update and unicode_literals related changes

comment:25 Changed 21 months ago by charettes

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

Since the patch was originally written by a core developer and marked as RFC by another one and it's really just a doc update to reflect the changes are taking effect in 1.5 I'm moving back to RFC. Feel free to move back to accepted. All tests still pass on Python 2.7.3rc1 sqlite3.

comment:26 Changed 21 months ago by Claude Paroz <claude@…>

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

In [121fd109de09ece4e263e508f9034df9b583da46]:

Fixed #5524 -- Do not remove cleaned_data when a form fails validation

cleaned_data is no longer deleted when form validation fails but only
contains the data that did validate.
Thanks to the various contributors to this patch (see ticket).

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.