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.

Sign up for GitHub

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

Jump to bottom

Fix the popitem bug in cookies #6192

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix the popitem bug in cookies #6192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ce82c34
Fix the popitem bug in cookies
kianelbo Jul 8, 2022
096f45c
Fix deletion bug of cookies' popitem
kianelbo May 8, 2023
d7d8920
Merge branch 'main' into 'cookies-popitem'
kianelbo Aug 30, 2023
8749d26
Implement 'RequestsCookieJar.popitem' independently
kianelbo Sep 2, 2023
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: 7 additions & 0 deletions src/requests/cookies.py
View file
Open in desktop
Expand Up @@ -346,6 +346,13 @@ def __delitem__(self, name):
"""
remove_cookie_by_name(self, name)

def popitem(self):
if not self:
raise KeyError("Cookie jar is empty")
next_cookie = next(iter(self))
self.clear(next_cookie.domain, next_cookie.path, next_cookie.name)
return next_cookie, next_cookie.value

def set_cookie(self, cookie, *args, **kwargs):
if (
hasattr(cookie.value, "startswith")
Expand Down
20 changes: 20 additions & 0 deletions tests/test_requests.py
View file
Open in desktop
Expand Up @@ -1364,6 +1364,26 @@ 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 •
edited

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 •
edited

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__


expected_jar = requests.cookies.RequestsCookieJar()
expected_jar.set("2nd_key", "2nd_value")
assert jar == expected_jar

jar.set("2nd_key", "2nd_value", path="another_path")
jar.set("2nd_key", "2nd_value", path="another_path", domain="another_dom")
cookie = next(iter(jar))
assert jar.popitem() == (cookie, cookie.value)
assert len(jar) == 2

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