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

Remove State and for<'a> as requirements from Middleware and Request #895

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

nyxtom
Copy link
Contributor

@nyxtom nyxtom commented Jul 15, 2022

Note: this would be a breaking change to the ecosystem as it eliminates the generics on all the traits (Server<State>, Listener<State>, Endpoint<State>, Request<State>, Next<'_>, Middleware<State>) however I think we could agree this is a net gain in ergonomics and allows users to add multiple .with_state via a new StateMiddleware (placing them in extensions on both request and response)

There are a few existing #645 #715 and #643 related to improving ergonomics. As it currently stands the generic State is required on Request<State>, as well as Middleware<State>, Next<'_, State>, Endpoint<State>, Listener<State>, and Server<State> and other places. Since we already have the ability to use extensions for things like req.ext::<User>() I figured why not just merge the two and call it a day.

I recognize that moving the State object out of the generics and into the Extensions may impact performance so this is something to consider - but I think this improvement in ergonomics and overall simplicity in navigating the code is worth it. The code now treats the state like any other middleware via the StateMiddleware<S>.

When you call app.with_state(state) this simply wraps the <T: Clone + Send + Sync + 'static> in a StateMiddleware::new(state). The StateMiddleware will set the request extensions via set_ext(data.clone()) and also on the response after calling next.run.

use tide::{Request};

/// The shared application state.
#[derive(Clone)]
struct State {
    name: String,
}

// Define a new instance of the state.
let state = State {
    name: "Nori".to_string()
};

// Initialize the application with state.
let mut app = tide::with_state(state);
app.at("/").get(|req: Request| async move {
    Ok(format!("Hello, {}!", &req.state::<State>().name))
});
app.listen("127.0.0.1:8080").await?;

The new req.state::<State>() has an .expects on it when you call it which will automatically unwrap for you (or error out when you pass it a type that wasn't set in the middleware. The same goes for res.state::<State>() which I like because now you can get the application state in the response as well.

The original .with_state on the Server was there for the generic type, but since state is now in middleware that function has been changed to require passing in the mutable server itself. The above example is the same as writing it below:

use tide::{Request};

/// The shared application state.
#[derive(Clone)]
struct State {
    name: String,
}

// Define a new instance of the state.
let state = State {
    name: "Nori".to_string()
};

// Initialize the application with state.
let mut app = tide::new();
app.with_state(state);
// app.with_state(state2);
app.at("/").get(|req: Request| async move {
    Ok(format!("Hello, {}!", &req.state::<State>().name))
});
app.listen("127.0.0.1:8080").await?;

The nice thing about this is now you can add multiple different state types rather than force everything into a single struct. You can also directly add the StateMiddleware by calling app.with(StateMiddleware::new(s)) although in practice I think people will stick to app.with_state.

Overall I think this is a great gain in ergonomics!

Update on Next<'_>

For this I may separate out into a different pull request but let me know

I've also now refactored the use of Next<'_> to no longer need the extra lifetimes in favor of Arc for endpoints and middleware. Selection routing will return these as well rather than explicit borrows with lifetime requirements. This simplifies some of the code and opens up the possibility for passing along impl Middleware function closures.

// This is an example of a function middleware that uses the
// application state. Because it depends on a specific request state,
// it would likely be closely tied to a specific application
fn user_loader(mut request: Request, next: Next) -> Pin<Box<dyn Future<Output = Result> + Send>> {
    Box::pin(async {
        if let Some(user) = request.state().find_user().await {
            tide::log::trace!("user loaded", {user: user.name});
            request.set_ext(user);
            Ok(next.run(request).await)
        // this middleware only needs to run before the endpoint, so
        // it just passes through the result of Next
        } else {
            // do not run endpoints, we could not find a user
            Ok(Response::new(StatusCode::Unauthorized))
        }
    })
}

This is the new format as it no longer requires <State> or Next<'_, State> or Next<'_> or any user_loader<'a> as the requirement for<'a> is now gone. I will update this pull request to also support the use of a function closure. This will solve #854 as well through the use of impl Middleware

async fn auth_middleware(request: tide::Request, next: tide::Next) -> tide::Result {
    let authenticated = match request.header("X-Auth") {
        Some(header) => header == "secret_key",
        None => false,
    };

    if authenticated {
        Ok(next.run(request).await)
    } else {
        Ok(tide::Response::new(tide::StatusCode::Unauthorized))
    }
}

In the process I've simplified the use of the Next so that it can be used as an Endpoint (rather than have a separate MiddlewareEndpoint). This was useful for the Server to wrap the existing Next selection and add in its own middleware.

@nyxtom nyxtom changed the title Remove State from Middleware and Request, Use Extensions Remove State and for<'a> from Middleware and Request, Use Extensions Jul 16, 2022
@nyxtom nyxtom changed the title Remove State and for<'a> from Middleware and Request, Use Extensions Remove State and for<'a> from Middleware and Request, Use Extensions and Arc Jul 16, 2022
@nyxtom nyxtom changed the title Remove State and for<'a> from Middleware and Request, Use Extensions and Arc Remove State and for<'a> as requirements from Middleware and Request Jul 16, 2022
@nyxtom nyxtom changed the title Remove State and for<'a> as requirements from Middleware and Request Remove State and for<'a> as requirements from Middleware, Request, Next Jul 16, 2022
@nyxtom nyxtom changed the title Remove State and for<'a> as requirements from Middleware, Request, Next Remove State and for<'a> as requirements from Middleware and Request Jul 16, 2022
examples/nested.rs Outdated Show resolved Hide resolved
@nyxtom
Copy link
Contributor Author

nyxtom commented Aug 1, 2022

@Fishrock123 would you mind taking a look at this as well? Trying to get this one to move forward for 0.17

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