Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4760 defect closed fixed (fixed)

HTTPPageGetter shouldn't make two requests with an afterFoundGet

Reported by: magcius Owned by:
Priority: normal Milestone:
Component: web Keywords:
Cc: magcius Branch: branches/double-redirect-get-4760
(diff, github, buildbot, log)
Author: magcius Launchpad Bug:

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 magcius 4 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 4 years ago by exarkun

  • Keywords review added
  • Owner jknight deleted

comment:2 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner set to magcius

Can you add a unit test?

comment:3 Changed 4 years ago by exarkun

  • Keywords review added
  • Owner magcius deleted

comment:4 Changed 4 years ago by spiv

  • Keywords review removed
  • Owner set to magcius

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 4 years ago by magcius

  • Cc magcius added
  • Keywords review added

comment:6 Changed 4 years ago by exarkun

  • 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 4 years ago by magcius

comment:7 Changed 4 years ago by magcius

  • Keywords review added

comment:8 Changed 4 years ago by magcius

  • Owner magcius deleted

comment:9 Changed 4 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/double-redirect-get-4760

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

comment:10 Changed 4 years ago by exarkun

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

refs #4760

comment:11 Changed 4 years ago by exarkun

  • Author changed from exarkun to magcius

comment:12 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner set to exarkun
  • Status changed from new to assigned

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 4 years ago by exarkun

(In [30588]) update copyright dates

refs #4760

comment:14 Changed 4 years ago by exarkun

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

refs #4760

comment:15 Changed 4 years ago by exarkun

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

refs #4760

comment:16 Changed 4 years ago by exarkun

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

refs #4760

comment:17 Changed 4 years ago by exarkun

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

(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 4 years ago by exarkun

Check out #4364 for the HTTPDownloader/afterFoundGet feature.

comment:19 Changed 3 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.