django-user-sessions icon indicating copy to clipboard operation
django-user-sessions copied to clipboard

Added cache backend

Open arescope opened this issue 9 years ago • 17 comments

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

arescope avatar Oct 01 '14 03:10 arescope

Coverage Status

Coverage decreased (-18.67%) when pulling 2f85e629b3ca4a69e1b785aa3e374c452fa5596a on ThatGreenSpace:add-cache-backend into 105ec5cf954be934c20220ba139247a94173d1b9 on Bouke:master.

coveralls avatar Oct 01 '14 03:10 coveralls

Coverage Status

Coverage decreased (-18.42%) when pulling cd8a62a90de75e6ff42b63870425e48e920a69de on ThatGreenSpace:add-cache-backend into 105ec5cf954be934c20220ba139247a94173d1b9 on Bouke:master.

coveralls avatar Oct 01 '14 03:10 coveralls

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?

Bouke avatar Oct 01 '14 06:10 Bouke

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:

  1. Write a specific test for testing the SessionStore in tests.py
  2. Create a different tests.py file with a different settings file
  3. 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

arescope avatar Oct 01 '14 06:10 arescope

Let's see....

  1. That would mean duplication of code, rather not. Also, it would be a more limited test suite.
  2. Similar objections as 1, however the tests would be more complete.
  3. 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!

Bouke avatar Oct 02 '14 19:10 Bouke

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!

arescope avatar Oct 03 '14 01:10 arescope

Coverage Status

Coverage decreased (-18.42%) when pulling df3a231b74afaa1c77f6534452912d59065cba58 on ThatGreenSpace:add-cache-backend into 105ec5cf954be934c20220ba139247a94173d1b9 on Bouke:master.

coveralls avatar Oct 03 '14 01:10 coveralls

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?

Bouke avatar May 11 '15 06:05 Bouke

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.

peterfarrell avatar May 11 '15 07:05 peterfarrell

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?

Bouke avatar May 14 '15 07:05 Bouke

O love this package, and this is a great PR for better use of session, please, make a effort to make this unit tests.

ebertti avatar May 23 '15 05:05 ebertti

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 avatar Oct 02 '15 17:10 tarkatronic

@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 avatar Oct 03 '15 20:10 Bouke

@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.

stewartpark avatar Jan 14 '16 22:01 stewartpark

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 avatar Jan 15 '16 06:01 Bouke

@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 avatar Jan 15 '16 18:01 stewartpark

@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

mjnaderi avatar Jan 20 '18 12:01 mjnaderi