tower-rs / tower-http

HTTP specific Tower utilities.
680 stars 159 forks source link

follow-redirect: Better integration with user-side redirection handling #404

Open tesaguri opened 1 year ago

tesaguri commented 1 year ago

Feature Request

Motivation

Users may want to implement their own redirection logics like <meta http-equiv="refresh" />. In that case, they might want to delegate to the FollowRedirect middleware's Policy object so that they can reuse the Limited policy's redirection count for example. But the current FollowRedirect middleware doesn't provide access to the policy object.

Proposal

Expose a constructor for Attempt and add a response extension type that contains the policy that was used in the request. With the new interface (let's call the extension type PolicyExtension for now), the motivating use case can be written like the following:

use http::StatusCode;
use tower_http::follow_redirect;
use tower_http::follow_redirect::policy::{Policy, PolicyExt};

let mut redirect_policy = follow_redirect::policy::Standard::default().or::<_, (), _>(Err(
    follow_redirect::policy::redirect_fn(|_| Err(MyError::TooManyRedirects)),
));
loop {
    let mut res = FollowRedirect::with_policy(&mut client, policy)
        .call(prepare_request(uri))
        .await?;

    if let ResponseKind::RedirectTo(location) = process_response(&mut res) {
        uri = location;
    } else {
        break;
    };

    redirect_policy = res
        .extensions_mut()
        .remove::<follow_redirect::PolicyExtension>()
        .unwrap()
        .0;

    let previous = &res
        .extensions()
        .get::<follow_redirect::RequestUri>()
        .unwrap()
        .0;
    // This is not quite readable. Maybe this should be a builder instead?
    let attempt = follow_redirect::policy::Attempt::new(StatusCode::FOUND, &uri, previous);
    assert!(redirect_policy.redirect(&attempt)?.is_follow());
}

A drawback would be that it doesn't let the user access the request body, and so the user should only use it for performing a request with an empty body (even if they can clone the original body, since the body might have been replaced with the BodyRepr::Empty variant), which I think should be explained in the documantation.

Another drawback would be that exposing a constructor for Attempt limits the possibility for future API change. If we were going to add an accessor for HeaderMap for example, we would need to either make it return an Option or make it return an empty map as a placeholder in the case of user-defined redirections, which might not be very useful for implementers of Policy.

Alternatives

Add the extension type, but do not expose the constructor for Attempt

With this alternative, the user would still be able to reuse the policy object in the subsequent request, but the user wouldn't be able to inform the policy object of the user-defined redirection.

We might be able to mitigate the pain by adding some inspection methods to the built-in policy types that return the policy's state like the remaining counter of a Limited, so that the user can, for example, check for the counter by themselves and reconstruct a policy object with a decremented counter, but that still would feel a little awkward. Though inspection methods themselves may be useful regardless of this alternative.

Return the request body too

This might allow some requests with non-empty body to continue, but the user still wouldn't be able to distinguish between BodyRepr::Empty and BodyRepr::None, which might be confusing and potentially error-prone.

Do nothing

The user would still be able to implement their redirection logic, resetting the policy object in every user-defined redirection. The user would need to implement their own logic to emulate the policies like Limited, and that logic wouldn't be able to share data with FollowRedirect's policy object.