Bug 1677440 - Implement a browser upload adapter

package.json

@@ -42,6 +42,8 @@
     "eslint-plugin-mocha": "^8.0.0",
     "eslint-plugin-notice": "^0.9.10",
     "geckodriver": "^1.21.0",
+    "jsdom": "16.4.0",
+    "jsdom-global": "3.0.2",
     "mocha": "^8.2.1",
     "selenium-webdriver": "^4.0.0-alpha.8",
     "sinon": "^9.2.2",

src/upload/index.ts

@@ -5,7 +5,7 @@
 import { GLEAN_VERSION } from "../constants";
 import Glean from "glean";
 import { Observer as PingsDatabaseObserver, PingInternalRepresentation } from "pings/database";
-import UploadAdapter from "upload/adapter";
+import Uploader from "upload/uploader";
 
 interface QueuedPing extends PingInternalRepresentation {
   identifier: string
@@ -23,6 +23,36 @@ const enum PingUploaderStatus {
   Cancelling,
 }
 
+/**
+ * The resulting status of an attempted ping upload.
+ */
+export const enum UploadResultStatus {
+  // A recoverable failure.
+  //
+  // During upload something went wrong,
+  // e.g. the network connection failed.
+  // The upload should be retried at a later time.
+  RecoverableFailure,
+  // An unrecoverable upload failure.
+  //
+  // A possible cause might be a malformed URL.
+  UnrecoverableFailure,
+  // Request was successfull.
+  //
+  // This can still indicate an error, depending on the status code.
+  Success,
+}
+
+/**
+ * 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
+}
+
 /**
  * A ping uploader. Manages a queue of pending pings to upload.
  *
@@ -117,20 +147,20 @@ class PingUploader implements PingsDatabaseObserver {
    *
    * @returns The status number of the response or `undefined` if unable to attempt upload.
    */
-  private async attemptPingUpload(ping: QueuedPing): Promise<number | undefined> {
+  private async attemptPingUpload(ping: QueuedPing): Promise<UploadResult> {
     if (!Glean.initialize) {
       console.warn("Attempted to upload a ping, but Glean is not initialized yet. Ignoring.");
-      return;
+      return { result: UploadResultStatus.RecoverableFailure };
     }
 
     const finalPing = this.preparePingForUpload(ping);
-    const status = await UploadAdapter.post(
+    const result = await Uploader.post(
       new URL(ping.path, Glean.serverEndpoint),
       finalPing.payload,
       finalPing.headers
     );
 
-    return status;
+    return result;
   }
 
   /**
@@ -166,24 +196,27 @@ class PingUploader implements PingsDatabaseObserver {
    *   * 500 - internal error
    *
    * @param identifier The identifier of the ping uploaded.
-   * @param status The status of a ping upload attempt.
+   * @param response The response of a ping upload attempt.
    *
    * @returns Whether or not to retry the upload attempt.
    */
-  private async processPingUploadResponse(identifier: string, status?: number): Promise<boolean> {
+  private async processPingUploadResponse(identifier: string, response: UploadResult): Promise<boolean> {
+    const { status, result } = response;
     if (status && status >= 200 && status < 300) {
       console.info(`Ping ${identifier} succesfully sent ${status}.`);
       await Glean.pingsDatabase.deletePing(identifier);
       return false;
     }
 
-    if (status && status >= 400 && status < 500) {
-      console.warn(`Unrecoverable upload failure while attempting to send ping ${identifier}. Error was ${status}.`);
+    if (result === UploadResultStatus.UnrecoverableFailure || (status && status >= 400 && status < 500)) {
+      console.warn(
+        `Unrecoverable upload failure while attempting to send ping ${identifier}. Error was ${status}.`);
       await Glean.pingsDatabase.deletePing(identifier);
       return false;
     }
 
-    console.warn(`Recoverable upload failure while attempting to send ping ${identifier}, will retry. Error was ${status}.`);
+    console.warn(
+      `Recoverable upload failure while attempting to send ping ${identifier}, will retry. Error was ${status}.`);
     return true;
   }
 

src/upload/uploader/browser.ts

@@ -0,0 +1,54 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * 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 { UploadResult, UploadResultStatus } from "upload";
+import { Uploader } from ".";
+
+class BrowserUploader extends Uploader {
+  async post(url: URL, body: string, headers: Record<string, string> = {}): Promise<UploadResult> {
+    const controller = new AbortController();
+    const timeout = setTimeout(() => controller.abort(), this.defaultTimeout);
+
+    let response;
+    try {
+      response = await fetch(url.toString(), {
+        headers,
+        method: "POST",
+        body: body,
+        keepalive: true,
+        // Strips any cookies or authorization headers from the request.
+        credentials: "omit",
+        signal: controller.signal,
+        redirect: "error",
+      });
+    } catch(e) {
+      // If we time out and call controller.abort,
+      // the fetch API will throw a DOMException with name "AbortError".
+      if (e instanceof DOMException) {
+        console.error("Timeout while attempting to upload ping.", e);
+      } else if (e instanceof TypeError) {
+        // From MDN: "A fetch() promise will reject with a TypeError
+        // when a network error is encountered or CORS is misconfigured on the server-side,
+        // although this usually means permission issues or similar"
+        // See: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch#checking_that_the_fetch_was_successful
+        //
+        // We will treat this as we treat server / network errors in this case.
+        console.error("Network while attempting to upload ping.", e);
+      } else {
+        console.error("Unknown error while attempting to upload ping.", e);
+      }
+
+      clearTimeout(timeout);
+      return { result: UploadResultStatus.RecoverableFailure };
+    }
+
+    clearTimeout(timeout);
+    return {
+      result: UploadResultStatus.Success,
+      status: (await response.json()).status
+    };
+  }
+}
+
+export default new BrowserUploader();

src/upload/uploader/index.ts

@@ -0,0 +1,46 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * 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 { UploadResult, UploadResultStatus } from "upload";
+
+/**
+ * Uploader interface, actualy uploading logic varies per platform.
+ */
+export abstract class Uploader {
+  // The timeout, in seconds, to use for all operations with the server.
+  protected defaultTimeout = 10_000;
+
+  /**
+   * Makes a POST request to a given url, with the given headers and body.
+   *
+   * @param url The URL to make the POST request
+   * @param body The stringified body of this post request
+   * @param headers Optional header to include in the request
+   *
+   * @returns The status code of the response.
+   */
+  abstract post(url: URL, body: string, headers?: Record<string, string>): Promise<UploadResult>;
+}
+
+// Default export for tests sake.
+//
+// This is necessary, because when building for release we will not use this index file.
+// Instead, each platform should have their own Uploader implementation and alias the "upload/adapter"
+// import to point to the their specific file.
+//
+// The aliasing is done in the webpack configuration, but tests don't use webpack and expect
+// this file to have a default export.
+//
+// TODO: remove this comment once Bug 1687516 is resolved.
+class MockUploader extends Uploader {
+  post(): Promise<UploadResult> {
+    const result: UploadResult = {
+      result: UploadResultStatus.Success,
+      status: 200
+    };
+    return Promise.resolve(result);
+  }
+}
+
+export default new MockUploader();

tests/storage.spec.ts

@@ -6,11 +6,11 @@ import { WebDriver } from "selenium-webdriver";
 import assert from "assert";
 
 import { setupFirefox, webExtensionAPIProxyBuilder } from "./utils/webext";
-import { StorageValue, Store } from "storage";
+import { Store } from "storage";
 
 import WeakStore from "storage/weak";
 import WebExtStore from "storage/persistent/webext";
-import { isUndefined } from "utils";
+import { isUndefined, JSONValue } from "utils";
 
 let firefox: WebDriver;
 
@@ -151,7 +151,7 @@ for (const store in stores) {
       });
 
       it("Attempting to update an existing entry doesn't error ", async function () {
-        const updater = (v?: StorageValue): string => `${v} new and improved!`;
+        const updater = (v?: JSONValue): string => `${v} new and improved!`;
         await store.update(index, updater);
         assert.strictEqual(updater(value), await store.get(index));
       });

tests/upload.spec.ts → tests/upload/index.spec.ts

@@ -7,8 +7,8 @@ import sinon from "sinon";
 import { v4 as UUIDv4 } from "uuid";
 
 import Glean from "glean";
-import PingUploader from "upload";
-import UploadAdapter from "upload/adapter";
+import PingUploader, { UploadResultStatus } from "upload";
+import Uploader from "upload/uploader";
 
 const sandbox = sinon.createSandbox();
 
@@ -97,7 +97,7 @@ describe("PingUploader", function() {
   });
 
   it("if multiple pings are enqueued subsequently, we don't attempt to upload each ping more than once", async function () {
-    const spy = sandbox.spy(UploadAdapter, "post");
+    const spy = sandbox.spy(Uploader, "post");
     await fillUpPingsDatabase(100);
     await waitForGleanUploader();
     assert.strictEqual(spy.callCount, 100);
@@ -121,8 +121,8 @@ describe("PingUploader", function() {
   });
 
   it("correctly deletes pings when upload is successfull", async function() {
-    // Always return succes response from upload attempt.
-    sandbox.stub(UploadAdapter, "post").callsFake(() => Promise.resolve(200));
+    // There is no need to stub the upload adapter here,
+    // as the default exported mock already returns a success response always.
     await fillUpPingsDatabase(10);
 
     await waitForGleanUploader();
@@ -132,17 +132,23 @@ describe("PingUploader", function() {
 
   it("correctly deletes pings when upload is unrecoverably unsuccesfull", async function() {
     // Always return unrecoverable failure response from upload attempt.
-    sandbox.stub(UploadAdapter, "post").callsFake(() => Promise.resolve(400));
+    sandbox.stub(Uploader, "post").callsFake(() => Promise.resolve({
+      status: 400,
+      result: UploadResultStatus.Success
+    }));
     await fillUpPingsDatabase(10);
-    
+
     await waitForGleanUploader();
     assert.deepStrictEqual(await Glean.pingsDatabase.getAllPings(), {});
     assert.strictEqual(Glean["pingUploader"]["queue"].length, 0);
   });
 
   it("correctly re-enqueues pings when upload is recovarbly unsuccesfull", async function() {
     // Always return recoverable failure response from upload attempt.
-    sandbox.stub(UploadAdapter, "post").callsFake(() => Promise.resolve(500));
+    sandbox.stub(Uploader, "post").callsFake(() => Promise.resolve({
+      status: 500,
+      result: UploadResultStatus.Success
+    }));
     await fillUpPingsDatabase(1);
 
     await waitForGleanUploader();
@@ -176,7 +182,10 @@ describe("PingUploader", function() {
 
   it("maximum of recoverable errors is enforced", async function () {
     // Always return recoverable failure response from upload attempt.
-    const stub = sandbox.stub(UploadAdapter, "post").callsFake(() => Promise.resolve(500));
+    const stub = sandbox.stub(Uploader, "post").callsFake(() => Promise.resolve({
+      status: 500,
+      result: UploadResultStatus.Success
+    }));
     await fillUpPingsDatabase(1);
 
     await waitForGleanUploader();