diff --git a/components/remote_settings/src/cache.rs b/components/remote_settings/src/cache.rs index 8f5236f2cb..aa70e93c54 100644 --- a/components/remote_settings/src/cache.rs +++ b/components/remote_settings/src/cache.rs @@ -158,4 +158,4 @@ mod test { } ); } -} \ No newline at end of file +} diff --git a/components/remote_settings/src/client.rs b/components/remote_settings/src/client.rs index ca733b780f..f7e27e03b8 100644 --- a/components/remote_settings/src/client.rs +++ b/components/remote_settings/src/client.rs @@ -4,6 +4,7 @@ use crate::config::RemoteSettingsConfig; use crate::error::{Error, Result}; +use crate::jexl_filtering::{create_jexl_context, evaluate_filter_expression}; use crate::storage::Storage; use crate::{RemoteSettingsContext, RemoteSettingsServer, UniffiCustomTypeConverter}; use parking_lot::Mutex; @@ -14,7 +15,6 @@ use std::{ }; use url::Url; use viaduct::{Request, Response}; -use crate::jexl_filtering::{create_jexl_context, evaluate_filter_expression}; const HEADER_BACKOFF: &str = "Backoff"; const HEADER_ETAG: &str = "ETag"; @@ -37,7 +37,12 @@ struct RemoteSettingsClientInner { } impl RemoteSettingsClient { - pub fn new_from_parts(collection_name: String, context: Option, storage: Storage, api_client: C) -> Self { + pub fn new_from_parts( + collection_name: String, + context: Option, + storage: Storage, + api_client: C, + ) -> Self { Self { context, collection_name, @@ -52,12 +57,18 @@ impl RemoteSettingsClient { } /// Filters records based on the presence and evaluation of `filter_expression`. - fn filter_records(&self, records: Vec, jexl_context: &serde_json::Value) -> Vec { + fn filter_records( + &self, + records: Vec, + jexl_context: &serde_json::Value, + ) -> Vec { records .into_iter() .filter(|record| { match &record.filter_expression { - Some(filter_expr) => evaluate_filter_expression(filter_expr, jexl_context).unwrap_or(false), + Some(filter_expr) => { + evaluate_filter_expression(filter_expr, jexl_context).unwrap_or(false) + } None => true, // Add records without a filter expression by default } }) @@ -80,11 +91,11 @@ impl RemoteSettingsClient { Some(records) if !records.is_empty() || !sync_if_empty => { // Filter and return cached records if they're present or if we don't need to sync let filtered_records = self.filter_records(records, &jexl_context); - return Ok(Some(filtered_records)); + Ok(Some(filtered_records)) } None if !sync_if_empty => { // No cached records and sync_if_empty is false, so we return None - return Ok(None); + Ok(None) } _ => { // Fetch new records if no cached records or if sync is required @@ -125,7 +136,12 @@ impl RemoteSettingsClient { storage: Storage, ) -> Result { let api_client = ViaductApiClient::new(server_url, &bucket_name, &collection_name)?; - Ok(Self::new_from_parts(collection_name, context, storage, api_client)) + Ok(Self::new_from_parts( + collection_name, + context, + storage, + api_client, + )) } pub fn update_config(&self, server_url: Url, bucket_name: String) -> Result<()> { @@ -925,10 +941,10 @@ mod test { "GET", format!("/attachments/{}", attachment_location).as_str(), ) - .with_body(attachment_bytes.clone()) - .with_status(200) - .with_header("content-type", "application/json") - .create(); + .with_body(attachment_bytes.clone()) + .with_status(200) + .with_header("content-type", "application/json") + .create(); let config = RemoteSettingsConfig { server: Some(RemoteSettingsServer::Custom { @@ -964,10 +980,10 @@ mod test { "GET", format!("/attachments/{}", attachment_location).as_str(), ) - .with_body(attachment_bytes) - .with_status(200) - .with_header("content-type", "application/json") - .create(); + .with_body(attachment_bytes) + .with_status(200) + .with_header("content-type", "application/json") + .create(); let config = RemoteSettingsConfig { server: Some(RemoteSettingsServer::Custom { @@ -992,12 +1008,12 @@ mod test { "GET", "/v1/buckets/the-bucket/collections/the-collection/records", ) - .with_body(response_body()) - .with_status(200) - .with_header("content-type", "application/json") - .with_header("Backoff", "60") - .with_header("etag", "\"1000\"") - .create(); + .with_body(response_body()) + .with_status(200) + .with_header("content-type", "application/json") + .with_header("Backoff", "60") + .with_header("etag", "\"1000\"") + .create(); let config = RemoteSettingsConfig { server: Some(RemoteSettingsServer::Custom { url: mockito::server_url(), @@ -1021,10 +1037,10 @@ mod test { "GET", "/v1/buckets/the-bucket/collections/the-collection/records", ) - .with_body("Boom!") - .with_status(500) - .with_header("Retry-After", "60") - .create(); + .with_body("Boom!") + .with_status(500) + .with_header("Retry-After", "60") + .create(); let config = RemoteSettingsConfig { server: Some(RemoteSettingsServer::Custom { url: mockito::server_url(), @@ -1047,26 +1063,26 @@ mod test { "GET", "/v1/buckets/the-bucket/collections/the-collection/records", ) - .match_query(Matcher::AllOf(vec![ - Matcher::UrlEncoded("a".into(), "b".into()), - Matcher::UrlEncoded("lt_c.d".into(), "5".into()), - Matcher::UrlEncoded("gt_e".into(), "15".into()), - Matcher::UrlEncoded("max_f".into(), "20".into()), - Matcher::UrlEncoded("min_g".into(), "10".into()), - Matcher::UrlEncoded("not_h".into(), "i".into()), - Matcher::UrlEncoded("like_j".into(), "*k*".into()), - Matcher::UrlEncoded("has_l".into(), "true".into()), - Matcher::UrlEncoded("has_m".into(), "false".into()), - Matcher::UrlEncoded("contains_n".into(), "o".into()), - Matcher::UrlEncoded("_sort".into(), "-b,a".into()), - Matcher::UrlEncoded("_fields".into(), "a,c,b".into()), - Matcher::UrlEncoded("_limit".into(), "3".into()), - ])) - .with_body(response_body()) - .with_status(200) - .with_header("content-type", "application/json") - .with_header("etag", "\"1000\"") - .create(); + .match_query(Matcher::AllOf(vec![ + Matcher::UrlEncoded("a".into(), "b".into()), + Matcher::UrlEncoded("lt_c.d".into(), "5".into()), + Matcher::UrlEncoded("gt_e".into(), "15".into()), + Matcher::UrlEncoded("max_f".into(), "20".into()), + Matcher::UrlEncoded("min_g".into(), "10".into()), + Matcher::UrlEncoded("not_h".into(), "i".into()), + Matcher::UrlEncoded("like_j".into(), "*k*".into()), + Matcher::UrlEncoded("has_l".into(), "true".into()), + Matcher::UrlEncoded("has_m".into(), "false".into()), + Matcher::UrlEncoded("contains_n".into(), "o".into()), + Matcher::UrlEncoded("_sort".into(), "-b,a".into()), + Matcher::UrlEncoded("_fields".into(), "a,c,b".into()), + Matcher::UrlEncoded("_limit".into(), "3".into()), + ])) + .with_body(response_body()) + .with_status(200) + .with_header("content-type", "application/json") + .with_header("etag", "\"1000\"") + .create(); let config = RemoteSettingsConfig { server: Some(RemoteSettingsServer::Custom { url: mockito::server_url(), @@ -1193,11 +1209,11 @@ mod test { "GET", "/v1/buckets/the-bucket/collections/the-collection/records", ) - .with_body(response_body()) - .with_status(200) - .with_header("content-type", "application/json") - .with_header("etag", "\"1000\"") - .create(); + .with_body(response_body()) + .with_status(200) + .with_header("content-type", "application/json") + .with_header("etag", "\"1000\"") + .create(); let config = RemoteSettingsConfig { server: Some(RemoteSettingsServer::Custom { url: mockito::server_url(), @@ -1236,11 +1252,11 @@ mod test { "GET", "/v1/buckets/the-bucket/collections/the-collection/records", ) - .with_body(response_body()) - .with_status(200) - .with_header("content-type", "application/json") - .with_header("etag", "\"1000\"") - .create(); + .with_body(response_body()) + .with_status(200) + .with_header("content-type", "application/json") + .with_header("etag", "\"1000\"") + .create(); let config = RemoteSettingsConfig { server: Some(RemoteSettingsServer::Custom { url: mockito::server_url(), @@ -1346,10 +1362,10 @@ mod test { "GET", "/v1/buckets/the-bucket/collections/the-collection/records", ) - .with_body(response_body()) - .with_status(200) - .with_header("content-type", "application/json") - .create(); + .with_body(response_body()) + .with_status(200) + .with_header("content-type", "application/json") + .create(); let config = RemoteSettingsConfig { server: Some(RemoteSettingsServer::Custom { @@ -1377,11 +1393,11 @@ mod test { "GET", "/v1/buckets/the-bucket/collections/the-collection/records", ) - .with_body(response_body()) - .with_status(200) - .with_header("content-type", "application/json") - .with_header("etag", "bad!") - .create(); + .with_body(response_body()) + .with_status(200) + .with_header("content-type", "application/json") + .with_header("etag", "bad!") + .create(); let config = RemoteSettingsConfig { server: Some(RemoteSettingsServer::Custom { @@ -1517,7 +1533,8 @@ mod test_new_client { &Url::parse("http://rs.example.com/v1").unwrap(), "main", "test-collection", - ).unwrap(); + ) + .unwrap(); assert_eq!(endpoints.root_url.to_string(), "http://rs.example.com/v1/"); assert_eq!( endpoints.collection_url.to_string(), @@ -1542,8 +1559,12 @@ mod test_new_client { // Note, don't make any api_client.expect_*() calls, the RemoteSettingsClient should not // attempt to make any requests for this scenario let storage = Storage::new(":memory:".into()).expect("Error creating storage"); - let rs_client = - RemoteSettingsClient::new_from_parts("test-collection".into(), None, storage, api_client); + let rs_client = RemoteSettingsClient::new_from_parts( + "test-collection".into(), + None, + storage, + api_client, + ); assert_eq!( rs_client.get_records(false).expect("Error getting records"), None @@ -1572,8 +1593,12 @@ mod test_new_client { } }); let storage = Storage::new(":memory:".into()).expect("Error creating storage"); - let rs_client = - RemoteSettingsClient::new_from_parts("test-collection".into(), None, storage, api_client); + let rs_client = RemoteSettingsClient::new_from_parts( + "test-collection".into(), + None, + storage, + api_client, + ); assert_eq!( rs_client.get_records(true).expect("Error getting records"), Some(records) @@ -1583,8 +1608,8 @@ mod test_new_client { #[cfg(test)] mod test_filtered_records { - use serde_json::json; use super::*; + use serde_json::json; /// Helper function to create a record with an optional filter expression. fn create_record(id: &str, filter_expr: Option<&str>) -> RemoteSettingsRecord { @@ -1603,19 +1628,19 @@ mod test_filtered_records { json!({ "env": { "version": "1.0.0" } }) } - #[test] fn test_filter_records_no_expression() { let api_client = MockApiClient::new(); let storage = Storage::new(":memory:".into()).expect("Error creating storage"); - let client = - RemoteSettingsClient::new_from_parts("test-collection".into(), None, storage, api_client);// Assume `new` sets up necessary dependencies + let client = RemoteSettingsClient::new_from_parts( + "test-collection".into(), + None, + storage, + api_client, + ); // Assume `new` sets up necessary dependencies let jexl_context = create_test_jexl_context(); - let records = vec![ - create_record("1", None), - create_record("2", None), - ]; + let records = vec![create_record("1", None), create_record("2", None)]; let filtered = client.filter_records(records, &jexl_context); @@ -1627,8 +1652,12 @@ mod test_filtered_records { fn test_filter_records_true_expression() { let api_client = MockApiClient::new(); let storage = Storage::new(":memory:".into()).expect("Error creating storage"); - let client = - RemoteSettingsClient::new_from_parts("test-collection".into(), None, storage, api_client);// Assume `new` sets up necessary dependencies + let client = RemoteSettingsClient::new_from_parts( + "test-collection".into(), + None, + storage, + api_client, + ); // Assume `new` sets up necessary dependencies let jexl_context = create_test_jexl_context(); let records = vec![ @@ -1646,8 +1675,12 @@ mod test_filtered_records { fn test_filter_records_false_expression() { let api_client = MockApiClient::new(); let storage = Storage::new(":memory:".into()).expect("Error creating storage"); - let client = - RemoteSettingsClient::new_from_parts("test-collection".into(), None, storage, api_client);// Assume `new` sets up necessary dependencies + let client = RemoteSettingsClient::new_from_parts( + "test-collection".into(), + None, + storage, + api_client, + ); // Assume `new` sets up necessary dependencies let jexl_context = create_test_jexl_context(); let records = vec![ @@ -1665,14 +1698,18 @@ mod test_filtered_records { fn test_filter_records_mixed_expressions() { let api_client = MockApiClient::new(); let storage = Storage::new(":memory:".into()).expect("Error creating storage"); - let client = - RemoteSettingsClient::new_from_parts("test-collection".into(), None, storage, api_client);// Assume `new` sets up necessary dependencies + let client = RemoteSettingsClient::new_from_parts( + "test-collection".into(), + None, + storage, + api_client, + ); // Assume `new` sets up necessary dependencies let jexl_context = create_test_jexl_context(); let records = vec![ - create_record("1", Some("env.version == '1.0.0'")), // true - create_record("2", Some("env.version == '2.0.0'")), // false - create_record("3", None), // no filter + create_record("1", Some("env.version == '1.0.0'")), // true + create_record("2", Some("env.version == '2.0.0'")), // false + create_record("3", None), // no filter ]; let filtered = client.filter_records(records, &jexl_context); @@ -1681,7 +1718,6 @@ mod test_filtered_records { assert_eq!(filtered.len(), 2); } - // #[test] // fn test_get_records_with_cached_data() { // let api_client = MockApiClient::new(); @@ -1701,11 +1737,18 @@ mod test_filtered_records { #[test] fn test_get_records_no_cached_sync_false() { let mut api_client = MockApiClient::new(); - api_client.expect_collection_url().returning(|| "test_url".into()); + api_client + .expect_collection_url() + .returning(|| "test_url".into()); let storage = Storage::new(":memory:".into()).expect("Error creating storage"); - let client = RemoteSettingsClient::new_from_parts("test-collection".into(), None, storage, api_client); + let client = RemoteSettingsClient::new_from_parts( + "test-collection".into(), + None, + storage, + api_client, + ); let records = client.get_records(false).expect("Error getting records"); assert!(records.is_none()); @@ -1714,12 +1757,21 @@ mod test_filtered_records { #[test] fn test_get_records_no_cached_sync_true() { let mut api_client = MockApiClient::new(); - api_client.expect_collection_url().returning(|| "test_url".into()); - api_client.expect_get_records().returning(|_| Ok(vec![create_record("1", None)])); + api_client + .expect_collection_url() + .returning(|| "test_url".into()); + api_client + .expect_get_records() + .returning(|_| Ok(vec![create_record("1", None)])); let storage = Storage::new(":memory:".into()).expect("Error creating storage"); - let client = RemoteSettingsClient::new_from_parts("test-collection".into(), None, storage, api_client); + let client = RemoteSettingsClient::new_from_parts( + "test-collection".into(), + None, + storage, + api_client, + ); let records = client.get_records(true).expect("Error getting records"); assert!(records.is_some()); @@ -1745,4 +1797,4 @@ mod test_filtered_records { // assert!(records.is_some()); // assert_eq!(records.unwrap().len(), 1); // Only one record should pass the filter // } -} \ No newline at end of file +} diff --git a/components/remote_settings/src/jexl_filtering.rs b/components/remote_settings/src/jexl_filtering.rs index 4352aabbae..0c5e1f9970 100644 --- a/components/remote_settings/src/jexl_filtering.rs +++ b/components/remote_settings/src/jexl_filtering.rs @@ -1,5 +1,4 @@ use crate::RemoteSettingsContext; -use anyhow::Error; use jexl_eval::Evaluator; use serde_json::{json, Value}; use std::cmp::Ordering; @@ -24,7 +23,7 @@ pub(crate) enum TransformError { } /// Transform for comparing versions. -fn version_compare(args: &[Value]) -> Result { +fn version_compare(args: &[Value]) -> Result { if args.len() != 2 { return Err(TransformError::InvalidArgumentCount("versionCompare").into()); } diff --git a/components/remote_settings/src/lib.rs b/components/remote_settings/src/lib.rs index 055060ba31..c4e5b35567 100644 --- a/components/remote_settings/src/lib.rs +++ b/components/remote_settings/src/lib.rs @@ -305,11 +305,11 @@ mod test { "GET", "/v1/buckets/the-bucket/collections/the-collection/records", ) - .with_body(response_body()) - .with_status(200) - .with_header("content-type", "application/json") - .with_header("etag", "\"1000\"") - .create(); + .with_body(response_body()) + .with_status(200) + .with_header("content-type", "application/json") + .with_header("etag", "\"1000\"") + .create(); let config = RemoteSettingsConfig { server: Some(RemoteSettingsServer::Custom { @@ -335,12 +335,12 @@ mod test { "GET", "/v1/buckets/the-bucket/collections/the-collection/records", ) - .match_query(Matcher::UrlEncoded("gt_last_modified".into(), "500".into())) - .with_body(response_body()) - .with_status(200) - .with_header("content-type", "application/json") - .with_header("etag", "\"1000\"") - .create(); + .match_query(Matcher::UrlEncoded("gt_last_modified".into(), "500".into())) + .with_body(response_body()) + .with_status(200) + .with_header("content-type", "application/json") + .with_header("etag", "\"1000\"") + .create(); let config = RemoteSettingsConfig { server: Some(RemoteSettingsServer::Custom { @@ -446,4 +446,4 @@ mod test { "last_modified": 1677694455368 } "#; -} \ No newline at end of file +} diff --git a/components/remote_settings/src/service.rs b/components/remote_settings/src/service.rs index a8e81aad1e..39e763eb5f 100644 --- a/components/remote_settings/src/service.rs +++ b/components/remote_settings/src/service.rs @@ -62,9 +62,7 @@ impl RemoteSettingsService { context: Option, ) -> Result> { let mut inner = self.inner.lock(); - let storage = Storage::new( - inner.storage_dir.join(format!("{collection_name}.sql")), - )?; + let storage = Storage::new(inner.storage_dir.join(format!("{collection_name}.sql")))?; let client = Arc::new(RemoteSettingsClient::new( inner.base_url.clone(), inner.bucket_name.clone(),