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

Implement filter_cookies() with domain-matching and path-matching #7777

Closed
wants to merge 0 commits into from
Closed

Implement filter_cookies() with domain-matching and path-matching #7777

wants to merge 0 commits into from

Conversation

xiangxli
Copy link
Contributor

@xiangxli xiangxli commented Nov 2, 2023

What do these changes do?

Implement filter_cookies() with domain-matching and path-matching on the keys, instead of testing every single cookie.

Are there changes in behavior for the user?

no

Related issue number

#7583

task 3

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Nov 2, 2023
Dreamsorcerer
Dreamsorcerer reviewed Nov 2, 2023
View reviewed changes
aiohttp/cookiejar.py Outdated
d = hostname
p = request_url.path

cookies = list(self._cookies[(d, p)].values())
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for looking into this. I think there's a couple of cases still missing here.

From my understanding, we need to handle subdomains. So, we probably want d and then d.split(".", maxsplit=1) and repeat until we've checked every domain level.

And then I believe the same again for paths.

So, maybe we end up with something like:

pairs = []
while d:
    p = request_url.path
    while p:
        pairs.append((d, p))
        p = p.rsplit("/", maxsplit=1)[0]
    d = d.split(".", maxsplit=1)[1]
cookies = itertools.chain.from_iterable(self._cookies[p].values() for p in pairs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am checking the cases, there are a few number of failed test cases.
Will commit the changes once done.

Dreamsorcerer
Dreamsorcerer reviewed Nov 2, 2023
View reviewed changes
aiohttp/cookiejar.py Outdated
Comment on lines 266 to 267
# shared cookie, it should have max of 1 entry
shared_cookie = self._cookies.get(("", "/"))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm just forgetting. But, is there anything in the code, or something in the spec which mentions this case?

Dreamsorcerer
Dreamsorcerer reviewed Nov 2, 2023
View reviewed changes
aiohttp/cookiejar.py Outdated Show resolved Hide resolved
Dreamsorcerer
Dreamsorcerer reviewed Nov 2, 2023
View reviewed changes
aiohttp/cookiejar.py Outdated Show resolved Hide resolved
Dreamsorcerer
Dreamsorcerer reviewed Nov 2, 2023
View reviewed changes
tests/test_cookiejar.py Outdated
jar.update_cookies(cookies)
cookies = jar.filter_cookies(URL("http://pathtest.com/"))

assert len(cookies) == 3
Copy link
Member

Choose a reason for hiding this comment

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

We should probably assert the actual values that should be in here.

We should also test with a call that includes a subdomain and a multilevel path.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest making some failing tests here, before doing the above updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I will add some failing test cases.

This was referenced Nov 3, 2023
Merged
Closed
@Dreamsorcerer Dreamsorcerer mentioned this pull request Nov 10, 2023
Open
3 tasks
@Dreamsorcerer
Copy link
Member

Still interested in finishing this PR?

@xiangxli
Copy link
Contributor Author

xiangxli commented Dec 4, 2023 •
edited
Loading

Still interested in finishing this PR?

@Dreamsorcerer
Sorry, been hold up on work. It does not seem I will have time before new year's eve. If somebody else could take it before that time it would be great.

I squeezed a few hours to finish the code, please review it when you got a change. @Dreamsorcerer

github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 4, 2023
View reviewed changes
aiohttp/helpers.py Outdated

# https://github.com/python/mypy/issues/14209
self._name = module + "." + name # type: ignore[possibly-undefined]
self._name = module + "." + name

Check failure

Code scanning / CodeQL

Potentially uninitialized local variable

Local variable 'module' may be used before it is initialized.
docs/conf.py Outdated
@@ -12,6 +12,7 @@
# All configuration values have a default; values that are commented out
# serve to show the default.

import io

Check notice

Code scanning / CodeQL

Unused import

Import of 'io' is not used.
tests/test_web_exceptions.py Outdated

def test_HTTPFound_empty_location() -> None:
with pytest.raises(ValueError):
web.HTTPFound(location="")

Check failure

Code scanning / CodeQL

Unused exception object

Instantiating an exception, but not raising it, has no effect.
tests/test_web_exceptions.py Outdated
web.HTTPFound(location="")

with pytest.raises(ValueError):
web.HTTPFound(location=None)

Check failure

Code scanning / CodeQL

Unused exception object

Instantiating an exception, but not raising it, has no effect.
tests/test_client_request.py Outdated

connector = BaseConnector()
connector = BaseConnector(loop=loop)

Check failure

Code scanning / CodeQL

Wrong name for an argument in a class instantiation

Keyword argument 'loop' is not a supported parameter name of [BaseConnector.__init__](1).
aiohttp/cookiejar.py Fixed Show fixed Hide fixed
@Dreamsorcerer
Copy link
Member

Uhh, I'm not sure what you just pushed, but now there's 150 files changed, with lots of recent changes to master being reverted...

@xiangxli
Copy link
Contributor Author

xiangxli commented Dec 4, 2023

Uhh, I'm not sure what you just pushed, but now there's 150 files changed, with lots of recent changes to master being reverted...

Could be an lagged version from master, let me fix that first.

@xiangxli xiangxli force-pushed the master branch 2 times, most recently from f36024b to 37135d2 Compare December 4, 2023 12:31
@xiangxli xiangxli closed this Dec 4, 2023
@xiangxli xiangxli mentioned this pull request Dec 4, 2023
Merged
5 tasks
@xiangxli
Copy link
Contributor Author

xiangxli commented Dec 4, 2023

PR closed by force push, continuing on the following new PR
#7944

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

@Dreamsorcerer Dreamsorcerer Dreamsorcerer left review comments

@webknjaz webknjaz Awaiting requested review from webknjaz

@asvetlov asvetlov Awaiting requested review from asvetlov

Assignees
No one assigned
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xiangxli @Dreamsorcerer