Skip to content

Commit

Permalink
feat: add jexl to remote-settings component
Browse files Browse the repository at this point in the history
  • Loading branch information
gruberb committed Nov 7, 2024
1 parent 5fefc05 commit 215e37e
Show file tree
Hide file tree
Showing 10 changed files with 326 additions and 38 deletions.
12 changes: 8 additions & 4 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion components/nimbus/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ log = "0.4"
thiserror = "1"
url = "2.5"
rkv = { version = "0.19", optional = true }
jexl-eval = "0.2.2"
jexl-eval = "0.3.0"
uuid = { version = "1.7", features = ["serde", "v4"]}
sha2 = "^0.10"
hex = "0.4"
Expand Down
4 changes: 4 additions & 0 deletions components/remote_settings/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ viaduct = { path = "../viaduct" }
url = "2.1" # mozilla-central can't yet take 2.2 (see bug 1734538)
camino = "1.0"
rusqlite = { workspace = true, features = ["bundled"] }
jexl-eval = "0.3.0"
regex = "1.10.5"
anyhow = "1.0"
firefox-versioning = { path = "../support/firefox-versioning" }

[build-dependencies]
uniffi = { workspace = true, features = ["build"] }
Expand Down
2 changes: 1 addition & 1 deletion components/remote_settings/src/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ mod test {
last_modified: 1300,
deleted: false,
attachment: None,
fields: fields("d")
fields: fields("d"),
},
],
}
Expand Down
188 changes: 170 additions & 18 deletions components/remote_settings/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@

use crate::config::RemoteSettingsConfig;
use crate::error::{Error, Result};
use crate::jexl_filter::JexlFilter;
use crate::storage::Storage;
use crate::{RemoteSettingsServer, UniffiCustomTypeConverter};
use crate::{RemoteSettingsContext, RemoteSettingsServer, UniffiCustomTypeConverter};
use parking_lot::Mutex;
use serde::{Deserialize, Serialize};
use std::{
Expand All @@ -26,6 +27,7 @@ const HEADER_RETRY_AFTER: &str = "Retry-After";
pub struct RemoteSettingsClient<C = ViaductApiClient> {
// This is immutable, so it can be outside the mutex
collection_name: String,
jexl_filter: JexlFilter,
inner: Mutex<RemoteSettingsClientInner<C>>,
}

Expand All @@ -35,9 +37,15 @@ struct RemoteSettingsClientInner<C> {
}

impl<C: ApiClient> RemoteSettingsClient<C> {
pub fn new_from_parts(collection_name: String, storage: Storage, api_client: C) -> Self {
pub fn new_from_parts(
collection_name: String,
storage: Storage,
jexl_filter: JexlFilter,
api_client: C,
) -> Self {
Self {
collection_name,
jexl_filter,
inner: Mutex::new(RemoteSettingsClientInner {
storage,
api_client,
Expand All @@ -48,6 +56,19 @@ impl<C: ApiClient> RemoteSettingsClient<C> {
&self.collection_name
}

/// Filters records based on the presence and evaluation of `filter_expression`.
fn filter_records(&self, records: Vec<RemoteSettingsRecord>) -> Vec<RemoteSettingsRecord> {
records
.into_iter()
.filter(|record| match record.fields.get("filter_expression") {
Some(serde_json::Value::String(filter_expr)) => {
self.jexl_filter.evaluate(filter_expr).unwrap_or(false)
}
_ => true, // Include records without a valid filter expression by default
})
.collect()
}

/// Get the current set of records.
///
/// If records are not present in storage this will normally return None. Use `sync_if_empty =
Expand All @@ -56,14 +77,29 @@ impl<C: ApiClient> RemoteSettingsClient<C> {
let mut inner = self.inner.lock();
let collection_url = inner.api_client.collection_url();

// Try to retrieve and filter cached records first
let cached_records = inner.storage.get_records(&collection_url)?;
if cached_records.is_some() || !sync_if_empty {
return Ok(cached_records);
}

let records = inner.api_client.get_records(None)?;
inner.storage.set_records(&collection_url, &records)?;
Ok(Some(records))
match cached_records {
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);
Ok(Some(filtered_records))
}
None if !sync_if_empty => {
// No cached records and sync_if_empty is false, so we return None
Ok(None)
}
_ => {
// Fetch new records if no cached records or if sync is required
let records = inner.api_client.get_records(None)?;
inner.storage.set_records(&collection_url, &records)?;

// Apply filtering to the newly fetched records
let filtered_records = self.filter_records(records);
Ok(Some(filtered_records))
}
}
}

pub fn sync(&self) -> Result<()> {
Expand All @@ -89,10 +125,18 @@ impl RemoteSettingsClient<ViaductApiClient> {
server_url: Url,
bucket_name: String,
collection_name: String,
context: Option<RemoteSettingsContext>,
storage: Storage,
) -> Result<Self> {
let api_client = ViaductApiClient::new(server_url, &bucket_name, &collection_name)?;
Ok(Self::new_from_parts(collection_name, storage, api_client))
let jexl_filter = JexlFilter::new(context);

Ok(Self::new_from_parts(
collection_name,
storage,
jexl_filter,
api_client,
))
}

pub fn update_config(&self, server_url: Url, bucket_name: String) -> Result<()> {
Expand Down Expand Up @@ -1414,11 +1458,11 @@ mod test {
"title": "jpg-attachment",
"content": "content",
"attachment": {
"filename": "jgp-attachment.jpg",
"location": "the-bucket/the-collection/d3a5eccc-f0ca-42c3-b0bb-c0d4408c21c9.jpg",
"hash": "2cbd593f3fd5f1585f92265433a6696a863bc98726f03e7222135ff0d8e83543",
"mimetype": "image/jpeg",
"size": 1374325
"filename": "jgp-attachment.jpg",
"location": "the-bucket/the-collection/d3a5eccc-f0ca-42c3-b0bb-c0d4408c21c9.jpg",
"hash": "2cbd593f3fd5f1585f92265433a6696a863bc98726f03e7222135ff0d8e83543",
"mimetype": "image/jpeg",
"size": 1374325
},
"id": "c5dcd1da-7126-4abb-846b-ec85b0d4d0d7",
"schema": 1677694447771,
Expand Down Expand Up @@ -1500,8 +1544,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(), storage, api_client);
let rs_client = RemoteSettingsClient::new_from_parts(
"test-collection".into(),
storage,
JexlFilter::new(None),
api_client,
);
assert_eq!(
rs_client.get_records(false).expect("Error getting records"),
None
Expand Down Expand Up @@ -1529,11 +1577,115 @@ mod test_new_client {
}
});
let storage = Storage::new(":memory:".into()).expect("Error creating storage");
let rs_client =
RemoteSettingsClient::new_from_parts("test-collection".into(), storage, api_client);
let rs_client = RemoteSettingsClient::new_from_parts(
"test-collection".into(),
storage,
JexlFilter::new(None),
api_client,
);
assert_eq!(
rs_client.get_records(true).expect("Error getting records"),
Some(records)
);
}
}

#[cfg(test)]
mod test_filtering_records {
use super::*;
use serde_json::json;

#[test]
fn test_get_records_filtered_app_version_pass() {
let mut api_client = MockApiClient::new();
let records = vec![RemoteSettingsRecord {
id: "record-0001".into(),
last_modified: 100,
deleted: false,
attachment: None,
fields: json!({"filter_expression": "app_version|versionCompare('4.0') >= 0"})
.as_object()
.unwrap()
.clone(),
}];
api_client.expect_collection_url().returning(|| {
"http://rs.example.com/v1/buckets/main/collections/test-collection".into()
});
api_client.expect_get_records().returning({
let records = records.clone();
move |timestamp| {
assert_eq!(timestamp, None);
Ok(records.clone())
}
});

let context = RemoteSettingsContext {
app_version: Some("4.4".to_string()),
..Default::default()
};

let mut storage = Storage::new(":memory:".into()).expect("Error creating storage");
let _ = storage.set_records(
"http://rs.example.com/v1/buckets/main/collections/test-collection",
&records,
);

let rs_client = RemoteSettingsClient::new_from_parts(
"test-collection".into(),
storage,
JexlFilter::new(Some(context)),
api_client,
);
assert_eq!(
rs_client.get_records(false).expect("Error getting records"),
Some(records)
);
}

#[test]
fn test_get_records_filtered_app_version_too_low() {
let mut api_client = MockApiClient::new();
let records = vec![RemoteSettingsRecord {
id: "record-0001".into(),
last_modified: 100,
deleted: false,
attachment: None,
fields: json!({"filter_expression": "app_version|versionCompare('4.0') >= 0"})
.as_object()
.unwrap()
.clone(),
}];
api_client.expect_collection_url().returning(|| {
"http://rs.example.com/v1/buckets/main/collections/test-collection".into()
});
api_client.expect_get_records().returning({
let records = records.clone();
move |timestamp| {
assert_eq!(timestamp, None);
Ok(records.clone())
}
});

let context = RemoteSettingsContext {
app_version: Some("3.9".to_string()),
..Default::default()
};

let mut storage = Storage::new(":memory:".into()).expect("Error creating storage");
let _ = storage.set_records(
"http://rs.example.com/v1/buckets/main/collections/test-collection",
&records,
);

let rs_client = RemoteSettingsClient::new_from_parts(
"test-collection".into(),
storage,
JexlFilter::new(Some(context)),
api_client,
);
assert_eq!(
rs_client.get_records(false).expect("Error getting records"),
Some(vec![])
);
}
}
60 changes: 60 additions & 0 deletions components/remote_settings/src/jexl_filter.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
use crate::RemoteSettingsContext;
use firefox_versioning::compare::version_compare;
use jexl_eval::Evaluator;
use serde_json::{json, Value};
use thiserror::Error;

#[derive(Error, Debug)]
pub(crate) enum ParseError {
#[error("Evaluation error: {0}")]
EvaluationError(String),
#[error("Invalid result type")]
InvalidResultType,
}

/// The JEXL filter is getting instatiated when a new `RemoteSettingsClient` is being created.
pub struct JexlFilter {
/// a JEXL `Evaluator` to run transforms and evaluations on.
evaluator: Evaluator<'static>,
/// The transformed `RemoteSettingsContext` as a `serde_json::Value`
context: Value,
}

impl JexlFilter {
/// Creating a new `JEXL` filter. If no `context` is set, all future `records` are being
/// evaluated as `true` by default.
pub(crate) fn new(context: Option<RemoteSettingsContext>) -> Self {
let env_context = match context {
Some(ctx) => {
serde_json::to_value(ctx).expect("Failed to serialize RemoteSettingsContext")
}
None => json!({}),
};

Self {
evaluator: Evaluator::new()
// We want to add more transforms later on. We started with `versionCompare`.
// https://remote-settings.readthedocs.io/en/latest/target-filters.html#transforms
// The goal is to get on pare with the desktop.
.with_transform("versionCompare", |args| Ok(version_compare(args)?)),
context: env_context,
}
}

/// Evaluates the given filter expression in the provided context.
/// Returns `Ok(true)` if the expression evaluates to true, `Ok(false)` otherwise.
pub(crate) fn evaluate(&self, filter_expr: &str) -> Result<bool, ParseError> {
if filter_expr.trim().is_empty() {
return Ok(true);
}

let result = self
.evaluator
.eval_in_context(filter_expr, &self.context)
.map_err(|e| {
ParseError::EvaluationError(format!("Failed to evaluate '{}': {}", filter_expr, e))
})?;

result.as_bool().ok_or(ParseError::InvalidResultType)
}
}
Loading

0 comments on commit 215e37e

Please sign in to comment.