Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Per-Request RedirectPolicy #353

Open
NyxCode opened this issue Sep 21, 2018 · 16 comments · May be fixed by #2440
Open

Per-Request RedirectPolicy #353

NyxCode opened this issue Sep 21, 2018 · 16 comments · May be fixed by #2440

Comments

@NyxCode
Copy link

NyxCode commented Sep 21, 2018

Since reusing Clients is a good idea it should be possible use a RedirectPolicy only for a certain request. Right now, I need to create a new Client for every RedirectPolicy which seems sub-optimal.

@NyxCode
Copy link
Author

NyxCode commented Sep 21, 2018

How about a redirect_policy: Option<RedirectPolicy> in the Request struct which, if present, overrides the RedirectPolicy which the client was configured with?

@seanmonstar
Copy link
Owner

Is it not possible to inspect the RedirectAttempt details and decide from there?

@NyxCode
Copy link
Author

NyxCode commented Sep 25, 2018

No, I don't think so.
Imagine you need to send 2 requests to the same URL - but only the first one may follow redirects and the second may not.
Also, a function fn do_an_api_req(client: &Client) would have to just "trust" the caller to use the right RedirectPolicy.

@seanmonstar
Copy link
Owner

What sort of information determines whether you can follow redirects? Can it be encoded in some other value that is shared into the closure of Redirect::Policy::custom?

@NyxCode
Copy link
Author

NyxCode commented Sep 25, 2018

In my case: no.
But even if it worked it would be pretty dirty because then (as explained above) my APIs had to blindly trust the caller and the caller would need to know what URLs I will call (which is impossible in my case)

@NyxCode
Copy link
Author

NyxCode commented Sep 25, 2018

(what would you do if the redirect policy depended on the response body of the previous request?)

@seanmonstar
Copy link
Owner

If redirecting depended on the response body, then even a per-request policy wouldn't be able to handle that.

@NyxCode
Copy link
Author

NyxCode commented Sep 25, 2018

you're right, bad argument, nvm.
Even though something like that might be nice to have, I think that usecase is pretty niche.
Do you agree with the other two arguments I made?

@seanmonstar
Copy link
Owner

It'd probably be useful to also see some prior-art in other HTTP clients.

I so far feel like this is also very niche, as it seems surprising that a server would sometimes return acceptable redirects and sometimes not.

@NyxCode
Copy link
Author

NyxCode commented Sep 25, 2018

Hm.
I'm coming from Java where, at least the standard http client, follows your design: set the redirect policy as a property of the client which every request follows.

In python on the other hand you pass a boolean directly to the request function (probably because there isn't an instance of a client which could store such settings)

You're probably right with the assumption that this usecase is not that common - even though I need it in the first project I use this library for. (;

@NyxCode
Copy link
Author

NyxCode commented Sep 25, 2018

An other implication of the this is that a crate which wraps a rest api is required to always create its own Client. This implies that such a crate would need to maintain state (since we want to reuse the underlying Client).

Example:

# may not follow redirects
fn login_with_credentials(client: &Client, ...) { ... }

would be impossible to implement safely. Instead, I would need to write

struct MyAPIWrapper {
   http_client: Client
}

impl MyAPIWrapper {
  fn new() { ...}
  fn login_with_credentials(&self,  ...) {}
}

just to make sure that the client I use for the request has the right redirect policy. This doesn't only have implications on performance and simplicity, but also on testability

@theduke
Copy link
Contributor

theduke commented Mar 4, 2019

I stumbled over this too.

My use case is logging in to a site and retrieving the cookies.
After a redirect, I can't retrieve the Set-Cookie header, and constructing a separate client just for logging in seems wasteful.

I think per-request redirect policy is pretty common in other clients.

@Aehmlo
Copy link

Aehmlo commented May 28, 2019

I'd just like to chime in and say that this is something I'd like to see as well. In my library, I sometimes send a HEAD request to an endpoint and inspect the Location header to determine the current version of an asset. An arbitrary amount of time later, the user can initiate a download of this resource, and I'd like to be able to just hit the endpoint again with a GET and follow the redirect(s) this time. In this specific case, I could use a custom policy to elicit a different behavior for HEAD vs. GET requests, but I don't see any way of accessing the request type from a custom policy.

@norcalli
Copy link

@seanmonstar as far as prior art goes, there's very little in the way of this kind of fine grained support, but there is support for the concept of middleware in the Request -> Response chain. The concept of default headers is a Request -> Request middleware, and cookies is Request -> Request for writing Cookie: and Response -> Response for Set-Cookie: propagation to the cookie jar and Response -> Request -> Response = Response -> Response for following redirects.

Viewed this way, we've already taken the plunge into adding this kind of functionality, except in an ad-hoc manner. Given the existence of these existing hooks, I think that allowing Response middleware via some kind of wrapping or middleware mechanism would be appropriate. I know you're already doing this in tower, basically, and warp, so perhaps it's not worth the full shebang in reqwest, but it seems like allowing adding Response hooks and disabling automatic redirects would satisfy a lot of usecases.

@kathampy
Copy link

kathampy commented Aug 29, 2019

When each request is a on behalf of a different "customer", different policies are required on a per-request basis (connection pooling only within a request, redirect policy, insecure TLS, root CAs, client certificates, DNS cache).
Reusing the client is currently impossible, and instantiating a new client is heavy and kills performance in my benchmarks. A brief inspection shows that a new TLS trust store is created from scratch each time within rustls, among other things.

@Boscop
Copy link

Boscop commented Apr 10, 2020

I have a similar situation where I only don't want to follow redirects for specific requests:
The situation is basically that the response sends a 302 Found with Content-Disposition: attachment; filename=.. header, but the body already contains the file data, and following the redirect would lead to a html page (not the desired file).
It's not really expressible as a policy because it's very context dependent.
So please allow overriding the redirect policy per request :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants