From 7c6fbae475dd1276c0946eead468f07b811f3cb8 Mon Sep 17 00:00:00 2001 From: Hansheng Zhao Date: Wed, 13 May 2026 12:28:57 -0400 Subject: [PATCH 1/3] refactor(ads-client): changed AdRequest.url to be AdRequest.environment instead for the cache hashing to be more stable. --- components/ads-client/src/mars.rs | 3 +- components/ads-client/src/mars/ad_request.rs | 81 +++++++++++-------- components/ads-client/src/mars/environment.rs | 2 +- 3 files changed, 49 insertions(+), 37 deletions(-) diff --git a/components/ads-client/src/mars.rs b/components/ads-client/src/mars.rs index 798a01e054..37233ac40f 100644 --- a/components/ads-client/src/mars.rs +++ b/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 { diff --git a/components/ads-client/src/mars/ad_request.rs b/components/ads-client/src/mars/ad_request.rs index 34953a7c15..679765d12c 100644 --- a/components/ads-client/src/mars/ad_request.rs +++ b/components/ads-client/src/mars/ad_request.rs @@ -7,38 +7,42 @@ 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, - /// 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(&self, state: &mut H) { - self.url.as_str().hash(state); - self.placements.hash(state); + self.environment.hash(state); self.ohttp.hash(state); + self.placements.hash(state); } } impl From 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,9 +52,9 @@ impl From for Request { impl AdRequest { pub fn try_new( context_id: String, - placements: Vec, - url: Url, + environment: Environment, ohttp: bool, + placements: Vec, ) -> Result { if placements.is_empty() { return Err(BuildRequestError::EmptyConfig); @@ -58,10 +62,10 @@ impl AdRequest { let mut request = AdRequest { context_id, + environment, headers: Headers::new(), ohttp, placements: vec![], - url, }; let mut used_placement_ids: HashSet = HashSet::new(); @@ -177,9 +181,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 +203,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,7 +229,6 @@ mod tests { placement: "example_placement_2".to_string(), }, ], - url, }; assert_eq!(request, expected_request); @@ -233,9 +236,10 @@ mod tests { #[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,16 +258,18 @@ 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()); } @@ -271,7 +277,6 @@ mod tests { 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 +288,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 +300,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 +331,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,11 +339,20 @@ 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)); } diff --git a/components/ads-client/src/mars/environment.rs b/components/ads-client/src/mars/environment.rs index 37ed91fdd1..537238b65f 100644 --- a/components/ads-client/src/mars/environment.rs +++ b/components/ads-client/src/mars/environment.rs @@ -11,7 +11,7 @@ static MARS_API_ENDPOINT_PROD: Lazy = Lazy::new(|| url!("https://ads.mozill static MARS_API_ENDPOINT_STAGING: Lazy = 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, From 1fb65d09d81ac20ae1b4a510c838bf0327b60791 Mon Sep 17 00:00:00 2001 From: Hansheng Zhao Date: Fri, 15 May 2026 13:02:50 -0500 Subject: [PATCH 2/3] refactor(ads-client): changed the environment.into_url to use the Url crate's path_segments_mut to build URL instead of string concat. --- components/ads-client/src/mars/environment.rs | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/components/ads-client/src/mars/environment.rs b/components/ads-client/src/mars/environment.rs index 537238b65f..65edf6a52a 100644 --- a/components/ads-client/src/mars/environment.rs +++ b/components/ads-client/src/mars/environment.rs @@ -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"), + ); + } } From 8d219e24f22962bd1596ec192b1ea685d6d7e01b Mon Sep 17 00:00:00 2001 From: Hansheng Zhao Date: Fri, 15 May 2026 13:04:43 -0500 Subject: [PATCH 3/3] refactor(ads-client): changed the ENDPOINT pattern, ensured that it contributes to the caching. --- components/ads-client/src/mars/ad_request.rs | 34 +++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/components/ads-client/src/mars/ad_request.rs b/components/ads-client/src/mars/ad_request.rs index 679765d12c..105ad11ef4 100644 --- a/components/ads-client/src/mars/ad_request.rs +++ b/components/ads-client/src/mars/ad_request.rs @@ -13,7 +13,7 @@ use crate::mars::Environment; use super::error::BuildRequestError; -const ENDPOINT: &str = "ads"; +const ENDPOINT: &str = "/ads"; #[derive(Debug, PartialEq, Serialize)] pub struct AdRequest { @@ -34,6 +34,7 @@ pub struct AdRequest { /// If response shape ever varies, add a version to this hash for variant tracking. impl Hash for AdRequest { fn hash(&self, state: &mut H) { + ENDPOINT.hash(state); self.environment.hash(state); self.ohttp.hash(state); self.placements.hash(state); @@ -356,4 +357,35 @@ mod tests { 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", + ); + } }