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

Method to delete a specific set of cookies #4942

Closed
Lonami opened this issue Aug 28, 2020 · 17 comments · Fixed by #5249
Closed

Method to delete a specific set of cookies #4942

Lonami opened this issue Aug 28, 2020 · 17 comments · Fixed by #5249

Comments

@Lonami
Copy link

Lonami commented Aug 28, 2020

Currently if one wants to delete some cookies, the way to do it is not really obvious:

new_cookies = []

for c in session.cookie_jar:
    if check(c):
        c['max-age'] = -1
        new_cookies.append((c.key, c))

session.cookie_jar.update_cookies(new_cookies)

It would be nice to have a method to do this cleanly.

Use case: one may want to clear the cookies from a session before trying again the same operation but not wishing to recreate the session (or being unable to do so).

@WolframAlph
Copy link
Member

WolframAlph commented Sep 8, 2020

Hi @Lonami there is clear method in https://github.com/aio-libs/aiohttp/blob/master/aiohttp/cookiejar.py#L71 . Is this something you missed? You can directly call session.cookie_jar.clear() .

@Lonami
Copy link
Author

Lonami commented Sep 8, 2020

I don't recall if I saw it, but that seems to delete all the cookies of the session. I was looking for something that deletes only some of the cookies. I'd expect a method where I can provide a list of cookies to delete.

@Lonami Lonami changed the title Method to delete cookies Method to delete a specific set of cookies Sep 8, 2020
@Lonami
Copy link
Author

Lonami commented Sep 8, 2020

I have updated the original post and title to clarify.

@WolframAlph
Copy link
Member

This makes sense now. @webknjaz can I start working on it?

@myusko
Copy link

myusko commented Sep 20, 2020

Someone is working on the issue? cc @webknjaz

@webknjaz
Copy link
Member

@WolframAlph @myusko feel free to implement this.

@asvetlov
Copy link
Member

CookieJar is for client cookies.
The cookie lifespan is controlled by a server usually, the jar is just client-side storage.
I'm curious what is your use-case?

@Lonami
Copy link
Author

Lonami commented Oct 18, 2020

I am using a single ClientSession to access multiple different sites, and at times I need to clear a specific cookie to have the specific site send me a fresh value while not altering the cookies in the rest of sites. Much like Firefox lets you clear the "site data", including cookies, of just one site and not the entire browser.

@asvetlov
Copy link
Member

Okay, I can imagine the clear_domain() method that deletes all cookies for the passed domain AND all subdomains regardless of cookie path and expiration dates.
Another alternative is extending the clear() method to accept an optional domain argument, delete all cookies if domain is None (the default behavior).

I unlikely can find time for the feature soon but open to review/merge a PR if somebody wants to propose it.

@Lonami
Copy link
Author

Lonami commented Oct 18, 2020

What about a filter parameter that accepts a callable with the cookie as input parameter and the boolean it returns determines whether to clear it or not? This gives the largest flexibility, and would also support the "delete domain" use case quite easily:

session.cookie_jar.clear(filter=lambda cookie: cookie.domain == 'example.com')

@asvetlov
Copy link
Member

My guts feel that filter can be a secondary solution, but people need something easier to understand in the first place.
BTW, are filtered records removed from the jar or kept in?

Maybe I'm wrong and good documentation with a couple of examples: one for a site with subdomains and another for something else -- can teach users comprehensively.

@Lonami
Copy link
Author

Lonami commented Oct 19, 2020

I was thinking something along the lines of:

def clear(self,
          domain: Optional[str]=None,
          filter: Optional[Callable[[SimpleCookie], bool]]=None) -> None:
    """
    Clears the cookies from this jar.

    If the ``domain`` parameter is specified, only the cookies belonging
    to that specific domain will be cleared.

    If the ``filter`` parameter is specified, only those cookies for which
    the filter returns `True` will be cleared. If any of the calls to the
    filter fail, no changes will be made to the jar.

    A `ValueError` is raised if both ``domain`` and ``filter`` are
    specified at the same time.
    """

People don't need to use filter if they don't need to, but when they do need it, it will be useful to have.

@asvetlov
Copy link
Member

The design is viable.
Small architecture note: you have mutually exclusive domain and filter parameters. We have eighter

  1. Prevent passing both domain and filter into the same call; mention in docs that params are mutually exclusive.
  2. Split clear() into three methods with separate names :)

@Lonami
Copy link
Author

Lonami commented Oct 19, 2020

We could technically make both parameters work, but because I realized it would probably be more confusing (is it clearing both domain and filter, or filtering only those in domain?), I edited my comment to mention that they are exclusive in the documentation. If you're happy with the proposed function signature I can go with that.

@asvetlov
Copy link
Member

Please use separate methods with the following signatures:

def clear(self, predicate: Optional[Callable[[SimpleCookie], bool]]=None) -> None:
    # drop cookies for which predicate(cookie) returns True,
    # remove everything is the predicate is omitted.

def clear_domain(self, domain: str) -> None:
    # drop cookies belonging to the domain
    # subdomains are cleared as well

clear_domain() should be implemented as a simple shortcut which calls clear() with a properly constructed predicate.

@Lonami
Copy link
Author

Lonami commented Oct 19, 2020

clear_domain() should be implemented as a simple shortcut which calls clear() with a properly constructed predicate.

Are you sure? The jar maps {domain: {name: cookie}}, so not using the predicate would be the most efficient way to do it (just drop the entire key for a domain), unless I'm missing something. Of course perhaps we shouldn't worry about that bit of performance, but it's better to double check.

@asvetlov
Copy link
Member

I don't think that the method is a performance bottleneck. For _host_only_cookies you need to iterate over the whole container (or change the data structure) anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants