Opened 11 years ago
Closed 11 years ago
#5549 closed (wontfix)
Remove duplicate code in SessionBase, SessionManager and Session
Reported by: | Adrian Holovaty | Owned by: | Tomáš Horáček |
---|---|---|---|
Component: | contrib.sessions | Version: | master |
Severity: | Keywords: | sprintdec01 | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
In #5548, leosoto pointed out that these two methods are almost identical:
django.contrib.sessions.models.SessionManager.get_new_session_key
django.contrib.sessions.backends.base.SessionBase._get_new_session_key
These should be refactored to remove the duplication.
Attachments (2)
Change History (10)
comment:1 Changed 11 years ago by
Component: | Core framework → django.contrib.sessions |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 Changed 11 years ago by
Owner: | changed from nobody to Tomáš Horáček |
---|
Changed 11 years ago by
Attachment: | session-refactoring.diff added |
---|
comment:3 Changed 11 years ago by
Has patch: | set |
---|---|
Keywords: | sprintdec01 added |
Summary: | Remove duplicate code in SessionBase andSessionManager → Remove duplicate code in SessionBase, SessionManager and Session |
comment:4 Changed 11 years ago by
comment:5 Changed 11 years ago by
Also, there should be clear instructions how to clear all session data (as opposed to deleting individual keys).
request.session.clear()
seems to do it, it should be documented (I assume this is correct looking at the code -- looks like the the corresponding row will be deleted from session table if session dict is empty) or an explicit, documented method for clearing the session should be added.
Perhaps a note like the following should be added:
Use request.session.clear() if you need to clear all session data. The corresponding session row will be automatically removed from the session table when session dictionary is empty.
Test cases should be added too, in the lines of the following:
>>> request = HttpRequest >>> # attach session to HttpRequest >>> request.session["foo"] = "bar" >>> request.session.clear() >>> request.session["foo"] Traceback (most recent call last): ... KeyError: 'foo'
comment:6 Changed 11 years ago by
See e.g. http://beaker.groovie.org/beaker/browser/beaker/session.py#L174 for session invalidation that really should be present in Django.
comment:7 Changed 11 years ago by
Anonymous, please don't hijack tickets for issues of your own. Your item has nothing to do with this ticket. If you want extra functionality added, read contributing.txt for how to propose it (discussion on django-dev and then maybe a ticket).
comment:8 Changed 11 years ago by
Resolution: | → wontfix |
---|---|
Status: | new → closed |
I'm going to mark this wontfix; the original problem was resolved in [6796] as part of #6081. The subsequent patch on this ticket is a backwards incompatible change that removes the ability to easily interact with session objects directly. Although you generally shouldn't be interacting with Session objects directly, I don't see any compelling reason to disable this functionality.
Patch removes duplicity in code and corrects session documentation