From f97ae4f590e471008369f25e97d3ec01dd298777 Mon Sep 17 00:00:00 2001 From: James Hugman Date: Mon, 9 Oct 2023 17:11:00 +0100 Subject: [PATCH 01/10] Refactor FieldBody to FeatureFieldBody to add pref-key --- .../nimbus-fml/fixtures/fe/misc-features.yaml | 28 + .../src/backends/frontend_manifest.rs | 12 +- .../support/nimbus-fml/src/client/mod.rs | 22 +- .../nimbus-fml/src/command_line/workflows.rs | 2 + .../support/nimbus-fml/src/defaults_merger.rs | 79 +-- .../fixtures/intermediate_representation.rs | 11 +- components/support/nimbus-fml/src/frontend.rs | 25 +- .../src/intermediate_representation.rs | 530 ++++++++---------- components/support/nimbus-fml/src/parser.rs | 24 +- 9 files changed, 359 insertions(+), 374 deletions(-) create mode 100644 components/support/nimbus-fml/fixtures/fe/misc-features.yaml diff --git a/components/support/nimbus-fml/fixtures/fe/misc-features.yaml b/components/support/nimbus-fml/fixtures/fe/misc-features.yaml new file mode 100644 index 0000000000..94d6bf41e4 --- /dev/null +++ b/components/support/nimbus-fml/fixtures/fe/misc-features.yaml @@ -0,0 +1,28 @@ +--- +about: + description: A manifest that should round trip + android: + class: FooNimbus + package: com.example.nimbus + ios: + class: FooNimbus + module: App +channels: + - debug +features: + onboarding: + description: A feature containing a field with pref-key + variables: + enabled: + description: If true, enable new style onboarding. + type: Boolean + pref-key: enrollment_enabled + default: false + messaging: + description: A feature allowing coenrollment + allow-coenrollment: true + variables: + enabled: + description: If true, enable this feature + type: Boolean + default: false diff --git a/components/support/nimbus-fml/src/backends/frontend_manifest.rs b/components/support/nimbus-fml/src/backends/frontend_manifest.rs index fd06b43ead..ebc822e881 100644 --- a/components/support/nimbus-fml/src/backends/frontend_manifest.rs +++ b/components/support/nimbus-fml/src/backends/frontend_manifest.rs @@ -5,7 +5,8 @@ use std::collections::BTreeMap; use crate::frontend::{ - EnumBody, EnumVariantBody, FeatureBody, FieldBody, ManifestFrontEnd, ObjectBody, Types, + EnumBody, EnumVariantBody, FeatureBody, FeatureFieldBody, FieldBody, ManifestFrontEnd, + ObjectBody, Types, }; use crate::intermediate_representation::{ EnumDef, FeatureDef, FeatureManifest, ObjectDef, PropDef, VariantDef, @@ -119,3 +120,12 @@ impl From for FieldBody { } } } + +impl From for FeatureFieldBody { + fn from(value: PropDef) -> Self { + Self { + pref_key: value.pref_key.clone(), + field: value.into(), + } + } +} diff --git a/components/support/nimbus-fml/src/client/mod.rs b/components/support/nimbus-fml/src/client/mod.rs index c87f342922..4fef4de660 100644 --- a/components/support/nimbus-fml/src/client/mod.rs +++ b/components/support/nimbus-fml/src/client/mod.rs @@ -185,12 +185,11 @@ mod unit_tests { vec![], vec![FeatureDef { name: "feature_i".into(), - props: vec![PropDef { - name: "prop_i_1".into(), - typ: TypeRef::String, - default: Value::String("prop_i_1_value".into()), - doc: "".into(), - }], + props: vec![PropDef::new( + "prop_i_1", + TypeRef::String, + Value::String("prop_i_1_value".into()), + )], doc: "feature_i description".to_string(), ..Default::default() }], @@ -202,12 +201,11 @@ mod unit_tests { vec![], vec![FeatureDef { name: "feature".into(), - props: vec![PropDef { - name: "prop_1".into(), - typ: TypeRef::String, - default: Value::String("prop_1_value".into()), - doc: "".into(), - }], + props: vec![PropDef::new( + "prop_1", + TypeRef::String, + Value::String("prop_1_value".into()), + )], doc: "feature description".to_string(), allow_coenrollment: true, }], diff --git a/components/support/nimbus-fml/src/command_line/workflows.rs b/components/support/nimbus-fml/src/command_line/workflows.rs index adb2a48fc1..936fd7e22f 100644 --- a/components/support/nimbus-fml/src/command_line/workflows.rs +++ b/components/support/nimbus-fml/src/command_line/workflows.rs @@ -959,6 +959,8 @@ mod test { test_single_merged_manifest_file("fixtures/fe/importing/diamond/00-app.yaml", "debug")?; test_single_merged_manifest_file("fixtures/fe/importing/diamond/01-lib.yaml", "debug")?; test_single_merged_manifest_file("fixtures/fe/importing/diamond/02-sublib.yaml", "debug")?; + + test_single_merged_manifest_file("fixtures/fe/misc-features.yaml", "debug")?; Ok(()) } diff --git a/components/support/nimbus-fml/src/defaults_merger.rs b/components/support/nimbus-fml/src/defaults_merger.rs index ccfd3deae2..802b373ba0 100644 --- a/components/support/nimbus-fml/src/defaults_merger.rs +++ b/components/support/nimbus-fml/src/defaults_merger.rs @@ -1036,12 +1036,7 @@ mod unit_tests { #[test] fn test_merge_feature_default_overwrite_field_default_based_on_channel() -> Result<()> { let mut feature_def = FeatureDef { - props: vec![PropDef { - name: "button-color".into(), - default: json!("blue"), - doc: "".into(), - typ: TypeRef::String, - }], + props: vec![PropDef::new("button-color", TypeRef::String, json!("blue"))], ..Default::default() }; let default_blocks = serde_json::from_value(json!([ @@ -1073,12 +1068,11 @@ mod unit_tests { merger.merge_feature_defaults(&mut feature_def, &default_blocks)?; assert_eq!( feature_def.props, - vec![PropDef { - name: "button-color".into(), - default: json!("dark-green"), - doc: "".into(), - typ: TypeRef::String, - }] + vec![PropDef::new( + "button-color", + TypeRef::String, + json!("dark-green"), + )] ); Ok(()) } @@ -1087,12 +1081,7 @@ mod unit_tests { fn test_merge_feature_default_field_default_not_overwritten_if_no_feature_default_for_channel( ) -> Result<()> { let mut feature_def = FeatureDef { - props: vec![PropDef { - name: "button-color".into(), - default: json!("blue"), - doc: "".into(), - typ: TypeRef::String, - }], + props: vec![PropDef::new("button-color", TypeRef::String, json!("blue"))], ..Default::default() }; let default_blocks = serde_json::from_value(json!([{ @@ -1116,12 +1105,7 @@ mod unit_tests { merger.merge_feature_defaults(&mut feature_def, &default_blocks)?; assert_eq!( feature_def.props, - vec![PropDef { - name: "button-color".into(), - default: json!("blue"), - doc: "".into(), - typ: TypeRef::String, - }] + vec![PropDef::new("button-color", TypeRef::String, json!("blue"),)] ); Ok(()) } @@ -1129,9 +1113,10 @@ mod unit_tests { #[test] fn test_merge_feature_default_overwrite_nested_field_default() -> Result<()> { let mut feature_def = FeatureDef { - props: vec![PropDef { - name: "Dialog".into(), - default: json!({ + props: vec![PropDef::new( + "Dialog", + TypeRef::String, + json!({ "button-color": "blue", "title": "hello", "inner": { @@ -1139,9 +1124,7 @@ mod unit_tests { "other-field": "other-value" } }), - doc: "".into(), - typ: TypeRef::String, - }], + )], ..Default::default() }; @@ -1190,9 +1173,10 @@ mod unit_tests { merger.merge_feature_defaults(&mut feature_def, &default_blocks)?; assert_eq!( feature_def.props, - vec![PropDef { - name: "Dialog".into(), - default: json!({ + vec![PropDef::new( + "Dialog", + TypeRef::String, + json!({ "button-color": "green", "title": "hello", "inner": { @@ -1201,9 +1185,7 @@ mod unit_tests { "new-field": "new-value" } }), - doc: "".into(), - typ: TypeRef::String, - }] + )] ); Ok(()) } @@ -1212,12 +1194,7 @@ mod unit_tests { fn test_merge_feature_default_overwrite_field_default_based_on_channel_using_only_no_channel_default( ) -> Result<()> { let mut feature_def = FeatureDef { - props: vec![PropDef { - name: "button-color".into(), - default: json!("blue"), - doc: "".into(), - typ: TypeRef::String, - }], + props: vec![PropDef::new("button-color", TypeRef::String, json!("blue"))], ..Default::default() }; let default_blocks = serde_json::from_value(json!([ @@ -1250,12 +1227,11 @@ mod unit_tests { merger.merge_feature_defaults(&mut feature_def, &default_blocks)?; assert_eq!( feature_def.props, - vec![PropDef { - name: "button-color".into(), - default: json!("dark-green"), - doc: "".into(), - typ: TypeRef::String, - }] + vec![PropDef::new( + "button-color", + TypeRef::String, + json!("dark-green"), + )] ); Ok(()) } @@ -1264,12 +1240,7 @@ mod unit_tests { fn test_merge_feature_default_throw_error_if_property_not_found_on_feature() -> Result<()> { let mut feature_def = FeatureDef { name: "feature".into(), - props: vec![PropDef { - name: "button-color".into(), - default: json!("blue"), - doc: "".into(), - typ: TypeRef::String, - }], + props: vec![PropDef::new("button-color", TypeRef::String, json!("blue"))], ..Default::default() }; let default_blocks = serde_json::from_value(json!([ diff --git a/components/support/nimbus-fml/src/fixtures/intermediate_representation.rs b/components/support/nimbus-fml/src/fixtures/intermediate_representation.rs index 7f99c497d6..260b9a4610 100644 --- a/components/support/nimbus-fml/src/fixtures/intermediate_representation.rs +++ b/components/support/nimbus-fml/src/fixtures/intermediate_representation.rs @@ -31,21 +31,20 @@ pub(crate) fn get_simple_homescreen_feature() -> FeatureManifest { FeatureDef::new( "homescreen", "Represents the homescreen feature", - vec![PropDef { - name: "sections-enabled".into(), - doc: "A map of booleans".into(), - typ: TypeRef::EnumMap( + vec![PropDef::new( + "sections-enabled", + TypeRef::EnumMap( Box::new(TypeRef::Enum("HomeScreenSection".into())), Box::new(TypeRef::Boolean), ), - default: json!({ + json!({ "top-sites": true, "jump-back-in": false, "recently-saved": false, "recent-explorations": false, "pocket": false, }), - }], + )], false, ), )]), diff --git a/components/support/nimbus-fml/src/frontend.rs b/components/support/nimbus-fml/src/frontend.rs index 4bb0d65e5b..86b5132d1b 100644 --- a/components/support/nimbus-fml/src/frontend.rs +++ b/components/support/nimbus-fml/src/frontend.rs @@ -30,6 +30,18 @@ pub(crate) struct EnumBody { pub(crate) variants: BTreeMap, } +#[derive(Debug, Deserialize, Serialize, Clone)] +#[serde(deny_unknown_fields)] +#[serde(rename_all = "kebab-case")] +pub(crate) struct FeatureFieldBody { + #[serde(flatten)] + pub(crate) field: FieldBody, + + #[serde(default)] + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) pref_key: Option, +} + #[derive(Debug, Deserialize, Serialize, Clone)] #[serde(deny_unknown_fields)] pub(crate) struct FieldBody { @@ -128,7 +140,7 @@ pub(crate) struct FeatureBody { // 1. Swift insists on args in the same order they were declared. // 2. imported features are declared and constructed in different runs of the tool. #[serde(skip_serializing_if = "BTreeMap::is_empty")] - pub(crate) variables: BTreeMap, + pub(crate) variables: BTreeMap, #[serde(alias = "defaults")] #[serde(skip_serializing_if = "Option::is_none")] pub(crate) default: Option>, @@ -206,6 +218,12 @@ impl ManifestFrontEnd { .collect() } + fn get_prop_def_from_feature_field(&self, nm: &str, body: &FeatureFieldBody) -> PropDef { + let mut prop = self.get_prop_def_from_field(nm, &body.field); + prop.pref_key = body.pref_key.clone(); + prop + } + /// Transforms a front-end field definition, a tuple of [`String`] and [`FieldBody`], /// into a [`PropDef`] /// @@ -214,7 +232,7 @@ impl ManifestFrontEnd { /// /// # Returns /// return the IR [`PropDef`] - fn get_prop_def_from_field(&self, nm: &String, body: &FieldBody) -> PropDef { + fn get_prop_def_from_field(&self, nm: &str, body: &FieldBody) -> PropDef { let types = self.get_types(); PropDef { name: nm.into(), @@ -233,6 +251,7 @@ impl ManifestFrontEnd { } }, default: json!(body.default), + pref_key: None, } } @@ -245,7 +264,7 @@ impl ManifestFrontEnd { for (nm, body) in &self.features { let mut fields: Vec<_> = Default::default(); for (fnm, field) in &body.variables { - fields.push(self.get_prop_def_from_field(fnm, field)); + fields.push(self.get_prop_def_from_feature_field(fnm, field)); } let mut def = FeatureDef { name: nm.clone(), diff --git a/components/support/nimbus-fml/src/intermediate_representation.rs b/components/support/nimbus-fml/src/intermediate_representation.rs index 5f7ddcce65..061a61ddaa 100644 --- a/components/support/nimbus-fml/src/intermediate_representation.rs +++ b/components/support/nimbus-fml/src/intermediate_representation.rs @@ -109,6 +109,18 @@ impl Display for TypeRef { } } +impl TypeRef { + pub(crate) fn supports_prefs(&self) -> bool { + match self { + Self::Boolean | Self::String | Self::Int | Self::BundleText(_) => true, + // There may be a chance that we can get Self::Option to work, but not at this time. + // This may be done by adding a branch to this match and adding a `preference_getter` to + // the `OptionalCodeType`. + _ => false, + } + } +} + /** * An identifier derived from a `FilePath` of a top-level or importable FML file. * @@ -445,11 +457,25 @@ impl FeatureManifest { } } for feature in self.iter_feature_defs() { + self.validate_feature_structure(feature)?; self.validate_feature_def(feature)?; } Ok(()) } + fn validate_feature_structure(&self, feature_def: &FeatureDef) -> Result<()> { + for v in &feature_def.props { + if v.pref_key.is_some() && !v.typ.supports_prefs() { + return Err(FMLError::ValidationError( + format!("features/{}/{}", feature_def.name, v.name), + "Pref keys can only be used with Boolean, String, Int and Text variables" + .to_string(), + )); + } + } + Ok(()) + } + fn validate_feature_def(&self, feature_def: &FeatureDef) -> Result<()> { for prop in &feature_def.props { let path = format!("features/{}.{}", feature_def.name, prop.name); @@ -903,11 +929,14 @@ impl TypeFinder for ObjectDef { #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct PropDef { - pub name: String, - pub doc: String, + pub(crate) name: String, + pub(crate) doc: String, #[serde(rename = "type")] - pub typ: TypeRef, - pub default: Literal, + pub(crate) typ: TypeRef, + pub(crate) default: Literal, + #[serde(default)] + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) pref_key: Option, } impl PropDef { @@ -923,6 +952,12 @@ impl PropDef { pub fn default(&self) -> Literal { self.default.clone() } + pub fn has_prefs(&self) -> bool { + self.pref_key.is_some() && self.typ.supports_prefs() + } + pub fn pref_key(&self) -> Option { + self.pref_key.clone() + } } impl TypeFinder for PropDef { @@ -972,6 +1007,28 @@ pub mod unit_tests { use crate::error::Result; use crate::fixtures::intermediate_representation::get_simple_homescreen_feature; + impl PropDef { + pub(crate) fn new(nm: &str, typ: TypeRef, default: Value) -> Self { + PropDef { + name: nm.into(), + doc: format!("{nm} property of type {typ}"), + typ, + default, + pref_key: None, + } + } + + pub(crate) fn new_with_doc(nm: &str, doc: &str, typ: TypeRef, default: Value) -> Self { + PropDef { + name: nm.into(), + doc: doc.into(), + typ, + default, + pref_key: None, + } + } + } + #[test] fn can_ir_represent_smoke_test() -> Result<()> { let reference_manifest = get_simple_homescreen_feature(); @@ -1015,12 +1072,7 @@ pub mod unit_tests { fm.add_feature(FeatureDef::new( "some_def", "my lovely qtest doc", - vec![PropDef { - name: "some prop".into(), - doc: "some prop doc".into(), - typ: TypeRef::String, - default: json!("default"), - }], + vec![PropDef::new("some prop", TypeRef::String, json!("default"))], true, )); fm.validate_manifest()?; @@ -1036,19 +1088,18 @@ pub mod unit_tests { fm.add_feature(FeatureDef::new( "homescreen", "Represents the homescreen feature", - vec![PropDef { - name: "sections-enabled".into(), - doc: "A map of booleans".into(), - typ: TypeRef::EnumMap( + vec![PropDef::new( + "sections-enabled", + TypeRef::EnumMap( Box::new(TypeRef::Enum("SectionId".into())), Box::new(TypeRef::String), ), - default: json!({ + json!({ "top-sites": true, "jump-back-in": false, "recently-saved": false, }), - }], + )], false, )); fm.validate_manifest() @@ -1063,32 +1114,30 @@ pub mod unit_tests { "otherhomescreen", "Represents the homescreen feature", vec![ - PropDef { - name: "duplicate-prop".into(), - doc: "A map of booleans".into(), - typ: TypeRef::EnumMap( + PropDef::new( + "duplicate-prop", + TypeRef::EnumMap( Box::new(TypeRef::Enum("SectionId".into())), Box::new(TypeRef::String), ), - default: json!({ + json!({ "top-sites": true, "jump-back-in": false, "recently-saved": false, }), - }, - PropDef { - name: "duplicate-prop".into(), - doc: "A map of booleans".into(), - typ: TypeRef::EnumMap( + ), + PropDef::new( + "duplicate-prop", + TypeRef::EnumMap( Box::new(TypeRef::Enum("SectionId".into())), Box::new(TypeRef::String), ), - default: json!({ + json!({ "top-sites": true, "jump-back-in": false, "recently-saved": false, }), - }, + ), ], false, )); @@ -1103,12 +1152,11 @@ pub mod unit_tests { fm.add_feature(FeatureDef::new( "some_def", "test doc", - vec![PropDef { - name: "prop name".into(), - doc: "prop doc".into(), - typ: TypeRef::Enum("EnumDoesntExist".into()), - default: json!(null), - }], + vec![PropDef::new( + "prop name", + TypeRef::Enum("EnumDoesntExist".into()), + json!(null), + )], false, )); fm.validate_manifest().expect_err( @@ -1123,12 +1171,11 @@ pub mod unit_tests { fm.add_feature(FeatureDef::new( "some_def", "test doc", - vec![PropDef { - name: "prop name".into(), - doc: "prop doc".into(), - typ: TypeRef::Object("ObjDoesntExist".into()), - default: json!(null), - }], + vec![PropDef::new( + "prop name", + TypeRef::Object("ObjDoesntExist".into()), + json!(null), + )], false, )); fm.validate_manifest().expect_err( @@ -1143,12 +1190,11 @@ pub mod unit_tests { fm.add_feature(FeatureDef::new( "some_def", "test doc", - vec![PropDef { - name: "prop name".into(), - doc: "prop doc".into(), - typ: TypeRef::EnumMap(Box::new(TypeRef::String), Box::new(TypeRef::String)), - default: json!(null), - }], + vec![PropDef::new( + "prop name", + TypeRef::EnumMap(Box::new(TypeRef::String), Box::new(TypeRef::String)), + json!(null), + )], false, )); fm.validate_manifest() @@ -1162,12 +1208,11 @@ pub mod unit_tests { fm.add_feature(FeatureDef::new( "some_def", "test doc", - vec![PropDef { - name: "prop name".into(), - doc: "prop doc".into(), - typ: TypeRef::List(Box::new(TypeRef::Enum("EnumDoesntExist".into()))), - default: json!(null), - }], + vec![PropDef::new( + "prop name", + TypeRef::List(Box::new(TypeRef::Enum("EnumDoesntExist".into()))), + json!(null), + )], false, )); fm.validate_manifest() @@ -1181,15 +1226,14 @@ pub mod unit_tests { fm.add_feature(FeatureDef::new( "some_def", "test doc", - vec![PropDef { - name: "prop name".into(), - doc: "prop doc".into(), - typ: TypeRef::EnumMap( + vec![PropDef::new( + "prop name", + TypeRef::EnumMap( Box::new(TypeRef::Enum("EnumDoesntExist".into())), Box::new(TypeRef::String), ), - default: json!(null), - }], + json!(null), + )], false, )); fm.validate_manifest().expect_err( @@ -1204,15 +1248,14 @@ pub mod unit_tests { fm.add_feature(FeatureDef::new( "some_def", "test doc", - vec![PropDef { - name: "prop name".into(), - doc: "prop doc".into(), - typ: TypeRef::EnumMap( + vec![PropDef::new( + "prop name", + TypeRef::EnumMap( Box::new(TypeRef::Enum("SectionId".into())), Box::new(TypeRef::Object("ObjDoesntExist".into())), ), - default: json!(null), - }], + json!(null), + )], false, )); fm.validate_manifest() @@ -1226,12 +1269,11 @@ pub mod unit_tests { fm.add_feature(FeatureDef::new( "some_def", "test doc", - vec![PropDef { - name: "prop name".into(), - doc: "prop doc".into(), - typ: TypeRef::StringMap(Box::new(TypeRef::Enum("EnumDoesntExist".into()))), - default: json!(null), - }], + vec![PropDef::new( + "prop name", + TypeRef::StringMap(Box::new(TypeRef::Enum("EnumDoesntExist".into()))), + json!(null), + )], false, )); fm.validate_manifest() @@ -1245,12 +1287,11 @@ pub mod unit_tests { fm.add_feature(FeatureDef::new( "some_def", "test doc", - vec![PropDef { - name: "prop name".into(), - doc: "prop doc".into(), - typ: TypeRef::Option(Box::new(TypeRef::Option(Box::new(TypeRef::String)))), - default: json!(null), - }], + vec![PropDef::new( + "prop name", + TypeRef::Option(Box::new(TypeRef::Option(Box::new(TypeRef::String)))), + json!(null), + )], false, )); fm.validate_manifest() @@ -1328,12 +1369,7 @@ pub mod unit_tests { #[test] fn test_validate_prop_defaults_string() -> Result<()> { - let mut prop = PropDef { - name: "key".into(), - doc: "simple string property".into(), - typ: TypeRef::String, - default: json!("default!"), - }; + let mut prop = PropDef::new("key", TypeRef::String, json!("default!")); let fm = get_one_prop_feature_manifest(vec![], vec![], &prop); let path = format!("test_validate_prop_defaults_string.{}", prop.name); fm.validate_prop_defaults(&path, &prop)?; @@ -1346,12 +1382,7 @@ pub mod unit_tests { #[test] fn test_validate_prop_defaults_int() -> Result<()> { - let mut prop = PropDef { - name: "key".into(), - doc: "simple integer property".into(), - typ: TypeRef::Int, - default: json!(100), - }; + let mut prop = PropDef::new("key", TypeRef::Int, json!(100)); let fm = get_one_prop_feature_manifest(vec![], vec![], &prop); let path = format!("test_validate_prop_defaults_int.{}", prop.name); fm.validate_prop_defaults(&path, &prop)?; @@ -1364,12 +1395,7 @@ pub mod unit_tests { #[test] fn test_validate_prop_defaults_bool() -> Result<()> { - let mut prop = PropDef { - name: "key".into(), - doc: "simple boolean property".into(), - typ: TypeRef::Boolean, - default: json!(true), - }; + let mut prop = PropDef::new("key", TypeRef::Boolean, json!(true)); let fm = get_one_prop_feature_manifest(vec![], vec![], &prop); let path = format!("test_validate_prop_defaults_bool.{}", prop.name); fm.validate_prop_defaults(&path, &prop)?; @@ -1382,12 +1408,11 @@ pub mod unit_tests { #[test] fn test_validate_prop_defaults_bundle_image() -> Result<()> { - let mut prop = PropDef { - name: "key".into(), - doc: "bundleImage string property".into(), - typ: TypeRef::BundleImage("Icon".into()), - default: json!("IconBlue"), - }; + let mut prop = PropDef::new( + "key", + TypeRef::BundleImage("Icon".into()), + json!("IconBlue"), + ); let fm = get_one_prop_feature_manifest(vec![], vec![], &prop); let path = format!("test_validate_prop_defaults_bundle_image.{}", prop.name); fm.validate_prop_defaults(&path, &prop)?; @@ -1401,12 +1426,11 @@ pub mod unit_tests { #[test] fn test_validate_prop_defaults_bundle_text() -> Result<()> { - let mut prop = PropDef { - name: "key".into(), - doc: "bundleText string property".into(), - typ: TypeRef::BundleText("Text".into()), - default: json!("BundledText"), - }; + let mut prop = PropDef::new( + "key", + TypeRef::BundleText("Text".into()), + json!("BundledText"), + ); let fm = get_one_prop_feature_manifest(vec![], vec![], &prop); let path = format!("test_validate_prop_defaults_bundle_text.{}", prop.name); fm.validate_prop_defaults(&path, &prop)?; @@ -1420,12 +1444,11 @@ pub mod unit_tests { #[test] fn test_validate_prop_defaults_option_null() -> Result<()> { - let mut prop = PropDef { - name: "key".into(), - doc: "Optional boolean property".into(), - typ: TypeRef::Option(Box::new(TypeRef::Boolean)), - default: json!(null), - }; + let mut prop = PropDef::new( + "key", + TypeRef::Option(Box::new(TypeRef::Boolean)), + json!(null), + ); let fm = get_one_prop_feature_manifest(vec![], vec![], &prop); let path = format!("test_validate_prop_defaults_option_null.{}", prop.name); fm.validate_prop_defaults(&path, &prop)?; @@ -1439,12 +1462,11 @@ pub mod unit_tests { #[test] fn test_validate_prop_defaults_nested_options() -> Result<()> { - let prop = PropDef { - name: "key".into(), - doc: "nested boolean optional property".into(), - typ: TypeRef::Option(Box::new(TypeRef::Option(Box::new(TypeRef::Boolean)))), - default: json!(true), - }; + let prop = PropDef::new( + "key", + TypeRef::Option(Box::new(TypeRef::Option(Box::new(TypeRef::Boolean)))), + json!(true), + ); let fm = get_one_prop_feature_manifest(vec![], vec![], &prop); let path = format!("test_validate_prop_defaults_nested_options.{}", prop.name); fm.validate_prop_defaults(&path, &prop) @@ -1454,12 +1476,11 @@ pub mod unit_tests { #[test] fn test_validate_prop_defaults_option_non_null() -> Result<()> { - let mut prop = PropDef { - name: "key".into(), - doc: "optional boolean property".into(), - typ: TypeRef::Option(Box::new(TypeRef::Boolean)), - default: json!(true), - }; + let mut prop = PropDef::new( + "key", + TypeRef::Option(Box::new(TypeRef::Boolean)), + json!(true), + ); let fm = get_one_prop_feature_manifest(vec![], vec![], &prop); let path = format!("test_validate_prop_defaults_option_non_null.{}", prop.name); fm.validate_prop_defaults(&path, &prop)?; @@ -1473,12 +1494,7 @@ pub mod unit_tests { #[test] fn test_validate_prop_defaults_enum() -> Result<()> { - let mut prop = PropDef { - name: "key".into(), - doc: "enum property".into(), - typ: TypeRef::Enum("ButtonColor".into()), - default: json!("blue"), - }; + let mut prop = PropDef::new("key", TypeRef::Enum("ButtonColor".into()), json!("blue")); let enum_defs = vec![EnumDef { name: "ButtonColor".into(), variants: vec![ @@ -1513,18 +1529,17 @@ pub mod unit_tests { #[test] fn test_validate_prop_defaults_enum_map() -> Result<()> { - let mut prop = PropDef { - name: "key".into(), - doc: "enum map property".into(), - typ: TypeRef::EnumMap( + let mut prop = PropDef::new( + "key", + TypeRef::EnumMap( Box::new(TypeRef::Enum("ButtonColor".into())), Box::new(TypeRef::Int), ), - default: json!({ + json!({ "blue": 1, "green": 22, }), - }; + ); let enum_defs = vec![EnumDef { name: "ButtonColor".into(), variants: vec![ @@ -1560,15 +1575,14 @@ pub mod unit_tests { #[test] fn test_validate_prop_defaults_string_map() -> Result<()> { - let mut prop = PropDef { - name: "key".into(), - doc: "string map property".into(), - typ: TypeRef::StringMap(Box::new(TypeRef::Int)), - default: json!({ + let mut prop = PropDef::new( + "key", + TypeRef::StringMap(Box::new(TypeRef::Int)), + json!({ "blue": 1, "green": 22, }), - }; + ); let path = format!("test_validate_prop_defaults_string_map.{}", &prop.name); let fm = get_one_prop_feature_manifest(vec![], vec![], &prop); fm.validate_prop_defaults(&path, &prop)?; @@ -1590,12 +1604,11 @@ pub mod unit_tests { #[test] fn test_validate_prop_defaults_list() -> Result<()> { - let mut prop = PropDef { - name: "key".into(), - doc: "list property".into(), - typ: TypeRef::List(Box::new(TypeRef::Int)), - default: json!([1, 3, 100]), - }; + let mut prop = PropDef::new( + "key", + TypeRef::List(Box::new(TypeRef::Int)), + json!([1, 3, 100]), + ); let path = format!("test_validate_prop_defaults_list.{}", &prop.name); let fm = get_one_prop_feature_manifest(vec![], vec![], &prop); fm.validate_prop_defaults(&path, &prop)?; @@ -1608,11 +1621,10 @@ pub mod unit_tests { #[test] fn test_validate_prop_defaults_object() -> Result<()> { - let mut prop = PropDef { - name: "key".into(), - doc: "simple string property".into(), - typ: TypeRef::Object("SampleObj".into()), - default: json!({ + let mut prop = PropDef::new( + "key", + TypeRef::Object("SampleObj".into()), + json!({ "int": 1, "string": "bobo", "enum": "green", @@ -1625,68 +1637,49 @@ pub mod unit_tests { }, "optional": 2, }), - }; + ); let obj_defs = vec![ ObjectDef { name: "SampleObj".into(), props: vec![ - PropDef { - name: "int".into(), - typ: TypeRef::Int, - doc: "".into(), - default: json!(1), - }, - PropDef { - name: "string".into(), - typ: TypeRef::String, - doc: "".into(), - default: json!("a string"), - }, - PropDef { - name: "enum".into(), - typ: TypeRef::Enum("ButtonColor".into()), - doc: "".into(), - default: json!("blue"), - }, - PropDef { - name: "list".into(), - typ: TypeRef::List(Box::new(TypeRef::Boolean)), - doc: "".into(), - default: json!([true, false]), - }, - PropDef { - name: "optional".into(), - typ: TypeRef::Option(Box::new(TypeRef::Int)), - doc: "".into(), - default: json!(null), - }, - PropDef { - name: "nestedObj".into(), - typ: TypeRef::Object("NestedObject".into()), - doc: "".into(), - default: json!({ + PropDef::new("int", TypeRef::Int, json!(1)), + PropDef::new("string", TypeRef::String, json!("a string")), + PropDef::new("enum", TypeRef::Enum("ButtonColor".into()), json!("blue")), + PropDef::new( + "list", + TypeRef::List(Box::new(TypeRef::Boolean)), + json!([true, false]), + ), + PropDef::new( + "optional", + TypeRef::Option(Box::new(TypeRef::Int)), + json!(null), + ), + PropDef::new( + "nestedObj", + TypeRef::Object("NestedObject".into()), + json!({ "enumMap": { "blue": 1, }, }), - }, + ), ], ..Default::default() }, ObjectDef { name: "NestedObject".into(), - props: vec![PropDef { - name: "enumMap".into(), - typ: TypeRef::EnumMap( + props: vec![PropDef::new( + "enumMap", + TypeRef::EnumMap( Box::new(TypeRef::Enum("ButtonColor".into())), Box::new(TypeRef::Int), ), - doc: "".into(), - default: json!({ + json!({ "blue": 4, "green": 2, }), - }], + )], ..Default::default() }, ]; @@ -1778,17 +1771,16 @@ pub mod unit_tests { #[test] fn test_validate_prop_defaults_enum_map_optional() -> Result<()> { - let prop = PropDef { - name: "key".into(), - doc: "enum map property".into(), - typ: TypeRef::EnumMap( + let prop = PropDef::new( + "key", + TypeRef::EnumMap( Box::new(TypeRef::Enum("ButtonColor".into())), Box::new(TypeRef::Option(Box::new(TypeRef::Int))), ), - default: json!({ + json!({ "blue": 1, }), - }; + ); let enum_defs = vec![EnumDef { name: "ButtonColor".into(), variants: vec![ @@ -1812,42 +1804,30 @@ pub mod unit_tests { #[test] fn test_iter_object_defs_deep_iterates_on_all_imports() -> Result<()> { - let prop_i = PropDef { - name: "key_i".into(), - doc: "simple string property".into(), - typ: TypeRef::Object("SampleObjImported".into()), - default: json!({ + let prop_i = PropDef::new( + "key_i", + TypeRef::Object("SampleObjImported".into()), + json!({ "string": "bobo", }), - }; + ); let obj_defs_i = vec![ObjectDef { name: "SampleObjImported".into(), - props: vec![PropDef { - name: "string".into(), - typ: TypeRef::String, - doc: "".into(), - default: json!("a string"), - }], + props: vec![PropDef::new("string", TypeRef::String, json!("a string"))], ..Default::default() }]; let fm_i = get_one_prop_feature_manifest(obj_defs_i, vec![], &prop_i); - let prop = PropDef { - name: "key".into(), - doc: "simple string property".into(), - typ: TypeRef::Object("SampleObj".into()), - default: json!({ + let prop = PropDef::new( + "key", + TypeRef::Object("SampleObj".into()), + json!({ "string": "bobo", }), - }; + ); let obj_defs = vec![ObjectDef { name: "SampleObj".into(), - props: vec![PropDef { - name: "string".into(), - typ: TypeRef::String, - doc: "".into(), - default: json!("a string"), - }], + props: vec![PropDef::new("string", TypeRef::String, json!("a string"))], ..Default::default() }]; let fm = get_one_prop_feature_manifest_with_imports( @@ -1867,20 +1847,10 @@ pub mod unit_tests { #[test] fn test_iter_feature_defs_deep_iterates_on_all_imports() -> Result<()> { - let prop_i = PropDef { - name: "key_i".into(), - doc: "simple string property".into(), - typ: TypeRef::String, - default: json!("string"), - }; + let prop_i = PropDef::new("key_i", TypeRef::String, json!("string")); let fm_i = get_one_prop_feature_manifest(vec![], vec![], &prop_i); - let prop = PropDef { - name: "key".into(), - doc: "simple string property".into(), - typ: TypeRef::String, - default: json!("string"), - }; + let prop = PropDef::new("key", TypeRef::String, json!("string")); let fm = get_one_prop_feature_manifest_with_imports( vec![], vec![], @@ -2024,12 +1994,11 @@ pub mod unit_tests { vec![], vec![FeatureDef { name: "feature_i".into(), - props: vec![PropDef { - name: "prop_i_1".into(), - typ: TypeRef::String, - default: Value::String("prop_i_1_value".into()), - doc: "".into(), - }], + props: vec![PropDef::new( + "prop_i_1", + TypeRef::String, + Value::String("prop_i_1_value".into()), + )], ..Default::default() }], HashMap::new(), @@ -2040,12 +2009,11 @@ pub mod unit_tests { vec![], vec![FeatureDef { name: "feature".into(), - props: vec![PropDef { - name: "prop_1".into(), - typ: TypeRef::String, - default: Value::String("prop_1_value".into()), - doc: "".into(), - }], + props: vec![PropDef::new( + "prop_1", + TypeRef::String, + Value::String("prop_1_value".into()), + )], ..Default::default() }], HashMap::from([(ModuleId::Local("test".into()), fm_i)]), @@ -2071,12 +2039,11 @@ pub mod unit_tests { vec![], vec![FeatureDef { name: "feature".into(), - props: vec![PropDef { - name: "prop_1".into(), - typ: TypeRef::String, - default: Value::String("prop_1_value".into()), - doc: "".into(), - }], + props: vec![PropDef::new( + "prop_1", + TypeRef::String, + Value::String("prop_1_value".into()), + )], ..Default::default() }], HashMap::new(), @@ -2101,12 +2068,11 @@ pub mod unit_tests { vec![], vec![FeatureDef { name: "feature".into(), - props: vec![PropDef { - name: "prop_1".into(), - typ: TypeRef::String, - default: Value::String("prop_1_value".into()), - doc: "".into(), - }], + props: vec![PropDef::new( + "prop_1", + TypeRef::String, + Value::String("prop_1_value".into()), + )], ..Default::default() }], HashMap::new(), @@ -2135,12 +2101,11 @@ pub mod unit_tests { vec![], vec![FeatureDef { name: "feature".into(), - props: vec![PropDef { - name: "prop_1".into(), - typ: TypeRef::Option(Box::new(TypeRef::String)), - default: Value::Null, - doc: "".into(), - }], + props: vec![PropDef::new( + "prop_1", + TypeRef::Option(Box::new(TypeRef::String)), + Value::Null, + )], ..Default::default() }], HashMap::new(), @@ -2169,12 +2134,11 @@ pub mod unit_tests { vec![], vec![FeatureDef { name: "feature".into(), - props: vec![PropDef { - name: "prop_1".into(), - typ: TypeRef::String, - default: Value::String("prop_1_value".into()), - doc: "".into(), - }], + props: vec![PropDef::new( + "prop_1", + TypeRef::String, + json!("prop_1_value"), + )], ..Default::default() }], HashMap::new(), @@ -2197,12 +2161,7 @@ pub mod unit_tests { fn test_validate_feature_config_errors_on_invalid_object_prop() -> Result<()> { let obj_defs = vec![ObjectDef { name: "SampleObj".into(), - props: vec![PropDef { - name: "string".into(), - typ: TypeRef::String, - doc: "".into(), - default: json!("a string"), - }], + props: vec![PropDef::new("string", TypeRef::String, json!("a string"))], ..Default::default() }]; let fm = get_feature_manifest( @@ -2210,14 +2169,13 @@ pub mod unit_tests { vec![], vec![FeatureDef { name: "feature".into(), - props: vec![PropDef { - name: "prop_1".into(), - typ: TypeRef::Object("SampleObj".into()), - default: json!({ + props: vec![PropDef::new( + "prop_1", + TypeRef::Object("SampleObj".into()), + json!({ "string": "a value" }), - doc: "".into(), - }], + )], ..Default::default() }], HashMap::new(), diff --git a/components/support/nimbus-fml/src/parser.rs b/components/support/nimbus-fml/src/parser.rs index 830a214f0b..de3284dd20 100644 --- a/components/support/nimbus-fml/src/parser.rs +++ b/components/support/nimbus-fml/src/parser.rs @@ -558,18 +558,18 @@ mod unit_tests { let obj_def = &ir.obj_defs["Button"]; assert!(obj_def.name == *"Button"); assert!(obj_def.doc == *"This is a button object"); - assert!(obj_def.props.contains(&PropDef { - name: "label".to_string(), - doc: "This is the label for the button".to_string(), - typ: TypeRef::String, - default: serde_json::Value::String("REQUIRED FIELD".to_string()), - })); - assert!(obj_def.props.contains(&PropDef { - name: "color".to_string(), - doc: "This is the color of the button".to_string(), - typ: TypeRef::Option(Box::new(TypeRef::String)), - default: serde_json::Value::Null, - })); + assert!(obj_def.props.contains(&PropDef::new_with_doc( + "label", + "This is the label for the button", + TypeRef::String, + serde_json::Value::String("REQUIRED FIELD".to_string()), + ))); + assert!(obj_def.props.contains(&PropDef::new_with_doc( + "color", + "This is the color of the button", + TypeRef::Option(Box::new(TypeRef::String)), + serde_json::Value::Null, + ))); // Validate parsed features assert!(ir.feature_defs.len() == 1); From 1d9e3a1e2fe6fd0b35739706b4f6e7f91af4204b Mon Sep 17 00:00:00 2001 From: James Hugman Date: Mon, 9 Oct 2023 15:28:13 +0100 Subject: [PATCH 02/10] Kotlin: Add SharedPreferences to helper files --- .../experiments/nimbus/FeaturesInterface.kt | 4 +++ .../org/mozilla/experiments/nimbus/Nimbus.kt | 2 ++ .../experiments/nimbus/NimbusBuilder.kt | 7 +++++ .../nimbus/internal/FeatureHolder.kt | 8 ++++-- .../nimbus/util/TestNimbusBuilder.kt | 11 +++++++- .../fixtures/android/runtime/Context.kt | 8 ++++++ .../src/backends/kotlin/gen_structs/mod.rs | 1 + .../templates/FeatureManifestTemplate.kt | 4 +-- .../ImportedModuleInitializationTemplate.kt | 8 ++++-- .../src/backends/kotlin/templates/macros.kt | 28 +++++++++++++++---- .../nimbus-fml/test/full_homescreen.kts | 2 +- .../test/threadsafe_feature_holder.kts | 2 +- 12 files changed, 68 insertions(+), 17 deletions(-) diff --git a/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/FeaturesInterface.kt b/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/FeaturesInterface.kt index fcae2492cd..5e2c29f90a 100644 --- a/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/FeaturesInterface.kt +++ b/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/FeaturesInterface.kt @@ -5,6 +5,7 @@ package org.mozilla.experiments.nimbus import android.content.Context +import android.content.SharedPreferences /** * Small interface to get the feature variables out of the Nimbus SDK. @@ -15,6 +16,9 @@ interface FeaturesInterface { val context: Context + val prefs: SharedPreferences? + get() = null + /** * Get the variables needed to configure the feature given by `featureId`. * diff --git a/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt b/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt index 429cc74cd9..d68abf7bf6 100644 --- a/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt +++ b/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt @@ -7,6 +7,7 @@ package org.mozilla.experiments.nimbus import android.content.Context +import android.content.SharedPreferences import android.content.pm.PackageInfo import android.content.pm.PackageManager import android.net.Uri @@ -58,6 +59,7 @@ data class NimbusServerSettings( @Suppress("LargeClass", "LongParameterList") open class Nimbus( override val context: Context, + override val prefs: SharedPreferences? = null, appInfo: NimbusAppInfo, coenrollingFeatureIds: List, server: NimbusServerSettings?, diff --git a/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/NimbusBuilder.kt b/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/NimbusBuilder.kt index 5ec8cae604..92c88ecae7 100644 --- a/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/NimbusBuilder.kt +++ b/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/NimbusBuilder.kt @@ -5,6 +5,7 @@ package org.mozilla.experiments.nimbus import android.content.Context +import android.content.SharedPreferences import android.net.Uri import androidx.annotation.RawRes import kotlinx.coroutines.runBlocking @@ -83,6 +84,11 @@ abstract class AbstractNimbusBuilder(val context: Context) */ var featureManifest: FeatureManifestInterface<*>? = null + /** + * The shared preferences used to configure the app. + */ + var sharedPreferences: SharedPreferences? = null + /** * Build a [Nimbus] singleton for the given [NimbusAppInfo]. Instances built with this method * have been initialized, and are ready for use by the app. @@ -210,6 +216,7 @@ class DefaultNimbusBuilder(context: Context) : AbstractNimbusBuilder( private var getSdk: () -> FeaturesInterface?, private val featureId: String, - private var create: (Variables) -> T, + private var create: (Variables, SharedPreferences?) -> T, ) { private val lock = ReentrantLock() @@ -48,7 +49,8 @@ class FeatureHolder( val variables = getSdk()?.getVariables(featureId, false) ?: run { NullVariables.instance } - create(variables).also { value -> + val prefs = getSdk()?.prefs + create(variables, prefs).also { value -> cachedValue = value } } @@ -108,7 +110,7 @@ class FeatureHolder( * * This is most likely useful during testing and other generated code. */ - fun withInitializer(create: (Variables) -> T) { + fun withInitializer(create: (Variables, SharedPreferences?) -> T) { lock.runBlock { this.create = create this.cachedValue = null diff --git a/components/nimbus/android/src/test/java/org/mozilla/experiments/nimbus/util/TestNimbusBuilder.kt b/components/nimbus/android/src/test/java/org/mozilla/experiments/nimbus/util/TestNimbusBuilder.kt index bdaf64a883..41a83db3c8 100644 --- a/components/nimbus/android/src/test/java/org/mozilla/experiments/nimbus/util/TestNimbusBuilder.kt +++ b/components/nimbus/android/src/test/java/org/mozilla/experiments/nimbus/util/TestNimbusBuilder.kt @@ -19,7 +19,16 @@ class TestNimbusBuilder(context: Context) : AbstractNimbusBuilder FeatureManifestDeclaration<'a> { "org.mozilla.experiments.nimbus.internal.FeatureManifestInterface".to_string(), "org.mozilla.experiments.nimbus.FeaturesInterface".to_string(), "org.json.JSONObject".to_string(), + "android.content.SharedPreferences".to_string(), ]) .filter(|i| i != &my_package) .collect::>() diff --git a/components/support/nimbus-fml/src/backends/kotlin/templates/FeatureManifestTemplate.kt b/components/support/nimbus-fml/src/backends/kotlin/templates/FeatureManifestTemplate.kt index 66cfe60276..a80d89a585 100644 --- a/components/support/nimbus-fml/src/backends/kotlin/templates/FeatureManifestTemplate.kt +++ b/components/support/nimbus-fml/src/backends/kotlin/templates/FeatureManifestTemplate.kt @@ -39,8 +39,8 @@ object {{ nimbus_object }} : FeatureManifestInterface<{{ nimbus_object }}.Featur {%- let class_name = raw_name|class_name %} {{ f.doc()|comment(" ") }} val {{raw_name|var_name}}: FeatureHolder<{{class_name}}> by lazy { - FeatureHolder({{ nimbus_object }}.getSdk, {{ raw_name|quoted }}) { variables -> - {{ class_name }}(variables) + FeatureHolder({{ nimbus_object }}.getSdk, {{ raw_name|quoted }}) { variables, prefs -> + {{ class_name }}(variables, prefs) } } {%- endfor %} diff --git a/components/support/nimbus-fml/src/backends/kotlin/templates/ImportedModuleInitializationTemplate.kt b/components/support/nimbus-fml/src/backends/kotlin/templates/ImportedModuleInitializationTemplate.kt index 33e61d1254..9d4717e30d 100644 --- a/components/support/nimbus-fml/src/backends/kotlin/templates/ImportedModuleInitializationTemplate.kt +++ b/components/support/nimbus-fml/src/backends/kotlin/templates/ImportedModuleInitializationTemplate.kt @@ -1,15 +1,17 @@ {%- import "macros.kt" as kt %} {%- let variables = "variables" %} +{%- let prefs = "prefs" %} {%- let context = "variables.context" %} {%- let class_name = self.inner.about().nimbus_object_name_kt() %} {{- class_name }}.features.apply { {%- for f in self.inner.features() %} - {{ f.name()|var_name }}.withInitializer { {{ variables }} -> + {{ f.name()|var_name }}.withInitializer { {{ variables }}: Variables, {{ prefs }}: SharedPreferences? -> {{ f.name()|class_name }}( - {{ variables }}, {%- for p in f.props() %} + _variables = {{ variables }}, + _prefs = {{ prefs }}, {%- for p in f.props() %} {{p.name()|var_name}} = {{ p.typ()|literal(self, p.default(), context) }}{% if !loop.last %},{% endif %} {%- endfor %} ) } {%- endfor %} - } \ No newline at end of file + } diff --git a/components/support/nimbus-fml/src/backends/kotlin/templates/macros.kt b/components/support/nimbus-fml/src/backends/kotlin/templates/macros.kt index 08e0845a73..816a85ee3c 100644 --- a/components/support/nimbus-fml/src/backends/kotlin/templates/macros.kt +++ b/components/support/nimbus-fml/src/backends/kotlin/templates/macros.kt @@ -11,6 +11,7 @@ {% macro render_constructor() -%} private constructor( private val _variables: Variables, + private val _prefs: SharedPreferences? = null, private val _defaults: Defaults) {% endmacro %} @@ -27,12 +28,13 @@ private constructor( {#- A constructor for application tests to use. #} - constructor(_variables: Variables = NullVariables.instance, {% for p in inner.props() %} + constructor(_variables: Variables = NullVariables.instance, _prefs: SharedPreferences? = null, {% for p in inner.props() %} {%- let t = p.typ() %} {{p.name()|var_name}}: {{ t|defaults_type_label }} = {{ t|literal(self, p.default(), "_variables.context") }}{% if !loop.last %},{% endif %} {%- endfor %} ) : this( _variables = _variables, + _prefs = _prefs, _defaults = Defaults({% for p in inner.props() %} {%- let nm = p.name()|var_name %}{{ nm }} = {{ nm }}{% if !loop.last %}, {% endif %} {%- endfor %}) @@ -41,11 +43,23 @@ private constructor( {# The property initializers #} {%- for p in inner.props() %} {%- let prop_kt = p.name()|var_name %} - {{ p.doc()|comment(" ") }} - val {{ prop_kt }}: {{ p.typ()|type_label }} by lazy { - {%- let defaults = format!("_defaults.{}", prop_kt) %} - {{ p.typ()|property(p.name(), "_variables", defaults)}} + {%- let type_kt = p.typ()|type_label %} + {%- let defaults = format!("_defaults.{}", prop_kt) %} + {%- let getter = p.typ()|property(p.name(), "_variables", defaults) %} + {{ p.doc()|comment(" ") }} + {%- if p.supports_lazy() %} + val {{ prop_kt }}: {{ type_kt }} by lazy { + {{ getter }} } + {%- else %} + val {{ prop_kt }}: {{ type_kt }} + get() { + fun getter() = {{ getter }} + return {% call prefs() %}?.let { _ -> + getter() + } ?: getter() + } + {%- endif %} {% endfor %} {#- toJSON #} @@ -57,4 +71,6 @@ private constructor( {%- endfor %} ) ) -{% endmacro %} \ No newline at end of file +{% endmacro %} + +{% macro prefs() %}this._prefs{% endmacro %} diff --git a/components/support/nimbus-fml/test/full_homescreen.kts b/components/support/nimbus-fml/test/full_homescreen.kts index 5eecf8fb49..eee04a7696 100644 --- a/components/support/nimbus-fml/test/full_homescreen.kts +++ b/components/support/nimbus-fml/test/full_homescreen.kts @@ -20,7 +20,7 @@ val api = MockNimbus("homescreen" to """{ "pocket": true } }""") -val holder = FeatureHolder(getSdk = { api }, featureId = "homescreen") { Homescreen(it) } +val holder = FeatureHolder(getSdk = { api }, featureId = "homescreen") { v, _ -> Homescreen(v) } val feature1 = holder.value() assert(feature1.sectionsEnabled[HomeScreenSection.TOP_SITES] == true) assert(feature1.sectionsEnabled[HomeScreenSection.JUMP_BACK_IN] == false) diff --git a/components/support/nimbus-fml/test/threadsafe_feature_holder.kts b/components/support/nimbus-fml/test/threadsafe_feature_holder.kts index 1ed66a074a..3e9f1f0b2d 100644 --- a/components/support/nimbus-fml/test/threadsafe_feature_holder.kts +++ b/components/support/nimbus-fml/test/threadsafe_feature_holder.kts @@ -20,7 +20,7 @@ class Feature(val string: String): FMLFeatureInterface { JSONObject(mapOf("string" to string)) } -val holder = FeatureHolder({ api }, featureId = "test-feature-holder") { Feature("NO CRASH") } +val holder = FeatureHolder({ api }, featureId = "test-feature-holder") { v, p -> Feature("NO CRASH") } repeat(10000) { scope.submit { From 13b1faf08b41849297e52b9263bd1431225e7e2e Mon Sep 17 00:00:00 2001 From: James Hugman Date: Mon, 9 Oct 2023 22:40:32 +0100 Subject: [PATCH 03/10] Kotlin: Generating preference getting in templated code --- .../backends/kotlin/gen_structs/bundled.rs | 12 ++++++++- .../backends/kotlin/gen_structs/filters.rs | 14 ++++++++++ .../backends/kotlin/gen_structs/primitives.rs | 27 +++++++++++++++++++ .../src/backends/kotlin/templates/macros.kt | 24 ++++++++++++----- .../support/nimbus-fml/src/backends/mod.rs | 9 +++++++ 5 files changed, 78 insertions(+), 8 deletions(-) diff --git a/components/support/nimbus-fml/src/backends/kotlin/gen_structs/bundled.rs b/components/support/nimbus-fml/src/backends/kotlin/gen_structs/bundled.rs index 2845f0c6e9..7541da15dd 100644 --- a/components/support/nimbus-fml/src/backends/kotlin/gen_structs/bundled.rs +++ b/components/support/nimbus-fml/src/backends/kotlin/gen_structs/bundled.rs @@ -6,7 +6,7 @@ use std::fmt::Display; use super::common::{code_type, quoted}; use crate::backends::{CodeOracle, CodeType, LiteralRenderer, TypeIdentifier, VariablesType}; -use crate::intermediate_representation::Literal; +use crate::intermediate_representation::{Literal, TypeRef}; use heck::SnakeCase; use unicode_segmentation::UnicodeSegmentation; @@ -86,6 +86,16 @@ impl CodeType for TextCodeType { } } + fn preference_getter( + &self, + oracle: &dyn CodeOracle, + prefs: &dyn Display, + pref_key: &dyn Display, + ) -> Option { + let ct = oracle.find(&TypeRef::String); + ct.preference_getter(oracle, prefs, pref_key) + } + fn is_resource_id(&self, literal: &Literal) -> bool { match literal { serde_json::Value::String(v) => is_resource_id(v), diff --git a/components/support/nimbus-fml/src/backends/kotlin/gen_structs/filters.rs b/components/support/nimbus-fml/src/backends/kotlin/gen_structs/filters.rs index f3d1b59547..b297339c9a 100644 --- a/components/support/nimbus-fml/src/backends/kotlin/gen_structs/filters.rs +++ b/components/support/nimbus-fml/src/backends/kotlin/gen_structs/filters.rs @@ -41,6 +41,20 @@ pub fn property( Ok(ct.property_getter(oracle, &vars, &prop, &default)) } +pub fn preference_getter( + type_: impl Borrow, + prefs: impl fmt::Display, + pref_key: impl fmt::Display, +) -> Result { + let oracle = &ConcreteCodeOracle; + let ct = oracle.find(type_.borrow()); + if let Some(getter) = ct.preference_getter(oracle, &prefs, &pref_key) { + Ok(getter) + } else { + unreachable!("The preference for type {} isn't available. This is a bug in Nimbus FML Kotlin generator", type_.borrow()); + } +} + pub fn to_json( prop: impl fmt::Display, type_: impl Borrow, diff --git a/components/support/nimbus-fml/src/backends/kotlin/gen_structs/primitives.rs b/components/support/nimbus-fml/src/backends/kotlin/gen_structs/primitives.rs index 3783b15292..bbee2bf5b0 100644 --- a/components/support/nimbus-fml/src/backends/kotlin/gen_structs/primitives.rs +++ b/components/support/nimbus-fml/src/backends/kotlin/gen_structs/primitives.rs @@ -66,6 +66,15 @@ impl CodeType for BooleanCodeType { _ => unreachable!("Expecting a boolean"), } } + + fn preference_getter( + &self, + _oracle: &dyn CodeOracle, + prefs: &dyn Display, + pref_key: &dyn Display, + ) -> Option { + Some(format!("{prefs}.getBoolean({}, false)", quoted(pref_key))) + } } pub(crate) struct IntCodeType; @@ -122,6 +131,15 @@ impl CodeType for IntCodeType { _ => unreachable!("Expecting a number"), } } + + fn preference_getter( + &self, + _oracle: &dyn CodeOracle, + prefs: &dyn Display, + pref_key: &dyn Display, + ) -> Option { + Some(format!("{prefs}.getInt({}, 0)", quoted(pref_key))) + } } pub(crate) struct StringCodeType; @@ -181,6 +199,15 @@ impl CodeType for StringCodeType { _ => unreachable!("Expecting a string"), } } + + fn preference_getter( + &self, + _oracle: &dyn CodeOracle, + prefs: &dyn Display, + pref_key: &dyn Display, + ) -> Option { + Some(format!("{prefs}.getString({}, \"\")", quoted(pref_key))) + } } #[cfg(test)] diff --git a/components/support/nimbus-fml/src/backends/kotlin/templates/macros.kt b/components/support/nimbus-fml/src/backends/kotlin/templates/macros.kt index 816a85ee3c..a85ae1f994 100644 --- a/components/support/nimbus-fml/src/backends/kotlin/templates/macros.kt +++ b/components/support/nimbus-fml/src/backends/kotlin/templates/macros.kt @@ -47,18 +47,28 @@ private constructor( {%- let defaults = format!("_defaults.{}", prop_kt) %} {%- let getter = p.typ()|property(p.name(), "_variables", defaults) %} {{ p.doc()|comment(" ") }} - {%- if p.supports_lazy() %} + {%- if !p.has_prefs() %} val {{ prop_kt }}: {{ type_kt }} by lazy { {{ getter }} } {%- else %} val {{ prop_kt }}: {{ type_kt }} - get() { - fun getter() = {{ getter }} - return {% call prefs() %}?.let { _ -> - getter() - } ?: getter() - } + get() = + {%- let prefs = "it" %} + {%- let key = p.pref_key().unwrap() %} + {% call prefs() %}?.let { + if ({{ prefs }}.contains({{ key|quoted }})) { + try { + {{ p.typ()|preference_getter(prefs, key) }} + } catch (e: ClassCastException) { + // This only gets to here if the app has written the + // wrong type to this preference. + null + } + } else { + null + } + } ?: {{ getter }} {%- endif %} {% endfor %} diff --git a/components/support/nimbus-fml/src/backends/mod.rs b/components/support/nimbus-fml/src/backends/mod.rs index bc99123141..b719aed709 100644 --- a/components/support/nimbus-fml/src/backends/mod.rs +++ b/components/support/nimbus-fml/src/backends/mod.rs @@ -145,6 +145,15 @@ pub trait CodeType { None } + fn preference_getter( + &self, + _oracle: &dyn CodeOracle, + _prefs: &dyn Display, + _pref_key: &dyn Display, + ) -> Option { + None + } + /// Call from the template fn as_json(&self, oracle: &dyn CodeOracle, prop: &dyn Display) -> String { self.as_json_transform(oracle, prop) From e2edb95692cbff933577eac895fef3a60ac89d30 Mon Sep 17 00:00:00 2001 From: James Hugman Date: Tue, 10 Oct 2023 12:51:10 +0100 Subject: [PATCH 04/10] Kotlin: Add integration tests for preference-overrides --- .../fixtures/android/runtime/Context.kt | 19 +++++-- .../fixtures/fe/pref_overrides.fml.yaml | 32 +++++++++++ .../nimbus-fml/src/command_line/workflows.rs | 9 ++++ .../nimbus-fml/test/pref_overrides.kts | 53 +++++++++++++++++++ 4 files changed, 108 insertions(+), 5 deletions(-) create mode 100644 components/support/nimbus-fml/fixtures/fe/pref_overrides.fml.yaml create mode 100644 components/support/nimbus-fml/test/pref_overrides.kts diff --git a/components/support/nimbus-fml/fixtures/android/runtime/Context.kt b/components/support/nimbus-fml/fixtures/android/runtime/Context.kt index 4c6c6fbc90..c78e847ced 100644 --- a/components/support/nimbus-fml/fixtures/android/runtime/Context.kt +++ b/components/support/nimbus-fml/fixtures/android/runtime/Context.kt @@ -36,10 +36,19 @@ object Resources { fun getResourceName(resId: Int) = "res:$resId" } -@Suppress("UNUSED_PARAMETER", "PACKAGE_OR_CLASSIFIER_REDECLARATION", "FunctionOnlyReturningConstant") +@Suppress("PACKAGE_OR_CLASSIFIER_REDECLARATION") class SharedPreferences { - fun contains(key: String): Boolean = false - fun getBoolean(key: String, def: Boolean): Boolean = def - fun getString(key: String, def: String): String = def - fun getInt(key: String, def: Int): Int = def + // Minimal interface used by generated code. + fun contains(key: String): Boolean = map.containsKey(key) + fun getBoolean(key: String, def: Boolean): Boolean = (map[key] as? Boolean) ?: def + fun getString(key: String, def: String): String = (map[key] as? String) ?: def + fun getInt(key: String, def: Int): Int = (map[key] as? Int) ?: def + + // For testing + val map = mutableMapOf() + fun put(key: String, value: Boolean) = map.put(key, value) + fun put(key: String, value: String) = map.put(key, value) + fun put(key: String, value: Int) = map.put(key, value) + fun clear() = map.clear() + fun remove(key: String) = map.remove(key) } diff --git a/components/support/nimbus-fml/fixtures/fe/pref_overrides.fml.yaml b/components/support/nimbus-fml/fixtures/fe/pref_overrides.fml.yaml new file mode 100644 index 0000000000..a21795e498 --- /dev/null +++ b/components/support/nimbus-fml/fixtures/fe/pref_overrides.fml.yaml @@ -0,0 +1,32 @@ +--- +about: + description: A test for gating via preference + kotlin: + class: .AppConfig + package: com.example.nimbus +channels: + - debug +features: + my-feature: + description: A feature with preference overrides + variables: + my-boolean: + type: Boolean + description: A boolean + pref-key: my-boolean-pref-key + default: false + my-int: + type: Int + description: An Int + pref-key: my-int-pref-key + default: 0 + my-string: + type: String + description: A String + pref-key: my-string-pref-key + default: manifest + my-text: + type: Text + description: A Text + pref-key: my-text-pref-key + default: the manifest diff --git a/components/support/nimbus-fml/src/command_line/workflows.rs b/components/support/nimbus-fml/src/command_line/workflows.rs index 936fd7e22f..6be9b59433 100644 --- a/components/support/nimbus-fml/src/command_line/workflows.rs +++ b/components/support/nimbus-fml/src/command_line/workflows.rs @@ -987,4 +987,13 @@ mod test { )?; Ok(()) } + + #[test] + fn test_with_preference_overrides() -> Result<()> { + generate_multiple_and_assert( + "test/pref_overrides.kts", + &[("fixtures/fe/pref_overrides.fml.yaml", "debug")], + )?; + Ok(()) + } } diff --git a/components/support/nimbus-fml/test/pref_overrides.kts b/components/support/nimbus-fml/test/pref_overrides.kts new file mode 100644 index 0000000000..bc4bce920a --- /dev/null +++ b/components/support/nimbus-fml/test/pref_overrides.kts @@ -0,0 +1,53 @@ +/* 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/. */ + +import android.content.Context as MockContext +import android.content.SharedPreferences as MockSharedPreferences + +import com.example.nimbus.AppConfig + +import org.mozilla.experiments.nimbus.HardcodedNimbusFeatures +import org.mozilla.experiments.nimbus.FeaturesInterface + +import org.json.JSONObject + +class PrefNimbusFeatures( + override val prefs: MockSharedPreferences, + val nimbus: HardcodedNimbusFeatures, +): FeaturesInterface { + override val context: MockContext = nimbus.context + override fun getVariables(featureId: String, recordExposureEvent: Boolean) = + nimbus.getVariables(featureId, recordExposureEvent) +} + +val context = MockContext() +val prefs = MockSharedPreferences() +val nimbusFromRust = HardcodedNimbusFeatures(context, + "my-feature" to JSONObject(mapOf( + "my-boolean" to false, + "my-int" to 100, + "my-string" to "from json", + "my-text" to "from json" + )) +) +val nimbus = PrefNimbusFeatures(prefs, nimbusFromRust) + +AppConfig.initialize { nimbus } + +val feature = AppConfig.features.myFeature.value() + +assert(feature.myBoolean == false) +assert(feature.myInt == 100) +assert(feature.myString == "from json") +assert(feature.myText == "from json") + +prefs.put("my-boolean-pref-key", true) +prefs.put("my-int-pref-key", 42) +prefs.put("my-string-pref-key", "from pref") +prefs.put("my-text-pref-key", "from pref") + +assert(feature.myBoolean == true) +assert(feature.myInt == 42) +assert(feature.myString == "from pref") +assert(feature.myText == "from pref") From 5c9ca0e85e0138453a74833d3854e37b9644b5d8 Mon Sep 17 00:00:00 2001 From: James Hugman Date: Wed, 11 Oct 2023 19:41:07 +0100 Subject: [PATCH 05/10] Kotlin: add isModified to detect preference overrides --- .../nimbus/internal/FeatureHolder.kt | 22 ++++++++++++++----- .../kotlin/templates/FeatureTemplate.kt | 15 ++++++++++++- .../src/intermediate_representation.rs | 4 ++++ .../nimbus-fml/test/pref_overrides.kts | 13 +++++++++++ 4 files changed, 47 insertions(+), 7 deletions(-) diff --git a/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/internal/FeatureHolder.kt b/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/internal/FeatureHolder.kt index cb5b7a8849..c683e1cef2 100644 --- a/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/internal/FeatureHolder.kt +++ b/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/internal/FeatureHolder.kt @@ -61,7 +61,9 @@ class FeatureHolder( * their behavior because of it. */ fun recordExposure() { - getSdk()?.recordExposureEvent(featureId) + if (!value().isModified()) { + getSdk()?.recordExposureEvent(featureId) + } } /** @@ -74,7 +76,9 @@ class FeatureHolder( * [recordExposure] instead. */ fun recordExperimentExposure(slug: String) { - getSdk()?.recordExposureEvent(featureId, slug) + if (!value().isModified()) { + getSdk()?.recordExposureEvent(featureId, slug) + } } /** @@ -172,8 +176,14 @@ interface FMLObjectInterface { * * App developers should use the generated concrete classes, which * implement this interface. - * - * This interface is really only here to allow bridging between Kotlin - * and other languages. */ -interface FMLFeatureInterface : FMLObjectInterface +interface FMLFeatureInterface : FMLObjectInterface { + /** + * A test if the feature configuration has been modified somehow, invalidating any experiment + * that uses it. + * + * This may be `true` if a `pref-key` has been set in the feature manifest and the user has + * set that preference. + */ + fun isModified(): Boolean = false +} diff --git a/components/support/nimbus-fml/src/backends/kotlin/templates/FeatureTemplate.kt b/components/support/nimbus-fml/src/backends/kotlin/templates/FeatureTemplate.kt index cf3a441a4d..4c4e5a341e 100644 --- a/components/support/nimbus-fml/src/backends/kotlin/templates/FeatureTemplate.kt +++ b/components/support/nimbus-fml/src/backends/kotlin/templates/FeatureTemplate.kt @@ -4,4 +4,17 @@ {{ inner.doc()|comment("") }} public class {{ inner.name()|class_name }} {% call kt::render_constructor() %} : FMLFeatureInterface { {% call kt::render_class_body(inner) %} -} \ No newline at end of file + + {%- if inner.has_prefs() %} + override fun isModified(): Boolean = + {% call kt::prefs() %}?.let { prefs -> + listOf( + {%- for p in inner.props() %} + {%- if p.has_prefs() %} + {{ p.pref_key().unwrap()|quoted }}, + {%- endif %} + {%- endfor %} + ).any { prefs.contains(it) } + } ?: false + {%- endif %} +} diff --git a/components/support/nimbus-fml/src/intermediate_representation.rs b/components/support/nimbus-fml/src/intermediate_representation.rs index 061a61ddaa..54119e32b5 100644 --- a/components/support/nimbus-fml/src/intermediate_representation.rs +++ b/components/support/nimbus-fml/src/intermediate_representation.rs @@ -824,6 +824,10 @@ impl FeatureDef { Value::Object(props) } + + pub fn has_prefs(&self) -> bool { + self.props.iter().any(|p| p.has_prefs()) + } } impl TypeFinder for FeatureDef { fn find_types(&self, types: &mut HashSet) { diff --git a/components/support/nimbus-fml/test/pref_overrides.kts b/components/support/nimbus-fml/test/pref_overrides.kts index bc4bce920a..f5da1f16a5 100644 --- a/components/support/nimbus-fml/test/pref_overrides.kts +++ b/components/support/nimbus-fml/test/pref_overrides.kts @@ -31,6 +31,17 @@ val nimbusFromRust = HardcodedNimbusFeatures(context, "my-text" to "from json" )) ) + +// Before initialization with hardcoded, just get values from the manifest. +val feature0 = AppConfig.features.myFeature.value() + +assert(feature0.myBoolean == false) +assert(feature0.myInt == 0) +assert(feature0.myString == "from manifest") +assert(feature0.myText == "from manifest") +assert(!feature0.isModified()) + + val nimbus = PrefNimbusFeatures(prefs, nimbusFromRust) AppConfig.initialize { nimbus } @@ -41,6 +52,7 @@ assert(feature.myBoolean == false) assert(feature.myInt == 100) assert(feature.myString == "from json") assert(feature.myText == "from json") +assert(!feature.isModified()) prefs.put("my-boolean-pref-key", true) prefs.put("my-int-pref-key", 42) @@ -51,3 +63,4 @@ assert(feature.myBoolean == true) assert(feature.myInt == 42) assert(feature.myString == "from pref") assert(feature.myText == "from pref") +assert(feature.isModified()) From badaf0a3a4f25b85497610fd7f39931633681947 Mon Sep 17 00:00:00 2001 From: James Hugman Date: Tue, 10 Oct 2023 13:47:41 +0100 Subject: [PATCH 06/10] Swift: Add UserDefaults to helper files --- components/nimbus/ios/Nimbus/FeatureHolder.swift | 10 ++++++---- .../nimbus/ios/Nimbus/FeatureInterface.swift | 10 ++++++++++ components/nimbus/ios/Nimbus/Nimbus.swift | 10 ++++++++++ components/nimbus/ios/Nimbus/NimbusBuilder.swift | 14 +++++++++++++- components/nimbus/ios/Nimbus/NimbusCreate.swift | 3 ++- .../src/backends/swift/gen_structs/mod.rs | 1 + .../swift/templates/FeatureManifestTemplate.swift | 4 ++-- .../ImportedModuleInitializationTemplate.swift | 11 +++++++---- .../backends/swift/templates/ObjectTemplate.swift | 2 +- .../src/backends/swift/templates/macros.swift | 14 +++++++++----- .../test/threadsafe_feature_holder.swift | 2 +- 11 files changed, 62 insertions(+), 19 deletions(-) diff --git a/components/nimbus/ios/Nimbus/FeatureHolder.swift b/components/nimbus/ios/Nimbus/FeatureHolder.swift index 5bd2dc60e9..e5d67b940e 100644 --- a/components/nimbus/ios/Nimbus/FeatureHolder.swift +++ b/components/nimbus/ios/Nimbus/FeatureHolder.swift @@ -19,11 +19,11 @@ public class FeatureHolder { private var getSdk: GetSdk private let featureId: String - private var create: (Variables) -> T + private var create: (Variables, UserDefaults?) -> T public init(_ getSdk: @escaping () -> FeaturesInterface?, featureId: String, - with create: @escaping (Variables) -> T) + with create: @escaping (Variables, UserDefaults?) -> T) { self.getSdk = getSdk self.featureId = featureId @@ -43,10 +43,12 @@ public class FeatureHolder { return v } var variables: Variables = NilVariables.instance + var defaults: UserDefaults? if let sdk = getSdk() { variables = sdk.getVariables(featureId: featureId, sendExposureEvent: false) + defaults = sdk.userDefaults } - let v = create(variables) + let v = create(variables, defaults) cachedValue = v return v } @@ -121,7 +123,7 @@ public class FeatureHolder { /// This changes the mapping between a `Variables` and the feature configuration object. /// /// This is most likely useful during testing and other generated code. - public func with(initializer: @escaping (Variables) -> T) { + public func with(initializer: @escaping (Variables, UserDefaults?) -> T) { lock.lock() defer { self.lock.unlock() } cachedValue = nil diff --git a/components/nimbus/ios/Nimbus/FeatureInterface.swift b/components/nimbus/ios/Nimbus/FeatureInterface.swift index 3c7eba2d92..9a4b7b4399 100644 --- a/components/nimbus/ios/Nimbus/FeatureInterface.swift +++ b/components/nimbus/ios/Nimbus/FeatureInterface.swift @@ -7,6 +7,8 @@ import Foundation /// /// This is intended to be standalone to allow for testing the Nimbus FML. public protocol FeaturesInterface: AnyObject { + var userDefaults: UserDefaults? { get } + /// Get the variables needed to configure the feature given by `featureId`. /// /// - Parameters: @@ -56,3 +58,11 @@ public protocol FeaturesInterface: AnyObject { /// is malformed, providing more detail to experiment owners of where to look for the problem. func recordMalformedConfiguration(featureId: String, with partId: String) } + +public extension FeaturesInterface { + var userDefaults: UserDefaults? { + get { + nil + } + } +} diff --git a/components/nimbus/ios/Nimbus/Nimbus.swift b/components/nimbus/ios/Nimbus/Nimbus.swift index 68e7fa98f6..5bf5bf5628 100644 --- a/components/nimbus/ios/Nimbus/Nimbus.swift +++ b/components/nimbus/ios/Nimbus/Nimbus.swift @@ -6,6 +6,8 @@ import Foundation import Glean public class Nimbus: NimbusInterface { + private let _userDefaults: UserDefaults? + private let nimbusClient: NimbusClientProtocol private let resourceBundles: [Bundle] @@ -28,11 +30,13 @@ public class Nimbus: NimbusInterface { init(nimbusClient: NimbusClientProtocol, resourceBundles: [Bundle], + userDefaults: UserDefaults?, errorReporter: @escaping NimbusErrorReporter) { self.errorReporter = errorReporter self.nimbusClient = nimbusClient self.resourceBundles = resourceBundles + self._userDefaults = userDefaults NilVariables.instance.set(bundles: resourceBundles) } } @@ -102,6 +106,12 @@ extension Nimbus: NimbusEventStore { } extension Nimbus: FeaturesInterface { + public var userDefaults: UserDefaults? { + get { + _userDefaults + } + } + public func recordExposureEvent(featureId: String, experimentSlug: String? = nil) { if let experimentSlug = experimentSlug { recordExposureFromExperiment(featureId: featureId, experimentSlug: experimentSlug) diff --git a/components/nimbus/ios/Nimbus/NimbusBuilder.swift b/components/nimbus/ios/Nimbus/NimbusBuilder.swift index 9aef418319..7495b83fc4 100644 --- a/components/nimbus/ios/Nimbus/NimbusBuilder.swift +++ b/components/nimbus/ios/Nimbus/NimbusBuilder.swift @@ -132,7 +132,7 @@ public class NimbusBuilder { var resourceBundles: [Bundle] = [.main] /** - * The object generated from the `nimbus.fml.yaml` file and the nimbus-gradle-plugin. + * The object generated from the `nimbus.fml.yaml` file. */ @discardableResult public func with(featureManifest: FeatureManifestInterface) -> NimbusBuilder { @@ -142,6 +142,17 @@ public class NimbusBuilder { var featureManifest: FeatureManifestInterface? + /** + * Main user defaults for the app. + */ + @discardableResult + public func with(userDefaults: UserDefaults) -> NimbusBuilder { + self.userDefaults = userDefaults + return self + } + + var userDefaults: UserDefaults = UserDefaults.standard + /** * The command line arguments for the app. This is useful for QA, and can be safely left in the app in production. */ @@ -240,6 +251,7 @@ public class NimbusBuilder { coenrollingFeatureIds: getCoenrollingFeatureIds(), dbPath: dbFilePath, resourceBundles: resourceBundles, + userDefaults: userDefaults, errorReporter: errorReporter) } diff --git a/components/nimbus/ios/Nimbus/NimbusCreate.swift b/components/nimbus/ios/Nimbus/NimbusCreate.swift index dcf4d4f61b..469a6693eb 100644 --- a/components/nimbus/ios/Nimbus/NimbusCreate.swift +++ b/components/nimbus/ios/Nimbus/NimbusCreate.swift @@ -38,6 +38,7 @@ public extension Nimbus { dbPath: String, resourceBundles: [Bundle] = [Bundle.main], enabled: Bool = true, + userDefaults: UserDefaults? = nil, errorReporter: @escaping NimbusErrorReporter = defaultErrorReporter ) throws -> NimbusInterface { guard enabled else { @@ -66,7 +67,7 @@ public extension Nimbus { ) ) - return Nimbus(nimbusClient: nimbusClient, resourceBundles: resourceBundles, errorReporter: errorReporter) + return Nimbus(nimbusClient: nimbusClient, resourceBundles: resourceBundles, userDefaults: userDefaults, errorReporter: errorReporter) } static func buildExperimentContext( diff --git a/components/support/nimbus-fml/src/backends/swift/gen_structs/mod.rs b/components/support/nimbus-fml/src/backends/swift/gen_structs/mod.rs index 9a62b6ab34..41c06404bf 100644 --- a/components/support/nimbus-fml/src/backends/swift/gen_structs/mod.rs +++ b/components/support/nimbus-fml/src/backends/swift/gen_structs/mod.rs @@ -100,6 +100,7 @@ impl<'a> FeatureManifestDeclaration<'a> { .flatten(), ) .chain(as_module) + .chain(vec!["Foundation".to_string()]) .filter(|i| i != my_module) .collect::>() .into_iter() diff --git a/components/support/nimbus-fml/src/backends/swift/templates/FeatureManifestTemplate.swift b/components/support/nimbus-fml/src/backends/swift/templates/FeatureManifestTemplate.swift index 770182d3d8..49be0fd786 100644 --- a/components/support/nimbus-fml/src/backends/swift/templates/FeatureManifestTemplate.swift +++ b/components/support/nimbus-fml/src/backends/swift/templates/FeatureManifestTemplate.swift @@ -100,8 +100,8 @@ public class {{ features_object }} { {%- let class_name = raw_name|class_name %} {{ f.doc()|comment(" ") }} public lazy var {{raw_name|var_name}}: FeatureHolder<{{class_name}}> = { - FeatureHolder({{ nimbus_object }}.shared.getSdk, featureId: {{ raw_name|quoted }}) { (variables) in - {{ class_name }}(variables) + FeatureHolder({{ nimbus_object }}.shared.getSdk, featureId: {{ raw_name|quoted }}) { variables, prefs in + {{ class_name }}(variables, prefs) } }() {%- endfor %} diff --git a/components/support/nimbus-fml/src/backends/swift/templates/ImportedModuleInitializationTemplate.swift b/components/support/nimbus-fml/src/backends/swift/templates/ImportedModuleInitializationTemplate.swift index 23d74c5f58..44c083d8fc 100644 --- a/components/support/nimbus-fml/src/backends/swift/templates/ImportedModuleInitializationTemplate.swift +++ b/components/support/nimbus-fml/src/backends/swift/templates/ImportedModuleInitializationTemplate.swift @@ -1,13 +1,16 @@ -{% import "macros.swift" as swift %} +{%- import "macros.swift" as swift %} {%- let variables = "variables" %} +{%- let prefs = "prefs" %} {%- let context = "" %} {%- let class_name = self.inner.about().nimbus_object_name_swift() %} {%- for f in self.inner.features() %} - {{ class_name }}.shared.features.{{ f.name()|var_name }}.with(initializer: { {{ variables }} in + {{ class_name }}.shared.features.{{ f.name()|var_name }}.with(initializer: { {{ variables }}, {{ prefs }} in {{ f.name()|class_name }}( - {{ variables }}, {%- for p in f.props() %} + {{ variables }}, + {{ prefs }}, + {%- for p in f.props() %} {{p.name()|var_name}}: {{ p.typ()|literal(self, p.default(), context) }}{% if !loop.last %},{% endif %} {%- endfor %} ) }) - {%- endfor %} \ No newline at end of file + {%- endfor %} diff --git a/components/support/nimbus-fml/src/backends/swift/templates/ObjectTemplate.swift b/components/support/nimbus-fml/src/backends/swift/templates/ObjectTemplate.swift index b68d7e4dd7..dfb6788393 100644 --- a/components/support/nimbus-fml/src/backends/swift/templates/ObjectTemplate.swift +++ b/components/support/nimbus-fml/src/backends/swift/templates/ObjectTemplate.swift @@ -8,7 +8,7 @@ public extension {{class_name}} { guard let defaults = defaults else { return self } - return {{class_name}}(variables: self._variables, defaults: defaults._defaults) + return {{class_name}}(variables: self._variables, prefs: self._prefs, defaults: defaults._defaults) } static func create(_ variables: Variables?) -> {{class_name}} { diff --git a/components/support/nimbus-fml/src/backends/swift/templates/macros.swift b/components/support/nimbus-fml/src/backends/swift/templates/macros.swift index 7ad09258b8..80a06da330 100644 --- a/components/support/nimbus-fml/src/backends/swift/templates/macros.swift +++ b/components/support/nimbus-fml/src/backends/swift/templates/macros.swift @@ -8,7 +8,7 @@ and rendering literals for Objects. -#} -{% macro render_class(inner) %} +{%- macro render_class(inner) %} {%- let raw_name = inner.name() %} {% let class_name = inner.name()|class_name -%} @@ -16,9 +16,11 @@ public class {{class_name}} { private let _variables: Variables private let _defaults: Defaults - private init(variables: Variables = NilVariables.instance, defaults: Defaults) { + private let _prefs: UserDefaults? + private init(variables: Variables = NilVariables.instance, prefs: UserDefaults? = nil, defaults: Defaults) { self._variables = variables self._defaults = defaults + self._prefs = prefs } {# The struct holds the default values that come from the manifest. They should completely specify all values needed for the feature #} @@ -32,12 +34,14 @@ public class {{class_name}} { {#- A constructor for application tests to use. #} public convenience init( - _ _variables: Variables = NilVariables.instance, {% for p in inner.props() %} + _ _variables: Variables = NilVariables.instance, + _ _prefs: UserDefaults? = nil, + {%- for p in inner.props() %} {%- let t = p.typ() %} {{p.name()|var_name}}: {{ t|defaults_type_label }} = {{ t|literal(self, p.default(), "") }}{% if !loop.last %},{% endif %} {%- endfor %} ) { - self.init(variables: _variables, defaults: Defaults({% for p in inner.props() %} + self.init(variables: _variables, prefs: _prefs, defaults: Defaults({% for p in inner.props() %} {{p.name()|var_name}}: {{p.name()|var_name}}{% if !loop.last %},{% endif %} {%- endfor %})) } @@ -54,4 +58,4 @@ public class {{class_name}} { {%- endfor %} } -{% endmacro %}} \ No newline at end of file +{% endmacro %}} diff --git a/components/support/nimbus-fml/test/threadsafe_feature_holder.swift b/components/support/nimbus-fml/test/threadsafe_feature_holder.swift index 0a2ac736cf..d1e391c404 100644 --- a/components/support/nimbus-fml/test/threadsafe_feature_holder.swift +++ b/components/support/nimbus-fml/test/threadsafe_feature_holder.swift @@ -12,7 +12,7 @@ let queue: OperationQueue = { }() let api: FeaturesInterface = HardcodedNimbusFeatures(with: ["test-feature-holder": "{}"]) -let holder = FeatureHolder({ api }, featureId: "test-feature-holder") { _ in "NO CRASH" } +let holder = FeatureHolder({ api }, featureId: "test-feature-holder") { _, _ in "NO CRASH" } for _ in 1 ..< 10000 { queue.addOperation { From cdbb5a6f0dbe6e08c390729cc7403e1719887c3d Mon Sep 17 00:00:00 2001 From: James Hugman Date: Tue, 10 Oct 2023 15:40:07 +0100 Subject: [PATCH 07/10] Swift: Generating preference getting in templated code --- .../nimbus/ios/Nimbus/FeatureInterface.swift | 4 +-- components/nimbus/ios/Nimbus/Nimbus.swift | 6 ++-- .../nimbus/ios/Nimbus/NimbusBuilder.swift | 2 +- .../nimbus/ios/Nimbus/NimbusCreate.swift | 7 ++++- .../src/backends/swift/templates/macros.swift | 31 ++++++++++++++++--- 5 files changed, 37 insertions(+), 13 deletions(-) diff --git a/components/nimbus/ios/Nimbus/FeatureInterface.swift b/components/nimbus/ios/Nimbus/FeatureInterface.swift index 9a4b7b4399..178dab0e48 100644 --- a/components/nimbus/ios/Nimbus/FeatureInterface.swift +++ b/components/nimbus/ios/Nimbus/FeatureInterface.swift @@ -61,8 +61,6 @@ public protocol FeaturesInterface: AnyObject { public extension FeaturesInterface { var userDefaults: UserDefaults? { - get { - nil - } + nil } } diff --git a/components/nimbus/ios/Nimbus/Nimbus.swift b/components/nimbus/ios/Nimbus/Nimbus.swift index 5bf5bf5628..0b5b3bea6f 100644 --- a/components/nimbus/ios/Nimbus/Nimbus.swift +++ b/components/nimbus/ios/Nimbus/Nimbus.swift @@ -36,7 +36,7 @@ public class Nimbus: NimbusInterface { self.errorReporter = errorReporter self.nimbusClient = nimbusClient self.resourceBundles = resourceBundles - self._userDefaults = userDefaults + _userDefaults = userDefaults NilVariables.instance.set(bundles: resourceBundles) } } @@ -107,9 +107,7 @@ extension Nimbus: NimbusEventStore { extension Nimbus: FeaturesInterface { public var userDefaults: UserDefaults? { - get { - _userDefaults - } + _userDefaults } public func recordExposureEvent(featureId: String, experimentSlug: String? = nil) { diff --git a/components/nimbus/ios/Nimbus/NimbusBuilder.swift b/components/nimbus/ios/Nimbus/NimbusBuilder.swift index 7495b83fc4..c03e77adf3 100644 --- a/components/nimbus/ios/Nimbus/NimbusBuilder.swift +++ b/components/nimbus/ios/Nimbus/NimbusBuilder.swift @@ -151,7 +151,7 @@ public class NimbusBuilder { return self } - var userDefaults: UserDefaults = UserDefaults.standard + var userDefaults = UserDefaults.standard /** * The command line arguments for the app. This is useful for QA, and can be safely left in the app in production. diff --git a/components/nimbus/ios/Nimbus/NimbusCreate.swift b/components/nimbus/ios/Nimbus/NimbusCreate.swift index 469a6693eb..56dccdeff4 100644 --- a/components/nimbus/ios/Nimbus/NimbusCreate.swift +++ b/components/nimbus/ios/Nimbus/NimbusCreate.swift @@ -67,7 +67,12 @@ public extension Nimbus { ) ) - return Nimbus(nimbusClient: nimbusClient, resourceBundles: resourceBundles, userDefaults: userDefaults, errorReporter: errorReporter) + return Nimbus( + nimbusClient: nimbusClient, + resourceBundles: resourceBundles, + userDefaults: userDefaults, + errorReporter: errorReporter + ) } static func buildExperimentContext( diff --git a/components/support/nimbus-fml/src/backends/swift/templates/macros.swift b/components/support/nimbus-fml/src/backends/swift/templates/macros.swift index 80a06da330..92860229b7 100644 --- a/components/support/nimbus-fml/src/backends/swift/templates/macros.swift +++ b/components/support/nimbus-fml/src/backends/swift/templates/macros.swift @@ -50,12 +50,35 @@ public class {{class_name}} { {# -#} {% for p in inner.props() %} {%- let prop_swift = p.name()|var_name %} + {%- let type_swift = p.typ()|type_label %} + {%- let defaults = format!("_defaults.{}", prop_swift) %} + {%- let getter = p.typ()|property(p.name(), "self._variables", defaults) %} {{ p.doc()|comment(" ") }} - public lazy var {{ prop_swift }}: {{ p.typ()|type_label }} = { - {%- let defaults = format!("_defaults.{}", prop_swift) %} - {{ p.typ()|property(p.name(), "self._variables", defaults)}} + {%- if !p.has_prefs() %} + public lazy var {{ prop_swift }}: {{ type_swift }} = { + {{ getter }} }() - {%- endfor %} + {%- else %} + public var {{ prop_swift }}: {{ type_swift }} { + {%- let prefs = "prefs" %} + {%- let key = p.pref_key().unwrap() %} + {#- Using `object(forKey:)` here as it returns an optional that just needs to be cast. + `integer(forKey:)` returns zero, `bool(forKey:)` returns `false` if the key is not + present. + + Now we only need to use the type label as part of the cast, we don't need to implement + separate type specific preference getters. + + `has_prefs()` checks if the type can be got from UserDefaults. + #} + if let {{ prefs }} = self._prefs, + let {{ prop_swift }} = {{ prefs }}.object(forKey: {{ key|quoted }}) as? {{ type_swift }} { + return {{ prop_swift }} + } + return {{ getter }} + } + {%- endif %} + {% endfor %} } {% endmacro %}} From dab7894feda9d9986f9a29cb84c6391739cf6b55 Mon Sep 17 00:00:00 2001 From: James Hugman Date: Tue, 10 Oct 2023 19:42:15 +0100 Subject: [PATCH 08/10] Swift: Kotlin: Add integration tests for preference-overrides --- .../fixtures/fe/pref_overrides.fml.yaml | 7 +- .../nimbus-fml/src/command_line/workflows.rs | 11 ++- .../nimbus-fml/test/pref_overrides.swift | 80 +++++++++++++++++++ 3 files changed, 95 insertions(+), 3 deletions(-) create mode 100644 components/support/nimbus-fml/test/pref_overrides.swift diff --git a/components/support/nimbus-fml/fixtures/fe/pref_overrides.fml.yaml b/components/support/nimbus-fml/fixtures/fe/pref_overrides.fml.yaml index a21795e498..aabce79601 100644 --- a/components/support/nimbus-fml/fixtures/fe/pref_overrides.fml.yaml +++ b/components/support/nimbus-fml/fixtures/fe/pref_overrides.fml.yaml @@ -4,6 +4,9 @@ about: kotlin: class: .AppConfig package: com.example.nimbus + swift: + class: AppConfig + module: App channels: - debug features: @@ -24,9 +27,9 @@ features: type: String description: A String pref-key: my-string-pref-key - default: manifest + default: from manifest my-text: type: Text description: A Text pref-key: my-text-pref-key - default: the manifest + default: from manifest diff --git a/components/support/nimbus-fml/src/command_line/workflows.rs b/components/support/nimbus-fml/src/command_line/workflows.rs index 6be9b59433..aaebe8a315 100644 --- a/components/support/nimbus-fml/src/command_line/workflows.rs +++ b/components/support/nimbus-fml/src/command_line/workflows.rs @@ -989,11 +989,20 @@ mod test { } #[test] - fn test_with_preference_overrides() -> Result<()> { + fn test_with_preference_overrides_kt() -> Result<()> { generate_multiple_and_assert( "test/pref_overrides.kts", &[("fixtures/fe/pref_overrides.fml.yaml", "debug")], )?; Ok(()) } + + #[test] + fn test_with_preference_overrides_swift() -> Result<()> { + generate_multiple_and_assert( + "test/pref_overrides.swift", + &[("fixtures/fe/pref_overrides.fml.yaml", "debug")], + )?; + Ok(()) + } } diff --git a/components/support/nimbus-fml/test/pref_overrides.swift b/components/support/nimbus-fml/test/pref_overrides.swift new file mode 100644 index 0000000000..5e258a2efb --- /dev/null +++ b/components/support/nimbus-fml/test/pref_overrides.swift @@ -0,0 +1,80 @@ +/* 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/. */ +import FeatureManifest +import Foundation + +class PrefNimbusFeatures { + private let _userDefaults: UserDefaults + private let nimbus: HardcodedNimbusFeatures + + init(_ prefs: UserDefaults, _ nimbus: HardcodedNimbusFeatures) { + self._userDefaults = prefs + self.nimbus = nimbus + } +} + +extension PrefNimbusFeatures: FeaturesInterface { + public var userDefaults: UserDefaults? { + get { + _userDefaults + } + } + + public func getVariables(featureId: String, sendExposureEvent: Bool) -> Variables { + return nimbus.getVariables(featureId: featureId, sendExposureEvent: sendExposureEvent) + } + + public func recordExposureEvent(featureId: String, experimentSlug: String?) { + nimbus.recordExposureEvent(featureId: featureId, experimentSlug: experimentSlug) + } + + public func recordMalformedConfiguration(featureId: String, with partId: String) { + nimbus.recordMalformedConfiguration(featureId: featureId, with: partId) + } +} + +// Test the defaults still work. +let feature0 = AppConfig.shared.features.myFeature.value() + +assert(feature0.myBoolean == false) +assert(feature0.myInt == 0) +assert(feature0.myString == "from manifest") +assert(feature0.myText == "from manifest") + +// Now test that JSON still has an effect. +let prefs = UserDefaults() +prefs.removeObject(forKey: "my-boolean-pref-key") +prefs.removeObject(forKey: "my-int-pref-key") +prefs.removeObject(forKey: "my-string-pref-key") +prefs.removeObject(forKey: "my-text-pref-key") + +let nimbusFromRust = HardcodedNimbusFeatures(with: + ["my-feature": [ + "my-boolean": false, + "my-int": 100, + "my-string": "from json", + "my-text": "from json" + ]] +) +let nimbus = PrefNimbusFeatures(prefs, nimbusFromRust) +AppConfig.shared.initialize { nimbus } + +let feature = AppConfig.shared.features.myFeature.value() + +assert(feature.myBoolean == false) +assert(feature.myInt == 100) +assert(feature.myString == "from json") +assert(feature.myText == "from json") + +// Now set with prefs. + +prefs.set(true, forKey: "my-boolean-pref-key") +prefs.set(42, forKey: "my-int-pref-key") +prefs.set("from pref", forKey: "my-string-pref-key") +prefs.set("from pref", forKey: "my-text-pref-key") + +assert(feature.myBoolean == true) +assert(feature.myInt == 42) +assert(feature.myString == "from pref") +assert(feature.myText == "from pref") From 67267c0d301d0fb75f8c450b4e9eb5723b92f2cc Mon Sep 17 00:00:00 2001 From: James Hugman Date: Wed, 11 Oct 2023 20:49:11 +0100 Subject: [PATCH 09/10] Swift: add isModified to detect preference overrides --- .../nimbus/ios/Nimbus/FeatureHolder.swift | 33 +++++++++++++++++-- .../swift/templates/FeatureTemplate.swift | 27 ++++++++++++++- .../swift/templates/ObjectTemplate.swift | 2 +- .../src/backends/swift/templates/macros.swift | 8 +++-- .../nimbus-fml/test/pref_overrides.swift | 3 ++ .../test/threadsafe_feature_holder.swift | 9 ++++- 6 files changed, 73 insertions(+), 9 deletions(-) diff --git a/components/nimbus/ios/Nimbus/FeatureHolder.swift b/components/nimbus/ios/Nimbus/FeatureHolder.swift index e5d67b940e..0c67c69568 100644 --- a/components/nimbus/ios/Nimbus/FeatureHolder.swift +++ b/components/nimbus/ios/Nimbus/FeatureHolder.swift @@ -12,7 +12,7 @@ public typealias GetSdk = () -> FeaturesInterface? /// /// There are methods useful for testing, and more advanced uses: these all start with `with`. /// -public class FeatureHolder { +public class FeatureHolder { private let lock = NSLock() private var cachedValue: T? @@ -56,7 +56,9 @@ public class FeatureHolder { /// Send an exposure event for this feature. This should be done when the user is shown the feature, and may change /// their behavior because of it. public func recordExposure() { - getSdk()?.recordExposureEvent(featureId: featureId, experimentSlug: nil) + if !value().isModified() { + getSdk()?.recordExposureEvent(featureId: featureId, experimentSlug: nil) + } } /// Send an exposure event for this feature, in the given experiment. @@ -69,7 +71,9 @@ public class FeatureHolder { /// /// - Parameter slug the experiment identifier, likely derived from the {value}. public func recordExperimentExposure(slug: String) { - getSdk()?.recordExposureEvent(featureId: featureId, experimentSlug: slug) + if !value().isModified() { + getSdk()?.recordExposureEvent(featureId: featureId, experimentSlug: slug) + } } /// Send a malformed feature event for this feature. @@ -130,3 +134,26 @@ public class FeatureHolder { create = initializer } } + +/// A bare-bones interface for the FML generated objects. +public protocol FMLObjectInterface {} + +/// A bare-bones interface for the FML generated features. +/// +/// App developers should use the generated concrete classes, which +/// implement this interface. +/// +public protocol FMLFeatureInterface: FMLObjectInterface { + /// A test if the feature configuration has been modified somehow, invalidating any experiment + /// that uses it. + /// + /// This may be `true` if a `pref-key` has been set in the feature manifest and the user has + /// set that preference. + func isModified() -> Bool +} + +public extension FMLFeatureInterface { + func isModified() -> Bool { + return false + } +} diff --git a/components/support/nimbus-fml/src/backends/swift/templates/FeatureTemplate.swift b/components/support/nimbus-fml/src/backends/swift/templates/FeatureTemplate.swift index 63afe0aa03..42873bcefc 100644 --- a/components/support/nimbus-fml/src/backends/swift/templates/FeatureTemplate.swift +++ b/components/support/nimbus-fml/src/backends/swift/templates/FeatureTemplate.swift @@ -1,3 +1,28 @@ {%- import "macros.swift" as swift %} {%- let inner = self.inner() %} -{% call swift::render_class(inner) %} \ No newline at end of file +{%- let class_name = inner.name()|class_name -%} +{% call swift::render_class(inner) %} + +{%- if inner.has_prefs() %} + +extension {{ class_name }}: FMLFeatureInterface { + public func isModified() -> Bool { + guard let prefs = {% call swift::prefs() %} else { + return false + } + let keys = [ + {%- for p in inner.props() %} + {%- if p.has_prefs() %} + {{ p.pref_key().unwrap()|quoted }}, + {%- endif %} + {%- endfor %} + ] + if let _ = keys.first(where: { prefs.object(forKey: $0) != nil }) { + return true + } + return false + } +} +{%- else %} +extension {{ class_name }}: FMLFeatureInterface {} +{%- endif %} diff --git a/components/support/nimbus-fml/src/backends/swift/templates/ObjectTemplate.swift b/components/support/nimbus-fml/src/backends/swift/templates/ObjectTemplate.swift index dfb6788393..ced91997e2 100644 --- a/components/support/nimbus-fml/src/backends/swift/templates/ObjectTemplate.swift +++ b/components/support/nimbus-fml/src/backends/swift/templates/ObjectTemplate.swift @@ -1,7 +1,7 @@ {%- import "macros.swift" as swift %} {%- let inner = self.inner() %} {% call swift::render_class(inner) -%} -{% let class_name = inner.name()|class_name -%} +{%- let class_name = inner.name()|class_name %} public extension {{class_name}} { func _mergeWith(_ defaults: {{class_name}}?) -> {{class_name}} { diff --git a/components/support/nimbus-fml/src/backends/swift/templates/macros.swift b/components/support/nimbus-fml/src/backends/swift/templates/macros.swift index 92860229b7..4c87c7c909 100644 --- a/components/support/nimbus-fml/src/backends/swift/templates/macros.swift +++ b/components/support/nimbus-fml/src/backends/swift/templates/macros.swift @@ -13,7 +13,7 @@ {% let class_name = inner.name()|class_name -%} {{ inner.doc()|comment("") }} -public class {{class_name}} { +public class {{class_name}}: FMLObjectInterface { private let _variables: Variables private let _defaults: Defaults private let _prefs: UserDefaults? @@ -71,7 +71,7 @@ public class {{class_name}} { `has_prefs()` checks if the type can be got from UserDefaults. #} - if let {{ prefs }} = self._prefs, + if let {{ prefs }} = {% call prefs() %}, let {{ prop_swift }} = {{ prefs }}.object(forKey: {{ key|quoted }}) as? {{ type_swift }} { return {{ prop_swift }} } @@ -81,4 +81,6 @@ public class {{class_name}} { {% endfor %} } -{% endmacro %}} +{%- endmacro %}} + +{% macro prefs() %}self._prefs{% endmacro %} diff --git a/components/support/nimbus-fml/test/pref_overrides.swift b/components/support/nimbus-fml/test/pref_overrides.swift index 5e258a2efb..73428b7bb3 100644 --- a/components/support/nimbus-fml/test/pref_overrides.swift +++ b/components/support/nimbus-fml/test/pref_overrides.swift @@ -41,6 +41,7 @@ assert(feature0.myBoolean == false) assert(feature0.myInt == 0) assert(feature0.myString == "from manifest") assert(feature0.myText == "from manifest") +assert(!feature0.isModified()) // Now test that JSON still has an effect. let prefs = UserDefaults() @@ -66,6 +67,7 @@ assert(feature.myBoolean == false) assert(feature.myInt == 100) assert(feature.myString == "from json") assert(feature.myText == "from json") +assert(!feature.isModified()) // Now set with prefs. @@ -78,3 +80,4 @@ assert(feature.myBoolean == true) assert(feature.myInt == 42) assert(feature.myString == "from pref") assert(feature.myText == "from pref") +assert(feature.isModified()) diff --git a/components/support/nimbus-fml/test/threadsafe_feature_holder.swift b/components/support/nimbus-fml/test/threadsafe_feature_holder.swift index d1e391c404..3dc954ad6a 100644 --- a/components/support/nimbus-fml/test/threadsafe_feature_holder.swift +++ b/components/support/nimbus-fml/test/threadsafe_feature_holder.swift @@ -5,6 +5,13 @@ import FeatureManifest import Foundation +class Feature: FMLFeatureInterface { + let string: String + init(_ string: String) { + self.string = string + } +} + let queue: OperationQueue = { let queue = OperationQueue() queue.maxConcurrentOperationCount = 5 @@ -12,7 +19,7 @@ let queue: OperationQueue = { }() let api: FeaturesInterface = HardcodedNimbusFeatures(with: ["test-feature-holder": "{}"]) -let holder = FeatureHolder({ api }, featureId: "test-feature-holder") { _, _ in "NO CRASH" } +let holder = FeatureHolder({ api }, featureId: "test-feature-holder") { _, _ in Feature("NO CRASH") } for _ in 1 ..< 10000 { queue.addOperation { From 426a907f79a9494d438c27286e46ba336b545edb Mon Sep 17 00:00:00 2001 From: James Hugman Date: Thu, 12 Oct 2023 13:17:20 +0100 Subject: [PATCH 10/10] Add changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b9186ea4aa..17e0b08b11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,9 @@ - This adds methods to get the feature_ids and descriptions from a loaded manifest. - Added a `channels` subcommmand to the command line ([#5844](https://github.com/mozilla/application-services/pull/5844)). - This prints the channels for the given manifest. +- Added `pref-key` to feature variables schema definition ([#5862](https://github.com/mozilla/application-services/pull/5862)). + - This allows developers to override remote and default values of top-level variables with preferences. + - Requires setting `userDefaults` and `sharedPreferences` in the call to `NimbusBuilder`. ### 🦊 What's Changed 🦊