Merge pull request #174 from Dexterp37/upload_circular_deps

Bug 1701578 - Remove a circular dependency in the upload module

Author: Dexterp37

glean/src/core/glean.ts

@@ -225,17 +225,16 @@ class Glean {
     // The configuration constructor will throw in case config has any incorrect prop.
     const correctConfig = new Configuration(config);
 
-    // Initialize the ping uploader with either the platform defaults or a custom
-    // provided uploader from the configuration object.
-    Glean.instance._pingUploader =
-      new PingUploader(correctConfig.httpClient ? correctConfig.httpClient : Glean.platform.uploader);
-
     Glean.instance._db = {
       metrics: new MetricsDatabase(Glean.platform.Storage),
       events: new EventsDatabase(Glean.platform.Storage),
-      pings: new PingsDatabase(Glean.platform.Storage, Glean.pingUploader)
+      pings: new PingsDatabase(Glean.platform.Storage)
     };
 
+    Glean.instance._pingUploader = new PingUploader(correctConfig, Glean.platform, Glean.pingsDatabase);
+
+    Glean.pingsDatabase.attachObserver(Glean.pingUploader);
+
     if (config?.plugins) {
       for (const plugin of config.plugins) {
         registerPluginToEvent(plugin);
@@ -264,6 +263,7 @@ class Glean {
       // This is fine, we are inside a dispatched task that is guaranteed to run before any
       // other task. No external API call will be executed before we leave this task.
       Glean.instance._initialized = true;
+      Glean.pingUploader.setInitialized(true);
 
       // The upload enabled flag may have changed since the last run, for
       // example by the changing of a config file.
@@ -295,7 +295,7 @@ class Glean {
         }
       }
 
-      await Glean.pingUploader.scanPendingPings();
+      await Glean.pingsDatabase.scanPendingPings();
 
       // Even though this returns a promise, there is no need to block on it returning.
       //

glean/src/core/pings/database.ts

@@ -64,8 +64,16 @@ class PingsDatabase {
   private store: Store;
   private observer?: Observer;
 
-  constructor(store: StorageBuilder, observer?: Observer) {
+  constructor(store: StorageBuilder) {
     this.store = new store("pings");
+  }
+  
+  /**
+   * Attach an observer that reacts to the pings storage changes.
+   *
+   * @param observer The new observer to attach. 
+   */
+  attachObserver(observer: Observer): void {
     this.observer = observer;
   }
 
@@ -128,6 +136,22 @@ class PingsDatabase {
     return finalPings;
   }
 
+  /**
+   * Scans the database for pending pings and enqueues them.
+   */
+  async scanPendingPings(): Promise<void> {
+    // If there's no observer, then there's no point in iterating.
+    if (!this.observer) {
+      return;
+    }
+
+    const pings = await this.getAllPings();
+    for (const identifier in pings) {
+      // Notify the observer that a new ping has been added to the pings database.
+      this.observer.update(identifier, pings[identifier]);
+    }
+  }
+
   /**
    * Clears all the pings from the database.
    */

glean/src/core/upload/index.ts

@@ -2,9 +2,12 @@
  * 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 Platform from "../../platform/index.js";
+import { Configuration } from "../config.js";
 import { GLEAN_VERSION } from "../constants.js";
 import { Observer as PingsDatabaseObserver, PingInternalRepresentation } from "../pings/database.js";
-import Glean from "../glean.js";
+import type PingsDatabase from "../pings/database.js";
+import PlatformInfo from "../platform_info.js";
 import Uploader, { UploadResult, UploadResultStatus } from "./uploader.js";
 
 interface QueuedPing extends PingInternalRepresentation {
@@ -45,10 +48,34 @@ class PingUploader implements PingsDatabaseObserver {
   // The object that concretely handles the ping transmission.
   private readonly uploader: Uploader;
 
-  constructor(uploader: Uploader) {
+  private readonly platformInfo: PlatformInfo;
+  private readonly serverEndpoint: string;
+  private readonly pingsDatabase: PingsDatabase;
+
+  // Whether or not Glean was initialized, as reported by the
+  // singleton object.
+  private initialized = false;
+
+  constructor(config: Configuration, platform: Platform, pingsDatabase: PingsDatabase) {
     this.queue = [];
     this.status = PingUploaderStatus.Idle;
-    this.uploader = uploader;
+    // Initialize the ping uploader with either the platform defaults or a custom
+    // provided uploader from the configuration object.
+    this.uploader = config.httpClient ? config.httpClient : platform.uploader;
+    this.platformInfo = platform.info;
+    this.serverEndpoint = config.serverEndpoint;
+    this.pingsDatabase = pingsDatabase;
+  }
+
+  /**
+   * Signals that initialization of Glean was completed.
+   *
+   * This is required in order to not depend on the Glean object.
+   *
+   * @param state An optional state to set the initialization status to.
+   */
+  setInitialized(state?: boolean): void {
+    this.initialized = state ?? true;
   }
 
   /**
@@ -105,7 +132,7 @@ class PingUploader implements PingsDatabaseObserver {
       "Date": (new Date()).toISOString(),
       "X-Client-Type": "Glean.js",
       "X-Client-Version": GLEAN_VERSION,
-      "User-Agent": `Glean/${GLEAN_VERSION} (JS on ${await Glean.platform.info.os()})`
+      "User-Agent": `Glean/${GLEAN_VERSION} (JS on ${await this.platformInfo.os()})`
     };
 
     return {
@@ -122,7 +149,7 @@ class PingUploader implements PingsDatabaseObserver {
    * @returns The status number of the response or `undefined` if unable to attempt upload.
    */
   private async attemptPingUpload(ping: QueuedPing): Promise<UploadResult> {
-    if (!Glean.initialized) {
+    if (!this.initialized) {
       console.warn("Attempted to upload a ping, but Glean is not initialized yet. Ignoring.");
       return { result: UploadResultStatus.RecoverableFailure };
     }
@@ -132,9 +159,7 @@ class PingUploader implements PingsDatabaseObserver {
       // We are sure that the applicationId is not `undefined` at this point,
       // this function is only called when submitting a ping
       // and that function return early when Glean is not initialized.
-      //
-      // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-      `${Glean.serverEndpoint!}${ping.path}`,
+      `${this.serverEndpoint}${ping.path}`,
       finalPing.payload,
       finalPing.headers
     );
@@ -183,14 +208,14 @@ class PingUploader implements PingsDatabaseObserver {
     const { status, result } = response;
     if (status && status >= 200 && status < 300) {
       console.info(`Ping ${identifier} succesfully sent ${status}.`);
-      await Glean.pingsDatabase.deletePing(identifier);
+      await this.pingsDatabase.deletePing(identifier);
       return false;
     }
 
     if (result === UploadResultStatus.UnrecoverableFailure || (status && status >= 400 && status < 500)) {
       console.warn(
         `Unrecoverable upload failure while attempting to send ping ${identifier}. Error was ${status ?? "no status"}.`);
-      await Glean.pingsDatabase.deletePing(identifier);
+      await this.pingsDatabase.deletePing(identifier);
       return false;
     }
 
@@ -273,16 +298,6 @@ class PingUploader implements PingsDatabaseObserver {
     this.queue = [];
   }
 
-  /**
-   * Scans the database for pending pings and enqueues them.
-   */
-  async scanPendingPings(): Promise<void> {
-    const pings = await Glean.pingsDatabase.getAllPings();
-    for (const identifier in pings) {
-      this.enqueuePing({ identifier, ...pings[identifier] });
-    }
-  }
-
   /**
    * Enqueues a new ping and trigger uploading of enqueued pings.
    *

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

@@ -48,13 +48,14 @@ describe("PingsDatabase", function() {
       let wasNotified = false;
       const identifier = "THE IDENTIFIER";
       const observer: Observer = {
-        update: (id: string): void => {
+        update(id: string): void {
           wasNotified = true;
           assert.strictEqual(id, identifier);
         }
       };
 
-      const db = new Database(Glean.platform.Storage, observer);
+      const db = new Database(Glean.platform.Storage);
+      db.attachObserver(observer);
       const path = "some/random/path/doesnt/matter";
 
       const payload = {
@@ -234,4 +235,51 @@ describe("PingsDatabase", function() {
       assert.strictEqual(Object.keys(await db["store"]._getWholeStore()).length, 0);
     });
   });
+
+  describe("pending pings", function() {
+    it("scanning the pending pings directory fills up the queue", async function() {
+      let resolver: (value: unknown) => void;
+      const testPromise = new Promise(r => resolver = r);
+      let pingIds: string[] = [];
+      const observer: Observer = {
+        update(id: string): void {
+          pingIds.push(id);
+
+          if (pingIds.length == 10) {
+            resolver(pingIds);
+          }
+        }
+      };
+      const db = new Database(Glean.platform.Storage);
+      db.attachObserver(observer);
+
+      const path = "some/random/path/doesnt/matter";
+      const payload = {
+        ping_info: {
+          seq: 1,
+          start_time: "2018-02-24+01:00",
+          end_time: "2018-02-25+11:00",
+        },
+        client_info: {
+          telemetry_sdk_build: "32.0.0"
+        }
+      };
+
+      for (let id = 0; id < 10; id++) {
+        const newPayload = payload;
+        newPayload.ping_info.seq = id;
+        await db.recordPing(path, `id-${id}`, payload);
+      }
+
+      // Reset the ids we've seen because `Observer` will get called once again in `record`.
+      pingIds = [];
+
+      await db.scanPendingPings();
+      await testPromise;
+      assert.strictEqual(pingIds.length, 10);
+      for (let id = 0; id < 10; id++) {
+        assert.ok(id in pingIds);
+      }
+    });
+  });
 });

glean/tests/core/upload/index.spec.ts

@@ -6,6 +6,7 @@ import assert from "assert";
 import sinon from "sinon";
 import { v4 as UUIDv4 } from "uuid";
 
+import { Configuration } from "../../../src/core/config";
 import Glean from "../../../src/core/glean";
 import PingType from "../../../src/core/pings";
 import collectAndStorePing from "../../../src/core/pings/maker";
@@ -66,15 +67,6 @@ describe("PingUploader", function() {
     await Glean.testResetGlean(testAppId);
   });
 
-  it("scanning the pending pings directory fills up the queue", async function() {
-    disableGleanUploader();
-    await fillUpPingsDatabase(10);
-
-    const uploader = new PingUploader(Glean.platform.uploader);
-    await uploader.scanPendingPings();
-    assert.strictEqual(uploader["queue"].length, 10);
-  });
-
   it("whenever the pings dabase records a new ping, upload is triggered", async function() {
     const spy = sandbox.spy(Glean["pingUploader"], "triggerUpload");
     await fillUpPingsDatabase(10);
@@ -85,10 +77,25 @@ describe("PingUploader", function() {
     disableGleanUploader();
     await fillUpPingsDatabase(10);
 
-    const uploader = new PingUploader(Glean.platform.uploader);
-    await uploader.scanPendingPings();
+    // Create a new uploader and attach it to the existing storage.
+    const uploader = new PingUploader(new Configuration(), Glean.platform, Glean.pingsDatabase);
+    uploader.setInitialized();
+    Glean.pingsDatabase.attachObserver(uploader);
+
+    // Mock the 'triggerUpload' function so that 'scanPendingPings' does not
+    // mistakenly trigger ping submission. Note that since we're swapping
+    // the function back later in this test, we can safely shut down the linter.
+
+    // eslint-disable-next-line @typescript-eslint/unbound-method
+    const uploadTriggerFunc = uploader.triggerUpload;
+    uploader.triggerUpload = async () => {
+      // Intentionally empty.
+    };
+    
+    await Glean.pingsDatabase.scanPendingPings();
     assert.strictEqual(uploader["queue"].length, 10);
 
+    uploader.triggerUpload = uploadTriggerFunc;
     await uploader.triggerUpload();
     assert.deepStrictEqual(await Glean.pingsDatabase.getAllPings(), {});
     assert.strictEqual(uploader["queue"].length, 0);
@@ -105,8 +112,10 @@ describe("PingUploader", function() {
     disableGleanUploader();
     await fillUpPingsDatabase(10);
 
-    const uploader = new PingUploader(Glean.platform.uploader);
-    await uploader.scanPendingPings();
+    const uploader = new PingUploader(new Configuration(), Glean.platform, Glean.pingsDatabase);
+    uploader.setInitialized();
+    Glean.pingsDatabase.attachObserver(uploader);
+    await Glean.pingsDatabase.scanPendingPings();
 
     // Trigger uploading, but don't wait for it to finish,
     // so that it is ongoing when we cancel.
@@ -157,7 +166,7 @@ describe("PingUploader", function() {
   });
 
   it("duplicates are not enqueued", function() {
-    const uploader = new PingUploader(Glean.platform.uploader);
+    const uploader = new PingUploader(new Configuration(), Glean.platform, Glean.pingsDatabase);
     for (let i = 0; i < 10; i++) {
       uploader["enqueuePing"]({
         identifier: "id",