Web browser-like redirection Agent for twisted.web.client

[twm]

The existing twisted.web.client.RedirectAgent implements redirection in a strictly by-the-​RFC manner, which breaks on web sites which assume the historic behavior of web browsers. An additional browser-like implementation would be useful for interacting with legacy systems. It should implement the following quirks:

• Treat 301 Moved Permanently and 302 Found redirects on non-{HEAD,GET} requests like 303 See Other: change the method to GET and proceed, instead of failing.
• Follow redirects which give relative URLs in the Location header, resolving them to absolute.

[exarkun]

#5462 was a duplicate.

[jonathanj]

(In [37854]) Branching to 'browser-like-redirect-agent-5434'

[jonathanj]

(In [37855]) Add tests for 301 and 302 redirects failing on POST requests too, refs #5434.

[jonathanj]

(In [37856]) Refactor RedirectAgent redirect reponse code handling, refs #5434.

[jonathanj]

(In [37857]) Implement BrowserLikeRedirectAgent, a redirect agent that treats HTTP 301 and 302 like HTTP 303 for non-HEAD/GET requests, changing the method to GET before proceeding. refs #5434

[jonathanj]

(In [37885]) Resolve relative URIs in RedirectAgent and BrowserLikeRedirectAgent, refs #5434, #5462.

[jonathanj]

​Build results

[rwall]

Reviewing...

[rwall]

Code Review

Thanks Jonathan. This branch looks pretty good to me. Here are a few notes and comments.

Notes:

• Merges cleanly to trunk
• Tests all pass on my Fedora 18 x86_64 laptop
• Coverage seems complete: ​https://buildbot.twistedmatrix.com/builds/twisted-coverage.py/twisted-coverage.py-r37885/twisted_web_client.html
• Pyflakes warnings (can probably be ignored): ​https://buildbot.twistedmatrix.com/builders/pyflakes/builds/220/steps/pyflakes/logs/new%20pyflakes%20errors *

Please fix the following:

1. Build Failures
   1. Python3 test failures need fixing (I think t.p.compat.nativeString may help): ​https://buildbot.twistedmatrix.com/builders/python-3.3-tests/builds/683/steps/shell/logs/stdio
   2. Twisted Checker. Some of these warnings need fixing: ​https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/509/steps/run-twistedchecker/logs/new%20twistedchecker%20errors
2. source:branches/browser-like-redirect-agent-5434/twisted/web/test/test_agent.py
   1. Minor nit...Why change C{GET} to I{GET}? I'd have thought C{GET} was more appropriate. Choose one and use it consistently throughout the tests and the client module.
   2. Typo "changes the methods" looks like it should be "changes the method"
   3. Create a new ticket or add an existing trac ticket number alongside the comment "XXX: If we had a way to get the final absolute URI"
   4. Minor nit...You seem to be using a mixture of L{RedirectAgent} and L{client.RedirectAgent} in the test docstrings. You should use L{client.RedirectAgent} because that's how it's imported in this module, but it doesn't really matter because API docs aren't generated for the test modules. It would be nice to be consistent though.
   5. Typo "origialResolveLocation"
   6. Add a link to "HTTP 1.1 bis draft" in the test docstring.
   7. BrowserLikeRedirectAgentTests.test_redirectToGet301 and test_redirectToGet302 docstrings contain L{RedirectAgent}. Should be L{BrowserLikeRedirectAgent}.
3. source:branches/browser-like-redirect-agent-5434/twisted/web/client.py
   1. Add @param, @type and @return to _urljoin. It's a private function, so I don't think it's strictly required by the coding standard, but it will be helpful to anyone reading the code.
   2. Should '#' be b'#' in _urljoin, to catch early any attempt to concat unicode string with byte string in Python3?

Please resubmit for review after answering or addressing the numbered comments above.

-RichardW.

[rwall]

I also suggest updating the Agent Howto documentation, which currently describes the strict redirect behaviour.

• ​https://twistedmatrix.com/documents/current/web/howto/client.html#auto8

[jonathanj]

Replying to rwall: Thank you for the review, Richard!

1. Minor nit...Why change C{GET} to I{GET}? I'd have thought C{GET} was more appropriate. Choose one and use it consistently throughout the tests and the client module.

C{} is usually used for content that could be valid Python code while I{} simply produces emphasis. Since the "GET" in this documentation is about the HTTP concept and not a Python string or constant it seems like I{} is more correct than C{}. I've changed the documentation, local to my changes, to use I{}.

3. Create a new ticket or add an existing trac ticket number alongside the comment "XXX: If we had a way to get the final absolute URI"

There is already a ticket for this and it's actually in review (which causes a weird chicken and egg problem) but I've updated the comment to mention it.

I've addressed all your other review points.

[tom.prince]

The XXX in _testRedirectURI should have a ticket to resolve it, or a note to in #5435 to change this.

Other than that, this looks good.

(The python3.3 builder is currently down, but running the tests locally, they pass)

I like the cleaned up tests.

Please merge, and add a note to #5435.

[jonathanj]

(In [38669]) Branching to 'browser-like-redirect-agent-5434-2'

[jonathanj]

(In [38672]) Merge browser-like-redirect-agent-5434-2.

Author: jonathanj Reviewer: tom.prince, rwall Fixes: #5434, #5462

Introduce BrowserLikeRedirectAgent, a RedirectAgent subclass, that handles redirects more like a web browser, specifically it treats HTTP 301 and 302 like HTTP 303 on non-HEAD/GET requests and changes the method to GET before proceeding.

RedirectAgent now also resolves relative URI references in the Location header against the most recent request URI.

[exarkun]

Note that assertEquals is on its way to being deprecated but this branch introduces several new uses of it.