Bug 1727349 - Drop _getWholeStore API from Store impls

CHANGELOG.md

@@ -12,6 +12,8 @@
 * [#658](https://github.com/mozilla/glean.js/pull/658): BUGFIX: Unblock ping uploading jobs after the maximum of upload failures are hit for a given uploading window.
 * [#661](https://github.com/mozilla/glean.js/pull/661): Include unminified version of library on Qt/QML builds.
 * [#647](https://github.com/mozilla/glean.js/pull/647): Implement the Text metric type.
+* [#681](https://github.com/mozilla/glean.js/pull/681): BUGFIX: Fix error in scanning events database upon initialization on Qt/QML.
+  * This bug prevents the changes introduced in [#526](https://github.com/mozilla/glean.js/pull/526) from working properly.
 
 # v0.18.1 (2021-07-22)
 

glean/src/core/pings/database.ts

@@ -153,15 +153,17 @@ class PingsDatabase {
    * @returns List of all currently stored pings in ascending order by date.
    */
   async getAllPings(): Promise<[ string, PingInternalRepresentation ][]> {
-    const allStoredPings = await this.store._getWholeStore();
+    const allStoredPings = await this.store.get();
     const finalPings: { [ident: string]: PingInternalRepresentation } = {};
-    for (const identifier in allStoredPings) {
-      const ping = allStoredPings[identifier];
-      if (isValidPingInternalRepresentation(ping)) {
-        finalPings[identifier] = ping;
-      } else {
-        log(LOG_TAG, "Unexpected data found in pings database. Deleting.", LoggingLevel.Warn);
-        await this.store.delete([identifier]);
+    if (isObject(allStoredPings)) {
+      for (const identifier in allStoredPings) {
+        const ping = allStoredPings[identifier];
+        if (isValidPingInternalRepresentation(ping)) {
+          finalPings[identifier] = ping;
+        } else {
+          log(LOG_TAG, "Unexpected data found in pings database. Deleting.", LoggingLevel.Warn);
+          await this.store.delete([identifier]);
+        }
       }
     }
 

glean/src/core/storage/index.ts

@@ -2,7 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-import type { JSONObject, JSONValue } from "../utils.js";
+import type { JSONValue } from "../utils.js";
 
 /**
  * The storage index in the ordered list of keys to navigate on the store
@@ -26,25 +26,16 @@ import type { JSONObject, JSONValue } from "../utils.js";
 export type StorageIndex = string[];
 
 interface Store {
-  /**
-   * Test-only API**
-   *
-   * Gets the whole store.
-   *
-   * @returns An object representing all the data recorded in the store.
-   */
-  _getWholeStore(): Promise<JSONObject>;
-
   /**
    * Gets the value recorded to the given index on the store.
    *
-   * @param index The index  of the entry to get.
+   * @param index The index  of the entry to get. If the index is empty,
+   *        the whole store is returned.
    * @returns The value found for the given index on the storage.
    *          In case nothing has been recorded on the given index, returns `undefined`.
-   * @throws - In case the index is an empty array.
-   *         - In case a value that is not `string` or `object` is found.
+   * @throws In case an value which is not valid JSON is found.
    */
-  get(index: StorageIndex): Promise<JSONValue | undefined>;
+  get(index?: StorageIndex): Promise<JSONValue | undefined>;
 
   /**
    * Updates a specific entry from the store.

glean/src/platform/qt/storage.ts

@@ -64,8 +64,8 @@ function getKeyValueArrayFromNestedObject(
 function queryResultToJSONObject(
   queryResult: LocalStorage.QueryResult | undefined
 ): JSONObject | undefined {
-  if (!queryResult) {
-    return {};
+  if (!queryResult || queryResult.rows.length === 0) {
+    return;
   }
 
   const obj: JSONObject = {};
@@ -95,12 +95,14 @@ function queryResultToJSONObject(
       target[index[index.length - 1]] = item.value;
     }
   }
+
   return obj;
 }
 
 class QMLStore implements Store {
   protected initialized: Promise<unknown>;
   private dbHandle?: LocalStorage.DatabaseHandle;
+  private logTag: string;
 
   constructor(
     private tableName: string,
@@ -109,6 +111,7 @@ class QMLStore implements Store {
     this.initialized = this._executeQuery(
       `CREATE TABLE IF NOT EXISTS ${tableName}(key VARCHAR(255), value VARCHAR(255));`
     );
+    this.logTag = `${LOG_TAG}.${tableName}`;
   }
 
   private _createKeyFromIndex(index: StorageIndex) {
@@ -128,7 +131,7 @@ class QMLStore implements Store {
       this.dbHandle = handle;
     } catch(e) {
       log(
-        LOG_TAG,
+        this.logTag,
         ["Error while attempting to access LocalStorage.\n", JSON.stringify(e)],
         LoggingLevel.Debug
       );
@@ -151,7 +154,7 @@ class QMLStore implements Store {
         });
       } catch (e) {
         log(
-          LOG_TAG,
+          this.logTag,
           [`Error executing LocalStorage query: ${query}.\n`, JSON.stringify(e)],
           LoggingLevel.Debug
         );
@@ -173,17 +176,21 @@ class QMLStore implements Store {
     return queryResultToJSONObject(result);
   }
 
-  async _getWholeStore(): Promise<JSONObject> {
+  private async _getWholeStore(): Promise<JSONObject | undefined> {
     const result = await this._executeOnceInitialized(`SELECT * FROM ${this.tableName}`);
-    return queryResultToJSONObject(result) || {};
+    return queryResultToJSONObject(result);
   }
 
-  async get(index: StorageIndex): Promise<JSONValue | undefined> {
+  async get(index: StorageIndex = []): Promise<JSONValue | undefined> {
+    if (index.length === 0) {
+      return this._getWholeStore();
+    }
+
     const obj = (await this._getFullResultObject(index)) || {};
     try {
       return getValueFromNestedObject(obj, index);
     } catch(e) {
-      log(LOG_TAG, [
+      log(this.logTag, [
         "Error getting value from database.",
         JSON.stringify((e as Error).message)
       ]);

glean/src/platform/test/storage.ts

@@ -27,12 +27,7 @@ class MockStore implements Store {
     this.rootKey = rootKey;
   }
 
-  _getWholeStore(): Promise<JSONObject> {
-    const result: JSONObject = (globalStore[this.rootKey] as JSONObject) || {};
-    return Promise.resolve(result);
-  }
-
-  get(index: StorageIndex): Promise<JSONValue | undefined> {
+  get(index: StorageIndex = []): Promise<JSONValue | undefined> {
     try {
       const value = getValueFromNestedObject(globalStore, [ this.rootKey, ...index ]);
       return Promise.resolve(value);

glean/src/platform/webext/storage.ts

@@ -52,6 +52,7 @@ function stripNulls(query: WebExtStoreQuery) {
  */
 class WebExtStore implements Store {
   private store;
+  private logTag: string;
 
   // The main key under which all other entries will be recorded for this store instance.
   private rootKey: string;
@@ -66,9 +67,10 @@ class WebExtStore implements Store {
     }
     this.store = browser.storage.local;
     this.rootKey = rootKey;
+    this.logTag = `${LOG_TAG}.${rootKey}`;
   }
 
-  async _getWholeStore(): Promise<JSONObject> {
+  private async _getWholeStore(): Promise<JSONObject> {
     const result = await this.store.get({ [this.rootKey]: {} });
     return result[this.rootKey];
   }
@@ -81,7 +83,7 @@ class WebExtStore implements Store {
    */
   private _buildQuery(index: StorageIndex): WebExtStoreQuery {
     let query = null;
-    for (const key of [ this.rootKey, ...index].reverse()) {
+    for (const key of [ this.rootKey, ...index ].reverse()) {
       query = { [key]: query };
     }
 
@@ -99,7 +101,7 @@ class WebExtStore implements Store {
     return { [this.rootKey]: transformFn(store) };
   }
 
-  async get(index: StorageIndex): Promise<JSONValue | undefined> {
+  async get(index: StorageIndex = []): Promise<JSONValue | undefined> {
     const query = this._buildQuery(index);
     const response = await this.store.get(query);
     stripNulls(response);
@@ -116,7 +118,7 @@ class WebExtStore implements Store {
     }
 
     log(
-      LOG_TAG,
+      this.logTag,
       [
         `Unexpected value found in storage for index ${JSON.stringify(index)}. Ignoring.
         ${JSON.stringify(response, null, 2)}`
@@ -153,7 +155,7 @@ class WebExtStore implements Store {
       );
       return this.store.set(query);
     } catch(e) {
-      log(LOG_TAG, ["Ignoring key", JSON.stringify(e)], LoggingLevel.Warn);
+      log(this.logTag, ["Ignoring key", JSON.stringify(e)], LoggingLevel.Warn);
     }
   }
 }

glean/tests/unit/core/metrics/database.spec.ts

@@ -398,8 +398,8 @@ describe("MetricsDatabase", function() {
       await db.record(appMetric, new StringMetric("appValue"));
 
       await db.getPingMetrics("aPing", true);
-      assert.notDeepStrictEqual(await db["userStore"]._getWholeStore(), {});
-      assert.deepStrictEqual(await db["pingStore"]._getWholeStore(), {
+      assert.notDeepStrictEqual(await db["userStore"].get(), {});
+      assert.deepStrictEqual(await db["pingStore"].get(), {
         "twoPing": {
           "string": {
             "ping.metric": "pingValue"
@@ -411,7 +411,7 @@ describe("MetricsDatabase", function() {
           }
         }
       });
-      assert.notDeepStrictEqual(await db["appStore"]._getWholeStore(), {});
+      assert.notDeepStrictEqual(await db["appStore"].get(), {});
     });
 
     it("reserved metrics are not added to snapshot", async function() {
@@ -472,9 +472,9 @@ describe("MetricsDatabase", function() {
       await db.record(appMetric, new StringMetric("appValue"));
 
       await db.clearAll();
-      assert.deepStrictEqual(await db["userStore"]._getWholeStore(), {});
-      assert.deepStrictEqual(await db["pingStore"]._getWholeStore(), {});
-      assert.deepStrictEqual(await db["appStore"]._getWholeStore(), {});
+      assert.deepStrictEqual(await db["userStore"].get(), undefined);
+      assert.deepStrictEqual(await db["pingStore"].get(), undefined);
+      assert.deepStrictEqual(await db["appStore"].get(), undefined);
     });
 
     it("clears separate stores correctly", async function() {
@@ -507,9 +507,9 @@ describe("MetricsDatabase", function() {
       await db.record(appMetric, new StringMetric("appValue"));
 
       await db.clear(Lifetime.User);
-      assert.deepStrictEqual(await db["userStore"]._getWholeStore(), {});
-      assert.notDeepStrictEqual(await db["pingStore"]._getWholeStore(), {});
-      assert.notDeepStrictEqual(await db["appStore"]._getWholeStore(), {});
+      assert.deepStrictEqual(await db["userStore"].get(), undefined);
+      assert.notDeepStrictEqual(await db["pingStore"].get(), undefined);
+      assert.notDeepStrictEqual(await db["appStore"].get(), undefined);
     });
 
     it("clears data from specific ping when specified", async function () {

glean/tests/unit/core/metrics/events_database.spec.ts

@@ -331,7 +331,7 @@ describe("EventsDatabase", function() {
     // Clear any events from previous tests.
     const rawStorage = new Glean.platform.Storage("events");
     await rawStorage.delete([]);
-    assert.deepStrictEqual({}, await rawStorage._getWholeStore());
+    assert.deepStrictEqual(await rawStorage.get(), undefined);
 
     // Initialize the database and inject some events.
     const db = new EventsDatabase(Glean.platform.Storage);

glean/tests/unit/core/metrics/labeled.spec.ts

@@ -61,7 +61,7 @@ describe("LabeledMetric", function() {
     ping.submit();
     await Context.dispatcher.testBlockOnQueue();
 
-    const storedPings = await Context.pingsDatabase["store"]._getWholeStore();
+    const storedPings = await Context.pingsDatabase["store"].get() as JSONObject;
     assert.strictEqual(Object.keys(storedPings).length, 1);
 
     // TODO: bug 1682282 will validate the payload schema.
@@ -114,7 +114,7 @@ describe("LabeledMetric", function() {
     ping.submit();
     await Context.dispatcher.testBlockOnQueue();
 
-    const storedPings = await Context.pingsDatabase["store"]._getWholeStore();
+    const storedPings = await Context.pingsDatabase["store"].get() as JSONObject;
     assert.strictEqual(Object.keys(storedPings).length, 1);
 
     // TODO: bug 1682282 will validate the payload schema.
@@ -291,7 +291,7 @@ describe("LabeledMetric", function() {
     ping.submit();
     await Context.dispatcher.testBlockOnQueue();
 
-    const storedPings = await Context.pingsDatabase["store"]._getWholeStore();
+    const storedPings = await Context.pingsDatabase["store"].get() as JSONObject;
     assert.strictEqual(Object.keys(storedPings).length, 1);
 
     // TODO: bug 1682282 will validate the payload schema.
@@ -339,7 +339,7 @@ describe("LabeledMetric", function() {
     ping.submit();
     await Context.dispatcher.testBlockOnQueue();
 
-    const storedPings = await Context.pingsDatabase["store"]._getWholeStore();
+    const storedPings = await Context.pingsDatabase["store"].get() as JSONObject;
     assert.strictEqual(Object.keys(storedPings).length, 1);
 
     // TODO: bug 1682282 will validate the payload schema.

glean/tests/unit/core/pings/database.spec.ts

@@ -9,6 +9,7 @@ import sinon from "sinon";
 import type { Observer} from "../../../../src/core/pings/database";
 import Database, { isValidPingInternalRepresentation } from "../../../../src/core/pings/database";
 import Glean from "../../../../src/core/glean";
+import type { JSONObject } from "../../../../src/core/utils";
 
 const sandbox = sinon.createSandbox();
 const now = new Date();
@@ -54,7 +55,7 @@ describe("PingsDatabase", function() {
         collectionDate: now.toISOString(), path, payload, headers
       });
 
-      assert.strictEqual(Object.keys(await db["store"]._getWholeStore()).length, 2);
+      assert.strictEqual(Object.keys(await db["store"].get() as JSONObject).length, 2);
     });
 
     it("observer is notified when a new ping is added to the database", async function() {
@@ -379,7 +380,7 @@ describe("PingsDatabase", function() {
       }
 
       await db.clearAll();
-      assert.strictEqual(Object.keys(await db["store"]._getWholeStore()).length, 0);
+      assert.strictEqual(Object.keys(await db["store"].get() || {}).length, 0);
     });
   });
 

glean/tests/unit/core/pings/ping_type.spec.ts

@@ -11,6 +11,7 @@ import { Lifetime } from "../../../../src/core/metrics/lifetime";
 import Glean from "../../../../src/core/glean";
 import { Context } from "../../../../src/core/context";
 import { stopGleanUploader } from "../../../utils";
+import type { JSONObject } from "../../../../src/core/utils";
 
 const sandbox = sinon.createSandbox();
 
@@ -55,7 +56,7 @@ describe("PingType", function() {
     counter.add();
 
     await submitSync(ping);
-    const storedPings = await Context.pingsDatabase["store"]._getWholeStore();
+    const storedPings = await Context.pingsDatabase["store"].get() as JSONObject;
     assert.strictEqual(Object.keys(storedPings).length, 1);
   });
 
@@ -75,11 +76,11 @@ describe("PingType", function() {
     });
 
     await submitSync(ping1);
-    let storedPings = await Context.pingsDatabase["store"]._getWholeStore();
-    assert.strictEqual(Object.keys(storedPings).length, 0);
+    let storedPings = await Context.pingsDatabase["store"].get();
+    assert.strictEqual(storedPings, undefined);
 
     await submitSync(ping2);
-    storedPings = await Context.pingsDatabase["store"]._getWholeStore();
+    storedPings = await Context.pingsDatabase["store"].get() as JSONObject;
     assert.strictEqual(Object.keys(storedPings).length, 1);
   });
 
@@ -92,8 +93,8 @@ describe("PingType", function() {
       sendIfEmpty: false,
     });
     await submitSync(ping);
-    const storedPings = await Context.pingsDatabase["store"]._getWholeStore();
-    assert.strictEqual(Object.keys(storedPings).length, 0);
+    const storedPings = await Context.pingsDatabase["store"].get();
+    assert.strictEqual(storedPings, undefined);
   });
 
   it("no pings are submitted if Glean has not been initialized", async function() {
@@ -105,8 +106,8 @@ describe("PingType", function() {
       sendIfEmpty: false,
     });
     await submitSync(ping);
-    const storedPings = await Context.pingsDatabase["store"]._getWholeStore();
-    assert.strictEqual(Object.keys(storedPings).length, 0);
+    const storedPings = await Context.pingsDatabase["store"].get();
+    assert.strictEqual(storedPings, undefined);
   });
 
   it("runs a validator with no metrics tests", async function() {