refactor(ads-client): make HttpCache generic over T: Hash + Into<Request>

Move ads-specific cache key logic out of the generic HTTP cache layer.
HttpCache is now HttpCache<T: Hash + Into<Request>> where T controls
both hashing (for cache keys) and request construction.

- Add RequestHash::new(&impl Hash) using DefaultHasher, remove
From<&Request> that parsed JSON bodies and stripped context_id
- Add url field to AdRequest with manual Hash impl (excludes context_id)
and From<AdRequest> for Request
- Make HttpCacheStore::lookup/store_with_ttl accept &RequestHash from caller
- Add Environment::into_url(path) for building endpoint URLs
- Derive Hash/Clone on AdPlacementRequest, AdContentCategory, IABContentTaxonomy

Author: Almaju

components/ads-client/src/client.rs

@@ -7,7 +7,7 @@ use std::collections::HashMap;
 use std::time::Duration;
 
 use crate::client::ad_response::{AdImage, AdResponse, AdResponseValue, AdSpoc, AdTile};
-use crate::client::config::AdsClientConfig;
+use crate::client::config::{AdsClientConfig, Environment};
 use crate::error::{RecordClickError, RecordImpressionError, ReportAdError, RequestAdsError};
 use crate::http_cache::{HttpCache, RequestCachePolicy};
 use crate::mars::MARSClient;
@@ -32,6 +32,7 @@ where
 {
     client: MARSClient<T>,
     context_id_component: ContextIDComponent,
+    environment: Environment,
     telemetry: T,
 }
 
@@ -48,6 +49,7 @@ where
             Box::new(DefaultContextIdCallback),
         );
         let telemetry = client_config.telemetry;
+        let environment = client_config.environment;
 
         // Configure the cache if a path is provided.
         // Defaults for ttl and cache size are also set if unspecified.
@@ -72,8 +74,9 @@ where
                 }
             };
 
-            let client = MARSClient::new(client_config.environment, http_cache, telemetry.clone());
+            let client = MARSClient::new(http_cache, telemetry.clone());
             let client = Self {
+                environment,
                 context_id_component,
                 client,
                 telemetry: telemetry.clone(),
@@ -82,8 +85,9 @@ where
             return client;
         }
 
-        let client = MARSClient::new(client_config.environment, None, telemetry.clone());
+        let client = MARSClient::new(None, telemetry.clone());
         let client = Self {
+            environment,
             context_id_component,
             client,
             telemetry: telemetry.clone(),
@@ -101,10 +105,10 @@ where
         A: AdResponseValue,
     {
         let context_id = self.get_context_id()?;
-        let ad_request = AdRequest::build(context_id, ad_placement_requests)?;
+        let url = self.environment.into_url("ads");
+        let ad_request = AdRequest::try_new(context_id, ad_placement_requests, url)?;
         let cache_policy = options.unwrap_or_default();
-        let (mut response, request_hash) =
-            self.client.fetch_ads::<A>(&ad_request, &cache_policy)?;
+        let (mut response, request_hash) = self.client.fetch_ads::<A>(ad_request, &cache_policy)?;
         response.add_request_hash_to_callbacks(&request_hash);
         Ok(response)
     }
@@ -222,7 +226,6 @@ pub enum ClientOperationEvent {
 #[cfg(test)]
 mod tests {
     use crate::{
-        client::config::Environment,
         ffi::telemetry::MozAdsTelemetryWrapper,
         test_utils::{
             get_example_happy_image_response, get_example_happy_spoc_response,
@@ -242,6 +245,7 @@ mod tests {
             Box::new(DefaultContextIdCallback),
         );
         AdsClient {
+            environment: Environment::Test,
             context_id_component,
             client,
             telemetry: MozAdsTelemetryWrapper::noop(),
@@ -272,7 +276,7 @@ mod tests {
             .create();
 
         let telemetry = MozAdsTelemetryWrapper::noop();
-        let mars_client = MARSClient::new(Environment::Test, None, telemetry);
+        let mars_client = MARSClient::new(None, telemetry);
         let ads_client = new_with_mars_client(mars_client);
 
         let ad_placement_requests = make_happy_placement_requests();
@@ -294,7 +298,7 @@ mod tests {
             .create();
 
         let telemetry = MozAdsTelemetryWrapper::noop();
-        let mars_client = MARSClient::new(Environment::Test, None, telemetry);
+        let mars_client = MARSClient::new(None, telemetry);
         let ads_client = new_with_mars_client(mars_client);
 
         let ad_placement_requests = make_happy_placement_requests();
@@ -316,7 +320,7 @@ mod tests {
             .create();
 
         let telemetry = MozAdsTelemetryWrapper::noop();
-        let mars_client = MARSClient::new(Environment::Test, None, telemetry.clone());
+        let mars_client = MARSClient::new(None, telemetry.clone());
         let ads_client = new_with_mars_client(mars_client);
 
         let ad_placement_requests = make_happy_placement_requests();
@@ -334,7 +338,7 @@ mod tests {
             .build()
             .unwrap();
         let telemetry = MozAdsTelemetryWrapper::noop();
-        let mars_client = MARSClient::new(Environment::Test, Some(cache), telemetry.clone());
+        let mars_client = MARSClient::new(Some(cache), telemetry.clone());
         let ads_client = new_with_mars_client(mars_client);
 
         let response = get_example_happy_image_response();

components/ads-client/src/client/ad_request.rs

@@ -4,27 +4,51 @@
 */
 
 use std::collections::HashSet;
+use std::hash::{Hash, Hasher};
 
 use serde::{Deserialize, Serialize};
+use url::Url;
+use viaduct::Request;
 
 use crate::error::BuildRequestError;
 
 #[derive(Debug, PartialEq, Serialize)]
 pub struct AdRequest {
     pub context_id: String,
     pub placements: Vec<AdPlacementRequest>,
+    /// Skipped to exclude from the request body
+    #[serde(skip)]
+    pub url: Url,
+}
+
+/// Hash implementation intentionally excludes `context_id` as it rotates
+/// on client re-instantiation and should not invalidate cached responses.
+impl Hash for AdRequest {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        self.url.as_str().hash(state);
+        self.placements.hash(state);
+    }
+}
+
+impl From<AdRequest> for Request {
+    fn from(ad_request: AdRequest) -> Self {
+        let url = ad_request.url.clone();
+        Request::post(url).json(&ad_request)
+    }
 }
 
 impl AdRequest {
-    pub fn build(
+    pub fn try_new(
         context_id: String,
         placements: Vec<AdPlacementRequest>,
+        url: Url,
     ) -> Result<Self, BuildRequestError> {
         if placements.is_empty() {
             return Err(BuildRequestError::EmptyConfig);
         };
 
         let mut request = AdRequest {
+            url,
             placements: vec![],
             context_id,
         };
@@ -56,20 +80,20 @@ impl AdRequest {
     }
 }
 
-#[derive(Debug, PartialEq, Serialize)]
+#[derive(Debug, Hash, PartialEq, Serialize)]
 pub struct AdPlacementRequest {
     pub placement: String,
     pub count: u32,
     pub content: Option<AdContentCategory>,
 }
 
-#[derive(Debug, Deserialize, PartialEq, Serialize)]
+#[derive(Debug, Deserialize, Hash, PartialEq, Serialize)]
 pub struct AdContentCategory {
     pub taxonomy: IABContentTaxonomy,
     pub categories: Vec<String>,
 }
 
-#[derive(Clone, Copy, Debug, Deserialize, PartialEq, Serialize)]
+#[derive(Debug, Deserialize, Hash, PartialEq, Serialize)]
 pub enum IABContentTaxonomy {
     #[serde(rename = "IAB-1.0")]
     IAB1_0,
@@ -142,7 +166,8 @@ mod tests {
 
     #[test]
     fn test_build_ad_request_happy() {
-        let request = AdRequest::build(
+        let url: Url = "https://example.com/ads".parse().unwrap();
+        let request = AdRequest::try_new(
             TEST_CONTEXT_ID.to_string(),
             vec![
                 AdPlacementRequest {
@@ -162,10 +187,12 @@ mod tests {
                     }),
                 },
             ],
+            url.clone(),
         )
         .unwrap();
 
         let expected_request = AdRequest {
+            url,
             context_id: TEST_CONTEXT_ID.to_string(),
             placements: vec![
                 AdPlacementRequest {
@@ -192,7 +219,8 @@ mod tests {
 
     #[test]
     fn test_build_ad_request_fails_on_duplicate_placement_id() {
-        let request = AdRequest::build(
+        let url: Url = "https://example.com/ads".parse().unwrap();
+        let request = AdRequest::try_new(
             TEST_CONTEXT_ID.to_string(),
             vec![
                 AdPlacementRequest {
@@ -212,13 +240,68 @@ mod tests {
                     }),
                 },
             ],
+            url,
         );
         assert!(request.is_err());
     }
 
     #[test]
     fn test_build_ad_request_fails_on_empty_request() {
-        let request = AdRequest::build(TEST_CONTEXT_ID.to_string(), vec![]);
+        let url: Url = "https://example.com/ads".parse().unwrap();
+        let request = AdRequest::try_new(TEST_CONTEXT_ID.to_string(), vec![], url);
         assert!(request.is_err());
     }
+
+    #[test]
+    fn test_context_id_ignored_in_hash() {
+        use crate::http_cache::RequestHash;
+
+        let url: Url = "https://example.com/ads".parse().unwrap();
+        let make_placements = || {
+            vec![AdPlacementRequest {
+                placement: "tile_1".to_string(),
+                count: 1,
+                content: None,
+            }]
+        };
+
+        let context_id_a = "aaaa-bbbb-cccc".to_string();
+        let context_id_b = "dddd-eeee-ffff".to_string();
+
+        let req1 = AdRequest::try_new(context_id_a, make_placements(), url.clone()).unwrap();
+        let req2 = AdRequest::try_new(context_id_b, make_placements(), url).unwrap();
+
+        assert_eq!(RequestHash::new(&req1), RequestHash::new(&req2));
+    }
+
+    #[test]
+    fn test_different_placements_produce_different_hash() {
+        use crate::http_cache::RequestHash;
+
+        let url: Url = "https://example.com/ads".parse().unwrap();
+
+        let req1 = AdRequest::try_new(
+            "same-id".to_string(),
+            vec![AdPlacementRequest {
+                placement: "tile_1".to_string(),
+                count: 1,
+                content: None,
+            }],
+            url.clone(),
+        )
+        .unwrap();
+
+        let req2 = AdRequest::try_new(
+            "same-id".to_string(),
+            vec![AdPlacementRequest {
+                placement: "tile_2".to_string(),
+                count: 3,
+                content: None,
+            }],
+            url,
+        )
+        .unwrap();
+
+        assert_ne!(RequestHash::new(&req1), RequestHash::new(&req2));
+    }
 }

components/ads-client/src/client/config.rs

@@ -33,14 +33,25 @@ pub enum Environment {
 }
 
 impl Environment {
-    pub fn into_mars_url(self) -> Url {
+    fn base_url(self) -> Url {
         match self {
             Environment::Prod => MARS_API_ENDPOINT_PROD.clone(),
             Environment::Staging => MARS_API_ENDPOINT_STAGING.clone(),
             #[cfg(test)]
             Environment::Test => Url::parse(&mockito::server_url()).unwrap(),
         }
     }
+
+    pub fn into_url(self, path: &str) -> Url {
+        let mut base = self.base_url();
+        // Ensure the path has a trailing slash so that `join` appends
+        // rather than replacing the last segment.
+        if !base.path().ends_with('/') {
+            base.set_path(&format!("{}/", base.path()));
+        }
+        base.join(path)
+            .expect("joining a path to a valid base URL must succeed")
+    }
 }
 
 #[derive(Clone, Debug)]
@@ -58,15 +69,12 @@ mod tests {
 
     #[test]
     fn prod_endpoint_parses_and_is_expected() {
-        let url = Environment::Prod.into_mars_url();
+        let url = Environment::Prod.into_url("ads");
 
-        assert_eq!(url.as_str(), "https://ads.mozilla.org/v1/");
+        assert_eq!(url.as_str(), "https://ads.mozilla.org/v1/ads");
 
         assert_eq!(url.scheme(), "https");
         assert_eq!(url.host(), Some(Host::Domain("ads.mozilla.org")));
-        assert_eq!(url.path(), "/v1/");
-
-        let url2 = Environment::Prod.into_mars_url();
-        assert!(url == url2);
+        assert_eq!(url.path(), "/v1/ads");
     }
 }