diff options
| author | Owen Jacobson <owen@grimoire.ca> | 2025-12-01 19:36:59 -0500 |
|---|---|---|
| committer | Owen Jacobson <owen@grimoire.ca> | 2025-12-02 01:12:03 -0500 |
| commit | 7ff4b80fa320c418a7bdca8007765dc0e6b0bda0 (patch) | |
| tree | 9341bfb3c64d25ccb49258e53a0001a67d2d19de | |
| parent | 9af71a82ca74fde48283d4e0a0adcd69f4fcb9dd (diff) | |
Cache static assets using an etag header.
When Pilcrow returns static assets, it now sets an `ETag` header, derived from the content of the asset. This header will change if the asset changes.
Conforming browsers _may_ cache the response, and make a conditional request with an `If-None-Match` header on future requests for the same asset. If we see that header, Pilcrow will check whether the loaded asset is the same as the one the browser already had, and skip the response with a 304 if appropriate. This cuts down on the number of times clients will load the same script files and the same assets from the server.
Endpoints that route to the index after doing some logic got a pretty major cleanup. The tangled logic between the actual handler and the error type made them challening to follow, and there wasn't a clean way to pass the `If-None-Match` header through into the error for use when determining the final response. This version instead combines the negative cases with _success_, which produces the desired responses with much more straightforwards code.
I've also opted to support `If-None-Match` for these endpoints, even though they do logically change if the underlying chat state changes - because the _response body_ does not change, and that's what the HTTP spec (and HTTP clients) care about in this context. They will, however, return the response in full for situations like Not Found.
(I know it looks like these endpoints now _require_ the `If-None-Match` header. Trust me on this: they do not. The `headers` create cooks up an empty `If-None-Match` header if none is supplied, and that empty header differs from any non-empty ETag.)
| -rw-r--r-- | Cargo.lock | 1 | ||||
| -rw-r--r-- | Cargo.toml | 1 | ||||
| -rw-r--r-- | src/ui/assets.rs | 91 | ||||
| -rw-r--r-- | src/ui/handlers/asset.rs | 11 | ||||
| -rw-r--r-- | src/ui/handlers/conversation.rs | 57 | ||||
| -rw-r--r-- | src/ui/handlers/index.rs | 18 | ||||
| -rw-r--r-- | src/ui/handlers/invite.rs | 44 | ||||
| -rw-r--r-- | src/ui/handlers/login.rs | 14 | ||||
| -rw-r--r-- | src/ui/handlers/me.rs | 18 | ||||
| -rw-r--r-- | src/ui/handlers/setup.rs | 18 | ||||
| -rw-r--r-- | src/ui/handlers/swatch.rs | 14 |
11 files changed, 176 insertions, 111 deletions
@@ -1968,6 +1968,7 @@ dependencies = [ "faker_rand", "futures", "headers", + "hex", "itertools", "mime", "nix", @@ -29,6 +29,7 @@ chrono = { version = "0.4.42", features = ["serde"] } clap = { version = "4.5.51", features = ["derive", "env"] } futures = "0.3.31" headers = "0.4.1" +hex = "0.4.3" itertools = "0.14.0" mime = "0.3.17" nix = { version = "0.30.1", features = ["fs"] } diff --git a/src/ui/assets.rs b/src/ui/assets.rs index ede1f7c..b8ca00b 100644 --- a/src/ui/assets.rs +++ b/src/ui/assets.rs @@ -1,8 +1,12 @@ +use std::str::FromStr; + use ::mime::Mime; use axum::{ - http::{StatusCode, header}, - response::{IntoResponse, Response}, + http::StatusCode, + response::{self, IntoResponse}, }; +use axum_extra::TypedHeader; +use headers::{ContentType, ETag, IfNoneMatch}; use rust_embed::EmbeddedFile; use super::{error::NotFound, mime}; @@ -20,9 +24,10 @@ impl Assets { let path = path.as_ref(); let mime = mime::from_path(path).fail("Failed to determine MIME type from asset path")?; - Self::get(path) - .map(|file| Asset(mime, file)) - .ok_or(Error::NotFound(path.into())) + let file = Self::get(path).ok_or(Error::NotFound(path.into()))?; + let asset = Asset::new(mime, file)?; + + Ok(asset) } pub fn index() -> Result<Asset, Internal> { @@ -33,20 +38,86 @@ impl Assets { } } -pub struct Asset(Mime, EmbeddedFile); +pub struct Asset { + mime: Mime, + file: EmbeddedFile, + etag: ETag, +} + +impl Asset { + fn new(mime: Mime, file: EmbeddedFile) -> Result<Self, Failed> { + let etag = Self::etag_from(&file).fail("Failed to compute ETag")?; + Ok(Self { mime, file, etag }) + } + + fn etag_from(file: &EmbeddedFile) -> Result<ETag, <ETag as FromStr>::Err> { + let digest = file.metadata.sha256_hash(); + let digest = hex::encode(digest); + + let tag = format!("\"{digest}\""); + let tag = ETag::from_str(&tag)?; + + Ok(tag) + } + + pub fn differs_from(&self, if_none_match: &IfNoneMatch) -> bool { + if_none_match.precondition_passes(&self.etag) + } +} impl IntoResponse for Asset { - fn into_response(self) -> Response { - let Self(mime, file) = self; + fn into_response(self) -> response::Response { + let Self { mime, file, etag } = self; ( StatusCode::OK, - [(header::CONTENT_TYPE, mime.as_ref())], + TypedHeader(ContentType::from(mime)), + TypedHeader(etag), file.data, ) .into_response() } } +// Clippy warns us here because the `Asset` variant is the size of, well, the Asset struct - itself +// quite large due to the embedded file metadata. There isn't a better alternative that I can see, +// and it's not causing any performance or stack exhaustion problems in practice. +#[expect(clippy::large_enum_variant)] +pub enum Response { + NotModified, + Asset(Asset), +} + +impl Response { + pub fn load(path: impl AsRef<str>, if_none_match: &IfNoneMatch) -> Result<Self, Error> { + let asset = Assets::load(path)?; + let response = Self::respond_with(asset, if_none_match); + Ok(response) + } + + pub fn index(if_none_match: &IfNoneMatch) -> Result<Self, Internal> { + let asset = Assets::index()?; + let response = Self::respond_with(asset, if_none_match); + Ok(response) + } + + fn respond_with(asset: Asset, if_none_match: &IfNoneMatch) -> Self { + if asset.differs_from(if_none_match) { + Self::Asset(asset) + } else { + Self::NotModified + } + } +} + +impl IntoResponse for Response { + fn into_response(self) -> response::Response { + match self { + Self::NotModified => StatusCode::NOT_MODIFIED.into_response(), + Self::Asset(asset) => asset.into_response(), + } + } +} + #[derive(Debug, thiserror::Error)] pub enum Error { #[error("not found: {0}")] @@ -56,7 +127,7 @@ pub enum Error { } impl IntoResponse for Error { - fn into_response(self) -> Response { + fn into_response(self) -> response::Response { match self { Self::NotFound(_) => NotFound(self.to_string()).into_response(), Self::Failed(_) => Internal::from(self).into_response(), diff --git a/src/ui/handlers/asset.rs b/src/ui/handlers/asset.rs index 1d5b8be..948d6d6 100644 --- a/src/ui/handlers/asset.rs +++ b/src/ui/handlers/asset.rs @@ -1,7 +1,12 @@ use axum::extract::Path; +use axum_extra::TypedHeader; +use headers::IfNoneMatch; -use crate::ui::assets::{Asset, Assets, Error}; +use crate::ui::assets::{Error, Response}; -pub async fn handler(Path(path): Path<String>) -> Result<Asset, Error> { - Assets::load(path) +pub async fn handler( + Path(path): Path<String>, + TypedHeader(if_none_match): TypedHeader<IfNoneMatch>, +) -> Result<Response, Error> { + Response::load(&path, &if_none_match) } diff --git a/src/ui/handlers/conversation.rs b/src/ui/handlers/conversation.rs index 102efc6..9a87d40 100644 --- a/src/ui/handlers/conversation.rs +++ b/src/ui/handlers/conversation.rs @@ -2,13 +2,15 @@ use axum::{ extract::{Path, State}, response::{self, IntoResponse, Redirect}, }; +use axum_extra::TypedHeader; +use headers::IfNoneMatch; use crate::{ conversation::{self, app, app::Conversations}, error::Internal, token::extract::Identity, ui::{ - assets::{Asset, Assets}, + assets::{self, Asset, Assets}, error::NotFound, }, }; @@ -17,44 +19,39 @@ pub async fn handler( State(conversations): State<Conversations>, identity: Option<Identity>, Path(conversation): Path<conversation::Id>, -) -> Result<Asset, Error> { - let _ = identity.ok_or(Error::NotLoggedIn)?; - conversations - .get(&conversation) - .await - .map_err(Error::from)?; + TypedHeader(if_none_match): TypedHeader<IfNoneMatch>, +) -> Result<Response, Internal> { + let response = if identity.is_none() { + Response::NotLoggedIn + } else { + match conversations.get(&conversation).await { + Ok(_) => { + let index = assets::Response::index(&if_none_match)?; + Response::Asset(index) + } + Err(app::GetError::NotFound(_) | app::GetError::Deleted(_)) => { + let index = Assets::index()?; + Response::NotFound(index) + } + Err(err @ app::GetError::Failed(_)) => return Err(err.into()), + } + }; - Assets::index().map_err(Error::Internal) + Ok(response) } -#[derive(Debug, thiserror::Error)] -pub enum Error { - #[error("conversation not found")] - NotFound, - #[error("not logged in")] +pub enum Response { NotLoggedIn, - #[error("{0}")] - Internal(Internal), -} - -impl From<app::GetError> for Error { - fn from(error: app::GetError) -> Self { - match error { - app::GetError::NotFound(_) | app::GetError::Deleted(_) => Self::NotFound, - app::GetError::Failed(_) => Self::Internal(error.into()), - } - } + NotFound(Asset), + Asset(assets::Response), } -impl IntoResponse for Error { +impl IntoResponse for Response { fn into_response(self) -> response::Response { match self { - Self::NotFound => match Assets::index() { - Ok(asset) => NotFound(asset).into_response(), - Err(internal) => internal.into_response(), - }, Self::NotLoggedIn => Redirect::temporary("/login").into_response(), - Self::Internal(error) => error.into_response(), + Self::NotFound(asset) => NotFound(asset).into_response(), + Self::Asset(asset) => asset.into_response(), } } } diff --git a/src/ui/handlers/index.rs b/src/ui/handlers/index.rs index 2fcb51c..de0b2b0 100644 --- a/src/ui/handlers/index.rs +++ b/src/ui/handlers/index.rs @@ -1,22 +1,20 @@ use axum::response::{self, IntoResponse, Redirect}; +use axum_extra::TypedHeader; +use headers::IfNoneMatch; -use crate::{ - error::Internal, - token::extract::Identity, - ui::assets::{Asset, Assets}, -}; +use crate::{error::Internal, token::extract::Identity, ui::assets::Response}; -pub async fn handler(identity: Option<Identity>) -> Result<Asset, Error> { +pub async fn handler( + identity: Option<Identity>, + TypedHeader(if_none_match): TypedHeader<IfNoneMatch>, +) -> Result<Response, Error> { let _ = identity.ok_or(Error::NotLoggedIn)?; - Assets::index().map_err(Error::Internal) + Response::index(&if_none_match).map_err(Error::Internal) } -#[derive(Debug, thiserror::Error)] pub enum Error { - #[error("not logged in")] NotLoggedIn, - #[error("{0}")] Internal(Internal), } diff --git a/src/ui/handlers/invite.rs b/src/ui/handlers/invite.rs index edd6dc1..e552318 100644 --- a/src/ui/handlers/invite.rs +++ b/src/ui/handlers/invite.rs @@ -2,12 +2,15 @@ use axum::{ extract::{Path, State}, response::{self, IntoResponse}, }; +use axum_extra::TypedHeader; +use headers::IfNoneMatch; use crate::{ error::Internal, invite, invite::app::Invites, ui::{ + assets, assets::{Asset, Assets}, error::NotFound, }, @@ -16,38 +19,27 @@ use crate::{ pub async fn handler( State(invites): State<Invites>, Path(invite): Path<invite::Id>, -) -> Result<Asset, Error> { - invites - .get(&invite) - .await - .map_err(Error::internal)? - .ok_or(Error::NotFound)?; - - Assets::index().map_err(Error::Internal) -} - -#[derive(Debug, thiserror::Error)] -pub enum Error { - #[error("invite not found")] - NotFound, - #[error("{0}")] - Internal(Internal), + TypedHeader(if_none_match): TypedHeader<IfNoneMatch>, +) -> Result<Response, Internal> { + if invites.get(&invite).await?.is_some() { + let index = assets::Response::index(&if_none_match)?; + Ok(Response::Found(index)) + } else { + let index = Assets::index()?; + Ok(Response::NotFound(index)) + } } -impl Error { - fn internal(err: impl Into<Internal>) -> Self { - Self::Internal(err.into()) - } +pub enum Response { + Found(assets::Response), + NotFound(Asset), } -impl IntoResponse for Error { +impl IntoResponse for Response { fn into_response(self) -> response::Response { match self { - Self::NotFound => match Assets::index() { - Ok(asset) => NotFound(asset).into_response(), - Err(internal) => internal.into_response(), - }, - Self::Internal(error) => error.into_response(), + Self::Found(asset) => asset.into_response(), + Self::NotFound(asset) => NotFound(asset).into_response(), } } } diff --git a/src/ui/handlers/login.rs b/src/ui/handlers/login.rs index 4562b04..c904d2a 100644 --- a/src/ui/handlers/login.rs +++ b/src/ui/handlers/login.rs @@ -1,8 +1,10 @@ -use crate::{ - error::Internal, - ui::assets::{Asset, Assets}, -}; +use axum_extra::TypedHeader; +use headers::IfNoneMatch; -pub async fn handler() -> Result<Asset, Internal> { - Assets::index() +use crate::{error::Internal, ui::assets::Response}; + +pub async fn handler( + TypedHeader(if_none_match): TypedHeader<IfNoneMatch>, +) -> Result<Response, Internal> { + Response::index(&if_none_match) } diff --git a/src/ui/handlers/me.rs b/src/ui/handlers/me.rs index 2fcb51c..de0b2b0 100644 --- a/src/ui/handlers/me.rs +++ b/src/ui/handlers/me.rs @@ -1,22 +1,20 @@ use axum::response::{self, IntoResponse, Redirect}; +use axum_extra::TypedHeader; +use headers::IfNoneMatch; -use crate::{ - error::Internal, - token::extract::Identity, - ui::assets::{Asset, Assets}, -}; +use crate::{error::Internal, token::extract::Identity, ui::assets::Response}; -pub async fn handler(identity: Option<Identity>) -> Result<Asset, Error> { +pub async fn handler( + identity: Option<Identity>, + TypedHeader(if_none_match): TypedHeader<IfNoneMatch>, +) -> Result<Response, Error> { let _ = identity.ok_or(Error::NotLoggedIn)?; - Assets::index().map_err(Error::Internal) + Response::index(&if_none_match).map_err(Error::Internal) } -#[derive(Debug, thiserror::Error)] pub enum Error { - #[error("not logged in")] NotLoggedIn, - #[error("{0}")] Internal(Internal), } diff --git a/src/ui/handlers/setup.rs b/src/ui/handlers/setup.rs index 5707765..ac91908 100644 --- a/src/ui/handlers/setup.rs +++ b/src/ui/handlers/setup.rs @@ -2,14 +2,15 @@ use axum::{ extract::State, response::{self, IntoResponse, Redirect}, }; +use axum_extra::TypedHeader; +use headers::IfNoneMatch; -use crate::{ - error::Internal, - setup::app::Setup, - ui::assets::{Asset, Assets}, -}; +use crate::{error::Internal, setup::app::Setup, ui::assets::Response}; -pub async fn handler(State(setup): State<Setup>) -> Result<Asset, Error> { +pub async fn handler( + State(setup): State<Setup>, + TypedHeader(if_none_match): TypedHeader<IfNoneMatch>, +) -> Result<Response, Error> { if setup .completed() .await @@ -18,15 +19,12 @@ pub async fn handler(State(setup): State<Setup>) -> Result<Asset, Error> { { Err(Error::SetupCompleted) } else { - Assets::index().map_err(Error::Internal) + Response::index(&if_none_match).map_err(Error::Internal) } } -#[derive(Debug, thiserror::Error)] pub enum Error { - #[error("setup already completed")] SetupCompleted, - #[error("{0}")] Internal(Internal), } diff --git a/src/ui/handlers/swatch.rs b/src/ui/handlers/swatch.rs index 4562b04..c904d2a 100644 --- a/src/ui/handlers/swatch.rs +++ b/src/ui/handlers/swatch.rs @@ -1,8 +1,10 @@ -use crate::{ - error::Internal, - ui::assets::{Asset, Assets}, -}; +use axum_extra::TypedHeader; +use headers::IfNoneMatch; -pub async fn handler() -> Result<Asset, Internal> { - Assets::index() +use crate::{error::Internal, ui::assets::Response}; + +pub async fn handler( + TypedHeader(if_none_match): TypedHeader<IfNoneMatch>, +) -> Result<Response, Internal> { + Response::index(&if_none_match) } |
