Merge pull request #534 from ChinYing-Li/bug_1702206

Bug 1702206: Refactor Uploader into class for external use

Author: Beatriz Rizental

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 [Full changelog](https://github.com/mozilla/glean.js/compare/v0.18.1...main)
 
+* [#534](https://github.com/mozilla/glean.js/pull/534): Expose `Uploader` base class through `@mozilla/glean/<platform>/uploader` entry point.
+
 # v0.18.1 (2021-07-22)
 
 [Full changelog](https://github.com/mozilla/glean.js/compare/v0.18.0...v0.18.1)

glean/package.json

@@ -9,7 +9,8 @@
     "./webext": "./dist/webext/index/webext.js",
     "./webext/private/metrics/*": "./dist/webext/core/metrics/types/*.js",
     "./webext/private/ping": "./dist/webext/core/pings/ping_type.js",
-    "./webext/plugins/*": "./dist/webext/plugins/*.js"
+    "./webext/plugins/*": "./dist/webext/plugins/*.js",
+    "./webext/uploader": "./dist/webext/core/upload/uploader.js"
   },
   "typesVersions": {
     "*": {
@@ -24,6 +25,9 @@
       ],
       "webext/plugins/*": [
         "./dist/webext/types/plugins/*"
+      ],
+      "webext/uploader": [
+        "./dist/webext/types/core/upload/uploader.d.ts"
       ]
     }
   },

glean/src/core/upload/index.ts

@@ -13,7 +13,7 @@ import log, { LoggingLevel } from "../log.js";
 import type { Observer as PingsDatabaseObserver, PingInternalRepresentation } from "../pings/database.js";
 import type PingsDatabase from "../pings/database.js";
 import type PlatformInfo from "../platform_info.js";
-import type { UploadResult } from "./uploader.js";
+import { UploadResult } from "./uploader.js";
 import type Uploader from "./uploader.js";
 import { UploadResultStatus } from "./uploader.js";
 
@@ -223,7 +223,7 @@ class PingUploader implements PingsDatabaseObserver {
         "Attempted to upload a ping, but Glean is not initialized yet. Ignoring.",
         LoggingLevel.Warn
       );
-      return { result: UploadResultStatus.RecoverableFailure };
+      return new UploadResult(UploadResultStatus.RecoverableFailure);
     }
 
     try {
@@ -247,9 +247,7 @@ class PingUploader implements PingsDatabaseObserver {
       );
       // An unrecoverable failure will make sure the offending ping is removed from the queue and
       // deleted from the database, which is what we want here.
-      return {
-        result: UploadResultStatus.UnrecoverableFailure
-      };
+      return new UploadResult(UploadResultStatus.RecoverableFailure);
     }
   }
 

glean/src/core/upload/uploader.ts

@@ -28,17 +28,19 @@ export const enum UploadResultStatus {
 /**
  * The result of an attempted ping upload.
  */
-export interface UploadResult {
-  // The status is only present if `result` is UploadResultStatus.Success
-  status?: number,
-  // The status of an upload attempt
-  result: UploadResultStatus
+export class UploadResult {
+  constructor(
+    // The status of an upload attempt
+    readonly result: UploadResultStatus,
+    // The status is only present if `result` is UploadResultStatus.Success
+    readonly status?: number
+  ) {}
 }
 
 /**
- * Uploader interface, actualy uploading logic varies per platform.
+ * Uploader abstract class, actualy uploading logic varies per platform.
  */
-export interface Uploader {
+export abstract class Uploader {
   /**
    * Makes a POST request to a given url, with the given headers and body.
    *
@@ -49,7 +51,7 @@ export interface Uploader {
    * @param headers Optional header to include in the request
    * @returns The status code of the response.
    */
-  post(url: string, body: string | Uint8Array, headers?: Record<string, string>): Promise<UploadResult>;
+  abstract post(url: string, body: string | Uint8Array, headers?: Record<string, string>): Promise<UploadResult>;
 }
 
 export default Uploader;

glean/src/platform/qt/uploader.ts

@@ -3,14 +3,13 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 import log, { LoggingLevel } from "../../core/log.js";
-import type Uploader from "../../core/upload/uploader.js";
-import type { UploadResult } from "../../core/upload/uploader.js";
-import { DEFAULT_UPLOAD_TIMEOUT_MS, UploadResultStatus } from "../../core/upload/uploader.js";
+import Uploader from "../../core/upload/uploader.js";
+import { DEFAULT_UPLOAD_TIMEOUT_MS, UploadResultStatus, UploadResult } from "../../core/upload/uploader.js";
 import { isString } from "../../core/utils.js";
 
 const LOG_TAG = "platform.qt.Uploader";
 
-class QtUploader implements Uploader {
+class QtUploader extends Uploader {
   async post(url: string, body: string | Uint8Array, headers: Record<string, string> = {}): Promise<UploadResult> {
     return new Promise((resolve) => {
       const xhr = new XMLHttpRequest();
@@ -23,30 +22,21 @@ class QtUploader implements Uploader {
 
       xhr.ontimeout = function(e) {
         log(LOG_TAG, ["Timeout while attempting to upload ping.\n", e.type], LoggingLevel.Error);
-        resolve({
-          result: UploadResultStatus.RecoverableFailure,
-        });
+        resolve(new UploadResult(UploadResultStatus.RecoverableFailure));
       };
 
       xhr.onerror = function(e) {
         log(LOG_TAG, ["Network rror while attempting to upload ping.\n", e.type], LoggingLevel.Error);
-        resolve({
-          result: UploadResultStatus.RecoverableFailure,
-        });
+        resolve(new UploadResult(UploadResultStatus.RecoverableFailure));
       };
 
       xhr.onabort = function (e) {
         log(LOG_TAG, ["The attempt to upload ping is aborted.\n", e.type], LoggingLevel.Error);
-        resolve({
-          result: UploadResultStatus.RecoverableFailure,
-        });
+        resolve(new UploadResult(UploadResultStatus.RecoverableFailure));
       };
 
       xhr.onload = () => {
-        resolve({
-          status: xhr.status,
-          result: UploadResultStatus.Success,
-        });
+        resolve(new UploadResult(UploadResultStatus.Success, xhr.status));
       };
 
       if (!isString(body)) {

glean/src/platform/test/index.ts

@@ -5,18 +5,14 @@
 import MockStorage from "../test/storage.js";
 import type PlatformInfo from "../../core/platform_info.js";
 import { KnownOperatingSystems } from "../../core/platform_info.js";
-import type { UploadResult} from "../../core/upload/uploader.js";
-import type Uploader from "../../core/upload/uploader.js";
-import { UploadResultStatus } from "../../core/upload/uploader.js";
+import Uploader from "../../core/upload/uploader.js";
+import { UploadResultStatus, UploadResult } from "../../core/upload/uploader.js";
 import type Platform from "../index.js";
 
-class MockUploader implements Uploader {
+class MockUploader extends Uploader {
   // eslint-disable-next-line @typescript-eslint/no-unused-vars
   post(_url: string, _body: string | Uint8Array, _headers?: Record<string, string>): Promise<UploadResult> {
-    const result: UploadResult = {
-      result: UploadResultStatus.Success,
-      status: 200
-    };
+    const result = new UploadResult(UploadResultStatus.Success, 200);
     return Promise.resolve(result);
   }
 }

glean/src/platform/webext/uploader.ts

@@ -3,13 +3,12 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 import log, { LoggingLevel } from "../../core/log.js";
-import type Uploader from "../../core/upload/uploader.js";
-import type { UploadResult } from "../../core/upload/uploader.js";
-import { DEFAULT_UPLOAD_TIMEOUT_MS, UploadResultStatus } from "../../core/upload/uploader.js";
+import Uploader from "../../core/upload/uploader.js";
+import { DEFAULT_UPLOAD_TIMEOUT_MS, UploadResultStatus, UploadResult } from "../../core/upload/uploader.js";
 
 const LOG_TAG = "platform.webext.Uploader";
 
-class BrowserUploader implements Uploader {
+class BrowserUploader extends Uploader {
   async post(url: string, body: string | Uint8Array, headers: Record<string, string> = {}): Promise<UploadResult> {
     const controller = new AbortController();
     const timeout = setTimeout(() => controller.abort(), DEFAULT_UPLOAD_TIMEOUT_MS);
@@ -44,14 +43,11 @@ class BrowserUploader implements Uploader {
       }
 
       clearTimeout(timeout);
-      return { result: UploadResultStatus.RecoverableFailure };
+      return new UploadResult(UploadResultStatus.RecoverableFailure);
     }
 
     clearTimeout(timeout);
-    return {
-      result: UploadResultStatus.Success,
-      status: response.status
-    };
+    return new UploadResult(UploadResultStatus.Success, response.status);
   }
 }
 

glean/tests/unit/platform/qt/uploader.spec.ts

@@ -7,7 +7,7 @@ import assert from "assert";
 import sinon from "sinon";
 
 import QtUploader from "../../../../src/platform/qt/uploader";
-import { DEFAULT_UPLOAD_TIMEOUT_MS, UploadResultStatus } from "../../../../src/core/upload/uploader";
+import { DEFAULT_UPLOAD_TIMEOUT_MS, UploadResult, UploadResultStatus } from "../../../../src/core/upload/uploader";
 
 describe("Uploader/Qt", function () {
   let server: sinon.SinonFakeServer;
@@ -25,9 +25,10 @@ describe("Uploader/Qt", function () {
       const response = QtUploader.post("/hello", "");
       server.respondWith("POST", "/hello", [status, {}, ""]);
       server.respond();
+      const expectedResponse = new UploadResult(UploadResultStatus.Success, status);
       assert.deepStrictEqual(
         await response,
-        { status: status, result: UploadResultStatus.Success });
+        expectedResponse);
     }
   });
 
@@ -37,8 +38,9 @@ describe("Uploader/Qt", function () {
     clock.tick(DEFAULT_UPLOAD_TIMEOUT_MS + 1);
     server.respondWith("POST", "/hello", [200, {}, ""]);
     server.respond();
+    const expectedResponse = new UploadResult(UploadResultStatus.RecoverableFailure);
     assert.deepStrictEqual(
       await response,
-      { result: UploadResultStatus.RecoverableFailure });
+      expectedResponse);
   });
 });

glean/tests/unit/platform/webext/uploader.spec.ts

@@ -8,7 +8,7 @@ import sinon from "sinon";
 
 import BrowserUploader from "../../../../src/platform/webext/uploader";
 import type { JSONObject } from "../../../../src/core/utils";
-import { UploadResultStatus } from "../../../../src/core/upload/uploader";
+import { UploadResult, UploadResultStatus } from "../../../../src/core/upload/uploader";
 
 const sandbox = sinon.createSandbox();
 
@@ -64,17 +64,19 @@ describe("Uploader/browser", function () {
     const stub = sandbox.stub(global, "fetch");
     for (const [index, status] of [200, 400, 500].entries()) {
       stub.onCall(index).returns(Promise.resolve(createResponse(status)));
+      const expectedResponse = new UploadResult(UploadResultStatus.Success, status);
       assert.deepStrictEqual(
         await BrowserUploader.post("https://localhost:8080", ""),
-        { status: status, result: UploadResultStatus.Success });
+        expectedResponse);
     }
   });
 
   it("doesn't throw if upload action throws", async function () {
     sandbox.stub(global, "fetch").callsFake(() => Promise.reject());
+    const expectedResponse = new UploadResult(UploadResultStatus.RecoverableFailure);
     assert.deepStrictEqual(
       await BrowserUploader.post("https://localhost:8080", ""),
-      { result: UploadResultStatus.RecoverableFailure }
+      expectedResponse
     );
   });
 });

glean/tests/unit/plugins/encryption.spec.ts

@@ -11,12 +11,11 @@ import compactDecrypt from "jose/jwe/compact/decrypt";
 import Glean from "../../../src/core/glean";
 import PingType from "../../../src/core/pings/ping_type";
 import type { JSONObject } from "../../../src/core/utils";
-import { WaitableUploader } from "../../../tests/utils";
+import { WaitableUploader } from "../../utils";
 import PingEncryptionPlugin from "../../../src/plugins/encryption";
 import collectAndStorePing, { makePath } from "../../../src/core/pings/maker";
-import type { UploadResult} from "../../../src/core/upload/uploader";
-import type Uploader from "../../../src/core/upload/uploader";
-import { UploadResultStatus } from "../../../src/core/upload/uploader";
+import Uploader from "../../../src/core/upload/uploader";
+import { UploadResultStatus, UploadResult } from "../../../src/core/upload/uploader";
 import CounterMetricType from "../../../src/core/metrics/types/counter";
 import { Lifetime } from "../../../src/core/metrics/lifetime";
 import { Context } from "../../../src/core/context";
@@ -82,13 +81,10 @@ describe("PingEncryptionPlugin", function() {
     let uploadPromiseResolve: (value: UploadSignature) => void;
     const uploadPromise = new Promise<UploadSignature>(r => uploadPromiseResolve = r);
 
-    class MockUploader implements Uploader {
+    class MockUploader extends Uploader {
       post(url: string, body: string, headers?: Record<string, string>): Promise<UploadResult> {
         uploadPromiseResolve({url, body, headers});
-        const result: UploadResult = {
-          result: UploadResultStatus.Success,
-          status: 200
-        };
+        const result = new UploadResult(UploadResultStatus.Success, 200);
         return Promise.resolve(result);
       }
     }