diff --git a/CHANGELOG.md b/CHANGELOG.md index a375dd0332..4c24ae5888 100644 --- a/CHANGELOG.md +++ b/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` 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) diff --git a/components/support/nimbus-fml/src/defaults/validator.rs b/components/support/nimbus-fml/src/defaults/validator.rs index 1337b400d4..666dad8a8d 100644 --- a/components/support/nimbus-fml/src/defaults/validator.rs +++ b/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) 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", + )); + } +}