[Django] #32088: Django Database Sessions - Can't Retrieve expire_date
Django
--------------------------------------------+------------------------
Reporter: Nate Pinchot | Owner: nobody
Type: New feature | Status: new
Component: contrib.sessions | Version: 3.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
--------------------------------------------+------------------------
For Django database sessions, there is no way to retrieve the datetime
that the session will expire without directly querying the model. The
model stores this value in the `expire_date` field, but it is not able to
be retrieved from the session object. This makes it complicated to
implement sliding expiration sessions if the sessions are not being
modified by some other means.
Of course, we could set `SESSION_SAVE_EVERY_REQUEST = True` to cause the
session to be modified on every request, but this is inefficient.
--
Ticket URL: <https://code.djangoproject.com/ticket/32088>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Nate Pinchot | Owner: nobody
Type: New feature | Status: new
Component: contrib.sessions | Version: 3.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Pallav Parikh):
Replying to [ticket:32088 Nate Pinchot]:
> For Django database sessions, there is no way to retrieve the datetime
that the session will expire without directly querying the model. The
model stores this value in the `expire_date` field, but it is not able to
be retrieved from the session object. This makes it complicated to
implement sliding expiration sessions if the sessions are not being
modified by some other means.
>
> Of course, we could set `SESSION_SAVE_EVERY_REQUEST = True` to cause the
session to be modified on every request, but this is inefficient.
what does `session object` represent here, is it `request.session` ?
can you please specify more details.
--
Ticket URL: <https://code.djangoproject.com/ticket/32088#comment:1>
Django
Reporter: Nate Pinchot | Owner: nobody
Type: New feature | Status: new
Component: contrib.sessions | Version: 3.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Old description:
> For Django database sessions, there is no way to retrieve the datetime
> that the session will expire without directly querying the model. The
> model stores this value in the `expire_date` field, but it is not able to
> be retrieved from the session object. This makes it complicated to
> implement sliding expiration sessions if the sessions are not being
> modified by some other means.
>
> Of course, we could set `SESSION_SAVE_EVERY_REQUEST = True` to cause the
> session to be modified on every request, but this is inefficient.
New description:
For Django database sessions, there is no way to retrieve the datetime
that the session will expire without directly querying the model. The
model stores this value in the `expire_date` field, but it is not able to
be retrieved from the session (i.e. `request.session`) object. This makes
it complicated to implement sliding expiration sessions if the sessions
are not being modified by some other means.
Of course, we could set `SESSION_SAVE_EVERY_REQUEST = True` to cause the
session to be modified on every request, but this is inefficient.
Currently, to retrieve the `expire_date` from a request's session we could
do this. (Taken from the docs
https://docs.djangoproject.com/en/3.1/topics/http/sessions/#using-
sessions-out-of-views)
{{{#!python
>>> from django.contrib.sessions.models import Session
>>> s = Session.objects.get(pk='2b1189a188b44ad18c35e113ac6ceead')
>>> s.expire_date
datetime.datetime(2005, 8, 20, 13, 35, 12)
}}}
This is not ideal since it requires an extra query to the database and if
we are in the request lifecycle we already have access to the session via
`request.session`. The ideal solution would be if
`django.contrib.sessions.backends.db.SessionStore` would make
`expire_date` available since it has already queried the session database
record in the `load()` function.
--
Comment (by Nate Pinchot):
Replying to [comment:1 Pallav Parikh]:
> Replying to [ticket:32088 Nate Pinchot]:
> > For Django database sessions, there is no way to retrieve the datetime
that the session will expire without directly querying the model. The
model stores this value in the `expire_date` field, but it is not able to
be retrieved from the session object. This makes it complicated to
implement sliding expiration sessions if the sessions are not being
modified by some other means.
> >
> > Of course, we could set `SESSION_SAVE_EVERY_REQUEST = True` to cause
the session to be modified on every request, but this is inefficient.
>
> what does `session object` represent here, is it `request.session` ?
> can you please specify more details.
Correct, I am referring to `request.session`. I updated the ticket
description and added more detail.
--
Ticket URL: <https://code.djangoproject.com/ticket/32088#comment:2>
Django
(SessionStore)
-------------------------------------+-------------------------------------
Reporter: Nate Pinchot | Owner: Pallav
| Parikh
Type: New feature | Status: assigned
Component: contrib.sessions | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Pallav Parikh):
* owner: nobody => Pallav Parikh
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32088#comment:6>
Django
(SessionStore)
-------------------------------------+-------------------------------------
Reporter: Nate Pinchot | Owner: Pallav
| Parikh
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Pallav Parikh):
* version: 3.1 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/32088#comment:7>
Django
(SessionStore)
-------------------------------------+-------------------------------------
Reporter: Nate Pinchot | Owner: Pallav
| Parikh
Type: New feature | Status: assigned
Component: contrib.sessions | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Pallav Parikh):
* has_patch: 0 => 1
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/32088#comment:8>
Django
(SessionStore)
-------------------------------------+-------------------------------------
Reporter: Nate Pinchot | Owner: Pallav
| Parikh
Type: New feature | Status: assigned
Component: contrib.sessions | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nate Pinchot):
Replying to [comment:5 Pallav Parikh]:
> Let me know, if this looks okay, or any other suggestion.
>
I think this is a great concept to solve it. Thank you!
Can we amend the list of setting expire_date cases?
** setting expire_date cases:
* on every `save()` call, sets `expire_date` for current `SessionStore`
i.e `request.session` instance.
* set new `datetime` value: Valid
* set anything else except `datetime`: Invalid -> in this case, does't set
new value but preserves current `expire_date` value
* on every `load()` call, sets `expire_date` for current `SessionStore`
i.e `request.session` instance.
This additional case would set `expire_date` efficiently each time the
session is loaded, so an additional query isn't needed if the session
isn't being saved. Currently, for my case since the session is not being
saved, I would cause an additional query by referencing
`request.session.expire_date`.
I see the PR you created.
https://github.com/django/django/pull/13524/files#diff-
0804b404a59dd4551b51b9e62e5a6ca8R44
Perhaps this PR could also update the `load()` function to something
similar to below.
{{{#!python
def load(self):
s = self._get_session_from_db()
if s:
self._expire_date = s.expire_date
return self.decode(s.session_data)
return {}
}}}
I would have put the suggestion in the PR instead, but since we've been
conversing here and also because the `load()` function is currently
unchanged, I could not add a targeted comment on the PR. If it's
preferable for me to comment on the PR instead with code suggestions, let
me know :)
Let me know your thoughts on this?
--
Ticket URL: <https://code.djangoproject.com/ticket/32088#comment:9>
Django
(SessionStore)
-------------------------------------+-------------------------------------
Reporter: Nate Pinchot | Owner: Pallav
| Parikh
Component: contrib.sessions | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: assigned => closed
* resolution: => wontfix
Comment:
Hi Nate.
I'm struggling to see the use-case here.
Sessions already expose methods to get and set the expiry, and these work
perfectly well with DB backend:
{{{
>>> import datetime
>>> from django.contrib.sessions.backends.db import SessionStore
>>> session = SessionStore('some_session_key')
>>> session.get_expiry_date()
datetime.datetime(2020, 10, 27, 9, 2, 7, 300236, tzinfo=<UTC>)
>>> # Bump the expiry date. We could use any base date, but let's choose
the existing session expiry:
>>> session.set_expiry(session.get_expiry_date() + datetime.timedelta(7))
>>> session.get_expiry_date()
datetime.datetime(2020, 11, 3, 9, 10, 43, 930981, tzinfo=<UTC>)
>>> # Session will be saved with new expiry:
>>> session.modified
True
}}}
The example you link to has been in the docs since the very beginning
21c4526557fb50b9b62f32065c7d3952ea35c5df — accessing the saved date on the
model isn't really something you need to do — it's sole purpose is to act
as an invalidator in the `get()` call.
You'd be welcome to add extra code to a subclass but I can't see that it's
something that we'd want to add to the code itself.
Perhaps I'm missing something but I think we need to say `wontfix` here.
--
Ticket URL: <https://code.djangoproject.com/ticket/32088#comment:11>
Django
(SessionStore)
-------------------------------------+-------------------------------------
Reporter: Nate Pinchot | Owner: Pallav
| Parikh
Type: New feature | Status: closed
Component: contrib.sessions | Version: master
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Unreviewed
--
Ticket URL: <https://code.djangoproject.com/ticket/32088#comment:12>
Django
(SessionStore)
-------------------------------------+-------------------------------------
Reporter: Nate Pinchot | Owner: Pallav
| Parikh
Type: New feature | Status: closed
Component: contrib.sessions | Version: master
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
HI Nate, yes, thanks.
In the end I think that's something you should add to a custom backend.
There's no reason you can't store the value in load, as you suggest, but
in 15 years it's not been needed and adding complexity to the API here
isn't something I think we should do for such a niche case.
--
Ticket URL: <https://code.djangoproject.com/ticket/32088#comment:14>
Django
(SessionStore)
-------------------------------------+-------------------------------------
Reporter: Nate Pinchot | Owner: Pallav
| Parikh
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by ahmadkhalili):
* status: closed => new
* resolution: wontfix =>
Comment:
Replying to [comment:14 Carlton Gibson]:
> HI Nate, yes, thanks.
>
> In the end I think that's something you should add to a custom backend.
There's no reason you can't store the value in load, as you suggest, but
in 15 years it's not been needed and adding complexity to the API here
isn't something I think we should do for such a niche case.
>
I have this problem too, i wanted create new ticket but when i found this
ticket it's made me happy. suppose we use database backend session, my
experience to face this problem:
{{{
from datetime import datetime
from django.contrib.sessions.backends.db import SessionStore
my_session = SessionStore()
# there is no session key specified, means we want create new db session
probably
session.get_expiry_date()
# get_expiry_date() func tell us new created sessions expiration time(it
is now in django 4, 14 day), that is logical and expected
# we have specific session key means we want work on our db session and
don't want new
my_session = SessionStore('some_session_key')
# get_expiry_date() always return new created sessions expiration time (14
days after .now())
if my_session.get_expiry_date() < datetime.now(): # so this is fail
my_session.create()
...
}}}
Here in this example django force us using
`my_session._get_session_from_db().expire_date` instead
`my_session.get_expiry_date()`, but one simple question: why we should do
additional work when we determined to django 'we have specific db session
to work on it' ?
All i want say in one sentence: get_expiry_date() should return expire
date of current session but when we have not eny session yet(like
`SessionStore()`) its logical get_expiry_date() returns new created
sessions expiration time.
--
Ticket URL: <https://code.djangoproject.com/ticket/32088#comment:15>
Django
(SessionStore)
-------------------------------------+-------------------------------------
Reporter: Nate Pinchot | Owner: Pallav
| Parikh
Component: contrib.sessions | Version: dev
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: new => closed
* resolution: => wontfix
Comment:
I appreciate you'd like to reopen the ticket, but please
[https://docs.djangoproject.com/en/stable/internals/contributing/triaging-
tickets/#closing-tickets follow the triaging guidelines with regards to
wontfix tickets] and take this to DevelopersMailingList.
--
Ticket URL: <https://code.djangoproject.com/ticket/32088#comment:16>