Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added cache backend #22

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

arescope
Copy link

@arescope arescope commented Oct 1, 2014

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

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@Bouke
Copy link
Collaborator

Bouke commented Oct 1, 2014

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?

@arescope
Copy link
Author

arescope commented Oct 1, 2014

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

@Bouke
Copy link
Collaborator

Bouke commented Oct 2, 2014

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!

@arescope
Copy link
Author

arescope commented Oct 3, 2014

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!

@coveralls
Copy link

Coverage Status

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

@Bouke
Copy link
Collaborator

Bouke commented May 11, 2015

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?

@peterfarrell
Copy link

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.

@Bouke
Copy link
Collaborator

Bouke commented May 14, 2015

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?

@ebertti
Copy link

ebertti commented May 23, 2015

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

@tarkatronic
Copy link

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.

@Bouke
Copy link
Collaborator

Bouke commented Oct 3, 2015

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

@stewartpark
Copy link

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

@Bouke
Copy link
Collaborator

Bouke commented Jan 15, 2016

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.

@stewartpark
Copy link

@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

@mjnaderi
Copy link

mjnaderi commented Jan 20, 2018

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants