Bug 2020683 - Disallow simultaneous gecko-pref and default on FML variables

CHANGELOG.md

@@ -9,6 +9,7 @@
 * The `MetricsHandler` interface now requires two additional methods: `record_database_load()` and `record_database_migration()`
 * In Kotlin expose `GleanMetrics.Pings.nimbusTargetingContext` as `Nimbus.Pings.nimbusTargetingContext` for downstream tests. ([#14542](https://github.com/mozilla/experimenter/issues/14542))
 * `recordEventOrThrow()` now returns `Deferred<Unit>` instead of `Job`. Callers must use `.await()` (instead of `.join()`) to observe exceptions thrown during event recording.
+* It is now enforced via `nimbus-fml validate` that feature variables do not use both `gecko-pref` and `default` for the same variable.
 
 [Full Changelog](In progress)
 

components/support/nimbus-fml/src/defaults/validator.rs

@@ -67,6 +67,8 @@ impl<'a> DefaultsValidator<'a> {
         // This is only checking if a Map with an Enum as key has a complete set of keys (i.e. all variants)
         self.validate_feature_enum_maps(feature_def)?;
 
+        self.validate_no_defaults_for_gecko_prefs(feature_def)?;
+
         // Now check the examples for this feature.
         let path = ErrorPath::feature(&feature_def.name);
         for ex in &feature_def.examples {
@@ -154,6 +156,20 @@ impl<'a> DefaultsValidator<'a> {
         Ok(())
     }
 
+    fn validate_no_defaults_for_gecko_prefs(&self, feature_def: &FeatureDef) -> Result<()> {
+        let path = ErrorPath::feature(&feature_def.name);
+        for prop in &feature_def.props {
+            if prop.gecko_pref.is_some() && !prop.default.is_null() {
+                let path = path.property(&prop.name);
+                return Err(FMLError::ValidationError(
+                    path.path,
+                    "gecko-pref and default are mutually exclusive".into(),
+                ));
+            }
+        }
+        Ok(())
+    }
+
     /// Check enum maps (Map<Enum, T>) have all keys represented.
     ///
     /// We split this out because if the FML has all keys, then any feature configs do as well.
@@ -1302,3 +1318,37 @@ mod string_alias {
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod test_gecko_prefs {
+    use serde_json::json;
+
+    use crate::error::FMLError;
+    use crate::intermediate_representation::{GeckoPrefDef, PrefBranch};
+
+    use super::*;
+
+    #[test]
+    fn test_validate_default_mutually_exclusive_with_gecko_pref() {
+        let mut prop = PropDef::new("var", &TypeRef::String, &json!("default-value"));
+        prop.gecko_pref = Some(GeckoPrefDef {
+            pref: "some.pref".into(),
+            branch: PrefBranch::User,
+        });
+
+        let feature = FeatureDef {
+            name: "Feature".to_string(),
+            props: vec![prop],
+            ..Default::default()
+        };
+
+        let objs = Default::default();
+        let enums = Default::default();
+        let validator = DefaultsValidator::new(&enums, &objs);
+
+        assert!(matches!(
+            validator.validate_feature_def(&feature),
+            Err(FMLError::ValidationError(path, reason)) if path == "features/Feature.var" && reason == "gecko-pref and default are mutually exclusive",
+        ));
+    }
+}