[1.5.x] Fixed #20922 -- Allowed customizing the serializer used by contrib.sessions #1488

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
django/contrib/messages/serializers.py
+MESSAGES_SESSION_KEY = SessionStorage.session_key
+
+
+class JSONMessagesSerializer(JSONSerializer):
@carljm

carljm Aug 20, 2013

Owner

I gave some further thought to the questions around extensibility of custom serializers, and I think the key problem may be that JSONMessagesSerializer is itself a bad idea, and a bad example for the community.

The right reason to create a custom session serializer is because you have a certain generic type of object (e.g. datetime) that you want to be able to store in the session from a variety of different places in your code, and the serialization logic should be centralized in one place.

But for highly purpose-specific serialization like this, where there is just one fixed session key and one code location where the data is being read from / written to the session, the better answer I think is to simply perform the serialization at that higher level, and store serialized data to the session.

So I don't think JSONMessagesSerializer should exist at all (which solves the ugly issue of putting it in the default project template settings.py). Instead the messages session backend should do the serialization itself and store only safe data in the session.

I also think this means there isn't really a need for a more complex hook-based API like @apollo13 had suggested. Custom session serializers should be relatively generic (in fact, if Django's default one gained the ability to handle datetimes, dates, and a few other common data types, I doubt most users would ever need a custom one at all). Third-party apps requiring their own custom session serializer should pretty much never happen; those apps should instead just provide API wrapping the session access (like contrib.messages does) and handle the serialization at that level instead.

@timgraham

timgraham Aug 20, 2013

Owner

I thought @apollo13's suggestion solved some of the problems with overly generic serializers, for example, having to introspect serialized values and determine the type of what was serialized (since the hook specifies the type of each named variable).

So, for your solution regarding messages, the solution would be to subclass SessionStorage, call it JSONSessionStorage, and also one for FallbackStorage which has storage_classes = (CookieStorage, JSONSessionStorage) which handles JSON encoding? It seems like we still have the ugliness of settings that depend on one another (MESSAGE_STORAGE and SESSION_SERIALIZER), but overall, this does seem like a better separation of concerns. I don't have experience with 3rd party apps that make use of session storage, but it seems like they may be unlikely to expose such "storage APIs" around all their session usage which could be a drawback of this solution.

Please correct me if I'm misunderstanding what your proposal would look like.

@shaib

shaib Aug 20, 2013

Member

If I understand correctly, we don't really need a JSONSessionStorage -- just modify SessionStorage to do what it would do, that would work with pickling as well.
But this, in itself, is less than optimal -- it requires contrib.messages to go through unnecessary hoops when being stored as a pickle.
I suggest, instead, to add an API on contrib.session to retrieve a "serialization method" -- a string defined by the session storage engine, with current values being "json" and "pickle". Then, the code in contrib.messages.storage.session could use that to pick the serialization method it uses, and there's no need to choose your session serializer based on whether or not you use messages, nor change your message settings based on your session storage.
This would require 3rd-party apps to adapt themselves for JSON storage in case they use the session in non-trivial ways. I am not sure this is such a bad idea. The non-backward-compatibility introduced in 1.6 would be "if an app saves to session something non-JSONable and you save sessions in cookies, things break by default" -- and there's no easy "external" way to work around it.
So perhaps this idea can somehow be merged with some notion of hooks, but I think it is better even without them.

@timgraham

timgraham Aug 20, 2013

Owner

I'ved added a commit to this PR which removes JSONMessagesSerializer; let me know what you guys think.

@carljm

carljm Aug 20, 2013

Owner

Yep, your latest commit is exactly the approach I was thinking of, looks great! I don't think the messages session storage needs both options, its fine for it to just always store safe values in session regardless of which session serializer is in use. Minor unnecessary work when pickle serializer is used, but aren't we moving towards removing pickle serializer anyway? I think in general the approach we should be encouraging for the community is to generally store only simple data structures in the session.

@shaib

shaib Aug 20, 2013

Member

Why would we remove the pickle serializer? It's only a problem when (de)serializing against user-controllable storage. Pickling, as far as I understand, is more efficient and just as safe when serializing to the DB, for example. It's true that when we need to choose defaults, we should lean towards "more robustly safe" rather than "economical", but let's not kill economical. It has its uses.

@carljm

carljm Aug 20, 2013

Owner

I don't have super strong feelings about whether we keep the pickle session serializer around, though I do think it's something of a security foot-gun. Regardless, I think that best-practice, especially for code that's intended for reuse, should be to only ever store simple native data structures in the session, so I think that's what we should model in contrib.messages. And I don't think contrib.messages should jump through hoops (and know way more about serialization formats than it really has any reason to) just in order to be slightly more efficient for the rare case when someone has a good reason to use a pickle serializer.

@timgraham

timgraham Aug 21, 2013

Owner

Latest commit serializes messages to a string using MessageEncoder regardless of SESSION_SERIALIZER.

There are no plans to remove PickleSerializer. @PaulMcMillan said: "I don't think that we need to remove pickle as a possible serializer - there are legitimate reasons to use it. There just needs to be a big warning about what happens when you do."

+from django.utils.importlib import import_module
+
+
+def import_by_path(dotted_path, error_prefix=''):
@shaib

shaib Aug 20, 2013

Member

In general, this utility is long overdue.

In detail, I think error_prefix is odd here -- you're essentially mixing a piece of error formatting (or even logging) with the function.

@apollo13

apollo13 Aug 20, 2013

Owner

Discussion of this function is outside of the scope of this ticket, this is merely a backport of what's already in master and 1.6: https://github.com/django/django/blob/master/django/utils/module_loading.py#L12

+ last name in the path. Raise ImproperlyConfigured if something goes wrong.
+ """
+ try:
+ module_path, class_name = dotted_path.rsplit('.', 1)
@shaib

shaib Aug 20, 2013

Member

As the code itself hints, there's no reason to assume the imported attribute is a class.

+ try:
+ module_path, class_name = dotted_path.rsplit('.', 1)
+ except ValueError:
+ raise ImproperlyConfigured("%s%s doesn't look like a module path" % (
@shaib

shaib Aug 20, 2013

Member

"... doesn't look like a path to a module attribute", "... doesn't look like a path to an object". It isn't supposed to be a module.

django/conf/project_template/project_name/settings.py
@@ -126,6 +126,8 @@
# 'django.contrib.admindocs',
)
+SESSION_SERIALIZER = 'django.contrib.messages.serializers.JSONSerializer'
@apollo13

apollo13 Aug 20, 2013

Owner

Shouldn't that be django.contrib.session.serializers.JSONSerializer?

Owner

apollo13 commented Aug 20, 2013

@timgraham I think your last approach is better and simple than the hooks approach, well done!

+
+.. versionadded:: 1.5.3
+
+Default: ``'django.contrib.sessions.serializers.PickleSerializer'``
@carljm

carljm Aug 20, 2013

Owner

Should it be somehow mentioned here that the new-project default differs from the global default?

Owner

apollo13 commented Aug 21, 2013

I think we can also revert the module_loading stuff to the old get_storage for 1.5 since we no longer touch it (should make it easier to port to 1.5)

django/contrib/messages/storage/session.py
@@ -1,4 +1,9 @@
+import json
+
+from django.conf import settings
@timgraham

timgraham Aug 21, 2013

Owner

unused import

Owner

carljm commented Aug 21, 2013

latest approach lgtm

else:
self.request.session.pop(self.session_key, None)
return []
+
+ def serialize_messages(self, messages):
+ encoder = MessageEncoder(separators=(',', ':'))
@apollo13

apollo13 Aug 21, 2013

Owner

I'd move the separators=(',', ':') into __init__ of MessageEncoder, so we always get "efficient" (having '__json_message' as key doesn't look to efficient ;)) json without having to specify it everywhere. But we can do this in a 1.6 cleanup commit after committing this.

@apollo13

apollo13 Aug 21, 2013

Owner

I'd do:

kwargs['separators'] = kwargs.get('separators', (',', ':'))

On Wed, Aug 21, 2013 at 8:06 PM, Tim Graham notifications@github.comwrote:

In django/contrib/messages/storage/session.py:

     else:
         self.request.session.pop(self.session_key, None)
     return []
  • def serialize_messages(self, messages):
  •    encoder = MessageEncoder(separators=(',', ':'))
    

look ok? https://gist.github.com/timgraham/dc1cc1abe202d3830eab


Reply to this email directly or view it on GitHubhttps://github.com/django/django/pull/1488/files#r5903355
.

@@ -15,6 +11,7 @@
from django.utils.crypto import salted_hmac
from django.utils import timezone
from django.utils.encoding import force_bytes
+from django.utils.module_loading import import_by_path
@apollo13

apollo13 Aug 21, 2013

Owner

Doesn't exist in 1.5, be careful when backporting

@apollo13

apollo13 Aug 21, 2013

Owner

Ups, nevermind; you have it in 1.5 now :/

django/contrib/sessions/serializers.py
+ return pickle.loads(data)
+
+
+class JSONSerializer(BaseJSONSerializer):
@apollo13

apollo13 Aug 21, 2013

Owner

This feels wrong, JSONSerializer = BaseJSONSerializer should work too (I know we could just import JSONSerializer from signing and use as is, but that feels wrong)

Owner

apollo13 commented Aug 21, 2013

Aside from the comments I just left it looks good to me, I'd appreciate if @dstufft and @PaulMcMillan could take a look over it and see if we missed something important.

Fixed #20922 -- Allowed customizing the serializer used by contrib.se…
…ssions

Added settings.SESSION_SERIALIZER which is the import path of a serializer
to use for sessions.
Owner

timgraham commented Aug 22, 2013

merged in 616a4d3

@timgraham timgraham closed this Aug 22, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment