Allow async predicate for cors AllowOrigin

[PoOnesNerfect]

This PR allows using async predicate for AllowOrigin.

Motivation

This is required when the allowed origins may change depending on the origin and the requested resource; this often requires accessing some external state such as a database or another service.

Pretty common use case is SaaS like CMS where a user makes API calls to your server from their client side. The user will provide the list of allowed origins that can access their data from the client side.

Solution

AllowOrigin now has a function async_predicate which takes a closure that returns a future that returns bool.

Internally, AllowOrigin::to_header is now AllowOrigin::to_future which returns a future AllowOriginFuture that returns Option<(HeaderName, HeaderValue)>.
For existing cases like exact, list, or predicate, the future returns immediately with the values on the first poll, so it should not incur much (if any) overhead.

One unfortunate thing about this feature is that, because the returned future needs to have a static lifetime, you cannot pass references to it like:

AllowOrigin::async_predicate(|origin, parts| async move { origin == "https://example.com" })

Instead, you must make sure all the values passed into the future have static lifetimes:

AllowOrigin::async_predicate(|origin, parts| {
    let origin = origin.to_owned();
    async move { origin == "https://example.com" }
})

[jplatte]

Thanks for the PR, looks very good! I'm just wondering about one small detail, see inline comment.

[jplatte]

It seems like users would virtually always have to clone the origin HeaderValue, so what do you think about changing this to

Suggested change

	F: Fn(&HeaderValue, &RequestParts) -> Fut + Send + Sync + 'static,
	F: Fn(HeaderValue, &RequestParts) -> Fut + Send + Sync + 'static,

?

[PoOnesNerfect]

That's an interesting point; I've thought about it for a little, and, for now, I'm still leaning towards keeping it as is, for more consistent type signatures.

For most, or all, async predicate use-cases, you would be awaiting on some external state, which requires you to follow the pattern of:

let client = get_api_client();

AllowOrigin::async_predicate(move |origin, parts| {
    let origin = origin.clone();
    let client = client.clone();
    async move {
        client.call_api(origin).await?;
        ...
    }
})

I'm not sure if removing the line let origin = origin.clone(); is a big lift in dx, when the pattern itself is already pretty verbose.
I would rather have the type signatures be consistent with the other allow origin functions.

Also, having origin as owned value may make it feel like there are two ways of doing things.

For example, this would work fine:

AllowOrigin::async_predicate(|origin, parts| async move { some_simple_logic(origin) })

So, users may try to do something like this, which will not compile:

AllowOrigin::async_predicate(|origin, parts| async move { some_simple_logic(origin, parts.uri().path()) })

This may give users a hard time with lifetime errors, if they haven't worked much with async closures.

So, I would just like there to be a single way of doing things, although it might be a bit more annoying, which is:

// this compiles
AllowOrigin::async_predicate(|origin, parts| {
    let origin = origin.clone();
    async move { some_simple_logic(origin) }
})

// this compiles
AllowOrigin::async_predicate(|origin, parts| {
    let origin = origin.clone();
    let path = parts.uri().path().to_owned();
    async move { some_simple_logic(origin, path) }
})

However, I just might be overthinking it too much, when it's just not a big deal, and will just be a better dx.

I don't think my opinion on this is strong, so I will just go with your decision.
If you think this change makes sense, I will make the change and re-request review.

[jplatte]

This is an interesting point about captured variables also needing clone. Could you try changing Fn to FnOnce + Clone? I'm pretty sure that then the extra cloning of captured variables can be internalized into the library as one clone of the closure.

[PoOnesNerfect]

that works! however, if you want to use parts, you still have to clone it before passing it to the future.
Should we also just provide parts as RequestParts instead of &RequestParts?

[PoOnesNerfect]

I wonder if it becomes too costly at that point, and it'd be better to just let the user decide what to pass into the future.

[jplatte]

Yeah for the origin I think it's okay to clone it always, it's extremely likely the user will need it and not that expensive to clone. The RequestParts are much more expensive to clone and virtually never needed in full, or even at all.

[PoOnesNerfect]

makes sense.
I've made the updates, and fixed the docs. Please take a look!

[clowzed]

Hi @jplatte
I'm excited about this feature. My project currently relies on workarounds that this PR could replace, improving efficiency significantly. I'm eager to integrate it without resorting to forking. Can we expedite the merge process?

[PoOnesNerfect]

Hi @jplatte I'm excited about this feature. My project currently relies on workarounds that this PR could replace, improving efficiency significantly. I'm eager to integrate it without resorting to forking. Can we expedite the merge process?

It should be merged within today or tomorrow, assuming there are no other issues. Thanks for waiting.

[clowzed]

Congratulations! Thanks for your work!