Opened 9 years ago

Last modified 9 years ago

#23764 assigned Cleanup/optimization

sessions.backends.cache.SessionStore does not respect settings.SESSION_SERIALIZER

Reported by: Ilya Baryshev Owned by: Ravi Kotecha
Component: contrib.sessions Version: 1.7
Severity: Normal Keywords: pickle json sessions
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

According to Session serialization docs, SESSION_SERIALIZER setting, introduced in 1.5.3, allows to customize how session data is stored. JSONSerializer is used as default since 1.6.

All session backends honor this setting, except for sessions.backends.cache.SessionStore, which still uses pickle as serialization format (not directly, but via cache backend).
It should be documented that cache store ignores this setting or SessionStore should be modified to support it.

Change History (6)

comment:1 by Michael Manfre, 9 years ago

Triage Stage: UnreviewedAccepted

The cache session backend should be consistent with the other session backends.

comment:2 by Ravi Kotecha, 9 years ago

Looks like this is still the case as of django 1.8rc; I am happy to fix by making sessions.backends.cache.SessionStore respect the SESSION_SERIALIZER setting if you think that's the right approach or I can fix the documentation to show that sessions.backends.cache.SessionStore doesn't support it.

comment:3 by Ravi Kotecha, 9 years ago

Owner: changed from nobody to Ravi Kotecha
Status: newassigned

comment:4 by Sasha Romijn, 9 years ago

I would agree with manfre that this should be consistent, and therefore be fixed. However, it would be worth looking into the commit and ticket that added this setting and modified the other backends, to see whether this was perhaps intentional for some special reason.

This would be a backwards incompatible change as well: if users are running sessions with the cache backend and have not explicitly set SESSION_SERIALIZER to PickleSerializer, upgrading will cause all sessions to be invalidated. (The default value of that setting is JSONSerializer.) Despite that, I still think this change is worth making, though it will have to be included in the release notes for 1.9.

comment:5 by Ravi Kotecha, 9 years ago

I've created a PR: https://github.com/django/django/pull/4372 and I'll repeat the commnt here:
Have fixed cache.SessionStore to respect the SESSION_SERIALIZER setting.
The django.contrib.sessions.backends.cache now .encodes in the .save method and .decodes in the load method.

I'm not sure how to add regression tests for this. One option is to add this to the test.session_tests.tests.SessionTestsMixin:

    def test_save_encodes(self):
        # regression test for #23764
        session = self.backend()
        session.encode = mock.MagicMock(return_value='encoded')
        session['some key'] = 'some unencoded value'
        session.save()
        session.encode.assert_called_with({'some key': 'some unencoded value'})

but this tests for breaks tests.sessions_tests.tests.CookieSessionTests because that backend gives self.serializer to the signer instead of using it directly. I don't want to put an ugly

def test_save_encdoes(self):
  pass

in there.

comment:6 by Ravi Kotecha, 9 years ago

Has patch: set
Needs documentation: set
Note: See TracTickets for help on using tickets.
Back to Top