Bug 2053517 - Revert 4a8546f9b047 and 36f1b0ea4bcd for build breakage

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.

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`.
      *

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<String>,
     server: NimbusServerSettings?,

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<T : NimbusInterface>(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<NimbusInter
         Nimbus(
             context,
             appInfo = appInfo,
+            prefs = sharedPreferences,
             coenrollingFeatureIds = getCoenrollingFeatureIds(),
             server = serverSettings,
             deviceInfo = createDeviceInfo(),

components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/internal/FeatureHolder.kt

@@ -4,6 +4,7 @@
 
 package org.mozilla.experiments.nimbus.internal
 
+import android.content.SharedPreferences
 import org.json.JSONObject
 import org.mozilla.experiments.nimbus.FeaturesInterface
 import org.mozilla.experiments.nimbus.HardcodedNimbusFeatures
@@ -23,7 +24,7 @@ import kotlin.concurrent.withLock
 class FeatureHolder<T : FMLFeatureInterface>(
     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<T : FMLFeatureInterface>(
                 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<T : FMLFeatureInterface>(
      * their behavior because of it.
      */
     fun recordExposure() {
-        getSdk()?.recordExposureEvent(featureId)
+        if (!value().isModified()) {
+            getSdk()?.recordExposureEvent(featureId)
+        }
     }
 
     /**
@@ -74,7 +78,9 @@ class FeatureHolder<T : FMLFeatureInterface>(
      * [recordExposure] instead.
      */
     fun recordExperimentExposure(slug: String) {
-        getSdk()?.recordExposureEvent(featureId, slug)
+        if (!value().isModified()) {
+            getSdk()?.recordExposureEvent(featureId, slug)
+        }
     }
 
     /**
@@ -110,7 +116,7 @@ class FeatureHolder<T : FMLFeatureInterface>(
      *
      * 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
+}

components/nimbus/android/src/test/java/org/mozilla/experiments/nimbus/util/TestNimbusBuilder.kt

@@ -21,6 +21,7 @@ class TestNimbusBuilder(context: Context) : AbstractNimbusBuilder<NimbusInterfac
     ): NimbusInterface =
         Nimbus(
             context = context,
+            prefs = sharedPreferences,
             appInfo = appInfo,
             coenrollingFeatureIds = listOf(),
             server = serverSettings,

components/support/nimbus-fml/fixtures/android/runtime/Context.kt

@@ -35,3 +35,20 @@ object Resources {
 
     fun getResourceName(resId: Int) = "res:$resId"
 }
+
+@Suppress("PACKAGE_OR_CLASSIFIER_REDECLARATION")
+class SharedPreferences {
+    // 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<String, Any>()
+    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)
+}

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

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

components/support/nimbus-fml/src/backends/frontend_manifest.rs

@@ -143,6 +143,7 @@ impl From<PropDef> for FieldBody {
 impl From<PropDef> 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(),

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<String> {
+        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),

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<T, P, K>(
+    type_: T,
+    _: &dyn askama::Values,
+    prefs: P,
+    pref_key: K,
+) -> Result<String, askama::Error>
+where
+    T: Borrow<TypeIdentifier>,
+    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<P, T>(prop: P, _: &dyn askama::Values, type_: T) -> Result<String, askama::Error>
 where

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::<HashSet<String>>()

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<String> {
+        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<String> {
+        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<String> {
+        Some(format!("{prefs}.getString({}, \"\")", quoted(pref_key)))
+    }
 }
 
 #[cfg(test)]

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 %}

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 %}
 }

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 %}
                 )