Bug 1772136 - Add option to throw if .get() fails to obtain records r=gbeckley,barret

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

browser/components/newtab/test/browser/browser_asrouter_experimentsAPILoader.js

@@ -174,6 +174,8 @@ const client = RemoteSettings("nimbus-desktop-experiments");
 // no `add_task` because we want to run this setup before each test not before
 // the entire test suite.
 async function setup(experiment) {
+  // Store the experiment in RS local db to bypass synchronization.
+  await client.db.importChanges({}, Date.now(), [experiment], { clear: true });
   await SpecialPowers.pushPrefEnv({
     set: [
       ["app.shield.optoutstudies.enabled", true],
@@ -184,8 +186,6 @@ async function setup(experiment) {
       ],
     ],
   });
-
-  await client.db.importChanges({}, Date.now(), [experiment], { clear: true });
 }
 
 async function cleanup() {

services/settings/RemoteSettingsClient.jsm

@@ -393,13 +393,14 @@ class RemoteSettingsClient extends EventEmitter {
   /**
    * Lists settings.
    *
-   * @param  {Object} options                  The options object.
-   * @param  {Object} options.filters          Filter the results (default: `{}`).
-   * @param  {String} options.order            The order to apply (eg. `"-last_modified"`).
-   * @param  {boolean} options.dumpFallback    Fallback to dump data if read of local DB fails (default: `true`).
-   * @param  {boolean} options.loadDumpIfNewer Use dump data if it is newer than local data (default: `true`).
-   * @param  {boolean} options.syncIfEmpty     Synchronize from server if local data is empty (default: `true`).
-   * @param  {boolean} options.verifySignature Verify the signature of the local data (default: `false`).
+   * @param  {Object} options                    The options object.
+   * @param  {Object} options.filters            Filter the results (default: `{}`).
+   * @param  {String} options.order              The order to apply (eg. `"-last_modified"`).
+   * @param  {boolean} options.dumpFallback      Fallback to dump data if read of local DB fails (default: `true`).
+   * @param  {boolean} options.emptyListFallback Fallback to empty list if no dump data and read of local DB fails (default: `true`).
+   * @param  {boolean} options.loadDumpIfNewer   Use dump data if it is newer than local data (default: `true`).
+   * @param  {boolean} options.syncIfEmpty       Synchronize from server if local data is empty (default: `true`).
+   * @param  {boolean} options.verifySignature   Verify the signature of the local data (default: `false`).
    * @return {Promise}
    */
   async get(options = {}) {
@@ -407,11 +408,13 @@ class RemoteSettingsClient extends EventEmitter {
       filters = {},
       order = "", // not sorted by default.
       dumpFallback = true,
+      emptyListFallback = true,
       loadDumpIfNewer = true,
       syncIfEmpty = true,
     } = options;
     let { verifySignature = false } = options;
 
+    const hasParallelCall = !!this._importingPromise;
     let data;
     try {
       let lastModified = await this.db.getLastModified();
@@ -478,8 +481,12 @@ class RemoteSettingsClient extends EventEmitter {
             verifySignature = false;
           }
         } catch (e) {
+          if (!hasParallelCall) {
+            // Sync or load dump failed. Throw.
+            throw e;
+          }
           // Report error, but continue because there could have been data
-          // loaded from a parrallel call.
+          // loaded from a parallel call.
           Cu.reportError(e);
         } finally {
           // then delete this promise again, as now we should have local data:
@@ -491,7 +498,7 @@ class RemoteSettingsClient extends EventEmitter {
       data = await this.db.list({ filters, order });
     } catch (e) {
       // If the local DB cannot be read (for unknown reasons, Bug 1649393)
-      // We fallback to the packaged data, and filter/sort in memory.
+      // or sync failed, we fallback to the packaged data, and filter/sort in memory.
       if (!dumpFallback) {
         throw e;
       }
@@ -502,11 +509,15 @@ class RemoteSettingsClient extends EventEmitter {
       );
       if (data !== null) {
         lazy.console.info(`${this.identifier} falling back to JSON dump`);
-      } else {
+      } else if (emptyListFallback) {
         lazy.console.info(
           `${this.identifier} no dump fallback, return empty list`
         );
         data = [];
+      } else {
+        // Obtaining the records failed, there is no dump, and we don't fallback
+        // to an empty list. Throw the original error.
+        throw e;
       }
       if (!lazy.ObjectUtils.isEmpty(filters)) {
         data = data.filter(r => lazy.Utils.filterObject(filters, r));

services/settings/docs/index.rst

@@ -73,6 +73,9 @@ Options
 * ``verifySignature``: verify the content signature of the local data (default: ``false``).
   An error is thrown if the local data was altered. This hurts performance, but can be used if your use case needs to be secure from local tampering.
 
+* ``emptyListFallback``: return an empty list if obtaining the records fails (default: ``true``).
+  If an error occurs during the reading of local data, or during synchronization, then an empty list is returned.
+
 
 Events
 ------

services/settings/test/unit/test_remote_settings.js

@@ -379,6 +379,23 @@ add_task(async function test_get_falls_back_to_dump_if_db_fails_later() {
 });
 add_task(clear_state);
 
+add_task(async function test_get_falls_back_to_dump_if_network_fails() {
+  if (IS_ANDROID) {
+    // Skip test: we don't ship remote settings dumps on Android (see package-manifest).
+    return;
+  }
+  const backup = clientWithDump.sync;
+  clientWithDump.sync = () => {
+    throw new Error("Sync error");
+  };
+
+  const records = await clientWithDump.get();
+  ok(records.length > 0, "dump content is returned");
+
+  clientWithDump.sync = backup;
+});
+add_task(clear_state);
+
 add_task(async function test_get_does_not_sync_if_empty_dump_is_provided() {
   if (IS_ANDROID) {
     // Skip test: we don't ship remote settings dumps on Android (see package-manifest).
@@ -415,7 +432,7 @@ add_task(
 );
 add_task(clear_state);
 
-add_task(async function test_get_ignores_synchronization_errors() {
+add_task(async function test_get_ignores_synchronization_errors_by_default() {
   // The monitor endpoint won't contain any information about this collection.
   let data = await RemoteSettings("some-unknown-key").get();
   equal(data.length, 0);
@@ -425,6 +442,22 @@ add_task(async function test_get_ignores_synchronization_errors() {
 });
 add_task(clear_state);
 
+add_task(async function test_get_throws_if_no_empty_fallback() {
+  // The monitor endpoint won't contain any information about this collection.
+  try {
+    await RemoteSettings("some-unknown-key").get({
+      emptyListFallback: false,
+    });
+    Assert.ok(false, ".get() should throw");
+  } catch (error) {
+    Assert.ok(
+      error.message.includes("Response from server unparseable"),
+      "Server error was thrown"
+    );
+  }
+});
+add_task(clear_state);
+
 add_task(async function test_get_verify_signature_no_sync() {
   // No signature in metadata, and no sync if empty.
   let error;

toolkit/components/nimbus/ExperimentAPI.jsm

@@ -279,6 +279,8 @@ const ExperimentAPI = {
         filters: { slug },
       });
     } catch (e) {
+      // If an error occurs in .get(), an empty list is returned and the destructuring
+      // assignment will throw.
       Cu.reportError(e);
       recipe = undefined;
     }

toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.jsm

@@ -205,7 +205,10 @@ class _RemoteSettingsExperimentLoader {
     let loadingError = false;
 
     try {
-      recipes = await this.remoteSettingsClient.get();
+      recipes = await this.remoteSettingsClient.get({
+        // Throw instead of returning an empty list.
+        emptyListFallback: false,
+      });
       lazy.log.debug(`Got ${recipes.length} recipes from Remote Settings`);
     } catch (e) {
       lazy.log.debug("Error getting recipes from remote settings.");
@@ -296,7 +299,10 @@ class _RemoteSettingsExperimentLoader {
     try {
       recipes = await lazy
         .RemoteSettings(collection || lazy.COLLECTION_ID)
-        .get();
+        .get({
+          // Throw instead of returning an empty list.
+          emptyListFallback: false,
+        });
     } catch (e) {
       Cu.reportError(e);
       throw new Error("Error getting recipes from remote settings.");

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

@@ -19,6 +19,9 @@ const { ExperimentFakes } = ChromeUtils.import(
 let rsClient;
 
 add_setup(async function() {
+  rsClient = RemoteSettings("nimbus-desktop-experiments");
+  await rsClient.db.importChanges({}, Date.now(), [], { clear: true });
+
   await SpecialPowers.pushPrefEnv({
     set: [
       ["messaging-system.log", "all"],
@@ -26,7 +29,6 @@ add_setup(async function() {
       ["app.shield.optoutstudies.enabled", true],
     ],
   });
-  rsClient = RemoteSettings("nimbus-desktop-experiments");
 
   registerCleanupFunction(async () => {
     await SpecialPowers.popPrefEnv();

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

@@ -112,7 +112,9 @@ async function setup(configuration) {
     }
   );
 
-  const cleanup = () => client.db.clear();
+  // Simulate a state where no experiment exists.
+  const cleanup = () =>
+    client.db.importChanges({}, Date.now(), [], { clear: true });
   return { client, cleanup };
 }
 
@@ -148,7 +150,7 @@ add_task(async function test_remote_fetch_and_ready() {
 
   await ExperimentAPI.ready();
 
-  let { client: rsClient, cleanup } = await setup();
+  let { cleanup } = await setup();
 
   // Fake being initialized so we can update recipes
   // we don't need to start any timers
@@ -224,7 +226,7 @@ add_task(async function test_remote_fetch_and_ready() {
   Assert.equal(barInstance.getVariable("remoteValue"), 3, "Has rollout value");
 
   // Clear RS db and load again. No configurations so should clear the cache.
-  await rsClient.db.clear();
+  await cleanup();
   await RemoteSettingsExperimentLoader.updateRecipes(
     "browser_rsel_remote_defaults"
   );

toolkit/components/normandy/lib/RecipeRunner.jsm

@@ -333,7 +333,10 @@ var RecipeRunner = {
       // Fetch recipes before execution in case we fail and exit early.
       let recipesAndSignatures;
       try {
-        recipesAndSignatures = await lazy.gRemoteSettingsClient.get();
+        recipesAndSignatures = await lazy.gRemoteSettingsClient.get({
+          // Do not return an empty list if an error occurs.
+          emptyListFallback: false,
+        });
       } catch (e) {
         await lazy.Uptake.reportRunner(lazy.Uptake.RUNNER_SERVER_ERROR);
         return;