SNOW-334890 - refactor of the base httpClient

snowflakedb · Oct 5, 2023 · 87b6c4b · 87b6c4b
1 parent 2d5406c
commit 87b6c4b
Show file tree

Hide file tree

Showing 6 changed files with 88 additions and 129 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
@@ -7,6 +7,7 @@ const Util = require('../util');
 const Errors = require('../errors');
 const Logger = require('../logger');
 const axios = require('axios');
+const Url = require("url");
 
 const DEFAULT_REQUEST_TIMEOUT = 360000;
 
@@ -34,69 +35,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);
+  let 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 +60,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 +116,59 @@ 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 backoffStrategy = this.constructExponentialBackoffStrategy();
+  const requestOptions =  {
+    method: options.method,
+    url: options.url,
+    headers: headers,
+    data: data,
+    timeout: timeout,
+    requestOCSP: true,
+    retryDelay: backoffStrategy,
+    rejectUnauthorized: true,
+    // we manually parse jsons or other structures from the server so they need to be text
+    responseType: options.responseType || 'text',
+  }
+
+  const parsedUrl = Url.parse(url);
+  const isHttps = parsedUrl.protocol === 'https:';
+  const agent = this.getAgent(options.url, isHttps, this._connectionConfig.getProxy(), mock);
+  if (isHttps) {
+    requestOptions.httpsAgent = agent;
+  } else {
+    requestOptions.httpAgent = agent;
+  }
+
+  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/lib/http/node.js b/lib/http/node.js
@@ -2,7 +2,6 @@
  * Copyright (c) 2015-2019 Snowflake Computing Inc. All rights reserved.
  */
 
-const Url = require('url');
 const Util = require('../util');
 const Base = require('./base');
 const HttpsAgent = require('../agent/https_ocsp_agent');
@@ -94,14 +93,12 @@ function isBypassProxy(proxy, url, bypassProxy) {
 /**
  * @inheritDoc
  */
-NodeHttpClient.prototype.getAgent = function (url, proxy, mock) {
+NodeHttpClient.prototype.getAgent = function (url, isHttps, proxy, mock) {
   const agentOptions = {keepAlive: GlobalConfig.getKeepAlive()};
   if (mock) {
     return mock.agentClass(agentOptions);
   }
 
-  const parsedUrl = Url.parse(url);
-  const isHttps = parsedUrl.protocol === 'https:';
   const bypassProxy = isBypassProxy(proxy, url);
   let agent = {};
 

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 =
           {