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

Conversation

kianelbo
Copy link

@kianelbo kianelbo commented Jul 8, 2022

Closes #6190

@nateprewitt nateprewitt added this to the 2.29.0 milestone Nov 16, 2022
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__

@nateprewitt nateprewitt modified the milestones: 2.29.0, 2.30.0 Apr 25, 2023
@nateprewitt nateprewitt removed this from the 2.30.0 milestone May 22, 2023
@nateprewitt nateprewitt added this to the 2.32.0 milestone Aug 13, 2023
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
4 participants