Skip to content

Make RequestsCookieJar.popitem() work#7513

Open
vineethsaivs wants to merge 2 commits into
psf:mainfrom
vineethsaivs:fix/cookiejar-popitem
Open

Make RequestsCookieJar.popitem() work#7513
vineethsaivs wants to merge 2 commits into
psf:mainfrom
vineethsaivs:fix/cookiejar-popitem

Conversation

@vineethsaivs

Copy link
Copy Markdown

Closes #6190

RequestsCookieJar subclasses both CookieJar and MutableMapping[str, str | None]. Its __iter__ is inherited from CookieJar and yields Cookie objects rather than names. The popitem() it inherited from MutableMapping does roughly:

key = next(iter(self))   # a Cookie object, not a name
value = self[key]        # __getitem__ looks up by name -> KeyError

so popitem() always raised KeyError, even on a non-empty jar:

>>> jar = requests.cookies.RequestsCookieJar()
>>> jar.set("a", "1"); jar.set("b", "2")
>>> jar.popitem()
KeyError: "name=Cookie(version=0, name='a', ...), domain=None, path=None"

This overrides popitem() to remove and return a (name, value) pair using the jar's existing dict-like view (iteritems()), and to raise KeyError only when the jar is empty, matching the documented behavior and the MutableMapping[str, str | None] contract.

Added a regression test (test_cookie_popitem) that pops both entries, checks each returned pair is a real (name, value) from the jar, and confirms a KeyError on the empty jar. It fails on main and passes with this change.

RequestsCookieJar subclasses both CookieJar and MutableMapping. Its __iter__
comes from CookieJar and yields Cookie objects rather than names, so the
popitem() inherited from MutableMapping did `self[next(iter(self))]`, i.e.
looked a cookie up by a Cookie object, and always raised KeyError even when the
jar was non-empty.

Override popitem() to remove and return a (name, value) pair using the jar's
dict-like view, and raise KeyError only when the jar is empty, matching the
documented behavior.

@avinashkamat48 avinashkamat48 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

popitem() removes by cookie name only (del self[name]) after reading the first (name, value) pair. That can remove more than the single cookie returned when the jar contains multiple cookies with the same name on different domains/paths, or hit the existing duplicate-name conflict behavior. Please add a test with two cookies sharing a name but differing by domain/path and remove the exact cookie object selected by the iterator rather than deleting by name alone.

popitem() read the first (name, value) pair from the iterator but then removed
the cookie with `del self[name]`, which deletes every cookie sharing that name
across other domains or paths. With two cookies of the same name on different
domains, that dropped both even though popitem() reported removing only one.

Select the cookie via the iterator and remove that exact object with
clear(domain, path, name), so only the returned cookie is discarded. Add a
regression test covering two same-named cookies on different domains.
@vineethsaivs

Copy link
Copy Markdown
Author

Good catch, fixed. popitem() now selects the cookie via the iterator and removes that exact object with clear(cookie.domain, cookie.path, cookie.name), so it can no longer drop other cookies that happen to share the name on a different domain or path. Added a regression test (test_cookie_popitem_removes_only_the_selected_cookie) that puts two cookies named dup on different domains and asserts popitem() removes exactly the one it returns and leaves the other intact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

requests.cookies.RequestsCookieJar: popitem() does not work

2 participants