Opened 4 years ago

Closed 3 years ago

#4922 enhancement closed fixed (fixed)

CookieAgent to add support for HTTP cookie handling

Reported by: gabrtv Owned by: jonathanj
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight Branch: branches/cookie-agent-4922
(diff, github, buildbot, log)
Author: Gabriel Monroy, jonathanj Launchpad Bug:

Description

This patch extents the twisted.web.client.Agent to add support for handling HTTP cookies. The new CookieLibAgent requires a cookieJar that implements the cookielib.CookieJar interface. Cookies are then automatically set and extracted during Agent request processing.

At the time of writing, this patch was built atop #3420 (Agent Persistent Connections).

This submission does not yet have tests. However it is performing relatively well inside a larger unit test infrastructure. I will try to add tests as my schedule permits, but I wanted to contribute the patch now in case others might find it useful.

Attachments (3)

cookieagent.patch (5.0 KB) - added by gabrtv 4 years ago.
CookieAgent implementation
cookieagent_composed.patch (5.0 KB) - added by gabrtv 4 years ago.
CookieAgent, composed
cookieagent_composed2.patch (4.1 KB) - added by gabrtv 4 years ago.
composed, minus dependency on persistent connections

Download all attachments as: .zip

Change History (17)

comment:1 Changed 4 years ago by DefaultCC Plugin

  • Cc jknight added

Changed 4 years ago by gabrtv

CookieAgent implementation

comment:2 Changed 4 years ago by gabrtv

  • Summary changed from CookieLibAgent to add support for HTTP cookie handling to CookieAgent to add support for HTTP cookie handling

comment:3 Changed 4 years ago by gabrtv

I should note that CookieAgent supports FileCookieJars which presumably perform some blocking file i/o (such as cookielib.MozillaCookieJar). Not sure if that's a problem or not.

comment:4 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner set to gabrtv

Thanks for working on this gabrtv. It will be great to have cookie support for Agent in Twisted.

The primary comment I have about this patch is that it would be simpler and more reusable if it composed a cookie-aware Agent-alike with Agent instead of subclassing Agent. This will also remove the dependency on the persistent connections branch.

Changed 4 years ago by gabrtv

CookieAgent, composed

Changed 4 years ago by gabrtv

composed, minus dependency on persistent connections

comment:5 Changed 4 years ago by gabrtv

CookieAgent patch is now composed, and should no longer have a dependency on agent persistent connections branch.

comment:6 Changed 4 years ago by gabrtv

  • Owner gabrtv deleted

comment:7 Changed 3 years ago by exarkun

  • Keywords review added

This is meant to be in review.

comment:8 follow-up: Changed 3 years ago by therve

  • Keywords review removed
  • Owner set to gabrtv

Thanks for the patch. The review:

  • You need to add tests.
  • The coding standard mentions 2 blank lines between methods, 3 between classes.
  • def __init__(self, agent, cookieJar, *args, **kwargs): It seems the extra arguments are unused.
  • It seems that the docstring of the request methods is an exact copy of the one of Agent.request. You should rather refer to Agent.request, and mentions the specific bits.
  • There may be a design problem with the way you manage the cookie jar: an Agent is stateless, so you should be able to issue as many requests as you want against one. The jar is "global" to the CookieAgent, so is modified after each response.
  • You set the cookie header directly on the header object. First, I think you forgot that headers is an optional argument, so it's necessarily existing. Then, I think we try not to modify the passed argument, so you should make a copy of the headers before.
  • It would be nice if you could define a fake urllib2 Request class, as you did for the response.

comment:9 Changed 3 years ago by jonathanj

  • Author changed from Gabriel Monroy to Gabriel Monroy, jonathanj
  • Branch set to branches/cookie-agent-4922

(In [31792]) Branching to 'cookie-agent-4922'

comment:10 Changed 3 years ago by jonathanj

(In [31805]) Apply cookieagent_composed2.patch from gabrtv. Address review commentary. Refs #4922.

comment:11 in reply to: ↑ 8 Changed 3 years ago by jonathanj

  • Keywords review added
  • Owner gabrtv deleted

Replying to therve:

  • There may be a design problem with the way you manage the cookie jar: an Agent is stateless, so you should be able to issue as many requests as you want against one. The jar is "global" to the CookieAgent, so is modified after each response.

As we discussed on IRC, I think that this is probably how you want CookieAgent to work. If you need separate cookie jars use separate cookie jars and agents; I added a sentence in the CookieAgent docstring to clarify that the cookie jar will be mutated.

Addressed all your other review points.

comment:12 Changed 3 years ago by therve

  • Keywords review removed
  • Owner set to jonathanj

Looks great. Some comments:

  • Please refer to the new IResponse interface instead of the Response class in docstrings.
  • The NEWS fragment should be slightly rephrased I think. Maybe "A new HTTP cookie-aware Twisted Web Agent wrapper is included in twisted.web.client.CookieAgent".

Please merge once fixed.

comment:13 Changed 3 years ago by jonathanj

(In [31863]) Refer to twisted.web.iweb.IResponse instead of twisted.web.client.Response, refs #4922.

comment:14 Changed 3 years ago by jonathanj

  • Resolution set to fixed
  • Status changed from new to closed

(In [31868]) Merge cookies-agent-4922.

Author: gabrtv, jonathanj
Reviewer: exarkun, therve
Fixes: #4922

Introduce twisted.web.client.CookieAgent, an HTTP cookie-aware Agent wrapper, that will read and write cookies to a cookielib.CookieJar instance.

Note: See TracTickets for help on using tickets.