Bug 1716562 - Use a pref for each property in the value field of an experiment r=k88hudson

Differential Revision: https://phabricator.services.mozilla.com/D118000

toolkit/components/nimbus/lib/ExperimentStore.jsm

@@ -15,6 +15,7 @@ const { XPCOMUtils } = ChromeUtils.import(
 const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
 XPCOMUtils.defineLazyModuleGetters(this, {
   FeatureManifest: "resource://nimbus/FeatureManifest.js",
+  PrefUtils: "resource://normandy/lib/PrefUtils.jsm",
 });
 
 const IS_MAIN_PROCESS =
@@ -60,27 +61,141 @@ XPCOMUtils.defineLazyGetter(this, "syncDataStore", () => {
         Cu.reportError(e);
       }
     },
+    _trySetTypedPrefValue(pref, value) {
+      let variableType = typeof value;
+      switch (variableType) {
+        case "boolean":
+          Services.prefs.setBoolPref(pref, value);
+          break;
+        case "number":
+          Services.prefs.setIntPref(pref, value);
+          break;
+        case "string":
+          Services.prefs.setStringPref(pref, value);
+          break;
+        case "object":
+          Services.prefs.setStringPref(pref, JSON.stringify(value));
+          break;
+      }
+    },
+    _clearBranchChildValues(prefBranch) {
+      const variablesBranch = Services.prefs.getBranch(prefBranch);
+      const prefChildList = variablesBranch.getChildList("");
+      for (let variable of prefChildList) {
+        variablesBranch.clearUserPref(variable);
+      }
+    },
+    /**
+     * Given a branch pref returns all child prefs and values
+     * { childPref: value }
+     * where value is parsed to the appropriate type
+     *
+     * @returns {Object[]}
+     */
+    _getBranchChildValues(prefBranch, featureId) {
+      const branch = Services.prefs.getBranch(prefBranch);
+      const prefChildList = branch.getChildList("");
+      let values = {};
+      if (!prefChildList.length) {
+        return null;
+      }
+      for (const childPref of prefChildList) {
+        let prefName = `${prefBranch}${childPref}`;
+        let value = PrefUtils.getPref(prefName);
+        // Try to parse string values that could be stringified objects
+        if (FeatureManifest[featureId]?.variables[childPref]?.type === "json") {
+          let parsedValue = tryJSONParse(value);
+          if (parsedValue) {
+            value = parsedValue;
+          }
+        }
+        values[childPref] = value;
+      }
+
+      return values;
+    },
     get(featureId) {
-      return this._tryParsePrefValue(experimentsPrefBranch, featureId);
+      let metadata = this._tryParsePrefValue(experimentsPrefBranch, featureId);
+      if (!metadata) {
+        return null;
+      }
+      let prefBranch = `${SYNC_DATA_PREF_BRANCH}${featureId}.`;
+      metadata.branch.feature.value = this._getBranchChildValues(
+        prefBranch,
+        featureId
+      );
+
+      return metadata;
     },
     getDefault(featureId) {
-      return this._tryParsePrefValue(defaultsPrefBranch, featureId);
+      let metadata = this._tryParsePrefValue(defaultsPrefBranch, featureId);
+      if (!metadata) {
+        return null;
+      }
+      let prefBranch = `${SYNC_DEFAULTS_PREF_BRANCH}${featureId}.`;
+      metadata.variables = this._getBranchChildValues(prefBranch, featureId);
+
+      return metadata;
     },
     set(featureId, value) {
-      this._trySetPrefValue(experimentsPrefBranch, featureId, value);
+      /* If the enrollment branch has variables we store those separately
+       * in pref branches of appropriate type:
+       * { featureId: "foo", value: { enabled: true } }
+       * gets stored as `${SYNC_DATA_PREF_BRANCH}foo.enabled=true`
+       */
+      if (value.branch?.feature?.value) {
+        for (let variable of Object.keys(value.branch.feature.value)) {
+          let prefName = `${SYNC_DATA_PREF_BRANCH}${featureId}.${variable}`;
+          this._trySetTypedPrefValue(
+            prefName,
+            value.branch.feature.value[variable]
+          );
+        }
+        this._trySetPrefValue(experimentsPrefBranch, featureId, {
+          ...value,
+          branch: {
+            ...value.branch,
+            feature: {
+              ...value.branch.feature,
+              value: null,
+            },
+          },
+        });
+      } else {
+        this._trySetPrefValue(experimentsPrefBranch, featureId, value);
+      }
     },
     setDefault(featureId, value) {
-      this._trySetPrefValue(defaultsPrefBranch, featureId, value);
+      /* We store configuration variables separately in pref branches of
+       * appropriate type:
+       * (feature: "foo") { variables: { enabled: true } }
+       * gets stored as `${SYNC_DEFAULTS_PREF_BRANCH}foo.enabled=true`
+       */
+      for (let variable of Object.keys(value.variables || {})) {
+        let prefName = `${SYNC_DEFAULTS_PREF_BRANCH}${featureId}.${variable}`;
+        this._trySetTypedPrefValue(prefName, value.variables[variable]);
+      }
+      this._trySetPrefValue(defaultsPrefBranch, featureId, {
+        ...value,
+        variables: null,
+      });
     },
     getAllDefaultBranches() {
-      return defaultsPrefBranch.getChildList("");
+      return defaultsPrefBranch.getChildList("").filter(
+        // Filter out remote defaults variable prefs
+        pref => !pref.includes(".")
+      );
     },
     delete(featureId) {
+      const prefBranch = `${SYNC_DATA_PREF_BRANCH}${featureId}.`;
+      this._clearBranchChildValues(prefBranch);
       try {
         experimentsPrefBranch.clearUserPref(featureId);
       } catch (e) {}
     },
     deleteDefault(featureId) {
+      let prefBranch = `${SYNC_DEFAULTS_PREF_BRANCH}${featureId}.`;
+      this._clearBranchChildValues(prefBranch);
       try {
         defaultsPrefBranch.clearUserPref(featureId);
       } catch (e) {}

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

@@ -685,3 +685,56 @@ add_task(async function test_remote_defaults_no_bucketConfig() {
   sandbox.restore();
   await rsClient.db.clear();
 });
+
+add_task(async function remote_defaults_variables_storage() {
+  let barFeature = new ExperimentFeature("bar", {
+    bar: { description: "mochitest" },
+  });
+  let remoteDefaults = [
+    {
+      id: "bar",
+      description: "test pref storage and types",
+      configurations: [
+        {
+          slug: "a",
+          isEarlyStartup: true,
+          variables: {
+            storage: 42,
+            object: { foo: "foo" },
+            string: "string",
+            bool: true,
+          },
+          enabled: true,
+          targeting: "true",
+        },
+      ],
+    },
+  ];
+
+  await setup(remoteDefaults);
+  await RemoteDefaultsLoader.syncRemoteDefaults("mochitest");
+  await barFeature.ready();
+
+  Assert.ok(
+    Services.prefs.getIntPref(`${SYNC_DEFAULTS_PREF_BRANCH}bar.storage`, 0),
+    "Stores variable in separate pref"
+  );
+  Assert.equal(
+    Services.prefs.getIntPref(`${SYNC_DEFAULTS_PREF_BRANCH}bar.storage`, 0),
+    42,
+    "Stores variable in correct type"
+  );
+  Assert.deepEqual(
+    barFeature.getRemoteConfig().variables,
+    remoteDefaults[0].configurations[0].variables,
+    "Test types are returned correctly"
+  );
+
+  ExperimentAPI._store._deleteForTests("bar");
+
+  Assert.equal(
+    Services.prefs.getIntPref(`${SYNC_DEFAULTS_PREF_BRANCH}bar.storage`, -1),
+    -1,
+    "Variable pref is cleared"
+  );
+});

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

@@ -120,6 +120,11 @@ add_task(async function has_sync_value_before_ready() {
     JSON.stringify(REMOTE_CONFIGURATION.configurations[0])
   );
 
+  Services.prefs.setBoolPref(
+    "nimbus.syncdefaultsstore.aboutwelcome.remoteValue",
+    REMOTE_CONFIGURATION.configurations[0].variables.remoteValue
+  );
+
   Assert.equal(
     feature.getValue().remoteValue,
     REMOTE_CONFIGURATION.configurations[0].variables.remoteValue,

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

@@ -6,6 +6,9 @@ const { ExperimentFakes } = ChromeUtils.import(
 const { ExperimentStore } = ChromeUtils.import(
   "resource://nimbus/lib/ExperimentStore.jsm"
 );
+const { FeatureManifest } = ChromeUtils.import(
+  "resource://nimbus/FeatureManifest.js"
+);
 
 const { SYNC_DATA_PREF_BRANCH, SYNC_DEFAULTS_PREF_BRANCH } = ExperimentStore;
 const { cleanupStorePrefCache } = ExperimentFakes;
@@ -262,7 +265,11 @@ add_task(async function test_sync_access_update() {
   store.updateExperiment("foo", {
     branch: {
       ...experiment.branch,
-      feature: { featureId: "aboutwelcome", enabled: true, value: "bar" },
+      feature: {
+        featureId: "aboutwelcome",
+        enabled: true,
+        value: { bar: "bar" },
+      },
     },
   });
 
@@ -270,9 +277,9 @@ add_task(async function test_sync_access_update() {
   let cachedExperiment = store.getExperimentForFeature("aboutwelcome");
 
   Assert.ok(cachedExperiment, "Got back 1 experiment");
-  Assert.equal(
+  Assert.deepEqual(
     cachedExperiment.branch.feature.value,
-    "bar",
+    { bar: "bar" },
     "Got updated value"
   );
 });
@@ -519,3 +526,162 @@ add_task(async function test_getAllExistingRemoteConfigIds() {
 
   cleanupStorePrefCache();
 });
+
+add_task(async function test_storeValuePerPref_noVariables() {
+  const store = ExperimentFakes.store();
+  const experiment = ExperimentFakes.experiment("foo", {
+    branch: {
+      slug: "variant",
+      feature: {
+        // Ensure it gets saved to prefs
+        isEarlyStartup: true,
+        featureId: "purple",
+        enabled: true,
+      },
+    },
+  });
+
+  await store.init();
+  store.addExperiment(experiment);
+
+  let branch = Services.prefs.getBranch(`${SYNC_DATA_PREF_BRANCH}purple.`);
+
+  Assert.ok(
+    Services.prefs.getStringPref(`${SYNC_DATA_PREF_BRANCH}purple`, ""),
+    "Experiment metadata saved to prefs"
+  );
+
+  Assert.equal(branch.getChildList("").length, 0, "No variables to store");
+
+  store._updateSyncStore({ ...experiment, active: false });
+  Assert.ok(
+    !Services.prefs.getStringPref(`${SYNC_DATA_PREF_BRANCH}purple`, ""),
+    "Experiment cleanup"
+  );
+});
+
+add_task(async function test_storeValuePerPref_withVariables() {
+  const store = ExperimentFakes.store();
+  const experiment = ExperimentFakes.experiment("foo", {
+    branch: {
+      slug: "variant",
+      feature: {
+        // Ensure it gets saved to prefs
+        isEarlyStartup: true,
+        featureId: "purple",
+        value: { color: "purple", enabled: true },
+      },
+    },
+  });
+
+  await store.init();
+  store.addExperiment(experiment);
+
+  let branch = Services.prefs.getBranch(`${SYNC_DATA_PREF_BRANCH}purple.`);
+
+  Assert.equal(
+    Services.prefs
+      .getStringPref(`${SYNC_DATA_PREF_BRANCH}purple`)
+      .indexOf("color"),
+    -1,
+    "Experiment metadata does not contain variables"
+  );
+
+  Assert.equal(branch.getChildList("").length, 2, "Enabled and color");
+
+  store._updateSyncStore({ ...experiment, active: false });
+  Assert.ok(
+    !Services.prefs.getStringPref(`${SYNC_DATA_PREF_BRANCH}purple`, ""),
+    "Experiment cleanup"
+  );
+  Assert.equal(branch.getChildList("").length, 0, "Variables are also removed");
+});
+
+add_task(async function test_storeValuePerPref_returnsSameValue() {
+  let store = ExperimentFakes.store();
+  const experiment = ExperimentFakes.experiment("foo", {
+    branch: {
+      slug: "variant",
+      feature: {
+        // Ensure it gets saved to prefs
+        isEarlyStartup: true,
+        featureId: "purple",
+        value: { color: "purple", enabled: true },
+      },
+    },
+  });
+
+  await store.init();
+  store.addExperiment(experiment);
+  let branch = Services.prefs.getBranch(`${SYNC_DATA_PREF_BRANCH}purple.`);
+
+  store = ExperimentFakes.store();
+  Assert.deepEqual(
+    store.getExperimentForFeature("purple"),
+    experiment,
+    "Returns the same value"
+  );
+
+  // Cleanup
+  store._updateSyncStore({ ...experiment, active: false });
+  Assert.ok(
+    !Services.prefs.getStringPref(`${SYNC_DATA_PREF_BRANCH}purple`, ""),
+    "Experiment cleanup"
+  );
+  Assert.deepEqual(branch.getChildList(""), [], "Variables are also removed");
+});
+
+add_task(async function test_storeValuePerPref_returnsSameValue_allTypes() {
+  let store = ExperimentFakes.store();
+  // Add a fake feature that matches the variables we're testing
+  FeatureManifest.purple = {
+    variables: {
+      string: { type: "string" },
+      bool: { type: "boolean" },
+      array: { type: "json" },
+      number1: { type: "int" },
+      number2: { type: "int" },
+      number3: { type: "int" },
+      json: { type: "json" },
+    },
+  };
+  const experiment = ExperimentFakes.experiment("foo", {
+    branch: {
+      slug: "variant",
+      feature: {
+        // Ensure it gets saved to prefs
+        isEarlyStartup: true,
+        featureId: "purple",
+        value: {
+          string: "string",
+          bool: true,
+          array: [1, 2, 3],
+          number1: 42,
+          number2: 0,
+          number3: -5,
+          json: { jsonValue: true },
+        },
+      },
+    },
+  });
+
+  await store.init();
+  store.addExperiment(experiment);
+  let branch = Services.prefs.getBranch(`${SYNC_DATA_PREF_BRANCH}purple.`);
+
+  store = ExperimentFakes.store();
+  Assert.deepEqual(
+    store.getExperimentForFeature("purple").branch.feature.value,
+    experiment.branch.feature.value,
+    "Returns the same value"
+  );
+
+  // Cleanup
+  store._updateSyncStore({ ...experiment, active: false });
+  Assert.ok(
+    !Services.prefs.getStringPref(`${SYNC_DATA_PREF_BRANCH}purple`, ""),
+    "Experiment cleanup"
+  );
+  Assert.deepEqual(branch.getChildList(""), [], "Variables are also removed");
+  delete FeatureManifest.purple;
+});