urllib3 icon indicating copy to clipboard operation
urllib3 copied to clipboard

Retry classes should be able to modify the request headers

Open theacodes opened this issue 8 years ago • 6 comments

It would be useful for Retry subclasses to be able to modify the request headers between retries.

This came up in a discussion over at https://github.com/google/oauth2client/pull/659. Essentially, we'd love to be able to write a Retry class that detects 401 errors, refreshes the credentials, and then updates the headers. This would reduce the code in our urlopen implementation, but wouldn't completely obviate the need for a custom HTTP class (for that, we'd need some way to inject headers before requests).

theacodes avatar Sep 26 '16 19:09 theacodes

I seem to recall that @haikuginger and I kicked around the idea of allowing for a request/response transformation pipeline: essentially, the ability to register handlers for requests/responses that would allow transformations. This would work really well for auth as well.

Given that the Retry logic doesn't help you as much as this idea does, can you give me an idea about how much help a custom Retry logic would give?

Lukasa avatar Sep 26 '16 19:09 Lukasa

Yeah, I've got a prototype that I need to sit down with and expand a bit, but that was my first reaction as well; a pipelining class into which you can insert any number of mutators that'll take effect in order.

haikuginger avatar Sep 26 '16 19:09 haikuginger

@haikuginger's idea sounds intriguing, I'd definitely be willing to experiment with how it'll work with auth.

It sounds like it'll allow us to get away with not needing to modify the headers in our retry class. 👍

theacodes avatar Sep 26 '16 19:09 theacodes

@Lukasa to go into more detail on sort of what we'd like to have instead of our custom class, here's a sketch of what I'd love to be able to reduce our code to:

def inject_auth(credentials, http, method, url, headers, body, **kwargs):
    if not credentials.valid:
        credentials.refresh()
    credentials.before_request(http, method, url, headers)

def add_credentials_to_http(http):
    http.on_before_request(functools.partial(inject_auth, credentials))
    # Retry on all methods when 401 is encountered, although we'd probably do this with a wrapping
    # custom retry object to avoid clobbering user settings.
    http.retries.method_whitelist = False
    http.retries.status_forcelist = (401,)

theacodes avatar Sep 26 '16 19:09 theacodes

@jonparrott, @Lukasa, want to take a look at this (very) rough draft of the HttpPipeline object?

https://github.com/haikuginger/httppipeline

haikuginger avatar Sep 27 '16 01:09 haikuginger

@haikuginger that looks like it could work quite well for our purposes.

theacodes avatar Sep 27 '16 03:09 theacodes