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

[mikechambers]

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

[mikechambers]

test project that reproduces issue

[rajesh.dhawan@…]

Fix to make copies of QueryDict instances pickle/unpickle friendly

[Rajesh Dhawan]

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.

[Alex Gaynor]

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

[Rajesh Dhawan]

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.

[mikechambers]

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)

[Rajesh Dhawan]

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.)

[Philipp Wollermann]

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?)

[Malcolm Tredinnick]

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.

[Rajesh Dhawan]

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.

[Rajesh Dhawan]

Simpler patch to make QueryDict unpickle friendly

[Colin Copeland]

rajeshdhawan's patch worked for me

[Malcolm Tredinnick]

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.

[Malcolm Tredinnick]

(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.

[Malcolm Tredinnick]

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.

[Jacob]

Milestone 1.0 deleted