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

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,8 @@
 # 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`.
+- The `nimbus-fml validate`, `nimbus-fml single-file`, and `nimbus-fml generate-experimenter` 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.
 
 [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/command_line/cli.rs

@@ -79,6 +79,10 @@ pub struct GenerateExperimenter {
 
     #[command(flatten)]
     pub loader_info: LoaderInfo,
+
+    #[arg(long)]
+    /// Enable lax gecko-pref validation.
+    pub lax_gecko_pref_validation: bool,
 }
 
 #[derive(Args)]
@@ -107,6 +111,10 @@ pub struct SingleFile {
 
     #[command(flatten)]
     pub loader_info: LoaderInfo,
+
+    #[arg(long)]
+    /// Enable lax gecko-pref validation.
+    pub lax_gecko_pref_validation: bool,
 }
 
 #[derive(Args)]
@@ -117,6 +125,10 @@ pub struct Validate {
 
     #[command(flatten)]
     pub loader_info: LoaderInfo,
+
+    #[arg(long)]
+    /// Enable lax gecko-pref validation.
+    pub lax_gecko_pref_validation: bool,
 }
 
 #[derive(Args)]

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

@@ -34,18 +34,21 @@ pub(crate) struct GenerateExperimenterManifestCmd {
     pub(crate) language: TargetLanguage,
     pub(crate) load_from_ir: bool,
     pub(crate) loader: LoaderConfig,
+    pub(crate) lax_gecko_pref_validation: bool,
 }
 
 pub(crate) struct GenerateSingleFileManifestCmd {
     pub(crate) manifest: String,
     pub(crate) output: PathBuf,
     pub(crate) channel: String,
     pub(crate) loader: LoaderConfig,
+    pub(crate) lax_gecko_pref_validation: bool,
 }
 
 pub(crate) struct ValidateCmd {
     pub(crate) manifest: String,
     pub(crate) loader: LoaderConfig,
+    pub(crate) lax_gecko_pref_validation: bool,
 }
 
 pub(crate) struct PrintChannelsCmd {

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

@@ -91,6 +91,7 @@ fn create_single_file_from_cli(
         output,
         channel,
         loader,
+        lax_gecko_pref_validation: cmd.lax_gecko_pref_validation,
     })
 }
 
@@ -110,6 +111,7 @@ fn create_generate_command_experimenter_from_cli(
         language,
         load_from_ir,
         loader,
+        lax_gecko_pref_validation: cmd.lax_gecko_pref_validation,
     };
     Ok(cmd)
 }
@@ -172,7 +174,11 @@ fn create_loader(
 fn create_validate_command_from_cli(cmd: &cli::Validate, cwd: &Path) -> Result<ValidateCmd> {
     let manifest = cmd.input.clone();
     let loader = create_loader(&cmd.input, &cmd.loader_info, cwd)?;
-    Ok(ValidateCmd { manifest, loader })
+    Ok(ValidateCmd {
+        manifest,
+        loader,
+        lax_gecko_pref_validation: cmd.lax_gecko_pref_validation,
+    })
 }
 
 fn create_print_channels_from_cli(cmd: &cli::Channels, cwd: &Path) -> Result<PrintChannelsCmd> {

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.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.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,7 @@ 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.lax_gecko_pref_validation)),
                 Err(e) => (c, Err(e)),
             }
         })
@@ -352,7 +366,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 +398,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 +422,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.
@@ -560,6 +576,7 @@ mod test {
             let cmd = ValidateCmd {
                 loader: Default::default(),
                 manifest,
+                lax_gecko_pref_validation: false,
             };
             validate(&cmd)?;
         }
@@ -573,6 +590,7 @@ mod test {
         let cmd = ValidateCmd {
             loader: Default::default(),
             manifest,
+            lax_gecko_pref_validation: false,
         };
         let result = validate(&cmd);
 
@@ -601,6 +619,7 @@ mod test {
             language: TargetLanguage::ExperimenterYAML,
             load_from_ir,
             loader,
+            lax_gecko_pref_validation: false,
         })
     }
 
@@ -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()?;
@@ -622,13 +641,14 @@ mod test {
             manifest,
             output: output.path().into(),
             channel: channel.to_string(),
+            lax_gecko_pref_validation: false,
         };
         generate_single_file_manifest(&cmd)?;
 
         // 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(())));
+    }
+}