Refactor ads-client to use Url type for callback URLs (#7038)

* refactor(ads-client): deserialize callbacks response as Url

* test(ads-client): click and impression callback urls

Author: Almaju

CHANGELOG.md

@@ -34,6 +34,7 @@
 - The `context_id` is now generated and rotated via the existing eponym component crate.
 - Added request caching mechanism using SQLite with configurable TTL and max size.
 - Added configuration options for the cache.
+- Deserialize callbacks with `url::Url`
 
 ### Relay
 - **⚠️ Breaking Change:** The error handling for the Relay component has been refactored for stronger forward compatibility and more transparent error reporting in Swift and Kotlin via UniFFI.

components/ads-client/Cargo.toml

@@ -24,7 +24,7 @@ serde_json = "1"
 thiserror = "2"
 once_cell = "1.5"
 uniffi = { version = "0.29.0" }
-url = "2"
+url = { version = "2", features = ["serde"] }
 uuid = { version = "1.3", features = ["v4"] }
 viaduct = { path = "../viaduct" }
 sql-support = { path = "../support/sql" }

components/ads-client/src/error.rs

@@ -109,20 +109,17 @@ pub enum EmitTelemetryError {
 
 #[derive(Debug, thiserror::Error)]
 pub enum CallbackRequestError {
-    #[error("URL parse error: {0}")]
-    UrlParse(#[from] url::ParseError),
-
-    #[error("Error sending request: {0}")]
-    Request(#[from] viaduct::ViaductError),
+    #[error("Could not fetch ads, MARS responded with: {0}")]
+    HTTPError(#[from] HTTPError),
 
     #[error("JSON error: {0}")]
     Json(#[from] serde_json::Error),
 
-    #[error("Could not fetch ads, MARS responded with: {0}")]
-    HTTPError(#[from] HTTPError),
-
     #[error("Callback URL missing: {message}")]
     MissingCallback { message: String },
+
+    #[error("Error sending request: {0}")]
+    Request(#[from] viaduct::ViaductError),
 }
 
 #[derive(Debug, thiserror::Error)]

components/ads-client/src/lib.rs

@@ -17,9 +17,10 @@ use instrument::TrackError;
 use mars::{DefaultMARSClient, MARSClient};
 use models::{AdContentCategory, AdRequest, AdResponse, IABContentTaxonomy, MozAd};
 use parking_lot::Mutex;
+use url::Url as AdsClientUrl;
 use uuid::Uuid;
 
-use crate::error::AdsClientApiError;
+use crate::error::{AdsClientApiError, CallbackRequestError};
 use crate::http_cache::{ByteSize, CacheMode, HttpCacheError, RequestCachePolicy};
 
 mod error;
@@ -33,6 +34,12 @@ mod test_utils;
 
 uniffi::setup_scaffolding!("ads_client");
 
+uniffi::custom_type!(AdsClientUrl, String, {
+    remote,
+    try_lift: |val| Ok(AdsClientUrl::parse(&val)?),
+    lower: |obj| obj.as_str().to_string(),
+});
+
 const DEFAULT_TTL_SECONDS: u64 = 300;
 const DEFAULT_MAX_CACHE_SIZE_MIB: u64 = 10;
 
@@ -200,23 +207,28 @@ impl MozAdsClientInner {
     }
 
     fn record_impression(&self, placement: &MozAdsPlacement) -> Result<(), RecordImpressionError> {
-        let impression_callback = placement.content.callbacks.impression.clone();
-
-        self.client.record_impression(impression_callback)?;
-        Ok(())
+        self.client
+            .record_impression(placement.content.callbacks.impression.clone())
     }
 
     fn record_click(&self, placement: &MozAdsPlacement) -> Result<(), RecordClickError> {
-        let click_callback = placement.content.callbacks.click.clone();
-
-        self.client.record_click(click_callback)?;
-        Ok(())
+        self.client
+            .record_click(placement.content.callbacks.click.clone())
     }
 
     fn report_ad(&self, placement: &MozAdsPlacement) -> Result<(), ReportAdError> {
         let report_ad_callback = placement.content.callbacks.report.clone();
 
-        self.client.report_ad(report_ad_callback)?;
+        match report_ad_callback {
+            Some(callback) => self.client.report_ad(callback)?,
+            None => {
+                return Err(ReportAdError::CallbackRequest(
+                    CallbackRequestError::MissingCallback {
+                        message: "Report callback url empty.".to_string(),
+                    },
+                ));
+            }
+        }
         Ok(())
     }
 
@@ -582,11 +594,16 @@ mod tests {
                 block_key: "abc123".into(),
                 alt_text: Some("An ad for a pet dragon".to_string()),
                 callbacks: AdCallbacks {
-                    click: Some("https://ads.fakeexample.org/click/example_ad_2_2".to_string()),
-                    impression: Some(
-                        "https://ads.fakeexample.org/impression/example_ad_2_2".to_string(),
+                    click: AdsClientUrl::parse("https://ads.fakeexample.org/click/example_ad_2_2")
+                        .unwrap(),
+                    impression: AdsClientUrl::parse(
+                        "https://ads.fakeexample.org/impression/example_ad_2_2",
+                    )
+                    .unwrap(),
+                    report: Some(
+                        AdsClientUrl::parse("https://ads.fakeexample.org/report/example_ad_2_2")
+                            .unwrap(),
                     ),
-                    report: Some("https://ads.fakeexample.org/report/example_ad_2_2".to_string()),
                 },
             });
 

components/ads-client/src/mars.rs

@@ -41,12 +41,9 @@ pub trait MARSClient: Sync + Send {
         request: &AdRequest,
         cache_policy: &RequestCachePolicy,
     ) -> Result<AdResponse, FetchAdsError>;
-    fn record_impression(
-        &self,
-        url_callback_string: Option<String>,
-    ) -> Result<(), RecordImpressionError>;
-    fn record_click(&self, url_callback_string: Option<String>) -> Result<(), RecordClickError>;
-    fn report_ad(&self, url_callback_string: Option<String>) -> Result<(), ReportAdError>;
+    fn record_impression(&self, callback: Url) -> Result<(), RecordImpressionError>;
+    fn record_click(&self, callback: Url) -> Result<(), RecordClickError>;
+    fn report_ad(&self, callback: Url) -> Result<(), ReportAdError>;
     fn get_context_id(&self) -> context_id::ApiResult<String>;
     fn cycle_context_id(&mut self) -> context_id::ApiResult<String>;
     fn get_mars_endpoint(&self) -> &Url;
@@ -97,9 +94,8 @@ impl DefaultMARSClient {
         }
     }
 
-    fn make_callback_request(&self, url_callback_string: &str) -> Result<(), CallbackRequestError> {
-        let url = Url::parse(url_callback_string)?;
-        let request = Request::get(url);
+    fn make_callback_request(&self, callback: Url) -> Result<(), CallbackRequestError> {
+        let request = Request::get(callback);
         let response = request.send()?;
         check_http_status_for_error(&response).map_err(Into::into)
     }
@@ -153,37 +149,16 @@ impl MARSClient for DefaultMARSClient {
         }
     }
 
-    fn record_impression(
-        &self,
-        url_callback_string: Option<String>,
-    ) -> Result<(), RecordImpressionError> {
-        match url_callback_string {
-            Some(callback) => self.make_callback_request(&callback).map_err(Into::into),
-            None => Err(CallbackRequestError::MissingCallback {
-                message: "Impression callback url empty.".to_string(),
-            }
-            .into()),
-        }
+    fn record_impression(&self, callback: Url) -> Result<(), RecordImpressionError> {
+        Ok(self.make_callback_request(callback)?)
     }
 
-    fn record_click(&self, url_callback_string: Option<String>) -> Result<(), RecordClickError> {
-        match url_callback_string {
-            Some(callback) => self.make_callback_request(&callback).map_err(Into::into),
-            None => Err(CallbackRequestError::MissingCallback {
-                message: "Click callback url empty.".to_string(),
-            }
-            .into()),
-        }
+    fn record_click(&self, callback: Url) -> Result<(), RecordClickError> {
+        Ok(self.make_callback_request(callback)?)
     }
 
-    fn report_ad(&self, url_callback_string: Option<String>) -> Result<(), ReportAdError> {
-        match url_callback_string {
-            Some(callback) => self.make_callback_request(&callback).map_err(Into::into),
-            None => Err(CallbackRequestError::MissingCallback {
-                message: "Report callback url empty.".to_string(),
-            }
-            .into()),
-        }
+    fn report_ad(&self, callback: Url) -> Result<(), ReportAdError> {
+        Ok(self.make_callback_request(callback)?)
     }
 
     fn clear_cache(&self) -> Result<(), HttpCacheError> {
@@ -225,36 +200,19 @@ mod tests {
         assert_ne!(client.get_context_id().unwrap(), TEST_CONTEXT_ID);
     }
 
-    #[test]
-    fn test_record_impression_with_empty_callback_should_fail() {
-        let client = create_test_client(mockito::server_url());
-        let result = client.record_impression(None);
-        assert!(result.is_err());
-    }
-
-    #[test]
-    fn test_record_click_with_empty_callback_should_fail() {
-        let client = create_test_client(mockito::server_url());
-        let result = client.record_click(None);
-        assert!(result.is_err());
-    }
-
-    #[test]
-    fn test_record_report_with_empty_callback_should_fail() {
-        let client = create_test_client(mockito::server_url());
-        let result = client.report_ad(None);
-        assert!(result.is_err());
-    }
-
     #[test]
     fn test_record_impression_with_valid_url_should_succeed() {
         viaduct_dev::init_backend_dev();
         let _m = mock("GET", "/impression_callback_url")
             .with_status(200)
             .create();
         let client = create_test_client(mockito::server_url());
-        let url = format!("{}/impression_callback_url", &mockito::server_url());
-        let result = client.record_impression(Some(url));
+        let url = Url::parse(&format!(
+            "{}/impression_callback_url",
+            &mockito::server_url()
+        ))
+        .unwrap();
+        let result = client.record_impression(url);
         assert!(result.is_ok());
     }
 
@@ -264,8 +222,8 @@ mod tests {
         let _m = mock("GET", "/click_callback_url").with_status(200).create();
 
         let client = create_test_client(mockito::server_url());
-        let url = format!("{}/click_callback_url", &mockito::server_url());
-        let result = client.record_click(Some(url));
+        let url = Url::parse(&format!("{}/click_callback_url", &mockito::server_url())).unwrap();
+        let result = client.record_click(url);
         assert!(result.is_ok());
     }
 
@@ -277,8 +235,12 @@ mod tests {
             .create();
 
         let client = create_test_client(mockito::server_url());
-        let url = format!("{}/report_ad_callback_url", &mockito::server_url());
-        let result = client.report_ad(Some(url));
+        let url = Url::parse(&format!(
+            "{}/report_ad_callback_url",
+            &mockito::server_url()
+        ))
+        .unwrap();
+        let result = client.report_ad(url);
         assert!(result.is_ok());
     }