Commit 15fd5a00 authored by Beth Rennie's avatar Beth Rennie
Browse files

Bug 1761105 - Validate Nimbus features using variables instead of schemas r=emcminn 

Not all features are complicated enough to require full on JSON schema to
describe their variables. For these simpler features, we generate (a lax) JSON
schema based on the contents of their `variables` field to validate them. This
way we can re-use the existing validation mechanism and do not have to write a
bespoke validator.

The schema generated is intended to be compatible with the one generated by
Experimenter.

Differential Revision: https://phabricator.services.mozilla.com/D141892
parent 88e985bc

toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.jsm

+52 −2
Original line number Diff line number Diff line
@@ -374,8 +374,11 @@ class _RemoteSettingsExperimentLoader { 
            );

          }

        } else {

          // TODO: Convert NimbusFeatures[featureId].manifest.variables into a

          //       schema OR add schemas for all.

          const schema = this._generateVariablesOnlySchema(

            featureId,

            NimbusFeatures[featureId].manifest

          );

          validator = validatorCache[featureId] = new Validator(schema);

          continue;

        }
@@ -401,6 +404,53 @@ class _RemoteSettingsExperimentLoader { 


    return true;

  }



  _generateVariablesOnlySchema(featureId, manifest) {

    // See-also: https://github.com/mozilla/experimenter/blob/main/app/experimenter/features/__init__.py#L21-L64

    const schema = {

      $schema: "https://json-schema.org/draft/2019-09/schema",

      title: featureId,

      description: manifest.description,

      type: "object",

      properties: {},

      additionalProperties: true,

    };



    for (const [varName, desc] of Object.entries(manifest.variables)) {

      const prop = {};

      switch (desc.type) {

        case "boolean":

        case "string":

          prop.type = desc.type;

          break;



        case "int":

          // NB: This is what Experimenter maps the int type to.

          prop.type = "number";

          break;



        case "json":

          // NB: Experimenter presently ignores the json type, it will still be

          // allowed under additionalProperties.

          continue;



        default:

          // NB: Experimenter doesn't outright reject invalid types either.

          Cu.reportError(

            `Feature ID ${featureId} has variable ${varName} with invalid FML type: ${prop.type}`

          );

          continue;

      }



      if (prop.type === "string" && !!desc.enum) {

        prop.enum = [...desc.enum];

      }



      schema.properties[varName] = prop;

    }



    return schema;

  }

}



const RemoteSettingsExperimentLoader = new _RemoteSettingsExperimentLoader();

toolkit/components/nimbus/test/browser/browser_remotesettingsexperimentloader_remote_defaults.js

+4 −2
Original line number Diff line number Diff line
@@ -296,10 +296,12 @@ add_task(async function test_remote_fetch_on_updateRecipes() { 


add_task(async function test_finalizeRemoteConfigs_cleanup() {

  const featureFoo = new ExperimentFeature("foo", {

    foo: { description: "mochitests" },

    description: "mochitests",

    variables: {},

  });

  const featureBar = new ExperimentFeature("bar", {

    foo: { description: "mochitests" },

    description: "mochitests",

    variables: {},

  });



  registerCleanupFunction(

toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader_updateRecipes.js

+105 −0
Original line number Diff line number Diff line
@@ -6,6 +6,9 @@ const { ExperimentFakes } = ChromeUtils.import( 
const { FirstStartup } = ChromeUtils.import(

  "resource://gre/modules/FirstStartup.jsm"

);

const { NimbusFeatures } = ChromeUtils.import(

  "resource://nimbus/ExperimentAPI.jsm"

);

const { PanelTestProvider } = ChromeUtils.import(

  "resource://activity-stream/lib/PanelTestProvider.jsm"

);
@@ -315,3 +318,105 @@ add_task(async function test_updateRecipes_invalidBranchAfterUpdate() { 
    "should call .onFinalize with an invalid branch"

  );

});



add_task(async function test_updateRecipes_simpleFeatureInvalidAfterUpdate() {

  const loader = ExperimentFakes.rsLoader();

  const manager = loader.manager;



  const recipe = ExperimentFakes.recipe("foo");

  const badRecipe = ExperimentFakes.recipe("foo", {

    branches: [

      {

        ...recipe.branches[0],

        features: [

          {

            featureId: "testFeature",

            value: { testInt: "abc123", enabled: true },

          },

        ],

      },

      {

        ...recipe.branches[1],

        features: [

          {

            featureId: "testFeature",

            value: { testInt: "xyz456", enabled: true },

          },

        ],

      },

    ],

  });



  const EXPECTED_SCHEMA = {

    $schema: "https://json-schema.org/draft/2019-09/schema",

    title: "testFeature",

    description: NimbusFeatures.testFeature.manifest.description,

    type: "object",

    properties: {

      testInt: {

        type: "number",

      },

    },

    additionalProperties: true,

  };



  sinon.spy(loader, "updateRecipes");

  sinon.spy(loader, "_generateVariablesOnlySchema");

  sinon.stub(loader, "setTimer");

  sinon.stub(loader.remoteSettingsClient, "get").resolves([recipe]);



  sinon.stub(manager, "onFinalize");

  sinon.stub(manager, "onRecipe");

  sinon.stub(manager.store, "ready").resolves();



  await loader.init();

  ok(manager.onRecipe.calledOnce, "should call .updateRecipes");

  equal(loader.manager.onRecipe.callCount, 1, "should call .onRecipe once");

  ok(

    loader.manager.onRecipe.calledWith(recipe, "rs-loader"),

    "should call .onRecipe with argument data"

  );

  equal(loader.manager.onFinalize.callCount, 1, "should call .onFinalize once");

  ok(

    loader.manager.onFinalize.calledWith("rs-loader", {

      recipeMismatches: [],

      invalidRecipes: [],

      invalidBranches: [],

    }),

    "should call .onFinalize with nomismatches or invalid recipes"

  );



  ok(

    loader._generateVariablesOnlySchema.calledOnce,

    "Should have generated a schema for testFeature"

  );

  ok(

    loader._generateVariablesOnlySchema.returned(EXPECTED_SCHEMA),

    "should have generated a schema with one field"

  );



  info("Replacing recipe with an invalid one");



  loader.remoteSettingsClient.get.resolves([badRecipe]);



  await loader.updateRecipes("timer");

  equal(

    manager.onRecipe.callCount,

    1,

    "should not have called .onRecipe again"

  );

  equal(

    manager.onFinalize.callCount,

    2,

    "should have called .onFinalize again"

  );



  ok(

    loader.manager.onFinalize.secondCall.calledWith("rs-loader", {

      recipeMismatches: [],

      invalidRecipes: [],

      invalidBranches: ["foo"],

    }),

    "should call .onFinalize with an invalid branch"

  );

});