Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Session 'initializer': Fixed Callable case of _initializer() in web.session.Session _load() #193

Open
wants to merge 2 commits into
from

Conversation

Projects
None yet
1 participant

in web.session.Session _load()

TLDR; Fixed _initialize for callable case. If initializer kwarg present, allow _initializer() to return dict which can be used to update self, i.e. for Session (assuming _initializer returns a dict) self.update(deepcopy(_initializer()))

Long explanation:
Updated Session._load(self), within the logic for self._initializer (determining if _initializer is present, and if so whether it is a dict versus callable func). If _initializer is callable, right now there is no way to update the Session's attributes (like one could do if they just provide initialize=some_dictionary). One bad solution is to require _initializer take an argument of self. The better solution (implemented here) is for _load() to allow/expect the callable case of _initializer to return a dictionary which can be used to update the Session's attributes, i.e. self.update(_initializer())

Updated Session._load(self), within the logic for self._initializer (…
…determining if _initializer is present, and if so wehther it is a dict versus callable func). If _initializer is callable, right now there is no way to update the Session's attributes (like one could do if they just provide initialize=some_dictionary). One bad solution is to require _initializer take an argument of self. The better solution (implemented here) is for _load() to allow/expect the callable case of _initializer to return a dictionary which can be used to update the Session's attributes, i.e. self.update(_initializer())

TLDR;

Extend the capability of web.session.Session initializer kwarg such that, if the value passed as initializer is a __call__able function, it would be allowed for this function to return (exclusively) a dictionary which would be used to update the sessions' values.

Problem

Let's assume we want to create a new session and have a segment of code which looks like this, where default_session is a __call__able function:

session = web.session.Session(app, storage_method, initializer=default_session)

The problem with the current implementation is, default_session will be successfully invoked (as is already implemented; see code snippet below), however, _initializer has no reference to 'self' (self being a reference to this instance of web.session.Session) and thus, scope-wise, default_session() has no means of dict .update'ing the values within the session.

This current incomplete implementation is handled within webpy on line 117, within web.session.Session_load:

if self._initializer:
     if isinstance(self._initializer, dict):
         self.update(deepcopy(self._initializer))
     elif hasattr(self._initializer, '__call__'):
         self._initializer() # <---- Here

Solutions

There are two main solutions to this problem.

Solution 1

The first is, it could be made a required that, in order for the web.session.Session initializer kwarg to accept a callable (function), rather than a dict, that function would have to take a session parameter which is a reference to it's corresponding web.session.Session instance. This would require the self._initializer invocation within the _load method of web.session.Session_load to take a single argument which is a reference to 'self' (aka the session instance). With this solution, web.session.Session._load would change in the following way:

if self._initializer:
     if isinstance(self._initializer, dict):
         self.update(deepcopy(self._initializer))
     elif hasattr(self._initializer, '__call__'):
         self._initializer(self) # <---- Here

example solution 1

Assuming web.session._load were updated as per above, default_session would then accept an argument (we can call it sess) for which the value 'self' (a reference to the session) is passed in, allowing us to perform a dict update on the session.

def default_session(sess):
    """sess references 'self' within web.session.Session"""
    sess.update({'logged': False})

session = web.session.Session(app, storage_method, initializer=default_session)

This solution is not preferred (and not the subject of this pull request) as it forces some users to modify their existing applications. The preferred solution would be a progressive enhancement which requires no adjustment for current users, however offers an optional additional benefit which didn't exist before to users who wish to implement it. Hence solution number 2.

Solution 2

The second (and in my humble opinion better) solution is, if web.session.Session is provided a __call__able function for the initializer kwarg and happens to returns a dictionary, we could safely assume the desired behavior is to update self (the session) with this dictionary value. This doesn't require any modification to how people use sessions (whereas solution 1 does) and causes no regressions, whereas it would now provide an additional benefit of allowing functionality which was previously challenging to achieve without black magic.

example solution 2

def default_session():
    """session will be .update'd() with returned dict"""
    return {'logged': False}

session = web.session.Session(app, storage_method, initializer=default_session)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment