From 424fb08ecd315c67dd3862c29e87eea7bf32f65c Mon Sep 17 00:00:00 2001 From: Owen Jacobson Date: Tue, 17 Jun 2025 00:46:24 -0400 Subject: Unify `setup_required` middlewares. The two middlewares were identical but for the specific `IntoResponse` impl used to generate the response when setup has not been completed. However, unifying them while still using `from_fn_with_state` lead to this horrorshow: .route_layer(middleware::from_fn_with_state( app.clone(), |state, req, next| { setup::middeware::setup_required(UNAVAILABLE, state, req, next) } )) It's a lot to read, and it surfaces the entire signature of a state-driven middleware `fn` into the call site solely to close over one argument (`UNAVAILABLE`). Rather than doing that, I've converted this middleware into a full blown Tower middleware, following . I considered taking this further and implementing a custom future to remove the allocation for `Box::pin`, but honestly, that allocation isn't hurting anyone and this code already got long enough in the translation. The new API looks like: .route_layer(setup::Required::or_unavailable(app.clone())) Or like: .route_layer(setup::Required::with_fallback(app.clone(), RESPONSE)) One thing I would have liked to have avoided is the additional `app.clone()` argument, but there isn't a way to extract the _state_ from a request inside of an Axum middleware. It has to be passed in externally - that's what `from_fn_with_state` is doing under the hood, as well. Using `State` as an extractor doesn't work; the `State` extractor is special in a _bunch_ of ways, and this is one of them. Other extractors would work. Realistically, I'd probably want to explore interfaces like .route_layer(setup::Required(app).or_unavailable()) or .route_layer(app.setup().required().or_unavailable()) --- src/cli.rs | 9 ++--- src/setup/middleware.rs | 20 ----------- src/setup/mod.rs | 4 +-- src/setup/required.rs | 88 +++++++++++++++++++++++++++++++++++++++++++++++++ src/ui/middleware.rs | 15 --------- src/ui/mod.rs | 1 - src/ui/routes/mod.rs | 9 +++-- 7 files changed, 98 insertions(+), 48 deletions(-) delete mode 100644 src/setup/middleware.rs create mode 100644 src/setup/required.rs delete mode 100644 src/ui/middleware.rs (limited to 'src') diff --git a/src/cli.rs b/src/cli.rs index 4232c00..042141d 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -15,12 +15,7 @@ use clap::{CommandFactory, Parser}; use sqlx::sqlite::SqlitePool; use tokio::net; -use crate::{ - app::App, - boot, channel, clock, db, event, expire, invite, message, - setup::{self, middleware::setup_required}, - ui, user, -}; +use crate::{app::App, boot, channel, clock, db, event, expire, invite, message, setup, ui, user}; /// Command-line entry point for running the `pilcrow` server. /// @@ -152,7 +147,7 @@ fn routers(app: &App) -> Router { app.clone(), expire::middleware, )) - .route_layer(middleware::from_fn_with_state(app.clone(), setup_required)), + .route_layer(setup::Required::or_unavailable(app.clone())), // API endpoints that handle setup setup::router(), // The UI (handles setup state itself) diff --git a/src/setup/middleware.rs b/src/setup/middleware.rs deleted file mode 100644 index 5f9996b..0000000 --- a/src/setup/middleware.rs +++ /dev/null @@ -1,20 +0,0 @@ -use axum::{ - extract::{Request, State}, - http::StatusCode, - middleware::Next, - response::{IntoResponse, Response}, -}; - -use crate::{app::App, error::Internal}; - -pub async fn setup_required(State(app): State, request: Request, next: Next) -> Response { - match app.setup().completed().await { - Ok(true) => next.run(request).await, - Ok(false) => ( - StatusCode::SERVICE_UNAVAILABLE, - "initial setup not completed", - ) - .into_response(), - Err(error) => Internal::from(error).into_response(), - } -} diff --git a/src/setup/mod.rs b/src/setup/mod.rs index 5a8fa37..62972b3 100644 --- a/src/setup/mod.rs +++ b/src/setup/mod.rs @@ -1,6 +1,6 @@ pub mod app; -pub mod middleware; pub mod repo; +mod required; mod routes; -pub use self::routes::router; +pub use self::{required::Layer as Required, routes::router}; diff --git a/src/setup/required.rs b/src/setup/required.rs new file mode 100644 index 0000000..5b7fe5b --- /dev/null +++ b/src/setup/required.rs @@ -0,0 +1,88 @@ +use axum::{ + extract::Request, + http::StatusCode, + response::{IntoResponse, Response}, +}; +use std::pin::Pin; +use std::task::{Context, Poll}; +use tower::Service; + +use crate::{app::App, error::Internal}; + +const UNAVAILABLE: (StatusCode, &str) = ( + StatusCode::SERVICE_UNAVAILABLE, + "initial setup not completed", +); + +#[derive(Clone)] +pub struct Layer { + app: App, + fallback: F, +} + +impl Layer<(StatusCode, &'static str)> { + pub fn or_unavailable(app: App) -> Self { + Self::with_fallback(app, UNAVAILABLE) + } +} + +impl Layer { + pub fn with_fallback(app: App, fallback: F) -> Self { + Layer { app, fallback } + } +} + +impl tower::Layer for Layer +where + Self: Clone, +{ + type Service = Middleware; + + fn layer(&self, inner: S) -> Self::Service { + let Self { app, fallback } = self.clone(); + Middleware { + inner, + app, + fallback, + } + } +} + +#[derive(Clone)] +pub struct Middleware { + inner: S, + app: App, + fallback: F, +} + +impl Service for Middleware +where + Self: Clone, + S: Service + Send + 'static, + S::Future: Send, + F: IntoResponse + Clone + Send + 'static, +{ + type Response = S::Response; + type Error = S::Error; + type Future = Pin> + Send>>; + + fn poll_ready(&mut self, ctx: &mut Context<'_>) -> Poll> { + self.inner.poll_ready(ctx) + } + + fn call(&mut self, req: Request) -> Self::Future { + let Self { + mut inner, + app, + fallback, + } = self.clone(); + + Box::pin(async move { + match app.setup().completed().await { + Ok(true) => inner.call(req).await, + Ok(false) => Ok(fallback.into_response()), + Err(error) => Ok(Internal::from(error).into_response()), + } + }) + } +} diff --git a/src/ui/middleware.rs b/src/ui/middleware.rs deleted file mode 100644 index f60ee1c..0000000 --- a/src/ui/middleware.rs +++ /dev/null @@ -1,15 +0,0 @@ -use axum::{ - extract::{Request, State}, - middleware::Next, - response::{IntoResponse, Redirect, Response}, -}; - -use crate::{app::App, error::Internal}; - -pub async fn setup_required(State(app): State, request: Request, next: Next) -> Response { - match app.setup().completed().await { - Ok(true) => next.run(request).await, - Ok(false) => Redirect::to("/setup").into_response(), - Err(error) => Internal::from(error).into_response(), - } -} diff --git a/src/ui/mod.rs b/src/ui/mod.rs index f8caa48..e834bba 100644 --- a/src/ui/mod.rs +++ b/src/ui/mod.rs @@ -1,6 +1,5 @@ mod assets; mod error; -mod middleware; mod mime; mod routes; diff --git a/src/ui/routes/mod.rs b/src/ui/routes/mod.rs index 80dc1e5..328eb73 100644 --- a/src/ui/routes/mod.rs +++ b/src/ui/routes/mod.rs @@ -1,6 +1,6 @@ -use axum::{Router, middleware, routing::get}; +use axum::{Router, response::Redirect, routing::get}; -use crate::{app::App, ui::middleware::setup_required}; +use crate::app::App; mod ch; mod get; @@ -21,7 +21,10 @@ pub fn router(app: &App) -> Router { .route("/login", get(login::get::handler)) .route("/ch/{channel}", get(ch::channel::get::handler)) .route("/invite/{invite}", get(invite::invite::get::handler)) - .route_layer(middleware::from_fn_with_state(app.clone(), setup_required)), + .route_layer(crate::setup::Required::with_fallback( + app.clone(), + Redirect::to("/setup"), + )), ] .into_iter() .fold(Router::default(), Router::merge) -- cgit v1.2.3 From 43375bcb875a31ce8c6132ce78552d45f64b261b Mon Sep 17 00:00:00 2001 From: Owen Jacobson Date: Tue, 17 Jun 2025 01:22:54 -0400 Subject: Use a fluent style for the middleware layers. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For endpoints that are unavailable, that default behaviour no longer needs to be specified: `Required(app)` will do that for you. For endpoints that are redirects until setup is completed, `Require(app).with_fallback(…response…)` will do that. To make this a bit harder to break by accident, the default unavailable response is now its own type. --- src/cli.rs | 2 +- src/setup/mod.rs | 2 +- src/setup/required.rs | 53 ++++++++++++++++++++++++++++++++++----------------- src/ui/routes/mod.rs | 5 +---- 4 files changed, 39 insertions(+), 23 deletions(-) (limited to 'src') diff --git a/src/cli.rs b/src/cli.rs index 042141d..7bfdbc0 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -147,7 +147,7 @@ fn routers(app: &App) -> Router { app.clone(), expire::middleware, )) - .route_layer(setup::Required::or_unavailable(app.clone())), + .route_layer(setup::Required(app.clone())), // API endpoints that handle setup setup::router(), // The UI (handles setup state itself) diff --git a/src/setup/mod.rs b/src/setup/mod.rs index 62972b3..a4b821c 100644 --- a/src/setup/mod.rs +++ b/src/setup/mod.rs @@ -3,4 +3,4 @@ pub mod repo; mod required; mod routes; -pub use self::{required::Layer as Required, routes::router}; +pub use self::{required::Required, routes::router}; diff --git a/src/setup/required.rs b/src/setup/required.rs index 5b7fe5b..2112e4b 100644 --- a/src/setup/required.rs +++ b/src/setup/required.rs @@ -5,34 +5,40 @@ use axum::{ }; use std::pin::Pin; use std::task::{Context, Poll}; -use tower::Service; +use tower::{Layer, Service}; use crate::{app::App, error::Internal}; -const UNAVAILABLE: (StatusCode, &str) = ( - StatusCode::SERVICE_UNAVAILABLE, - "initial setup not completed", -); - #[derive(Clone)] -pub struct Layer { - app: App, - fallback: F, -} +pub struct Required(pub App); -impl Layer<(StatusCode, &'static str)> { - pub fn or_unavailable(app: App) -> Self { - Self::with_fallback(app, UNAVAILABLE) +impl Required { + pub fn with_fallback(self, fallback: F) -> WithFallback { + let Self(app) = self; + WithFallback { app, fallback } } } -impl Layer { - pub fn with_fallback(app: App, fallback: F) -> Self { - Layer { app, fallback } +impl Layer for Required { + type Service = Middleware; + + fn layer(&self, inner: S) -> Self::Service { + let Self(app) = self.clone(); + Middleware { + inner, + app, + fallback: Unavailable, + } } } -impl tower::Layer for Layer +#[derive(Clone)] +pub struct WithFallback { + app: App, + fallback: F, +} + +impl Layer for WithFallback where Self: Clone, { @@ -86,3 +92,16 @@ where }) } } + +#[derive(Clone)] +pub struct Unavailable; + +impl IntoResponse for Unavailable { + fn into_response(self) -> Response { + ( + StatusCode::SERVICE_UNAVAILABLE, + "initial setup not completed", + ) + .into_response() + } +} diff --git a/src/ui/routes/mod.rs b/src/ui/routes/mod.rs index 328eb73..dc94773 100644 --- a/src/ui/routes/mod.rs +++ b/src/ui/routes/mod.rs @@ -21,10 +21,7 @@ pub fn router(app: &App) -> Router { .route("/login", get(login::get::handler)) .route("/ch/{channel}", get(ch::channel::get::handler)) .route("/invite/{invite}", get(invite::invite::get::handler)) - .route_layer(crate::setup::Required::with_fallback( - app.clone(), - Redirect::to("/setup"), - )), + .route_layer(crate::setup::Required(app.clone()).with_fallback(Redirect::to("/setup"))), ] .into_iter() .fold(Router::default(), Router::merge) -- cgit v1.2.3