Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the popitem bug in cookies #6192

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions requests/cookies.py
Expand Up @@ -10,6 +10,7 @@
import calendar
import copy
import time
from http.cookiejar import Cookie

from ._internal_utils import to_native_string
from .compat import Morsel, MutableMapping, cookielib, urlparse, urlunparse
Expand Down Expand Up @@ -324,14 +325,16 @@ def __contains__(self, name):
except CookieConflictError:
return True

def __getitem__(self, name):
def __getitem__(self, key):
"""Dict-like __getitem__() for compatibility with client code. Throws
exception if there are more than one cookie with name. In that case,
use the more explicit get() method instead.

.. warning:: operation is O(n), not O(1).
"""
return self._find_no_duplicates(name)
if isinstance(key, Cookie):
key = key.name
return self._find_no_duplicates(key)

def __setitem__(self, name, value):
"""Dict-like __setitem__ for compatibility with client code. Throws
Expand Down
10 changes: 10 additions & 0 deletions tests/test_requests.py
Expand Up @@ -1350,6 +1350,16 @@ def test_cookie_duplicate_names_raises_cookie_conflict_error(self):
with pytest.raises(requests.cookies.CookieConflictError):
jar.get(key)

def test_cookies_popitem(self):
jar = requests.cookies.RequestsCookieJar()
with pytest.raises(KeyError):
jar.popitem()

jar.set("1st_key", "1st_value")
jar.set("2nd_key", "2nd_value")
cookie = next(iter(jar))
assert jar.popitem() == (cookie, cookie.value)
Copy link
Contributor

@graingert graingert Apr 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add an assertion of what the jar looks like afterwards, I think __delitem__ needs a fix also?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a test for when cookie.name is not unique in the jar, eg different values for cookie.domain and cookie.path

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call @graingert , __delitem__ had the same problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like popitem will delete multiple items instead of just one, can you add a test for that?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but it's because of remove_cookie_by_name. I don't know what can be done about it without breaking its main functionality.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one has been stuck for a long time. Do you have any suggestions for the remove_cookie_by_name problem?

Copy link
Contributor

@graingert graingert Aug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can implement popitem directly instead of editing __delitem__ and __getitem__


def test_cookie_policy_copy(self):
class MyCookiePolicy(cookielib.DefaultCookiePolicy):
pass
Expand Down