The iterable produced by Session.resolve_redirects does not include the very first response

[zackw]

If you are manually walking over redirects, you probably want to structure your code like this:

redirect_sequence = session.send_with_manual_redirect_walking(request, ...)
for resp in redirect_sequence:
    # do something with 'resp'

The existing API does not let you do that. You must write either

first_response = session.send(request, ..., allow_redirects=False)
# do something with 'first_response'
for resp in session.resolve_redirects(first_response, request, ...)
    # do something with 'resp'

which involves writing the same "do something with" code in two places, or

resp = session.send(request, ..., allow_redirects=False)
redir_iter = session.resolve_redirects(resp, request, ...)
while True:
    # do something with 'resp'
    if not resp.is_redirect: break
    resp = next(redir_iter)

which is un-Pythonic loop structure.

Since Session.resolve_redirects must remain as is for compatibility's sake, the only way to fix this is to add either a new mode to send (allow_redirects=MANUAL?) or a new Session method (perhaps in fact called send_with_manual_redirect_walking) which returns an iterable that does include the very first response. I do not particularly care which, or what the new method is called in the second case.

[Lukasa]

It seems like the best approach is the second with a better method name.

[zackw]

What do you think a better method name would be? I am chronically bad at names.

[Lukasa]

Heh, there's not really a good one here, but could be iter_send(). Matches an established trend in requests to use iter_x.

[piotr-dobrogost]

Wouldn't iter_locations or iter_redirects be better?

[Lukasa]

I thought about it, but both are misleading, as they contain the first response (which is not a redirect). Could be iter_responses?

Anyway, this is bikeshedding of the highest order. The question is are we happy to add this method to the API?

[zackw]

It's the very last response in the sequence that's not a redirect, but yeah, one of them isn't. I rather like iter_responses.

[sigmavirus24]

I like iter_send better than iter_responses. They both sound a little nebulous though. The former makes a bit more sense to me with the understanding that allow_redirects is not a parameter.

All things considered, the goal here is more to provide an equivalent to send that gives the user more control than request over their redirects. Since it really will be a companion to send then, I think iter_send makes more sense. iter_responses could mean too many things. We have to design this API with the constraint that the users taking advantage of this will will be advanced users. They should already be familiar with send so iter_send should be an intuitive leap to them, even if the name still is a bit vague.

---

On a side note, allow me to play devil's advocate. I can easily see people complaining that we only allow for this handling of redirects on such a low level. People will want this change to bubble up to iter_response (to correlate to the response method) as well as corresponding methods for iter_get, iter_post, etc. (regardless of whether or not some of those make sense based on the RFCs and the way servers behave). In other words, as devil's advocate, I'm warning of what could be perceived by some users as a foot in the door to further API extensions that are unnecessary and ugly. This is less of an argument against this change, and more of a warning that we should be careful how we choose to architect and document this.

[Lukasa]

Taking @sigmavirus24's concern on board, is there an elegant way we can do this outside of the library, e.g. in the toolbelt?

[zackw]

I'm not familiar with this "toolbelt"? But, an alternative would be a Session.prepare_request_for_redirect method that takes an .is_redirect response and produces a new PreparedRequest to follow the redirect. That is sufficiently low-level that I don't think it would induce feature creep, but makes it straightforward to write the generator yourself if you want it:

def iter_send(session, request, **kwargs):
    resp = session.send(request, allow_redirects=False, **kwargs)
    while resp.is_redirect:
        yield resp
        resp = session.send(session.prepare_request_for_redirect(resp),
                            allow_redirects=False, **kwargs)
   yield resp

Since backward compatibility dictates preserving resolve_redirects, we are going to want a method like this anyway to house shared code between iter_send and resolve_redirects.

Having said that, personally I'm not much concerned about the feature creep issue, because I think anyone who wants to do manual redirection chasing is going to want to work with the Session API anyway. If nothing else, you probably need Session-level control over cookies.

[sigmavirus24]

Taking @sigmavirus24's concern on board

It isn't a very strong concern. It's more of a pattern I've seen develop as of late. People watch the repo for a tiny change and use that change to get their foot in the door for a larger one that is widely unnecessary. It's a tiny concern that's ever present now.

Likewise, I think the toolbelt could easily accomodate this. That said, I'm not convinced it should be either in or outside of the core (i.e., I don't actually know where it belongs).

I've also been thinking along the same lines @zackw, but more geared towards making an eventual refactor a lot easier. I like having a compliment to prepare_request sibling. How does prepare_redirected_request sound?

[Lukasa]

I can get behind that idea, though I'm +0.5 until I see some code.

[sigmavirus24]

I really want to get some work done on the toolbelt and betamax today. If @zackw has the time to throw together an example of prepare_redirected_request that'd be great. Otherwise, I'll likely work on it later this week.

[zackw]

@sigmavirus24 Not a problem - it's a simple matter of moving code around. See #1965.

(I am going to be offline for most of the rest of the day, though.)

[sigmavirus24]

It's the weekend. Enjoy your Sunday! 🍰