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

Custom with filters #16

Closed
bbatha opened this issue Aug 2, 2018 · 21 comments
Closed

Custom with filters #16

bbatha opened this issue Aug 2, 2018 · 21 comments
Labels
rfc Extra attention is needed

Comments

@bbatha
Copy link

bbatha commented Aug 2, 2018

Under the current api, Filter::with requires that the type be W: Wrap however the Wrap trait is sealed making it difficult to write custom wrapping filters. For instance, I'm trying to write a filter that will forward a header from a request to the response. So far I've come up with the filter to generate the either grab the request id or generate a new one:

let req_id = warp::header::<String>("x-request-id").or_else(|_| Ok(("generated".to_owned(),)))

But I can't figure out using the combinators provided how to store the request-id in the filter and use it on the response.

It also seems that you cannot call map after returning a warp::reply in the combinator chain so I can't even make the above work with boiler plate something like

let my_route = req_id.map(warp::reply).map(|id: String, reply: warp::Reply| {
    reply.set_header("x-request-id", id)
})
@seanmonstar
Copy link
Owner

Right, the with and Wrap thing is sealed since it is pretty new, and I wasn't sure yet if it could fit all needs.

In your example, I wouldn't expect req_id.map(warp::reply) to work, since the req_id filter has 1 value extracted, and warp::reply() takes 0 arguments...

Would this work for you?

let my_route = req_id.map(|id| {
    Response::builder()
        .header("x-request-id", id)
        .body("")
})

@bbatha
Copy link
Author

bbatha commented Aug 2, 2018

Right, the with and Wrap thing is sealed since it is pretty new, and I wasn't sure yet if it could fit all needs.

Totally understood. The library is excellent so far and after handlers have a lot more complexity.

Would this work for you?

Ya the composition is a little awkward since this is a filter that I'd like to be universally be a applied. But I landed on something like this:

    let req_id = warp::header::<String>("x-request-id").or_else(|_| {
        Ok(("deadbeef".to_owned(),))
    }).map(|id: String| {
        let mut res = Response::builder();
        res.header("x-request-id", id.as_str());

        res
    }).map(|mut res: response::Builder| {
        res.body("bar").unwrap()
    });

Which allows me to compose different body handlers after, and filters that don't pass along arguments before.

@kamalmarhubi
Copy link
Contributor

I'm learning that writing these is kinda non-trivial: you need to name the Wrapped filter type, which means you effectively have to write the Filter impl by hand, and avoid any closures. This in turn leads you to need to write a Future impl by hand. I hope some of this gets easier with existential types, but in the meantime it's kinda rough.

This abstraction, or one like it, is pretty necessary though: it's the only way to put something on "both sides" of a filter, which is a common middleware requirement.

@seanmonstar
Copy link
Owner

@kamalmarhubi yep exactly! I've been working on a CORS filter, and it needs to be a Wrap, and I want to make use of the other filters, so I've resigned it to just returning Filter::boxed() for now.

@seanmonstar seanmonstar added the rfc Extra attention is needed label Aug 13, 2018
@chachi
Copy link

chachi commented Jan 1, 2020

Has there been any progress on this issue recently? Warp is awesome and adding the ability to inject middleware using custom with filters is the last major piece I'm missing for productivity.

@jxs
Copy link
Collaborator

jxs commented Jan 14, 2020

I think this can now be achieved via:

use std::convert::Infallible;
use warp::Filter;
use hyper::{Body, Request};
use tower_service::Service;

#[tokio::main]
async fn main() -> Result<(), hyper::error::Error> {
    let route = warp::any().map(|| "Hello From Warp!");
    let mut warp_svc = warp::service(route);
    let make_svc = hyper::service::make_service_fn(move |_| async move {
        let svc = hyper::service::service_fn(move |req: Request<Body>| async move {
            println!("before request");
            let resp = warp_svc.call(req).await;
            println!("after request");
            resp
        });
        Ok::<_, Infallible>(svc)
    });

    hyper::Server::bind(&([127, 0, 0, 1], 3030).into())
        .serve(make_svc)
        .await?;

    Ok(())
}

@fhsgoncalves
Copy link

fhsgoncalves commented Jan 24, 2020

@jxs it worked for me, but my use case is a little more complex:

If I change the route to be a BoxedFilter (to turn easier return the route from a function):

let route = warp::any().map(|| "Hello From Warp!").boxed();

It fails to compile:

$ cargo run

   Compiling warp-report-1 v0.1.0 (/home/fernando/repos/sandbox/warp-report-1)
error[E0507]: cannot move out of `warp_svc`, a captured variable in an `FnMut` closure
  --> src/main.rs:11:83
   |
9  |       let mut warp_svc = warp::service(route);
   |           ------------ captured outer variable
10 |       let make_svc = hyper::service::make_service_fn(move |_| async move {
11 |           let svc = hyper::service::service_fn(move |req: Request<Body>| async move {
   |  ___________________________________________________________________________________^
12 | |             println!("before request");
13 | |             let resp = warp_svc.call(req).await;
   | |                        --------
   | |                        |
   | |                        move occurs because `warp_svc` has type `warp::filter::service::FilteredService<warp::filter::boxed::BoxedFilter<(&str,)>>`, which does not implement the `Copy` trait
   | |                        move occurs due to use in generator
14 | |             println!("after request");
15 | |             resp
16 | |         });
   | |_________^ move out of `warp_svc` occurs here

error[E0507]: cannot move out of `warp_svc`, a captured variable in an `FnMut` closure
  --> src/main.rs:10:72
   |
9  |       let mut warp_svc = warp::service(route);
   |           ------------ captured outer variable
10 |       let make_svc = hyper::service::make_service_fn(move |_| async move {
   |  ________________________________________________________________________^
11 | |         let svc = hyper::service::service_fn(move |req: Request<Body>| async move {
12 | |             println!("before request");
13 | |             let resp = warp_svc.call(req).await;
   | |                        --------
   | |                        |
   | |                        move occurs because `warp_svc` has type `warp::filter::service::FilteredService<warp::filter::boxed::BoxedFilter<(&str,)>>`, which does not implement the `Copy` trait
   | |                        move occurs due to use in generator
...  |
17 | |         Ok::<_, Infallible>(svc)
18 | |     });
   | |_____^ move out of `warp_svc` occurs here

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0507`.
error: could not compile `warp-report-1`.

To learn more, run the command again with --verbose.

And if I don't use the boxed filter, but add "state" filter:

    let http_client = hyper::Client::new();

    let route = warp::any()
            .and(warp::any().map(move || http_client.clone()))
            .map(|_http_client| "Hello From Warp!");

it will fail with a similar error:

$ cargo run
   Compiling warp-report-1 v0.1.0 (/home/fernando/repos/sandbox/warp-report-1)
error[E0507]: cannot move out of `warp_svc`, a captured variable in an `FnMut` closure
  --> src/main.rs:20:83
   |
18 |       let mut warp_svc = warp::service(route);
   |           ------------ captured outer variable
19 |       let make_svc = hyper::service::make_service_fn(move |_| async move {
20 |           let svc = hyper::service::service_fn(move |req: Request<Body>| async move {
   |  ___________________________________________________________________________________^
21 | |             println!("before request");
22 | |             let resp = warp_svc.call(req).await;
   | |                        --------
   | |                        |
   | |                        move occurs because `warp_svc` has type `warp::filter::service::FilteredService<warp::filter::map::Map<warp::filter::and::And<impl warp::filter::Filter+std::marker::Copy, warp::filter::map::Map<impl warp::filter::Filter+std::marker::Copy, [closure@src/main.rs:15:34: 15:61 http_client:hyper::client::Client<hyper::client::connect::http::HttpConnector>]>>, [closure@src/main.rs:16:18: 16:51]>>`, which does not implement the `Copy` trait
   | |                        move occurs due to use in generator
23 | |             println!("after request");
24 | |             resp
25 | |         });
   | |_________^ move out of `warp_svc` occurs here

error[E0507]: cannot move out of `warp_svc`, a captured variable in an `FnMut` closure
  --> src/main.rs:19:72
   |
18 |       let mut warp_svc = warp::service(route);
   |           ------------ captured outer variable
19 |       let make_svc = hyper::service::make_service_fn(move |_| async move {
   |  ________________________________________________________________________^
20 | |         let svc = hyper::service::service_fn(move |req: Request<Body>| async move {
21 | |             println!("before request");
22 | |             let resp = warp_svc.call(req).await;
   | |                        --------
   | |                        |
   | |                        move occurs because `warp_svc` has type `warp::filter::service::FilteredService<warp::filter::map::Map<warp::filter::and::And<impl warp::filter::Filter+std::marker::Copy, warp::filter::map::Map<impl warp::filter::Filter+std::marker::Copy, [closure@src/main.rs:15:34: 15:61 http_client:hyper::client::Client<hyper::client::connect::http::HttpConnector>]>>, [closure@src/main.rs:16:18: 16:51]>>`, which does not implement the `Copy` trait
   | |                        move occurs due to use in generator
...  |
26 | |         Ok::<_, Infallible>(svc)
27 | |     });
   | |_____^ move out of `warp_svc` occurs here

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0507`.
error: could not compile `warp-report-1`.

To learn more, run the command again with --verbose.

The same seems to occur when I added the cors wrapper:

    let route = warp::any()
        .map(|| "Hello From Warp!")
        .with(warp::cors().allow_any_origin());

it failed with a similar error too:

$ cargo run
   Compiling warp-report-1 v0.1.0 (/home/fernando/repos/sandbox/warp-report-1)
error[E0507]: cannot move out of `warp_svc`, a captured variable in an `FnMut` closure
  --> src/main.rs:18:83
   |
16 |       let mut warp_svc = warp::service(route);
   |           ------------ captured outer variable
17 |       let make_svc = hyper::service::make_service_fn(move |_| async move {
18 |           let svc = hyper::service::service_fn(move |req: Request<Body>| async move {
   |  ___________________________________________________________________________________^
19 | |             println!("before request");
20 | |             let resp = warp_svc.call(req).await;
   | |                        --------
   | |                        |
   | |                        move occurs because `warp_svc` has type `warp::filter::service::FilteredService<warp::filters::cors::internal::CorsFilter<warp::filter::map::Map<impl warp::filter::Filter+std::marker::Copy, [closure@src/main.rs:13:14: 13:35]>>>`, which does not implement the `Copy` trait
   | |                        move occurs due to use in generator
21 | |             println!("after request");
22 | |             resp
23 | |         });
   | |_________^ move out of `warp_svc` occurs here

error[E0507]: cannot move out of `warp_svc`, a captured variable in an `FnMut` closure
  --> src/main.rs:17:72
   |
16 |       let mut warp_svc = warp::service(route);
   |           ------------ captured outer variable
17 |       let make_svc = hyper::service::make_service_fn(move |_| async move {
   |  ________________________________________________________________________^
18 | |         let svc = hyper::service::service_fn(move |req: Request<Body>| async move {
19 | |             println!("before request");
20 | |             let resp = warp_svc.call(req).await;
   | |                        --------
   | |                        |
   | |                        move occurs because `warp_svc` has type `warp::filter::service::FilteredService<warp::filters::cors::internal::CorsFilter<warp::filter::map::Map<impl warp::filter::Filter+std::marker::Copy, [closure@src/main.rs:13:14: 13:35]>>>`, which does not implement the `Copy` trait
   | |                        move occurs due to use in generator
...  |
24 | |         Ok::<_, Infallible>(svc)
25 | |     });
   | |_____^ move out of `warp_svc` occurs here

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0507`.
error: could not compile `warp-report-1`.

To learn more, run the command again with --verbose.

I'm new to the language, so I don't know if I'm doing something wrong on the code, but it seems that some filters are missing the implementation of the Copy trait.

The custom with filter is super important to my application, and it is blocking me to release it in production.

Please, let me know if I should do something in a different way, or if I can help in someway!

Thank you!

@asonix
Copy link

asonix commented Jan 24, 2020

In order to avoid the problem of "move out of" errors, you can try putting .clone() on filters. Not all of them implement Copy, but they should all implement Clone.

It may require cloning at each step of the move closures to actually work, but it shouldn't be impossible

@fhsgoncalves
Copy link

fhsgoncalves commented Jan 25, 2020

@asonix thank you! It worked! I've tried clone the route before, but not as much as I needed.

I'll post an example of that here, for future reference if anyone face the same error:

use std::convert::Infallible;
use warp::Filter;
use hyper::{Body, Request};
use tower_service::Service;

#[tokio::main]
async fn main() -> Result<(), hyper::error::Error> {
    let http_client = hyper::Client::new();

    let route = warp::any()
        .and(warp::any().map(move || http_client.clone()))
        .map(|_http_client| "Hello From Warp!")
        .with(warp::cors().allow_any_origin())
        .boxed();

    let warp_svc = warp::service(route);
    let make_svc = hyper::service::make_service_fn(move |_| {
        let warp_svc = warp_svc.clone();
        async move {
            let svc = hyper::service::service_fn(move |req: Request<Body>| {
                let mut warp_svc = warp_svc.clone();    
                async move {
                    println!("before request");
                    let resp = warp_svc.call(req).await;
                    println!("after request");
                    resp
                }
            });
            Ok::<_, Infallible>(svc)
        }
    });

    hyper::Server::bind(&([127, 0, 0, 1], 3030).into())
        .serve(make_svc)
        .await?;

    Ok(())
}

@fahrradflucht
Copy link

fahrradflucht commented Apr 25, 2020

In my opinion the current advised solution of using hyper service conversion to implement "wrapping" functionality isn't good enough. As @hawkw put it in #408, this is just not a very "warpy" API. If I'm not missing something, you can't convert a hyper service back to a filter, which breaks the complete composability chain.

I understood the reason to seal wrap was that @seanmonstar wasn't sure yet if it could fit all needs. This was almost 2 years ago though. The API didn't really change since and PRs like #408 and #513 show that it is useful in its current form. So to move forward with this RFC I think it would make sense to either collect problems we are seeing with the current API, or allow custom with / Wrap implementations in user land.

@kitsuneninetails
Copy link

I must also admit to becoming a bit frustrated with the lack of easy customization with warp filters. One of the strengths of FP is the ability to mix and match and customize due to referential transparency, but warp hides all of its customizable parts behind private(crate) flags, and doesn't provide any hooks to make our own custom filters.

I'm looking to make a custom authentication filter which combines a few steps (getting the auth header, checking the token with a configurable service, and returning the identity data). I can do it with a ton of snaking .ands .and_thens .or_else and .recovers, but it'd be easier if I could just have a .and(authn(service_config)) and I can get all of the twisty passages, all alike into a simple filter for others to use.

Why is this being locked away behind private flags?

@jxs
Copy link
Collaborator

jxs commented Jul 15, 2020

In my opinion the current advised solution of using hyper service conversion to implement "wrapping" functionality isn't good enough. As @hawkw put it in #408, this is just not a very "warpy" API. If I'm not missing something, you can't convert a hyper service back to a filter, which breaks the complete composability chain.

@fahrradflucht as seen with #640 unsealing Wrap also seems not to provide a "warpy" API, with that in mind, would a solution like wrap_fn that allows for a filter to be plugged before and after, as shown in this example provide a better solution?

@jxs jxs mentioned this issue Jul 15, 2020
@hgzimmerman
Copy link
Contributor

I personally like wrap_fn given that unsealing Wrap isn't practical without unsealing Filter as well. I think the linked example demonstrates that it should be possible to copy headers from the request to the response like the OP desires, although I think a concrete example demonstrating that would be nice.
You should be able to convert a Reply into a Response and manipulate its headers with headers_mut().

I do think that its a little weird in that any custom filter will look like .with(wrap_fn(custom())) when .with(custom()) would read much better. It might be worth exploring creating wrap_fn as well as unsealing Wrap so that cleaner .with() calls could be possible.

I've realized my pet issue with getting tracing support doesn't seem to be resolved by just unsealing Wrap because it also relies on warp::route::Route which is private. I'll focus my attention towards the relevant issues and PRs there.

@jxs
Copy link
Collaborator

jxs commented Jul 16, 2020

You should be able to convert a Reply into a Response and manipulate its headers with headers_mut().

yeah that can be achieved by calling into_response on a Reply

I do think that its a little weird in that any custom filter will look like .with(wrap_fn(custom())) when .with(custom()) would read
much better.

that is unfortunately a limitation of lack of specialization, impl<'a, T, F> WrapSealed<F> for &'a T is already being used

@songday
Copy link

songday commented Aug 14, 2020

wrap_fn is nice!
Do you have a plan that ship this feature?

@drogus
Copy link

drogus commented Aug 15, 2020

I would also like to use something like wrap_fn that @jxs implemented. Fwiw I used it in a project and it works as advertised.

Is there any way I could help with it to move it forward? Is there any code needed to ship it? Or is it more about figuring out the best developer ergonomics?

jxs added a commit to jxs/warp that referenced this issue Aug 20, 2020
jxs added a commit to jxs/warp that referenced this issue Aug 20, 2020
@jxs
Copy link
Collaborator

jxs commented Aug 20, 2020

submitted #693

jxs added a commit to jxs/warp that referenced this issue Aug 20, 2020
@apiraino apiraino mentioned this issue Aug 21, 2020
@jxs jxs closed this as completed in ec6d1ab Aug 24, 2020
128f pushed a commit to 128f/warp that referenced this issue Aug 26, 2020
jxs added a commit to jxs/warp that referenced this issue Jun 27, 2021
@thehappycheese
Copy link

thehappycheese commented Jun 1, 2023

@jxs may we please have an example of how to implement the example in the original post using the wrap_fn introduced in #693. I am also trying to implement 'x-request-id' but I am stuck.


edit: After many hours i found a workable solution.
I had to convert my entire program to use Response::builder() instead of yielding string literals (as suggested in @ seanmonstar 's initial response); and I had to insert this into the beginning of every filter chain:

let echo_x_request_id =
  warp::any()
  .and(warp::header::optional::<u64>("x-request-id"))
  .map(|request_id:Option<u64>| {
    let resp = Response::builder();
    if let Some(request_id) = request_id {
      resp.header("x-request-id", format!("{}", request_id))
    }else{
      resp
    }
  });

I wanted to use .with(wrap_fn(...)) and i tried following the example here as mentioned on #693, but the furthest I could get was the following:

use warp::{Filter, reply::Response};

pub fn echo_header_x_request_id<FilterType, ExtractType>(
    filter: FilterType
) -> impl Filter<Extract = (&'static str,)> + Clone + Send + Sync + 'static
where
    FilterType: Filter<
        Extract = (ExtractType,),
        Error = std::convert::Infallible
    > + Clone + Send + Sync + 'static,
    FilterType::Extract: warp::Reply,
{
    warp::any()
        .and(filter)
        .and(warp::header::<i64>("x-request-id"))
        .map(|something:warp::Reply /*???*/, id:i64|{
            // somehow inject the header???
            todo!()
        }).recover(|something|something)
}

If i remove this line .and(warp::header::<i64>("x-request-id")) the type errors go away, but then it is useless.

@kjvalencik
Copy link

@thehappycheese possibly something like this?

use std::convert::Infallible;

use warp::{http::HeaderValue, reply::Response, Filter, Rejection, Reply};

fn add_request_id<F, T>(
    filter: F,
) -> impl Filter<Extract = (Response,), Error = Rejection> + Clone + Send + Sync + 'static
where
    F: Filter<Extract = (T,), Error = Infallible> + Clone + Send + Sync + 'static,
    T: Reply,
{
    static HEADER: &str = "x-request-id";

    warp::any()
        .and(warp::header::optional::<u64>(HEADER))
        .and(filter)
        .map(|id, res: T| {
            let mut res = res.into_response();

            if let Some(id) = id {
                res.headers_mut().insert(HEADER, HeaderValue::from(id));
            }

            res
        })
}

#[tokio::main]
async fn main() {
    let routes = warp::any()
        .map(|| "Hello, World!")
        .with(warp::wrap_fn(add_request_id));

    warp::serve(routes).run(([127, 0, 0, 1], 3030)).await;
}

@thehappycheese
Copy link

thehappycheese commented Jun 3, 2023

@thehappycheese possibly something like this?

Hi @kjvalencik :)

Thankyou so much for for that example it worked for me. I had not yet understood that headers need to be extracted prior to the wrapped filter, and I didn't know about the into_response function.

If you don't mind one more question, is there a reason you used Infallible? Can res.into_response(); panic? I had to replace it with Rejection. Many thanks in advance :)

This is the code I ended up with
use warp::{http::HeaderValue, reply::Response, Filter, Rejection, Reply};
/// Echoes back the `x-request-id` header from the request, if it is present and
/// can be parsed as a `u64`. Otherwise the response is not modified.
/// If the header is present and valid it will be echoed even if the response
/// from `filter` is a rejection (edit: actually no it doesn't seem to add the header to a rejection :( )
///
/// # Example
///
/// ```
/// use warp::{
///     Filter,
///     reply::Reply,
///     http::StatusCode,
///     wrap_fn,
/// };
///
/// let filter = warp::any()
///     .map(|| StatusCode::OK)
///     .with(wrap_fn(echo_x_request_id));
/// ```
pub fn echo_x_request_id<F, T>(
    filter: F,
) -> impl Filter<Extract = (Response,), Error = Rejection> + Clone + Send + Sync + 'static
where
    F: Filter<Extract = (T,), Error = Rejection> + Clone + Send + Sync + 'static,
    T: Reply,
{
    static HEADER: &str = "x-request-id";
    warp::any()
        .and(
            warp::header::optional::<u64>(HEADER) // only echo if valid u64
                .or(warp::any().map(|| None)) // prevent invalid header rejection
                .unify(),
        )
        .and(filter)
        .map(move |id: Option<u64>, reply: T| {
            let mut response = reply.into_response();
            // note response status is not checked; the header will be
            // echo-ed even on a rejection
            // edit: mmm it seems that isn't true. Could be a problem
            if let Some(id) = id {
                response.headers_mut().insert(HEADER, HeaderValue::from(id));
            }
            response
        })
}

@kjvalencik
Copy link

I was using Infallible because my example code was Infallible. I wanted it to be generic over either but I couldn't figure out a way to do it. Mostly because CombineRejection is private and there's no map_err on filters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc Extra attention is needed
Projects
None yet
Development

No branches or pull requests