Support for (optionally) declaring the Pickle Protocol when writing to session store

[sayeghr]

Added support for explicitly declaring the Pickle Protocol. This is to support sharing a session between flask apps using different python versions (2 or 3).

This pull request is to fix a specific problem when you have two flask apps, one written in Python 2 and one written in Python 3 that shares a server-side session. Python 3 uses Pickle Protocol 3 by default, but Python 2 can only read from Pickle Protocol 2 or less.

[mbr]

Currently, the serialization code is supposed to be protocol-agnostic (you can swap it out with JSON or anything else), adding this configuration option would change that slightly and it is not a very common usecase.

Before merging this, have you looked at the alternative? Something like (untested code below, I hope it gets the idea across):

class SessionPickler(object):
    def __init__(self, protocol_version=2):
        self.protocol_version = protocol_version

    def loads(self, string):
        return pickle.loads(string)

    def dumps(self, obj):
        return pickle.dumps(obj, self.protocol_version)

KVSessionInterface.serialization_method = SessionPickler(2)

If you felt like it, you could get clever with inheritance and functools.partials =)

[sayeghr]

The KVSessionInterface class has the serialization_method hard coded with pickle, so I think it would be OK to have an optional way to pass in the pickling protocol you wish to use. If you want to use JSON instead you could simply omit the SESSION_PICKLE_PROTOCOL key from your flask config with no ill effects. Your proposed alternative solution would require monkey patching the KVSessionInterface at runtime which feels evil to me.

[mbr]

The KVSessionInterface class has the serialization_method hard coded with pickle, so I think it would be OK to have an optional way to pass in the pickling protocol you wish to use.

I would disagree here, it's not hardcoded but a class variable. To me, hardcoded would be if I was calling pickle.dumps instead of self.serialization_method.dumps, for example.

The pattern is used across flask as well, have a look at https://github.com/pallets/flask/blob/c5900a1adf8e868eca745225f3cf32218cdbbb23/flask/sessions.py#L290:

class SecureCookieSessionInterface(SessionInterface):
    """The default session interface that stores sessions in signed cookies
    through the :mod:`itsdangerous` module.
    """
    #: the salt that should be applied on top of the secret key for the
    #: signing of cookie based sessions.
    salt = 'cookie-session'
    #: the hash function to use for the signature.  The default is sha1
    digest_method = staticmethod(hashlib.sha1)
    #: the name of the itsdangerous supported key derivation.  The default
    #: is hmac.
    key_derivation = 'hmac'
    #: A python serializer for the payload.  The default is a compact
    #: JSON derived serializer with support for some extra Python types
    #: such as datetime objects or tuples.
    serializer = session_json_serializer
    session_class = SecureCookieSession

salt, digest_method, key_derivation, serializer and session_class are all overridable class variables, for this reason they are also well documented. The latter not being the based for Flask-KVSession is probably a shortcoming here.

[sayeghr]

Okay, I agree with you but it would be nice to be able to set the pickling protocol within your library without having to create your own pickler subclass. Especially since your default implementation uses pickle and already accepts optional variables from the flask app configuration.

I will implement the pickler subclass in my codebase if you do not want to accept this pull request.

[sayeghr]

Is there anything I can add to this PR to make you more willing to accept it? One option would be to check if the serialization_method is of the pickle class before respecting the SESSION_PICKLE_PROTOCOL config.