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

Feat-328/add ExtensionMatcher struct #346

Merged
merged 2 commits into from
Oct 30, 2024

Conversation

calebbourg
Copy link
Contributor

Adds two implementations. One for Const and one for closures.

Adds two implementations. One for Const and one for closures (FnBox).
Copy link
Member

@GlenDC GlenDC left a comment

Choose a reason for hiding this comment

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

You were on the right path, I added some code suggestions that you are free to copy as-is or change where you see fit. Some other remarks:

  • In general don't box unless you have to. Thanks to embracing generics we do not need to box here, the way I showed you in my code suggestions is one way on how to avoid it while still allowing multiple variants;
  • Matchers take an optional Extensions struct, yes. But that's not the one to check against. This can be used by matchers to inject extra data (side-effects) that was generated as part of matching. E.g. a URI path matcher can insert there key fragments for its dynamic parts. We have to check instead against the Context which embeds an Extensions. And for the ExtensionMatcher there is no side-effect data to insert. So no need to test for that either.

I know it's a bit tricky, hope this makes sense to you. If not, don't mind asking more follow up questions :)

marker: T,
}

struct Const<T: Send + Sync>(T);
Copy link
Member

Choose a reason for hiding this comment

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

This I would replace with something like the following:

mod private {
    use std::marker::PhantomData;

    pub(super) trait ExtensionPredicate<T>: Send + Sync + 'static {
        fn call(&self, value: &T) -> bool;
    }

    pub(super) struct PredicateConst<T>(pub(super) T);

    impl<T> ExtensionPredicate<T> for PredicateConst<T>
    where
        T: Clone + Eq + Send + Sync + 'static,
    {
        #[inline]
        fn call(&self, value: &T) -> bool {
            self.0.eq(value)
        }
    }

    pub(super) struct PredicateFn<F, T>(pub(super) F, pub(super) PhantomData<fn() -> T>);

    impl<F, T> ExtensionPredicate<T> for PredicateFn<F, T>
    where
        F: Fn(&T) -> bool + Send + Sync + 'static,
        T: Clone + Eq + Send + Sync + 'static,
    {
        #[inline]
        fn call(&self, value: &T) -> bool {
            self.0(value)
        }
    }
}

};
use std::any::Any;

pub struct ExtensionMatcher<T: Send + Sync> {
Copy link
Member

Choose a reason for hiding this comment

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

This is how I would define the struct:

/// A matcher which allows you to match based on an extension,
/// either by comparing with an extension ([`ExtensionMatcher::with_const`]),
/// or using a custom predicate ([`ExtensionMatcher::with_fn`]).
pub struct ExtensionMatcher<P, T> {
    predicate: P,
    _marker: PhantomData<fn() -> T>,
}

struct Const<T: Send + Sync>(T);
struct FnBox(Box<dyn Fn(&dyn Any) -> bool + Send + Sync>);

impl ExtensionMatcher<FnBox> {
Copy link
Member

Choose a reason for hiding this comment

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

This is how you can create the fn variant:

impl<F, T> ExtensionMatcher<private::PredicateFn<F, T>, T>
where
    F: Fn(&T) -> bool + Send + Sync + 'static,
    T: Clone + Eq + Send + Sync + 'static,
{
    /// Create a new [`ExtensionMatcher`] with a predicate `F`,
    /// that will be called using the found `T` (if one is present),
    /// to let it decide whether or not the extension value is a match or not.
    pub fn with_fn(predicate: F) -> Self {
        Self {
            predicate: private::PredicateFn(predicate, PhantomData),
            _marker: PhantomData,
        }
    }
}

}
}

impl<T: Send + Sync + Sync + std::cmp::PartialEq> ExtensionMatcher<Const<T>> {
Copy link
Member

Choose a reason for hiding this comment

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

This can be replaced with the following:

impl<T> ExtensionMatcher<private::PredicateConst<T>, T>
where
    T: Clone + Eq + Send + Sync + 'static,
{
    /// Create a new [`ExtensionMatcher`] with a const value `T`,
    /// that will be [`Eq`]-checked to find a match.
    pub fn with_const(value: T) -> Self {
        Self {
            predicate: private::PredicateConst(value),
            _marker: PhantomData,
        }
    }
}

}
}

impl<T: Send + Sync + std::cmp::PartialEq + 'static, State, Request> Matcher<State, Request>
Copy link
Member

Choose a reason for hiding this comment

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

Thanks to the private trait trick you can now just implement the ExtensionMatcher with just 1 impl block:

impl<State, Request, P, T> Matcher<State, Request> for ExtensionMatcher<P, T>
where
    State: Clone + Send + Sync + 'static,
    Request: Send + 'static,
    T: Clone + Send + Sync + 'static,
    P: private::ExtensionPredicate<T>,
{
    fn matches(&self, _ext: Option<&mut Extensions>, ctx: &Context<State>, _req: &Request) -> bool {
        ctx.get::<T>()
            .map(|v| self.predicate.call(v))
            .unwrap_or_default()
    }
}

rama-core/src/matcher/ext.rs Show resolved Hide resolved
Removes boxing and provides a typed abstraction for predicates.
Design and offered by Glen De Cauwsemaecker <[email protected]>
@calebbourg
Copy link
Contributor Author

@GlenDC Thank you for the feedback and help! I learned a tremendous amount in just reading and applying your suggestions.
0b6d8d7

@GlenDC GlenDC merged commit 77ccf5e into plabayo:main Oct 30, 2024
32 checks passed
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 this pull request may close these issues.

2 participants