SNOW-334890 - refactor of the base httpClient

snowflakedb · Oct 5, 2023 · 63f71a8 · 63f71a8
1 parent 2d5406c
commit 63f71a8
Show file tree

Hide file tree

Showing 5 changed files with 80 additions and 125 deletions.
diff --git a/lib/authentication/auth_web.js b/lib/authentication/auth_web.js
@@ -7,7 +7,7 @@ const net = require('net');
 const querystring = require('querystring');
 const URLUtil = require('./../../lib/url_util');
 const Util = require('./../../lib/util');
-var SsoUrlProvider = require('../authentication/sso_url_provider');
+const SsoUrlProvider = require('../authentication/sso_url_provider');
 
 /**
  * Creates an external browser authenticator.
@@ -23,7 +23,7 @@ function auth_web(connectionConfig, httpClient, webbrowser) {
 
   const host = connectionConfig.host;
   const browserActionTimeout = connectionConfig.getBrowserActionTimeout();
-  const ssoUrlProvider = new SsoUrlProvider(connectionConfig, httpClient);
+  const ssoUrlProvider = new SsoUrlProvider(httpClient);
 
   if (!Util.exists(host)) {
     throw new Error(`Invalid value for host: ${host}`);

diff --git a/lib/authentication/sso_url_provider.js b/lib/authentication/sso_url_provider.js
@@ -9,13 +9,11 @@ const { rest } = require('../global_config');
 /**
  * Creates a new instance of an SsoUrlProvider.
  *
- * @param {Object} connectionConfig
  * @param {Object} httpClient
  * @constructor
  */
-function SsoUrlProvider(connectionConfig, httpClient) {
+function SsoUrlProvider(httpClient) {
 
-  Errors.assertInternal(Util.isObject(connectionConfig));
   Errors.assertInternal(Util.isObject(httpClient));
 
   const port = rest.HTTPS_PORT;
@@ -54,18 +52,17 @@ function SsoUrlProvider(connectionConfig, httpClient) {
       }
     };
 
-    const agent = httpClient.getAgent(url, connectionConfig.getProxy());
-
     const requestOptions =
       {
+        method: 'post',
+        url: url,
         headers: header,
-        requestOCSP: false,
-        rejectUnauthorized: true,
-        httpsAgent: agent
+        data: body,
+        responseType: 'json'
       };
 
     // Post request to get the SSO URL
-    return httpClient.post(url, body, requestOptions)
+    return httpClient.requestAsync(requestOptions)
       .then((response) => {
         const data = response['data']['data'];
         return data;

diff --git a/lib/connection/connection.js b/lib/connection/connection.js
@@ -236,7 +236,7 @@ function Connection(context)
     }
 
     // Get authenticator to use
-    var auth = Authenticator.getAuthenticator(connectionConfig, context.getServices().ssoUrlProvider);
+    const auth = Authenticator.getAuthenticator(connectionConfig, context.getServices().ssoUrlProvider);
 
     try
     {
@@ -297,7 +297,7 @@ function Connection(context)
     var self = this;
 
     // Get authenticator to use
-    var auth = Authenticator.getAuthenticator(connectionConfig, context.getHttpClient());
+    const auth = Authenticator.getAuthenticator(connectionConfig, context.getHttpClient());
 
     try
     {

diff --git a/lib/http/base.js b/lib/http/base.js
@@ -34,69 +34,9 @@ HttpClient.prototype.getConnectionConfig = function ()
  *
  * @returns {Object} an object representing the request that was issued.
  */
-HttpClient.prototype.request = function (options)
-{
-  // validate input
-  Errors.assertInternal(Util.isObject(options));
-  Errors.assertInternal(Util.isString(options.method));
-  Errors.assertInternal(Util.isString(options.url));
-  Errors.assertInternal(!Util.exists(options.headers) ||
-    Util.isObject(options.headers));
-  Errors.assertInternal(!Util.exists(options.json) ||
-    Util.isObject(options.json));
-  Errors.assertInternal(!Util.exists(options.callback) ||
-    Util.isFunction(options.callback));
-
-  var headers;
-  var json;
-  var body;
-  var request;
-
-  // normalize the headers
-  headers = normalizeHeaders(options.headers);
-
-  // create a function to send the request
-  var sendRequest = async function sendRequest()
-  {
-    const url = options.url;
-
-    const timeout = options.timeout ||
-      this._connectionConfig.getTimeout() ||
-      DEFAULT_REQUEST_TIMEOUT;
-
-    Logger.getInstance().trace(`CALL ${options.method} with timeout ${timeout}: ${url}`);
-    // build the basic request options
-
-    var requestOptions =
-      {
-        method: options.method,
-        url: url,
-        headers: headers,
-        gzip: options.gzip,
-        json: json,
-        body: body,
-        timeout: timeout,
-        requestOCSP: true,
-        rejectUnauthorized: true,
-        // we manually parse jsons or other structures from the server so they need to be text
-        responseType: 'text',
-      };
-
-    let mock;
-    if (this._connectionConfig.agentClass)
-    {
-      mock = {
-        agentClass: this._connectionConfig.agentClass
-      }
-    }
-
-    // add the agent and proxy options
-    const agent = this.getAgent(url, this._connectionConfig.getProxy(), mock);
-
-    requestOptions.data = requestOptions.body;
-    requestOptions.httpsAgent = agent;
-    requestOptions.retryDelay = this.constructExponentialBackoffStrategy();
-
+HttpClient.prototype.request = function (options) {
+  const requestOptions = prepareRequestOptions.call(this, options);
+  var sendRequest = async function sendRequest() {
     request = axios.request(requestOptions).then(response => {
       if (Util.isFunction(options.callback)) {
         return options.callback(null, normalizeResponse(response), response.data);
@@ -119,55 +59,34 @@ HttpClient.prototype.request = function (options)
   };
   sendRequest = sendRequest.bind(this);
 
-  // if a request body is specified and compression is enabled,
-  // try to compress the request body before sending the request
-  json = body = options.json;
-  if (body)
-  {
-    var bufferUncompressed = Buffer.from(JSON.stringify(body), 'utf8');
-    zlib.gzip(bufferUncompressed, null, function (err, bufferCompressed)
-    {
-      // if the compression was successful
-      if (!err)
-      {
-        json = undefined; // don't specify the 'json' option
-
-        // use the compressed buffer as the body and
-        // set the appropriate content encoding
-        body = bufferCompressed;
-        headers['Content-Encoding'] = 'gzip';
-      }
-      else
-      {
-        Logger.getInstance().warn('Could not compress request body.');
-      }
-
-      sendRequest();
-    });
-  }
-  else
-  {
-    if (body)
-    {
-      Logger.getInstance().trace('Request body compression disabled.');
-    }
-
-    process.nextTick(sendRequest);
-  }
+  Logger.getInstance().trace(`CALL ${requestOptions.method} with timeout ${requestOptions.timeout}: ${requestOptions.url}`);
+  process.nextTick(sendRequest);
 
   // return an externalized request object that only contains
   // methods we're comfortable exposing to the outside world
   return {
-    abort: function ()
-    {
-      if (request)
-      {
+    abort: function () {
+      if (request) {
         request.abort();
       }
     }
   };
 };
 
+/**
+ * Issues an HTTP request.
+ *
+ * @param {Object} options
+ *
+ * @returns {Object} an object representing the request that was issued.
+ */
+HttpClient.prototype.requestAsync = async function (options)
+{
+  const requestOptions = prepareRequestOptions.call(this, options);
+
+  return axios.request(requestOptions);
+};
+
 /**
  * @abstract
  * Returns the module to use when making HTTP requests. Subclasses must override
@@ -196,6 +115,53 @@ HttpClient.prototype.post = axios.post;
 
 module.exports = HttpClient;
 
+function prepareRequestOptions(options) {
+  let headers = normalizeHeaders(options.headers) || {};
+
+  const timeout = options.timeout ||
+    this._connectionConfig.getTimeout() ||
+    DEFAULT_REQUEST_TIMEOUT;
+
+  let data = options.data || options.json;
+
+  if (data) {
+    var bufferUncompressed = Buffer.from(JSON.stringify(data), 'utf8');
+    zlib.gzip(bufferUncompressed, null, function (err, bufferCompressed) {
+      // if the compression was successful
+      if (!err) {
+        data = bufferCompressed;
+        headers['Content-Encoding'] = 'gzip';
+      } else {
+        Logger.getInstance().warn('Could not compress request data.');
+      }
+    });
+  }
+  let mock;
+  if (this._connectionConfig.agentClass) {
+    mock = {
+      agentClass: this._connectionConfig.agentClass
+    }
+  }
+  const agent = this.getAgent(options.url, this._connectionConfig.getProxy(), mock);
+  const backoffStrategy = this.constructExponentialBackoffStrategy();
+
+  const requestOptions =
+    {
+      method: options.method,
+      url: options.url,
+      headers: headers,
+      data: data,
+      timeout: timeout,
+      requestOCSP: true,
+      httpsAgent: agent,
+      retryDelay: backoffStrategy,
+      rejectUnauthorized: true,
+      // we manually parse jsons or other structures from the server so they need to be text
+      responseType: options.responseType || 'text',
+    };
+  return requestOptions;
+}
+
 /**
  * Normalizes a request headers object so that we get the same behavior
  * regardless of whether we're using request.js or browser-request.js.

diff --git a/test/unit/authentication/authentication_test.js b/test/unit/authentication/authentication_test.js
@@ -92,11 +92,7 @@ describe('external browser authentication', function ()
       }
     });
     mock('httpclient', {
-      getAgent: function (url, body, header)
-      {
-        return new HttpAgent();
-      },
-      post: async function (url, body, header)
+      requestAsync: async function (options)
       {
         const data =
           {
@@ -152,11 +148,7 @@ describe('external browser authentication', function ()
     });
 
     mock('httpclient', {
-      getAgent: function (url, body, header)
-      {
-        return new HttpAgent();
-      },
-      post: async function (url, body, header)
+      requestAsync: async function (options)
       {
         const data =
           {