From 125c50d69012b1740cf99750434dbd862778d2b9 Mon Sep 17 00:00:00 2001 From: Beth Rennie Date: Thu, 15 Oct 2026 17:16:05 -0400 Subject: [PATCH] Bug 2053517 - Revert 4a8546f9b047 and 36f1b0ea4bcd for build breakage This reverts commit 4a8546f9b0477c34696429db16f8b441a4cd8d5c. This reverts commit 36f1b0ea4bcd28df6e43fea0054986f283faa518. --- CHANGELOG.md | 4 - .../experiments/nimbus/FeaturesInterface.kt | 4 + .../org/mozilla/experiments/nimbus/Nimbus.kt | 2 + .../experiments/nimbus/NimbusBuilder.kt | 7 ++ .../nimbus/internal/FeatureHolder.kt | 30 +++++-- .../nimbus/util/TestNimbusBuilder.kt | 1 + .../fixtures/android/runtime/Context.kt | 17 ++++ .../nimbus-fml/fixtures/fe/misc-features.yaml | 8 ++ .../fixtures/fe/pref_overrides.fml.yaml | 35 ++++++++ .../src/backends/frontend_manifest.rs | 1 + .../backends/kotlin/gen_structs/bundled.rs | 12 ++- .../backends/kotlin/gen_structs/filters.rs | 21 +++++ .../src/backends/kotlin/gen_structs/mod.rs | 1 + .../backends/kotlin/gen_structs/primitives.rs | 27 ++++++ .../templates/FeatureManifestTemplate.kt | 4 +- .../kotlin/templates/FeatureTemplate.kt | 16 ++++ .../ImportedModuleInitializationTemplate.kt | 5 +- .../src/backends/kotlin/templates/macros.kt | 26 +++++- .../support/nimbus-fml/src/backends/mod.rs | 9 ++ .../swift/templates/FeatureTemplate.swift | 22 +++++ .../src/backends/swift/templates/macros.swift | 30 ++++++- .../support/nimbus-fml/src/fixtures/mod.rs | 12 ++- components/support/nimbus-fml/src/frontend.rs | 6 ++ .../src/intermediate_representation.rs | 25 ++++++ .../nimbus-fml/test/pref_overrides.kts | 66 +++++++++++++++ .../nimbus-fml/test/pref_overrides.swift | 83 +++++++++++++++++++ detekt-baseline.xml | 10 +++ 27 files changed, 460 insertions(+), 24 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 create mode 100644 components/support/nimbus-fml/test/pref_overrides.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 6123c01e51a..024d95c3c95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,10 +29,6 @@ ### Ads-Client - Telemetry is now a process-wide singleton internally; the binding API is unchanged, but the `MozAdsTelemetry` callback now receives metrics from every client in the process. -### Nimbus - -- The `pref-key` annotation for FML feature variables has been removed. ([#7559](https://github.com/mozilla/application-services/pull/7559/)) - ### Sync15 - Better interoperability with clients which choose engines per device: sync IDs are adopted from `meta/syncids` rather than minted, and our declined engines are kept when `meta/global` has no engines or declined engines. 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 49441641652..d129ea9acbe 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 import org.mozilla.experiments.nimbus.internal.FeatureUpdateDispatcher /** @@ -16,6 +17,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 6320fd092fc..5a7aaab557d 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 @@ -66,6 +67,7 @@ const val NIMBUS_DATA_DIR: String = "nimbus_data" @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 e1930a97c67..5e9f31cb4c9 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 @@ -82,6 +83,11 @@ abstract class AbstractNimbusBuilder(val context: Context) */ var featureManifest: FeatureManifestInterface<*>? = null + /** + * The shared preferences used to configure the app. + */ + var sharedPreferences: SharedPreferences? = null + /** * Additional targeting context that will be recorded to Glean. */ @@ -207,6 +213,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() @@ -50,7 +51,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 } } @@ -61,7 +63,9 @@ class FeatureHolder( * their behavior because of it. */ fun recordExposure() { - getSdk()?.recordExposureEvent(featureId) + if (!value().isModified()) { + getSdk()?.recordExposureEvent(featureId) + } } /** @@ -74,7 +78,9 @@ class FeatureHolder( * [recordExposure] instead. */ fun recordExperimentExposure(slug: String) { - getSdk()?.recordExposureEvent(featureId, slug) + if (!value().isModified()) { + getSdk()?.recordExposureEvent(featureId, slug) + } } /** @@ -110,7 +116,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.withLock { this.create = create this.cachedValue = null @@ -176,10 +182,18 @@ interface FMLObjectInterface { } /** - * A marker interface for FML generated feature classes. + * A bare-bones interface for the FML generated features. * * App developers should use the generated concrete classes, which * implement this interface. */ -@Suppress("EmptyClassBlock") -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/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 012b5b5dba3..03da9bdf347 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 @@ -21,6 +21,7 @@ class TestNimbusBuilder(context: Context) : AbstractNimbusBuilder() + 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/misc-features.yaml b/components/support/nimbus-fml/fixtures/fe/misc-features.yaml index bac43a53d8a..94d6bf41e41 100644 --- a/components/support/nimbus-fml/fixtures/fe/misc-features.yaml +++ b/components/support/nimbus-fml/fixtures/fe/misc-features.yaml @@ -10,6 +10,14 @@ about: 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 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 00000000000..aabce79601f --- /dev/null +++ b/components/support/nimbus-fml/fixtures/fe/pref_overrides.fml.yaml @@ -0,0 +1,35 @@ +--- +about: + description: A test for gating via preference + kotlin: + class: .AppConfig + package: com.example.nimbus + swift: + class: AppConfig + module: App +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: from manifest + my-text: + type: Text + description: A Text + pref-key: my-text-pref-key + default: from manifest diff --git a/components/support/nimbus-fml/src/backends/frontend_manifest.rs b/components/support/nimbus-fml/src/backends/frontend_manifest.rs index 2b6ead1e93d..7821ba23c03 100644 --- a/components/support/nimbus-fml/src/backends/frontend_manifest.rs +++ b/components/support/nimbus-fml/src/backends/frontend_manifest.rs @@ -143,6 +143,7 @@ impl From for FieldBody { impl From for FeatureFieldBody { fn from(value: PropDef) -> Self { Self { + pref_key: value.pref_key.clone(), gecko_pref: value.gecko_pref.clone(), string_alias: value.string_alias.as_ref().map(TypeRef::to_string), field: value.into(), 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 3fb8eb69f1b..df606392127 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::ToSnakeCase; use icu_segmenter::GraphemeClusterSegmenter; @@ -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 7d3587224dc..890c6cdf456 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 @@ -65,6 +65,27 @@ where Ok(ct.property_getter(oracle, &vars, &prop, &default)) } +#[askama::filter_fn] +pub fn preference_getter( + type_: T, + _: &dyn askama::Values, + prefs: P, + pref_key: K, +) -> Result +where + T: Borrow, + P: fmt::Display, + K: fmt::Display, +{ + 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()); + } +} + #[askama::filter_fn] pub fn to_json(prop: P, _: &dyn askama::Values, type_: T) -> Result where diff --git a/components/support/nimbus-fml/src/backends/kotlin/gen_structs/mod.rs b/components/support/nimbus-fml/src/backends/kotlin/gen_structs/mod.rs index 9219b49afd9..e82944ba055 100644 --- a/components/support/nimbus-fml/src/backends/kotlin/gen_structs/mod.rs +++ b/components/support/nimbus-fml/src/backends/kotlin/gen_structs/mod.rs @@ -130,6 +130,7 @@ impl<'a> FeatureManifestDeclaration<'a> { "org.mozilla.experiments.nimbus.internal.PrefBranch".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/gen_structs/primitives.rs b/components/support/nimbus-fml/src/backends/kotlin/gen_structs/primitives.rs index 942dc685489..d5811700a94 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/FeatureManifestTemplate.kt b/components/support/nimbus-fml/src/backends/kotlin/templates/FeatureManifestTemplate.kt index 197351e6f82..95172b93fb6 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/FeatureTemplate.kt b/components/support/nimbus-fml/src/backends/kotlin/templates/FeatureTemplate.kt index 55a0a5f800e..f53b005b520 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,20 @@ {{ inner.doc()|comment("") }} public class {{ inner.name()|class_name }} {{ kt::render_constructor() }} : FMLFeatureInterface { {{ kt::render_class_body(inner) }} + + {%- if inner.has_prefs() %} + override fun isModified(): Boolean = + {{ kt::prefs() }}?.let { prefs -> + listOf( + {%- for p in inner.props() %} + {%- if p.has_prefs() %} + {{ p.pref_key().unwrap()|quoted }}, + {%- endif %} + {%- if p.has_gecko_prefs() %} + {{ p.gecko_pref().unwrap().pref()|quoted }}, + {%- endif %} + {%- endfor %} + ).any { prefs.contains(it) } + } ?: false + {%- endif %} } 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 f8135872c8b..9d4717e30dd 100644 --- a/components/support/nimbus-fml/src/backends/kotlin/templates/ImportedModuleInitializationTemplate.kt +++ b/components/support/nimbus-fml/src/backends/kotlin/templates/ImportedModuleInitializationTemplate.kt @@ -1,13 +1,14 @@ {%- 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 }}: Variables -> + {{ f.name()|var_name }}.withInitializer { {{ variables }}: Variables, {{ prefs }}: SharedPreferences? -> {{ f.name()|class_name }}( _variables = {{ variables }}, - {%- for p in f.props() %} + _prefs = {{ prefs }}, {%- for p in f.props() %} {{p.name()|var_name}} = {{ p.typ()|literal(self, p.default(), context) }}{% if !loop.last %},{% endif %} {%- endfor %} ) 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 e05d52fa8bb..5fae7870ce7 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 %}) @@ -45,9 +47,29 @@ private constructor( {%- let defaults = format!("_defaults.{}", prop_kt) %} {%- let getter = p.typ()|property(p.name(), "_variables", defaults) %} {{ p.doc()|comment(" ") }} + {%- if !p.has_prefs() %} val {{ prop_kt }}: {{ type_kt }} by lazy { {{ getter }} } + {%- else %} + val {{ prop_kt }}: {{ type_kt }} + get() = + {%- let prefs = "it" %} + {%- let key = p.pref_key().unwrap() %} + {{ 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 %} {#- toJSON #} @@ -60,3 +82,5 @@ private constructor( ) ) {% endmacro %} + +{% macro prefs() %}this._prefs{% endmacro %} diff --git a/components/support/nimbus-fml/src/backends/mod.rs b/components/support/nimbus-fml/src/backends/mod.rs index e00481e16b0..6e18ac20d16 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) 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 a1faea64968..5d89f852de2 100644 --- a/components/support/nimbus-fml/src/backends/swift/templates/FeatureTemplate.swift +++ b/components/support/nimbus-fml/src/backends/swift/templates/FeatureTemplate.swift @@ -3,4 +3,26 @@ {%- let class_name = inner.name()|class_name -%} {{ swift::render_class(inner) }} +{%- if inner.has_prefs() %} + +extension {{ class_name }}: FMLFeatureInterface { + public func isModified() -> Bool { + guard let prefs = {{ 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/macros.swift b/components/support/nimbus-fml/src/backends/swift/templates/macros.swift index a4ffdcfeb99..c58b100e064 100644 --- a/components/support/nimbus-fml/src/backends/swift/templates/macros.swift +++ b/components/support/nimbus-fml/src/backends/swift/templates/macros.swift @@ -13,13 +13,15 @@ {% let class_name = inner.name()|class_name -%} {{ inner.doc()|comment("") }} -public class {{class_name}}: FMLFeatureInterface { +public class {{class_name}}: FMLObjectInterface { private let _variables: Variables private let _defaults: Defaults + private let _prefs: UserDefaults? - private init(variables: Variables = NilVariables.instance, defaults: Defaults) { + 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 #} @@ -34,6 +36,7 @@ public class {{class_name}}: FMLFeatureInterface { public convenience init( _ _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 %} @@ -67,10 +70,33 @@ public class {{class_name}}: FMLFeatureInterface { {%- let defaults = format!("_defaults.{}", prop_swift) %} {%- let getter = p.typ()|property(p.name(), "self._variables", defaults) %} {{ p.doc()|comment(" ") }} + {%- if !p.has_prefs() %} public lazy var {{ prop_swift }}: {{ type_swift }} = { {{ getter }} }() + {%- 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 }} = {{ prefs() }}, + let {{ prop_swift }} = {{ prefs }}.object(forKey: {{ key|quoted }}) as? {{ type_swift }} { + return {{ prop_swift }} + } + return {{ getter }} + } + {%- endif %} {% endfor %} } {%- endmacro %}} + +{% macro prefs() %}self._prefs{% endmacro %} diff --git a/components/support/nimbus-fml/src/fixtures/mod.rs b/components/support/nimbus-fml/src/fixtures/mod.rs index 4a808f671b2..53a64f2b756 100644 --- a/components/support/nimbus-fml/src/fixtures/mod.rs +++ b/components/support/nimbus-fml/src/fixtures/mod.rs @@ -54,6 +54,7 @@ impl PropDef { typ: typ.clone(), default: value.clone(), doc: format!("{nm} property of type {typ}"), + pref_key: None, gecko_pref: None, string_alias: None, } @@ -63,17 +64,18 @@ impl PropDef { nm: &str, typ: &TypeRef, value: &Value, - pref: &str, - branch: PrefBranch, + pref_key: &str, + pref_branch: PrefBranch, ) -> Self { Self { name: nm.to_string(), typ: typ.clone(), default: value.clone(), doc: format!("{nm} property of type {typ}"), + pref_key: None, gecko_pref: Some(GeckoPrefDef { - pref: pref.into(), - branch, + pref: pref_key.into(), + branch: pref_branch, }), string_alias: None, } @@ -85,6 +87,7 @@ impl PropDef { typ: typ.clone(), default: value.clone(), doc: nm.to_string(), + pref_key: None, gecko_pref: None, string_alias: Some(sa.clone()), } @@ -96,6 +99,7 @@ impl PropDef { doc: doc.to_string(), typ: typ.clone(), default: default.clone(), + pref_key: None, gecko_pref: None, string_alias: None, } diff --git a/components/support/nimbus-fml/src/frontend.rs b/components/support/nimbus-fml/src/frontend.rs index 0f854cce5e7..5714465a2ee 100644 --- a/components/support/nimbus-fml/src/frontend.rs +++ b/components/support/nimbus-fml/src/frontend.rs @@ -38,6 +38,10 @@ pub(crate) struct FeatureFieldBody { #[serde(flatten)] pub(crate) field: FieldBody, + #[serde(default)] + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) pref_key: Option, + #[serde(default)] #[serde(skip_serializing_if = "Option::is_none")] pub(crate) gecko_pref: Option, @@ -354,6 +358,7 @@ impl ManifestFrontEnd { 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.clone_from(&body.pref_key); prop.gecko_pref.clone_from(&body.gecko_pref); if let Some(s) = &body.string_alias { prop.string_alias = Some(TypeRef::StringAlias(s.clone())); @@ -389,6 +394,7 @@ impl ManifestFrontEnd { }, default: json!(body.default), string_alias: None, + pref_key: Default::default(), gecko_pref: Default::default(), } } diff --git a/components/support/nimbus-fml/src/intermediate_representation.rs b/components/support/nimbus-fml/src/intermediate_representation.rs index 60b99fcbb30..71c3c51fc76 100644 --- a/components/support/nimbus-fml/src/intermediate_representation.rs +++ b/components/support/nimbus-fml/src/intermediate_representation.rs @@ -117,6 +117,18 @@ impl Display for TypeRef { } impl TypeRef { + pub(crate) fn supports_prefs(&self) -> bool { + match self { + Self::Boolean | Self::String | Self::Int | Self::StringAlias(_) | 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, + } + } + pub(crate) fn supports_gecko_prefs(&self, lax_pref_validation: bool) -> bool { match self { Self::Option(boxed) => matches!(**boxed, Self::Boolean | Self::Int | Self::String), @@ -577,6 +589,10 @@ impl FeatureDef { Value::Object(props) } + pub fn has_prefs(&self) -> bool { + self.props.iter().any(|p| p.has_prefs()) + } + pub fn has_gecko_prefs(&self) -> bool { self.props.iter().any(|p| p.has_gecko_prefs()) } @@ -745,6 +761,9 @@ pub struct PropDef { pub(crate) default: Literal, #[serde(default)] #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) pref_key: Option, + #[serde(default)] + #[serde(skip_serializing_if = "Option::is_none")] pub(crate) gecko_pref: Option, #[serde(default)] #[serde(skip_serializing_if = "Option::is_none")] @@ -764,9 +783,15 @@ 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 has_gecko_prefs(&self) -> bool { self.gecko_pref.is_some() && self.typ.supports_gecko_prefs(false) } + pub fn pref_key(&self) -> Option { + self.pref_key.clone() + } pub fn gecko_pref(&self) -> Option { self.gecko_pref.clone() } 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 00000000000..f7899317bb3 --- /dev/null +++ b/components/support/nimbus-fml/test/pref_overrides.kts @@ -0,0 +1,66 @@ +/* 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" + )) +) + +// 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 } + +val feature = AppConfig.features.myFeature.value() + +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) +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") +assert(feature.isModified()) 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 00000000000..73428b7bb35 --- /dev/null +++ b/components/support/nimbus-fml/test/pref_overrides.swift @@ -0,0 +1,83 @@ +/* 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") +assert(!feature0.isModified()) + +// 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") +assert(!feature.isModified()) + +// 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") +assert(feature.isModified()) diff --git a/detekt-baseline.xml b/detekt-baseline.xml index 005475e9169..a1ac0eb65d3 100644 --- a/detekt-baseline.xml +++ b/detekt-baseline.xml @@ -12,6 +12,7 @@ UndocumentedPublicClass:ArgumentProcessor.kt$CliArgs UndocumentedPublicClass:Context.kt$Context UndocumentedPublicClass:Context.kt$Resources + UndocumentedPublicClass:Context.kt$SharedPreferences UndocumentedPublicClass:DatabaseLoginsStorage.kt$KeyRegenerationEventReason UndocumentedPublicClass:Drawable.kt$Drawable UndocumentedPublicClass:FeatureHolder.kt$NimbusFeatureException : Exception @@ -81,6 +82,15 @@ UndocumentedPublicFunction:Context.kt$Resources$fun getDrawable(resId: Int, theme: String) UndocumentedPublicFunction:Context.kt$Resources$fun getIdentifier(resName: String, defType: String, packageName: String): Int? UndocumentedPublicFunction:Context.kt$Resources$fun getResourceName(resId: Int) + UndocumentedPublicFunction:Context.kt$SharedPreferences$fun clear() + UndocumentedPublicFunction:Context.kt$SharedPreferences$fun contains(key: String): Boolean + UndocumentedPublicFunction:Context.kt$SharedPreferences$fun getBoolean(key: String, def: Boolean): Boolean + UndocumentedPublicFunction:Context.kt$SharedPreferences$fun getInt(key: String, def: Int): Int + UndocumentedPublicFunction:Context.kt$SharedPreferences$fun getString(key: String, def: String): String + UndocumentedPublicFunction:Context.kt$SharedPreferences$fun put(key: String, value: Boolean) + UndocumentedPublicFunction:Context.kt$SharedPreferences$fun put(key: String, value: Int) + UndocumentedPublicFunction:Context.kt$SharedPreferences$fun put(key: String, value: String) + UndocumentedPublicFunction:Context.kt$SharedPreferences$fun remove(key: String) UndocumentedPublicFunction:CreateKey.kt$fun createKey() UndocumentedPublicFunction:DatabaseLoginsStorage.kt$DatabaseLoginsStorage$@Throws(LoginsApiException::class) fun add(entry: LoginEntry): Login UndocumentedPublicFunction:DatabaseLoginsStorage.kt$DatabaseLoginsStorage$@Throws(LoginsApiException::class) fun addOrUpdate(entry: LoginEntry): Login