From 2d3ae0252bac13a7b7910262614848135564b103 Mon Sep 17 00:00:00 2001 From: Bastian Gruber Date: Mon, 4 Nov 2024 10:35:53 -0400 Subject: [PATCH] feat: move versioning into its own support crate Multiple components in application-services need to filter for the exact firefox version. Instead of duplicating the code, we move it into its own support crate, which other components can now make use of. --- Cargo.lock | 9 ++++ Cargo.toml | 1 + components/nimbus/Cargo.toml | 3 +- components/nimbus/src/error.rs | 9 ++++ components/nimbus/src/lib.rs | 1 - components/nimbus/src/targeting.rs | 34 +++------------ components/nimbus/src/tests/helpers.rs | 1 + components/nimbus/src/tests/mod.rs | 1 - .../nimbus/src/tests/stateful/test_nimbus.rs | 2 + .../nimbus/src/tests/test_enrollment.rs | 12 +++++- components/nimbus/src/tests/test_evaluator.rs | 4 ++ .../nimbus/tests/test_message_helpers.rs | 1 + .../support/firefox-versioning/Cargo.toml | 10 +++++ .../support/firefox-versioning/src/compare.rs | 34 +++++++++++++++ .../support/firefox-versioning/src/error.rs | 17 ++++++++ .../support/firefox-versioning/src/lib.rs | 7 ++++ .../firefox-versioning/src/version.rs} | 42 ++++++++++--------- .../tests/test_versioning.rs | 10 +++-- 18 files changed, 142 insertions(+), 56 deletions(-) create mode 100644 components/support/firefox-versioning/Cargo.toml create mode 100644 components/support/firefox-versioning/src/compare.rs create mode 100644 components/support/firefox-versioning/src/error.rs create mode 100644 components/support/firefox-versioning/src/lib.rs rename components/{nimbus/src/versioning.rs => support/firefox-versioning/src/version.rs} (91%) rename components/{nimbus/src => support/firefox-versioning}/tests/test_versioning.rs (95%) diff --git a/Cargo.lock b/Cargo.lock index 3e90ef7c4..73d09feda 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1522,6 +1522,14 @@ dependencies = [ "log", ] +[[package]] +name = "firefox-versioning" +version = "0.1.0" +dependencies = [ + "serde_json", + "thiserror", +] + [[package]] name = "fixedbitset" version = "0.4.2" @@ -2697,6 +2705,7 @@ dependencies = [ "ctor 0.2.8", "env_logger", "error-support", + "firefox-versioning", "glean-build", "hex", "jexl-eval", diff --git a/Cargo.toml b/Cargo.toml index 3dfefc867..c78c98665 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -55,6 +55,7 @@ members = [ "examples/*/", "testing/separated/*/", + "components/support/firefox-versioning", ] exclude = [ diff --git a/components/nimbus/Cargo.toml b/components/nimbus/Cargo.toml index b813cb346..45d4a5e43 100644 --- a/components/nimbus/Cargo.toml +++ b/components/nimbus/Cargo.toml @@ -17,7 +17,7 @@ name = "nimbus" default=["stateful"] rkv-safe-mode = ["dep:rkv"] stateful-uniffi-bindings = [] -stateful = ["rkv-safe-mode", "stateful-uniffi-bindings", "dep:remote_settings", "dep:regex"] +stateful = ["rkv-safe-mode", "stateful-uniffi-bindings", "dep:remote_settings", "dep:regex", "dep:firefox-versioning"] [dependencies] anyhow = "1" @@ -40,6 +40,7 @@ error-support = { path = "../support/error" } remote_settings = { path = "../remote_settings", optional = true } cfg-if = "1.0.0" regex = { version = "1.10.5", optional = true } +firefox-versioning = { path = "../support/firefox-versioning", optional = true } [build-dependencies] uniffi = { workspace = true, features = ["build"] } diff --git a/components/nimbus/src/error.rs b/components/nimbus/src/error.rs index d7701dca4..9372de6dc 100644 --- a/components/nimbus/src/error.rs +++ b/components/nimbus/src/error.rs @@ -8,6 +8,8 @@ //! TODO: Implement proper error handling, this would include defining the error enum, //! impl std::error::Error using `thiserror` and ensuring all errors are handled appropriately +#[cfg(feature = "stateful")] +use firefox_versioning::error::VersionParsingError; use std::num::{ParseIntError, TryFromIntError}; #[derive(Debug, thiserror::Error)] @@ -107,4 +109,11 @@ impl<'a> From> for NimbusError { } } +#[cfg(feature = "stateful")] +impl From for NimbusError { + fn from(eval_error: VersionParsingError) -> Self { + NimbusError::VersionParsingError(eval_error.to_string()) + } +} + pub type Result = std::result::Result; diff --git a/components/nimbus/src/lib.rs b/components/nimbus/src/lib.rs index b0eb84643..b693eba66 100644 --- a/components/nimbus/src/lib.rs +++ b/components/nimbus/src/lib.rs @@ -15,7 +15,6 @@ mod targeting; pub mod error; pub mod metrics; pub mod schema; -pub mod versioning; pub use enrollment::{EnrolledFeature, EnrollmentStatus}; pub use error::{NimbusError, Result}; diff --git a/components/nimbus/src/targeting.rs b/components/nimbus/src/targeting.rs index 1734b3d05..2392aa4c5 100644 --- a/components/nimbus/src/targeting.rs +++ b/components/nimbus/src/targeting.rs @@ -2,16 +2,18 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -use crate::{versioning::Version, NimbusError, Result}; +use crate::{NimbusError, Result}; + use jexl_eval::Evaluator; use serde::Serialize; -use serde_json::{json, Value}; +use serde_json::Value; cfg_if::cfg_if! { if #[cfg(feature = "stateful")] { use anyhow::anyhow; use crate::{TargetingAttributes, stateful::behavior::{EventStore, EventQueryType, query_event_store}}; use std::sync::{Arc, Mutex}; + use firefox_versioning::compare::version_compare; } } @@ -82,11 +84,11 @@ pub fn jexl_eval_raw( context: &Context, #[cfg(feature = "stateful")] event_store: Arc>, ) -> Result { - let evaluator = - Evaluator::new().with_transform("versionCompare", |args| Ok(version_compare(args)?)); + let evaluator = Evaluator::new(); #[cfg(feature = "stateful")] let evaluator = evaluator + .with_transform("versionCompare", |args| Ok(version_compare(args)?)) .with_transform("eventSum", |args| { Ok(query_event_store( event_store.clone(), @@ -150,30 +152,6 @@ pub fn jexl_eval( } } -fn version_compare(args: &[Value]) -> Result { - let curr_version = args.first().ok_or_else(|| { - NimbusError::VersionParsingError("current version doesn't exist in jexl transform".into()) - })?; - let curr_version = curr_version.as_str().ok_or_else(|| { - NimbusError::VersionParsingError("current version in jexl transform is not a string".into()) - })?; - let min_version = args.get(1).ok_or_else(|| { - NimbusError::VersionParsingError("minimum version doesn't exist in jexl transform".into()) - })?; - let min_version = min_version.as_str().ok_or_else(|| { - NimbusError::VersionParsingError("minimum version is not a string in jexl transform".into()) - })?; - let min_version = Version::try_from(min_version)?; - let curr_version = Version::try_from(curr_version)?; - Ok(json!(if curr_version > min_version { - 1 - } else if curr_version < min_version { - -1 - } else { - 0 - })) -} - #[cfg(feature = "stateful")] fn bucket_sample(args: &[Value]) -> anyhow::Result { fn get_arg_as_u32(args: &[Value], idx: usize, name: &str) -> anyhow::Result { diff --git a/components/nimbus/src/tests/helpers.rs b/components/nimbus/src/tests/helpers.rs index 1b16cb515..ef3433598 100644 --- a/components/nimbus/src/tests/helpers.rs +++ b/components/nimbus/src/tests/helpers.rs @@ -342,6 +342,7 @@ pub(crate) fn get_test_experiments() -> Vec { ] } +#[cfg(feature = "stateful")] pub fn get_ios_rollout_experiment() -> Experiment { serde_json::from_value(json!( { diff --git a/components/nimbus/src/tests/mod.rs b/components/nimbus/src/tests/mod.rs index f849c061b..16986e4d1 100644 --- a/components/nimbus/src/tests/mod.rs +++ b/components/nimbus/src/tests/mod.rs @@ -10,7 +10,6 @@ mod test_evaluator; mod test_lib_bw_compat; mod test_sampling; mod test_schema; -mod test_versioning; #[cfg(feature = "stateful")] mod stateful { diff --git a/components/nimbus/src/tests/stateful/test_nimbus.rs b/components/nimbus/src/tests/stateful/test_nimbus.rs index 950b28a0c..eaab98a93 100644 --- a/components/nimbus/src/tests/stateful/test_nimbus.rs +++ b/components/nimbus/src/tests/stateful/test_nimbus.rs @@ -917,6 +917,7 @@ fn delete_test_creation_date>(path: P) -> Result<()> { Ok(()) } +#[cfg(feature = "stateful")] #[test] fn test_ios_rollout() -> Result<()> { let metrics = TestMetrics::new(); @@ -1645,6 +1646,7 @@ fn test_new_enrollment_in_targeting_mid_run() -> Result<()> { Ok(()) } +#[cfg(feature = "stateful")] #[test] fn test_recorded_context_recorded() -> Result<()> { let metrics = TestMetrics::new(); diff --git a/components/nimbus/src/tests/test_enrollment.rs b/components/nimbus/src/tests/test_enrollment.rs index fd9b4da98..065ada8c4 100644 --- a/components/nimbus/src/tests/test_enrollment.rs +++ b/components/nimbus/src/tests/test_enrollment.rs @@ -10,12 +10,19 @@ use crate::{ enrollment::*, error::Result, tests::helpers::{ - get_ios_rollout_experiment, get_multi_feature_experiment, get_single_feature_experiment, - get_test_experiments, no_coenrolling_features, + get_multi_feature_experiment, get_single_feature_experiment, get_test_experiments, + no_coenrolling_features, }, AppContext, AvailableRandomizationUnits, Branch, BucketConfig, Experiment, FeatureConfig, NimbusTargetingHelper, TargetingAttributes, }; + +cfg_if::cfg_if! { + if #[cfg(feature = "stateful")] { + use crate::tests::helpers::get_ios_rollout_experiment; + + } +} use serde_json::{json, Value}; use std::collections::{HashMap, HashSet}; use uuid::Uuid; @@ -411,6 +418,7 @@ fn enrollment_evolver<'a>( EnrollmentsEvolver::new(aru, targeting_helper, ids) } +#[cfg(feature = "stateful")] #[test] fn test_ios_rollout_experiment() -> Result<()> { let exp = &get_ios_rollout_experiment(); diff --git a/components/nimbus/src/tests/test_evaluator.rs b/components/nimbus/src/tests/test_evaluator.rs index 2e6ed9de9..216f8867a 100644 --- a/components/nimbus/src/tests/test_evaluator.rs +++ b/components/nimbus/src/tests/test_evaluator.rs @@ -109,6 +109,7 @@ fn test_geo_targeting_fails_properly() -> Result<()> { Ok(()) } +#[cfg(feature = "stateful")] #[test] fn test_minimum_version_targeting_passes() -> Result<()> { // Here's our valid jexl statement @@ -121,6 +122,7 @@ fn test_minimum_version_targeting_passes() -> Result<()> { Ok(()) } +#[cfg(feature = "stateful")] #[test] fn test_minimum_version_targeting_fails() -> Result<()> { // Here's our valid jexl statement @@ -138,6 +140,7 @@ fn test_minimum_version_targeting_fails() -> Result<()> { Ok(()) } +#[cfg(feature = "stateful")] #[test] fn test_targeting_specific_version() -> Result<()> { // Here's our valid jexl statement that targets **only** 96 versions @@ -196,6 +199,7 @@ fn test_targeting_invalid_transform() -> Result<()> { Ok(()) } +#[cfg(feature = "stateful")] #[test] fn test_targeting() { // Here's our valid jexl statement diff --git a/components/nimbus/tests/test_message_helpers.rs b/components/nimbus/tests/test_message_helpers.rs index c35c3e81e..7300758b8 100644 --- a/components/nimbus/tests/test_message_helpers.rs +++ b/components/nimbus/tests/test_message_helpers.rs @@ -20,6 +20,7 @@ mod message_tests { use super::*; + #[cfg(feature = "stateful")] #[test] fn test_jexl_expression() -> Result<()> { let nimbus = common::new_test_client("jexl_test")?; diff --git a/components/support/firefox-versioning/Cargo.toml b/components/support/firefox-versioning/Cargo.toml new file mode 100644 index 000000000..8a55ab034 --- /dev/null +++ b/components/support/firefox-versioning/Cargo.toml @@ -0,0 +1,10 @@ +[package] +name = "firefox-versioning" +version = "0.1.0" +authors = ["Nimbus Team "] +license = "MPL-2.0" +edition = "2021" + +[dependencies] +serde_json = "1.0" +thiserror = "1.0" diff --git a/components/support/firefox-versioning/src/compare.rs b/components/support/firefox-versioning/src/compare.rs new file mode 100644 index 000000000..bbab4e685 --- /dev/null +++ b/components/support/firefox-versioning/src/compare.rs @@ -0,0 +1,34 @@ +/* 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 crate::error::VersionParsingError; +use crate::version::Version; +use serde_json::{json, Value}; +use std::convert::TryFrom; + +pub type Result = std::result::Result; + +pub fn version_compare(args: &[Value]) -> Result { + let curr_version = args.first().ok_or_else(|| { + VersionParsingError::ParseError("current version doesn't exist in jexl transform".into()) + })?; + let curr_version = curr_version.as_str().ok_or_else(|| { + VersionParsingError::ParseError("current version in jexl transform is not a string".into()) + })?; + let min_version = args.get(1).ok_or_else(|| { + VersionParsingError::ParseError("minimum version doesn't exist in jexl transform".into()) + })?; + let min_version = min_version.as_str().ok_or_else(|| { + VersionParsingError::ParseError("minimum version is not a string in jexl transform".into()) + })?; + let min_version = Version::try_from(min_version)?; + let curr_version = Version::try_from(curr_version)?; + Ok(json!(if curr_version > min_version { + 1 + } else if curr_version < min_version { + -1 + } else { + 0 + })) +} diff --git a/components/support/firefox-versioning/src/error.rs b/components/support/firefox-versioning/src/error.rs new file mode 100644 index 000000000..9f3b76071 --- /dev/null +++ b/components/support/firefox-versioning/src/error.rs @@ -0,0 +1,17 @@ +/* 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/. */ + +/// The `VersioningError` indicates that a Firefox Version, being passed down as a `String`, +/// cannot be parsed into a `Version`. +/// +/// It can either be caused by a non-ASCII character or a integer overflow. +#[derive(Debug, PartialEq, thiserror::Error)] +pub enum VersionParsingError { + /// Indicates that a number overflowed. + #[error("Overflow Error: {0}")] + Overflow(String), + /// Indicates a general parsing error. + #[error("Parsing Error: {0}")] + ParseError(String), +} diff --git a/components/support/firefox-versioning/src/lib.rs b/components/support/firefox-versioning/src/lib.rs new file mode 100644 index 000000000..9350a24ab --- /dev/null +++ b/components/support/firefox-versioning/src/lib.rs @@ -0,0 +1,7 @@ +/* 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/. */ + +pub mod compare; +pub mod error; +pub mod version; diff --git a/components/nimbus/src/versioning.rs b/components/support/firefox-versioning/src/version.rs similarity index 91% rename from components/nimbus/src/versioning.rs rename to components/support/firefox-versioning/src/version.rs index 5f3c9731c..371ba985f 100644 --- a/components/nimbus/src/versioning.rs +++ b/components/support/firefox-versioning/src/version.rs @@ -2,8 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -//! ## Nimbus SDK App Version Comparison -//! The Nimbus SDK supports comparing app versions that follow the Firefox versioning scheme. +//! ## Firefox Version Comparison //! This module was ported from the Firefox Desktop implementation. You can find the Desktop implementation //! in [this C++ file](https://searchfox.org/mozilla-central/rev/468a65168dd0bc3c7d602211a566c16e66416cce/xpcom/base/nsVersionComparator.cpp) //! There's also some more documentation in the [IDL](https://searchfox.org/mozilla-central/rev/468a65168dd0bc3c7d602211a566c16e66416cce/xpcom/base/nsIVersionComparator.idl#9-31) @@ -49,7 +48,7 @@ //! ## Example version comparisons //! The following comparisons are taken directly from [the brief documentation in Mozilla-Central](https://searchfox.org/mozilla-central/rev/468a65168dd0bc3c7d602211a566c16e66416cce/xpcom/base/nsIVersionComparator.idl#9-31) //! ``` -//! use nimbus::versioning::Version; +//! use firefox_versioning::version::Version; //! let v1 = Version::try_from("1.0pre1").unwrap(); //! let v2 = Version::try_from("1.0pre2").unwrap(); //! let v3 = Version::try_from("1.0").unwrap(); @@ -84,19 +83,24 @@ //! < 1.1pre10a //! < 1.1pre10 -use crate::NimbusError; +use crate::error::VersionParsingError; use std::cmp::Ordering; #[derive(Debug, Default, Clone, PartialEq)] -pub(crate) struct VersionPart { - pub(crate) num_a: i32, - pub(crate) str_b: String, - pub(crate) num_c: i32, - pub(crate) extra_d: String, +pub struct VersionPart { + pub num_a: i32, + pub str_b: String, + pub num_c: i32, + pub extra_d: String, } +/// Represents a version in the form of a sequence of version parts. +/// +/// The `Version` struct is used to compare application versions that follow +/// a dot-separated format (e.g., `1.0.0`, `98.2pre1.0-beta`). Each part of the version +/// is represented by a `VersionPart`. #[derive(Debug, Default, Clone)] -pub struct Version(pub(crate) Vec); +pub struct Version(pub Vec); impl PartialEq for Version { fn eq(&self, other: &Self) -> bool { @@ -172,7 +176,7 @@ impl PartialOrd for VersionPart { } impl TryFrom<&'_ str> for Version { - type Error = NimbusError; + type Error = VersionParsingError; fn try_from(value: &'_ str) -> Result { let versions = value .split('.') @@ -183,15 +187,15 @@ impl TryFrom<&'_ str> for Version { } impl TryFrom for Version { - type Error = NimbusError; - fn try_from(curr_part: String) -> std::result::Result { + type Error = VersionParsingError; + fn try_from(curr_part: String) -> Result { curr_part.as_str().try_into() } } -fn char_at(value: &str, idx: usize) -> Result { +fn char_at(value: &str, idx: usize) -> Result { value.chars().nth(idx).ok_or_else(|| { - NimbusError::VersionParsingError(format!( + VersionParsingError::Overflow(format!( "Tried to access character {} in string {}, but it has size {}", idx, value, @@ -209,13 +213,13 @@ fn is_num_c(c: char) -> bool { c.is_numeric() || c == '+' || c == '-' } -fn parse_version_num(val: i32, res: &mut i32) -> Result<(), NimbusError> { +fn parse_version_num(val: i32, res: &mut i32) -> Result<(), VersionParsingError> { if *res == 0 { *res = val; } else { let res_l = *res as i64; if (res_l * 10) + val as i64 > i32::MAX as i64 { - return Err(NimbusError::VersionParsingError( + return Err(VersionParsingError::Overflow( "Number parsing overflows an i32".into(), )); } @@ -226,11 +230,11 @@ fn parse_version_num(val: i32, res: &mut i32) -> Result<(), NimbusError> { } impl TryFrom<&'_ str> for VersionPart { - type Error = NimbusError; + type Error = VersionParsingError; fn try_from(value: &'_ str) -> Result { if value.chars().any(|c| !c.is_ascii()) { - return Err(NimbusError::VersionParsingError(format!( + return Err(VersionParsingError::ParseError(format!( "version string {} contains non-ascii characters", value ))); diff --git a/components/nimbus/src/tests/test_versioning.rs b/components/support/firefox-versioning/tests/test_versioning.rs similarity index 95% rename from components/nimbus/src/tests/test_versioning.rs rename to components/support/firefox-versioning/tests/test_versioning.rs index 25fb376e5..65e60c5bb 100644 --- a/components/nimbus/src/tests/test_versioning.rs +++ b/components/support/firefox-versioning/tests/test_versioning.rs @@ -2,8 +2,10 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use crate::versioning::*; -use crate::{error::Result, NimbusError}; +use firefox_versioning::error::VersionParsingError; +use firefox_versioning::version::{Version, VersionPart}; + +pub type Result = std::result::Result; #[test] fn test_wild_card_to_version_part() -> Result<()> { @@ -162,7 +164,7 @@ fn test_compare_wild_card() -> Result<()> { #[test] fn test_non_ascii_throws_error() -> Result<()> { let err = Version::try_from("92🥲.1.2pre").expect_err("Should have thrown error"); - if let NimbusError::VersionParsingError(_) = err { + if let VersionParsingError::ParseError(_) = err { // Good! } else { panic!("Expected VersionParsingError, got {:?}", err) @@ -178,7 +180,7 @@ fn test_version_number_parsing_overflows() -> Result<()> { // this is greater than i32::MAX, should return an error let err = VersionPart::try_from("2147483648").expect_err("Should throw error, it overflows an i32"); - if let NimbusError::VersionParsingError(_) = err { + if let VersionParsingError::Overflow(_) = err { // OK } else { panic!("Expected a VersionParsingError, got {:?}", err)