diff --git a/components/remote_settings/src/client.rs b/components/remote_settings/src/client.rs index d8dca71ff6..74bc2a2154 100644 --- a/components/remote_settings/src/client.rs +++ b/components/remote_settings/src/client.rs @@ -151,9 +151,15 @@ impl RemoteSettingsClient { .get_last_modified_timestamp(&collection_url)? .unwrap_or(0); if packaged_data.timestamp > cached_timestamp { - inner - .storage - .set_records(&collection_url, &packaged_data.data)?; + // Remove previously cached data (packaged data does not have tombstones). + inner.storage.empty()?; + // Insert new packaged data. + inner.storage.insert_collection_content( + &collection_url, + &packaged_data.data, + packaged_data.timestamp, + CollectionMetadata::default(), + )?; return Ok(Some(self.filter_records(packaged_data.data))); } } @@ -169,7 +175,12 @@ impl RemoteSettingsClient { // Case 3: sync_if_empty=true (None, true) => { let changeset = inner.api_client.fetch_changeset(None)?; - inner.storage.set_records(&collection_url, &changeset.changes)?; + inner.storage.insert_collection_content( + &collection_url, + &changeset.changes, + changeset.timestamp, + changeset.metadata, + )?; Some(self.filter_records(changeset.changes)) } // Case 4: Nothing to return @@ -182,7 +193,12 @@ impl RemoteSettingsClient { let collection_url = inner.api_client.collection_url(); let mtime = inner.storage.get_last_modified_timestamp(&collection_url)?; let changeset = inner.api_client.fetch_changeset(mtime)?; - inner.storage.merge_records(&collection_url, &changeset.changes) + inner.storage.insert_collection_content( + &collection_url, + &changeset.changes, + changeset.timestamp, + changeset.metadata, + ) } /// Downloads an attachment from [attachment_location]. NOTE: there are no guarantees about a @@ -1795,7 +1811,7 @@ mod jexl_tests { }; let mut storage = Storage::new(":memory:".into()).expect("Error creating storage"); - let _ = storage.set_collection_content( + let _ = storage.insert_collection_content( "http://rs.example.com/v1/buckets/main/collections/test-collection", &records, 42, @@ -1853,7 +1869,7 @@ mod jexl_tests { }; let mut storage = Storage::new(":memory:".into()).expect("Error creating storage"); - let _ = storage.set_collection_content( + let _ = storage.insert_collection_content( "http://rs.example.com/v1/buckets/main/collections/test-collection", &records, 42, @@ -1940,7 +1956,12 @@ mod cached_data_tests { let mut api_client = MockApiClient::new(); let mut storage = Storage::new(":memory:".into())?; - storage.set_records(collection_url, &vec![old_record.clone()])?; + storage.insert_collection_content( + collection_url, + &vec![old_record.clone()], + 42, + CollectionMetadata::default(), + )?; api_client .expect_collection_url() @@ -2095,7 +2116,12 @@ mod cached_data_tests { attachment: None, fields: serde_json::Map::new(), }]; - storage.set_records(&collection_url, &cached_records)?; + storage.insert_collection_content( + &collection_url, + &cached_records, + 42, + CollectionMetadata::default(), + )?; api_client .expect_collection_url() @@ -2131,7 +2157,12 @@ mod cached_data_tests { // Set up empty cached records let cached_records: Vec = vec![]; - storage.set_records(&collection_url, &cached_records)?; + storage.insert_collection_content( + &collection_url, + &cached_records, + 42, + CollectionMetadata::default(), + )?; api_client .expect_collection_url() diff --git a/components/remote_settings/src/storage.rs b/components/remote_settings/src/storage.rs index 74cc32ac28..2da5f05024 100644 --- a/components/remote_settings/src/storage.rs +++ b/components/remote_settings/src/storage.rs @@ -2,7 +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::{Attachment, RemoteSettingsRecord, Result}; +use crate::{ + client::CollectionMetadata, client::CollectionSignature, Attachment, RemoteSettingsRecord, + Result, +}; use camino::Utf8PathBuf; use rusqlite::{params, Connection, OptionalExtension, Transaction}; use serde_json; @@ -34,7 +37,7 @@ impl Storage { Ok(storage) } - // Create the different tables for records and attachements for every new sqlite path + // Create the different tables for records and attachments for every new sqlite path fn initialize_database(&self) -> Result<()> { self.conn.execute_batch( " @@ -50,7 +53,10 @@ impl Storage { ); CREATE TABLE IF NOT EXISTS collection_metadata ( collection_url TEXT PRIMARY KEY, + bucket TEXT, last_modified INTEGER, + signature TEXT, + x5u TEXT, fetched BOOLEAN ); ", @@ -61,7 +67,7 @@ impl Storage { /// Get the last modified timestamp for the stored records /// /// Returns None if no records are stored or if `collection_url` does not match the - /// last `collection_url` passed to `set_records` / `merge_records` + /// last `collection_url` passed to `insert_collection_content` pub fn get_last_modified_timestamp(&self, collection_url: &str) -> Result> { let mut stmt = self .conn @@ -75,7 +81,7 @@ impl Storage { /// Get cached records for this collection /// /// Returns None if no records are stored or if `collection_url` does not match the `collection_url` passed - /// to `set_records`. + /// to `insert_collection_content`. pub fn get_records( &mut self, collection_url: &str, @@ -105,6 +111,36 @@ impl Storage { result } + /// Get cached content for this collection + /// + /// Returns None if no data is stored or if `collection_url` does not match the `collection_url` passed + /// to `insert_collection_content`. + pub fn get_collection_metadata( + &self, + collection_url: &str, + ) -> Result> { + let mut stmt_metadata = self.conn.prepare( + "SELECT bucket, signature, x5u FROM collection_metadata WHERE collection_url = ?", + )?; + + if let Some(metadata) = stmt_metadata + .query_row(params![collection_url], |row| { + Ok(CollectionMetadata { + bucket: row.get(0).unwrap_or_default(), + signature: CollectionSignature { + signature: row.get(1).unwrap_or_default(), + x5u: row.get(2).unwrap_or_default(), + }, + }) + }) + .optional()? + { + Ok(Some(metadata)) + } else { + Ok(None) + } + } + /// Get cached attachment data /// /// This returns the last attachment data sent to [Self::set_attachment]. @@ -140,30 +176,13 @@ impl Storage { } } - /// Set the list of records stored in the database, clearing out any previously stored records - pub fn set_records( - &mut self, - collection_url: &str, - records: &[RemoteSettingsRecord], - ) -> Result<()> { - let tx = self.conn.transaction()?; - - tx.execute("DELETE FROM records", [])?; - tx.execute("DELETE FROM collection_metadata", [])?; - let max_last_modified = Self::update_record_rows(&tx, collection_url, records)?; - Self::update_collection_metadata(&tx, collection_url, max_last_modified)?; - tx.commit()?; - Ok(()) - } - - /// Merge new records with records stored in the database - /// - /// Records with `deleted=false` will be inserted into the DB, replacing any previously stored - /// records with the same ID. Records with `deleted=true` will be removed. - pub fn merge_records( + /// Set cached content for this collection. + pub fn insert_collection_content( &mut self, collection_url: &str, records: &[RemoteSettingsRecord], + timestamp: u64, + metadata: CollectionMetadata, ) -> Result<()> { let tx = self.conn.transaction()?; @@ -179,8 +198,9 @@ impl Storage { "DELETE FROM collection_metadata where collection_url <> ?", [collection_url], )?; - let max_last_modified = Self::update_record_rows(&tx, collection_url, records)?; - Self::update_collection_metadata(&tx, collection_url, max_last_modified)?; + + Self::update_record_rows(&tx, collection_url, records)?; + Self::update_collection_metadata(&tx, collection_url, timestamp, metadata)?; tx.commit()?; Ok(()) } @@ -218,12 +238,20 @@ impl Storage { tx: &Transaction<'_>, collection_url: &str, last_modified: u64, + metadata: CollectionMetadata, ) -> Result<()> { // Update the metadata - let fetched = true; tx.execute( - "INSERT OR REPLACE INTO collection_metadata (collection_url, last_modified, fetched) VALUES (?, ?, ?)", - (collection_url, last_modified, fetched), + "INSERT OR REPLACE INTO collection_metadata \ + (collection_url, last_modified, bucket, signature, x5u, fetched) \ + VALUES (?, ?, ?, ?, ?, true)", + ( + collection_url, + last_modified, + metadata.bucket, + metadata.signature.signature, + metadata.signature.x5u, + ), )?; Ok(()) } @@ -271,7 +299,10 @@ impl Storage { #[cfg(test)] mod tests { use super::Storage; - use crate::{Attachment, RemoteSettingsRecord, Result, RsJsonObject}; + use crate::{ + client::CollectionMetadata, client::CollectionSignature, Attachment, RemoteSettingsRecord, + Result, RsJsonObject, + }; use sha2::{Digest, Sha256}; #[test] @@ -303,7 +334,12 @@ mod tests { ]; // Set records - storage.set_records(collection_url, &records)?; + storage.insert_collection_content( + collection_url, + &records, + 300, + CollectionMetadata::default(), + )?; // Get records let fetched_records = storage.get_records(collection_url)?; @@ -316,7 +352,7 @@ mod tests { // Get last modified timestamp let last_modified = storage.get_last_modified_timestamp(collection_url)?; - assert_eq!(last_modified, Some(200)); + assert_eq!(last_modified, Some(300)); Ok(()) } @@ -345,7 +381,12 @@ mod tests { let collection_url = "https://example.com/api"; // Set empty records - storage.set_records(collection_url, &Vec::::default())?; + storage.insert_collection_content( + collection_url, + &Vec::::default(), + 42, + CollectionMetadata::default(), + )?; // Get records let fetched_records = storage.get_records(collection_url)?; @@ -353,7 +394,7 @@ mod tests { // Get last modified timestamp when no records let last_modified = storage.get_last_modified_timestamp(collection_url)?; - assert_eq!(last_modified, Some(0)); + assert_eq!(last_modified, Some(42)); Ok(()) } @@ -540,7 +581,12 @@ mod tests { .expect("No attachment metadata for record"); // Set records and attachment - storage.set_records(collection_url, &records)?; + storage.insert_collection_content( + collection_url, + &records, + 42, + CollectionMetadata::default(), + )?; storage.set_attachment(collection_url, &metadata.location, attachment)?; // Verify they are stored @@ -589,8 +635,12 @@ mod tests { }]; // Set records for collection_url1 - storage.set_records(collection_url1, &records_collection_url1)?; - + storage.insert_collection_content( + collection_url1, + &records_collection_url1, + 42, + CollectionMetadata::default(), + )?; // Verify records for collection_url1 let fetched_records = storage.get_records(collection_url1)?; assert!(fetched_records.is_some()); @@ -599,7 +649,12 @@ mod tests { assert_eq!(fetched_records, records_collection_url1); // Set records for collection_url2, which will clear records for all collections - storage.set_records(collection_url2, &records_collection_url2)?; + storage.insert_collection_content( + collection_url2, + &records_collection_url2, + 300, + CollectionMetadata::default(), + )?; // Verify that records for collection_url1 have been cleared let fetched_records = storage.get_records(collection_url1)?; @@ -616,13 +671,13 @@ mod tests { let last_modified1 = storage.get_last_modified_timestamp(collection_url1)?; assert_eq!(last_modified1, None); let last_modified2 = storage.get_last_modified_timestamp(collection_url2)?; - assert_eq!(last_modified2, Some(200)); + assert_eq!(last_modified2, Some(300)); Ok(()) } #[test] - fn test_storage_set_records() -> Result<()> { + fn test_storage_insert_collection_content() -> Result<()> { let mut storage = Storage::new(":memory:".into())?; let collection_url = "https://example.com/api"; @@ -638,7 +693,12 @@ mod tests { }]; // Set initial records - storage.set_records(collection_url, &initial_records)?; + storage.insert_collection_content( + collection_url, + &initial_records, + 42, + CollectionMetadata::default(), + )?; // Verify initial records let fetched_records = storage.get_records(collection_url)?; @@ -656,7 +716,12 @@ mod tests { .unwrap() .clone(), }]; - storage.set_records(collection_url, &updated_records)?; + storage.insert_collection_content( + collection_url, + &updated_records, + 300, + CollectionMetadata::default(), + )?; // Verify updated records let fetched_records = storage.get_records(collection_url)?; @@ -665,7 +730,7 @@ mod tests { // Verify last modified timestamp let last_modified = storage.get_last_modified_timestamp(collection_url)?; - assert_eq!(last_modified, Some(200)); + assert_eq!(last_modified, Some(300)); Ok(()) } @@ -759,14 +824,24 @@ mod tests { ]; // Set initial records - storage.set_records(collection_url, &initial_records)?; + storage.insert_collection_content( + collection_url, + &initial_records, + 1000, + CollectionMetadata::default(), + )?; // Verify initial records let fetched_records = storage.get_records(collection_url)?.unwrap(); assert_eq!(fetched_records, initial_records); // Update records - storage.merge_records(collection_url, &updated_records)?; + storage.insert_collection_content( + collection_url, + &updated_records, + 1300, + CollectionMetadata::default(), + )?; // Verify updated records let mut fetched_records = storage.get_records(collection_url)?.unwrap(); @@ -776,6 +851,42 @@ mod tests { // Verify last modified timestamp let last_modified = storage.get_last_modified_timestamp(collection_url)?; assert_eq!(last_modified, Some(1300)); + Ok(()) + } + #[test] + fn test_storage_get_collection_content() -> Result<()> { + let mut storage = Storage::new(":memory:".into())?; + + let collection_url = "https://example.com/api"; + let initial_records = vec![RemoteSettingsRecord { + id: "2".to_string(), + last_modified: 200, + deleted: false, + attachment: None, + fields: serde_json::json!({"key": "value2"}) + .as_object() + .unwrap() + .clone(), + }]; + + // Set initial records + storage.insert_collection_content( + collection_url, + &initial_records, + 1337, + CollectionMetadata { + bucket: "main".into(), + signature: CollectionSignature { + signature: "b64encodedsig".into(), + x5u: "http://15u/".into(), + }, + }, + )?; + + let metadata = storage.get_collection_metadata(collection_url)?.unwrap(); + + assert_eq!(metadata.signature.signature, "b64encodedsig"); + assert_eq!(metadata.signature.x5u, "http://15u/"); Ok(()) }