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

[fireattack]

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
}

[kianelbo]

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]

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]

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]

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

[nateprewitt]

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]

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