HFQR / xitca-web

Apache License 2.0
717 stars 43 forks source link

Xitca Return a router to Main App #883

Closed andodeki closed 8 months ago

andodeki commented 8 months ago

I am trying to implement a Xitca router with state but I am having trouble. I have an axum implementation as below in system.rs

const NAME: &str = "Nigiginc HTTP\n";
const PONG: &str = "pong\n";

pub fn router(state: Arc<AppState>, metrics_config:
&HttpMetricsConfig) -> Router {
    let mut router = Router::new()
        .route("/", get(|| async { NAME }))
        .route("/ping", get(|| async { PONG }))
        .route("/stats", get(get_stats));
    if metrics_config.enabled {
        router = router.route(&metrics_config.endpoint, get(get_metrics));
    }

    router.with_state(state)
}

then in main.rs i have

let app_state = build_app_state(&config, system).await;
    let mut app = Router::new()
        .merge(system::router(app_state.clone(), &config.metrics))
        .merge(users::router(app_state.clone()))
        .layer(middleware::from_fn_with_state(app_state.clone(), jwt_auth));

So the question is can i return a router from another file and merger the router in the main where Xitca server is started?

fakeshadow commented 8 months ago

Right now xitca-web does not have dedicated api for merging routers. Instead you can pass the App by value to your modules where it needs to be configured. for example:

use xitca_web::{handler::handler_service, route::get, App, NestApp};

fn main() -> std::io::Result<()> {
    let mut app = App::new();
    app = mod1::route(app, ());
    app.serve().bind("127.0.0.1:8080")?.run().wait()
}

mod mod1 {
    use super::*;

    // config app and return it to main.
    pub(super) fn route<C: 'static>(app: NestApp<C>, config: ()) -> NestApp<C> {
        app.at("/", get(handler_service(|| async { "" })))
            .at("/ping", get(handler_service(|| async { "" })))
            .at("/stats", get(handler_service(|| async { "" })))
    }
}

This is not perfect and we are still debating how to introduce a better api for scoped configuration and ideas are welcomed. That said in general we want to keep the public api simple and introduce advanced feature in the form of types and additive functions rather than slash another App::xxxx method on it.

andodeki commented 8 months ago

it seems to work. Thanks. What about passing state?

fakeshadow commented 8 months ago

yea the state part is exactly where it's not perfect. There are some additional types needed to be exported to describe a stateful App. It's absent for now due to more public types could be bloating the api surface and confuse people. We would try to ship it in 0.2 release where a stateful App can be annotated in a single type. For now the only way to add state to App with this pattern is to use runtime state. for example:

app.enclosed(xitca_web::middleware::Extension::new(String::new()))
    .serve()
    .bind("127.0.0.1:8080")?
    .run()
    .wait()

// in your handler function:
use xitca_web::handler::extension::ExtensionRef;
// ExtensionRef is a zero copy runtime checked state extractor.
async fn handler(ExtensionRef(string): ExtensionRef<'_, String>) -> &'static str {
    todo!()
}

It's basically the same thing as axum runtime checked state.

andodeki commented 8 months ago

so the initial

let mut app = App::new();

has an error

the trait `Borrow<AppState>` is not implemented for `()`

when i implement with_state

let mut app = App::with_state(app_state.clone());

i get an error arguments to the route function are incorrect expectedApp<AppRouter<...>, ...>, foundApp<AppRouter<_>, Box<...>>`

so i found this https://github.com/HFQR/xitca-web/blob/bb2279011525dd7d46365eae8c6a92408cecd321/web/src/app/mod.rs#L474

i now get an error too

fakeshadow commented 8 months ago

Compile time state through App::with_state would not work with NestApp type (which we want to tackle in next release)

And a self contained example similar to axum for runtime checked state would be like this:

use xitca_web::{
    handler::{extension::ExtensionRef, handler_service},
    middleware::Extension,
    route::get,
    App, NestApp,
};

#[tokio::main]
async fn main() -> std::io::Result<()> {
    let mut app = App::new();
    app = mod1::route(app);

    let state = build_state().await;

    app.enclosed(Extension::new(state))
        .serve()
        .bind("127.0.0.1:8080")?
        .run()
        .await
}

async fn build_state() -> String {
    String::from("hello,world!")
}

mod mod1 {
    use super::*;

    pub(super) fn route<C: 'static>(app: NestApp<C>) -> NestApp<C> {
        app.at("/", get(handler_service(handler)))
            .at("/ping", get(handler_service(handler)))
            .at("/stats", get(handler_service(handler)))
    }
}

async fn handler(ExtensionRef(string): ExtensionRef<'_, String>) -> String {
    string.clone()
}

You can pass the state to your module configuration function if you need it for setting up but rather than that specific case state can just live in main and you don't need actively clone it into any module.

andodeki commented 8 months ago

the sample above is running but when i have my handler as below

async fn get_stats(
    StateRef(state): StateRef<'_, AppState>,
    ExtensionRef(identity): ExtensionRef<'_, Identity>,
) -> Result<Stats, CustomError> {
    let system = state.system.read();
    let stats = system
        .get_stats(&Session::stateless(identity.user_id, identity.ip_address))
        .await?;
    Ok(stats)
}

How do i handle CustomError Enum

use crate::infrastructure::error::Error;
use serde::Serialize;
use thiserror::Error;

#[derive(Debug, Error)]
pub enum CustomError {
    #[error(transparent)]
    Error(#[from] Error),
}
fakeshadow commented 8 months ago

StateRef only works with App::with_state api. For Extension you have to use only ExtensionRef/ExtensionOwn in your handler function.

for error handling please reference the error_handle and macros example. xitca-web use a different style of error handling from most other web frameworks so it would also be helpful if you can give doc a read https://docs.rs/xitca-web/latest/xitca_web/error/index.html

for a quick start you can just do following:

Cargo.toml:
xitca-web = { version = "0.1", features = ["codegen"] }

use xitca_web::codegen::error_impl;

#[derive(Debug, Error)]
pub enum CustomError {
    #[error(transparent)]
    Error(#[from] Error),
}

#[error_impl]
impl CustomError {
    async fn call<C>(&self, ctx: WebContext<'_, C>) -> WebResponse {
        // logic of generating a response from your error. 
    }
}

Then you would be able to return Result<_, CustomError> in your handler function.

andodeki commented 8 months ago

With the same handler i get this error

error[E0277]: the trait bound `C: Borrow<AppState>` is not satisfied
   --> src/http/xitcav_http/system.rs:64:23
    |
64  |         .at("/stats", get(handler_service(get_stats)))
    |          --           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Borrow<AppState>` is not implemented for `C`
    |          |
    |          required by a bound introduced by this call
    |

   error[E0277]: the trait bound `for<'r2> models::stats::Stats: Responder<WebContext<'r2, C>>` is not satisfied
   --> src/http/xitcav_http/system.rs:64:23
    |
64  |         .at("/stats", get(handler_service(get_stats)))
    |          --           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `for<'r2> Responder<WebContext<'r2, C>>` is not implemented for `models::stats::Stats`
    |          |
    |          required by a bound introduced by this call
    |

    error[E0277]: the trait bound `for<'r2> models::stats::Stats: Responder<WebContext<'r2, C>>` is not satisfied
   --> src/http/xitcav_http/system.rs:64:10
    |
64  |         .at("/stats", get(handler_service(get_stats)))
    |          ^^ the trait `for<'r2> Responder<WebContext<'r2, C>>` is not implemented for `models::stats::Stats`
    |
fakeshadow commented 8 months ago

Have you removed StateRef(state): StateRef<'_, AppState> from your handler argument?

andodeki commented 8 months ago

Yes, on a different handler i have

async fn get_metrics(
    ctx: &WebContext<'_, AppState>,
) -> Result<String, XitcaCustomError> {
    let system = ctx.state().system.read();
    Ok(system.metrics.get_formatted_output())
}
fakeshadow commented 8 months ago

yea ok the AppState type has to be removed too. Without App::with_state API your application always have empty compile time state. In other word you should use &WebContext<'_>

andodeki commented 8 months ago

if i have &WebContext<'_> as the type i can have access to my state. Also when i use App::with_state i have arguments error on the route function type parameter.

fakeshadow commented 8 months ago

Let me get this straight. StateRef<'_, C> and WebContext<'_, C> ONLY work with App::with_state(<C>). The way you are setting up router making it impossible to make use of them. You have to use Extension coupled with ExtensionRef to construct and access your state.

andodeki commented 8 months ago

what about this implementation

 use xitca_web::{handler::handler_service, App, NestApp, WebContext};
 // a function return an App instance.
fn app() -> NestApp<usize> {
    App::new().at("/index", handler_service(|_: &WebContext<'_, usize>| async { "" }))
 }

 // nest app would be registered with /v2 as prefix therefore "/v2/index" become accessible.
App::with_state(996usize).at("/v2", app());
fakeshadow commented 8 months ago

NestApp is supposed to be used for nesting routers. It works similar to axum's Router::nest API.

It's by accident that it can be used to partially enable similar usage for axum's Router::merge API. But obviously at the cost of compile time state API.

andodeki commented 8 months ago

I have this handler

pub async fn get_stats(
    StateRef(state): StateRef<'_, AppState>,
    ExtensionRef(identity): ExtensionRef<'_, Identity>,
) -> Result<Stats, XitcaCustomError> {
    let system = state.system.read();
    let stats = system
        .get_stats(&Session::stateless(identity.user_id, identity.ip_address))
        .await?;
    Ok(stats)
}

when i use

let server = App::with_state(app_state)
        .at("/", get(handler_service(|| async { NAME })))
        .at("/ping", get(handler_service(|| async { PONG })))
        .at("/metrics", get(handler_service(get_metrics)))
        .at("/stats", get(handler_service(get_stats)))
        .serve()
        .listen(listener)
        .unwrap()
        .run();

i get this error

    |          ^^ the trait `for<'r2> Responder<WebContext<'r2, _, _>>` is not implemented for `models::stats::Stats`
fakeshadow commented 8 months ago

you have to impl Responder trait for your Stat type. for example:

use xitca_web::{handler::Responder, WebContext, http::WebResponse, error::Error};

impl<'r, C, B> Responder<WebContext<'r, C, B>> for Stats {
    type Response = WebResponse;
    type Error = Error<C>;
    async fn respond(self, ctx: WebContext<'r, C, B>) -> Result<Self::Response, Self::Error> {
        // logic for generating response from Stats 
        todo!()
    }
}

it‘s similar to axum's IntoResponse trait but with async

if by any chance you want to use state in the responder trait you can concrete the C type to your AppState

andodeki commented 8 months ago

I noticed Xitca also has Intoresponse So every custom type i return on the handler i have to implement Responder trait for that type?

fakeshadow commented 8 months ago

yea it does. Though IntoResponse is a trait from xitca-http crate. It's only used by xitca-web internally and from public API point of view Responder is what equals to axum's IntoResponse. You can also look into already existing type wrapper. For example xitca_web::handler::json::Json would be able to used as responder type shortcut if your Stats is json object. example:

Cargo.toml:
xitca-web = { version = "0.1", features = ["json"] }

use xitca_web::handler::json::Json;

pub async fn get_stats(
    StateRef(state): StateRef<'_, AppState>,
    ExtensionRef(identity): ExtensionRef<'_, Identity>,
) -> Result<Json<Stats>, XitcaCustomError> {
    let system = state.system.read();
    let stats = system
        .get_stats(&Session::stateless(identity.user_id, identity.ip_address))
        .await?;
    Ok(Json(stats))
}
andodeki commented 8 months ago

Ok thats helpful big time now when i call the endpoint i get this output on the terminal

2024-01-13T18:30:07.929891Z ERROR xitca_http::util::middleware::logger: closed
2024-01-13T18:30:08.206930Z ERROR xitca_http::util::middleware::logger: closed
2024-01-13T18:30:08.485108Z ERROR xitca_http::util::middleware::logger: closed
2024-01-13T18:30:08.760389Z ERROR xitca_http::util::middleware::logger: closed
2024-01-13T18:30:09.036560Z ERROR xitca_http::util::middleware::logger: closed
2024-01-13T18:30:09.311944Z ERROR xitca_http::util::middleware::logger: closed
fakeshadow commented 8 months ago

np. the closed message usually happens when remote client close connection prematurely. Do you have reproducible example of how to trigger it?

andodeki commented 8 months ago

i think its how i hit the endpoint with

for i in {1..10}; do curl http://0.0.0.0:3001/; sleep 1; done;
fakeshadow commented 8 months ago

yea xitca-http is default to strictly expecting client to respect long connection. if curl send a http/1.1 request and does not respect the keep-alive value (by default it's 5 secs) server provides it will be treated as an error. This could be an annoying default for some people and I'm open to change it in the future.

fakeshadow commented 8 months ago

for client respecting long connection you can try to visit your endpoint with browser and see the server print out tracing log when keep-alive is expired and long connection is gracefully closed.

andodeki commented 8 months ago

I wonder in the full example you had give above it does not give that error when i use curl

fakeshadow commented 8 months ago

tracing log is only visible when utilizing tracing and tracing-subscribe crates in your application. The behavior is the same regardless if the log is enabled or not.

fakeshadow commented 8 months ago

For example you can add tracing_subscriber::fmt().with_env_filter("xitca_http=off").init(); where the xitca_http=off would turn off xitca-http's log output.

andodeki commented 8 months ago

Oh i have added tracing to the provided example above and with curl it give the closed error

fakeshadow commented 8 months ago

Let's keep this issue open for now until a better router merging api is implemented. Hopefully in xitca-web 0.2

andodeki commented 8 months ago

Ok

fakeshadow commented 8 months ago

With #884 following code can be used for scoped router configuration:

use xitca_web::{
    handler::{handler_service, state::StateRef},
    route::get,
    App, NestApp,
};

#[tokio::main]
async fn main() -> std::io::Result<()> {
    let state = build_state().await;

    let mut app = App::new();
    app = mod1::route(app);

    app.with_state(state)
        .serve()
        .bind("127.0.0.1:8080")?
        .run()
        .await
}

async fn build_state() -> String {
    String::from("hello,world!")
}

mod mod1 {
    use super::*;

    pub(super) fn route(app: NestApp<String>) -> NestApp<String> {
        app.at("/", get(handler_service(handler)))
    }

    async fn handler(StateRef(state): StateRef<'_, String>) -> String {
        state.clone()
    }
}
andodeki commented 8 months ago

hey sorry for the late reply what version does this work with. Because i have tried patching but not compiling

somewheve commented 8 months ago

try this code based on main branch

andodeki commented 8 months ago

i have

Cargo.toml
xitca-web = { git = "https://github.com/HFQR/xitca-web.git", branch="main" , features = ["codegen", "cookie", "json"]}
xitca-http = { git = "https://github.com/HFQR/xitca-web.git", branch="main" , features = ["http1","http2", "router", "http3", "openssl", "rustls"]}

i get this in terminal

$ cargo r
    Blocking waiting for file lock on package cache
    Updating git repository `https://github.com/HFQR/xitca-web.git`
    Updating crates.io index
error: failed to select a version for the requirement `xitca-router = "^0.2"`
candidate versions found which didn't match: 0.1.0
location searched: crates.io index
required by package `xitca-http v0.2.0 (https://github.com/HFQR/xitca-web.git?branch=main#c4967f60)`
    ... which satisfies git dependency `xitca-http` of package `dev v0.1.0 (/Users/aok/Projects/rustdev/MadeByMakepad/dev)`
perhaps a crate was updated and forgotten to be re-vendored?
somewheve commented 8 months ago
error: failed to select a version for the requirement `xitca-router = "^0.2"`

xitca-route need 0.2?

xitca-route= { git = "https://github.com/HFQR/xitca-web.git", branch="main" , features = ["http1","http2", "router", "http3", "openssl", "rustls"]}

maybe you need add this to Cargo.toml.

andodeki commented 8 months ago

yeah i have added xitca-router

$ cargo r
    Blocking waiting for file lock on package cache
    Updating git repository `https://github.com/HFQR/xitca-web.git`
    Updating crates.io index
error: failed to select a version for the requirement `xitca-router = "^0.2"`
candidate versions found which didn't match: 0.1.0
location searched: crates.io index
required by package `xitca-http v0.2.0 (https://github.com/HFQR/xitca-web.git?branch=main#c4967f60)`
    ... which satisfies git dependency `xitca-http` of package `dev v0.1.0 (/Users/aok/Projects/rustdev/MadeByMakepad/dev)`
perhaps a crate was updated and forgotten to be re-vendored?
somewheve commented 8 months ago
[dependencies]
xitca-web = { version= "0.2",  features = [
    "codegen",
    "cookie",
    "json",
] }
xitca-router = "0.2"
xitca-http = { version="0.2", features = [
    "http1",
    "http2",
    "router",
    "http3",
    "openssl",
    "rustls",
]}

[patch.crates-io]
xitca-router = { git = "https://github.com/HFQR/xitca-web.git", branch = "main" }
xitca-web = { git = "https://github.com/HFQR/xitca-web.git", branch = "main" }
xitca-http = { git = "https://github.com/HFQR/xitca-web.git", branch = "main" }

you need use [patch-crate.io] to overwrite dependency

fakeshadow commented 8 months ago

@andodeki The following style would be better for you:

[dependencies]
xitca-web = { version = "0.2", features = ["codegen", "cookie", "json"] }

[patch.crates-io]
xitca-http = { git = "https://github.com/HFQR/xitca-web.git", rev = "c4967f6" }
xitca-router = { git = "https://github.com/HFQR/xitca-web.git", rev = "c4967f6" }
xitca-web = { git = "https://github.com/HFQR/xitca-web.git", rev = "c4967f6" }

Personally I do not recommend depending on github dep and it's best to stick with crate releases.

andodeki commented 8 months ago

With #884 following code can be used for scoped router configuration:

use xitca_web::{
    handler::{handler_service, state::StateRef},
    route::get,
    App, NestApp,
};

#[tokio::main]
async fn main() -> std::io::Result<()> {
    let state = build_state().await;

    let mut app = App::new();
    app = mod1::route(app);

    app.with_state(state)
        .serve()
        .bind("127.0.0.1:8080")?
        .run()
        .await
}

async fn build_state() -> String {
    String::from("hello,world!")
}

mod mod1 {
    use super::*;

    pub(super) fn route(app: NestApp<String>) -> NestApp<String> {
        app.at("/", get(handler_service(handler)))
    }

    async fn handler(StateRef(state): StateRef<'_, String>) -> String {
        state.clone()
    }
}
error[E0277]: the trait bound `for<'r2> (dyn for<'r> ServiceObject<WebContext<'r, AppState>, for<'r> Error = RouterError<xitca_web::error::Error<AppState>>, for<'r> Response = xitca_http::Response<xitca_http::ResponseBody>> + 'static): xitca_service::Service<WebContext<'r2, Arc<AppState>>>` is not satisfied
   --> src/http/xitcav_http/http_server.rs:205:10
    |
205 |         .serve()
    |          ^^^^^ the trait `for<'r2> xitca_service::Service<WebContext<'r2, Arc<AppState>>>` is not implemented for `(dyn for<'r> ServiceObject<WebContext<'r, AppState>, for<'r> Error = RouterError<xitca_web::error::Error<AppState>>, for<'r> Response = xitca_http::Response<xitca_http::ResponseBody>> + 'static)`
    |

    error[E0277]: the size for values of type `(dyn for<'r> ServiceObject<WebContext<'r, AppState>, for<'r> Error = RouterError<xitca_web::error::Error<AppState>>, for<'r> Response = xitca_http::Response<xitca_http::ResponseBody>> + 'static)` cannot be known at compilation time
   --> src/http/xitcav_http/http_server.rs:205:10
    |
205 |         .serve()
    |          ^^^^^ doesn't have a size known at compile-time
    |
fakeshadow commented 8 months ago

application state is type based. example:

App::new().with_state(AppState::default())  // your state is `AppState` where you should use `WebContext<'_, AppState>` and `Error<AppState>`

App::new().with_state(Arc::new(AppState::default())) // your state is `Arc<AppState>` where you should use `WebContext<'_, Arc<AppState>>`
fakeshadow commented 8 months ago

btw the error message maybe not very clear on what you should do but it still offers a strong clue to it.

the trait `for<'r2> xitca_service::Service<WebContext<'r2, Arc<AppState>>>` is not implemented for `(dyn for<'r> ServiceObject<WebContext<'r, AppState>

For example this message indicate you are trying to use Arc<AppState> as your application state and your route handlers and/or middlewares are expecting WebContext<'r, AppState> and that's the type mismatch you had.

andodeki commented 8 months ago

what about this route function i have it as this

pub(super) fn route(app: NestApp<AppState>) -> NestApp<AppState> {
        app.at("/", get(handler_service(handler)))
    }
fakeshadow commented 8 months ago

yes you have to use the correct type anywhere it's needed.

andodeki commented 8 months ago

so i have

let app_state: Arc<AppState> = build_app_state(&config, system).await;
let mut app = App::new().with_state(app_state);
app = system::routes(app);

but i get mismatched types error with that code on the routes function

fakeshadow commented 8 months ago

you have to use the correct type. correct means the same type as the type your App::with_state received.

andodeki commented 8 months ago

This is the mismatch error

error[E0308]: mismatched types
   --> src/http/xitcav_http/http_server.rs:149:11
    |
148 |     let mut app = App::new().with_state(app_state);
    |                   -------------------------------- expected due to this value
149 |     app = system::routes(app);
    |           ^^^^^^^^^^^^^^^^^^^ expected `App<AppRouter<_>, Box<...>>`, found `App<AppRouter<Box<...>>>`
    |
    = note: expected struct `App<AppRouter<_>, Box<dyn Fn() -> Pin<Box<...>> + Send + Sync>>`
fakeshadow commented 8 months ago
let mut app = App::new();
app = mod1::route(app);

app.with_state(state);

the order matters

andodeki commented 8 months ago

with this order i get the initial error i got

error[E0277]: the trait bound `for<'r2> (dyn for<'r> ServiceObject<WebContext<'r, AppState>, for<'r> Error = RouterError<xitca_web::error::Error<AppState>>, for<'r> Response = xitca_http::Response<xitca_http::ResponseBody>> + 'static): xitca_service::Service<WebContext<'r2>>` is not satisfied
   --> src/http/xitcav_http/http_server.rs:154:10
    |
154 |         .serve()
    |          ^^^^^ the trait `for<'r2> xitca_service::Service<WebContext<'r2>>` is not implemented for `(dyn for<'r> ServiceObject<WebContext<'r, AppState>, for<'r> Error = RouterError<xitca_web::error::Error<AppState>>, for<'r> Response = xitca_http::Response<xitca_http::ResponseBody>> + 'static)`

error[E0277]: the size for values of type `(dyn for<'r> ServiceObject<WebContext<'r, AppState>, for<'r> Error = RouterError<xitca_web::error::Error<AppState>>, for<'r> Response = xitca_http::Response<xitca_http::ResponseBody>> + 'static)` cannot be known at compilation time
   --> src/http/xitcav_http/http_server.rs:154:10
    |
154 |         .serve()
    |          ^^^^^ doesn't have a size known at compile-time
    |
fakeshadow commented 8 months ago

Like I said you have to use the correct type for your application state. You are using different types. Take a look at this line and reference my previous replies.

the trait `for<'r2> xitca_service::Service<WebContext<'r2>>` is not implemented for `(dyn for<'r> ServiceObject<WebContext<'r, AppState>
andodeki commented 8 months ago

i cant figure out where i am using the different type for AppState in the code the routes function or the handler function?