util: unify and rename combinators #499
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, the `ServiceExt` trait has `with` and `try_with` methods for composing a `Service` with functions that modify the request type, and `map_err` and `map_ok` methods for composing a service with functions that modify the response type. Meanwhile, `ServiceBuilder` has `map_request` for composing a service with a request mapping function, and `map_response` for composing a service with a response-mapping function. These combinators are very similar in purpose, but are implemented using different middleware types that essentially duplicate the same behavior. Before releasing 0.4, we should probably unify these APIs. In particular, it would be good to de-duplicate the middleware service types, and to unify the naming. This commit makes the following changes: - Rename the `ServiceExt::with` and `ServiceExt::try_with` combinators to `map_request` and `try_map_request` - Rename the `ServiceExt::map_ok` combinator to `map_response` - Unify the `ServiceBuilder::map_request` and `ServiceExt::map_request` combinators to use the same `Service` type - Unify the `ServiceBuilder::map_response` and `ServiceExt::map_response` combinators to use the same `Service` type - Unify the `ServiceBuilder::map_err` and `ServiceExt::map_err` combinators to use the same `Service` type - Only take `FnOnce + Clone` when in response/err combinators, which require cloning into the future type. `MapRequest` and `TryMapRequest` now take `FnMut(Request)`s and don't clone them every time they're called - Reexport future types for combinators where it makes sense. - Add a `try_map_request` method to `ServiceBuilder` Closes #498
|
cc @hlb8122 --- sorry for hacking up your code, but i think this should improve the overall API surface a bit! |
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Dec 28, 2020
Currently, `ServiceExt` and `ServiceBuilder` provide combinators for mapping successful responses to other responses, and mapping errors to other errors, but don't provide a way to map between `Ok` and `Err` results. For completeness, this branch adds a new `then` combinator, which takes a function from `Result` to `Result` and applies it when the service's future completes. This can be used for recovering from some errors or for rejecting some `Ok` responses. It can also be used for behaviors that should be run when a service's future completes regardless of whether it completed successfully or not. Depends on #499
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Dec 28, 2020
Currently, `ServiceExt` and `ServiceBuilder` provide combinators for mapping successful responses to other responses, and mapping errors to other errors, but don't provide a way to map between `Ok` and `Err` results. For completeness, this branch adds a new `then` combinator, which takes a function from `Result` to `Result` and applies it when the service's future completes. This can be used for recovering from some errors or for rejecting some `Ok` responses. It can also be used for behaviors that should be run when a service's future completes regardless of whether it completed successfully or not. Depends on #499
seanmonstar
reviewed
Dec 28, 2020
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
…nto eliza/unify-combinators
LucioFranco
approved these changes
Jan 4, 2021
hawkw
added a commit
that referenced
this pull request
Jan 4, 2021
Currently, `ServiceExt` and `ServiceBuilder` provide combinators for mapping successful responses to other responses, and mapping errors to other errors, but don't provide a way to map between `Ok` and `Err` results. For completeness, this branch adds a new `then` combinator, which takes a function from `Result` to `Result` and applies it when the service's future completes. This can be used for recovering from some errors or for rejecting some `Ok` responses. It can also be used for behaviors that should be run when a service's future completes regardless of whether it completed successfully or not. Depends on #499
hawkw
added a commit
that referenced
this pull request
Jan 4, 2021
Currently, `ServiceExt` and `ServiceBuilder` provide combinators for mapping successful responses to other responses, and mapping errors to other errors, but don't provide a way to map between `Ok` and `Err` results. For completeness, this branch adds a new `then` combinator, which takes a function from `Result` to `Result` and applies it when the service's future completes. This can be used for recovering from some errors or for rejecting some `Ok` responses. It can also be used for behaviors that should be run when a service's future completes regardless of whether it completed successfully or not. Depends on #499
Merged
hawkw
added a commit
that referenced
this pull request
Jan 6, 2021
## Motivation It was pointed out that there is currently some overlap between the `try_with` `Service` combinator and `tower::filter` middleware (see #499 (comment) ). `try_with` synchronously maps from a `Request` -> `Result<DifferentRequest, Error>`, while `tower::filter` _asynchronously_ maps from a `&Request` to a `Result<(), Error>`. The key differences are: - `try_with` takes a request by value, and allows the predicate to return a *different* request value - `try_with` also permits changing the _type_ of the request - `try_with` is synchronous, while `tower::filter` is asynchronous - `tower::filter` has a `Predicate` trait, which can be implemented by more than just functions. For example, a struct with a `HashSet` could implement `Predicate` by failing requests that match the values in the hashset. It definitely seems like there's demand for both synchronous and asynchronous request filtering. However, the APIs we have currently differ pretty significantly. It would be nice to make them more consistent with each other. As an aside, `tower::filter` [does not seem all that widely used][1]. Meanwhile, `linkerd2-proxy` defines its own `RequestFilter` middleware, using a [predicate trait][2] that's essentially in between `tower::filter` and `ServiceExt::try_with`: - it's synchronous, like `try_with` - it allows modifying the type of the request, like `try_with` - it uses a trait for predicates, rather than a `Fn`, like `tower::filter` - it uses a similar naming scheme to `tower::filter` ("filtering" rather than "with"/"map"). [1]: https://github.com/search?l=&p=1&q=%22tower%3A%3Afilter%22+extension%3Ars&ref=advsearch&type=Code [2]: https://github.com/linkerd/linkerd2-proxy/blob/24bee8cbc5413b4587a14bea1e2714ce1f1f919a/linkerd/stack/src/request_filter.rs#L8-L12 ## Solution This branch rewrites `tower::filter` to make the following changes: * Predicates are synchronous by default. A separate `AsyncFilter` type and an `AsyncPredicate` trait are available for predicates returning futures. * Predicates may now return a new `Request` type, allowing `Filter` and `AsyncFilter` to subsume `try_map_request`. * Predicates may now return any error type, and errors are now converted into `BoxError`s. Closes #502 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the
ServiceExttrait haswithandtry_withmethods forcomposing a
Servicewith functions that modify the request type, andmap_errandmap_okmethods for composing a service with functionsthat modify the response type. Meanwhile,
ServiceBuilderhasmap_requestfor composing a service with a request mapping function,and
map_responsefor composing a service with a response-mappingfunction. These combinators are very similar in purpose, but are
implemented using different middleware types that essentially duplicate
the same behavior.
Before releasing 0.4, we should probably unify these APIs. In
particular, it would be good to de-duplicate the middleware service
types, and to unify the naming.
This commit makes the following changes:
ServiceExt::withandServiceExt::try_withcombinatorsto
map_requestandtry_map_requestServiceExt::map_okcombinator tomap_responseServiceBuilder::map_requestandServiceExt::map_requestcombinators to use the same
ServicetypeServiceBuilder::map_responseandServiceExt::map_responsecombinators to use the sameServicetypeServiceBuilder::map_errandServiceExt::map_errcombinators to use the same
ServicetypeFnOnce + Clonewhen in response/err combinators, whichrequire cloning into the future type.
MapRequestandTryMapRequestnow take
FnMut(Request)s and don't clone them every time they'recalled
try_map_requestmethod toServiceBuilderCloses #498