Add a single retry to HttpRequestHandler if it gets a network error. (#85)

justinrosenthal · web-flow · commit 3e6e3964fb4d · 2017-09-26T11:36:25.000-07:00
diff --git a/src/utils/api-request.ts b/src/utils/api-request.ts
@@ -32,6 +32,7 @@ export type ApiCallbackFunction = (data: Object) => void;
 export class HttpRequestHandler {
   /**
    * Sends HTTP requests and returns a promise that resolves with the result.
+   * Will retry once if the first attempt encounters an AppErrorCodes.NETWORK_ERROR.
    *
    * @param {string} host The HTTP host.
    * @param {number} port The port number.
@@ -50,6 +51,43 @@ export class HttpRequestHandler {
       data?: Object,
       headers?: Object,
       timeout?: number): Promise<Object> {
+    // Convenience for calling the real _sendRequest() method with the original params.
+    const sendOneRequest = () => {
+      return this._sendRequest(host, port, path, httpMethod, data, headers, timeout);
+    };
+
+    return sendOneRequest()
+      .catch((response: { statusCode: number, error: string|Object }) => {
+        // Retry if the request failed due to a network error.
+        if (response.error instanceof FirebaseAppError) {
+          if ((<FirebaseAppError> response.error).hasCode(AppErrorCodes.NETWORK_ERROR)) {
+            return sendOneRequest();
+          }
+        }
+        return Promise.reject(response);
+      });
+  }
+
+  /**
+   * Sends HTTP requests and returns a promise that resolves with the result.
+   *
+   * @param {string} host The HTTP host.
+   * @param {number} port The port number.
+   * @param {string} path The endpoint path.
+   * @param {HttpMethod} httpMethod The http method.
+   * @param {Object} [data] The request JSON.
+   * @param {Object} [headers] The request headers.
+   * @param {number} [timeout] The request timeout in milliseconds.
+   * @return {Promise<Object>} A promise that resolves with the response.
+   */
+  private _sendRequest(
+      host: string,
+      port: number,
+      path: string,
+      httpMethod: HttpMethod,
+      data?: Object,
+      headers?: Object,
+      timeout?: number): Promise<Object> {
     let requestData;
     if (data) {
       try {
@@ -189,13 +227,11 @@ export class SignedApiRequestHandler extends HttpRequestHandler {
       data: Object,
       headers: Object,
       timeout: number): Promise<Object> {
-    let ancestorSendRequest = super.sendRequest;
-
     return this.app_.INTERNAL.getToken().then((accessTokenObj) => {
       let headersCopy: Object = deepCopy(headers);
       let authorizationHeaderKey = 'Authorization';
       headersCopy[authorizationHeaderKey] = 'Bearer ' + accessTokenObj.accessToken;
-      return ancestorSendRequest(host, port, path, httpMethod, data, headersCopy, timeout);
+      return super.sendRequest(host, port, path, httpMethod, data, headersCopy, timeout);
     });
   }
 }
diff --git a/src/utils/error.ts b/src/utils/error.ts
@@ -73,30 +73,67 @@ export class FirebaseError extends Error {
 }
 
 /**
- * Firebase App error code structure. This extends FirebaseError.
+ * A FirebaseError with a prefix in front of the error code.
  *
+ * @param {string} codePrefix The prefix to apply to the error code.
  * @param {string} code The error code.
  * @param {string} message The error message.
  * @constructor
  */
-export class FirebaseAppError extends FirebaseError {
-  constructor(code: string, message: string) {
+class PrefixedFirebaseError extends FirebaseError {
+  constructor(private codePrefix: string, code: string, message: string) {
     super({
-      code: 'app/' + code,
+      code: `${codePrefix}/${code}`,
       message,
     });
+
+    /* tslint:disable:max-line-length */
+    // Set the prototype explicitly. See the following link for more details:
+    // https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work
+    /* tslint:enable:max-line-length */
+    (this as any).__proto__ = PrefixedFirebaseError.prototype;
+  }
+
+  /**
+   * Allows the error type to be checked without needing to know implementation details
+   * of the code prefixing.
+   *
+   * @param {string} code The non-prefixed error code to test against.
+   * @return {boolean} True if the code matches, false otherwise.
+   */
+  public hasCode(code: string): boolean {
+    return `${this.codePrefix}/${code}` === this.code;
+  }
+}
+
+/**
+ * Firebase App error code structure. This extends PrefixedFirebaseError.
+ *
+ * @param {string} code The error code.
+ * @param {string} message The error message.
+ * @constructor
+ */
+export class FirebaseAppError extends PrefixedFirebaseError {
+  constructor(code: string, message: string) {
+    super('app', code, message);
+
+    /* tslint:disable:max-line-length */
+    // Set the prototype explicitly. See the following link for more details:
+    // https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work
+    /* tslint:enable:max-line-length */
+    (this as any).__proto__ = FirebaseAppError.prototype;
   }
 }
 
 /**
- * Firebase Auth error code structure. This extends FirebaseError.
+ * Firebase Auth error code structure. This extends PrefixedFirebaseError.
  *
  * @param {ErrorInfo} info The error code info.
  * @param {string} [message] The error message. This will override the default
  *     message if provided.
  * @constructor
  */
-export class FirebaseAuthError extends FirebaseError {
+export class FirebaseAuthError extends PrefixedFirebaseError {
   /**
    * Creates the developer-facing error corresponding to the backend error code.
    *
@@ -129,19 +166,25 @@ export class FirebaseAuthError extends FirebaseError {
 
   constructor(info: ErrorInfo, message?: string) {
     // Override default message if custom message provided.
-    super({code: 'auth/' + info.code, message: message || info.message});
+    super('auth', info.code, message || info.message);
+
+    /* tslint:disable:max-line-length */
+    // Set the prototype explicitly. See the following link for more details:
+    // https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work
+    /* tslint:enable:max-line-length */
+    (this as any).__proto__ = FirebaseAuthError.prototype;
   }
 }
 
 
 /**
- * Firebase Messaging error code structure. This extends FirebaseError.
+ * Firebase Messaging error code structure. This extends PrefixedFirebaseError.
  *
  * @param {ErrorInfo} info The error code info.
  * @param {string} [message] The error message. This will override the default message if provided.
  * @constructor
  */
-export class FirebaseMessagingError extends FirebaseError {
+export class FirebaseMessagingError extends PrefixedFirebaseError {
   /**
    * Creates the developer-facing error corresponding to the backend error code.
    *
@@ -174,7 +217,13 @@ export class FirebaseMessagingError extends FirebaseError {
 
   constructor(info: ErrorInfo, message?: string) {
     // Override default message if custom message provided.
-    super({code: 'messaging/' + info.code, message: message || info.message});
+    super('messaging', info.code, message || info.message);
+
+    /* tslint:disable:max-line-length */
+    // Set the prototype explicitly. See the following link for more details:
+    // https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work
+    /* tslint:enable:max-line-length */
+    (this as any).__proto__ = FirebaseMessagingError.prototype;
   }
 }
 
diff --git a/test/unit/utils/api-request.spec.ts b/test/unit/utils/api-request.spec.ts
@@ -100,15 +100,15 @@ function mockRequest(
 }
 
 /**
- * Returns a mocked out error response for a dummy URL.
+ * Returns a mocked out HTTP error response for a dummy URL.
  *
  * @param {number} [statusCode] Optional response status code.
  * @param {string} [responseContentType] Optional response content type.
  * @param {any} [response] Optional response.
  *
  * @return {Object} A nock response object.
  */
-function mockRequestWithError(
+function mockRequestWithHttpError(
   statusCode = 400,
   responseContentType = 'application/json',
   response: any = mockErrorResponse,
@@ -124,6 +124,19 @@ function mockRequestWithError(
     });
 }
 
+/**
+ * Returns a mocked out error response for a dummy URL, useful for simulating
+ * network errors.
+ *
+ * @param {Error} [err] The request error.
+ *
+ * @return {Object} A nock response object.
+ */
+function mockRequestWithError(err: Error) {
+  return nock('https://' + mockHost)
+    .get(mockPath)
+    .replyWithError(err);
+}
 
 describe('HttpRequestHandler', () => {
   let mockedRequests: nock.Scope[] = [];
@@ -151,14 +164,12 @@ describe('HttpRequestHandler', () => {
 
 
   describe('sendRequest', () => {
-    it('should be rejected on an unexpected error', () => {
-      httpsRequestStub = sinon.stub(https, 'request');
-      httpsRequestStub.returns(mockRequestStream);
+    it('should be rejected, after 1 retry, on multiple network errors', () => {
+      mockedRequests.push(mockRequestWithError(new Error('first error')));
+      mockedRequests.push(mockRequestWithError(new Error('second error')));
 
       const sendRequestPromise = httpRequestHandler.sendRequest(mockHost, mockPort, mockPath, 'GET');
 
-      mockRequestStream.emit('error', new Error('some error'));
-
       return sendRequestPromise
         .then(() => {
           throw new Error('Unexpected success.');
@@ -168,7 +179,15 @@ describe('HttpRequestHandler', () => {
           expect(response.error).to.have.property('code', 'app/network-error');
           expect(response.statusCode).to.equal(502);
         });
-     });
+    });
+
+    it('should succeed, after 1 retry, on a single network error', () => {
+      mockedRequests.push(mockRequestWithError(new Error('first error')));
+      mockedRequests.push(mockRequest());
+
+      return httpRequestHandler.sendRequest(mockHost, mockPort, mockPath, 'GET')
+        .should.eventually.be.fulfilled.and.deep.equal(mockSuccessResponse);
+    });
 
     it('should be rejected on a network timeout', () => {
       httpsRequestStub = sinon.stub(https, 'request');
@@ -224,7 +243,7 @@ describe('HttpRequestHandler', () => {
 
     describe('with JSON response', () => {
       it('should be rejected given a 4xx response', () => {
-        mockedRequests.push(mockRequestWithError(400));
+        mockedRequests.push(mockRequestWithHttpError(400));
 
         return httpRequestHandler.sendRequest(mockHost, mockPort, mockPath, 'GET')
           .should.eventually.be.rejected.and.deep.equal({
@@ -234,7 +253,7 @@ describe('HttpRequestHandler', () => {
       });
 
       it('should be rejected given a 5xx response', () => {
-        mockedRequests.push(mockRequestWithError(500));
+        mockedRequests.push(mockRequestWithHttpError(500));
 
         return httpRequestHandler.sendRequest(mockHost, mockPort, mockPath, 'GET')
           .should.eventually.be.rejected.and.deep.equal({
@@ -244,7 +263,7 @@ describe('HttpRequestHandler', () => {
       });
 
       it('should be rejected given an error when parsing the JSON response', () => {
-        mockedRequests.push(mockRequestWithError(400, undefined, mockTextErrorResponse));
+        mockedRequests.push(mockRequestWithHttpError(400, undefined, mockTextErrorResponse));
 
         return httpRequestHandler.sendRequest(mockHost, mockPort, mockPath, 'GET')
           .then(() => {
@@ -275,7 +294,7 @@ describe('HttpRequestHandler', () => {
 
     describe('with text response', () => {
       it('should be rejected given a 4xx response', () => {
-        mockedRequests.push(mockRequestWithError(400, 'text/html'));
+        mockedRequests.push(mockRequestWithHttpError(400, 'text/html'));
 
         return httpRequestHandler.sendRequest(mockHost, mockPort, mockPath, 'GET')
           .should.eventually.be.rejected.and.deep.equal({
@@ -285,7 +304,7 @@ describe('HttpRequestHandler', () => {
       });
 
       it('should be rejected given a 5xx response', () => {
-        mockedRequests.push(mockRequestWithError(500, 'text/html'));
+        mockedRequests.push(mockRequestWithHttpError(500, 'text/html'));
 
         return httpRequestHandler.sendRequest(mockHost, mockPort, mockPath, 'GET')
           .should.eventually.be.rejected.and.deep.equal({
