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

Add FollowRedirect middleware #79

Merged
merged 16 commits into from
Apr 3, 2021
Merged

Conversation

tesaguri
Copy link
Contributor

@tesaguri tesaguri commented Mar 18, 2021

This is a middleware that retries requests to follow HTTP redirections.

This includes Policy trait to customize behavior of the middleware, which is heavily inspired by reqwest::redirect::Policy.

Policy also has an optional method clone_body that tries to clone a request body, like tower::retry::Policy::clone_request. But even if the body cannot be cloned, FollowRedirect uses <ReqBody as Default>::default when <ReqBody as http_body::Body>::size_hint returns zero, so that it can handle common GET requests without clone_body.

The API proposed here is a proof of concept to some extent, and it is fine if this is not fully merged at once (2fe3fea...22212ba 3f8deda...22212ba 3f8deda...0e6a949 is optional in particular).

Fixes #78.

@tesaguri tesaguri changed the title Follow redirect Add FollowEedirect middleware Mar 18, 2021
@tesaguri tesaguri changed the title Add FollowEedirect middleware Add FollowRedirect middleware Mar 18, 2021
Commit 04cc691 added a new type
parameter to `Policy` trait to represent the error type and, as a side
effect, complicated type inference when calling the trait methods. To
mitigate the issue, this commit moves the `Policy` combinators from
trait methods to freestanding functions.

This also adds `select` combinator function, which is useful in
combination with `Action::error`.
@davidpdrsn davidpdrsn added the A-new-middleware Area: new middleware proposals label Mar 20, 2021
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great work on this!

I left some comments but overall I think this setup is nice.

I would also like to hear what @hawkw thinks. Adding her as reviewer.

tower-http/src/follow_redirect/mod.rs Show resolved Hide resolved
tower-http/src/follow_redirect/mod.rs Show resolved Hide resolved
tower-http/src/follow_redirect/mod.rs Outdated Show resolved Hide resolved
tower-http/src/follow_redirect/mod.rs Outdated Show resolved Hide resolved
tower-http/src/follow_redirect/mod.rs Outdated Show resolved Hide resolved
tower-http/src/follow_redirect/policy/join.rs Outdated Show resolved Hide resolved
tower-http/src/follow_redirect/policy/mod.rs Show resolved Hide resolved
tower-http/src/follow_redirect/policy/mod.rs Outdated Show resolved Hide resolved
tower-http/src/follow_redirect/policy/mod.rs Outdated Show resolved Hide resolved
tower-http/src/follow_redirect/policy/select.rs Outdated Show resolved Hide resolved
@davidpdrsn davidpdrsn requested a review from hawkw March 20, 2021 16:58
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks good! I'm gonna try and integrate it with our code at embark as the final test.

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its working quite well! I ended up with something like this:

// automatically follow redirects
.option_layer(self.max_redirects.map(|limit| {
    FollowRedirectLayer::with_policy(
        Limited::new(limit).and::<_, _, HttpError>(clone_body_fn(Body::try_clone)),
    )
}))

@lpil
Copy link
Contributor

lpil commented Mar 25, 2021

Hey really cool!

@davidpdrsn
Copy link
Member

Think I'm just going to merge this. We can always improve things later.

@davidpdrsn davidpdrsn merged commit 0600188 into tower-rs:master Apr 3, 2021
@tesaguri tesaguri deleted the follow-redirect branch April 4, 2021 03:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-new-middleware Area: new middleware proposals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Client-side redirection middleware
3 participants