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 util::option_layer and ServiceBuilder::option_layer #553

Closed
kazk opened this issue Feb 9, 2021 · 3 comments · Fixed by #555
Closed

Add util::option_layer and ServiceBuilder::option_layer #553

kazk opened this issue Feb 9, 2021 · 3 comments · Fixed by #555

Comments

@kazk
Copy link
Contributor

kazk commented Feb 9, 2021

Hi, thanks for tower. kube is now using it under the hood (replaced reqwest with tower + hyper) 🎉

We have a conditional Layer to handle certain authentication methods and found ServiceBuilderExt::optional_layer in tonic very useful. Can we have that included in tower?

We use it like the following:

let maybe_auth = match &auth {
    Authentication::None => None,
    Authentication::Basic(s) => {
        default_headers.insert(
            AUTHORIZATION,
            HeaderValue::from_str(s).map_err(Error::InvalidBasicAuth)?,
        );
        None
    }
    Authentication::Token(s) => {
        default_headers.insert(
            AUTHORIZATION,
            HeaderValue::from_str(s).map_err(Error::InvalidBearerToken)?,
        );
        None
    }
    Authentication::RefreshableToken(r) => Some(AuthLayer::new(r.clone())),
};
let common = ServiceBuilder::new()
    // ... omitted
    .map_request(move |r| set_default_headers(r, default_headers.clone()))
    .into_inner();
ServiceBuilder::new()
    .layer(common)
    .optional_layer(maybe_auth)
    .service(client)
@hawkw
Copy link
Member

hawkw commented Feb 9, 2021

I think something like that would be great --- are you interested in opening a PR?

@kazk
Copy link
Contributor Author

kazk commented Feb 9, 2021

Sure, I can try. The credit goes to @LucioFranco.

impl<L> ServiceBuilder<L> {
    // ...
    #[cfg(feature = "util")]
    #[cfg_attr(docsrs, doc(cfg(feature = "util")))]
    fn optional_layer<T>(self, inner: Option<T>) -> ServiceBuilder<Stack<OptionalLayer<T>, L>> {
        self.layer(OptionalLayer::new(inner))
    }
}


#[derive(Clone, Debug)]
pub struct OptionalLayer<L> {
    inner: Option<L>,
}
impl<L> OptionalLayer<L> {
    pub fn new(inner: Option<L>) -> Self {
        OptionalLayer { inner }
    }
}
impl<S, L> Layer<S> for OptionalLayer<L>
where
    L: Layer<S>,
{
    type Service = crate::util::Either<L::Service, S>;
    fn layer(&self, s: S) -> Self::Service {
        if let Some(inner) = &self.inner {
            crate::util::Either::A(inner.layer(s))
        } else {
            crate::util::Either::B(s)
        }
    }
}

Where should OptionalLayer go? Looking at other methods, it should probably be under tower/src/util/, but there's already another Optional.

@davidpdrsn
Copy link
Member

Great idea! We actually have basically the same thing at Embark but I didn't consider upstreaming it. What we have:

use tower::{layer::util::Identity, util::Either};

/// Convert an `Option<Layer>` into a `Layer`
pub fn option_layer<L>(layer: Option<L>) -> Either<L, Identity> {
    if let Some(layer) = layer {
        Either::A(layer)
    } else {
        Either::B(Identity::new())
    }
}

This uses the fact that Either<Layer1, Layer2> is also a Layer.

I think a decent setup would be to have an option_layer function exposed at tower::util::option_layer and then adding another method to ServiceBuilder that calls it with an Option<L>.

In my opinion we don't need a new OptionalLayer type.

I also prefer the name OptionLayer (or option_layer) rather than OptionalLayer as the former is more closely aligned to Option.

kazk added a commit to kazk/tower that referenced this issue Feb 9, 2021
@kazk kazk changed the title Add ServiceBuilder::optional_layer Add tower::util::option_layer and ServiceBuilder::option_layer Feb 9, 2021
kazk added a commit to kazk/tower that referenced this issue Feb 9, 2021
kazk added a commit to kazk/tower that referenced this issue Feb 9, 2021
kazk added a commit to kazk/tower that referenced this issue Feb 9, 2021
kazk added a commit to kazk/tower that referenced this issue Feb 9, 2021
@kazk kazk changed the title Add tower::util::option_layer and ServiceBuilder::option_layer Add util::option_layer and ServiceBuilder::option_layer Feb 9, 2021
davidpdrsn pushed a commit that referenced this issue Feb 10, 2021
* Add `util::option_layer` and `ServiceBuilder::option_layer`

Closes #553

* Apply suggestions to improve the docs

Co-authored-by: Eliza Weisman <[email protected]>

Co-authored-by: Eliza Weisman <[email protected]>
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.

3 participants