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

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

Open
tesaguri opened this issue Sep 4, 2023 · 0 comments
Open
Labels
T-middleware Topic: middleware

Comments

@tesaguri
Copy link
Contributor

tesaguri commented Sep 4, 2023

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.

@tesaguri tesaguri changed the title follow-redirect: Support user-side redirection handling follow-redirect: Better integration with user-side redirection handling Sep 21, 2023
@jplatte jplatte added the T-middleware Topic: middleware label Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-middleware Topic: middleware
Projects
None yet
Development

No branches or pull requests

2 participants