Proposal: potential way to follow bad url shorteners 

I've ran into an issue that runs tangent to issue #441.

We have a content indexer that is powered by `requests`.  Some of the larger URL shorteners exhibit some weird behaviors depending on the user agent string.

* (variant of #441) depending on the user-agent header, a particular shortener will either send a proper 301 response OR a HTTP 200 containing a meta-refresh value of 0.

         <head><meta name="referrer" content="always"><noscript><META http-equiv="refresh" content="0;URL={URL}"></noscript><title>{URL}</title></head><script>window.opener = null; location.replace("{URL}")</script>

* (new?) a handful of url shorteners may send a HTTP-200 response with a `location` header to the redirect.  (yes I know, it breaks spec and makes no sense.

To handle both of these (and several other scenarios) I have a novel suggestion.  The callback hooks could be used to allow developers to catch and follow these types of redirect oddities within a single configured request.

A developer would be able to handle edge cases like the above by with a hook:

      def bad_shortener_callback(r):
          r_location = r.headers.get('location')
          if r.status_code == 200 and r_location!= r.url:
                r.is_redirect = True
                r.redirect_location_override = r_location
          return r

In order to make this work, a slight change would be needed: 

The redirect url is pulled via this line:

https://github.com/kennethreitz/requests/blob/f72684e13c5074a671506d29c1b5638156680ea7/requests/sessions.py#L116

    url = resp.headers['location']

I propose this change to sessions.py

    -url = resp.headers['location']
    +url = resp.redirect_location

 and then extending models.py accordingly

    + redirect_location_override = None

    @property
    def redirect_location(self):
          if self.redirect_location_override is not None:
             return self.redirect_location_override
         return self.headers['location']

if this is acceptable, I would be happy to issue a PR that includes tests.


Thanks for the suggestion!

So, I think I don't fully understand why this needs to be a Requests patch. What advantages would you or other users get from having this integrated into Requests?

I'm not sure how to interpret your question, so I'm going to answer it a few ways:

And I'll refer to this URL flow, where the 301s are a proper redirect and the 200 is standing in for a malformed response (meta-refresh or 200+location).

	A -> [301] -> B -> [200] -> C -> [301] -> D

1. What is the utility in general?

In terms of shorteners, it's incredibly common to encounter somewhat broken servers or a <meta-refresh> tag.  Being able to follow this stuff is often desired. 

Regarding the two examples I mentioned above, one is major social network and the other is one of the largest commercial url shortening services that handles hundreds of domains.  


2. A patch streamlines work.

With a patch in place, consumer code to handle bad servers would look something like the following. 

    def handle_response(r):
        ...
        r.location_override = 'url2'
        return r
    
    r = requests.get('url', max_redirects=4, hooks={'response': handle_response} )
    print(r)
    > <Response [200]>
    print(r.history)
    > [<Response [301]>, <Response [200]>, <Response [301]>]

I know what you're thinking -- "so elegant! much requests!" . Right?  Right!

Without a patch in place, it's quite messy/ugly to keep reprocessing the urls while adhering to an intended max_redirects - otherwise you have an endless recursion.  This is just a rough try at rewriting the above:

    def needs_redirect(r):
        ...
        return alt_location

	max_redirects = 4
	s = requests.Session(max_redirects=max_redirects)  # use a session so we don't have to create two objects with the same configuration kwargs)
	r = s.get(url)
	r_history = r.history  # we're going to overwrite r, so ugh.
	while True:
		redirect_location = needs_redirect(r)
		if not redirect_location:
			break
		_max_redirects = max_redirects - len(r_history)  # compute this as part of the redirects
		if _max_redirects <= 0:
			raise requests.exceptions.TooManyRedirects('Exceeded %s redirects.' % max_redirects, response=r)  # we'd raise an error within requests if a patch existed
		r = s.get(url, max_redirects=_max_redirects)
		r_history.extend(r.history)
    print(r)
    > <Response [200]>
    print(r_history)
    > [<Response [301]>, <Response [200]>, <Response [301]>]


3. Abusing the object's API

A hook could potentially overwrite the existing response data without the need for a patch, but that would make dealing with the response history very messy while debugging/developing and could break:

    def handle_response(r):
        ...
        r.headers['requests-original-location'] = r.headers.get('location')
        r.original_status_code = r.status_code
        r.headers['location'] = 'url2'
        r.status_code = 301
        return r
    
    r = requests.get('url', max_redirects=4, hooks={'response': handle_response} )
    print(r)
    > <Response [200]>
    print(r.history)
    > [<Response [301]>, <Response [301]>, <Response [301]>]

Note that this functions like the patched version, but r.history will show the overwritten value for the second item -- it can be hard to see what's going on.  This could be replaced with another object that inherits from Response, but that gets into even more code.

So the TLDR is that the proposed functionality would allow users to elegantly support some common scenarios while also adhering to limits set by max_redirects:

• follow bad shorteners (location but HTTP_OK)
• act upon html meta-refresh tags, or perhaps follow a canonical?
• see a clear and concise url history (don't break/fake the history)


So, I'm inclined to call this a "niche" issue: while I'm sure it's common in some uses, in general it is very uncommon.

I think we could consider solving the wacky "200 with Location header" as part of requests' redirect handling code. But I'm disinclined to give arbitrary hooks the option to trigger further redirects. It seems like additional code that doesn't meet the requirements of broad utility. In your second case, writing the code external to Requests, that code doesn't seem too unreasonable.

So I think I'm a tentative -0.5 on the idea of the bulk API change, but maybe -0 on the 200-with-Location if we can get some citations about why the hell browsers allow that and whether there is a threat model we need to consider there.

So, we've talked several times of refactoring our redirect handling into more discrete parts. If we strike the right balance, the user could override the logic that determines whether or not there's a redirect to follow. I don't think we need to start adding headers as response attributes. We've already shot those proposals down several times.

regarding the comment from @lukasa on general utility (not the suggested implementation): i disagree about broad utility of following non-location redirects.  i know this means allowing users to operate on "html", but please consider that many consumers will eventually consume the HTML redirect (which could be a meta-refresh, rel="canonical", type="og:url" or several others).  it's not a niche use, but a common one.

regarding the implementation details comment from @sigmavirus24:  I do agree. stashing headers was a way to not suggest a larger patch.

i was a bit surprised the redirect handling used a `while` loop to create a generator that is immediately consumed, and then just overrides the request.   i had looked through the commits and tickets, and it seems like approach was dictated by earlier api behaviors that no longer exist (or I haven't seen).

> but please consider that many consumers will eventually consume the HTML redirect (which could be a meta-refresh, rel="canonical", type="og:url" or several others). it's not a niche use, but a common one.

But Requests does *nothing* with HTML nor will it ever. Dealing with the HTML should be the user's choice. If something like this is *that* common, the [toolbelt](/sigmavirus24/requests-toolbelt) could grow some functionality for it, but I doubt that would be without extra issues.

> stashing headers was a way to not suggest a larger patch.

I appreciate that. Sometimes smaller patches are significantly worse in impact than larger ones. :)

> i was a bit surprised the redirect handling used a while loop to create a generator that is immediately consumed, and then just overrides the request. i had looked through the commits and tickets, and it seems like approach was dictated by earlier api behaviors that no longer exist (or I haven't seen).

It is an API that was solidified when Kenneth rewrote Requests overnight and released v1.0. We stick to that API so as to not break users currently relying on it. We can expand that particular bit though and have done so in the past where it makes sense.

I have no problem with achieving pluggability via a refactor of the redirect following logic. For example, it could be enough to update `SessionRedirectMixin` with a method `get_redirect_target` which we use to determine if a response needs us to redirect, and then call that.

I can try to do a PR for that.

I'd definitely be interested in seeing one. 

Just to be clear - I'm not suggesting that `requests` act on HTML data or natively support edge cases with redirects.  Dealing with that stuff is opening Pandora's Box -- new issues can (and will) keep popping up and needing support, and many use cases won't care.  Personally, I have one application that requires strict HTTP compliance and another one that doesn't care how malformed a response is, and aggressively wants/needs to consume it.  I wouldn't want to slow down one of my applications with the overhead of code needed for the other.  That stuff doesn't belong in the core library.

My intent is just to make it easier for users to be able to find and handle issues as they arise.  The existing "hooks" processing seemed like a natural choice, as it would allow users to write custom solutions as edge cases are discovered and immediately deploy and share them.

Still working out details, but how does this general approach look:

    class SessionRedirectMixin(object):
    + def get_redirect_target(self, resp):
    +     """Receives a Response. Returns a redirect URI or `None`"""
    +     if resp.is_redirect:
    +         return resp.headers['location']
    +     return None

    def resolve_redirects(self, resp, req, stream=False, timeout=None,
        ...
        - while resp.is_redirect:
        + while self.get_redirect_target(resp):
        ...
        - url = resp.headers['location']
        + url = self.get_redirect_target(resp)

That would enable pluggability via:

    class CustomSessionRedirectMixin(object):

        def get_redirect_target(self, resp):
            "custom override"
            pass

    class CustomSession(SessionRedirectMixinCustom, requests.Session):
        pass

    r = s.get('http://example.com')



So *broadly* I think this is fine. I'd be inclined to want to avoid calling `get_redirect_target` multiple times, but yes, I can get behind that general API shape.

I don't like processing `get_redirect_target` twice either.  The result needs to be consulted twice: once to determine if a redirect condition exists, and again to grab the result.  Caching the result in the Session is too messy - a history cleanup would be needed.  The best place to stash the calculated value would be on the `Response` object. (I thought about setting`Response._redirect_uri`) but I wanted to avoid touching that object.

This is one of those rare situations where I actually miss Perl.  (it supports assignments like `while url = self.get_redirect_target(resp):`)

I'll spend a few more days thinking over this.  

It shouldn't need to be cached: it just needs to be obtained at the bottom of the while loop (and of course once at the start of the generator).

oh, that is so obvious. how did i miss that! thanks.

Closing this, it was merged.