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

[Feature Request] More granular app state #207

Open
aliemjay opened this issue May 1, 2022 · 6 comments
Open

[Feature Request] More granular app state #207

aliemjay opened this issue May 1, 2022 · 6 comments

Comments

@aliemjay
Copy link
Contributor

aliemjay commented May 1, 2022

I imagine the following api for the same granularity and modularity of actix-web app state, but statically checked.

This can leverage the current BorrowReq trait.

#[derive(XitcaState)]
struct MyState {
    #[state]
    my_str: String,
    #[state]
    config: PayloadExtractionConfig,
}

impl MyState {
    async fn construct() -> Self {
        todo!()
    }
}

fn main() {
    App::with_async_state(MyState::construct)
        .at("/", get(handler_service(handler)));
}


async fn handler(
    _: Bytes, // Extractor that depends on PayloadExtractConfig
    _: StateRef<'_, String>, // granular state extractor
    _: StateReq<'_, MyState>, // should be possible for backward-compatibility
) {
}
@fakeshadow
Copy link
Collaborator

fakeshadow commented May 1, 2022

Good idea. What about codegen only for FromRequest trait instead?
BorrowReq is a trait I would like to keep at http layer and internal. A generic request type without extra trait(exclude std traits) is still a goal for now for xitca_http::util::service module.

@aliemjay
Copy link
Contributor Author

aliemjay commented May 1, 2022

This is how I imagine the implementation:

// desugaring of codegen :
impl AsRef<String> for MyState {}
impl AsRef<PayloadConfig> for MyState {}


impl<'r, 'a, S, S2> FromRequest<'a, WebRequest<'r, S>> for StateRef<'a, S2>
where
  S: AsRef<S2>,
{}

It shouldn't touch xitca_http. Indeed it can be implemented with no change to xitca-web either, but I think it's important to standardize a trait for such use case. AsRef looks suitable but it doesn't have the impl impl<T> AsRef<T> for T.

@fakeshadow
Copy link
Collaborator

fakeshadow commented May 1, 2022

In fact this works. There is a catch though. At least one of the handler have to infer the State type by extract it.
This is problem introduced by the recent change of BuildService trait and I believe the App is failing to infer the State type to the handlers.

// New StateRef implementation.
impl<'a, 'r, S, T> FromRequest<'a, WebRequest<'r, S>> for StateRef<'a, T>
where
    T: 'static,
    S: 'static + Borrow<T>,
{
    type Type<'b> = StateRef<'b, T>;
    type Error = Infallible;
    type Future = impl Future<Output = Result<Self, Self::Error>> where WebRequest<'r, S>: 'a;

    #[inline]
    fn from_request(req: &'a WebRequest<'r, S>) -> Self::Future {
        async move { Ok(StateRef(req.state().borrow())) }
    }
}
// typed state and codegen for borrow fields.
#[derive(Clone, Debug)]
struct State {
    field1: String,
    field2: u32,
}

impl Borrow<String> for State {
    fn borrow(&self) -> &String {
        &self.field1
    }
}

impl Borrow<u32> for State {
    fn borrow(&self) -> &u32 {
        &self.field2
    }
}
// pass state to app.
App::with_current_thread_state(State {
    field1: String::from("state"),
    field2: 996,
})

// extract fields work fine.
async fn handler(
    StateRef(state): StateRef<'_, String>,
    StateRef(state2): StateRef<'_, u32>,
    StateRef(state3): StateRef<'_, State>,
    req: &WebRequest<'_, State>,
) -> String {
    assert_eq!("state", state);
    assert_eq!(&996, state2);
    assert_eq!(state, req.state().field1.as_str());
    state.to_string()
}

@fakeshadow
Copy link
Collaborator

fakeshadow commented May 1, 2022

This is how I imagine the implementation:

// desugaring of codegen :
impl AsRef<String> for MyState {}
impl AsRef<PayloadConfig> for MyState {}


impl<'r, 'a, S, S2> FromRequest<'a, WebRequest<'r, S>> for StateRef<'a, S2>
where
  S: AsRef<S2>,
{}

It shouldn't touch xitca_http. Indeed it can be implemented with no change to xitca-web either, but I think it's important to standardize a trait for such use case. AsRef looks suitable but it doesn't have the impl impl<T> AsRef<T> for T.

I don't understand these std borrow traits. AsRef is basically the same as Borrow from what I see. and judging by the StateRef naming it could be better to use AsRef.
That said the lack of borrow &self could be a problem.

@aliemjay
Copy link
Contributor Author

aliemjay commented May 1, 2022

I think Borrow is suitable too. AFAIK Borrow just have more restriction regarding the impl of Hash to make it suitable for HashTable keys.

@fakeshadow
Copy link
Collaborator

Implemented with #208

This issue should remain open as it's still half way for this feature with two questions left:

  1. Where should the macro live? Currently I put it under xitca-http-codegen.
  2. Better handling inside macro. Support tuple/enum/union, generic types, etc.

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

No branches or pull requests

2 participants