Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#4760 defect closed fixed (fixed)

HTTPPageGetter shouldn't make two requests with an afterFoundGet

Reported by: Jasper St. Pierre Owned by:
Priority: normal Milestone:
Component: web Keywords:
Cc: Jasper St. Pierre Branch: branches/double-redirect-get-4760
branch-diff, diff-cov, branch-cov, buildbot
Author: magcius

Description

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.

Attachments (1)

twisted-web-changes.diff (3.6 KB) - added by Jasper St. Pierre 6 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 6 years ago by Jean-Paul Calderone

Keywords: review added
Owner: jknight deleted

comment:2 Changed 6 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jasper St. Pierre

Can you add a unit test?

comment:3 Changed 6 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jasper St. Pierre deleted

comment:4 Changed 6 years ago by spiv

Keywords: review removed
Owner: set to Jasper St. Pierre

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.

comment:5 Changed 6 years ago by Jasper St. Pierre

Cc: Jasper St. Pierre added
Keywords: review added

comment:6 Changed 6 years ago by Jean-Paul Calderone

Keywords: review removed

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.

Changed 6 years ago by Jasper St. Pierre

Attachment: twisted-web-changes.diff added

comment:7 Changed 6 years ago by Jasper St. Pierre

Keywords: review added

comment:8 Changed 6 years ago by Jasper St. Pierre

Owner: Jasper St. Pierre deleted

comment:9 Changed 6 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/double-redirect-get-4760

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

comment:10 Changed 6 years ago by Jean-Paul Calderone

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

refs #4760

comment:11 Changed 6 years ago by Jean-Paul Calderone

Author: exarkunmagcius

comment:12 Changed 6 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jean-Paul Calderone
Status: newassigned

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!

comment:13 Changed 6 years ago by Jean-Paul Calderone

(In [30588]) update copyright dates

refs #4760

comment:14 Changed 6 years ago by Jean-Paul Calderone

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

refs #4760

comment:15 Changed 6 years ago by Jean-Paul Calderone

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

refs #4760

comment:16 Changed 6 years ago by Jean-Paul Calderone

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

refs #4760

comment:17 Changed 6 years ago by Jean-Paul Calderone

Resolution: fixed
Status: assignedclosed

(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.

comment:18 Changed 6 years ago by Jean-Paul Calderone

Check out #4364 for the HTTPDownloader/afterFoundGet feature.

comment:19 Changed 6 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.