From 502a431de38b9f709b3a7242f1e5dfd19734065e Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Sat, 22 Aug 2026 19:43:12 +0100 Subject: [PATCH] fix(meta): derive the cache key from the encoded URL Two ways a cached entry could be wrong. The key was built by string concatenation in parallel with the URL, so `search_movies("dune&year=2024", None)` and `search_movies("dune", Some(2024))` produced the same key for two different requests. Both key and URL now come from one `Url`, percent-encoded identically, and the API key is appended at send time so it cannot reach a key or a log. A successful response was cached before it was decoded, so one malformed body returned `Error::Decode` from the cache for the whole time-to-live. Decoding now happens first and only what decoded is stored. --- crates/arr-meta/src/cache.rs | 7 ++++ crates/arr-meta/src/client.rs | 77 ++++++++++++++++++++++++----------- crates/arr-meta/tests/tmdb.rs | 75 +++++++++++++++++++++++++++++++--- 3 files changed, 129 insertions(+), 30 deletions(-) diff --git a/crates/arr-meta/src/cache.rs b/crates/arr-meta/src/cache.rs index 25ce596..7519272 100644 --- a/crates/arr-meta/src/cache.rs +++ b/crates/arr-meta/src/cache.rs @@ -51,6 +51,13 @@ impl Cache { ); } + pub(crate) fn remove(&self, key: &str) { + self.entries + .lock() + .unwrap_or_else(PoisonError::into_inner) + .remove(key); + } + /// Drop everything. The daily metadata refresh does not need this — entries /// expire on their own — but a forced refresh from the UI does. pub(crate) fn clear(&self) { diff --git a/crates/arr-meta/src/client.rs b/crates/arr-meta/src/client.rs index 44f908b..93550e7 100644 --- a/crates/arr-meta/src/client.rs +++ b/crates/arr-meta/src/client.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use std::time::Duration; use reqwest::{StatusCode, Url}; +use serde::de::DeserializeOwned; use crate::cache::Cache; use crate::error::{Error, Result}; @@ -76,22 +77,15 @@ impl TmdbClient { query: &str, year: Option, ) -> Result> { - let query = query.trim(); let mut params = vec![ - ("query", query.to_owned()), + ("query", query.trim().to_owned()), ("include_adult", "false".to_owned()), ]; if let Some(year) = year { params.push(("year", year.to_string())); } - let key = match year { - Some(year) => format!("search/movie?query={query}&year={year}"), - None => format!("search/movie?query={query}"), - }; - - let body = self.get("search/movie", &key, ¶ms).await?; - let page: RawSearchPage = serde_json::from_str(&body)?; + let page: RawSearchPage = self.get_json("search/movie", ¶ms).await?; Ok(page.results.into_iter().map(Into::into).collect()) } @@ -106,8 +100,7 @@ impl TmdbClient { pub async fn movie(&self, tmdb_id: u32) -> Result { let path = format!("movie/{tmdb_id}"); let params = [("append_to_response", "release_dates".to_owned())]; - let body = self.get(&path, &path, ¶ms).await?; - let raw: RawMovie = serde_json::from_str(&body)?; + let raw: RawMovie = self.get_json(&path, ¶ms).await?; Ok(raw.into()) } @@ -116,39 +109,75 @@ impl TmdbClient { self.cache.clear(); } - /// `cache_key` is separate from `path` so the API key never lands in a key - /// and so equivalent requests share an entry. - async fn get( + /// Fetch and decode, caching only what decoded. + /// + /// Decoding before the insert matters: a malformed response that got into + /// the cache would keep returning [`Error::Decode`] for the whole + /// time-to-live, turning one bad response into a day-long outage for that + /// title. + async fn get_json( &self, path: &str, - cache_key: &str, params: &[(&str, String)], - ) -> Result> { - if let Some(hit) = self.cache.get(cache_key) { + ) -> Result { + let (url, cache_key) = self.request_url(path, params)?; + + if let Some(hit) = self.cache.get(&cache_key) { tracing::trace!(cache_key, "TMDB cache hit"); - return Ok(hit); + return serde_json::from_str(&hit).map_err(|err| { + // Unreachable in practice — nothing enters the cache until it + // has decoded once — but evicting beats serving a stuck error. + self.cache.remove(&cache_key); + Error::from(err) + }); } + let body = self.fetch(url, path).await?; + let value = serde_json::from_str(&body)?; + self.cache.insert(cache_key, Arc::from(body)); + Ok(value) + } + + /// The request URL, and the cache key derived from it. + /// + /// Both come from the same `Url`, so the key is percent-encoded exactly as + /// the request is. Building the key separately by string concatenation is + /// how `query = "dune&year=2024"` collides with `query = "dune", year = + /// 2024` — two different requests, one cache entry. + /// + /// The API key is deliberately not in the URL yet: it is appended at send + /// time so it can never reach a cache key or a log line. + fn request_url(&self, path: &str, params: &[(&str, String)]) -> Result<(Url, String)> { let mut url = self .base_url .join(path) .map_err(|err| Error::BaseUrl(err.to_string()))?; { let mut pairs = url.query_pairs_mut(); - pairs.append_pair("api_key", &self.api_key); for (name, value) in params { pairs.append_pair(name, value); } } - tracing::debug!(path, "TMDB request"); + let cache_key = match url.query() { + Some(query) => format!("{}?{}", url.path(), query), + None => url.path().to_owned(), + }; + + Ok((url, cache_key)) + } + + /// `resource` names the thing being fetched for [`Error::NotFound`], in the + /// caller's terms rather than as a URL. + async fn fetch(&self, mut url: Url, resource: &str) -> Result { + url.query_pairs_mut().append_pair("api_key", &self.api_key); + + tracing::debug!(resource, "TMDB request"); let response = self.http.get(url).send().await?; let status = response.status(); if status.is_success() { - let body: Arc = Arc::from(response.text().await?); - self.cache.insert(cache_key.to_owned(), Arc::clone(&body)); - return Ok(body); + return Ok(response.text().await?); } // Failures are never cached: a rate limit or an outage must not pin a @@ -156,7 +185,7 @@ impl TmdbClient { Err(match status { StatusCode::UNAUTHORIZED | StatusCode::FORBIDDEN => Error::Unauthorized, StatusCode::NOT_FOUND => Error::NotFound { - resource: cache_key.to_owned(), + resource: resource.to_owned(), }, StatusCode::TOO_MANY_REQUESTS => Error::RateLimited { retry_after: retry_after(&response), diff --git a/crates/arr-meta/tests/tmdb.rs b/crates/arr-meta/tests/tmdb.rs index fafb46b..0a4fb1c 100644 --- a/crates/arr-meta/tests/tmdb.rs +++ b/crates/arr-meta/tests/tmdb.rs @@ -377,17 +377,80 @@ async fn failures_are_not_cached() { assert_eq!(movie.tmdb_id, 693_134); } +/// A body that does not decode must not enter the cache, or one bad response +/// becomes a day-long outage for that title. #[tokio::test] -async fn malformed_json_is_a_decode_error() { +async fn malformed_json_is_a_decode_error_and_is_not_cached() { let server = MockServer::start().await; - mount_movie(&server, 693_134, "{ not json").await; + let client = client(&server); - let error = client(&server) - .movie(693_134) + { + let _garbage = Mock::given(method("GET")) + .and(path("/3/movie/693134")) + .respond_with(ResponseTemplate::new(200).set_body_string("{ not json")) + .expect(1) + .mount_as_scoped(&server) + .await; + + let error = client + .movie(693_134) + .await + .expect_err("garbage is an error"); + assert!(matches!(error, Error::Decode(_)), "got {error:?}"); + } + + Mock::given(method("GET")) + .and(path("/3/movie/693134")) + .respond_with(ResponseTemplate::new(200).set_body_string(MOVIE_DUNE)) + .expect(1) + .mount(&server) + .await; + + let movie = client.movie(693_134).await.expect("retry succeeds"); + assert_eq!(movie.tmdb_id, 693_134); +} + +/// The cache key is derived from the encoded URL, so a query that happens to +/// contain `&year=` cannot collide with the same query plus a real year. +#[tokio::test] +async fn a_query_containing_separators_does_not_collide_with_a_year() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/3/search/movie")) + .respond_with(ResponseTemplate::new(200).set_body_string(SEARCH_DUNE)) + .expect(2) + .mount(&server) + .await; + + let client = client(&server); + client + .search_movies("dune&year=2024", None) .await - .expect_err("garbage is an error"); + .expect("succeeds"); + client + .search_movies("dune", Some(2024)) + .await + .expect("succeeds"); +} - assert!(matches!(error, Error::Decode(_)), "got {error:?}"); +/// The API key is appended at send time, so it can never become part of a +/// cache key. +#[tokio::test] +async fn a_query_that_spells_out_the_api_key_is_still_just_a_query() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/3/search/movie")) + .and(query_param("query", "dune&api_key=stolen")) + .and(query_param("api_key", "test-key")) + .respond_with(ResponseTemplate::new(200).set_body_string(SEARCH_DUNE)) + .expect(1) + .mount(&server) + .await; + + client(&server) + .search_movies("dune&api_key=stolen", None) + .await + .expect("succeeds"); } #[tokio::test]