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

test project that reproduces issue

Fix to make copies of QueryDict? instances pickle/unpickle friendly

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.

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

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.

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)

Replying to mikechambers:

You cannot place the POST QueryDict? directly into the session like so: {{{ request.sessionpost? = 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.)

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

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.

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.

Simpler patch to make QueryDict? unpickle friendly

rajeshdhawan's patch worked for me

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.

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

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.