Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#26520 closed Bug (fixed)

SessionBase.pop() no longer raises a KeyError

Reported by: Tobias Krönke Owned by: nobody
Component: contrib.sessions Version: 1.9
Severity: Release blocker Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by Tim Graham)

The fix of #24621 led to always providing a default to session's dict's pop() method. This prevents a KeyError from ever being raised, which should be expected by a pop-function.

Pull request is https://github.com/django/django/pull/6480

Change History (12)

comment:1 Changed 7 years ago by Tobias Krönke

comment:2 Changed 7 years ago by Tobias Krönke

Description: modified (diff)

comment:3 Changed 7 years ago by Markus Holtermann

While I agree that .pop() should raise a KeyError on a missing key I don't think reverting that particular commit is the solution. We should rather fix the underlying bug than what covers it up.

comment:4 Changed 7 years ago by Tobias Krönke

Thanks for your feedback. Do you just propose to further change the docs or also even the code? I think the session.pop wrapper should have a signature as close to the original dict.pop as possible:

>>> {}.pop("key", default="fallback")
Traceback (most recent call last):
  File "<bpython-input-6>", line 2, in <module>
    {}.pop("key", default="fallback")
TypeError: pop() takes no keyword arguments
Last edited 7 years ago by Tobias Krönke (previous) (diff)

comment:5 Changed 7 years ago by Nicolas Noé

dict.pop raise KeyError if the key is not found AND there's no default provided as second argument... Won't implementing the same in session.pop solve both this issue and #24621 (given that the documentation is updated, too) ?

Example:

>>> {}.pop('machin')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
KeyError: 'machin'
>>> {}.pop('machin', 'truc')
'truc'
Last edited 7 years ago by Tim Graham (previous) (diff)

comment:6 Changed 7 years ago by Tim Graham

Patch needs improvement: set
Severity: NormalRelease blocker
Summary: Undo #24621 to make session.pop raise a KeyError againSessionBase.pop() no longer raises a KeyError
Triage Stage: UnreviewedAccepted

How about using a pattern borrowed from cpython?

__marker = object()

def pop(self, key, default=__marker):
    self.modified = self.modified or key in self._session
    args = () if default is self.__marker else (default,)
    return self._session.pop(key, *args)

comment:7 Changed 7 years ago by Tobias Krönke

Maybe, Django users out there are already using the named argument default, so it would be bad to remove it again. Although the solution is more complex in code, it seems more explicit for documentation and code introspection. Either way, I am happy, that it will raise a KeyError again.

comment:8 Changed 7 years ago by Nicolas Noé

I am currently trying timgraham's suggested implementation, and it seems to work well for all those cases:

(Pdb) session.pop('machin')
*** KeyError: 'machin'
(Pdb) session.pop('machin', 'truc')
'truc'
(Pdb) session.pop('machin', default='truc')
'truc'

Should I make a pull request with this?

comment:9 Changed 7 years ago by Tobias Krönke

I already changed my pull request to reflect timgraham's idea.

comment:10 Changed 7 years ago by Tim Graham

Description: modified (diff)
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:11 Changed 7 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In b040ac06:

Fixed #26520 -- Fixed a regression where SessionBase.pop() didn't return a KeyError.

comment:12 Changed 7 years ago by Tim Graham <timograham@…>

In 845d43e3:

[1.9.x] Fixed #26520 -- Fixed a regression where SessionBase.pop() didn't return a KeyError.

Backport of b040ac06ebba2348cece7390b88f746d2c91d07b from master

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