Preventing deep copying the request when making http calls (#412)

* Preventing unnecessary copying of the entire request when making http calls

* Preventing copying at AuthorizedHttpClient

Author: hiranya911

src/utils/api-request.ts

@@ -180,11 +180,10 @@ export class HttpClient {
  * Sends an HTTP request based on the provided configuration. This is a wrapper around the http and https
  * packages of Node.js, providing content processing, timeouts and error handling.
  */
-function sendRequest(httpRequestConfig: HttpRequestConfig): Promise<LowLevelResponse> {
-  const config: HttpRequestConfig = deepCopy(httpRequestConfig);
+function sendRequest(config: HttpRequestConfig): Promise<LowLevelResponse> {
   return new Promise((resolve, reject) => {
     let data: Buffer;
-    const headers = config.headers || {};
+    const headers = Object.assign({}, config.headers);
     let fullUrl: string = config.url;
     if (config.data) {
       // GET and HEAD do not support body in request.
@@ -197,15 +196,14 @@ function sendRequest(httpRequestConfig: HttpRequestConfig): Promise<LowLevelResp
         }
 
         // Parse URL and append data to query string.
-        const configUrl = new url.URL(fullUrl);
-        for (const key in config.data as any) {
-          if (config.data.hasOwnProperty(key)) {
-            configUrl.searchParams.append(
-                key,
-                (config.data as {[key: string]: string})[key]);
+        const parsedUrl = new url.URL(fullUrl);
+        const dataObj = config.data as {[key: string]: string};
+        for (const key in dataObj) {
+          if (dataObj.hasOwnProperty(key)) {
+            parsedUrl.searchParams.append(key, dataObj[key]);
           }
         }
-        fullUrl = configUrl.toString();
+        fullUrl = parsedUrl.toString();
       } else if (validator.isObject(config.data)) {
         data = Buffer.from(JSON.stringify(config.data), 'utf-8');
         if (typeof headers['Content-Type'] === 'undefined') {
@@ -362,8 +360,8 @@ export class AuthorizedHttpClient extends HttpClient {
 
   public send(request: HttpRequestConfig): Promise<HttpResponse> {
     return this.app.INTERNAL.getToken().then((accessTokenObj) => {
-      const requestCopy = deepCopy(request);
-      requestCopy.headers = requestCopy.headers || {};
+      const requestCopy = Object.assign({}, request);
+      requestCopy.headers = Object.assign({}, request.headers);
       const authHeader = 'Authorization';
       requestCopy.headers[authHeader] = `Bearer ${accessTokenObj.accessToken}`;
 

test/unit/utils/api-request.spec.ts

@@ -16,7 +16,6 @@
 
 'use strict';
 
-import * as _ from 'lodash';
 import * as chai from 'chai';
 import * as nock from 'nock';
 import * as sinon from 'sinon';
@@ -28,8 +27,9 @@ import * as mocks from '../../resources/mocks';
 
 import {FirebaseApp} from '../../../src/firebase-app';
 import {
-  ApiSettings, HttpClient, HttpError, AuthorizedHttpClient, ApiCallbackFunction,
+  ApiSettings, HttpClient, HttpError, AuthorizedHttpClient, ApiCallbackFunction, HttpRequestConfig,
 } from '../../../src/utils/api-request';
+import { deepCopy } from '../../../src/utils/deep-copy';
 import {Agent} from 'http';
 
 chai.should();
@@ -95,7 +95,7 @@ describe('HttpClient', () => {
   let transportSpy: sinon.SinonSpy = null;
 
   afterEach(() => {
-    _.forEach(mockedRequests, (mockedRequest) => mockedRequest.done());
+    mockedRequests.forEach((mockedRequest) => mockedRequest.done());
     mockedRequests = [];
     if (transportSpy) {
       transportSpy.restore();
@@ -202,6 +202,38 @@ describe('HttpClient', () => {
     });
   });
 
+  it('should not mutate the arguments', () => {
+    const reqData = {request: 'data'};
+    const scope = nock('https://' + mockHost, {
+      reqheaders: {
+        'Authorization': 'Bearer token',
+        'Content-Type': (header) => {
+          return header.startsWith('application/json'); // auto-inserted
+        },
+        'My-Custom-Header': 'CustomValue',
+      },
+    }).post(mockPath, reqData)
+    .reply(200, {success: true}, {
+      'content-type': 'application/json',
+    });
+    mockedRequests.push(scope);
+    const client = new HttpClient();
+    const request: HttpRequestConfig = {
+      method: 'POST',
+      url: mockUrl,
+      headers: {
+        'authorization': 'Bearer token',
+        'My-Custom-Header': 'CustomValue',
+      },
+      data: reqData,
+    };
+    const requestCopy = deepCopy(request);
+    return client.send(request).then((resp) => {
+      expect(resp.status).to.equal(200);
+      expect(request).to.deep.equal(requestCopy);
+    });
+  });
+
   it('should make a GET request with the provided headers and data', () => {
     const reqData = {key1: 'value1', key2: 'value2'};
     const respData = {success: true};
@@ -421,7 +453,7 @@ describe('AuthorizedHttpClient', () => {
   });
 
   afterEach(() => {
-    _.forEach(mockedRequests, (mockedRequest) => mockedRequest.done());
+    mockedRequests.forEach((mockedRequest) => mockedRequest.done());
     mockedRequests = [];
     return mockApp.delete();
   });
@@ -516,9 +548,8 @@ describe('AuthorizedHttpClient', () => {
     const respData = {success: true};
     const options = {
       reqheaders: {
-        'Authorization': 'Bearer token',
         'Content-Type': (header: string) => {
-          return header.startsWith('application/json'); // auto-inserted by Axios
+          return header.startsWith('application/json'); // auto-inserted
         },
         'My-Custom-Header': 'CustomValue',
       },
@@ -544,6 +575,39 @@ describe('AuthorizedHttpClient', () => {
       expect(resp.data).to.deep.equal(respData);
     });
   });
+
+  it('should not mutate the arguments', () => {
+    const reqData = {request: 'data'};
+    const options = {
+      reqheaders: {
+        'Content-Type': (header: string) => {
+          return header.startsWith('application/json'); // auto-inserted
+        },
+        'My-Custom-Header': 'CustomValue',
+      },
+    };
+    Object.assign(options.reqheaders, requestHeaders.reqheaders);
+    const scope = nock('https://' + mockHost, options)
+      .post(mockPath, reqData)
+      .reply(200, {success: true}, {
+        'content-type': 'application/json',
+      });
+    mockedRequests.push(scope);
+    const client = new AuthorizedHttpClient(mockApp);
+    const request: HttpRequestConfig = {
+      method: 'POST',
+      url: mockUrl,
+      headers: {
+        'My-Custom-Header': 'CustomValue',
+      },
+      data: reqData,
+    };
+    const requestCopy = deepCopy(request);
+    return client.send(request).then((resp) => {
+      expect(resp.status).to.equal(200);
+      expect(request).to.deep.equal(requestCopy);
+    });
+  });
 });
 
 describe('ApiSettings', () => {