django-user-sessions
django-user-sessions copied to clipboard
Added cache backend
Hi,
I added the cache backend for users who use the cache as session backend. It could be a lil bit refactor with the db backend.
It would be nice if you can merge it :)
Best regards,
Alberto
Coverage decreased (-18.67%) when pulling 2f85e629b3ca4a69e1b785aa3e374c452fa5596a on ThatGreenSpace:add-cache-backend into 105ec5cf954be934c20220ba139247a94173d1b9 on Bouke:master.
Coverage decreased (-18.42%) when pulling cd8a62a90de75e6ff42b63870425e48e920a69de on ThatGreenSpace:add-cache-backend into 105ec5cf954be934c20220ba139247a94173d1b9 on Bouke:master.
Hi @arescope it looks like a nice addition. Did you base your work on that of Django's default cache backend? In order to merge it, it'll need some documentation and unit tests. Can you also provide those?
Yes I am using redis as cache backend so it would be nice to have it merged :).
Yes sure, I can do it during this week. Actually I was about to do it, but I prefer to talk with you with that before.
Currently your tests are tied to the db backend, for instance you use Session
model in some part of the tests and the db backend is provided in the test settings.py
Would you prefer to:
- Write a specific test for testing the
SessionStore
intests.py
- Create a different tests.py file with a different settings file
- Adapt all the files to make it working in both backends and run twice, one for each backend.
I am fine with all solutions, but let me know which one you feel more comfortable :)
Best regards,
Alberto
Let's see....
- That would mean duplication of code, rather not. Also, it would be a more limited test suite.
- Similar objections as 1, however the tests would be more complete.
- Same test scenarios, less duplication. Might need a
@skipIf
for DB-specific test cases.
So I think 3 is the most straight-forward solution and provides the best all-round test scenarios. Using a flag to set either cache backend, it would be possible to extend the test matrix of tox and travis. Thanks!
Yup,
I also thought that. I cannot give you an ETA for do it because now I am quite busy with my project, but will try as soon as possible. I want this merged :-).
Thanks!
Coverage decreased (-18.42%) when pulling df3a231b74afaa1c77f6534452912d59065cba58 on ThatGreenSpace:add-cache-backend into 105ec5cf954be934c20220ba139247a94173d1b9 on Bouke:master.
What would be the added benefit of using this cache backend vs. the Django's cache backend? This project's selling point is that one can run a query over all sessions (in the database). However if a cache backend is used, no sessions can be run on them, right? So why not use Django's cache backend?
The main class should probably be rebased django.contrib.sessions.cache
on instead of SessionBase
as it currently is in the PR
You cannot just use Django's session cache directly because the __init__
method won't accept the additional kwargs of user_agent and ip, and therefore throws and exception. See this __init__
method signature:
https://github.com/django/django/blob/master/django/contrib/sessions/backends/cache.py#L13
So some overrides are needed and a custom class is required if you want to use django-user-sessions with cached based sessions. That is why there is a PR. However, some refactoring on the right parent class would simplify the PR. Only __init__
, load
, save
and clear
need to be implemented. The rest is duplication from the sessions cache class.
The goal of this package is to provide Session models as first-class ORM models. This allows to query the session store (database) by user, to see all (in)active sessions by a user. Or to list all users with an active session. Using a cache backend, I cannot see how these features can be provided?
O love this package, and this is a great PR for better use of session, please, make a effort to make this unit tests.
Is there likely to be any more movement on this PR? Is it worth resurrecting the work on here, or would it be best to start a new fork? I'm very interested in the functionality of this package, but I need the speed of a cache based session backend, as opposed to database backed.
@tarkatronic I would be happy to merge useful session backends into this package, however I'm not convinced adding a cache backend is in line with this package's goal. See my earlier remarks above.
@Bouke Since you mentioned it's not one of the main goals... IMO, this should be one of the priorities. If your goal is to provide an alternative middleware & all other session infrastructures as opposed to the Django's default one, I think it's essential to have these kinds of options in backend for django-user-sessions to be used on production. If there are a fairly large number of active users that are using the product, I'm concerned that this can be a serious performance issue. Which is the reason why I deferred using this...
I can continue this work if the original PR author is absent. Tell me what you think.
I'm still not seeing how a cache backend can be queried for a list of all sessions, or user's sessions, as I challenged the original submitter before (without nasty hacks). So if I'm missing something, please enlighten me. Otherwise working on the PR would be a waste of time.
@Bouke oh, no. I haven't looked into how the current PR is implemented, I'm talking about its necessity in general since you explicitly mentioned that it's not one of the main goals.
I'm talking about the equivalent of cache
or cached_db
, which I was looking for originally. I believe this should be one of the priorities.
Since this library has the db backend, cached_db should be fairly easy to implement.
https://github.com/django/django/blob/master/django/contrib/sessions/backends/cached_db.py
@stewartpark @Bouke We have implemented a customized cached_db
backend for this purpose. Sessions have a foreign key to User and store user agent and IP.
https://github.com/QueraTeam/django-qsessions