Django

Code

Ticket #4729 (closed: fixed)

Opened 1 year ago

Last modified 1 year ago

SessionWrapper.pop does not set modified flag

Reported by: Ben Slavin <benjamin.slavin@gmail.com> Assigned to: adrian
Milestone: Component: Contrib apps
Version: SVN Keywords:
Cc: mir@noris.de Triage Stage: Ready for checkin
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

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

sessionwrapper_pop_modified.patch (438 bytes) - added by Ben Slavin <benjamin.slavin@gmail.com> on 06/29/07 13:24:02.
Sets SessionWrapper?.modified when SessionWrapper?.pop is called
sessionwrapper_pop_modified.2.patch (471 bytes) - added by Ben Slavin <benjamin.slavin@gmail.com> on 06/29/07 17:02:07.
Sets SessionWrapper.modified when SessionWrapper.pop finds an item
sessionwrapper_pop_modified.3.patch (1.1 kB) - added by SmileyChris on 06/29/07 18:07:00.
with tests
ticket_4729__rev_6373.diff (480 bytes) - added by __hawkeye__ on 09/18/07 16:47:40.
Updated to new session implementation
ticket_4729__rev_6447.diff (1.7 kB) - added by __hawkeye__ on 10/03/07 09:45:40.
Patch for new session implementation, including tests

Change History

06/29/07 13:24:02 changed by Ben Slavin <benjamin.slavin@gmail.com>

  • attachment sessionwrapper_pop_modified.patch added.

Sets SessionWrapper?.modified when SessionWrapper?.pop is called

06/29/07 16:13:08 changed by SmileyChris

  • needs_better_patch changed.
  • stage changed from Unreviewed to Design decision needed.
  • needs_tests set to 1.
  • needs_docs changed.

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...

06/29/07 16:37:52 changed by Ben Slavin <benjamin.slavin@gmail.com>

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.

06/29/07 16:52:24 changed by Collin Grady <cgrady@the-magi.us>

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

06/29/07 17:02:07 changed by Ben Slavin <benjamin.slavin@gmail.com>

  • attachment sessionwrapper_pop_modified.2.patch added.

Sets SessionWrapper.modified when SessionWrapper.pop finds an item

06/29/07 17:03:44 changed by Ben Slavin <benjamin.slavin@gmail.com>

Collin... bah! ;-)

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

06/29/07 17:07:14 changed by SmileyChris

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

06/29/07 17:14:35 changed by Ben Slavin <benjamin.slavin@gmail.com>

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.

06/29/07 18:07:00 changed by SmileyChris

  • attachment sessionwrapper_pop_modified.3.patch added.

with tests

06/29/07 18:09:04 changed by SmileyChris

  • needs_tests deleted.
  • stage changed from Design decision needed to Ready 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

07/03/07 10:02:40 changed by adrian

  • status changed from new to closed.
  • resolution set to fixed.

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

09/18/07 16:47:40 changed by __hawkeye__

  • attachment ticket_4729__rev_6373.diff added.

Updated to new session implementation

09/18/07 16:51:08 changed by __hawkeye__

  • status changed from closed to reopened.
  • resolution deleted.

[6333] reintroduced this issue.

Patch has been included.

09/20/07 15:02:45 changed by mir

  • cc set to mir@noris.de.

09/27/07 12:20:14 changed by gwilson

  • needs_tests set to 1.
  • stage changed from Ready for checkin to Accepted.

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

10/03/07 09:45:40 changed by __hawkeye__

  • attachment ticket_4729__rev_6447.diff added.

Patch for new session implementation, including tests

10/03/07 09:47:58 changed by __hawkeye__

  • needs_tests deleted.
  • stage changed from Accepted to Ready 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).

10/04/07 10:10:59 changed by Thomas Güttler <hv@tbz-pariv.de>

Tests look good. Please commit.

10/20/07 05:13:00 changed by mtredinnick

  • status changed from reopened to closed.
  • resolution set to fixed.

(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.

Add/Change #4729 (SessionWrapper.pop does not set modified flag)




Change Properties
Action