Opened 16 years ago

Closed 16 years ago

Last modified 12 years ago

#7233 closed (fixed)

placing request.POST.copy() in session wipes all session values

Reported by: mikechambers Owned by: nobody
Component: contrib.sessions Version: dev
Severity: Keywords:
Cc: rajesh.dhawan@…, dsalvetti@…, philipp@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Ramiro Morales)

Version : (0, 97, 'pre')

The issue is that it appears trying to store a QueryDict (request.POST, or a copy of a QueryDict) in the session, will wipe all session data.

You can see the original thread here:

http://groups.google.com/group/django-users/browse_thread/thread/260d9b94f773655e

Basically:

request.session['form_post_data'] = request.POST.copy()
request.session['foo'] = "bar"

Then, in another request:

print request.session.keys()

prints []

(i.e. no keys)

But:

request.session['foo'] = "bar"

then in another request:

print request.session.keys()
prints ['foo']

There seems to be two potential issues:

  1. Cannot store request.POST.copy() or request.POST in the session (should you be able to do this?)
  2. Trying to place request.POST.copy() in the session wipes all session values.

More info on the following threads:

http://groups.google.com/group/django-users/browse_thread/thread/260d9b94f773655e

and

http://groups.google.com/group/django-developers/browse_thread/thread/ec651a3b5f877698

Attachments (3)

sessiontest.zip (11.4 KB ) - added by mikechambers 16 years ago.
test project that reproduces issue
querydict_pickle_fix.diff (1.9 KB ) - added by rajesh.dhawan@… 16 years ago.
Fix to make copies of QueryDict instances pickle/unpickle friendly
picklable_QueryDict.diff (518 bytes ) - added by Rajesh Dhawan 16 years ago.
Simpler patch to make QueryDict unpickle friendly

Download all attachments as: .zip

Change History (20)

by mikechambers, 16 years ago

Attachment: sessiontest.zip added

test project that reproduces issue

by rajesh.dhawan@…, 16 years ago

Attachment: querydict_pickle_fix.diff added

Fix to make copies of QueryDict instances pickle/unpickle friendly

comment:1 by Rajesh Dhawan, 16 years ago

Cc: rajesh.dhawan@… added
Has patch: set

The problem is that django.http.QueryDict instances (even mutable copies of them) do not unpickle properly because the unpickle mechanism does not called init which makes such instances devoid of attributes _mutable and encoding. Without these two attributes pretty much all methods of the resulting instance are unusable. In particular _assert_mutable() crashes and prevents setitem() from working.

The bottom line is that these instances do get pickled and stored into the session but when the session is restored and the object is being unpickled that fails. The session mechanism simply disregards this (rightfully) and returns an empty session dictionary. So no other session data is readable.

I've attached a patch that *hopefully* fixes this. The patch ensures that _mutable and encoding values are saved in the pickled stream and are restored back during unpickling.

comment:2 by Alex Gaynor, 16 years ago

If I understand it correctly couldn't you just implement __getstate__ and __setstate__ methods on QueryDict.

in reply to:  2 comment:3 by Rajesh Dhawan, 16 years ago

Replying to Alex:

If I understand it correctly couldn't you just implement __getstate__ and __setstate__ methods on QueryDict.

Normally, that would work. My original attempt to fix this was indeed to implement those methods only to discover that because QueryDict.__setitem__ bombs (on the above mentioned _mutable assert check), __setstate__ fails during unpickling.

in reply to:  1 ; comment:4 by mikechambers, 16 years ago

Replying to rajeshdhawan:

I just tested the patch, and you can now store a copy of QueryDict in the session.

This now works:

	request.session['post_copy'] = request.POST.copy()

You cannot place the POST QueryDict directly into the session like so:

	request.session['post'] = request.POST

I am not sure if that is the intended behavior (although I am guessing it is)

in reply to:  4 comment:5 by Rajesh Dhawan, 16 years ago

Replying to mikechambers:

You cannot place the POST QueryDict directly into the session like so:

	request.session['post'] = request.POST

I am not sure if that is the intended behavior (although I am guessing it is)

I think that should be the intended behaviour since request.POST is documented to be immutable. The docs recommend using request.POST.copy() if you need a mutable clone that you can modify (and, I assume, pickle, save in session, restore from session, etc.)

comment:6 by anonymous, 16 years ago

Needs tests: set

comment:7 by Simon Greenhill, 16 years ago

Triage Stage: UnreviewedAccepted

comment:8 by Ramiro Morales, 16 years ago

Description: modified (diff)

comment:9 by Djoume Salvetti, 16 years ago

Cc: dsalvetti@… added

comment:10 by Philipp Wollermann, 16 years ago

Cc: philipp@… added
milestone: 1.0

I just tripped over this bug too.. debugged my code, then Djangos code for two hours, came to the same conclusions as the others here. This is really bad, to swallow all exceptions occuring on unpickling the session-data, even in DEBUG = True mode! It creates extremely hard to find errors, because you debug your code for ages and don't see anything wrong - and then you realize that Django clears your session on the next request.. you debug that too, think it's a bug in Django .. and THEN realize you were doing something not allowed, but Django never complained by an exception, but just kept quiet and cleared your session on the next request.

So, it would be really cool to:

  • Throw exceptions on unpickle-errors to the developer and don't silently discard them (at least not in Debug mode!)
  • Make QuerySets pickle-able (why is request.POST a QueryDict at all?)

comment:11 by Malcolm Tredinnick, 16 years ago

Patch needs improvement: set

I don't really like the approach in this patch because it's effectively creating a new class object with the appropriate mutable setting, rather than a class instance. In other words, it feels like __new__ is being used for the wrong purpose and __init__ (or similar) should really be used. A solution based on __setstate__ and __getstate__ would be preferred. Rajesh mentioned a problem then with __setitem__, but I don't see why __setstate__ can't set the _mutable attribute as almost the first thing it does to work around that. I'll try to have a poke at this, probably after the beta release, but if anybody wants to get there first, I'd be looking hard at the __getstate__/__setstate__ approach.

Also, this really, really needs a test case so that we can assure it doesn't regress. The data structure is already tested in tests/regressiontests/httpwrappers/, so extra things can go in there.

comment:12 by Rajesh Dhawan, 16 years ago

Malcolm, your instincts are dead on. The fix here should be much simpler than what my original patch does. So I had another look at this and am attaching a patch that seems to do the trick while being minimal. It simply defines _mutable (and encoding) with default values outside of __init__ so that the session unpickler doesn't choke on trying to find those attributes.

by Rajesh Dhawan, 16 years ago

Attachment: picklable_QueryDict.diff added

Simpler patch to make QueryDict unpickle friendly

comment:13 by Colin Copeland, 16 years ago

rajeshdhawan's patch worked for me

comment:14 by Malcolm Tredinnick, 16 years ago

This patch still doesn't completely solve the problem. There's an issue that the order in which attributes are pickled depends upon dictionary hashing order, which varies from platform to platform. On the machine I'm on right now, for example, I can happily pickle and unpickle QuerySets, mutable or immutable, without any problems.

It seems that in some cases, however, the _mutable attribute is pickled (and therefore restored) after some of the dictionary items. Take that one step further and you realise that it's also possible for encoding to be pickled after those values, which means that it will have a value of None during calls to __setitem__(), which will lead to an exception being raised by unicode(), since that cannot have a second parameter value of None.

I've got a patch that works around that, but I can't commit it right this minute because I'm having trouble reaching the subversion repo over the network I'm on. Will commit it tomorrow morning. Mostly making a note here so that another committer doesn't accidentally duplicate the thinking and work.

comment:15 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

(In [8460]) Fixed #7233 -- Ensured that QueryDict classes are always unpicklable. This
problem only arose on some systems, since it depends upon the order in which
the attributes are pickled. Makes reliable testing kind of tricky.

comment:16 by Malcolm Tredinnick, 16 years ago

Just in case anybody is reading this in the future, my analysis here was partially bogus. The real issue with caching and pickling is that we use pickle protocol 2 when caching, which changes the behaviour of how things are unpickled. My initial tests were only with the default (level 0?) pickle protocol. In any case, it's still fixed with the above commit.

comment:17 by Jacob, 12 years ago

milestone: 1.0

Milestone 1.0 deleted

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