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

Idea: Split FromRequest trait into three (like the Fn traits) #1115

Closed
jplatte opened this issue Jun 23, 2022 · 8 comments · Fixed by #1272
Closed

Idea: Split FromRequest trait into three (like the Fn traits) #1115

jplatte opened this issue Jun 23, 2022 · 8 comments · Fixed by #1272
Assignees
Labels
A-axum-core C-musings Category: musings about a better world E-hard Call for participation: Experience needed to fix: Hard / a lot
Milestone

Comments

@jplatte
Copy link
Member

jplatte commented Jun 23, 2022

FromRequest with a shared-reference parameter would mostly be useful for middleware or other places where you have a request that you want to pull some bits out of while knowing you're not going to modify it for the final handler.
FromRequestMut would be used for all but the last extractor and FromRequestOnce would be used for the last one.

I think this could even obviate the need for the RequestParts type.

I'll mark as hard because I think it requires changes to all of the crates, including both macro_rules! and proc-macro code.

@jplatte jplatte added C-musings Category: musings about a better world E-hard Call for participation: Experience needed to fix: Hard / a lot breaking change A PR that makes a breaking change. A-axum-core labels Jun 23, 2022
@davidpdrsn
Copy link
Member

I think this sounds very promising. I'm gonna explore it!

@davidpdrsn
Copy link
Member

I give this a quick shut but ran into some issues

trait IntoResponse {}
impl IntoResponse for () {}

struct Request<B>(B);

trait FromRequestOnce<B>: Sized {
    type Rejection: IntoResponse;

    fn from_request_once(req: Request<B>) -> Result<Self, Self::Rejection>;
}

trait FromRequestMut<B>: FromRequestOnce<B> + Sized {
    fn from_request_mut(req: &mut Request<B>) -> Result<Self, Self::Rejection>;
}

// I think this impl would be nice, otherwise you have to manually implement both traits all the
// time
impl<B, T> FromRequestOnce<B> for T
where
    T: FromRequestMut<B>,
{
    // what type should this be? `Rejection` is defined in `FromRequestOnce` which is the trait
    // we're implementing, but we're delegating to `FromRequestMut` which doesn't have it...
    type Rejection = T::Rejection;

    fn from_request_once(mut req: Request<B>) -> Result<Self, Self::Rejection> {
        T::from_request_mut(&mut req)
    }
}

// error[E0275]: overflow evaluating the requirement `<() as FromRequestOnce<B>>::Rejection == _`
impl<B> FromRequestMut<B> for () {
    fn from_request_mut(req: &mut Request<B>) -> Result<Self, Self::Rejection> {
        todo!()
    }
}

@jplatte
Copy link
Member Author

jplatte commented Jun 23, 2022

Hm, I wonder whether the trait hierarchy is even needed, maybe the blanket impls are enough. In that case I think the solution would be to just have type Rejection on all of the traits.

@davidpdrsn
Copy link
Member

Yeah that was my first though as well but that leads to conflicting impls:

trait FromRequestOnce<B> {
    type Rejection: IntoResponse;

    fn from_request_once(req: Request<B>) -> Result<Self, Self::Rejection>;
}

trait FromRequestMut<B>: Sized {
    type Rejection: IntoResponse;

    fn from_request_mut(req: &mut Request<B>) -> Result<Self, Self::Rejection>;
}

impl<B, T> FromRequestOnce<B> for T
where
    T: FromRequestMut<B>,
{
    type Rejection = T::Rejection;

    fn from_request_once(mut req: Request<B>) -> Result<Self, Self::Rejection> {
        T::from_request_mut(&mut req)
    }
}

// error[E0119]: conflicting implementations of trait `FromRequestOnce<_>` for type `Bytes`
impl<B> FromRequestOnce<B> for Bytes {
    fn from_request_once(mut req: Request<B>) -> Result<Self, Self::Rejection> {
        todo!()
    }
}

@davidpdrsn
Copy link
Member

Something that does work is

trait FromRequestOnce<B>: Sized {
    type Rejection: IntoResponse;

    fn from_request_once(req: Request<B>) -> Result<Self, Self::Rejection>;
}

// this works because there is no blanket impl
// but it requires users to manually write a `FromRequestOnce` impl that
// delegates to `FromRequestMut`
// maybe thats okay..?
trait FromRequestMut<B>: FromRequestOnce<B> + Sized {
    fn from_request_mut(req: &mut Request<B>) -> Result<Self, Self::Rejection>;
}

impl<B> FromRequestOnce<B> for Bytes {
    type Rejection = ();

    fn from_request_once(req: Request<B>) -> Result<Self, <Self as FromRequestOnce<B>>::Rejection> {
        todo!()
    }
}

impl<B> FromRequestMut<B> for () {
    fn from_request_mut(req: &mut Request<B>) -> Result<Self, Self::Rejection> {
        todo!()
    }
}

// could we provide a macro to generate this?
impl<B> FromRequestOnce<B> for () {
    type Rejection = ();

    fn from_request_once(mut req: Request<B>) -> Result<Self, Self::Rejection> {
        <Self as FromRequestMut<B>>::from_request_mut(&mut req)
    }
}

@davidpdrsn davidpdrsn added this to the 0.6 milestone Jun 25, 2022
@davidpdrsn
Copy link
Member

Maybes something like this

struct RequestParts<R, B>(B, PhantomData<R>);

struct Mut;
struct Owned;

impl<R, B> RequestParts<R, B> {
    // fn headers()
    // fn extensions()
    // ...
}

// only `RequestParts<Owned, _>` allows you to take the body
impl<B> RequestParts<Owned, B> {
    // fn take_body()
}

// `R` is either `Mut` or `Owned`
trait FromRequest<R, B>: Sized {
    type Rejection: IntoResponse;

    fn from_request(req: &mut RequestParts<R, B>) -> Result<Self, Self::Rejection>;
}

// this impl applies to any `R`, i.e. both `Mut` and `Owned`
impl<R, B> FromRequest<R, B> for () {
    type Rejection = ();

    fn from_request(req: &mut RequestParts<R, B>) -> Result<Self, Self::Rejection> {
        todo!()
    }
}

// this only works for `Owned`
impl<B> FromRequest<Owned, B> for Bytes {
    type Rejection = ();

    fn from_request(req: &mut RequestParts<Owned, B>) -> Result<Self, Self::Rejection> {
        todo!()
    }
}

// `Handler` impls

trait Handler<T, B> {}

impl<B, F> Handler<(), B> for F where F: Fn() {}

impl<B, F, T1> Handler<(T1,), B> for F
where
    F: Fn(T1),
    T1: FromRequest<Owned, B>,
{
}

impl<B, F, T1, T2> Handler<(T1, T2), B> for F
where
    F: Fn(T1, T2),
    T1: FromRequest<Mut, B>,
    T2: FromRequest<Owned, B>,
{
}

@davidpdrsn
Copy link
Member

I took stab at implement that in #1121. Looking very promising!

@jplatte
Copy link
Member Author

jplatte commented Jul 19, 2022

Regarding the conflicting impls, this only happens when the trait has a generic parameter. Without that, it works. I guess it's allowed for downstream crates to implement FromRequestMut<LocalType> for Bytes and that's why there is a conflict?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-axum-core C-musings Category: musings about a better world E-hard Call for participation: Experience needed to fix: Hard / a lot
Projects
None yet
2 participants