From 898055bc84db9bed0794635400e8d45e68c114c1 Mon Sep 17 00:00:00 2001 From: ahanot Date: Mon, 21 Sep 2026 20:12:47 -0400 Subject: [PATCH] refactor(ads-client): move pop_request_hash_from_url onto RequestHash and drop common Move pop_request_hash_from_url to RequestHash::pop_from_url, and move bytesize and clock out of common to the crate root. No behaviour change. --- .../integration-tests/tests/http_cache.rs | 2 +- components/ads-client/src/ads_store.rs | 2 +- .../ads-client/src/ads_store/builder.rs | 2 +- components/ads-client/src/ads_store/store.rs | 8 ++-- .../ads-client/src/{common => }/bytesize.rs | 0 components/ads-client/src/client.rs | 6 +-- .../ads-client/src/{common => }/clock.rs | 0 components/ads-client/src/common.rs | 2 - components/ads-client/src/http_cache.rs | 2 +- .../ads-client/src/http_cache/builder.rs | 2 +- .../ads-client/src/http_cache/request_hash.rs | 47 +++++++++++++++++++ components/ads-client/src/http_cache/store.rs | 4 +- components/ads-client/src/lib.rs | 3 +- components/ads-client/src/mars.rs | 6 +-- components/ads-client/src/mars/ad_response.rs | 45 +----------------- 15 files changed, 67 insertions(+), 64 deletions(-) rename components/ads-client/src/{common => }/bytesize.rs (100%) rename components/ads-client/src/{common => }/clock.rs (100%) delete mode 100644 components/ads-client/src/common.rs diff --git a/components/ads-client/integration-tests/tests/http_cache.rs b/components/ads-client/integration-tests/tests/http_cache.rs index 3bf91487405..9adcccf8ffc 100644 --- a/components/ads-client/integration-tests/tests/http_cache.rs +++ b/components/ads-client/integration-tests/tests/http_cache.rs @@ -6,7 +6,7 @@ use std::hash::{Hash, Hasher}; use std::time::Duration; -use ads_client::common::bytesize::ByteSize; +use ads_client::bytesize::ByteSize; use ads_client::http_cache::{CacheOutcome, CachePolicy, HttpCache}; use mockito::mock; use viaduct::{Client, ClientSettings, Request}; diff --git a/components/ads-client/src/ads_store.rs b/components/ads-client/src/ads_store.rs index b2fbf0d1dc9..265cee9b502 100644 --- a/components/ads-client/src/ads_store.rs +++ b/components/ads-client/src/ads_store.rs @@ -5,7 +5,7 @@ pub mod store; use crate::{ ads::PlacementId, ads_store::{builder::AdsStoreBuilder, store::AdsStoreHolder}, - common::bytesize::ByteSize, + bytesize::ByteSize, }; use std::path::Path; diff --git a/components/ads-client/src/ads_store/builder.rs b/components/ads-client/src/ads_store/builder.rs index fd925f24915..3b2ef436cc1 100644 --- a/components/ads-client/src/ads_store/builder.rs +++ b/components/ads-client/src/ads_store/builder.rs @@ -5,7 +5,7 @@ use super::connection_initializer::AdsStoreConnectionInitializer; use crate::ads_store::store::AdsStoreHolder; use crate::ads_store::AdsStore; -use crate::common::bytesize::ByteSize; +use crate::bytesize::ByteSize; use crate::telemetry::Telemetry; use rusqlite::Connection; use sql_support::open_database; diff --git a/components/ads-client/src/ads_store/store.rs b/components/ads-client/src/ads_store/store.rs index 54b397593c3..b3f880e1a46 100644 --- a/components/ads-client/src/ads_store/store.rs +++ b/components/ads-client/src/ads_store/store.rs @@ -3,10 +3,10 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ use crate::ads::Ads; -use crate::common::bytesize::ByteSize; -use crate::common::clock::Clock; +use crate::bytesize::ByteSize; +use crate::clock::Clock; use crate::mars::error::FetchAdsError; -use crate::{ads::PlacementId, common::clock::CacheClock}; +use crate::{ads::PlacementId, clock::CacheClock}; use parking_lot::Mutex; use rusqlite::{params, Connection, OptionalExtension, Result as SqliteResult}; use std::sync::Arc; @@ -44,7 +44,7 @@ impl AdsStoreHolder { #[cfg(test)] pub fn new_with_test_clock(conn: Connection) -> Self { - use crate::common::clock::TestClock; + use crate::clock::TestClock; Self { conn: Mutex::new(conn), diff --git a/components/ads-client/src/common/bytesize.rs b/components/ads-client/src/bytesize.rs similarity index 100% rename from components/ads-client/src/common/bytesize.rs rename to components/ads-client/src/bytesize.rs diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index 71cef6314e5..7a600064bd9 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -6,7 +6,7 @@ use crate::ads::{AdImage, AdSpoc, AdTile}; #[cfg(feature = "stateful")] use crate::ads_store::AdsStore; -use crate::common::bytesize::ByteSize; +use crate::bytesize::ByteSize; use crate::http_cache::{CachePolicy, HttpCache}; use crate::mars::ad_request::{AdPlacementRequest, AdRequestFlags}; use crate::mars::ad_response::{AdResponse, AdResponseValue}; @@ -119,7 +119,7 @@ where // TODO: Re-enable cache invalidation behind a Nimbus experiment. // The mobile team has requested this be temporarily disabled. // let mut click_url = click_url.clone(); - // if let Some(request_hash) = pop_request_hash_from_url(&mut click_url) { + // if let Some(request_hash) = RequestHash::pop_from_url(&mut click_url) { // let _ = self.client.invalidate_cache_by_hash(&request_hash); // } self.client @@ -140,7 +140,7 @@ where // TODO: Re-enable cache invalidation behind a Nimbus experiment. // The mobile team has requested this be temporarily disabled. // let mut impression_url = impression_url.clone(); - // if let Some(request_hash) = pop_request_hash_from_url(&mut impression_url) { + // if let Some(request_hash) = RequestHash::pop_from_url(&mut impression_url) { // let _ = self.client.invalidate_cache_by_hash(&request_hash); // } diff --git a/components/ads-client/src/common/clock.rs b/components/ads-client/src/clock.rs similarity index 100% rename from components/ads-client/src/common/clock.rs rename to components/ads-client/src/clock.rs diff --git a/components/ads-client/src/common.rs b/components/ads-client/src/common.rs deleted file mode 100644 index 9bf27276888..00000000000 --- a/components/ads-client/src/common.rs +++ /dev/null @@ -1,2 +0,0 @@ -pub mod bytesize; -pub mod clock; diff --git a/components/ads-client/src/http_cache.rs b/components/ads-client/src/http_cache.rs index f7dfacd5d3b..c971985ead5 100644 --- a/components/ads-client/src/http_cache.rs +++ b/components/ads-client/src/http_cache.rs @@ -16,7 +16,7 @@ use self::{ store::HttpCacheStore, strategy::{CacheFirst, NetworkFirst}, }; -use crate::common::bytesize::ByteSize; +use crate::bytesize::ByteSize; use std::hash::Hash; use viaduct::{Client, Request, Response}; diff --git a/components/ads-client/src/http_cache/builder.rs b/components/ads-client/src/http_cache/builder.rs index 76458aec06d..f48b92580aa 100644 --- a/components/ads-client/src/http_cache/builder.rs +++ b/components/ads-client/src/http_cache/builder.rs @@ -4,7 +4,7 @@ use super::connection_initializer::HttpCacheConnectionInitializer; use super::store::HttpCacheStore; -use crate::common::bytesize::ByteSize; +use crate::bytesize::ByteSize; use crate::http_cache::HttpCache; use rusqlite::Connection; use sql_support::open_database; diff --git a/components/ads-client/src/http_cache/request_hash.rs b/components/ads-client/src/http_cache/request_hash.rs index 927e85fc6f8..9d31e5aac0e 100644 --- a/components/ads-client/src/http_cache/request_hash.rs +++ b/components/ads-client/src/http_cache/request_hash.rs @@ -5,6 +5,8 @@ use std::collections::hash_map::DefaultHasher; use std::hash::{Hash, Hasher}; +use url::Url; + #[derive(Clone, Debug, PartialEq)] pub struct RequestHash(String); @@ -14,6 +16,31 @@ impl RequestHash { value.hash(&mut hasher); RequestHash(format!("{:x}", hasher.finish())) } + + /// Takes the `request_hash` query parameter out of `url`, leaving the other + /// parameters in place. + // TODO: Remove this allow(dead_code) when cache invalidation is re-enabled behind Nimbus experiment + #[allow(dead_code)] + pub fn pop_from_url(url: &mut Url) -> Option { + let mut request_hash = None; + let mut query = url::form_urlencoded::Serializer::new(String::new()); + + for (key, value) in url.query_pairs() { + if key == "request_hash" { + request_hash = Some(RequestHash::from(value.as_ref())); + } else { + query.append_pair(&key, &value); + } + } + + let query_string = query.finish(); + if query_string.is_empty() { + url.set_query(None); + } else { + url.set_query(Some(&query_string)); + } + request_hash + } } impl From<&str> for RequestHash { @@ -62,4 +89,24 @@ mod tests { let hash2 = RequestHash::from(hash_string); assert_eq!(hash2.to_string(), "xyz789"); } + + #[test] + fn pop_from_url_takes_the_hash_and_keeps_other_params() { + let mut url_with_hash = + Url::parse("https://example.com/callback?request_hash=abc123def456&other=param") + .unwrap(); + let extracted = RequestHash::pop_from_url(&mut url_with_hash); + assert_eq!(extracted, Some(RequestHash::from("abc123def456"))); + assert_eq!(url_with_hash.query(), Some("other=param")); + + let mut url_without_hash = Url::parse("https://example.com/callback?other=param").unwrap(); + let extracted_none = RequestHash::pop_from_url(&mut url_without_hash); + assert_eq!(extracted_none, None); + assert_eq!(url_without_hash.query(), Some("other=param")); + + let mut url_no_query = Url::parse("https://example.com/callback").unwrap(); + let extracted_empty = RequestHash::pop_from_url(&mut url_no_query); + assert_eq!(extracted_empty, None); + assert_eq!(url_no_query.query(), None); + } } diff --git a/components/ads-client/src/http_cache/store.rs b/components/ads-client/src/http_cache/store.rs index bc9288d5320..1a13bae4c6f 100644 --- a/components/ads-client/src/http_cache/store.rs +++ b/components/ads-client/src/http_cache/store.rs @@ -5,7 +5,7 @@ use std::{collections::HashMap, sync::Arc, time::Duration}; use crate::{ - common::clock::{CacheClock, Clock}, + clock::{CacheClock, Clock}, http_cache::{request_hash::RequestHash, ByteSize}, }; use parking_lot::Mutex; @@ -46,7 +46,7 @@ impl HttpCacheStore { #[cfg(test)] pub fn new_with_test_clock(conn: Connection) -> Self { - use crate::common::clock::TestClock; + use crate::clock::TestClock; Self { conn: Mutex::new(conn), diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 56aafd849a6..bf51739038e 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -17,8 +17,9 @@ use mars::ad_request::{AdPlacementRequest, AdRequestFlags}; mod ads; #[cfg(feature = "stateful")] pub mod ads_store; +pub mod bytesize; mod client; -pub mod common; +pub mod clock; mod ffi; pub mod http_cache; mod mars; diff --git a/components/ads-client/src/mars.rs b/components/ads-client/src/mars.rs index 58e69887ddb..5c3e72af7ad 100644 --- a/components/ads-client/src/mars.rs +++ b/components/ads-client/src/mars.rs @@ -356,7 +356,7 @@ mod tests { let cache = HttpCache::builder("test_fetch_ads_cache_hit_skips_network.db") .default_ttl(std::time::Duration::from_secs(300)) - .max_size(crate::common::bytesize::ByteSize::mib(1)) + .max_size(crate::bytesize::ByteSize::mib(1)) .build() .unwrap(); let client = make_test_client(Some(cache)); @@ -394,7 +394,7 @@ mod tests { viaduct_dev::init_backend_dev(); let cache = HttpCache::builder("test_record_click.db") .default_ttl(std::time::Duration::from_secs(300)) - .max_size(crate::common::bytesize::ByteSize::mib(1)) + .max_size(crate::bytesize::ByteSize::mib(1)) .build() .unwrap(); @@ -413,7 +413,7 @@ mod tests { viaduct_dev::init_backend_dev(); let cache = HttpCache::builder("test_record_impression.db") .default_ttl(std::time::Duration::from_secs(300)) - .max_size(crate::common::bytesize::ByteSize::mib(1)) + .max_size(crate::bytesize::ByteSize::mib(1)) .build() .unwrap(); diff --git a/components/ads-client/src/mars/ad_response.rs b/components/ads-client/src/mars/ad_response.rs index 87acc3b451f..b6798d503cd 100644 --- a/components/ads-client/src/mars/ad_response.rs +++ b/components/ads-client/src/mars/ad_response.rs @@ -9,7 +9,6 @@ use crate::telemetry::Telemetry; use serde::de::DeserializeOwned; use serde::Serialize; use std::collections::HashMap; -use url::Url; #[derive(Debug, PartialEq, Serialize)] pub struct AdResponse { @@ -83,29 +82,6 @@ impl AdResponse { } } -// TODO: Remove this allow(dead_code) when cache invalidation is re-enabled behind Nimbus experiment -#[allow(dead_code)] -pub fn pop_request_hash_from_url(url: &mut Url) -> Option { - let mut request_hash = None; - let mut query = url::form_urlencoded::Serializer::new(String::new()); - - for (key, value) in url.query_pairs() { - if key == "request_hash" { - request_hash = Some(RequestHash::from(value.as_ref())); - } else { - query.append_pair(&key, &value); - } - } - - let query_string = query.finish(); - if query_string.is_empty() { - url.set_query(None); - } else { - url.set_query(Some(&query_string)); - } - request_hash -} - pub trait AdResponseValue: DeserializeOwned { fn callbacks_mut(&mut self) -> &mut AdCallbacks; fn cap_key(&self) -> Option { @@ -142,6 +118,7 @@ mod tests { use super::*; use serde_json::{from_str, json}; + use url::Url; use url_macro::url; #[test] @@ -609,24 +586,4 @@ mod tests { .unwrap_or("") .contains("request_hash=abc123def456")); } - - #[test] - fn test_pop_request_hash_from_url() { - let mut url_with_hash = - Url::parse("https://example.com/callback?request_hash=abc123def456&other=param") - .unwrap(); - let extracted = pop_request_hash_from_url(&mut url_with_hash); - assert_eq!(extracted, Some(RequestHash::from("abc123def456"))); - assert_eq!(url_with_hash.query(), Some("other=param")); - - let mut url_without_hash = Url::parse("https://example.com/callback?other=param").unwrap(); - let extracted_none = pop_request_hash_from_url(&mut url_without_hash); - assert_eq!(extracted_none, None); - assert_eq!(url_without_hash.query(), Some("other=param")); - - let mut url_no_query = Url::parse("https://example.com/callback").unwrap(); - let extracted_empty = pop_request_hash_from_url(&mut url_no_query); - assert_eq!(extracted_empty, None); - assert_eq!(url_no_query.query(), None); - } }