From 86ef587eb491268c022162cc35145f205d79838e Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 27 Aug 2026 15:51:44 -0700 Subject: [PATCH 01/11] feat: Initial commit --- components/ads-client/src/ads_store.rs | 168 +++++ .../ads-client/src/ads_store/builder.rs | 171 +++++ .../src/ads_store/connection_initializer.rs | 89 +++ components/ads-client/src/ads_store/store.rs | 645 ++++++++++++++++++ components/ads-client/src/client.rs | 30 +- components/ads-client/src/client/config.rs | 6 + components/ads-client/src/ffi.rs | 22 +- components/ads-client/src/ffi/telemetry.rs | 1 + components/ads-client/src/http_cache.rs | 4 +- .../ads-client/src/http_cache/builder.rs | 3 +- components/ads-client/src/lib.rs | 1 + components/ads-client/src/mars/ad_response.rs | 38 ++ 12 files changed, 1173 insertions(+), 5 deletions(-) create mode 100644 components/ads-client/src/ads_store.rs create mode 100644 components/ads-client/src/ads_store/builder.rs create mode 100644 components/ads-client/src/ads_store/connection_initializer.rs create mode 100644 components/ads-client/src/ads_store/store.rs diff --git a/components/ads-client/src/ads_store.rs b/components/ads-client/src/ads_store.rs new file mode 100644 index 00000000000..ed58d90b819 --- /dev/null +++ b/components/ads-client/src/ads_store.rs @@ -0,0 +1,168 @@ +pub mod builder; +pub mod connection_initializer; +pub mod store; + +use crate::{ + ads_store::{builder::AdsStoreBuilder, store::AdsStoreHolder}, + http_cache::ByteSize, +}; +use std::path::Path; + +/// Identification of placement sent and returned from MARS (eg: `mock_spoc_1`) +#[derive(Debug, Hash, PartialEq, Eq, Clone)] +pub struct PlacementId(String); + +impl PlacementId { + pub fn new(s: &str) -> PlacementId { + PlacementId(s.to_string()) + } + pub fn into_inner(self) -> String { + self.0 + } +} + +impl AsRef for PlacementId { + fn as_ref(&self) -> &str { + &self.0 + } +} + +pub struct AdsStore { + #[allow(dead_code)] + max_size: ByteSize, + holder: AdsStoreHolder, +} + +impl AdsStore { + pub fn builder>(db_path: P) -> AdsStoreBuilder { + AdsStoreBuilder::new(db_path.as_ref()) + } + + pub fn clear(&self) -> Result<(), rusqlite::Error> { + self.holder.clear_all()?; + Ok(()) + } + + pub fn shutdown_db(self) -> Result<(), rusqlite::Error> { + self.holder.close() + } + + pub fn invalidate_by_id(&self, placement_id: &PlacementId) -> Result<(), rusqlite::Error> { + self.holder.invalidate_ad_by_id(placement_id)?; + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use std::time::Duration; + + use super::*; + use crate::mars::ad_response::{AdCallbacks, AdImage, RawAdType}; + use url::Url; + + #[test] + fn test_ads_store_creation() { + // Test that AdsStore can be created successfully with test config + let store: Result = AdsStore::builder("test_store.db").build(); + assert!(store.is_ok()); + } + + #[test] + fn test_clear_store() { + let store: AdsStore = AdsStore::builder("test_clear.db").build().unwrap(); + + // Create a test request and response + let base_url = mockito::server_url(); + let ad = AdImage { + url: "https://ads.fakeexample.org/example_ad_1".to_string(), + image_url: "https://ads.fakeexample.org/example_image_1".to_string(), + format: "billboard".to_string(), + block_key: "abc123".into(), + alt_text: Some("An ad for a puppy".to_string()), + callbacks: AdCallbacks { + click: Url::parse(&format!("{}/click/example_ad_1", base_url)).unwrap(), + impression: Url::parse(&format!("{}/impression/example_ad_1", base_url)).unwrap(), + report: Some(Url::parse(&format!("{}/report/example_ad_1", base_url)).unwrap()), + }, + }; + + // TODO: Conversion to raw ad, or remove raw ad. + let placement_id = PlacementId::new("mock_billboard_1"); + let body = serde_json::to_vec(&ad).unwrap(); + + store + .holder + .store_with_ttl( + &placement_id, + RawAdType::Image, + body, + &Duration::from_secs(300), + ) + .unwrap(); + + // Verify it's cached + let retrieved = store.holder.lookup(&placement_id).unwrap(); + assert!(retrieved.is_some()); + + // Clear the cache + store.clear().unwrap(); + + // Verify it's cleared + let retrieved_after_clear = store.holder.lookup(&placement_id).unwrap(); + assert!(retrieved_after_clear.is_none()); + } + + #[test] + fn test_invalidate_by_hash() { + let store: AdsStore = AdsStore::builder("test_invalidate.db").build().unwrap(); + + // Create a test request and response + let base_url = mockito::server_url(); + let ad = AdImage { + url: "https://ads.fakeexample.org/example_ad_1".to_string(), + image_url: "https://ads.fakeexample.org/example_image_1".to_string(), + format: "billboard".to_string(), + block_key: "abc123".into(), + alt_text: Some("An ad for a puppy".to_string()), + callbacks: AdCallbacks { + click: Url::parse(&format!("{}/click/example_ad_1", base_url)).unwrap(), + impression: Url::parse(&format!("{}/impression/example_ad_1", base_url)).unwrap(), + report: Some(Url::parse(&format!("{}/report/example_ad_1", base_url)).unwrap()), + }, + }; + + // TODO: Conversion to raw ad, or remove raw ad. + let placement_id_1 = PlacementId::new("mock_billboard_1"); + let placement_id_2 = PlacementId::new("mock_billboard_2"); + let body = serde_json::to_vec(&ad).unwrap(); + + store + .holder + .store_with_ttl( + &placement_id_1, + RawAdType::Image, + body.clone(), + &Duration::from_secs(300), + ) + .unwrap(); + + store + .holder + .store_with_ttl( + &placement_id_2, + RawAdType::Image, + body.clone(), + &Duration::from_secs(300), + ) + .unwrap(); + + assert!(store.holder.lookup(&placement_id_1).unwrap().is_some()); + assert!(store.holder.lookup(&placement_id_2).unwrap().is_some()); + + store.invalidate_by_id(&placement_id_1).unwrap(); + + assert!(store.holder.lookup(&placement_id_1).unwrap().is_none()); + assert!(store.holder.lookup(&placement_id_2).unwrap().is_some()); + } +} diff --git a/components/ads-client/src/ads_store/builder.rs b/components/ads-client/src/ads_store/builder.rs new file mode 100644 index 00000000000..b4870063c6d --- /dev/null +++ b/components/ads-client/src/ads_store/builder.rs @@ -0,0 +1,171 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +use super::connection_initializer::HttpCacheConnectionInitializer; +use crate::ads_store::store::AdsStoreHolder; +use crate::ads_store::AdsStore; +use crate::http_cache::ByteSize; +use rusqlite::Connection; +use sql_support::open_database; +use std::path::PathBuf; + +// TODO: Do we want to make this customizable? +const DEFAULT_MAX_SIZE: ByteSize = ByteSize::mib(10); +const MIN_STORE_SIZE: ByteSize = ByteSize::kib(1); +const MAX_STORE_SIZE: ByteSize = ByteSize::mib(100); + +#[derive(Debug, thiserror::Error)] +pub enum AdsStoreBuilderError { + #[error("Database path cannot be empty")] + EmptyDbPath, + #[error("Database error: {0}")] + Database(#[from] open_database::Error), + #[error( + "Maximum store size must be between {min_size} and {max_size}, got {size_bytes} bytes" + )] + InvalidMaxSize { + max_size: String, + min_size: String, + size_bytes: u64, + }, +} + +pub struct AdsStoreBuilder { + db_path: PathBuf, + max_size: Option, +} + +impl AdsStoreBuilder { + pub fn new(db_path: impl Into) -> Self { + Self { + db_path: db_path.into(), + max_size: None, + } + } + + pub fn max_size(mut self, max_size: ByteSize) -> Self { + self.max_size = Some(max_size); + self + } + + fn open_connection(&self) -> Result { + let initializer = HttpCacheConnectionInitializer {}; + let conn = if cfg!(test) { + open_database::open_memory_database(&initializer)? + } else { + open_database::open_database(&self.db_path, &initializer)? + }; + Ok(conn) + } + + fn validate(&self) -> Result<(), AdsStoreBuilderError> { + if self.db_path.to_string_lossy().trim().is_empty() { + return Err(AdsStoreBuilderError::EmptyDbPath); + } + + if let Some(max_size) = self.max_size { + if max_size < MIN_STORE_SIZE || max_size > MAX_STORE_SIZE { + return Err(AdsStoreBuilderError::InvalidMaxSize { + size_bytes: max_size.as_u64(), + min_size: MIN_STORE_SIZE.to_string(), + max_size: MAX_STORE_SIZE.to_string(), + }); + } + } + + Ok(()) + } + + // TODO: Currently, we do not allow modifying the fields, but we anticipate needing to do so in the future, so we keep this pattern. + pub fn build(&self) -> Result { + self.validate()?; + + let conn = self.open_connection()?; + let holder = AdsStoreHolder::new(conn); + let max_size = self.max_size.unwrap_or(DEFAULT_MAX_SIZE); + Ok(AdsStore { max_size, holder }) + } + + #[cfg(test)] + pub fn build_for_time_dependent_tests(&self) -> Result { + self.validate()?; + + let conn = self.open_connection()?; + let max_size = DEFAULT_MAX_SIZE; + let holder = AdsStoreHolder::new_with_test_clock(conn); + + Ok(AdsStore { max_size, holder }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn make_test_builder(path: &str) -> AdsStoreBuilder { + AdsStoreBuilder::new(path) + } + + #[test] + fn test_store_builder_with_defaults() { + let builder = make_test_builder("test.db"); + assert_eq!(builder.db_path, PathBuf::from("test.db")); + assert_eq!(builder.max_size, None); + assert!(builder.build().is_ok()); + } + + #[test] + fn test_cache_builder_valid_custom() { + let builder = make_test_builder("custom.db").max_size(ByteSize::b(1024)); + + assert_eq!(builder.db_path, PathBuf::from("custom.db")); + assert_eq!(builder.max_size, Some(ByteSize::b(1024))); + assert!(builder.build().is_ok()); + } + + #[test] + fn test_validation_empty_db_path() { + let result = make_test_builder(" ").build(); + assert!(matches!(result, Err(AdsStoreBuilderError::EmptyDbPath))); + } + + #[test] + fn test_validation_max_size_too_small() { + let result = make_test_builder("test.db") + .max_size(ByteSize::b(512)) + .build(); + assert!(matches!( + result, + Err(AdsStoreBuilderError::InvalidMaxSize { + size_bytes: 512, + min_size: _, + max_size: _, + }) + )); + } + + #[test] + fn test_validation_max_size_too_large() { + let result = make_test_builder("test.db") + .max_size(ByteSize::b(2 * 1024 * 1024 * 1024)) + .build(); + assert!(matches!( + result, + Err(AdsStoreBuilderError::InvalidMaxSize { + size_bytes: 2147483648, + min_size: _, + max_size: _, + }) + )); + } + + #[test] + fn test_validation_max_size_boundaries() { + let builder_min = make_test_builder("test.db").max_size(MIN_STORE_SIZE); + assert!(builder_min.build().is_ok()); + + let builder_max = make_test_builder("test.db").max_size(MAX_STORE_SIZE); + assert!(builder_max.build().is_ok()); + } +} diff --git a/components/ads-client/src/ads_store/connection_initializer.rs b/components/ads-client/src/ads_store/connection_initializer.rs new file mode 100644 index 00000000000..9e1e3c941d6 --- /dev/null +++ b/components/ads-client/src/ads_store/connection_initializer.rs @@ -0,0 +1,89 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +use rusqlite::Connection; +use sql_support::open_database; +use std::time::Duration; + +pub struct HttpCacheConnectionInitializer {} + +impl open_database::ConnectionInitializer for HttpCacheConnectionInitializer { + const NAME: &'static str = "ads_cache"; + const END_VERSION: u32 = 1; + + fn prepare(&self, conn: &Connection, _db_empty: bool) -> open_database::Result<()> { + conn.execute_batch("PRAGMA journal_mode=wal;")?; + conn.busy_timeout(Duration::from_secs(5))?; + Ok(()) + } + + fn init(&self, tx: &rusqlite::Transaction<'_>) -> open_database::Result<()> { + const SCHEMA: &str = " + CREATE TABLE IF NOT EXISTS ads ( + cached_at INTEGER NOT NULL, + expires_at INTEGER NOT NULL, + placement_id TEXT NOT NULL, + placement_type SMALLINT NOT NULL, + placement_body BLOB NOT NULL, + size_bytes INTEGER NOT NULL, + ttl_seconds INTEGER NOT NULL, + PRIMARY KEY (placement_id) + ); + CREATE INDEX IF NOT EXISTS idx_ads_cached_at ON ads(cached_at); + CREATE INDEX IF NOT EXISTS idx_ads_expires_at ON ads(expires_at); + CREATE INDEX IF NOT EXISTS idx_ads_placement_id ON ads(placement_id); + "; + // If the schema fails to initialize, it might be corrupted or outdated so we drop the table and try again + if tx.execute_batch(SCHEMA).is_err() { + tx.execute_batch("DROP TABLE IF EXISTS ads")?; + tx.execute_batch(SCHEMA)?; + } + Ok(()) + } + + fn upgrade_from( + &self, + conn: &rusqlite::Transaction<'_>, + version: u32, + ) -> open_database::Result<()> { + match version { + 0 => self.init(conn), + _ => Err(open_database::Error::IncompatibleVersion(version)), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use rusqlite::Connection; + use sql_support::open_database::ConnectionInitializer; + + #[test] + fn test_corrupted_schema_is_recreated() { + let mut conn = Connection::open_in_memory().unwrap(); + let initializer = HttpCacheConnectionInitializer {}; + + // Create a corrupted table with only one column + conn.execute_batch("CREATE TABLE ads (placement_id TEXT);") + .unwrap(); + + // Run init - should drop the corrupted table and recreate it properly + let tx = conn.transaction().unwrap(); + initializer.init(&tx).unwrap(); + tx.commit().unwrap(); + + // Verify the table was recreated with correct schema by checking column count + let column_count: i64 = conn + .query_row("SELECT COUNT(*) FROM pragma_table_info('ads')", [], |row| { + row.get(0) + }) + .unwrap(); + + assert!( + column_count > 1, + "Table should have more than 1 column after recreation" + ); + } +} diff --git a/components/ads-client/src/ads_store/store.rs b/components/ads-client/src/ads_store/store.rs new file mode 100644 index 00000000000..6f048b38a81 --- /dev/null +++ b/components/ads-client/src/ads_store/store.rs @@ -0,0 +1,645 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +use std::{sync::Arc, time::Duration}; + +use crate::{ + ads_store::PlacementId, + http_cache::{ + clock::{CacheClock, Clock}, + ByteSize, + }, + mars::ad_response::{RawAd, RawAdType}, +}; +use parking_lot::Mutex; +use rusqlite::{params, Connection, OptionalExtension, Result as SqliteResult}; + +#[cfg(test)] +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum FaultKind { + None, + Lookup, + Store, + Trim, + Cleanup, +} + +pub struct AdsStoreHolder { + conn: Mutex, + clock: Arc, + #[cfg(test)] + fault: parking_lot::Mutex, +} + +impl AdsStoreHolder { + pub fn new(conn: Connection) -> Self { + Self { + conn: Mutex::new(conn), + clock: Arc::new(CacheClock), + #[cfg(test)] + fault: parking_lot::Mutex::new(FaultKind::None), + } + } + + pub fn close(self) -> Result<(), rusqlite::Error> { + let conn = self.conn.into_inner(); + conn.close().map_err(|(_, err)| err) + } + + #[cfg(test)] + pub fn new_with_test_clock(conn: Connection) -> Self { + use crate::http_cache::clock::TestClock; + + Self { + conn: Mutex::new(conn), + clock: Arc::new(TestClock::new(chrono::Utc::now().timestamp())), + #[cfg(test)] + fault: parking_lot::Mutex::new(FaultKind::None), + } + } + + #[cfg(test)] + pub fn get_clock(&self) -> &dyn Clock { + &*self.clock + } + + /// Removes all entries from cache. + pub fn clear_all(&self) -> SqliteResult { + let conn = self.conn.lock(); + let mut total = 0; + total += conn.execute("DELETE FROM ads", [])?; + Ok(total) + } + + /// Returns total size of the cache in bytes. + pub fn current_total_size_bytes(&self) -> SqliteResult { + let conn = self.conn.lock(); + let size_bytes_ads: u64 = + conn.query_row("SELECT COALESCE(SUM(size_bytes),0) FROM ads", [], |row| { + row.get(0) + })?; + Ok(ByteSize::b(size_bytes_ads)) + } + + /// Removes all entries from the store whose expires_at is at or before the current time. + pub fn delete_expired_entries(&self) -> SqliteResult { + #[cfg(test)] + if *self.fault.lock() == FaultKind::Cleanup { + return Err(Self::forced_fault_error("forced cleanup failure")); + } + let mut conn = self.conn.lock(); + let tx = conn.transaction()?; + let mut total = 0; + total += tx.execute( + "DELETE FROM ads WHERE expires_at <= ?1", + params![self.clock.now_epoch_seconds()], + )?; + tx.commit()?; + Ok(total) + } + /// Lookup is agnostic to expiration. If it exists in the store, it will return the result. + pub fn lookup(&self, placement_id: &PlacementId) -> SqliteResult> { + #[cfg(test)] + if *self.fault.lock() == FaultKind::Lookup { + return Err(Self::forced_fault_error("forced lookup failure")); + } + let conn = self.conn.lock(); + // TODO: Should we use body or explicit fields? + conn.query_row( + "SELECT placement_id, placement_type, placement_body + FROM ads WHERE placement_id = ?1", + params![placement_id.as_ref()], + |row| { + let placement_id: String = row.get(0)?; + let placement_type: u8 = row.get(1)?; + let placement_body: Vec = row.get(2)?; + + let placement_id = PlacementId::new(&placement_id); + let placement_type = RawAdType::try_from(placement_type).map_err(|e| { + rusqlite::Error::FromSqlConversionFailure( + 1, + rusqlite::types::Type::Integer, + e.into(), + ) + })?; + + Ok(RawAd { + placement_id, + placement_type, + placement_body, + }) + }, + ) + .optional() + } + + /// Upsert an object into the store with an expires_at defined by the given ttl_seconds. + /// Calling this method will always store an object regardless of headers or policy. + /// Logic to determine the correct ttl or cache/no-cache should happen before calling this. + /// TODO: maybe this should take a raw ad? maybe no need for raw ad at all? + pub fn store_with_ttl( + &self, + placement_id: &PlacementId, + placement_type: RawAdType, + placement_body: Vec, + ttl: &Duration, + ) -> SqliteResult<()> { + #[cfg(test)] + if *self.fault.lock() == FaultKind::Store { + return Err(Self::forced_fault_error("forced store failure")); + } + let placement_id_str : &str = placement_id.as_ref(); + // placement_id char count + u8 (placement_type) + body length + // TODO: is it actually 8 bytes? https://stackoverflow.com/questions/2761563/what-is-the-difference-between-related-sqlite-data-types-like-int-integer-smal + let size_bytes = (placement_id_str.chars().count() + 8 + placement_body.len()) as i64; + let now = self.clock.now_epoch_seconds(); + let ttl_seconds = ttl.as_secs(); + let expires_at = now + ttl_seconds as i64; + + let conn = self.conn.lock(); + conn.execute( + "INSERT INTO ads ( + cached_at, + expires_at, + placement_id, + placement_type, + placement_body, + size_bytes, + ttl_seconds + ) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7) + ON CONFLICT(placement_id) DO UPDATE SET + cached_at=excluded.cached_at, + expires_at=excluded.expires_at, + placement_type=excluded.placement_type, + placement_body=excluded.placement_body, + size_bytes=excluded.size_bytes, + ttl_seconds=excluded.ttl_seconds", + params![ + now, + expires_at, + placement_id_str, + placement_type.to_u8(), + placement_body, + size_bytes, + ttl_seconds as i64, + ], + )?; + Ok(()) + } + + pub fn invalidate_ad_by_id(&self, placement_id: &PlacementId) -> SqliteResult { + let conn = self.conn.lock(); + conn.execute( + "DELETE FROM ads WHERE placement_id = ?1", + params![&placement_id.as_ref()], + ) + } + + pub fn trim_to_max_size(&self, max_size: &ByteSize) -> SqliteResult<()> { + #[cfg(test)] + if *self.fault.lock() == FaultKind::Trim { + return Err(Self::forced_fault_error("forced trim failure")); + } + loop { + let total = self.current_total_size_bytes()?; + if total.as_u64() <= max_size.as_u64() { + break; + } + let conn = self.conn.lock(); + conn.execute( + "DELETE FROM ads WHERE rowid IN ( + SELECT rowid FROM ads ORDER BY cached_at ASC LIMIT 1 + )", + [], + )?; + } + Ok(()) + } + + #[cfg(test)] + pub fn set_fault(&self, kind: FaultKind) { + *self.fault.lock() = kind; + } + + #[cfg(test)] + fn forced_fault_error(msg: &str) -> rusqlite::Error { + rusqlite::Error::SqliteFailure( + rusqlite::ffi::Error { + code: rusqlite::ErrorCode::InternalMalfunction, + extended_code: 0, + }, + Some(msg.to_string()), + ) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::{ + ads_store::connection_initializer::HttpCacheConnectionInitializer, + mars::ad_response::{AdCallbacks, AdImage}, + }; + use sql_support::open_database; + use std::time::Duration; + use url::Url; + + fn fetch_timestamps(store: &AdsStoreHolder, placement_id: &PlacementId) -> (i64, i64, i64) { + let conn = store.conn.lock(); + conn.query_row( + "SELECT + cached_at, + expires_at, + COALESCE(ttl_seconds, -1) + FROM ads WHERE placement_id = ?1", + rusqlite::params![&placement_id.as_ref()], + |row| { + let cached_at: i64 = row.get(0)?; + let expires_at: i64 = row.get(1)?; + let ttl: i64 = row.get(2)?; + Ok((cached_at, expires_at, ttl)) + }, + ) + .expect("row should exist") + } + + // Create a sample ad for tests. The body defaults to an example serialized AdImage (if body is None). + fn create_test_raw_ad(placement_id: &str, body: Option>) -> RawAd { + let base_url = mockito::server_url(); + let ad = AdImage { + url: "https://ads.fakeexample.org/example_ad_1".to_string(), + image_url: "https://ads.fakeexample.org/example_image_1".to_string(), + format: "billboard".to_string(), + block_key: "abc123".into(), + alt_text: Some("An ad for a puppy".to_string()), + callbacks: AdCallbacks { + click: Url::parse(&format!("{}/click/example_ad_1", base_url)).unwrap(), + impression: Url::parse(&format!("{}/impression/example_ad_1", base_url)).unwrap(), + report: Some(Url::parse(&format!("{}/report/example_ad_1", base_url)).unwrap()), + }, + }; + RawAd { + placement_id: PlacementId::new(placement_id), + placement_type: RawAdType::Image, + placement_body: body.unwrap_or(serde_json::to_vec(&ad).unwrap()), + } + } + + fn create_test_store() -> AdsStoreHolder { + let initializer = HttpCacheConnectionInitializer {}; + let conn = open_database::open_memory_database(&initializer) + .expect("failed to open memory cache db"); + AdsStoreHolder::new_with_test_clock(conn) + } + + #[test] + fn test_lookup_fault_injection() { + let store = create_test_store(); + store.set_fault(FaultKind::Lookup); + + let ad = create_test_raw_ad("mock_billboard_1", None); + let err = store.lookup(&ad.placement_id).unwrap_err(); + + match err { + rusqlite::Error::SqliteFailure(_, Some(msg)) => { + assert!(msg.contains("forced lookup failure")); + } + other => panic!("unexpected error: {other:?}"), + } + } + + #[test] + fn test_store_fault_injection() { + let store = create_test_store(); + store.set_fault(FaultKind::Store); + + let ad = create_test_raw_ad("mock_billboard_1", None); + + let err = store + .store_with_ttl( + &ad.placement_id, + ad.placement_type, + ad.placement_body, + &Duration::from_secs(300), + ) + .unwrap_err(); + match err { + rusqlite::Error::SqliteFailure(_, Some(msg)) => { + assert!(msg.contains("forced store failure")); + } + other => panic!("unexpected error: {other:?}"), + } + } + + #[test] + fn test_trim_fault_injection() { + let store = create_test_store(); + store.set_fault(FaultKind::Trim); + + let ad = create_test_raw_ad("mock_billboard_1", None); + store + .store_with_ttl( + &ad.placement_id, + ad.placement_type, + ad.placement_body, + &Duration::from_secs(300), + ) + .unwrap(); + + let err = store.trim_to_max_size(&ByteSize::b(1)).unwrap_err(); + match err { + rusqlite::Error::SqliteFailure(_, Some(msg)) => { + assert!(msg.contains("forced trim failure")); + } + other => panic!("unexpected error: {other:?}"), + } + } + + #[test] + fn test_cleanup_fault_injection() { + let store = create_test_store(); + store.set_fault(FaultKind::Cleanup); + + let err = store.delete_expired_entries().unwrap_err(); + match err { + rusqlite::Error::SqliteFailure(_, Some(msg)) => { + assert!(msg.contains("forced cleanup failure")); + } + other => panic!("unexpected error: {other:?}"), + } + } + + #[test] + fn test_store_ads_with_ttl_sets_fields_consistently() { + let store = create_test_store(); + let ad = create_test_raw_ad("mock_billboard_1", None); + + let ttl = Duration::from_secs(5); + store + .store_with_ttl(&ad.placement_id, ad.placement_type, ad.placement_body, &ttl) + .unwrap(); + + let (cached_at, expires_at, ttl_seconds) = fetch_timestamps(&store, &ad.placement_id); + assert_eq!(ttl_seconds, ttl.as_secs() as i64); + let diff = expires_at - cached_at; + let ttl_seconds = ttl.as_secs(); + assert!( + (diff == ttl_seconds as i64) + || (diff == ttl_seconds as i64 - 1) + || (diff == ttl_seconds as i64 + 1), + "unexpected expires_at diff: got {diff}, want ~{ttl_seconds}" + ); + } + + #[test] + fn test_upsert_ads_refreshes_ttl_and_expiry() { + let store = create_test_store(); + let ad = create_test_raw_ad("mock_billboard_1", None); + + store + .store_with_ttl( + &ad.placement_id, + ad.placement_type, + ad.placement_body.clone(), + &Duration::from_secs(300), + ) + .unwrap(); + let (c1, e1, t1) = fetch_timestamps(&store, &ad.placement_id); + assert_eq!(t1, 300); + + store.get_clock().advance(3); + + store + .store_with_ttl( + &ad.placement_id, + ad.placement_type, + ad.placement_body, + &Duration::from_secs(1), + ) + .unwrap(); + let (c2, e2, t2) = fetch_timestamps(&store, &ad.placement_id); + assert_eq!(t2, 1); + assert!(c2 > c1); + assert!(e2 < e1, "expires_at should move earlier when TTL shrinks"); + } + + #[test] + fn test_delete_expired_removes_only_expired_ads() { + let store = create_test_store(); + + let ad_exp = create_test_raw_ad("mock_billboard_1", None); + let ad_fresh = create_test_raw_ad("mock_billboard_2", None); + + store + .store_with_ttl( + &ad_exp.placement_id, + ad_exp.placement_type, + ad_exp.placement_body, + &Duration::from_secs(1), + ) + .unwrap(); + store + .store_with_ttl( + &ad_fresh.placement_id, + ad_fresh.placement_type, + ad_fresh.placement_body, + &Duration::from_secs(10), + ) + .unwrap(); + + assert!(store.lookup(&ad_exp.placement_id).unwrap().is_some()); + assert!(store.lookup(&ad_fresh.placement_id).unwrap().is_some()); + + store.clock.advance(2); + let removed = store.delete_expired_entries().unwrap(); + assert!( + removed >= 1, + "expected at least one expired row to be deleted" + ); + + assert!(store.lookup(&ad_exp.placement_id).unwrap().is_none()); + assert!(store.lookup(&ad_fresh.placement_id).unwrap().is_some()); + } + + #[test] + fn test_lookups_is_expired_agnostic() { + let store = create_test_store(); + let ad = create_test_raw_ad("mock_billboard_1", None); + + store + .store_with_ttl( + &ad.placement_id, + ad.placement_type, + ad.placement_body, + &Duration::from_secs(1), + ) + .unwrap(); + store.clock.advance(2); + assert!(store.lookup(&ad.placement_id).unwrap().is_some()); + + store.delete_expired_entries().unwrap(); + assert!(store.lookup(&ad.placement_id).unwrap().is_none()); + } + + #[test] + fn test_zero_ttl_expires_ads_immediately_after_tick() { + let store = create_test_store(); + let ad = create_test_raw_ad("mock_billboard_1", None); + + store + .store_with_ttl( + &ad.placement_id, + ad.placement_type, + ad.placement_body, + &Duration::from_secs(0), + ) + .unwrap(); + assert!(store.lookup(&ad.placement_id).unwrap().is_some()); + + store.clock.advance(2); + let removed = store.delete_expired_entries().unwrap(); + assert!(removed >= 1); + assert!(store.lookup(&ad.placement_id).unwrap().is_none()); + } + + #[test] + fn test_store_and_retrieve_ads() { + let store = create_test_store(); + let ad = create_test_raw_ad("mock_billboard_1", None); + + store + .store_with_ttl( + &ad.placement_id, + ad.placement_type, + ad.placement_body.clone(), + &Duration::from_secs(300), + ) + .unwrap(); + + let retrieved = store.lookup(&ad.placement_id).unwrap().unwrap(); + assert_eq!(retrieved.placement_body, ad.placement_body); + } + + #[test] + fn test_ttl_expiration_ads() { + let store = create_test_store(); + let ad = create_test_raw_ad("mock_billboard_1", Some(b"test response".to_vec())); + + store + .store_with_ttl( + &ad.placement_id, + ad.placement_type, + ad.placement_body, + &Duration::from_secs(300), + ) + .unwrap(); + + let retrieved = store.lookup(&ad.placement_id).unwrap().unwrap(); + assert_eq!(retrieved.placement_body, b"test response"); + + store.clock.advance(2); + + let retrieved_after_expiry = store.lookup(&ad.placement_id).unwrap(); + assert!(retrieved_after_expiry.is_some()); + } + + #[test] + fn test_max_size_eviction_ads() { + let initializer = HttpCacheConnectionInitializer {}; + let conn = open_database::open_memory_database(&initializer) + .expect("failed to open memory cache db"); + let store = AdsStoreHolder::new(conn); + + for i in 0..5 { + let large_body = vec![0u8; 300]; + let ad = create_test_raw_ad(&format!("mock_billboard_{i}"), Some(large_body)); + store + .store_with_ttl( + &ad.placement_id, + ad.placement_type, + ad.placement_body, + &Duration::from_secs(300), + ) + .unwrap(); + } + + store.trim_to_max_size(&ByteSize::kib(1)).unwrap(); + + let total_size = store.current_total_size_bytes().unwrap(); + assert!(total_size.as_u64() <= 1024); + + let first_placement_id = PlacementId::new("mock_billboard_0"); + let first_cached = store.lookup(&first_placement_id).unwrap(); + assert!(first_cached.is_none()); + } + + #[test] + fn test_clear_all_ads() { + let store = create_test_store(); + let ad_1 = create_test_raw_ad("mock_billboard_1", None); + + store + .store_with_ttl( + &ad_1.placement_id, + ad_1.placement_type, + ad_1.placement_body, + &Duration::from_secs(300), + ) + .unwrap(); + + let ad_2 = create_test_raw_ad("mock_billboard_2", None); + store + .store_with_ttl( + &ad_2.placement_id, + ad_2.placement_type, + ad_2.placement_body, + &Duration::from_secs(300), + ) + .unwrap(); + + assert!(store.lookup(&ad_1.placement_id).unwrap().is_some()); + assert!(store.lookup(&ad_2.placement_id).unwrap().is_some()); + + let deleted_count = store.clear_all().unwrap(); + assert_eq!(deleted_count, 2); + + assert!(store.lookup(&ad_1.placement_id).unwrap().is_none()); + assert!(store.lookup(&ad_2.placement_id).unwrap().is_none()); + } + + #[test] + fn test_invalidate_ad_by_placement_id() { + let store = create_test_store(); + + let ad_1 = create_test_raw_ad("mock_billboard_1", None); + let ad_2 = create_test_raw_ad("mock_billboard_2", None); + + store + .store_with_ttl( + &ad_1.placement_id, + ad_1.placement_type, + ad_1.placement_body, + &Duration::from_secs(300), + ) + .unwrap(); + store + .store_with_ttl( + &ad_2.placement_id, + ad_2.placement_type, + ad_2.placement_body, + &Duration::from_secs(300), + ) + .unwrap(); + + assert!(store.lookup(&ad_1.placement_id).unwrap().is_some()); + assert!(store.lookup(&ad_2.placement_id).unwrap().is_some()); + + let deleted = store.invalidate_ad_by_id(&ad_1.placement_id).unwrap(); + assert_eq!(deleted, 1); + + assert!(store.lookup(&ad_1.placement_id).unwrap().is_none()); + assert!(store.lookup(&ad_2.placement_id).unwrap().is_some()); + } +} diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index 1089948649b..059081b5954 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -6,6 +6,7 @@ use std::collections::HashMap; use std::time::Duration; +use crate::ads_store::AdsStore; use crate::http_cache::{ByteSize, CachePolicy, HttpCache}; use crate::mars::ad_request::{AdPlacementRequest, AdRequestFlags}; use crate::mars::ad_response::{AdImage, AdResponse, AdResponseValue, AdSpoc, AdTile}; @@ -42,6 +43,7 @@ where client: MARSClient, context_id_provider: Box, telemetry: T, + ads_store: Option, } impl AdsClient @@ -85,12 +87,24 @@ where } }); + let ads_store = client_config.store_config.and_then(|x| { + match AdsStore::builder(x.db_path).build() { + Ok(store) => Some(store), + Err(e) => { + // TODO: Telemetry needs to work + telemetry.record(&e); + None + } + } + }); + let client = MARSClient::new(environment, http_cache, telemetry.clone()); telemetry.record(&ClientOperationEvent::New); Self { client, context_id_provider, telemetry: telemetry.clone(), + ads_store, } } @@ -105,8 +119,13 @@ where self.telemetry.shutdown(); // Shutdown DB - self.client.shutdown_db()?; + let r = self.client.shutdown_db(); + + if let Some(ads_store) = self.ads_store.take() { + ads_store.shutdown_db()?; + } + r?; Ok(()) } @@ -278,6 +297,7 @@ mod tests { use std::{assert_eq, assert_ne, sync::Arc}; use crate::{ + ads_store::builder::AdsStoreBuilder, ffi::telemetry::MozAdsTelemetryWrapper, mars::Environment, test_utils::{ @@ -301,6 +321,11 @@ mod tests { Box::new(DefaultContextIdCallback), )), telemetry, + ads_store: Some( + AdsStoreBuilder::new("test_store.db") + .build() + .expect("Simplest AdsStoreBuilder should be constructable"), + ), } } @@ -311,6 +336,7 @@ mod tests { context_id_provider: None, environment: Environment::Test, telemetry: MozAdsTelemetryWrapper::noop(), + store_config: None, }; let client = AdsClient::new(config); let context_id = client.get_context_id().unwrap(); @@ -415,6 +441,7 @@ mod tests { context_id_provider: Some(Box::new(FixedContextId)), environment: Environment::Test, telemetry: MozAdsTelemetryWrapper::noop(), + store_config: None, }; let client = AdsClient::new(config); @@ -534,6 +561,7 @@ mod tests { context_id_provider: None, environment: Environment::Test, telemetry: noop_telemetry, + store_config: None, }; let mut client = AdsClient::new(config); diff --git a/components/ads-client/src/client/config.rs b/components/ads-client/src/client/config.rs index 7c86c241418..2e96c34fa63 100644 --- a/components/ads-client/src/client/config.rs +++ b/components/ads-client/src/client/config.rs @@ -11,6 +11,7 @@ where T: Telemetry, { pub cache_config: Option, + pub store_config: Option, pub context_id_provider: Option>, pub environment: Environment, pub telemetry: T, @@ -22,3 +23,8 @@ pub struct AdsCacheConfig { pub default_cache_ttl_seconds: Option, pub max_size_mib: Option, } + +#[derive(Clone, Debug)] +pub struct AdsStoreConfig { + pub db_path: String, +} diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index ba50352fdc1..e7435746c2e 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -8,7 +8,7 @@ pub mod telemetry; use std::sync::Arc; -use crate::client::config::{AdsCacheConfig, AdsClientConfig}; +use crate::client::config::{AdsCacheConfig, AdsClientConfig, AdsStoreConfig}; use crate::client::{AdsClient, ContextIdProvider}; use crate::ffi::telemetry::MozAdsTelemetryWrapper; use crate::http_cache::CachePolicy; @@ -107,6 +107,7 @@ struct MozAdsClientBuilderInner { context_id_provider: Option>, environment: Option, telemetry: Option>, + store_config: Option, } impl Default for MozAdsClientBuilder { @@ -137,6 +138,7 @@ impl MozAdsClientBuilder { .clone() .map(MozAdsTelemetryWrapper::new) .unwrap_or_else(MozAdsTelemetryWrapper::noop), + store_config: inner.store_config.clone().map(Into::into), }; let client = AdsClient::new(client_config); MozAdsClient { @@ -149,6 +151,11 @@ impl MozAdsClientBuilder { self } + pub fn store_config(self: Arc, store_config: MozAdsStoreConfig) -> Arc { + self.0.lock().store_config = Some(store_config); + self + } + pub fn context_id_provider( self: Arc, provider: Arc, @@ -186,6 +193,11 @@ pub struct MozAdsCacheConfig { pub max_size_mib: Option, } +#[derive(Clone, uniffi::Record)] +pub struct MozAdsStoreConfig { + pub db_path: String, +} + #[derive(Debug, PartialEq, uniffi::Record)] pub struct MozAdsContentCategory { pub categories: Vec, @@ -452,6 +464,14 @@ impl From for AdsCacheConfig { } } +impl From for AdsStoreConfig { + fn from(config: MozAdsStoreConfig) -> Self { + Self { + db_path: config.db_path, + } + } +} + impl From<&MozAdsPlacementRequest> for AdPlacementRequest { fn from(request: &MozAdsPlacementRequest) -> Self { Self { diff --git a/components/ads-client/src/ffi/telemetry.rs b/components/ads-client/src/ffi/telemetry.rs index 02a6fee2e46..3fc7bc8c277 100644 --- a/components/ads-client/src/ffi/telemetry.rs +++ b/components/ads-client/src/ffi/telemetry.rs @@ -20,6 +20,7 @@ pub trait MozAdsTelemetry: Send + Sync { fn record_client_error(&self, label: String, value: String); fn record_client_operation_total(&self, label: String); fn record_deserialization_error(&self, label: String, value: String); + // TODO: rename these fn record_http_cache_outcome(&self, label: String, value: String); } diff --git a/components/ads-client/src/http_cache.rs b/components/ads-client/src/http_cache.rs index 81b056bb1df..bfc08e93e1b 100644 --- a/components/ads-client/src/http_cache.rs +++ b/components/ads-client/src/http_cache.rs @@ -5,7 +5,8 @@ mod builder; mod bytesize; mod cache_control; -mod clock; +// TODO: Remove this +pub mod clock; mod connection_initializer; mod outcome; mod request_hash; @@ -24,6 +25,7 @@ use viaduct::{Client, Request, Response}; pub use self::builder::HttpCacheBuilderError; pub use self::bytesize::ByteSize; + pub use self::outcome::CacheOutcome; pub use self::request_hash::RequestHash; use std::path::Path; diff --git a/components/ads-client/src/http_cache/builder.rs b/components/ads-client/src/http_cache/builder.rs index 97f92f004b0..ac4e7615bc2 100644 --- a/components/ads-client/src/http_cache/builder.rs +++ b/components/ads-client/src/http_cache/builder.rs @@ -2,11 +2,10 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use crate::http_cache::HttpCache; - use super::bytesize::ByteSize; use super::connection_initializer::HttpCacheConnectionInitializer; use super::store::HttpCacheStore; +use crate::http_cache::HttpCache; use rusqlite::Connection; use sql_support::open_database; use std::path::PathBuf; diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 87cecb2a5f8..03e652df526 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -15,6 +15,7 @@ use client::AdsClient; use error_support::error; use http_cache::CachePolicy; use mars::ad_request::{AdPlacementRequest, AdRequestFlags}; +pub mod ads_store; mod client; mod ffi; pub mod http_cache; diff --git a/components/ads-client/src/mars/ad_response.rs b/components/ads-client/src/mars/ad_response.rs index 5e057b7606b..55e406ee93f 100644 --- a/components/ads-client/src/mars/ad_response.rs +++ b/components/ads-client/src/mars/ad_response.rs @@ -3,6 +3,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +use crate::ads_store::PlacementId; use crate::http_cache::RequestHash; use crate::telemetry::Telemetry; use serde::de::DeserializeOwned; @@ -141,6 +142,43 @@ pub struct AdTile { pub url: String, } +// TODO: Still needed? +#[derive(Debug)] +pub struct RawAd { + pub placement_id: PlacementId, + pub placement_type: RawAdType, + pub placement_body: Vec, +} + +#[derive(Clone, Copy, Debug)] +pub enum RawAdType { + Image, + Spoc, + Tile, +} + +impl RawAdType { + pub fn to_u8(self) -> u8 { + match self { + RawAdType::Image => 0, + RawAdType::Spoc => 1, + RawAdType::Tile => 2, + } + } +} + +impl TryFrom for RawAdType { + type Error = String; + fn try_from(value: u8) -> Result { + Ok(match value { + 0 => RawAdType::Image, + 1 => RawAdType::Spoc, + 2 => RawAdType::Tile, + x => return Err(format!("Invalid variant for RawAdType: {x}")), + }) + } +} + #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] pub struct SpocFrequencyCaps { pub cap_key: String, From c17bbede533466a794aa9b5aef7a89ec8272860b Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 27 Aug 2026 16:01:06 -0700 Subject: [PATCH 02/11] fix: rename rawad --- components/ads-client/src/ads_store.rs | 8 +- .../ads-client/src/ads_store/builder.rs | 2 - .../src/ads_store/connection_initializer.rs | 4 +- components/ads-client/src/ads_store/store.rs | 112 +++++++++--------- components/ads-client/src/http_cache.rs | 1 - components/ads-client/src/mars/ad_response.rs | 27 ++--- 6 files changed, 75 insertions(+), 79 deletions(-) diff --git a/components/ads-client/src/ads_store.rs b/components/ads-client/src/ads_store.rs index ed58d90b819..0668090969c 100644 --- a/components/ads-client/src/ads_store.rs +++ b/components/ads-client/src/ads_store.rs @@ -58,7 +58,7 @@ mod tests { use std::time::Duration; use super::*; - use crate::mars::ad_response::{AdCallbacks, AdImage, RawAdType}; + use crate::mars::ad_response::{AdCallbacks, AdImage, StorableAdType}; use url::Url; #[test] @@ -95,7 +95,7 @@ mod tests { .holder .store_with_ttl( &placement_id, - RawAdType::Image, + StorableAdType::Image, body, &Duration::from_secs(300), ) @@ -141,7 +141,7 @@ mod tests { .holder .store_with_ttl( &placement_id_1, - RawAdType::Image, + StorableAdType::Image, body.clone(), &Duration::from_secs(300), ) @@ -151,7 +151,7 @@ mod tests { .holder .store_with_ttl( &placement_id_2, - RawAdType::Image, + StorableAdType::Image, body.clone(), &Duration::from_secs(300), ) diff --git a/components/ads-client/src/ads_store/builder.rs b/components/ads-client/src/ads_store/builder.rs index b4870063c6d..9c606d3dd24 100644 --- a/components/ads-client/src/ads_store/builder.rs +++ b/components/ads-client/src/ads_store/builder.rs @@ -10,7 +10,6 @@ use rusqlite::Connection; use sql_support::open_database; use std::path::PathBuf; -// TODO: Do we want to make this customizable? const DEFAULT_MAX_SIZE: ByteSize = ByteSize::mib(10); const MIN_STORE_SIZE: ByteSize = ByteSize::kib(1); const MAX_STORE_SIZE: ByteSize = ByteSize::mib(100); @@ -77,7 +76,6 @@ impl AdsStoreBuilder { Ok(()) } - // TODO: Currently, we do not allow modifying the fields, but we anticipate needing to do so in the future, so we keep this pattern. pub fn build(&self) -> Result { self.validate()?; diff --git a/components/ads-client/src/ads_store/connection_initializer.rs b/components/ads-client/src/ads_store/connection_initializer.rs index 9e1e3c941d6..6d1ffd5eb76 100644 --- a/components/ads-client/src/ads_store/connection_initializer.rs +++ b/components/ads-client/src/ads_store/connection_initializer.rs @@ -24,8 +24,8 @@ impl open_database::ConnectionInitializer for HttpCacheConnectionInitializer { cached_at INTEGER NOT NULL, expires_at INTEGER NOT NULL, placement_id TEXT NOT NULL, - placement_type SMALLINT NOT NULL, - placement_body BLOB NOT NULL, + ad_type SMALLINT NOT NULL, + ad_body BLOB NOT NULL, size_bytes INTEGER NOT NULL, ttl_seconds INTEGER NOT NULL, PRIMARY KEY (placement_id) diff --git a/components/ads-client/src/ads_store/store.rs b/components/ads-client/src/ads_store/store.rs index 6f048b38a81..6301468ef86 100644 --- a/components/ads-client/src/ads_store/store.rs +++ b/components/ads-client/src/ads_store/store.rs @@ -10,7 +10,7 @@ use crate::{ clock::{CacheClock, Clock}, ByteSize, }, - mars::ad_response::{RawAd, RawAdType}, + mars::ad_response::{StorableAd, StorableAdType}, }; use parking_lot::Mutex; use rusqlite::{params, Connection, OptionalExtension, Result as SqliteResult}; @@ -99,7 +99,7 @@ impl AdsStoreHolder { Ok(total) } /// Lookup is agnostic to expiration. If it exists in the store, it will return the result. - pub fn lookup(&self, placement_id: &PlacementId) -> SqliteResult> { + pub fn lookup(&self, placement_id: &PlacementId) -> SqliteResult> { #[cfg(test)] if *self.fault.lock() == FaultKind::Lookup { return Err(Self::forced_fault_error("forced lookup failure")); @@ -107,16 +107,16 @@ impl AdsStoreHolder { let conn = self.conn.lock(); // TODO: Should we use body or explicit fields? conn.query_row( - "SELECT placement_id, placement_type, placement_body + "SELECT placement_id, ad_type, ad_body FROM ads WHERE placement_id = ?1", params![placement_id.as_ref()], |row| { let placement_id: String = row.get(0)?; - let placement_type: u8 = row.get(1)?; - let placement_body: Vec = row.get(2)?; + let ad_type: u8 = row.get(1)?; + let ad_body: Vec = row.get(2)?; let placement_id = PlacementId::new(&placement_id); - let placement_type = RawAdType::try_from(placement_type).map_err(|e| { + let ad_type = StorableAdType::try_from(ad_type).map_err(|e| { rusqlite::Error::FromSqlConversionFailure( 1, rusqlite::types::Type::Integer, @@ -124,10 +124,10 @@ impl AdsStoreHolder { ) })?; - Ok(RawAd { + Ok(StorableAd { placement_id, - placement_type, - placement_body, + ad_type, + ad_body, }) }, ) @@ -141,8 +141,8 @@ impl AdsStoreHolder { pub fn store_with_ttl( &self, placement_id: &PlacementId, - placement_type: RawAdType, - placement_body: Vec, + ad_type: StorableAdType, + ad_body: Vec, ttl: &Duration, ) -> SqliteResult<()> { #[cfg(test)] @@ -150,9 +150,9 @@ impl AdsStoreHolder { return Err(Self::forced_fault_error("forced store failure")); } let placement_id_str : &str = placement_id.as_ref(); - // placement_id char count + u8 (placement_type) + body length + // placement_id char count + u8 (ad_type) + body length // TODO: is it actually 8 bytes? https://stackoverflow.com/questions/2761563/what-is-the-difference-between-related-sqlite-data-types-like-int-integer-smal - let size_bytes = (placement_id_str.chars().count() + 8 + placement_body.len()) as i64; + let size_bytes = (placement_id_str.chars().count() + 8 + ad_body.len()) as i64; let now = self.clock.now_epoch_seconds(); let ttl_seconds = ttl.as_secs(); let expires_at = now + ttl_seconds as i64; @@ -163,8 +163,8 @@ impl AdsStoreHolder { cached_at, expires_at, placement_id, - placement_type, - placement_body, + ad_type, + ad_body, size_bytes, ttl_seconds ) @@ -172,16 +172,16 @@ impl AdsStoreHolder { ON CONFLICT(placement_id) DO UPDATE SET cached_at=excluded.cached_at, expires_at=excluded.expires_at, - placement_type=excluded.placement_type, - placement_body=excluded.placement_body, + ad_type=excluded.ad_type, + ad_body=excluded.ad_body, size_bytes=excluded.size_bytes, ttl_seconds=excluded.ttl_seconds", params![ now, expires_at, placement_id_str, - placement_type.to_u8(), - placement_body, + ad_type.to_u8(), + ad_body, size_bytes, ttl_seconds as i64, ], @@ -266,7 +266,7 @@ mod tests { } // Create a sample ad for tests. The body defaults to an example serialized AdImage (if body is None). - fn create_test_raw_ad(placement_id: &str, body: Option>) -> RawAd { + fn create_test_raw_ad(placement_id: &str, body: Option>) -> StorableAd { let base_url = mockito::server_url(); let ad = AdImage { url: "https://ads.fakeexample.org/example_ad_1".to_string(), @@ -280,10 +280,10 @@ mod tests { report: Some(Url::parse(&format!("{}/report/example_ad_1", base_url)).unwrap()), }, }; - RawAd { + StorableAd { placement_id: PlacementId::new(placement_id), - placement_type: RawAdType::Image, - placement_body: body.unwrap_or(serde_json::to_vec(&ad).unwrap()), + ad_type: StorableAdType::Image, + ad_body: body.unwrap_or(serde_json::to_vec(&ad).unwrap()), } } @@ -320,8 +320,8 @@ mod tests { let err = store .store_with_ttl( &ad.placement_id, - ad.placement_type, - ad.placement_body, + ad.ad_type, + ad.ad_body, &Duration::from_secs(300), ) .unwrap_err(); @@ -342,8 +342,8 @@ mod tests { store .store_with_ttl( &ad.placement_id, - ad.placement_type, - ad.placement_body, + ad.ad_type, + ad.ad_body, &Duration::from_secs(300), ) .unwrap(); @@ -378,7 +378,7 @@ mod tests { let ttl = Duration::from_secs(5); store - .store_with_ttl(&ad.placement_id, ad.placement_type, ad.placement_body, &ttl) + .store_with_ttl(&ad.placement_id, ad.ad_type, ad.ad_body, &ttl) .unwrap(); let (cached_at, expires_at, ttl_seconds) = fetch_timestamps(&store, &ad.placement_id); @@ -401,8 +401,8 @@ mod tests { store .store_with_ttl( &ad.placement_id, - ad.placement_type, - ad.placement_body.clone(), + ad.ad_type, + ad.ad_body.clone(), &Duration::from_secs(300), ) .unwrap(); @@ -414,8 +414,8 @@ mod tests { store .store_with_ttl( &ad.placement_id, - ad.placement_type, - ad.placement_body, + ad.ad_type, + ad.ad_body, &Duration::from_secs(1), ) .unwrap(); @@ -435,16 +435,16 @@ mod tests { store .store_with_ttl( &ad_exp.placement_id, - ad_exp.placement_type, - ad_exp.placement_body, + ad_exp.ad_type, + ad_exp.ad_body, &Duration::from_secs(1), ) .unwrap(); store .store_with_ttl( &ad_fresh.placement_id, - ad_fresh.placement_type, - ad_fresh.placement_body, + ad_fresh.ad_type, + ad_fresh.ad_body, &Duration::from_secs(10), ) .unwrap(); @@ -471,8 +471,8 @@ mod tests { store .store_with_ttl( &ad.placement_id, - ad.placement_type, - ad.placement_body, + ad.ad_type, + ad.ad_body, &Duration::from_secs(1), ) .unwrap(); @@ -491,8 +491,8 @@ mod tests { store .store_with_ttl( &ad.placement_id, - ad.placement_type, - ad.placement_body, + ad.ad_type, + ad.ad_body, &Duration::from_secs(0), ) .unwrap(); @@ -512,14 +512,14 @@ mod tests { store .store_with_ttl( &ad.placement_id, - ad.placement_type, - ad.placement_body.clone(), + ad.ad_type, + ad.ad_body.clone(), &Duration::from_secs(300), ) .unwrap(); let retrieved = store.lookup(&ad.placement_id).unwrap().unwrap(); - assert_eq!(retrieved.placement_body, ad.placement_body); + assert_eq!(retrieved.ad_body, ad.ad_body); } #[test] @@ -530,14 +530,14 @@ mod tests { store .store_with_ttl( &ad.placement_id, - ad.placement_type, - ad.placement_body, + ad.ad_type, + ad.ad_body, &Duration::from_secs(300), ) .unwrap(); let retrieved = store.lookup(&ad.placement_id).unwrap().unwrap(); - assert_eq!(retrieved.placement_body, b"test response"); + assert_eq!(retrieved.ad_body, b"test response"); store.clock.advance(2); @@ -558,8 +558,8 @@ mod tests { store .store_with_ttl( &ad.placement_id, - ad.placement_type, - ad.placement_body, + ad.ad_type, + ad.ad_body, &Duration::from_secs(300), ) .unwrap(); @@ -583,8 +583,8 @@ mod tests { store .store_with_ttl( &ad_1.placement_id, - ad_1.placement_type, - ad_1.placement_body, + ad_1.ad_type, + ad_1.ad_body, &Duration::from_secs(300), ) .unwrap(); @@ -593,8 +593,8 @@ mod tests { store .store_with_ttl( &ad_2.placement_id, - ad_2.placement_type, - ad_2.placement_body, + ad_2.ad_type, + ad_2.ad_body, &Duration::from_secs(300), ) .unwrap(); @@ -619,16 +619,16 @@ mod tests { store .store_with_ttl( &ad_1.placement_id, - ad_1.placement_type, - ad_1.placement_body, + ad_1.ad_type, + ad_1.ad_body, &Duration::from_secs(300), ) .unwrap(); store .store_with_ttl( &ad_2.placement_id, - ad_2.placement_type, - ad_2.placement_body, + ad_2.ad_type, + ad_2.ad_body, &Duration::from_secs(300), ) .unwrap(); diff --git a/components/ads-client/src/http_cache.rs b/components/ads-client/src/http_cache.rs index bfc08e93e1b..fb297c8fb07 100644 --- a/components/ads-client/src/http_cache.rs +++ b/components/ads-client/src/http_cache.rs @@ -25,7 +25,6 @@ use viaduct::{Client, Request, Response}; pub use self::builder::HttpCacheBuilderError; pub use self::bytesize::ByteSize; - pub use self::outcome::CacheOutcome; pub use self::request_hash::RequestHash; use std::path::Path; diff --git a/components/ads-client/src/mars/ad_response.rs b/components/ads-client/src/mars/ad_response.rs index 55e406ee93f..13542fe00da 100644 --- a/components/ads-client/src/mars/ad_response.rs +++ b/components/ads-client/src/mars/ad_response.rs @@ -142,39 +142,38 @@ pub struct AdTile { pub url: String, } -// TODO: Still needed? #[derive(Debug)] -pub struct RawAd { +pub struct StorableAd { pub placement_id: PlacementId, - pub placement_type: RawAdType, - pub placement_body: Vec, + pub ad_type: StorableAdType, + pub ad_body: Vec, } #[derive(Clone, Copy, Debug)] -pub enum RawAdType { +pub enum StorableAdType { Image, Spoc, Tile, } -impl RawAdType { +impl StorableAdType { pub fn to_u8(self) -> u8 { match self { - RawAdType::Image => 0, - RawAdType::Spoc => 1, - RawAdType::Tile => 2, + StorableAdType::Image => 0, + StorableAdType::Spoc => 1, + StorableAdType::Tile => 2, } } } -impl TryFrom for RawAdType { +impl TryFrom for StorableAdType { type Error = String; fn try_from(value: u8) -> Result { Ok(match value { - 0 => RawAdType::Image, - 1 => RawAdType::Spoc, - 2 => RawAdType::Tile, - x => return Err(format!("Invalid variant for RawAdType: {x}")), + 0 => StorableAdType::Image, + 1 => StorableAdType::Spoc, + 2 => StorableAdType::Tile, + x => return Err(format!("Invalid variant for StorableAdType: {x}")), }) } } From 640873f201b91af3c1d1db30f6346a22360b4002 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 27 Aug 2026 17:39:06 -0700 Subject: [PATCH 03/11] fix: storablead --- components/ads-client/src/ads_store.rs | 59 ++++---- components/ads-client/src/ads_store/store.rs | 132 +++--------------- components/ads-client/src/mars/ad_response.rs | 2 +- 3 files changed, 50 insertions(+), 143 deletions(-) diff --git a/components/ads-client/src/ads_store.rs b/components/ads-client/src/ads_store.rs index 0668090969c..6c9e6f9bbc0 100644 --- a/components/ads-client/src/ads_store.rs +++ b/components/ads-client/src/ads_store.rs @@ -58,7 +58,7 @@ mod tests { use std::time::Duration; use super::*; - use crate::mars::ad_response::{AdCallbacks, AdImage, StorableAdType}; + use crate::mars::ad_response::{AdCallbacks, AdImage, StorableAd, StorableAdType}; use url::Url; #[test] @@ -87,29 +87,26 @@ mod tests { }, }; - // TODO: Conversion to raw ad, or remove raw ad. - let placement_id = PlacementId::new("mock_billboard_1"); - let body = serde_json::to_vec(&ad).unwrap(); + let ad = StorableAd { + placement_id: PlacementId::new("mock_billboard_1"), + ad_type: StorableAdType::Image, + ad_body: serde_json::to_vec(&ad).unwrap(), + }; store .holder - .store_with_ttl( - &placement_id, - StorableAdType::Image, - body, - &Duration::from_secs(300), - ) + .store_with_ttl(ad.clone(), &Duration::from_secs(300)) .unwrap(); // Verify it's cached - let retrieved = store.holder.lookup(&placement_id).unwrap(); + let retrieved = store.holder.lookup(&ad.placement_id).unwrap(); assert!(retrieved.is_some()); // Clear the cache store.clear().unwrap(); // Verify it's cleared - let retrieved_after_clear = store.holder.lookup(&placement_id).unwrap(); + let retrieved_after_clear = store.holder.lookup(&ad.placement_id).unwrap(); assert!(retrieved_after_clear.is_none()); } @@ -132,37 +129,33 @@ mod tests { }, }; - // TODO: Conversion to raw ad, or remove raw ad. - let placement_id_1 = PlacementId::new("mock_billboard_1"); - let placement_id_2 = PlacementId::new("mock_billboard_2"); - let body = serde_json::to_vec(&ad).unwrap(); + let ad_1 = StorableAd { + placement_id: PlacementId::new("mock_billboard_1"), + ad_type: StorableAdType::Image, + ad_body: serde_json::to_vec(&ad).unwrap(), + }; + let ad_2 = StorableAd { + placement_id: PlacementId::new("mock_billboard_2"), + ad_type: StorableAdType::Image, + ad_body: serde_json::to_vec(&ad).unwrap(), + }; store .holder - .store_with_ttl( - &placement_id_1, - StorableAdType::Image, - body.clone(), - &Duration::from_secs(300), - ) + .store_with_ttl(ad_1.clone(), &Duration::from_secs(300)) .unwrap(); store .holder - .store_with_ttl( - &placement_id_2, - StorableAdType::Image, - body.clone(), - &Duration::from_secs(300), - ) + .store_with_ttl(ad_2.clone(), &Duration::from_secs(300)) .unwrap(); - assert!(store.holder.lookup(&placement_id_1).unwrap().is_some()); - assert!(store.holder.lookup(&placement_id_2).unwrap().is_some()); + assert!(store.holder.lookup(&ad_1.placement_id).unwrap().is_some()); + assert!(store.holder.lookup(&ad_2.placement_id).unwrap().is_some()); - store.invalidate_by_id(&placement_id_1).unwrap(); + store.invalidate_by_id(&ad_1.placement_id).unwrap(); - assert!(store.holder.lookup(&placement_id_1).unwrap().is_none()); - assert!(store.holder.lookup(&placement_id_2).unwrap().is_some()); + assert!(store.holder.lookup(&ad_1.placement_id).unwrap().is_none()); + assert!(store.holder.lookup(&ad_2.placement_id).unwrap().is_some()); } } diff --git a/components/ads-client/src/ads_store/store.rs b/components/ads-client/src/ads_store/store.rs index 6301468ef86..bf784025999 100644 --- a/components/ads-client/src/ads_store/store.rs +++ b/components/ads-client/src/ads_store/store.rs @@ -138,21 +138,15 @@ impl AdsStoreHolder { /// Calling this method will always store an object regardless of headers or policy. /// Logic to determine the correct ttl or cache/no-cache should happen before calling this. /// TODO: maybe this should take a raw ad? maybe no need for raw ad at all? - pub fn store_with_ttl( - &self, - placement_id: &PlacementId, - ad_type: StorableAdType, - ad_body: Vec, - ttl: &Duration, - ) -> SqliteResult<()> { + pub fn store_with_ttl(&self, ad: StorableAd, ttl: &Duration) -> SqliteResult<()> { #[cfg(test)] if *self.fault.lock() == FaultKind::Store { return Err(Self::forced_fault_error("forced store failure")); } - let placement_id_str : &str = placement_id.as_ref(); + let placement_id_str: &str = ad.placement_id.as_ref(); // placement_id char count + u8 (ad_type) + body length // TODO: is it actually 8 bytes? https://stackoverflow.com/questions/2761563/what-is-the-difference-between-related-sqlite-data-types-like-int-integer-smal - let size_bytes = (placement_id_str.chars().count() + 8 + ad_body.len()) as i64; + let size_bytes = (placement_id_str.chars().count() + 8 + ad.ad_body.len()) as i64; let now = self.clock.now_epoch_seconds(); let ttl_seconds = ttl.as_secs(); let expires_at = now + ttl_seconds as i64; @@ -180,8 +174,8 @@ impl AdsStoreHolder { now, expires_at, placement_id_str, - ad_type.to_u8(), - ad_body, + ad.ad_type.to_u8(), + ad.ad_body, size_bytes, ttl_seconds as i64, ], @@ -318,12 +312,7 @@ mod tests { let ad = create_test_raw_ad("mock_billboard_1", None); let err = store - .store_with_ttl( - &ad.placement_id, - ad.ad_type, - ad.ad_body, - &Duration::from_secs(300), - ) + .store_with_ttl(ad, &Duration::from_secs(300)) .unwrap_err(); match err { rusqlite::Error::SqliteFailure(_, Some(msg)) => { @@ -339,14 +328,7 @@ mod tests { store.set_fault(FaultKind::Trim); let ad = create_test_raw_ad("mock_billboard_1", None); - store - .store_with_ttl( - &ad.placement_id, - ad.ad_type, - ad.ad_body, - &Duration::from_secs(300), - ) - .unwrap(); + store.store_with_ttl(ad, &Duration::from_secs(300)).unwrap(); let err = store.trim_to_max_size(&ByteSize::b(1)).unwrap_err(); match err { @@ -375,13 +357,11 @@ mod tests { fn test_store_ads_with_ttl_sets_fields_consistently() { let store = create_test_store(); let ad = create_test_raw_ad("mock_billboard_1", None); - + let placement_id = ad.placement_id.clone(); let ttl = Duration::from_secs(5); - store - .store_with_ttl(&ad.placement_id, ad.ad_type, ad.ad_body, &ttl) - .unwrap(); + store.store_with_ttl(ad, &ttl).unwrap(); - let (cached_at, expires_at, ttl_seconds) = fetch_timestamps(&store, &ad.placement_id); + let (cached_at, expires_at, ttl_seconds) = fetch_timestamps(&store, &placement_id); assert_eq!(ttl_seconds, ttl.as_secs() as i64); let diff = expires_at - cached_at; let ttl_seconds = ttl.as_secs(); @@ -397,14 +377,8 @@ mod tests { fn test_upsert_ads_refreshes_ttl_and_expiry() { let store = create_test_store(); let ad = create_test_raw_ad("mock_billboard_1", None); - store - .store_with_ttl( - &ad.placement_id, - ad.ad_type, - ad.ad_body.clone(), - &Duration::from_secs(300), - ) + .store_with_ttl(ad.clone(), &Duration::from_secs(300)) .unwrap(); let (c1, e1, t1) = fetch_timestamps(&store, &ad.placement_id); assert_eq!(t1, 300); @@ -412,12 +386,7 @@ mod tests { store.get_clock().advance(3); store - .store_with_ttl( - &ad.placement_id, - ad.ad_type, - ad.ad_body, - &Duration::from_secs(1), - ) + .store_with_ttl(ad.clone(), &Duration::from_secs(1)) .unwrap(); let (c2, e2, t2) = fetch_timestamps(&store, &ad.placement_id); assert_eq!(t2, 1); @@ -433,20 +402,10 @@ mod tests { let ad_fresh = create_test_raw_ad("mock_billboard_2", None); store - .store_with_ttl( - &ad_exp.placement_id, - ad_exp.ad_type, - ad_exp.ad_body, - &Duration::from_secs(1), - ) + .store_with_ttl(ad_exp.clone(), &Duration::from_secs(1)) .unwrap(); store - .store_with_ttl( - &ad_fresh.placement_id, - ad_fresh.ad_type, - ad_fresh.ad_body, - &Duration::from_secs(10), - ) + .store_with_ttl(ad_fresh.clone(), &Duration::from_secs(10)) .unwrap(); assert!(store.lookup(&ad_exp.placement_id).unwrap().is_some()); @@ -469,12 +428,7 @@ mod tests { let ad = create_test_raw_ad("mock_billboard_1", None); store - .store_with_ttl( - &ad.placement_id, - ad.ad_type, - ad.ad_body, - &Duration::from_secs(1), - ) + .store_with_ttl(ad.clone(), &Duration::from_secs(1)) .unwrap(); store.clock.advance(2); assert!(store.lookup(&ad.placement_id).unwrap().is_some()); @@ -489,12 +443,7 @@ mod tests { let ad = create_test_raw_ad("mock_billboard_1", None); store - .store_with_ttl( - &ad.placement_id, - ad.ad_type, - ad.ad_body, - &Duration::from_secs(0), - ) + .store_with_ttl(ad.clone(), &Duration::from_secs(0)) .unwrap(); assert!(store.lookup(&ad.placement_id).unwrap().is_some()); @@ -510,12 +459,7 @@ mod tests { let ad = create_test_raw_ad("mock_billboard_1", None); store - .store_with_ttl( - &ad.placement_id, - ad.ad_type, - ad.ad_body.clone(), - &Duration::from_secs(300), - ) + .store_with_ttl(ad.clone(), &Duration::from_secs(300)) .unwrap(); let retrieved = store.lookup(&ad.placement_id).unwrap().unwrap(); @@ -528,12 +472,7 @@ mod tests { let ad = create_test_raw_ad("mock_billboard_1", Some(b"test response".to_vec())); store - .store_with_ttl( - &ad.placement_id, - ad.ad_type, - ad.ad_body, - &Duration::from_secs(300), - ) + .store_with_ttl(ad.clone(), &Duration::from_secs(300)) .unwrap(); let retrieved = store.lookup(&ad.placement_id).unwrap().unwrap(); @@ -556,12 +495,7 @@ mod tests { let large_body = vec![0u8; 300]; let ad = create_test_raw_ad(&format!("mock_billboard_{i}"), Some(large_body)); store - .store_with_ttl( - &ad.placement_id, - ad.ad_type, - ad.ad_body, - &Duration::from_secs(300), - ) + .store_with_ttl(ad.clone(), &Duration::from_secs(300)) .unwrap(); } @@ -581,22 +515,12 @@ mod tests { let ad_1 = create_test_raw_ad("mock_billboard_1", None); store - .store_with_ttl( - &ad_1.placement_id, - ad_1.ad_type, - ad_1.ad_body, - &Duration::from_secs(300), - ) + .store_with_ttl(ad_1.clone(), &Duration::from_secs(300)) .unwrap(); let ad_2 = create_test_raw_ad("mock_billboard_2", None); store - .store_with_ttl( - &ad_2.placement_id, - ad_2.ad_type, - ad_2.ad_body, - &Duration::from_secs(300), - ) + .store_with_ttl(ad_2.clone(), &Duration::from_secs(300)) .unwrap(); assert!(store.lookup(&ad_1.placement_id).unwrap().is_some()); @@ -617,20 +541,10 @@ mod tests { let ad_2 = create_test_raw_ad("mock_billboard_2", None); store - .store_with_ttl( - &ad_1.placement_id, - ad_1.ad_type, - ad_1.ad_body, - &Duration::from_secs(300), - ) + .store_with_ttl(ad_1.clone(), &Duration::from_secs(300)) .unwrap(); store - .store_with_ttl( - &ad_2.placement_id, - ad_2.ad_type, - ad_2.ad_body, - &Duration::from_secs(300), - ) + .store_with_ttl(ad_2.clone(), &Duration::from_secs(300)) .unwrap(); assert!(store.lookup(&ad_1.placement_id).unwrap().is_some()); diff --git a/components/ads-client/src/mars/ad_response.rs b/components/ads-client/src/mars/ad_response.rs index 13542fe00da..8b5196e139a 100644 --- a/components/ads-client/src/mars/ad_response.rs +++ b/components/ads-client/src/mars/ad_response.rs @@ -142,7 +142,7 @@ pub struct AdTile { pub url: String, } -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct StorableAd { pub placement_id: PlacementId, pub ad_type: StorableAdType, From 66ffa6f822da35d1cd37bd8a471421e967af1501 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 27 Aug 2026 18:02:24 -0700 Subject: [PATCH 04/11] fix: adds msising telemetry --- components/ads-client/src/client.rs | 1 - components/ads-client/src/ffi/telemetry.rs | 12 ++++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index 059081b5954..1626788ed4e 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -91,7 +91,6 @@ where match AdsStore::builder(x.db_path).build() { Ok(store) => Some(store), Err(e) => { - // TODO: Telemetry needs to work telemetry.record(&e); None } diff --git a/components/ads-client/src/ffi/telemetry.rs b/components/ads-client/src/ffi/telemetry.rs index 3fc7bc8c277..b4aad658cda 100644 --- a/components/ads-client/src/ffi/telemetry.rs +++ b/components/ads-client/src/ffi/telemetry.rs @@ -8,6 +8,7 @@ use std::sync::Arc; use parking_lot::RwLock; +use crate::ads_store::builder::AdsStoreBuilderError; use crate::client::error::RequestAdsError; use crate::client::ClientOperationEvent; use crate::http_cache::{CacheOutcome, HttpCacheBuilderError}; @@ -102,6 +103,17 @@ impl Telemetry for MozAdsTelemetryWrapper { }); return; } + if let Some(cache_builder_error) = event.downcast_ref::() { + inner.record_build_cache_error( + match cache_builder_error { + AdsStoreBuilderError::EmptyDbPath => "store_empty_db_path".to_string(), + AdsStoreBuilderError::Database(_) => "store_database_error".to_string(), + AdsStoreBuilderError::InvalidMaxSize { .. } => "store_invalid_max_size".to_string(), + }, + format!("{}", cache_builder_error), + ); + return; + } if let Some(cache_builder_error) = event.downcast_ref::() { inner.record_build_cache_error( match cache_builder_error { From d44acb5651de79fdf29d96788c02fd7455e9ed25 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 27 Aug 2026 18:50:49 -0700 Subject: [PATCH 05/11] fix: remove ttl --- components/ads-client/src/ads_store.rs | 15 +- .../src/ads_store/connection_initializer.rs | 7 +- components/ads-client/src/ads_store/store.rs | 193 ++---------------- components/ads-client/src/client.rs | 19 +- components/ads-client/src/ffi/telemetry.rs | 5 +- 5 files changed, 34 insertions(+), 205 deletions(-) diff --git a/components/ads-client/src/ads_store.rs b/components/ads-client/src/ads_store.rs index 6c9e6f9bbc0..aeffa190ea3 100644 --- a/components/ads-client/src/ads_store.rs +++ b/components/ads-client/src/ads_store.rs @@ -93,10 +93,7 @@ mod tests { ad_body: serde_json::to_vec(&ad).unwrap(), }; - store - .holder - .store_with_ttl(ad.clone(), &Duration::from_secs(300)) - .unwrap(); + store.holder.store_ad(ad.clone()).unwrap(); // Verify it's cached let retrieved = store.holder.lookup(&ad.placement_id).unwrap(); @@ -140,15 +137,9 @@ mod tests { ad_body: serde_json::to_vec(&ad).unwrap(), }; - store - .holder - .store_with_ttl(ad_1.clone(), &Duration::from_secs(300)) - .unwrap(); + store.holder.store_ad(ad_1.clone()).unwrap(); - store - .holder - .store_with_ttl(ad_2.clone(), &Duration::from_secs(300)) - .unwrap(); + store.holder.store_ad(ad_2.clone()).unwrap(); assert!(store.holder.lookup(&ad_1.placement_id).unwrap().is_some()); assert!(store.holder.lookup(&ad_2.placement_id).unwrap().is_some()); diff --git a/components/ads-client/src/ads_store/connection_initializer.rs b/components/ads-client/src/ads_store/connection_initializer.rs index 6d1ffd5eb76..14b6d776461 100644 --- a/components/ads-client/src/ads_store/connection_initializer.rs +++ b/components/ads-client/src/ads_store/connection_initializer.rs @@ -21,17 +21,14 @@ impl open_database::ConnectionInitializer for HttpCacheConnectionInitializer { fn init(&self, tx: &rusqlite::Transaction<'_>) -> open_database::Result<()> { const SCHEMA: &str = " CREATE TABLE IF NOT EXISTS ads ( - cached_at INTEGER NOT NULL, - expires_at INTEGER NOT NULL, + stored_at INTEGER NOT NULL, placement_id TEXT NOT NULL, ad_type SMALLINT NOT NULL, ad_body BLOB NOT NULL, size_bytes INTEGER NOT NULL, - ttl_seconds INTEGER NOT NULL, PRIMARY KEY (placement_id) ); - CREATE INDEX IF NOT EXISTS idx_ads_cached_at ON ads(cached_at); - CREATE INDEX IF NOT EXISTS idx_ads_expires_at ON ads(expires_at); + CREATE INDEX IF NOT EXISTS idx_ads_stored_at ON ads(stored_at); CREATE INDEX IF NOT EXISTS idx_ads_placement_id ON ads(placement_id); "; // If the schema fails to initialize, it might be corrupted or outdated so we drop the table and try again diff --git a/components/ads-client/src/ads_store/store.rs b/components/ads-client/src/ads_store/store.rs index bf784025999..57b5904d013 100644 --- a/components/ads-client/src/ads_store/store.rs +++ b/components/ads-client/src/ads_store/store.rs @@ -2,7 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use std::{sync::Arc, time::Duration}; +use std::sync::Arc; use crate::{ ads_store::PlacementId, @@ -105,7 +105,6 @@ impl AdsStoreHolder { return Err(Self::forced_fault_error("forced lookup failure")); } let conn = self.conn.lock(); - // TODO: Should we use body or explicit fields? conn.query_row( "SELECT placement_id, ad_type, ad_body FROM ads WHERE placement_id = ?1", @@ -138,7 +137,7 @@ impl AdsStoreHolder { /// Calling this method will always store an object regardless of headers or policy. /// Logic to determine the correct ttl or cache/no-cache should happen before calling this. /// TODO: maybe this should take a raw ad? maybe no need for raw ad at all? - pub fn store_with_ttl(&self, ad: StorableAd, ttl: &Duration) -> SqliteResult<()> { + pub fn store_ad(&self, ad: StorableAd) -> SqliteResult<()> { #[cfg(test)] if *self.fault.lock() == FaultKind::Store { return Err(Self::forced_fault_error("forced store failure")); @@ -148,36 +147,28 @@ impl AdsStoreHolder { // TODO: is it actually 8 bytes? https://stackoverflow.com/questions/2761563/what-is-the-difference-between-related-sqlite-data-types-like-int-integer-smal let size_bytes = (placement_id_str.chars().count() + 8 + ad.ad_body.len()) as i64; let now = self.clock.now_epoch_seconds(); - let ttl_seconds = ttl.as_secs(); - let expires_at = now + ttl_seconds as i64; let conn = self.conn.lock(); conn.execute( "INSERT INTO ads ( - cached_at, - expires_at, + stored_at, placement_id, ad_type, ad_body, - size_bytes, - ttl_seconds + size_bytes ) - VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7) + VALUES (?1, ?2, ?3, ?4, ?5) ON CONFLICT(placement_id) DO UPDATE SET - cached_at=excluded.cached_at, - expires_at=excluded.expires_at, + stored_at=excluded.stored_at, ad_type=excluded.ad_type, ad_body=excluded.ad_body, - size_bytes=excluded.size_bytes, - ttl_seconds=excluded.ttl_seconds", + size_bytes=excluded.size_bytes", params![ now, - expires_at, placement_id_str, ad.ad_type.to_u8(), ad.ad_body, size_bytes, - ttl_seconds as i64, ], )?; Ok(()) @@ -204,7 +195,7 @@ impl AdsStoreHolder { let conn = self.conn.lock(); conn.execute( "DELETE FROM ads WHERE rowid IN ( - SELECT rowid FROM ads ORDER BY cached_at ASC LIMIT 1 + SELECT rowid FROM ads ORDER BY stored_at ASC LIMIT 1 )", [], )?; @@ -237,28 +228,8 @@ mod tests { mars::ad_response::{AdCallbacks, AdImage}, }; use sql_support::open_database; - use std::time::Duration; use url::Url; - fn fetch_timestamps(store: &AdsStoreHolder, placement_id: &PlacementId) -> (i64, i64, i64) { - let conn = store.conn.lock(); - conn.query_row( - "SELECT - cached_at, - expires_at, - COALESCE(ttl_seconds, -1) - FROM ads WHERE placement_id = ?1", - rusqlite::params![&placement_id.as_ref()], - |row| { - let cached_at: i64 = row.get(0)?; - let expires_at: i64 = row.get(1)?; - let ttl: i64 = row.get(2)?; - Ok((cached_at, expires_at, ttl)) - }, - ) - .expect("row should exist") - } - // Create a sample ad for tests. The body defaults to an example serialized AdImage (if body is None). fn create_test_raw_ad(placement_id: &str, body: Option>) -> StorableAd { let base_url = mockito::server_url(); @@ -311,9 +282,7 @@ mod tests { let ad = create_test_raw_ad("mock_billboard_1", None); - let err = store - .store_with_ttl(ad, &Duration::from_secs(300)) - .unwrap_err(); + let err = store.store_ad(ad).unwrap_err(); match err { rusqlite::Error::SqliteFailure(_, Some(msg)) => { assert!(msg.contains("forced store failure")); @@ -328,7 +297,7 @@ mod tests { store.set_fault(FaultKind::Trim); let ad = create_test_raw_ad("mock_billboard_1", None); - store.store_with_ttl(ad, &Duration::from_secs(300)).unwrap(); + store.store_ad(ad).unwrap(); let err = store.trim_to_max_size(&ByteSize::b(1)).unwrap_err(); match err { @@ -353,137 +322,17 @@ mod tests { } } - #[test] - fn test_store_ads_with_ttl_sets_fields_consistently() { - let store = create_test_store(); - let ad = create_test_raw_ad("mock_billboard_1", None); - let placement_id = ad.placement_id.clone(); - let ttl = Duration::from_secs(5); - store.store_with_ttl(ad, &ttl).unwrap(); - - let (cached_at, expires_at, ttl_seconds) = fetch_timestamps(&store, &placement_id); - assert_eq!(ttl_seconds, ttl.as_secs() as i64); - let diff = expires_at - cached_at; - let ttl_seconds = ttl.as_secs(); - assert!( - (diff == ttl_seconds as i64) - || (diff == ttl_seconds as i64 - 1) - || (diff == ttl_seconds as i64 + 1), - "unexpected expires_at diff: got {diff}, want ~{ttl_seconds}" - ); - } - - #[test] - fn test_upsert_ads_refreshes_ttl_and_expiry() { - let store = create_test_store(); - let ad = create_test_raw_ad("mock_billboard_1", None); - store - .store_with_ttl(ad.clone(), &Duration::from_secs(300)) - .unwrap(); - let (c1, e1, t1) = fetch_timestamps(&store, &ad.placement_id); - assert_eq!(t1, 300); - - store.get_clock().advance(3); - - store - .store_with_ttl(ad.clone(), &Duration::from_secs(1)) - .unwrap(); - let (c2, e2, t2) = fetch_timestamps(&store, &ad.placement_id); - assert_eq!(t2, 1); - assert!(c2 > c1); - assert!(e2 < e1, "expires_at should move earlier when TTL shrinks"); - } - - #[test] - fn test_delete_expired_removes_only_expired_ads() { - let store = create_test_store(); - - let ad_exp = create_test_raw_ad("mock_billboard_1", None); - let ad_fresh = create_test_raw_ad("mock_billboard_2", None); - - store - .store_with_ttl(ad_exp.clone(), &Duration::from_secs(1)) - .unwrap(); - store - .store_with_ttl(ad_fresh.clone(), &Duration::from_secs(10)) - .unwrap(); - - assert!(store.lookup(&ad_exp.placement_id).unwrap().is_some()); - assert!(store.lookup(&ad_fresh.placement_id).unwrap().is_some()); - - store.clock.advance(2); - let removed = store.delete_expired_entries().unwrap(); - assert!( - removed >= 1, - "expected at least one expired row to be deleted" - ); - - assert!(store.lookup(&ad_exp.placement_id).unwrap().is_none()); - assert!(store.lookup(&ad_fresh.placement_id).unwrap().is_some()); - } - - #[test] - fn test_lookups_is_expired_agnostic() { - let store = create_test_store(); - let ad = create_test_raw_ad("mock_billboard_1", None); - - store - .store_with_ttl(ad.clone(), &Duration::from_secs(1)) - .unwrap(); - store.clock.advance(2); - assert!(store.lookup(&ad.placement_id).unwrap().is_some()); - - store.delete_expired_entries().unwrap(); - assert!(store.lookup(&ad.placement_id).unwrap().is_none()); - } - - #[test] - fn test_zero_ttl_expires_ads_immediately_after_tick() { - let store = create_test_store(); - let ad = create_test_raw_ad("mock_billboard_1", None); - - store - .store_with_ttl(ad.clone(), &Duration::from_secs(0)) - .unwrap(); - assert!(store.lookup(&ad.placement_id).unwrap().is_some()); - - store.clock.advance(2); - let removed = store.delete_expired_entries().unwrap(); - assert!(removed >= 1); - assert!(store.lookup(&ad.placement_id).unwrap().is_none()); - } - #[test] fn test_store_and_retrieve_ads() { let store = create_test_store(); let ad = create_test_raw_ad("mock_billboard_1", None); - store - .store_with_ttl(ad.clone(), &Duration::from_secs(300)) - .unwrap(); + store.store_ad(ad.clone()).unwrap(); let retrieved = store.lookup(&ad.placement_id).unwrap().unwrap(); assert_eq!(retrieved.ad_body, ad.ad_body); } - #[test] - fn test_ttl_expiration_ads() { - let store = create_test_store(); - let ad = create_test_raw_ad("mock_billboard_1", Some(b"test response".to_vec())); - - store - .store_with_ttl(ad.clone(), &Duration::from_secs(300)) - .unwrap(); - - let retrieved = store.lookup(&ad.placement_id).unwrap().unwrap(); - assert_eq!(retrieved.ad_body, b"test response"); - - store.clock.advance(2); - - let retrieved_after_expiry = store.lookup(&ad.placement_id).unwrap(); - assert!(retrieved_after_expiry.is_some()); - } - #[test] fn test_max_size_eviction_ads() { let initializer = HttpCacheConnectionInitializer {}; @@ -494,9 +343,7 @@ mod tests { for i in 0..5 { let large_body = vec![0u8; 300]; let ad = create_test_raw_ad(&format!("mock_billboard_{i}"), Some(large_body)); - store - .store_with_ttl(ad.clone(), &Duration::from_secs(300)) - .unwrap(); + store.store_ad(ad.clone()).unwrap(); } store.trim_to_max_size(&ByteSize::kib(1)).unwrap(); @@ -514,14 +361,10 @@ mod tests { let store = create_test_store(); let ad_1 = create_test_raw_ad("mock_billboard_1", None); - store - .store_with_ttl(ad_1.clone(), &Duration::from_secs(300)) - .unwrap(); + store.store_ad(ad_1.clone()).unwrap(); let ad_2 = create_test_raw_ad("mock_billboard_2", None); - store - .store_with_ttl(ad_2.clone(), &Duration::from_secs(300)) - .unwrap(); + store.store_ad(ad_2.clone()).unwrap(); assert!(store.lookup(&ad_1.placement_id).unwrap().is_some()); assert!(store.lookup(&ad_2.placement_id).unwrap().is_some()); @@ -540,12 +383,8 @@ mod tests { let ad_1 = create_test_raw_ad("mock_billboard_1", None); let ad_2 = create_test_raw_ad("mock_billboard_2", None); - store - .store_with_ttl(ad_1.clone(), &Duration::from_secs(300)) - .unwrap(); - store - .store_with_ttl(ad_2.clone(), &Duration::from_secs(300)) - .unwrap(); + store.store_ad(ad_1.clone()).unwrap(); + store.store_ad(ad_2.clone()).unwrap(); assert!(store.lookup(&ad_1.placement_id).unwrap().is_some()); assert!(store.lookup(&ad_2.placement_id).unwrap().is_some()); diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index 1626788ed4e..41c6c22cb7c 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -87,15 +87,16 @@ where } }); - let ads_store = client_config.store_config.and_then(|x| { - match AdsStore::builder(x.db_path).build() { - Ok(store) => Some(store), - Err(e) => { - telemetry.record(&e); - None - } - } - }); + let ads_store = + client_config + .store_config + .and_then(|x| match AdsStore::builder(x.db_path).build() { + Ok(store) => Some(store), + Err(e) => { + telemetry.record(&e); + None + } + }); let client = MARSClient::new(environment, http_cache, telemetry.clone()); telemetry.record(&ClientOperationEvent::New); diff --git a/components/ads-client/src/ffi/telemetry.rs b/components/ads-client/src/ffi/telemetry.rs index b4aad658cda..63bbd9eaad1 100644 --- a/components/ads-client/src/ffi/telemetry.rs +++ b/components/ads-client/src/ffi/telemetry.rs @@ -21,7 +21,6 @@ pub trait MozAdsTelemetry: Send + Sync { fn record_client_error(&self, label: String, value: String); fn record_client_operation_total(&self, label: String); fn record_deserialization_error(&self, label: String, value: String); - // TODO: rename these fn record_http_cache_outcome(&self, label: String, value: String); } @@ -108,7 +107,9 @@ impl Telemetry for MozAdsTelemetryWrapper { match cache_builder_error { AdsStoreBuilderError::EmptyDbPath => "store_empty_db_path".to_string(), AdsStoreBuilderError::Database(_) => "store_database_error".to_string(), - AdsStoreBuilderError::InvalidMaxSize { .. } => "store_invalid_max_size".to_string(), + AdsStoreBuilderError::InvalidMaxSize { .. } => { + "store_invalid_max_size".to_string() + } }, format!("{}", cache_builder_error), ); From 276602acd10988ffea4fce51fe694d703cb7cc20 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 27 Aug 2026 19:09:46 -0700 Subject: [PATCH 06/11] fix: clippy --- components/ads-client/src/ads_store.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/components/ads-client/src/ads_store.rs b/components/ads-client/src/ads_store.rs index aeffa190ea3..51918853c52 100644 --- a/components/ads-client/src/ads_store.rs +++ b/components/ads-client/src/ads_store.rs @@ -55,8 +55,6 @@ impl AdsStore { #[cfg(test)] mod tests { - use std::time::Duration; - use super::*; use crate::mars::ad_response::{AdCallbacks, AdImage, StorableAd, StorableAdType}; use url::Url; From 9d4be84971152d8ac6ebb67898acee8b59fa5667 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 27 Aug 2026 19:22:17 -0700 Subject: [PATCH 07/11] fix: some cleanup --- components/ads-client/src/ads_store/store.rs | 37 +------------------- 1 file changed, 1 insertion(+), 36 deletions(-) diff --git a/components/ads-client/src/ads_store/store.rs b/components/ads-client/src/ads_store/store.rs index 57b5904d013..6a24304d3c0 100644 --- a/components/ads-client/src/ads_store/store.rs +++ b/components/ads-client/src/ads_store/store.rs @@ -22,7 +22,6 @@ pub enum FaultKind { Lookup, Store, Trim, - Cleanup, } pub struct AdsStoreHolder { @@ -82,23 +81,6 @@ impl AdsStoreHolder { Ok(ByteSize::b(size_bytes_ads)) } - /// Removes all entries from the store whose expires_at is at or before the current time. - pub fn delete_expired_entries(&self) -> SqliteResult { - #[cfg(test)] - if *self.fault.lock() == FaultKind::Cleanup { - return Err(Self::forced_fault_error("forced cleanup failure")); - } - let mut conn = self.conn.lock(); - let tx = conn.transaction()?; - let mut total = 0; - total += tx.execute( - "DELETE FROM ads WHERE expires_at <= ?1", - params![self.clock.now_epoch_seconds()], - )?; - tx.commit()?; - Ok(total) - } - /// Lookup is agnostic to expiration. If it exists in the store, it will return the result. pub fn lookup(&self, placement_id: &PlacementId) -> SqliteResult> { #[cfg(test)] if *self.fault.lock() == FaultKind::Lookup { @@ -133,10 +115,7 @@ impl AdsStoreHolder { .optional() } - /// Upsert an object into the store with an expires_at defined by the given ttl_seconds. - /// Calling this method will always store an object regardless of headers or policy. - /// Logic to determine the correct ttl or cache/no-cache should happen before calling this. - /// TODO: maybe this should take a raw ad? maybe no need for raw ad at all? + /// Upsert an object into the store. pub fn store_ad(&self, ad: StorableAd) -> SqliteResult<()> { #[cfg(test)] if *self.fault.lock() == FaultKind::Store { @@ -308,20 +287,6 @@ mod tests { } } - #[test] - fn test_cleanup_fault_injection() { - let store = create_test_store(); - store.set_fault(FaultKind::Cleanup); - - let err = store.delete_expired_entries().unwrap_err(); - match err { - rusqlite::Error::SqliteFailure(_, Some(msg)) => { - assert!(msg.contains("forced cleanup failure")); - } - other => panic!("unexpected error: {other:?}"), - } - } - #[test] fn test_store_and_retrieve_ads() { let store = create_test_store(); From af1f25d869baa6728e8186651d9ec0f98e3b5766 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 27 Aug 2026 19:25:20 -0700 Subject: [PATCH 08/11] fix: size --- components/ads-client/src/ads_store/store.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/components/ads-client/src/ads_store/store.rs b/components/ads-client/src/ads_store/store.rs index 6a24304d3c0..1241b4a6ca7 100644 --- a/components/ads-client/src/ads_store/store.rs +++ b/components/ads-client/src/ads_store/store.rs @@ -122,9 +122,7 @@ impl AdsStoreHolder { return Err(Self::forced_fault_error("forced store failure")); } let placement_id_str: &str = ad.placement_id.as_ref(); - // placement_id char count + u8 (ad_type) + body length - // TODO: is it actually 8 bytes? https://stackoverflow.com/questions/2761563/what-is-the-difference-between-related-sqlite-data-types-like-int-integer-smal - let size_bytes = (placement_id_str.chars().count() + 8 + ad.ad_body.len()) as i64; + let size_bytes = ad.ad_body.len() as i64; let now = self.clock.now_epoch_seconds(); let conn = self.conn.lock(); From 1254bc543e8f886f7c41fed5dbf7535cce543cec Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 27 Aug 2026 19:28:17 -0700 Subject: [PATCH 09/11] fix: extracted bytesize and clock --- components/ads-client/src/{http_cache => database}/bytesize.rs | 0 components/ads-client/src/{http_cache => database}/clock.rs | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename components/ads-client/src/{http_cache => database}/bytesize.rs (100%) rename components/ads-client/src/{http_cache => database}/clock.rs (100%) diff --git a/components/ads-client/src/http_cache/bytesize.rs b/components/ads-client/src/database/bytesize.rs similarity index 100% rename from components/ads-client/src/http_cache/bytesize.rs rename to components/ads-client/src/database/bytesize.rs diff --git a/components/ads-client/src/http_cache/clock.rs b/components/ads-client/src/database/clock.rs similarity index 100% rename from components/ads-client/src/http_cache/clock.rs rename to components/ads-client/src/database/clock.rs From 998c2119208e7c2cf8b57a28e455373b0f7f4466 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 27 Aug 2026 19:34:02 -0700 Subject: [PATCH 10/11] fix: extracts bytesize and clock --- .../ads-client/integration-tests/tests/http_cache.rs | 3 ++- components/ads-client/src/ads_store.rs | 2 +- components/ads-client/src/ads_store/builder.rs | 2 +- components/ads-client/src/ads_store/store.rs | 12 +++++------- components/ads-client/src/client.rs | 3 ++- components/ads-client/src/database.rs | 2 ++ components/ads-client/src/http_cache.rs | 5 +---- components/ads-client/src/http_cache/builder.rs | 2 +- components/ads-client/src/http_cache/store.rs | 9 ++++----- components/ads-client/src/lib.rs | 1 + components/ads-client/src/mars.rs | 6 +++--- 11 files changed, 23 insertions(+), 24 deletions(-) create mode 100644 components/ads-client/src/database.rs diff --git a/components/ads-client/integration-tests/tests/http_cache.rs b/components/ads-client/integration-tests/tests/http_cache.rs index 8651c07712f..3e646122501 100644 --- a/components/ads-client/integration-tests/tests/http_cache.rs +++ b/components/ads-client/integration-tests/tests/http_cache.rs @@ -6,7 +6,8 @@ use std::hash::{Hash, Hasher}; use std::time::Duration; -use ads_client::http_cache::{ByteSize, CacheOutcome, CachePolicy, HttpCache}; +use ads_client::database::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 51918853c52..34c44331ad3 100644 --- a/components/ads-client/src/ads_store.rs +++ b/components/ads-client/src/ads_store.rs @@ -4,7 +4,7 @@ pub mod store; use crate::{ ads_store::{builder::AdsStoreBuilder, store::AdsStoreHolder}, - http_cache::ByteSize, + database::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 9c606d3dd24..f2f429cc4f1 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::HttpCacheConnectionInitializer; use crate::ads_store::store::AdsStoreHolder; use crate::ads_store::AdsStore; -use crate::http_cache::ByteSize; +use crate::database::bytesize::ByteSize; use rusqlite::Connection; use sql_support::open_database; use std::path::PathBuf; diff --git a/components/ads-client/src/ads_store/store.rs b/components/ads-client/src/ads_store/store.rs index 1241b4a6ca7..657defaf8b2 100644 --- a/components/ads-client/src/ads_store/store.rs +++ b/components/ads-client/src/ads_store/store.rs @@ -2,18 +2,16 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use std::sync::Arc; - +use crate::database::bytesize::ByteSize; +use crate::database::clock::Clock; use crate::{ ads_store::PlacementId, - http_cache::{ - clock::{CacheClock, Clock}, - ByteSize, - }, + database::clock::CacheClock, mars::ad_response::{StorableAd, StorableAdType}, }; use parking_lot::Mutex; use rusqlite::{params, Connection, OptionalExtension, Result as SqliteResult}; +use std::sync::Arc; #[cfg(test)] #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -48,7 +46,7 @@ impl AdsStoreHolder { #[cfg(test)] pub fn new_with_test_clock(conn: Connection) -> Self { - use crate::http_cache::clock::TestClock; + use crate::database::clock::TestClock; Self { conn: Mutex::new(conn), diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index 41c6c22cb7c..a80519f3486 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -7,7 +7,8 @@ use std::collections::HashMap; use std::time::Duration; use crate::ads_store::AdsStore; -use crate::http_cache::{ByteSize, CachePolicy, HttpCache}; +use crate::database::bytesize::ByteSize; +use crate::http_cache::{CachePolicy, HttpCache}; use crate::mars::ad_request::{AdPlacementRequest, AdRequestFlags}; use crate::mars::ad_response::{AdImage, AdResponse, AdResponseValue, AdSpoc, AdTile}; use crate::mars::error::{RecordClickError, RecordImpressionError, ReportAdError}; diff --git a/components/ads-client/src/database.rs b/components/ads-client/src/database.rs new file mode 100644 index 00000000000..9bf27276888 --- /dev/null +++ b/components/ads-client/src/database.rs @@ -0,0 +1,2 @@ +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 fb297c8fb07..2b4256139d0 100644 --- a/components/ads-client/src/http_cache.rs +++ b/components/ads-client/src/http_cache.rs @@ -3,10 +3,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ mod builder; -mod bytesize; mod cache_control; -// TODO: Remove this -pub mod clock; mod connection_initializer; mod outcome; mod request_hash; @@ -14,6 +11,7 @@ mod store; mod strategy; mod ttl; +use crate::database::bytesize::ByteSize; use self::{ builder::HttpCacheBuilder, store::HttpCacheStore, @@ -24,7 +22,6 @@ use std::hash::Hash; use viaduct::{Client, Request, Response}; pub use self::builder::HttpCacheBuilderError; -pub use self::bytesize::ByteSize; pub use self::outcome::CacheOutcome; pub use self::request_hash::RequestHash; use std::path::Path; diff --git a/components/ads-client/src/http_cache/builder.rs b/components/ads-client/src/http_cache/builder.rs index ac4e7615bc2..5af4a610616 100644 --- a/components/ads-client/src/http_cache/builder.rs +++ b/components/ads-client/src/http_cache/builder.rs @@ -2,9 +2,9 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use super::bytesize::ByteSize; use super::connection_initializer::HttpCacheConnectionInitializer; use super::store::HttpCacheStore; +use crate::database::bytesize::ByteSize; use crate::http_cache::HttpCache; use rusqlite::Connection; use sql_support::open_database; diff --git a/components/ads-client/src/http_cache/store.rs b/components/ads-client/src/http_cache/store.rs index a80f5a956ca..ae1bcf36bb4 100644 --- a/components/ads-client/src/http_cache/store.rs +++ b/components/ads-client/src/http_cache/store.rs @@ -4,10 +4,9 @@ use std::{collections::HashMap, sync::Arc, time::Duration}; -use crate::http_cache::{ - clock::{CacheClock, Clock}, - request_hash::RequestHash, - ByteSize, +use crate::{ + database::clock::{CacheClock, Clock}, + http_cache::{request_hash::RequestHash, ByteSize}, }; use parking_lot::Mutex; use rusqlite::{params, Connection, OptionalExtension, Result as SqliteResult}; @@ -47,7 +46,7 @@ impl HttpCacheStore { #[cfg(test)] pub fn new_with_test_clock(conn: Connection) -> Self { - use crate::http_cache::clock::TestClock; + use crate::database::clock::TestClock; Self { conn: Mutex::new(conn), diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 03e652df526..848975fa9fe 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -17,6 +17,7 @@ use http_cache::CachePolicy; use mars::ad_request::{AdPlacementRequest, AdRequestFlags}; pub mod ads_store; mod client; +pub mod database; 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 4519e0bf7e7..f26cba6de0e 100644 --- a/components/ads-client/src/mars.rs +++ b/components/ads-client/src/mars.rs @@ -259,7 +259,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::http_cache::ByteSize::mib(1)) + .max_size(crate::database::bytesize::ByteSize::mib(1)) .build() .unwrap(); let client = make_test_client(Some(cache)); @@ -295,7 +295,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::http_cache::ByteSize::mib(1)) + .max_size(crate::database::bytesize::ByteSize::mib(1)) .build() .unwrap(); @@ -314,7 +314,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::http_cache::ByteSize::mib(1)) + .max_size(crate::database::bytesize::ByteSize::mib(1)) .build() .unwrap(); From 1aea4423d8c5bcb5474710e4af2d1461498e318f Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 27 Aug 2026 19:34:48 -0700 Subject: [PATCH 11/11] fix: clippy --- components/ads-client/src/http_cache.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/ads-client/src/http_cache.rs b/components/ads-client/src/http_cache.rs index 2b4256139d0..e09da1c828d 100644 --- a/components/ads-client/src/http_cache.rs +++ b/components/ads-client/src/http_cache.rs @@ -11,12 +11,12 @@ mod store; mod strategy; mod ttl; -use crate::database::bytesize::ByteSize; use self::{ builder::HttpCacheBuilder, store::HttpCacheStore, strategy::{CacheFirst, NetworkFirst}, }; +use crate::database::bytesize::ByteSize; use std::hash::Hash; use viaduct::{Client, Request, Response};