#5434 enhancement closed fixed (fixed)
Web browser-like redirection Agent for twisted.web.client
Reported by: | Tom Most | Owned by: | Jonathan Jacobs |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | web | Keywords: | agent |
Cc: | jknight, Tom Most, Jonathan Jacobs | Branch: |
branches/browser-like-redirect-agent-5434-2
branch-diff, diff-cov, branch-cov, buildbot |
Author: | jonathanj |
Description (last modified by )
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.
Change History (21)
comment:1 Changed 11 years ago by
Cc: | jknight added |
---|
comment:2 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 11 years ago by
Cc: | Tom Most added |
---|
comment:4 Changed 10 years ago by
comment:5 Changed 9 years ago by
Author: | → jonathanj |
---|---|
Branch: | → branches/browser-like-redirect-agent-5434 |
(In [37854]) Branching to 'browser-like-redirect-agent-5434'
comment:6 Changed 9 years ago by
comment:7 Changed 9 years ago by
comment:8 Changed 9 years ago by
comment:9 Changed 9 years ago by
comment:10 Changed 9 years ago by
Keywords: | agent review added |
---|
comment:12 Changed 9 years ago by
Cc: | Jonathan Jacobs added |
---|
comment:14 follow-up: 16 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Richard Wall to Jonathan Jacobs |
Status: | assigned → new |
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:
- Build Failures
- 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
- Twisted Checker. Some of these warnings need fixing: https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/509/steps/run-twistedchecker/logs/new%20twistedchecker%20errors
- source:branches/browser-like-redirect-agent-5434/twisted/web/test/test_agent.py
- 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.
- Typo "changes the methods" looks like it should be "changes the method"
- 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"
- 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.
- Typo "origialResolveLocation"
- Add a link to "HTTP 1.1 bis draft" in the test docstring.
- BrowserLikeRedirectAgentTests.test_redirectToGet301 and test_redirectToGet302 docstrings contain L{RedirectAgent}. Should be L{BrowserLikeRedirectAgent}.
- source:branches/browser-like-redirect-agent-5434/twisted/web/client.py
- 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.
- 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.
comment:15 Changed 9 years ago by
I also suggest updating the Agent Howto documentation, which currently describes the strict redirect behaviour.
comment:16 Changed 9 years ago by
Keywords: | review added |
---|
Replying to rwall: Thank you for the review, Richard!
- 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{}
.
- 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.
comment:17 Changed 9 years ago by
Owner: | Jonathan Jacobs deleted |
---|
comment:18 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jonathan Jacobs |
comment:19 Changed 9 years ago by
Branch: | branches/browser-like-redirect-agent-5434 → branches/browser-like-redirect-agent-5434-2 |
---|
(In [38669]) Branching to 'browser-like-redirect-agent-5434-2'
comment:20 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(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.
comment:21 Changed 9 years ago by
Note that assertEquals
is on its way to being deprecated but this branch introduces several new uses of it.
#5462 was a duplicate.