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

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

Open
fireattack opened this issue Jul 5, 2022 · 6 comments · May be fixed by #6192
Open

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

fireattack opened this issue Jul 5, 2022 · 6 comments · May be fixed by #6192

Comments

@fireattack
Copy link

fireattack commented Jul 5, 2022 •
edited

requests.cookies.RequestsCookieJar's popitem() method doesn't seem to work even if cookies is not empty.

Expected Result

From the doc:

remove and return some (key, value) pair as a 2-tuple; but raise KeyError if D is empty.

(Also, I'm not sure what exactly is "D" here. I assume it means the cookies obj itself.)

Actual Result

It always raises KeyError even when it's not empty.

Reproduction Steps

import requests

r = requests.get('https://google.com')
print(len(r.cookies)) # = 3

r.cookies.popitem()

System Information

$ python -m requests.help

{
  "chardet": {
    "version": "4.0.0"
  },
  "charset_normalizer": {
    "version": "2.0.10"
  },
  "cryptography": {
    "version": "3.4.8"
  },
  "idna": {
    "version": "2.10"
  },
  "implementation": {
    "name": "CPython",
    "version": "3.9.1"
  },
  "platform": {
    "release": "10",
    "system": "Windows"
  },
  "pyOpenSSL": {
    "openssl_version": "101010cf",
    "version": "20.0.1"
  },
  "requests": {
    "version": "2.28.1"
  },
  "system_ssl": {
    "version": "1010107f"
  },
  "urllib3": {
    "version": "1.26.3"
  },
  "using_charset_normalizer": false,
  "using_pyopenssl": true
}
@fireattack fireattack changed the title requests.cookies.popitem() does not work requests.cookies.RequestsCookieJar: popitem() does not work Jul 5, 2022
@kianelbo kianelbo linked a pull request Jul 8, 2022 that will close this issue
Open
@kianelbo
Copy link

kianelbo commented Jul 8, 2022

I found the issue and hopefully fixed it but I couldn't find the documentation entry, perhaps because popitem is not implemented explicitly in the code. Is there any way to improve the doc?

@sigmavirus24
Copy link
Contributor

Correct @kianelbo support was not intended for that method but we can't use the mapping interface without it. To be fair, exposing cookies as if they're a simple mapping was a huge design mistake. Unfortunately we're here now so I think we have to fix this unless @nateprewitt or @sethmlarson agree that we should fix the cookies design in some other backwards compatible fashion

@nateprewitt
Copy link
Member

Yeah, I agree we should just fix it. Long term, I'm in favor of us reworking cookies entirely, as we've discussed before.

For the moment, this seems reasonably straight forward to get the inherited methods working. Can we get a quick manual pass to verify this fixes the others that come through mapping?

@kianelbo
Copy link

@nateprewitt do you mean just making the inherited methods work for now? They're currently ok other than popitem which the PR fixes.

@nateprewitt
Copy link
Member

Thanks for confirming, @kianelbo. I have your PR on the shortlist for review this week and we'll look at getting this change added.

@kianelbo
Copy link

Hi. Any plans for reviewing the PR? I was really excited to see it merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix the popitem bug in cookies kianelbo/requests
4 participants
@sigmavirus24 @fireattack @nateprewitt @kianelbo