Opened 9 years ago

Closed 9 years ago

#4729 closed (fixed)

SessionWrapper.pop does not set modified flag

Reported by: Ben Slavin <benjamin.slavin@…> Owned by: Adrian Holovaty
Component: Contrib apps Version: master
Severity: Keywords:
Cc: mir@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

In #4338 a pop method was added to the SessionWrapper class.

This method alters the state of the underlying Session object but does not set the modified flag. This prevents the change from being persisted to the database unless SESSION_SAVE_EVERY_REQUEST is set or another modification is made to the Session.

This bug was introduced in [5306].

Attachments (5)

sessionwrapper_pop_modified.patch (438 bytes) - added by Ben Slavin <benjamin.slavin@…> 9 years ago.
Sets SessionWrapper.modified when SessionWrapper.pop is called
sessionwrapper_pop_modified.2.patch (471 bytes) - added by Ben Slavin <benjamin.slavin@…> 9 years ago.
Sets SessionWrapper.modified when SessionWrapper.pop finds an item
sessionwrapper_pop_modified.3.patch (1.1 KB) - added by Chris Beaven 9 years ago.
with tests
ticket_4729__rev_6373.diff (480 bytes) - added by Ben Slavin 9 years ago.
Updated to new session implementation
ticket_4729__rev_6447.diff (1.7 KB) - added by Ben Slavin 9 years ago.
Patch for new session implementation, including tests

Download all attachments as: .zip

Change History (19)

Changed 9 years ago by Ben Slavin <benjamin.slavin@…>

Sets SessionWrapper.modified when SessionWrapper.pop is called

comment:1 Changed 9 years ago by Chris Beaven

Needs tests: set
Triage Stage: UnreviewedDesign decision needed

Well, technically pop() doesn't guarantee that we are modifying the session. .pop('nonexistantkey', None) isn't doing any modification (just like get() isn't setting self.accessed).

Perhaps it would be better if SessionWrapper._session_cache was a subclass of dict which handled .accessed and .modified` itself...

comment:2 Changed 9 years ago by Ben Slavin <benjamin.slavin@…>

You're right that .pop() doesn't guarantee modification. This was something that I had considered, but I think it's better to err on the side of safety. I do like your suggestion of subclassing dict, but I don't know if it's necessary.

Re: your comment about .accessed... this actually isn't true. .get() doesn't need to modify .accessed since ._get_session() handles that already.

comment:3 Changed 9 years ago by Collin Grady <cgrady@…>

self.modified = key in self._session ? :)

Changed 9 years ago by Ben Slavin <benjamin.slavin@…>

Sets SessionWrapper.modified when SessionWrapper.pop finds an item

comment:4 Changed 9 years ago by Ben Slavin <benjamin.slavin@…>

Collin... bah! ;-)

I've attached a new patch with self.modified = self.modified or key in self._session

comment:5 Changed 9 years ago by Chris Beaven

Is it worth fixing .get in the same ticket or should we open a new one?

comment:6 Changed 9 years ago by Ben Slavin <benjamin.slavin@…>

If we decide to change .get we actually have to make a number of changes... __getitem__, keys, items, get. We'd also have to change the functionality of _get_session and would require anyone accessing ._session elsewhere to manually set .accessed (I don't know if this is done in practice anywhere).

Because of all the items touched, I think that it might be better to open another ticket, but I'm not convinced that the current behavior of ._get_session is problematic.

Changed 9 years ago by Chris Beaven

with tests

comment:7 Changed 9 years ago by Chris Beaven

Needs tests: unset
Triage Stage: Design decision neededReady for checkin

Actually, I was getting confused. .get does set .accessed, since it uses the ._session property.

There's not much to decide upon in this ticket, it's an obvious problem. I'm promoting to checkin

comment:8 Changed 9 years ago by Adrian Holovaty

Resolution: fixed
Status: newclosed

(In [5592]) Fixed #4729 -- SessionWrapper.pop now sets modified flag if necessary. Thanks, Ben Slavin, SmileyChris and Collin Grady

Changed 9 years ago by Ben Slavin

Attachment: ticket_4729__rev_6373.diff added

Updated to new session implementation

comment:9 Changed 9 years ago by Ben Slavin

Resolution: fixed
Status: closedreopened

[6333] reintroduced this issue.

Patch has been included.

comment:10 Changed 9 years ago by Michael Radziej

Cc: mir@… added

comment:11 Changed 9 years ago by Gary Wilson

Needs tests: set
Triage Stage: Ready for checkinAccepted

The tests also need some alteration so that there is a failure seen before the application of this patch.

Changed 9 years ago by Ben Slavin

Attachment: ticket_4729__rev_6447.diff added

Patch for new session implementation, including tests

comment:12 Changed 9 years ago by Ben Slavin

Needs tests: unset
Triage Stage: AcceptedReady for checkin

Attached a new patch.

Promoting to 'ready for checkin' because the tests existed prior to [6333] but were erroneously deleted in the new session implementation (which is why this problem reappeared).

comment:13 Changed 9 years ago by Thomas Güttler <hv@…>

Tests look good. Please commit.

comment:14 Changed 9 years ago by Malcolm Tredinnick

Resolution: fixed
Status: reopenedclosed

(In [6558]) Fixed #4729 -- Restored functionality to the Session class so that popping a
value marks it as modified. This was accidentally lost in the changes in
[6333]. Thanks, hawkeye.

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