refactor(ads-client): move a constructor onto its type and retire the common drawer

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};

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;
 

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;

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),

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);
         // }
 

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};

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;

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<Self> {
+        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);
+    }
 }

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),

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;

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();
 

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<A: AdResponseValue> {
@@ -83,29 +82,6 @@ impl<A: AdResponseValue> AdResponse<A> {
     }
 }
 
-// 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<RequestHash> {
-    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<String> {
@@ -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);
-    }
 }