Opened 3 years ago

Closed 3 years ago

#5157 enhancement closed fixed (fixed)

Support redirect for twisted.web.client.Agent

Reported by: therve Owned by: therve
Priority: normal Milestone:
Component: web Keywords: httpclient
Cc: jknight Branch: branches/agent-redirect-5157
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description

We need to support redirects, probably through a new RedirectAgent class.

Change History (14)

comment:1 Changed 3 years ago by DefaultCC Plugin

  • Cc jknight added

comment:2 Changed 3 years ago by therve

  • Author set to therve
  • Branch set to branches/agent-redirect-5157

(In [32178]) Branching to 'agent-redirect-5157'

comment:3 Changed 3 years ago by therve

  • Keywords review added
  • Owner therve deleted

I've made a first implementation of RedirectAgent in the branch, but I think I need some feedback.

The implementation is fairly strict (treating 301 and 302 as 307). I'm returning a fair number of ResponseFailed, which I modified to include the response in case the redirect responsibility is given to the caller. Other than that I'm handling redirect limit rather easily.

I've also been wondering about keeping track of 301 to not query it the second time the same agent gets a request againts a permanently redirected URI.

comment:4 Changed 3 years ago by exarkun

How does the branch handle redirect responses on requests that include bodies?

comment:5 Changed 3 years ago by therve

For now, the branch ignores the body on the subsequent request after redirect (ie, pass the body the first time, and pass None afterwards). I failed to find something in the RFC about it.

comment:6 Changed 3 years ago by therve

Actually, I implemented that:

The action required MAY be carried out by the user agent without interaction with the user if and  only if the method used in the second request is GET or HEAD.

IE, the redirected request will only ever be HEAD or GET, so won't contain a body.

comment:7 Changed 3 years ago by therve

  • Keywords httpclient added

comment:8 Changed 3 years ago by exarkun

  1. twisted.web.client.RedirectLimitReached looks like it means the same thing as twisted.web.error.InfiniteRedirection. InfiniteRedirection looks like it offers some nice attributes, too. Can we avoid introducing the latter and just re-use the former?
  2. Likewise for client.NoAutomaticRedirect / error.PageRedirect.
  3. The other new exception type, RedirectWithNoLocation, should probably be defined in twisted.web.error as well.
  4. twisted.web.http defines symbolic names for 301, 302, 303, 307 - use those instead of the literal integers in _handleResponse.

I didn't get a chance to finish this, I'll try to look again soon if no one else beats me to it.

comment:9 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Okay, I suppose I did mostly finish. The only other points are that the news fragment is missing and that the http client howto should be updated with an example for RedirectAgent. :)

Thanks!

comment:10 Changed 3 years ago by therve

Almost there, just need to update the documentation.

comment:11 Changed 3 years ago by therve

  • Keywords review added
  • Owner therve deleted

Everything handled, I think.

comment:12 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Looks good to me. The tests could perhaps use the response code mnemonics instead of the literal codes (we have them all defined in twisted.web.http I believe). However, it's low level code for the handling of particular response codes, so having the literal values doesn't bother me too much. Up to you.

Buildbot likes it. Please merge when you're happy with it.

comment:13 Changed 3 years ago by therve

I wondered about the tests. But for example we're using 200 almost everywhere, so I don't think using the literal values is that bad. Thanks!

comment:14 Changed 3 years ago by therve

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

(In [32466]) Merge agent-redirect-5157

Author: therve
Reviewer: exarkun
Fixes: #5157

Add a new twisted.web.client.RedirectAgent for redirect support the HTTP 1.1
client stack. The branch also makes the response available in ResponseFailed
errors.

Note: See TracTickets for help on using tickets.