Opened 10 years ago

Closed 9 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: UI/UX:


In #5548, leosoto pointed out that these two methods are almost identical:



These should be refactored to remove the duplication.

Attachments (2)

session-refactoring.diff (5.6 KB) - added by Tomáš Horáček 9 years ago.
Patch removes duplicity in code and corrects session documentation
ticket_5549__rev_6814.diff (4.1 KB) - added by Ben Slavin 9 years ago.
Goes beyond #6081.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 10 years ago by Adrian Holovaty

Component: Core frameworkdjango.contrib.sessions
Triage Stage: UnreviewedAccepted

comment:2 Changed 9 years ago by Tomáš Horáček

Owner: changed from nobody to Tomáš Horáček

Changed 9 years ago by Tomáš Horáček

Attachment: session-refactoring.diff added

Patch removes duplicity in code and corrects session documentation

comment:3 Changed 9 years ago by Tomáš Horáček

Has patch: set
Keywords: sprintdec01 added
Summary: Remove duplicate code in SessionBase andSessionManagerRemove duplicate code in SessionBase, SessionManager and Session

comment:4 Changed 9 years ago by Ben Slavin

Accidentally missed this and filed #6081. This is more comprehensive, but #6081 has already been checked in.

Changed 9 years ago by Ben Slavin

Attachment: ticket_5549__rev_6814.diff added

Goes beyond #6081.

comment:5 Changed 9 years ago by anonymous

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 9 years ago by anonymous

See e.g. for session invalidation that really should be present in Django.

comment:7 Changed 9 years ago by Malcolm Tredinnick

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 9 years ago by Russell Keith-Magee

Resolution: wontfix
Status: newclosed

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.

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