new cache-policy & cache middleware structure to support full page caching

src/config.rs

@@ -61,7 +61,6 @@ pub struct Config {
     // If both are absent, don't generate the header. If only one is present,
     // generate just that directive. Values are in seconds.
     pub(crate) cache_control_stale_while_revalidate: Option<u32>,
-    pub(crate) cache_control_max_age: Option<u32>,
 
     pub(crate) cdn_backend: CdnKind,
 
@@ -145,7 +144,6 @@ impl Config {
             cache_control_stale_while_revalidate: maybe_env(
                 "CACHE_CONTROL_STALE_WHILE_REVALIDATE",
             )?,
-            cache_control_max_age: maybe_env("CACHE_CONTROL_MAX_AGE")?,
 
             cdn_backend: env("DOCSRS_CDN_BACKEND", CdnKind::Dummy)?,
 

src/test/mod.rs

@@ -6,15 +6,16 @@ use crate::db::{Pool, PoolClient};
 use crate::error::Result;
 use crate::repositories::RepositoryStatsUpdater;
 use crate::storage::{Storage, StorageKind};
-use crate::web::Server;
+use crate::web::{cache, Server};
 use crate::{BuildQueue, Config, Context, Index, Metrics};
 use anyhow::Context as _;
 use fn_error_context::context;
+use iron::headers::CacheControl;
 use log::error;
 use once_cell::unsync::OnceCell;
 use postgres::Client as Connection;
 use reqwest::{
-    blocking::{Client, ClientBuilder, RequestBuilder},
+    blocking::{Client, ClientBuilder, RequestBuilder, Response},
     Method,
 };
 use std::{fs, net::SocketAddr, panic, sync::Arc, time::Duration};
@@ -42,21 +43,79 @@ pub(crate) fn wrapper(f: impl FnOnce(&TestEnvironment) -> Result<()>) {
     }
 }
 
+/// check a request if the cache control header matches NoCache
+pub(crate) fn assert_no_cache(res: &Response) {
+    assert_eq!(
+        res.headers()
+            .get("Cache-Control")
+            .expect("missing cache-control header"),
+        cache::NO_CACHE,
+    );
+}
+
+/// check a request if the cache control header matches the given cache config.
+pub(crate) fn assert_cache_control(
+    res: &Response,
+    cache_policy: cache::CachePolicy,
+    config: &Config,
+) {
+    assert!(config.cache_control_stale_while_revalidate.is_some());
+    let cache_control = res.headers().get("Cache-Control");
+
+    let expected_directives = cache_policy.render(config);
+    if expected_directives.is_empty() {
+        assert!(cache_control.is_none());
+    } else {
+        assert_eq!(
+            cache_control.expect("missing cache-control header"),
+            &CacheControl(expected_directives).to_string()
+        );
+    }
+}
+
 /// Make sure that a URL returns a status code between 200-299
 pub(crate) fn assert_success(path: &str, web: &TestFrontend) -> Result<()> {
     let status = web.get(path).send()?.status();
     assert!(status.is_success(), "failed to GET {}: {}", path, status);
     Ok(())
 }
 
+/// Make sure that a URL returns a status code between 200-299,
+/// also check the cache-control headers.
+pub(crate) fn assert_success_cached(
+    path: &str,
+    web: &TestFrontend,
+    cache_policy: cache::CachePolicy,
+    config: &Config,
+) -> Result<()> {
+    let response = web.get(path).send()?;
+    assert_cache_control(&response, cache_policy, config);
+    let status = response.status();
+    assert!(status.is_success(), "failed to GET {}: {}", path, status);
+    Ok(())
+}
+
 /// Make sure that a URL returns a 404
 pub(crate) fn assert_not_found(path: &str, web: &TestFrontend) -> Result<()> {
-    let status = web.get(path).send()?.status();
-    assert_eq!(status, 404, "GET {} should have been a 404", path);
+    let response = web.get(path).send()?;
+
+    // for now, 404s should always have `no-cache`
+    assert_no_cache(&response);
+
+    assert_eq!(
+        response.status(),
+        404,
+        "GET {} should have been a 404",
+        path
+    );
     Ok(())
 }
 
-fn assert_redirect_common(path: &str, expected_target: &str, web: &TestFrontend) -> Result<()> {
+fn assert_redirect_common(
+    path: &str,
+    expected_target: &str,
+    web: &TestFrontend,
+) -> Result<Response> {
     let response = web.get_no_redirect(path).send()?;
     let status = response.status();
     if !status.is_redirection() {
@@ -83,7 +142,7 @@ fn assert_redirect_common(path: &str, expected_target: &str, web: &TestFrontend)
         anyhow::bail!("got redirect to {redirect_target}");
     }
 
-    Ok(())
+    Ok(response)
 }
 
 /// Makes sure that a URL redirects to a specific page, but doesn't check that the target exists
@@ -93,7 +152,7 @@ pub(crate) fn assert_redirect_unchecked(
     expected_target: &str,
     web: &TestFrontend,
 ) -> Result<()> {
-    assert_redirect_common(path, expected_target, web)
+    assert_redirect_common(path, expected_target, web).map(|_| ())
 }
 
 /// Make sure that a URL redirects to a specific page, and that the target exists and is not another redirect
@@ -110,6 +169,28 @@ pub(crate) fn assert_redirect(path: &str, expected_target: &str, web: &TestFront
     Ok(())
 }
 
+/// Make sure that a URL redirects to a specific page, and that the target exists and is not another redirect.
+/// Also verifies that the redirect's cache-control header matches the provided cache policy.
+#[context("expected redirect from {path} to {expected_target}")]
+pub(crate) fn assert_redirect_cached(
+    path: &str,
+    expected_target: &str,
+    cache_policy: cache::CachePolicy,
+    web: &TestFrontend,
+    config: &Config,
+) -> Result<()> {
+    let redirect_response = assert_redirect_common(path, expected_target, web)?;
+    assert_cache_control(&redirect_response, cache_policy, config);
+
+    let response = web.get_no_redirect(expected_target).send()?;
+    let status = response.status();
+    if !status.is_success() {
+        anyhow::bail!("failed to GET {expected_target}: {status}");
+    }
+
+    Ok(())
+}
+
 pub(crate) struct TestEnvironment {
     build_queue: OnceCell<Arc<BuildQueue>>,
     config: OnceCell<Arc<Config>>,
@@ -187,6 +268,10 @@ impl TestEnvironment {
         config.local_archive_cache_path =
             std::env::temp_dir().join(format!("docsrs-test-index-{}", rand::random::<u64>()));
 
+        // set stale content serving so Cache::ForeverInCdn and Cache::ForeverInCdnAndStaleInBrowser
+        // are actually different.
+        config.cache_control_stale_while_revalidate = Some(86400);
+
         config
     }
 

src/web/builds.rs

@@ -1,4 +1,4 @@
-use super::{match_version, redirect_base, MatchSemver};
+use super::{cache::CachePolicy, match_version, redirect_base, MatchSemver};
 use crate::{
     db::Pool,
     docbuilder::Limits,
@@ -7,9 +7,7 @@ use crate::{
 };
 use chrono::{DateTime, Utc};
 use iron::{
-    headers::{
-        AccessControlAllowOrigin, CacheControl, CacheDirective, ContentType, Expires, HttpDate,
-    },
+    headers::{AccessControlAllowOrigin, ContentType},
     status, IronResult, Request, Response, Url,
 };
 use router::Router;
@@ -107,12 +105,8 @@ pub fn build_list_handler(req: &mut Request) -> IronResult<Response> {
     if is_json {
         let mut resp = Response::with((status::Ok, serde_json::to_string(&builds).unwrap()));
         resp.headers.set(ContentType::json());
-        resp.headers.set(Expires(HttpDate(time::now())));
-        resp.headers.set(CacheControl(vec![
-            CacheDirective::NoCache,
-            CacheDirective::NoStore,
-            CacheDirective::MustRevalidate,
-        ]));
+        resp.extensions
+            .insert::<CachePolicy>(CachePolicy::NoStoreMustRevalidate);
         resp.headers.set(AccessControlAllowOrigin::Any);
 
         Ok(resp)
@@ -131,7 +125,10 @@ pub fn build_list_handler(req: &mut Request) -> IronResult<Response> {
 
 #[cfg(test)]
 mod tests {
-    use crate::test::{wrapper, FakeBuild};
+    use crate::{
+        test::{assert_cache_control, wrapper, FakeBuild},
+        web::cache::CachePolicy,
+    };
     use chrono::{DateTime, Duration, Utc};
     use kuchiki::traits::TendrilSink;
     use reqwest::StatusCode;
@@ -156,12 +153,9 @@ mod tests {
                 ])
                 .create()?;
 
-            let page = kuchiki::parse_html().one(
-                env.frontend()
-                    .get("/crate/foo/0.1.0/builds")
-                    .send()?
-                    .text()?,
-            );
+            let response = env.frontend().get("/crate/foo/0.1.0/builds").send()?;
+            assert_cache_control(&response, CachePolicy::NoCaching, &env.config());
+            let page = kuchiki::parse_html().one(response.text()?);
 
             let rows: Vec<_> = page
                 .select("ul > li a.release")
@@ -200,12 +194,9 @@ mod tests {
                 ])
                 .create()?;
 
-            let value: serde_json::Value = serde_json::from_str(
-                &env.frontend()
-                    .get("/crate/foo/0.1.0/builds.json")
-                    .send()?
-                    .text()?,
-            )?;
+            let response = env.frontend().get("/crate/foo/0.1.0/builds.json").send()?;
+            assert_cache_control(&response, CachePolicy::NoStoreMustRevalidate, &env.config());
+            let value: serde_json::Value = serde_json::from_str(&response.text()?)?;
 
             assert_eq!(value.pointer("/0/build_status"), Some(&true.into()));
             assert_eq!(