Patch for [ 735515 ] urllib2 should cache 301 redir

[orsenthil]

BPO	1755841
Nosy	@orsenthil, @pitrou, @bitdancer
Files	urllib2-301-patch.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/orsenthil'
closed_at = None
created_at = <Date 2007-07-18.02:32:18.000>
labels = ['type-feature', 'library']
title = 'Patch for [ 735515 ] urllib2 should cache 301 redir'
updated_at = <Date 2014-02-03.19:20:04.012>
user = 'https://github.com/orsenthil'

bugs.python.org fields:

activity = <Date 2014-02-03.19:20:04.012>
actor = 'BreamoreBoy'
assignee = 'orsenthil'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2007-07-18.02:32:18.000>
creator = 'orsenthil'
dependencies = []
files = ['15754']
hgrepos = []
issue_num = 1755841
keywords = ['patch']
message_count = 22.0
messages = ['52902', '52903', '52904', '52905', '52906', '52907', '52908', '52909', '52910', '96899', '96937', '96952', '96959', '97155', '97161', '97262', '97264', '97265', '97282', '97283', '97293', '110588']
nosy_count = 5.0
nosy_names = ['jjlee', 'orsenthil', 'pitrou', 'vila', 'r.david.murray']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue1755841'
versions = ['Python 3.2']

[orsenthil]

Tracker item python.org/sf/735515 mentions about urllib2 to cache 301 redirections.
Attached patch tries to implement the same.

Comments on Version 1 of patch:
a) Initializes a dictionary to store the redirection.
b) If req already in cache, return the previous Request object.
c) Otherwise handle the same as 302 and store the Request object.
d) Checks for loop errors in 301.

Just noticed, that it I missed max-redirect checks.
Please comment on this patch, with next version I shall add the max redirect check.

[orsenthil]

I have made some changes to patch. Made it use just the self.cache to cache and follow the same logic for redirect and loop as 302 would follow. I thought this is ok. Did a basic testing to verify if it caught 301 redirect, yes it does.

File Added: urllib2-cache-301.patch

[smontanaro]

Looks reasonable to me, however I wonder if you shouldn't purge cached information periodically. Does RFC 2616 provide any guidance?

Skip

[orsenthil]

Hi Skip,
I have implemented Cache redirection for 301 errors only which will be Permanant redirection. Expiry header is not expected for 301 direction, so we might not purge it.
RFC 2616 states that if cache is implemented for 302 redirects ( temporary redirects) than Expiry header should be handled and cache purged.

For our change it is not required.

Thanks,
Senthil

[smontanaro]

How much extra work would it be to handle 302 redirects and the Expiry header? (I don't want to hold you up from the rest of your SoC tasks. Time's running out.)

Note also that you should also include a patch to the urllib2 documentation and, if possible, one or more test cases to exercise the new code.

Skip

[orsenthil]

File Added: urllib2-301-cache.patch

[orsenthil]

File Added: test_urllib2-cache.patch

[orsenthil]

File Added: liburllib2.patch

[orsenthil]

Hi Skip,
Added the updated patch and also added patches to test and documentation also.

Did some investigation on Cache redirection and expires for 302. IMHO, it should be handled as a separate feature request (based on the need), as we will have to handle the Cache-Control header values in addition to expires header. This would amount for good amount of change w.r.t the HTTPRedirectHandler itself.
[http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html section 14.9]

As for the current Tracker item, please verify and if found suitable checkin to the Python trunk.

Thanks,
Senthil

[orsenthil]

I am attaching an updated patch for caching the 301 redirects.
As per RFC 2616:

10.3.2 301 Moved Permanently
...
...references returned by the server, where possible. This response
is cacheable unless indicated otherwise.

So, I have included an additional argument to the 301 method called
cacheable=True. Which can be used to turn off the cache if required.

I would like to seek some comments on the patch, specifically on adding
this cacheable=True keyword argument. If its fine, I would go ahead and
check it in.

[pitrou]

I have trouble understanding what the patch does. I would expect it to
cache the <original URL> -> <redirected URL> mapping, but it seems to
cache the final HTTP response instead.

Also, it's not obvious in which situations the default for cacheable
would be overriden. Aren't http_error_301 and friends for internal use?

[orsenthil]

Antoine Pitrou added the Comment:

I have trouble understanding what the patch does. I would expect it to
cache the <original URL> -> <redirected URL> mapping, but it seems
cache the final HTTP response instead.

Oops. My mistake. I got carried away by my misunderstanding of the RFC
section on 301. The patch is wrong. I coded it to cache the response
from the redirection instead of just the redirected URL.
I shall write the correct one to cache just the redirection.

Aren't http_error_301 and friends for internal use?

Not really. They are exposed methods and I believe are being used by
clients. There have been bug reports related those redirection
methods. Even the related bpo-735515, explains pretty clearly about
the redirection required. (My bad again and my comment in that issue
is irrelevant.) In this issue, John says that there is no obvious need
to change the interface.

RFC had a statement along the lines that "301 redirection is cached by
default, unless indicated otherwise". This is where I thought, an option
to turn-off the cacheable behavior might be needed.

Thanks for looking at this quickly.