Skip to content

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

Closed
xiangxli wants to merge 0 commit intoaio-libs:masterfrom
xiangxli:master
Closed

Implement filter_cookies() with domain-matching and path-matching#7777
xiangxli wants to merge 0 commit intoaio-libs:masterfrom
xiangxli:master

Conversation

@xiangxli
Copy link
Copy Markdown
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
Copy Markdown
Member

@Dreamsorcerer Dreamsorcerer Nov 2, 2023

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
Copy Markdown
Contributor Author

@xiangxli xiangxli Nov 3, 2023

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
Copy Markdown
Member

@Dreamsorcerer Dreamsorcerer Nov 2, 2023

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
Dreamsorcerer
Dreamsorcerer reviewed Nov 2, 2023
View reviewed changes
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
Copy Markdown
Member

@Dreamsorcerer Dreamsorcerer Nov 2, 2023

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
Copy Markdown
Member

@Dreamsorcerer Dreamsorcerer Nov 2, 2023

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
Copy Markdown
Contributor Author

@xiangxli xiangxli Nov 3, 2023

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
Copy Markdown
Member

Dreamsorcerer commented Dec 4, 2023

Still interested in finishing this PR?

@xiangxli
Copy link
Copy Markdown
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
# 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
return mock.Mock()

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).
@Dreamsorcerer
Copy link
Copy Markdown
Member

Dreamsorcerer 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...

@xiangxli
Copy link
Copy Markdown
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
Copy Markdown
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.

3 participants

@xiangxli @Dreamsorcerer @github-advanced-security