diff --git a/clippy.toml b/clippy.toml index 00aa4ed8e..c5f0ebf94 100644 --- a/clippy.toml +++ b/clippy.toml @@ -14,3 +14,8 @@ reason = """ [[disallowed-types]] path = "semver::Version" reason = "use our own custom db::types::version::Version so you can use it with sqlx" + +[[disallowed-types]] +path = "axum_extra::headers::IfNoneMatch" +reason = "use our own custom web::headers::IfNoneMatch for sane behaviour with missing headers" + diff --git a/src/storage/database.rs b/src/storage/database.rs index 4c29d1715..c7d8a7b49 100644 --- a/src/storage/database.rs +++ b/src/storage/database.rs @@ -1,5 +1,5 @@ use super::{BlobUpload, FileRange, StorageMetrics, StreamingBlob}; -use crate::{InstanceMetrics, db::Pool, error::Result}; +use crate::{InstanceMetrics, db::Pool, error::Result, web::headers::compute_etag}; use chrono::{DateTime, Utc}; use futures_util::stream::{Stream, TryStreamExt}; use sqlx::Acquire; @@ -123,6 +123,8 @@ impl DatabaseBackend { }); let content = result.content.unwrap_or_default(); let content_len = content.len(); + + let etag = compute_etag(&content); Ok(StreamingBlob { path: result.path, mime: result @@ -130,6 +132,7 @@ impl DatabaseBackend { .parse() .unwrap_or(mime::APPLICATION_OCTET_STREAM), date_updated: result.date_updated, + etag: Some(etag), content: Box::new(io::Cursor::new(content)), content_length: content_len, compression, diff --git a/src/storage/mod.rs b/src/storage/mod.rs index 7e558d24d..0019b094d 100644 --- a/src/storage/mod.rs +++ b/src/storage/mod.rs @@ -22,6 +22,7 @@ use crate::{ utils::spawn_blocking, }; use anyhow::anyhow; +use axum_extra::headers; use chrono::{DateTime, Utc}; use dashmap::DashMap; use fn_error_context::context; @@ -57,7 +58,7 @@ type FileRange = RangeInclusive; pub(crate) struct PathNotFoundError; /// represents a blob to be uploaded to storage. -#[derive(Clone, Debug, PartialEq, Eq, Hash)] +#[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct BlobUpload { pub(crate) path: String, pub(crate) mime: Mime, @@ -76,11 +77,12 @@ impl From for BlobUpload { } } -#[derive(Clone, Debug, PartialEq, Eq, Hash)] +#[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct Blob { pub(crate) path: String, pub(crate) mime: Mime, pub(crate) date_updated: DateTime, + pub(crate) etag: Option, pub(crate) content: Vec, pub(crate) compression: Option, } @@ -89,6 +91,7 @@ pub(crate) struct StreamingBlob { pub(crate) path: String, pub(crate) mime: Mime, pub(crate) date_updated: DateTime, + pub(crate) etag: Option, pub(crate) compression: Option, pub(crate) content_length: usize, pub(crate) content: Box, @@ -100,6 +103,7 @@ impl std::fmt::Debug for StreamingBlob { .field("path", &self.path) .field("mime", &self.mime) .field("date_updated", &self.date_updated) + .field("etag", &self.etag) .field("compression", &self.compression) .finish() } @@ -134,6 +138,7 @@ impl StreamingBlob { ); self.compression = None; + // not touching the etag, it should represent the original content Ok(self) } @@ -148,12 +153,27 @@ impl StreamingBlob { path: self.path, mime: self.mime, date_updated: self.date_updated, + etag: self.etag, // downloading doesn't change the etag content: content.into_inner(), compression: self.compression, }) } } +impl From for StreamingBlob { + fn from(value: Blob) -> Self { + Self { + path: value.path, + mime: value.mime, + date_updated: value.date_updated, + etag: value.etag, + compression: value.compression, + content_length: value.content.len(), + content: Box::new(io::Cursor::new(value.content)), + } + } +} + pub fn get_file_list>(path: P) -> Box>> { let path = path.as_ref().to_path_buf(); if path.is_file() { @@ -583,6 +603,7 @@ impl AsyncStorage { path: format!("{archive_path}/{path}"), mime: detect_mime(path), date_updated: stream.date_updated, + etag: stream.etag, content: stream.content, content_length: stream.content_length, compression: None, @@ -756,7 +777,6 @@ impl AsyncStorage { mime, content, compression: Some(alg), - // this field is ignored by the backend }); } Ok((blobs, file_paths)) @@ -1135,7 +1155,7 @@ pub(crate) fn source_archive_path(name: &str, version: &Version) -> String { #[cfg(test)] mod test { use super::*; - use crate::test::TestEnvironment; + use crate::{test::TestEnvironment, web::headers::compute_etag}; use std::env; use test_case::test_case; @@ -1151,6 +1171,7 @@ mod test { mime: mime::APPLICATION_OCTET_STREAM, date_updated: Utc::now(), compression: alg, + etag: Some(compute_etag(&content)), content_length: content.len(), content: Box::new(io::Cursor::new(content)), } @@ -1292,6 +1313,7 @@ mod test { path: "some_path.db".into(), mime: mime::APPLICATION_OCTET_STREAM, date_updated: Utc::now(), + etag: None, compression: Some(alg), content_length: compressed_index_content.len(), content: Box::new(io::Cursor::new(compressed_index_content)), @@ -1494,9 +1516,8 @@ mod test { /// This is the preferred way to test whether backends work. #[cfg(test)] mod backend_tests { - use crate::test::TestEnvironment; - use super::*; + use crate::{test::TestEnvironment, web::headers::compute_etag}; fn get_file_info(files: &[FileEntry], path: impl AsRef) -> Option<&FileEntry> { let path = path.as_ref(); @@ -1560,6 +1581,9 @@ mod backend_tests { let found = storage.get(path, usize::MAX)?; assert_eq!(blob.mime, found.mime); assert_eq!(blob.content, found.content); + // while our db backend just does MD5, + // it seems like minio does it too :) + assert_eq!(found.etag, Some(compute_etag(&blob.content))); // default visibility is private assert!(!storage.get_public_access(path)?); @@ -1593,20 +1617,26 @@ mod backend_tests { content: b"test content\n".to_vec(), }; + let full_etag = compute_etag(&blob.content); + storage.store_blobs(vec![blob.clone()])?; - assert_eq!( - blob.content[0..=4], - storage - .get_range("foo/bar.txt", usize::MAX, 0..=4, None)? - .content - ); - assert_eq!( - blob.content[5..=12], - storage - .get_range("foo/bar.txt", usize::MAX, 5..=12, None)? - .content - ); + let mut etags = Vec::new(); + + for range in [0..=4, 5..=12] { + let partial_blob = storage.get_range("foo/bar.txt", usize::MAX, range.clone(), None)?; + let range = (*range.start() as usize)..=(*range.end() as usize); + assert_eq!(blob.content[range], partial_blob.content); + + etags.push(partial_blob.etag.unwrap()); + } + if let [etag1, etag2] = &etags[..] { + assert_ne!(etag1, etag2); + assert_ne!(etag1, &full_etag); + assert_ne!(etag2, &full_etag); + } else { + panic!("expected two etags"); + } for path in &["bar.txt", "baz.txt", "foo/baz.txt"] { assert!( diff --git a/src/storage/s3.rs b/src/storage/s3.rs index c67a8ee01..3e417ba38 100644 --- a/src/storage/s3.rs +++ b/src/storage/s3.rs @@ -1,5 +1,5 @@ use super::{BlobUpload, FileRange, StorageMetrics, StreamingBlob}; -use crate::{Config, InstanceMetrics}; +use crate::{Config, InstanceMetrics, web::headers::compute_etag}; use anyhow::{Context as _, Error}; use async_stream::try_stream; use aws_config::BehaviorVersion; @@ -10,6 +10,7 @@ use aws_sdk_s3::{ types::{Delete, ObjectIdentifier, Tag, Tagging}, }; use aws_smithy_types_convert::date_time::DateTimeExt; +use axum_extra::headers; use chrono::Utc; use futures_util::{ future::TryFutureExt, @@ -17,7 +18,7 @@ use futures_util::{ stream::{FuturesUnordered, Stream, StreamExt}, }; use std::sync::Arc; -use tracing::{error, warn}; +use tracing::{error, instrument, warn}; const PUBLIC_ACCESS_TAG: &str = "static-cloudfront-access"; const PUBLIC_ACCESS_VALUE: &str = "allow"; @@ -180,6 +181,7 @@ impl S3Backend { .map(|_| ()) } + #[instrument(skip(self))] pub(super) async fn get_stream( &self, path: &str, @@ -190,7 +192,11 @@ impl S3Backend { .get_object() .bucket(&self.bucket) .key(path) - .set_range(range.map(|r| format!("bytes={}-{}", r.start(), r.end()))) + .set_range( + range + .as_ref() + .map(|r| format!("bytes={}-{}", r.start(), r.end())), + ) .send() .await .convert_errors()?; @@ -204,6 +210,41 @@ impl S3Backend { let compression = res.content_encoding.as_ref().and_then(|s| s.parse().ok()); + let etag = if let Some(s3_etag) = res.e_tag + && !s3_etag.is_empty() + { + if let Some(range) = range { + // we can generate a unique etag for a range of the remote object too, + // by just concatenating the original etag with the range start and end. + // + // About edge cases: + // When the etag of the full archive changes after a rebuild, + // all derived etags for files inside the archive will also change. + // + // This could lead to _changed_ ETags, where the single file inside the archive + // is actually the same. + // + // AWS implementation (an minio) is to just use an MD5 hash of the file as + // ETag + Some(compute_etag(format!( + "{}-{}-{}", + s3_etag, + range.start(), + range.end() + ))) + } else { + match s3_etag.parse::() { + Ok(etag) => Some(etag), + Err(err) => { + error!(?err, s3_etag, "Failed to parse ETag from S3"); + None + } + } + } + } else { + None + }; + Ok(StreamingBlob { path: path.into(), mime: res @@ -213,6 +254,7 @@ impl S3Backend { .parse() .unwrap_or(mime::APPLICATION_OCTET_STREAM), date_updated, + etag, content_length: res .content_length .and_then(|length| length.try_into().ok()) diff --git a/src/test/mod.rs b/src/test/mod.rs index 44419a4bc..0740b50ef 100644 --- a/src/test/mod.rs +++ b/src/test/mod.rs @@ -15,16 +15,17 @@ use crate::{ web::{ build_axum_app, cache::{self, TargetCdn}, - headers::SURROGATE_CONTROL, + headers::{IfNoneMatch, SURROGATE_CONTROL}, page::TemplateData, }, }; use anyhow::Context as _; use axum::body::Bytes; use axum::{Router, body::Body, http::Request, response::Response as AxumResponse}; +use axum_extra::headers::{ETag, HeaderMapExt as _}; use fn_error_context::context; use futures_util::stream::TryStreamExt; -use http::header::CACHE_CONTROL; +use http::{HeaderMap, StatusCode, header::CACHE_CONTROL}; use http_body_util::BodyExt; use opentelemetry_sdk::metrics::{InMemoryMetricExporter, PeriodicReader}; use serde::de::DeserializeOwned; @@ -121,6 +122,9 @@ impl AxumResponseTestExt for axum::response::Response { } pub(crate) trait AxumRouterTestExt { + async fn get_with_headers(&self, path: &str, f: F) -> Result + where + F: FnOnce(&mut HeaderMap); async fn get_and_follow_redirects(&self, path: &str) -> Result; async fn assert_redirect_cached_unchecked( &self, @@ -130,6 +134,11 @@ pub(crate) trait AxumRouterTestExt { config: &Config, ) -> Result; async fn assert_not_found(&self, path: &str) -> Result<()>; + async fn assert_success_and_conditional_get( + &self, + path: &str, + expected_body: &str, + ) -> Result<()>; async fn assert_success_cached( &self, path: &str, @@ -175,6 +184,37 @@ impl AxumRouterTestExt for axum::Router { Ok(response) } + async fn assert_success_and_conditional_get( + &self, + path: &str, + expected_body: &str, + ) -> Result<()> { + let etag: ETag = { + // uncached response + let response = self.assert_success(path).await?; + let etag: ETag = response.headers().typed_get().unwrap(); + + assert_eq!(response.text().await?, expected_body); + + etag + }; + + let if_none_match = IfNoneMatch::from(etag.clone()); + + { + // cached response + let response = self + .get_with_headers(path, |headers| { + headers.typed_insert(if_none_match); + }) + .await?; + assert_eq!(response.status(), StatusCode::NOT_MODIFIED); + // etag is repeated + assert_eq!(response.headers().typed_get::().unwrap(), etag); + } + Ok(()) + } + async fn assert_not_found(&self, path: &str) -> Result<()> { let response = self.get(path).await?; @@ -209,6 +249,19 @@ impl AxumRouterTestExt for axum::Router { .await?) } + async fn get_with_headers(&self, path: &str, f: F) -> Result + where + F: FnOnce(&mut HeaderMap), + { + let mut builder = Request::builder().uri(path); + f(builder.headers_mut().unwrap()); + + Ok(self + .clone() + .oneshot(builder.body(Body::empty()).unwrap()) + .await?) + } + async fn get_and_follow_redirects(&self, path: &str) -> Result { let mut path = path.to_owned(); for _ in 0..=10 { diff --git a/src/web/file.rs b/src/web/file.rs index a99997c99..915be15f6 100644 --- a/src/web/file.rs +++ b/src/web/file.rs @@ -1,6 +1,6 @@ //! Database based file handler -use super::cache::CachePolicy; +use super::{cache::CachePolicy, headers::IfNoneMatch}; use crate::{ Config, error::Result, @@ -9,12 +9,14 @@ use crate::{ use axum::{ body::Body, extract::Extension, - http::{ - StatusCode, - header::{CONTENT_TYPE, LAST_MODIFIED}, - }, + http::StatusCode, response::{IntoResponse, Response as AxumResponse}, }; +use axum_extra::{ + TypedHeader, + headers::{ContentType, LastModified}, +}; +use std::time::SystemTime; use tokio_util::io::ReaderStream; #[derive(Debug)] @@ -37,21 +39,11 @@ impl File { } } -impl IntoResponse for File { - fn into_response(self) -> AxumResponse { - ( - StatusCode::OK, - [ - (CONTENT_TYPE, self.0.mime.as_ref()), - ( - LAST_MODIFIED, - &self.0.date_updated.format("%a, %d %b %Y %T %Z").to_string(), - ), - ], - Extension(CachePolicy::ForeverInCdnAndBrowser), - self.0.content, - ) - .into_response() +#[cfg(test)] +impl File { + pub fn into_response(self, if_none_match: Option<&IfNoneMatch>) -> AxumResponse { + let streaming_blob: StreamingBlob = self.0.into(); + StreamingFile(streaming_blob).into_response(if_none_match) } } @@ -63,36 +55,105 @@ impl StreamingFile { pub(super) async fn from_path(storage: &AsyncStorage, path: &str) -> Result { Ok(StreamingFile(storage.get_stream(path).await?)) } -} -impl IntoResponse for StreamingFile { - fn into_response(self) -> AxumResponse { - // Convert the AsyncBufRead into a Stream of Bytes - let stream = ReaderStream::new(self.0.content); - let body = Body::from_stream(stream); - ( - StatusCode::OK, - [ - (CONTENT_TYPE, self.0.mime.as_ref()), - ( - LAST_MODIFIED, - &self.0.date_updated.format("%a, %d %b %Y %T %Z").to_string(), - ), - ], - Extension(CachePolicy::ForeverInCdnAndBrowser), - body, - ) - .into_response() + pub fn into_response(self, if_none_match: Option<&IfNoneMatch>) -> AxumResponse { + const CACHE_POLICY: CachePolicy = CachePolicy::ForeverInCdnAndBrowser; + let last_modified = LastModified::from(SystemTime::from(self.0.date_updated)); + + if let Some(if_none_match) = if_none_match + && let Some(ref etag) = self.0.etag + && !if_none_match.precondition_passes(etag) + { + ( + StatusCode::NOT_MODIFIED, + // it's generally recommended to repeat caching headers on 304 responses + TypedHeader(etag.clone()), + TypedHeader(last_modified), + Extension(CACHE_POLICY), + ) + .into_response() + } else { + // Convert the AsyncBufRead into a Stream of Bytes + let stream = ReaderStream::new(self.0.content); + + ( + StatusCode::OK, + TypedHeader(ContentType::from(self.0.mime)), + TypedHeader(last_modified), + self.0.etag.map(TypedHeader), + Extension(CACHE_POLICY), + Body::from_stream(stream), + ) + .into_response() + } } } #[cfg(test)] mod tests { use super::*; - use crate::test::TestEnvironment; + use crate::{storage::CompressionAlgorithm, test::TestEnvironment, web::headers::compute_etag}; + use axum_extra::headers::{ETag, HeaderMapExt as _}; use chrono::Utc; - use http::header::CACHE_CONTROL; - use std::rc::Rc; + use http::header::{CACHE_CONTROL, ETAG, LAST_MODIFIED}; + use std::{io, rc::Rc}; + + fn streaming_blob( + content: impl Into>, + alg: Option, + ) -> StreamingBlob { + let content = content.into(); + StreamingBlob { + path: "some_path.db".into(), + mime: mime::APPLICATION_OCTET_STREAM, + date_updated: Utc::now(), + compression: alg, + etag: Some(compute_etag(&content)), + content_length: content.len(), + content: Box::new(io::Cursor::new(content)), + } + } + + #[tokio::test] + async fn test_stream_into_response() -> Result<()> { + const CONTENT: &[u8] = b"Hello, world!"; + let etag: ETag = { + // first request normal + let stream = StreamingFile(streaming_blob(CONTENT, None)); + let resp = stream.into_response(None); + assert!(resp.status().is_success()); + assert!(resp.headers().get(CACHE_CONTROL).is_none()); + let cache = resp + .extensions() + .get::() + .expect("missing cache response extension"); + assert!(matches!(cache, CachePolicy::ForeverInCdnAndBrowser)); + assert!(resp.headers().get(LAST_MODIFIED).is_some()); + + resp.headers().typed_get().unwrap() + }; + + let if_none_match = IfNoneMatch::from(etag); + + { + // cached request + let stream = StreamingFile(streaming_blob(CONTENT, None)); + let resp = stream.into_response(Some(&if_none_match)); + assert_eq!(resp.status(), StatusCode::NOT_MODIFIED); + + // cache related headers are repeated on the not-modified response + assert!(resp.headers().get(CACHE_CONTROL).is_none()); + let cache = resp + .extensions() + .get::() + .expect("missing cache response extension"); + assert!(matches!(cache, CachePolicy::ForeverInCdnAndBrowser)); + assert!(resp.headers().get(LAST_MODIFIED).is_some()); + assert!(resp.headers().get(ETAG).is_some()); + } + + Ok(()) + } #[tokio::test(flavor = "multi_thread")] async fn file_roundtrip_axum() -> Result<()> { @@ -111,7 +172,8 @@ mod tests { file.0.date_updated = now; - let resp = file.into_response(); + let resp = file.into_response(None); + assert!(resp.status().is_success()); assert!(resp.headers().get(CACHE_CONTROL).is_none()); let cache = resp .extensions() @@ -120,7 +182,7 @@ mod tests { assert!(matches!(cache, CachePolicy::ForeverInCdnAndBrowser)); assert_eq!( resp.headers().get(LAST_MODIFIED).unwrap(), - &now.format("%a, %d %b %Y %T UTC").to_string(), + &now.format("%a, %d %b %Y %T GMT").to_string(), ); Ok(()) diff --git a/src/web/headers/mod.rs b/src/web/headers/mod.rs index 7c1eef31b..03525d402 100644 --- a/src/web/headers/mod.rs +++ b/src/web/headers/mod.rs @@ -2,6 +2,7 @@ mod canonical_url; mod if_none_match; mod surrogate_key; +use axum_extra::headers::ETag; pub use canonical_url::CanonicalUrl; use http::HeaderName; pub(crate) use if_none_match::IfNoneMatch; @@ -14,8 +15,7 @@ pub static SURROGATE_CONTROL: HeaderName = HeaderName::from_static("surrogate-co /// compute our etag header value from some content /// /// Has to match the implementation in our build-script. -#[cfg(test)] -pub fn compute_etag>(content: T) -> axum_extra::headers::ETag { +pub fn compute_etag>(content: T) -> ETag { let digest = md5::compute(&content); format!("\"{:x}\"", digest).parse().unwrap() } diff --git a/src/web/metrics.rs b/src/web/metrics.rs index 0bc44576f..68cb059e3 100644 --- a/src/web/metrics.rs +++ b/src/web/metrics.rs @@ -143,7 +143,23 @@ pub(crate) async fn request_recorder( let result = next.run(request).await; let resp_time = start.elapsed().as_secs_f64(); - let attrs = [KeyValue::new("route", route_name.to_string())]; + // to be able to differentiate between kinds of responses (e.g., 2xx vs 4xx vs 5xx) + // in response times, or RPM. + // Special case for 304 Not Modified since it's about caching and not just redirecting. + let status_kind = match result.status() { + StatusCode::NOT_MODIFIED => "not_modified", + s if s.is_informational() => "informational", + s if s.is_success() => "success", + s if s.is_redirection() => "redirection", + s if s.is_client_error() => "client_error", + s if s.is_server_error() => "server_error", + _ => "other", + }; + + let attrs = [ + KeyValue::new("route", route_name.to_string()), + KeyValue::new("status_kind", status_kind), + ]; metrics .routes_visited diff --git a/src/web/rustdoc.rs b/src/web/rustdoc.rs index 802e845f2..aef08b6f3 100644 --- a/src/web/rustdoc.rs +++ b/src/web/rustdoc.rs @@ -19,6 +19,7 @@ use crate::{ rustdoc::{PageKind, RustdocParams}, }, file::StreamingFile, + headers::IfNoneMatch, match_version, metrics::WebMetrics, page::{ @@ -35,7 +36,8 @@ use axum::{ http::StatusCode, response::{IntoResponse, Response as AxumResponse}, }; -use http::{HeaderValue, Uri, header, uri::Authority}; +use axum_extra::{headers::ContentType, typed_header::TypedHeader}; +use http::{Uri, uri::Authority}; use serde::Deserialize; use std::{ collections::HashMap, @@ -193,6 +195,7 @@ pub(crate) static DOC_RUST_LANG_ORG_REDIRECTS: LazyLock, path: impl AsRef, + if_none_match: Option<&IfNoneMatch>, ) -> AxumResult { let path = path.as_ref().to_owned(); // FIXME: this could be optimized: when a path doesn't exist @@ -204,8 +207,8 @@ async fn try_serve_legacy_toolchain_asset( // toolchain specific resources into the new folder, // which is reached via the new handler. Ok(StreamingFile::from_path(&storage, &path) - .await - .map(IntoResponse::into_response)?) + .await? + .into_response(if_none_match)) } /// Handler called for `/:crate` and `/:crate/:version` URLs. Automatically redirects to the docs @@ -215,6 +218,7 @@ pub(crate) async fn rustdoc_redirector_handler( params: RustdocParams, Extension(storage): Extension>, mut conn: DbConnection, + if_none_match: Option>, RawQuery(original_query): RawQuery, ) -> AxumResult { let params = params.with_page_kind(PageKind::Rustdoc); @@ -255,7 +259,7 @@ pub(crate) async fn rustdoc_redirector_handler( .binary_search(&extension) .is_ok() { - return try_serve_legacy_toolchain_asset(storage, params.name()) + return try_serve_legacy_toolchain_asset(storage, params.name(), if_none_match.as_deref()) .instrument(info_span!("serve static asset")) .await; } @@ -319,7 +323,7 @@ pub(crate) async fn rustdoc_redirector_handler( ) .await { - Ok(blob) => Ok(StreamingFile(blob).into_response()), + Ok(blob) => Ok(StreamingFile(blob).into_response(if_none_match.as_deref())), Err(err) => { if !matches!(err.downcast_ref(), Some(AxumNope::ResourceNotFound)) && !matches!(err.downcast_ref(), Some(crate::storage::PathNotFoundError)) @@ -332,7 +336,12 @@ pub(crate) async fn rustdoc_redirector_handler( // docs that were affected by this bug. // https://github.com/rust-lang/docs.rs/issues/1979 if inner_path.starts_with("search-") || inner_path.starts_with("settings-") { - try_serve_legacy_toolchain_asset(storage, inner_path).await + try_serve_legacy_toolchain_asset( + storage, + inner_path, + if_none_match.as_deref(), + ) + .await } else { Err(err.into()) } @@ -400,10 +409,7 @@ impl RustdocPage { } else { CachePolicy::ForeverInCdnAndStaleInBrowser }), - [( - header::CONTENT_TYPE, - HeaderValue::from_static(mime::TEXT_HTML_UTF_8.as_ref()), - )], + TypedHeader(ContentType::from(mime::TEXT_HTML_UTF_8)), Body::from_stream(utils::rewrite_rustdoc_html_stream( template_data, rustdoc_html.content, @@ -436,6 +442,7 @@ pub(crate) async fn rustdoc_html_server_handler( Extension(config): Extension>, Extension(csp): Extension>, RawQuery(original_query): RawQuery, + if_none_match: Option>, mut conn: DbConnection, ) -> AxumResult { let params = params.with_page_kind(PageKind::Rustdoc); @@ -598,7 +605,7 @@ pub(crate) async fn rustdoc_html_server_handler( // default asset caching behaviour is `Cache::ForeverInCdnAndBrowser`. // This is an edge-case when we serve invocation specific static assets under `/latest/`: // https://github.com/rust-lang/docs.rs/issues/1593 - return Ok(StreamingFile(blob).into_response()); + return Ok(StreamingFile(blob).into_response(if_none_match.as_deref())); } let latest_release = krate.latest_release()?; @@ -910,10 +917,13 @@ pub(crate) async fn download_handler( pub(crate) async fn static_asset_handler( Path(path): Path, Extension(storage): Extension>, + if_none_match: Option>, ) -> AxumResult { let storage_path = format!("{RUSTDOC_STATIC_STORAGE_PREFIX}{path}"); - Ok(StreamingFile::from_path(&storage, &storage_path).await?) + Ok(StreamingFile::from_path(&storage, &storage_path) + .await? + .into_response(if_none_match.as_deref())) } #[cfg(test)] @@ -2986,14 +2996,39 @@ mod test { .await?; let web = env.web_app().await; - let response = web.get(&format!("/dummy/0.1.0/{name}")).await?; - assert!(response.status().is_success()); - assert_eq!(response.text().await?, "content"); + + web.assert_success_and_conditional_get(&format!("/dummy/0.1.0/{name}"), "content") + .await?; Ok(()) }) } + #[tokio::test(flavor = "multi_thread")] + #[test_case("folder/file.js")] + #[test_case("root.css")] + async fn test_static_asset_handler(path: &str) -> Result<()> { + let env = TestEnvironment::new().await?; + + let storage = env.async_storage(); + storage + .store_one( + format!("{RUSTDOC_STATIC_STORAGE_PREFIX}{path}"), + b"static content", + ) + .await?; + + let web = env.web_app().await; + + web.assert_success_and_conditional_get( + &format!("/-/rustdoc.static/{path}"), + "static content", + ) + .await?; + + Ok(()) + } + #[test_case("search-1234.js")] #[test_case("settings-1234.js")] fn fallback_to_root_storage_for_some_js_assets(path: &str) { @@ -3020,12 +3055,11 @@ mod test { "{:?}", response.headers().get("Location"), ); - assert!(web.get("/asset.js").await?.status().is_success()); - assert!(web.get(&format!("/{path}")).await?.status().is_success()); - let response = web.get(&format!("/dummy/0.1.0/{path}")).await?; - assert!(response.status().is_success()); - assert_eq!(response.text().await?, "more_content"); + web.assert_success_and_conditional_get("/asset.js", "content") + .await?; + web.assert_success_and_conditional_get(&format!("/dummy/0.1.0/{path}"), "more_content") + .await?; Ok(()) }) diff --git a/src/web/source.rs b/src/web/source.rs index abac50349..815ff1e53 100644 --- a/src/web/source.rs +++ b/src/web/source.rs @@ -13,6 +13,7 @@ use crate::{ }, file::StreamingFile, headers::CanonicalUrl, + headers::IfNoneMatch, match_version, page::templates::{RenderBrands, RenderRegular, RenderSolid, filters}, }, @@ -20,7 +21,7 @@ use crate::{ use anyhow::{Context as _, Result}; use askama::Template; use axum::{Extension, response::IntoResponse}; -use axum_extra::headers::HeaderMapExt; +use axum_extra::{TypedHeader, headers::HeaderMapExt}; use mime::Mime; use std::{cmp::Ordering, sync::Arc}; use tracing::instrument; @@ -190,6 +191,7 @@ pub(crate) async fn source_browser_handler( Extension(storage): Extension>, Extension(config): Extension>, mut conn: DbConnection, + if_none_match: Option>, ) -> AxumResult { let params = params.with_page_kind(PageKind::Source); let matched_release = match_version(&mut conn, params.name(), params.req_version()) @@ -275,7 +277,7 @@ pub(crate) async fn source_browser_handler( let is_text = stream.mime.type_() == mime::TEXT || stream.mime == mime::APPLICATION_JSON; if !is_text { // if the file isn't text, serve it directly to the client - let mut response = StreamingFile(stream).into_response(); + let mut response = StreamingFile(stream).into_response(if_none_match.as_deref()); response.headers_mut().typed_insert(canonical_url); response .extensions_mut() @@ -352,10 +354,12 @@ pub(crate) async fn source_browser_handler( mod tests { use crate::{ test::{AxumResponseTestExt, AxumRouterTestExt, TestEnvironment, async_wrapper}, - web::{cache::CachePolicy, encode_url_path}, + web::{cache::CachePolicy, encode_url_path, headers::IfNoneMatch}, }; use anyhow::Result; + use axum_extra::headers::{ContentType, ETag, HeaderMapExt as _}; use kuchikiki::traits::TendrilSink; + use mime::APPLICATION_PDF; use reqwest::StatusCode; use test_case::test_case; @@ -447,24 +451,34 @@ mod tests { .create() .await?; let web = env.web_app().await; - let response = web.get("/crate/fake/0.1.0/source/some_file.pdf").await?; + + const URL: &str = "/crate/fake/0.1.0/source/some_file.pdf"; + + // first request, uncached + let response = web.get(URL).await?; assert!(response.status().is_success()); + let headers = response.headers(); assert_eq!( - response.headers().get("link").unwrap(), + headers.get("link").unwrap(), "; rel=\"canonical\"" ); assert_eq!( - response - .headers() - .get("content-type") - .unwrap() - .to_str() - .unwrap(), - "application/pdf" + headers.typed_get::().unwrap(), + APPLICATION_PDF.into(), ); - response.assert_cache_control(CachePolicy::ForeverInCdnAndStaleInBrowser, env.config()); + + let etag: ETag = headers.typed_get().unwrap(); + assert!(response.text().await?.contains("some_random_content")); + + let response = web + .get_with_headers(URL, |headers| { + headers.typed_insert(IfNoneMatch::from(etag)); + }) + .await?; + assert_eq!(response.status(), StatusCode::NOT_MODIFIED); + Ok(()) }); }