Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#18458 closed Bug (fixed)

Method session.get_expiry_age() does not return what the doc says

Reported by: martin.bouladour@… Owned by: nobody
Component: contrib.sessions Version: 1.4
Severity: Normal Keywords: session expiration expiry age get_expiry_age
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This is either a bug in the doc or in the code.

The doc for the SessionBase.get_expiry_age() method [1] says:

get_expiry_age()
Returns the number of seconds until this session expires. For sessions with no custom expiration (or those set to expire at browser close), this will equal SESSION_COOKIE_AGE.

This means that it should return the session's remaining time to live when expiration has been customized using one of these:
A) set_expiry(seconds) # integer != 0
B) set_expiry(datetime)
C) set_expiry(timedelta)

It works as documented in the cases B and C. But when set_expiry() has been used with seconds (A), then get_expiry_age() does the unexpected thing of returning the exact value that has been passed to set_expiry(). In other words, when you give 300 to set_expiry(), get_expiry_age() will always return 300. That is not “the number of seconds until this session expires”.

Looking at the code [2], it is obvious that a difference is calculated only when the expiry value is a datetime:

    def get_expiry_age(self):
        """Get the number of seconds until the session expires."""
        expiry = self.get('_session_expiry')
        if not expiry: # Checks both None and 0 cases
            return settings.SESSION_COOKIE_AGE
        if not isinstance(expiry, datetime):
            return expiry
        delta = expiry - timezone.now()
        return delta.days * 86400 + delta.seconds

I'm not sure about the purpose of this method and I don't know the design decisions that led to it, but I assume that if you don't want this function to use the session's last-modified datetime, then you may just need to edit the documentation to clarify this point (and maybe the docstring).

On the other hand, in my humble opinion, I think that this method’s behavior is quite surprising (even if it were working as documented) because the value it returns really depends on how the expiration has been defined. I believe it would be nice to make it return the number of seconds until the session expires for any active session, with no exceptional cases. And while we're at it, maybe also rename it to something that reflect better what it does, for instance "get_remaining_time_to_live" or "get_time_left". (Then again, maybe I didn't understand the purpose of this method.)

I hope this is helping.

Anyway, thank you all for this great framework!

Martin Bouladour

[1] https://docs.djangoproject.com/en/1.4/topics/http/sessions/#django.contrib.sessions.backends.base.SessionBase.get_expiry_age

[2] https://github.com/django/django/blob/1.4/django/contrib/sessions/backends/base.py#L170

Change History (4)

comment:1 Changed 3 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 2 years ago by aaugustin

  • Triage Stage changed from Design decision needed to Accepted

I recently worked on this code and came to the conclusion that get_expiry_age and get_expiry_date are designed to work at the time when the session is saved, and not at any other point in time.

get_expiry_age is used to determine the session cookie's maximum age. That probably explains the name. Since it's a public API, it's better to keep the current name, even if it isn't great.

In order to fix #18194, I may have to introduced an optional parameter to pass the "reference time" (ie. the time the session was saved at); if not provided, this parameter would default to the current time. That would resolve this issue.

Otherwise, it'll just be a documentation issue.

Last edited 2 years ago by aaugustin (previous) (diff)

comment:3 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from new to closed

In cd17a24083f3ef17cf4c40a41c9d03c250d817c6:

Added optional kwargs to get_expiry_age/date.

This change allows for cleaner tests: we can test the exact output.

Refs #18194: this change makes it possible to compute session expiry
dates at times other than when the session is saved.

Fixed #18458: the existence of the modification kwarg implies that you
must pass it to get_expiry_age/date if you call these functions outside
of a short request - response cycle (the intended use case).

comment:4 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

In 845d8408e7aa09a63c98f160b2c6bc9fa6dbb20f:

[1.5.x] Added optional kwargs to get_expiry_age/date.

This change allows for cleaner tests: we can test the exact output.

Refs #18194: this change makes it possible to compute session expiry
dates at times other than when the session is saved.

Fixed #18458: the existence of the modification kwarg implies that you
must pass it to get_expiry_age/date if you call these functions outside
of a short request - response cycle (the intended use case).

Backport of cd17a24 from master.

Note: See TracTickets for help on using tickets.
Back to Top