Opened 17 years ago
Closed 17 years ago
#4729 closed (fixed)
SessionWrapper.pop does not set modified flag
Reported by: | Owned by: | Adrian Holovaty | |
---|---|---|---|
Component: | Contrib apps | Version: | dev |
Severity: | Keywords: | ||
Cc: | mir@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
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)
Change History (19)
by , 17 years ago
Attachment: | sessionwrapper_pop_modified.patch added |
---|
comment:1 by , 17 years ago
Needs tests: | set |
---|---|
Triage Stage: | Unreviewed → Design 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 by , 17 years ago
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.
by , 17 years ago
Attachment: | sessionwrapper_pop_modified.2.patch added |
---|
Sets SessionWrapper.modified when SessionWrapper.pop finds an item
comment:4 by , 17 years ago
Collin... bah! ;-)
I've attached a new patch with self.modified = self.modified or key in self._session
comment:6 by , 17 years ago
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.
comment:7 by , 17 years ago
Needs tests: | unset |
---|---|
Triage Stage: | Design decision needed → 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
comment:8 by , 17 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
by , 17 years ago
Attachment: | ticket_4729__rev_6373.diff added |
---|
Updated to new session implementation
comment:9 by , 17 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
[6333] reintroduced this issue.
Patch has been included.
comment:10 by , 17 years ago
Cc: | added |
---|
comment:11 by , 17 years ago
Needs tests: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
The tests also need some alteration so that there is a failure seen before the application of this patch.
by , 17 years ago
Attachment: | ticket_4729__rev_6447.diff added |
---|
Patch for new session implementation, including tests
comment:12 by , 17 years ago
Needs tests: | unset |
---|---|
Triage Stage: | Accepted → 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).
comment:14 by , 17 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Sets SessionWrapper.modified when SessionWrapper.pop is called