Bug 2026043 - Require Option<T> for gecko-pref variables and require null defaults by default (#7296)

In bug 2020683 we made the default field mutually exclusive with
gecko-pref. However, this lead to another error, namely that gecko-pref
was only valid for string, integer, and boolean types and `null` is not
a valid value for these types. This issue lead to a backout in bug 2025587.

This patch re-adds the requirement of null defaults for gecko-pref
variables but also adds the requirement that these variables must be
`Option<T>`, where `T` is a string, boolean, or integer.

These constraints *can* be overridden by supplying the
`--lax-gecko-pref-validation` flag to the `fml single-file`,
`fml generate-experimenter`, and `fml validate` commands. This will
allow Experimenter to continue to ingest manifests without error.

Once the invalid manifest is ingested into Experimenter, we can manually
edit the manifest to remove the invalid feature and eventually disable
this validation mode.

Author: freshstrangemusic

CHANGELOG.md

@@ -1,7 +1,9 @@
 # v151.0 (In progress)
 
 ## ⚠️ Breaking Changes ⚠️
-- `nimbus-fml validate` no longer enforces that variables do not use both `gecko-pref` and `default` due to causing CI failures in Experimenter.
+- It is now enforced by `nimbus-fml` that feature variables using gecko-pref must be have `type: Option<T>`, for `T` in `Boolean`, `Int`, and `String`.
+- `nimbus-fml` commands now all have a `--lax-gecko-pref-validation` flag to bypass the above restriction, as well as the restriction that `gecko-pref` and `default` are mutually exclusive.
+- The `FmlLoaderConfig` now has a `lax_gecko_pref_validation` field to allow `FmlClient` consumers (i.e., Experimenter) to opt-in to lax validation.
 
 [Full Changelog](In progress)
 

components/support/nimbus-fml/fixtures/fe/gecko-pref.yaml

@@ -0,0 +1,36 @@
+---
+about:
+  description: This file codes for all features in the Fenix browser
+  kotlin:
+    class: .nimbus.FxNimbus
+    package: com.example.app
+  swift:
+    class: MyNimbus
+    module: Client
+channels:
+  - release
+  - beta
+  - nightly
+  - developer
+features:
+  gecko-nimbus-validation:
+    description: "Feature for validating Nimbus Gecko functionality"
+    variables:
+      test-preference-bool:
+        description: "A test gecko preference"
+        type: Option<Boolean>
+        gecko-pref:
+          pref: "gecko.nimbus.test.bool"
+          branch: "default"
+      test-preference-int:
+        description: "A test gecko preference"
+        type: Option<Int>
+        gecko-pref:
+          pref: "gecko.nimbus.test.int"
+          branch: "default"
+      test-preference-string:
+        description: "A test gecko preference"
+        type: Option<String>
+        gecko-pref:
+          pref: "gecko.nimbus.test.string"
+          branch: "default"

components/support/nimbus-fml/src/client/config.rs

@@ -11,6 +11,7 @@ pub struct FmlLoaderConfig {
     pub cache: Option<String>,
     pub refs: HashMap<String, String>,
     pub ref_files: Vec<String>,
+    pub lax_gecko_pref_validation: bool,
 }
 
 impl From<FmlLoaderConfig> for LoaderConfig {
@@ -22,6 +23,7 @@ impl From<FmlLoaderConfig> for LoaderConfig {
             refs: value.refs.into_iter().collect(),
             repo_files: value.ref_files,
             cache_dir: cache,
+            lax_gecko_pref_validation: value.lax_gecko_pref_validation,
         }
     }
 }

components/support/nimbus-fml/src/client/mod.rs

@@ -79,7 +79,7 @@ impl FmlClient {
         let path = files.file_path(&manifest_path)?;
         let parser: Parser = Parser::new(files, path)?;
         let ir = parser.get_intermediate_representation(Some(&channel))?;
-        ir.validate_manifest()?;
+        ir.validate_manifest_with(config.lax_gecko_pref_validation)?;
 
         Ok(FmlClient {
             default_json: get_default_json_for_manifest(&ir)?,

components/support/nimbus-fml/src/command_line/cli.rs

@@ -174,6 +174,10 @@ pub struct LoaderInfo {
     /// If INPUT is a remote file, then use this as the tag or branch name.
     #[arg(long = "ref")]
     pub ref_: Option<String>,
+
+    #[arg(long)]
+    /// Enable lax gecko-pref validation.
+    pub lax_gecko_pref_validation: bool,
 }
 
 impl From<Language> for TargetLanguage {

components/support/nimbus-fml/src/command_line/mod.rs

@@ -166,6 +166,7 @@ fn create_loader(
         repo_files,
         cwd,
         refs,
+        lax_gecko_pref_validation: loader_info.lax_gecko_pref_validation,
     })
 }
 

components/support/nimbus-fml/src/command_line/workflows.rs

@@ -87,6 +87,7 @@ fn generate_struct_single(
         manifest_path,
         cmd.load_from_ir,
         Some(&cmd.channel),
+        false,
     )?;
     generate_struct_from_ir(&ir, cmd)
 }
@@ -112,15 +113,27 @@ fn generate_struct_from_ir(ir: &FeatureManifest, cmd: &GenerateStructCmd) -> Res
 pub(crate) fn generate_experimenter_manifest(cmd: &GenerateExperimenterManifestCmd) -> Result<()> {
     let files: FileLoader = TryFrom::try_from(&cmd.loader)?;
     let path = files.file_path(&cmd.manifest)?;
-    let ir = load_feature_manifest(files, path, cmd.load_from_ir, None)?;
+    let ir = load_feature_manifest(
+        files,
+        path,
+        cmd.load_from_ir,
+        None,
+        cmd.loader.lax_gecko_pref_validation,
+    )?;
     backends::experimenter_manifest::generate_manifest(ir, cmd)?;
     Ok(())
 }
 
 pub(crate) fn generate_single_file_manifest(cmd: &GenerateSingleFileManifestCmd) -> Result<()> {
     let files: FileLoader = TryFrom::try_from(&cmd.loader)?;
     let path = files.file_path(&cmd.manifest)?;
-    let fm = load_feature_manifest(files, path, false, Some(&cmd.channel))?;
+    let fm = load_feature_manifest(
+        files,
+        path,
+        false,
+        Some(&cmd.channel),
+        cmd.loader.lax_gecko_pref_validation,
+    )?;
     let frontend: ManifestFrontEnd = fm.into();
     std::fs::write(&cmd.output, serde_yaml::to_string(&frontend)?)?;
     Ok(())
@@ -131,14 +144,15 @@ fn load_feature_manifest(
     path: FilePath,
     load_from_ir: bool,
     channel: Option<&str>,
+    lax_gecko_pref_validation: bool,
 ) -> Result<FeatureManifest> {
     let ir = if !load_from_ir {
         let parser: Parser = Parser::new(files, path)?;
         parser.get_intermediate_representation(channel)?
     } else {
         files.read_ir::<FeatureManifest>(&path)?
     };
-    ir.validate_manifest()?;
+    ir.validate_manifest_with(lax_gecko_pref_validation)?;
     Ok(ir)
 }
 
@@ -302,7 +316,10 @@ pub(crate) fn validate(cmd: &ValidateCmd) -> Result<()> {
         .map(|c| {
             let intermediate_representation = parser.get_intermediate_representation(Some(c));
             match intermediate_representation {
-                Ok(ir) => (c, ir.validate_manifest()),
+                Ok(ir) => (
+                    c,
+                    ir.validate_manifest_with(cmd.loader.lax_gecko_pref_validation),
+                ),
                 Err(e) => (c, Err(e)),
             }
         })
@@ -352,7 +369,7 @@ pub(crate) fn print_channels(cmd: &PrintChannelsCmd) -> Result<()> {
 pub(crate) fn print_info(cmd: &PrintInfoCmd) -> Result<()> {
     let files: FileLoader = TryFrom::try_from(&cmd.loader)?;
     let path = files.file_path(&cmd.manifest)?;
-    let fm = load_feature_manifest(files, path.clone(), false, cmd.channel.as_deref())?;
+    let fm = load_feature_manifest(files, path.clone(), false, cmd.channel.as_deref(), false)?;
     let info = if let Some(feature_id) = &cmd.feature {
         ManifestInfo::from_feature(&path, &fm, feature_id)?
     } else {
@@ -384,6 +401,7 @@ mod test {
         "fixtures/ir/full_homescreen.ir.json",
         "fixtures/fe/importing/simple/app.yaml",
         "fixtures/fe/importing/diamond/00-app.yaml",
+        "fixtures/fe/gecko-pref.yaml",
     ];
 
     #[allow(dead_code)]
@@ -407,7 +425,8 @@ mod test {
     fn generate_struct_cli_overrides(from_cli: AboutBlock, cmd: &GenerateStructCmd) -> Result<()> {
         let files: FileLoader = TryFrom::try_from(&cmd.loader)?;
         let path = files.file_path(&cmd.manifest)?;
-        let mut ir = load_feature_manifest(files, path, cmd.load_from_ir, Some(&cmd.channel))?;
+        let mut ir =
+            load_feature_manifest(files, path, cmd.load_from_ir, Some(&cmd.channel), false)?;
 
         // We do a dance here to make sure that we can override class names and package names during tests,
         // and while we still have to support setting those options from the command line.
@@ -611,7 +630,7 @@ mod test {
         // Load the source file, and get the default_json()
         let files: FileLoader = TryFrom::try_from(&loader)?;
         let src = files.file_path(&manifest)?;
-        let fm = load_feature_manifest(files, src, false, Some(channel))?;
+        let fm = load_feature_manifest(files, src, false, Some(channel), false)?;
         let expected = fm.default_json();
 
         let output = NamedTempFile::new()?;
@@ -628,7 +647,7 @@ mod test {
         // Reload the generated file, and get the default_json()
         let dest = FilePath::Local(output.path().into());
         let files: FileLoader = TryFrom::try_from(&loader)?;
-        let fm = load_feature_manifest(files, dest, false, Some(channel))?;
+        let fm = load_feature_manifest(files, dest, false, Some(channel), false)?;
         let observed = fm.default_json();
 
         // They should be the same.

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

@@ -15,6 +15,8 @@ use std::collections::{BTreeMap, HashMap, HashSet};
 pub(crate) struct DefaultsValidator<'a> {
     enum_defs: &'a BTreeMap<String, EnumDef>,
     object_defs: &'a BTreeMap<String, ObjectDef>,
+
+    lax_gecko_pref_validation: bool,
 }
 
 impl<'a> DefaultsValidator<'a> {
@@ -25,9 +27,15 @@ impl<'a> DefaultsValidator<'a> {
         Self {
             enum_defs,
             object_defs,
+            lax_gecko_pref_validation: false,
         }
     }
 
+    pub(crate) fn with_lax_gecko_pref_validation(mut self, value: bool) -> Self {
+        self.lax_gecko_pref_validation = value;
+        self
+    }
+
     pub(crate) fn validate_object_def(&self, object_def: &ObjectDef) -> Result<(), FMLError> {
         let mut errors = Default::default();
         let path = ErrorPath::object(&object_def.name);
@@ -67,6 +75,10 @@ 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)?;
 
+        if !self.lax_gecko_pref_validation {
+            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 +166,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 +1328,40 @@ 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",
+        ));
+
+        let validator = DefaultsValidator::new(&enums, &objs).with_lax_gecko_pref_validation(true);
+        assert!(matches!(validator.validate_feature_def(&feature), Ok(())));
+    }
+}

components/support/nimbus-fml/src/fml.udl

@@ -23,6 +23,7 @@ dictionary FmlLoaderConfig {
     string? cache;
     record<DOMString, string> refs;
     sequence<string> ref_files;
+    boolean lax_gecko_pref_validation;
 };
 
 interface FmlClient {

components/support/nimbus-fml/src/intermediate_representation.rs

@@ -129,6 +129,14 @@ impl TypeRef {
         }
     }
 
+    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),
+            Self::Boolean | Self::Int | Self::String => lax_pref_validation,
+            _ => false,
+        }
+    }
+
     pub(crate) fn name(&self) -> Option<&str> {
         match self {
             Self::Enum(s) | Self::Object(s) | Self::StringAlias(s) => Some(s),
@@ -303,22 +311,33 @@ impl FeatureManifest {
         Ok(())
     }
 
+    #[allow(dead_code)]
     pub fn validate_manifest(&self) -> Result<()> {
+        self.validate_manifest_with(false)
+    }
+
+    pub fn validate_manifest_with(&self, lax_gecko_pref_validation: bool) -> Result<()> {
         // We then validate that each type_ref is valid
-        self.validate_schema()?;
-        self.validate_defaults()?;
+        self.validate_schema_with(lax_gecko_pref_validation)?;
+        self.validate_defaults_with(lax_gecko_pref_validation)?;
 
         // Validating the imported manifests.
         // This is not only validating the well formed-ness of the imported manifests
         // but also the defaults that are sent into the child manifests.
         for child in self.all_imports.values() {
-            child.validate_manifest()?;
+            child.validate_manifest_with(lax_gecko_pref_validation)?;
         }
         Ok(())
     }
 
+    #[allow(dead_code)]
     fn validate_schema(&self) -> Result<(), FMLError> {
-        let validator = SchemaValidator::new(&self.enum_defs, &self.obj_defs);
+        self.validate_schema_with(false)
+    }
+
+    fn validate_schema_with(&self, lax_gecko_pref_validation: bool) -> Result<(), FMLError> {
+        let validator = SchemaValidator::new(&self.enum_defs, &self.obj_defs)
+            .with_lax_gecko_pref_validation(lax_gecko_pref_validation);
         for object in self.iter_object_defs() {
             validator.validate_object_def(object)?;
         }
@@ -329,8 +348,14 @@ impl FeatureManifest {
         Ok(())
     }
 
+    #[allow(dead_code)]
     fn validate_defaults(&self) -> Result<()> {
-        let validator = DefaultsValidator::new(&self.enum_defs, &self.obj_defs);
+        self.validate_defaults_with(false)
+    }
+
+    fn validate_defaults_with(&self, lax_gecko_pref_validation: bool) -> Result<()> {
+        let validator = DefaultsValidator::new(&self.enum_defs, &self.obj_defs)
+            .with_lax_gecko_pref_validation(lax_gecko_pref_validation);
         for object in self.iter_object_defs() {
             validator.validate_object_def(object)?;
         }
@@ -762,7 +787,7 @@ impl PropDef {
         self.pref_key.is_some() && self.typ.supports_prefs()
     }
     pub fn has_gecko_prefs(&self) -> bool {
-        self.gecko_pref.is_some() && self.typ.supports_prefs()
+        self.gecko_pref.is_some() && self.typ.supports_gecko_prefs(false)
     }
     pub fn pref_key(&self) -> Option<String> {
         self.pref_key.clone()