refactor(ads-client): changed AdRequest.url to be environment for a more stable hashing

components/ads-client/src/mars.rs

@@ -67,8 +67,7 @@ where
     where
         A: AdResponseValue,
     {
-        let url = self.environment.into_url("ads");
-        let mut ad_request = AdRequest::try_new(context_id, placements, url, ohttp)?;
+        let mut ad_request = AdRequest::try_new(context_id, self.environment, ohttp, placements)?;
         let request_hash = RequestHash::new(&ad_request);
 
         if ohttp {

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

@@ -7,38 +7,43 @@ use std::collections::HashSet;
 use std::hash::{Hash, Hasher};
 
 use serde::{Deserialize, Serialize};
-use url::Url;
 use viaduct::{Headers, Request};
 
+use crate::mars::Environment;
+
 use super::error::BuildRequestError;
 
+const ENDPOINT: &str = "/ads";
+
 #[derive(Debug, PartialEq, Serialize)]
 pub struct AdRequest {
     pub context_id: String,
+    /// Skipped to exclude from the request body
+    #[serde(skip)]
+    pub environment: Environment,
     #[serde(skip)]
     pub headers: Headers,
     #[serde(skip)]
     pub ohttp: bool,
     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.
 /// `headers` are also excluded as they are request metadata, not cache keys.
+/// If response shape ever varies, add a version to this hash for variant tracking.
 impl Hash for AdRequest {
     fn hash<H: Hasher>(&self, state: &mut H) {
-        self.url.as_str().hash(state);
-        self.placements.hash(state);
+        ENDPOINT.hash(state);
+        self.environment.hash(state);
         self.ohttp.hash(state);
+        self.placements.hash(state);
     }
 }
 
 impl From<AdRequest> for Request {
     fn from(ad_request: AdRequest) -> Self {
-        let url = ad_request.url.clone();
+        let url = ad_request.environment.into_url(ENDPOINT);
         let mut request = Request::post(url).json(&ad_request);
         request.headers.extend(ad_request.headers);
         request
@@ -48,20 +53,20 @@ impl From<AdRequest> for Request {
 impl AdRequest {
     pub fn try_new(
         context_id: String,
-        placements: Vec<AdPlacementRequest>,
-        url: Url,
+        environment: Environment,
         ohttp: bool,
+        placements: Vec<AdPlacementRequest>,
     ) -> Result<Self, BuildRequestError> {
         if placements.is_empty() {
             return Err(BuildRequestError::EmptyConfig);
         };
 
         let mut request = AdRequest {
             context_id,
+            environment,
             headers: Headers::new(),
             ohttp,
             placements: vec![],
-            url,
         };
 
         let mut used_placement_ids: HashSet<String> = HashSet::new();
@@ -177,9 +182,10 @@ mod tests {
 
     #[test]
     fn test_build_ad_request_happy() {
-        let url: Url = "https://example.com/ads".parse().unwrap();
         let request = AdRequest::try_new(
             TEST_CONTEXT_ID.to_string(),
+            Environment::Test,
+            false,
             vec![
                 AdPlacementRequest {
                     content: Some(AdContentCategory {
@@ -198,13 +204,12 @@ mod tests {
                     placement: "example_placement_2".to_string(),
                 },
             ],
-            url.clone(),
-            false,
         )
         .unwrap();
 
         let expected_request = AdRequest {
             context_id: TEST_CONTEXT_ID.to_string(),
+            environment: Environment::Test,
             headers: Headers::new(),
             ohttp: false,
             placements: vec![
@@ -225,17 +230,17 @@ mod tests {
                     placement: "example_placement_2".to_string(),
                 },
             ],
-            url,
         };
 
         assert_eq!(request, expected_request);
     }
 
     #[test]
     fn test_build_ad_request_fails_on_duplicate_placement_id() {
-        let url: Url = "https://example.com/ads".parse().unwrap();
         let request = AdRequest::try_new(
             TEST_CONTEXT_ID.to_string(),
+            Environment::Test,
+            false,
             vec![
                 AdPlacementRequest {
                     content: Some(AdContentCategory {
@@ -254,24 +259,25 @@ mod tests {
                     placement: "example_placement_1".to_string(),
                 },
             ],
-            url,
-            false,
         );
         assert!(request.is_err());
     }
 
     #[test]
     fn test_build_ad_request_fails_on_empty_request() {
-        let url: Url = "https://example.com/ads".parse().unwrap();
-        let request = AdRequest::try_new(TEST_CONTEXT_ID.to_string(), vec![], url, false);
+        let request = AdRequest::try_new(
+            TEST_CONTEXT_ID.to_string(),
+            Environment::Test,
+            false,
+            vec![],
+        );
         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 {
                 content: None,
@@ -283,8 +289,10 @@ mod tests {
         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(), false).unwrap();
-        let req2 = AdRequest::try_new(context_id_b, make_placements(), url, false).unwrap();
+        let req1 =
+            AdRequest::try_new(context_id_a, Environment::Test, false, make_placements()).unwrap();
+        let req2 =
+            AdRequest::try_new(context_id_b, Environment::Test, false, make_placements()).unwrap();
 
         assert_eq!(RequestHash::new(&req1), RequestHash::new(&req2));
     }
@@ -293,29 +301,27 @@ mod tests {
     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(),
+            Environment::Test,
+            false,
             vec![AdPlacementRequest {
                 content: None,
                 count: 1,
                 placement: "tile_1".to_string(),
             }],
-            url.clone(),
-            false,
         )
         .unwrap();
 
         let req2 = AdRequest::try_new(
             "same-id".to_string(),
+            Environment::Test,
+            false,
             vec![AdPlacementRequest {
                 content: None,
                 count: 3,
                 placement: "tile_2".to_string(),
             }],
-            url,
-            false,
         )
         .unwrap();
 
@@ -326,7 +332,6 @@ mod tests {
     fn test_ohttp_flag_produces_different_hash() {
         use crate::http_cache::RequestHash;
 
-        let url: Url = "https://example.com/ads".parse().unwrap();
         let make_placements = || {
             vec![AdPlacementRequest {
                 content: None,
@@ -335,12 +340,52 @@ mod tests {
             }]
         };
 
-        let req_direct =
-            AdRequest::try_new("same-id".to_string(), make_placements(), url.clone(), false)
-                .unwrap();
-        let req_ohttp =
-            AdRequest::try_new("same-id".to_string(), make_placements(), url, true).unwrap();
+        let req_direct = AdRequest::try_new(
+            "same-id".to_string(),
+            Environment::Test,
+            false,
+            make_placements(),
+        )
+        .unwrap();
+        let req_ohttp = AdRequest::try_new(
+            "same-id".to_string(),
+            Environment::Test,
+            true,
+            make_placements(),
+        )
+        .unwrap();
 
         assert_ne!(RequestHash::new(&req_direct), RequestHash::new(&req_ohttp));
     }
+
+    #[test]
+    fn test_endpoint_const_participates_in_hash() {
+        use std::collections::hash_map::DefaultHasher;
+
+        let request = AdRequest::try_new(
+            TEST_CONTEXT_ID.to_string(),
+            Environment::Test,
+            false,
+            vec![AdPlacementRequest {
+                content: None,
+                count: 1,
+                placement: "tile_1".to_string(),
+            }],
+        )
+        .unwrap();
+
+        let mut full = DefaultHasher::new();
+        request.hash(&mut full);
+
+        let mut without_endpoint = DefaultHasher::new();
+        request.environment.hash(&mut without_endpoint);
+        request.ohttp.hash(&mut without_endpoint);
+        request.placements.hash(&mut without_endpoint);
+
+        assert_ne!(
+            full.finish(),
+            without_endpoint.finish(),
+            "ENDPOINT must contribute to AdRequest hash",
+        );
+    }
 }

components/ads-client/src/mars/environment.rs

@@ -11,7 +11,7 @@ static MARS_API_ENDPOINT_PROD: Lazy<Url> = Lazy::new(|| url!("https://ads.mozill
 
 static MARS_API_ENDPOINT_STAGING: Lazy<Url> = Lazy::new(|| url!("https://ads.allizom.org/v1/"));
 
-#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)]
+#[derive(Clone, Copy, Debug, Default, Eq, Hash, PartialEq)]
 pub enum Environment {
     #[default]
     Prod,
@@ -22,14 +22,12 @@ pub enum Environment {
 
 impl Environment {
     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")
+        let mut url = self.base_url();
+        url.path_segments_mut()
+            .expect("base URL must be hierarchical")
+            .pop_if_empty()
+            .extend(path.split('/').filter(|segment| !segment.is_empty()));
+        url
     }
 
     fn base_url(self) -> Url {
@@ -69,4 +67,16 @@ mod tests {
         assert_eq!(url.host(), Some(Host::Domain("ads.allizom.org")));
         assert_eq!(url.path(), "/v1/ads");
     }
+
+    #[test]
+    fn into_url_normalizes_slash() {
+        assert_eq!(
+            Environment::Prod.into_url("/ads"),
+            Environment::Prod.into_url("ads"),
+        );
+        assert_eq!(
+            Environment::Prod.into_url("//ads/with/extra//slashes//"),
+            Environment::Prod.into_url("ads/with/extra/slashes"),
+        );
+    }
 }