HTTPPageGetter shouldn't make two requests with an afterFoundGet

[magcius]

Currently, when afterFoundGet gets a 302 back, it will change the method to a GET and then accidentally make two requests.

Also, because the fix is so small (and partially related), I made HTTPDownloader accept afterFoundGet. This is required when using something like downloadPage where it doesn't let you get at the HTTPDownloader afterwards.

[exarkun]

Can you add a unit test?

[spiv]

Thanks for writing that test. It looks like its intent is good, but unfortunately it fails for me:

$ ./bin/trial twisted.web.test.test_webclient
…
===============================================================================
[ERROR]
Traceback (most recent call last):
Failure: twisted.web.error.InfiniteRedirection: 302 Infinite redirection detected to /afterFoundGet302

twisted.web.test.test_webclient.WebClientSSLTestCase.test_afterFoundGet302
twisted.web.test.test_webclient.WebClientTestCase.test_afterFoundGet302
-------------------------------------------------------------------------------
Ran 76 tests in 0.535s

FAILED (skips=2, errors=2, successes=72)

Also self.assertEquals(self.afterFoundGetTestResource, 1) doesn't make sense to me; why would a resource equal an int?

I'm guessing the solution to those issues is something like:

1. redirect to a different URL rather than itself, perhaps /
2. assertEquals with afterFoundGetTestResource.count

Finally on that test, self.assertEquals(f._redirectCount, 1) looks like it should be deleted entirely.

The other obvious blocker for this fix is a newsfile. Would you mind adding a 4760.bugfix file to twisted/web/topfiles with a sentence or two describing this change? See http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles

This looks like a pretty straightforward fix, so it would be very nice to get it landed. If you don't have much time to look at this soon let us know and probably someone else will work on finishing it.

[exarkun]

Unfortunately now the test passes whether or not the fix is applied. I think the problem is that the test is counting the wrong thing. The original target URL is only requested once. It's the redirect target that's requested twice.

[exarkun]

(In [30586]) Branching to 'double-redirect-get-4760'

[exarkun]

(In [30587]) Apply twisted-web-changes.diff

refs #4760

[exarkun]

Thanks for sticking with this, magcius.

Some minor stuff that I'll fix up:

1. Copyright date on modified files needs to be updated to include 2011
2. It's cool to refer to a ticket in a test method docstring, but it doesn't need to be put up front, it's not the most important thing the docstring conveys.

Aside from this points, I like the patch, except for mixing together the redirect fix with adding a new feature to downloadPage. That should get a separate ticket (and unit tests!).

I'll fix the two minor review points, back out the downloadPage feature addition, and merge this. Thanks again!

[exarkun]

(In [30588]) update copyright dates

refs #4760

[exarkun]

(In [30589]) Back out afterFoundGet addition to HTTPDownloader

refs #4760

[exarkun]

(In [30590]) Move the ticket reference to the end of the docstring

refs #4760

[exarkun]

(In [30591]) don't forget to add the news fragment

refs #4760

[exarkun]

(In [30592]) Merge double-redirect-get-4760

Author: magcius Reviewer: spiv, exarkun Fixes: #4760

Fix HTTPPageGetter and related code so that when afterFoundGet is used only one request is made for the redirect target location, not two.

[exarkun]

Check out #4364 for the HTTPDownloader/afterFoundGet feature.