diff --git a/components/remote_settings/src/client.rs b/components/remote_settings/src/client.rs index 1019f408f8..20ed5a6a4e 100644 --- a/components/remote_settings/src/client.rs +++ b/components/remote_settings/src/client.rs @@ -177,10 +177,27 @@ impl RemoteSettingsClient { /// Downloads an attachment from [attachment_location]. NOTE: there are no guarantees about a /// maximum size, so use care when fetching potentially large attachments. pub fn get_attachment(&self, attachment_location: &str) -> Result> { - self.inner + let mut inner = self.inner.lock(); + let collection_url = inner.api_client.collection_url(); + + if let Some(attachment) = inner + .storage + .get_attachment(&collection_url, attachment_location)? + { + return Ok(attachment); + } + + let attachment = self + .inner .lock() .api_client - .get_attachment(attachment_location) + .get_attachment(attachment_location)?; + + inner + .storage + .set_attachment(&collection_url, attachment_location, &attachment)?; + + Ok(attachment) } } diff --git a/components/remote_settings/src/lib.rs b/components/remote_settings/src/lib.rs index 4457644a00..a0518c4ab3 100644 --- a/components/remote_settings/src/lib.rs +++ b/components/remote_settings/src/lib.rs @@ -155,8 +155,6 @@ impl RemoteSettingsClient { /// this is that there is not much an application can do in this situation other than fall back /// to the same default handling as if records have not been synced. /// - /// TODO(Bug 1919141): - /// /// Application-services schedules regular dumps of the server data for specific collections. /// For these collections, `get_records` will never return None. If you would like to add your /// collection to this list, please reach out to the DISCO team. @@ -198,8 +196,8 @@ impl RemoteSettingsClient { /// - This method will throw if there is a network or other error when fetching the /// attachment data. #[handle_error(Error)] - pub fn get_attachment(&self, attachment_id: String) -> ApiResult> { - self.internal.get_attachment(&attachment_id) + pub fn get_attachment(&self, attachment_location: String) -> ApiResult> { + self.internal.get_attachment(&attachment_location) } } diff --git a/components/remote_settings/src/storage.rs b/components/remote_settings/src/storage.rs index 46e4ab4f3e..501f7822cb 100644 --- a/components/remote_settings/src/storage.rs +++ b/components/remote_settings/src/storage.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 crate::{Attachment, RemoteSettingsRecord, Result}; +use crate::{RemoteSettingsRecord, Result}; use camino::Utf8PathBuf; use rusqlite::{params, Connection, OptionalExtension}; use serde_json; @@ -114,19 +114,14 @@ impl Storage { &self, collection_url: &str, attachment_id: &str, - ) -> Result> { + ) -> Result>> { let mut stmt = self .conn .prepare("SELECT data FROM attachments WHERE id = ? AND collection_url = ?")?; - let result: Option> = stmt + + Ok(stmt .query_row((attachment_id, collection_url), |row| row.get(0)) - .optional()?; - if let Some(data) = result { - let attachment: Attachment = serde_json::from_slice(&data)?; - Ok(Some(attachment)) - } else { - Ok(None) - } + .optional()?) } /// Set the list of records stored in the database, clearing out any previously stored records @@ -171,7 +166,7 @@ impl Storage { &mut self, collection_url: &str, attachment_id: &str, - attachment: Attachment, + attachment: &[u8], ) -> Result<()> { let tx = self.conn.transaction()?; @@ -181,11 +176,9 @@ impl Storage { params![collection_url], )?; - let data = serde_json::to_vec(&attachment)?; - tx.execute( "INSERT OR REPLACE INTO attachments (id, collection_url, data) VALUES (?, ?, ?)", - params![attachment_id, collection_url, data], + params![attachment_id, collection_url, attachment], )?; tx.commit()?; @@ -300,8 +293,7 @@ mod tests { let mut storage = Storage::new(":memory:".into())?; let collection_url = "https://example.com/api"; - let attachment_id = "attachment1"; - let attachment = Attachment { + let attachment_metadata = Attachment { filename: "abc".to_string(), mimetype: "application/json".to_string(), location: "tmp".to_string(), @@ -309,11 +301,14 @@ mod tests { size: 1024, }; + let attachment = &[0x18, 0x64]; + // Store attachment - storage.set_attachment(collection_url, attachment_id, attachment.clone())?; + storage.set_attachment(collection_url, &attachment_metadata.location, attachment)?; // Get attachment - let fetched_attachment = storage.get_attachment(collection_url, attachment_id)?; + let fetched_attachment = + storage.get_attachment(collection_url, &attachment_metadata.location)?; assert!(fetched_attachment.is_some()); let fetched_attachment = fetched_attachment.unwrap(); assert_eq!(fetched_attachment, attachment); @@ -326,15 +321,16 @@ mod tests { let mut storage = Storage::new(":memory:".into())?; let collection_url = "https://example.com/api"; - let attachment_id = "attachment1"; - let attachment_1 = Attachment { + + let attachment_metadata_1 = Attachment { filename: "abc".to_string(), mimetype: "application/json".to_string(), location: "tmp".to_string(), hash: "abc123".to_string(), size: 1024, }; - let attachment_2 = Attachment { + + let attachment_metadata_2 = Attachment { filename: "def".to_string(), mimetype: "application/json".to_string(), location: "tmp".to_string(), @@ -342,14 +338,25 @@ mod tests { size: 2048, }; + let attachment_1 = &[0x18, 0x64]; + let attachment_2 = &[0x12, 0x48]; + // Store first attachment - storage.set_attachment(collection_url, attachment_id, attachment_1.clone())?; + storage.set_attachment( + collection_url, + &attachment_metadata_1.location, + attachment_1, + )?; // Replace attachment with new data - storage.set_attachment(collection_url, attachment_id, attachment_2.clone())?; + storage.set_attachment( + collection_url, + &attachment_metadata_2.location, + attachment_2, + )?; // Get attachment - let fetched_attachment = storage.get_attachment(collection_url, attachment_id)?; + let fetched_attachment = storage.get_attachment(collection_url, "tmp")?; assert!(fetched_attachment.is_some()); let fetched_attachment = fetched_attachment.unwrap(); assert_eq!(fetched_attachment, attachment_2); @@ -363,32 +370,45 @@ mod tests { let collection_url_1 = "https://example.com/api1"; let collection_url_2 = "https://example.com/api2"; - let attachment_id_1 = "attachment1"; - let attachment_id_2 = "attachment2"; - let attachment_1 = Attachment { + + let attachment_metadata_1 = Attachment { filename: "abc".to_string(), mimetype: "application/json".to_string(), - location: "tmp".to_string(), + location: "first_tmp".to_string(), hash: "abc123".to_string(), size: 1024, }; - let attachment_2 = Attachment { + + let attachment_metadata_2 = Attachment { filename: "def".to_string(), mimetype: "application/json".to_string(), - location: "tmp".to_string(), + location: "second_tmp".to_string(), hash: "def456".to_string(), size: 2048, }; + let attachment_1 = &[0x18, 0x64]; + let attachment_2 = &[0x12, 0x48]; + // Set attachments for two different collections - storage.set_attachment(collection_url_1, attachment_id_1, attachment_1.clone())?; - storage.set_attachment(collection_url_2, attachment_id_2, attachment_2.clone())?; + storage.set_attachment( + collection_url_1, + &attachment_metadata_1.location, + attachment_1, + )?; + storage.set_attachment( + collection_url_2, + &attachment_metadata_2.location, + attachment_2, + )?; // Verify that only the attachment for the second collection remains - let fetched_attachment_1 = storage.get_attachment(collection_url_1, attachment_id_1)?; + let fetched_attachment_1 = + storage.get_attachment(collection_url_1, &attachment_metadata_1.location)?; assert!(fetched_attachment_1.is_none()); - let fetched_attachment_2 = storage.get_attachment(collection_url_2, attachment_id_2)?; + let fetched_attachment_2 = + storage.get_attachment(collection_url_2, &attachment_metadata_2.location)?; assert!(fetched_attachment_2.is_some()); let fetched_attachment_2 = fetched_attachment_2.unwrap(); assert_eq!(fetched_attachment_2, attachment_2); @@ -430,30 +450,31 @@ mod tests { id: "2".to_string(), last_modified: 200, deleted: false, - attachment: None, + attachment: Some(Attachment { + filename: "abc".to_string(), + mimetype: "application/json".to_string(), + location: "tmp".to_string(), + hash: "abc123".to_string(), + size: 1024, + }), fields: serde_json::json!({"key": "value2"}) .as_object() .unwrap() .clone(), }, ]; - let attachment_id = "attachment1"; - let attachment = Attachment { - filename: "abc".to_string(), - mimetype: "application/json".to_string(), - location: "tmp".to_string(), - hash: "abc123".to_string(), - size: 1024, - }; + + let attachment_location = "tmp"; + let attachment = &[0x18, 0x64]; // Set records and attachment storage.set_records(collection_url, &records)?; - storage.set_attachment(collection_url, attachment_id, attachment.clone())?; + storage.set_attachment(collection_url, attachment_location, attachment)?; // Verify they are stored let fetched_records = storage.get_records(collection_url)?; assert!(fetched_records.is_some()); - let fetched_attachment = storage.get_attachment(collection_url, attachment_id)?; + let fetched_attachment = storage.get_attachment(collection_url, attachment_location)?; assert!(fetched_attachment.is_some()); // Empty the storage @@ -462,7 +483,7 @@ mod tests { // Verify they are deleted let fetched_records = storage.get_records(collection_url)?; assert!(fetched_records.is_none()); - let fetched_attachment = storage.get_attachment(collection_url, attachment_id)?; + let fetched_attachment = storage.get_attachment(collection_url, attachment_location)?; assert!(fetched_attachment.is_none()); Ok(())