custom impl

Author: syphar

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"
+

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,
@@ -14,7 +14,7 @@ use axum::{
 };
 use axum_extra::{
     TypedHeader,
-    headers::{ContentType, IfNoneMatch, LastModified},
+    headers::{ContentType, LastModified},
 };
 use std::time::SystemTime;
 use tokio_util::io::ReaderStream;

src/web/headers/if_none_match.rs

@@ -0,0 +1,67 @@
+//! Adapted version of `headers::IfNoneMatch`.
+//!
+//! The combination of `TypedHeader` and `IfNoneMatch` works in odd ways.
+//! They are built in a way that a _missing_ `If-None-Match` header will lead to:
+//!
+//! 1. extractor with `TypedHeader<IfNoneMatch>` returning `IfNoneMatch("")`
+//! 2. extractor with `Option<TypedHeader<IfNoneMatch>>` returning `Some(IfNoneMatch(""))`
+//!
+//! Where I would expect:
+//! 1. a failure because of the missing header
+//! 2. `None` for the missing header
+//!
+//! This could be solved by either adapting `TypedHeader` or `IfNoneMatch`, I'm not sure which is
+//! right.
+//!
+//! Some reading material for those interested:
+//! * https://github.com/hyperium/headers/issues/204
+//! * https://github.com/hyperium/headers/pull/165
+//! * https://github.com/tokio-rs/axum/issues/1781
+//! * https://github.com/tokio-rs/axum/pull/1810
+//! * https://github.com/tokio-rs/axum/pull/2475
+//!
+//! Right now I feel like adapting `IfNoneMatch` is the "most correct-ish" option.
+
+#[allow(clippy::disallowed_types)]
+mod header_impl {
+    use axum_extra::headers::{self, ETag, Header, IfNoneMatch as OriginalIfNoneMatch};
+    use derive_more::Deref;
+
+    #[derive(Debug, Clone, PartialEq, Deref)]
+    pub(crate) struct IfNoneMatch(pub axum_extra::headers::IfNoneMatch);
+
+    impl Header for IfNoneMatch {
+        fn name() -> &'static http::HeaderName {
+            OriginalIfNoneMatch::name()
+        }
+
+        fn decode<'i, I>(values: &mut I) -> Result<Self, headers::Error>
+        where
+            Self: Sized,
+            I: Iterator<Item = &'i http::HeaderValue>,
+        {
+            let mut values = values.peekable();
+            // NOTE: this is the difference to the original implementation.
+            // When there is no header set, I want the decoding to fail.
+            // This makes Option<TypedHeader<H>> return `None`, and also matches
+            // most other header implementations.
+            if values.peek().is_none() {
+                Err(headers::Error::invalid())
+            } else {
+                OriginalIfNoneMatch::decode(&mut values).map(IfNoneMatch)
+            }
+        }
+
+        fn encode<E: Extend<http::HeaderValue>>(&self, values: &mut E) {
+            self.0.encode(values)
+        }
+    }
+
+    impl From<ETag> for IfNoneMatch {
+        fn from(value: ETag) -> Self {
+            Self(value.into())
+        }
+    }
+}
+
+pub(crate) use header_impl::IfNoneMatch;

src/web/headers/mod.rs

@@ -1,9 +1,11 @@
 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;
 pub use surrogate_key::{SURROGATE_KEY, SurrogateKey, SurrogateKeys};
 
 /// Fastly's Surrogate-Control header

src/web/rustdoc.rs

@@ -19,6 +19,7 @@ use crate::{
             rustdoc::{PageKind, RustdocParams},
         },
         file::StreamingFile,
+        headers::IfNoneMatch,
         match_version,
         metrics::WebMetrics,
         page::{
@@ -35,10 +36,7 @@ use axum::{
     http::StatusCode,
     response::{IntoResponse, Response as AxumResponse},
 };
-use axum_extra::{
-    headers::{ContentType, IfNoneMatch},
-    typed_header::TypedHeader,
-};
+use axum_extra::{headers::ContentType, typed_header::TypedHeader};
 use http::{Uri, uri::Authority};
 use serde::Deserialize;
 use std::{

src/web/source.rs

@@ -13,17 +13,15 @@ use crate::{
         },
         file::File as DbFile,
         headers::CanonicalUrl,
+        headers::IfNoneMatch,
         match_version,
         page::templates::{RenderBrands, RenderRegular, RenderSolid, filters},
     },
 };
 use anyhow::{Context as _, Result};
 use askama::Template;
 use axum::{Extension, response::IntoResponse};
-use axum_extra::{
-    TypedHeader,
-    headers::{HeaderMapExt, IfNoneMatch},
-};
+use axum_extra::{TypedHeader, headers::HeaderMapExt};
 use mime::Mime;
 use std::{cmp::Ordering, sync::Arc};
 use tracing::instrument;

src/web/statics.rs

@@ -1,4 +1,6 @@
-use super::{cache::CachePolicy, metrics::request_recorder, routes::get_static};
+use super::{
+    cache::CachePolicy, headers::IfNoneMatch, metrics::request_recorder, routes::get_static,
+};
 use axum::{
     Router as AxumRouter,
     extract::{Extension, Request},
@@ -8,7 +10,7 @@ use axum::{
     routing::get_service,
 };
 use axum_extra::{
-    headers::{ETag, HeaderMapExt as _, HeaderValue, IfNoneMatch},
+    headers::{ETag, HeaderMapExt as _, HeaderValue},
     typed_header::TypedHeader,
 };
 use http::{Method, StatusCode, Uri};
@@ -137,25 +139,23 @@ pub(crate) fn build_static_router() -> AxumRouter {
 
 #[cfg(test)]
 mod tests {
-    use super::{STYLE_CSS, VENDORED_CSS};
+    use super::*;
     use crate::{
         test::{AxumResponseTestExt, AxumRouterTestExt, async_wrapper},
-        web::{cache::CachePolicy, headers::compute_etag},
+        web::headers::compute_etag,
     };
-    use axum::{Router, body::Body, extract::Request, response::Response as AxumResponse};
-    use axum_extra::headers::{ETag, HeaderMapExt as _, IfNoneMatch};
+    use axum::{Router, body::Body};
     use http::{
         HeaderMap,
         header::{CONTENT_LENGTH, CONTENT_TYPE, ETAG},
     };
-    use reqwest::StatusCode;
     use std::fs;
     use test_case::test_case;
     use tower::ServiceExt as _;
 
     const STATIC_SEARCH_PATHS: &[&str] = &["static", "vendor"];
 
-    fn content_length(resp: &AxumResponse) -> u64 {
+    fn content_length(resp: &Response) -> u64 {
         resp.headers()
             .get(CONTENT_LENGTH)
             .expect("content-length header")
@@ -165,7 +165,7 @@ mod tests {
             .unwrap()
     }
 
-    fn etag(resp: &AxumResponse) -> ETag {
+    fn etag(resp: &Response) -> ETag {
         let etag = resp.headers().get(ETAG).unwrap();
         etag.to_str().unwrap().parse().unwrap()
     }