From 6d27e7d54dacbb0d859050784fe54dae0d0a2cba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Paw=C5=82owski?= Date: Fri, 6 Dec 2024 15:33:02 +0100 Subject: [PATCH] SNOW-1801434 add GUID to request in node.js driver (#965) --- index.d.ts | 8 ++ lib/constants/sf_params.js | 2 + lib/http/request_util.js | 23 +++- lib/services/sf.js | 33 +++++- test/integration/testRequestParams.js | 72 +++++++++++ test/integration/testUtil.js | 31 +++-- .../test_utils/httpInterceptorUtils.js | 112 ++++++++++++++++++ test/unit/mock/mock_http_client.js | 43 +++++-- 8 files changed, 297 insertions(+), 27 deletions(-) create mode 100644 lib/constants/sf_params.js create mode 100644 test/integration/testRequestParams.js create mode 100644 test/integration/test_utils/httpInterceptorUtils.js diff --git a/index.d.ts b/index.d.ts index f687dbb02..f9ee49d5c 100644 --- a/index.d.ts +++ b/index.d.ts @@ -615,6 +615,14 @@ declare module 'snowflake-sdk' { */ requestId?: string; + /** + * The request GUID is a unique identifier of an HTTP request issued to Snowflake. + * Unlike the requestId, it is regenerated even when the request is resend with the retry mechanism. + * If not specified, request GUIDs are attached to all requests to Snowflake for better traceability. + * In the majority of cases it should not be set or filled with false value. + */ + excludeGuid?: string; + /** * Use different rest endpoints based on whether the query id is available. */ diff --git a/lib/constants/sf_params.js b/lib/constants/sf_params.js new file mode 100644 index 000000000..cd8cdf400 --- /dev/null +++ b/lib/constants/sf_params.js @@ -0,0 +1,2 @@ +exports.paramsNames = {}; +exports.paramsNames.SF_REQUEST_GUID = 'request_guid'; diff --git a/lib/http/request_util.js b/lib/http/request_util.js index a86db249a..0a778d5e5 100644 --- a/lib/http/request_util.js +++ b/lib/http/request_util.js @@ -1,3 +1,4 @@ +const sfParams = require('../constants/sf_params'); /** * Should work with not yet parsed options as well (before calling prepareRequestOptions method). @@ -6,7 +7,7 @@ * @returns {string} */ exports.describeRequestFromOptions = function (requestOptions) { - return exports.describeRequestData({ method: requestOptions.method, url: requestOptions.url }); + return exports.describeRequestData({ method: requestOptions.method, url: requestOptions.url, guid: requestOptions.params?.[sfParams.paramsNames.SF_REQUEST_GUID] }); }; /** @@ -16,11 +17,21 @@ exports.describeRequestFromOptions = function (requestOptions) { * @returns {string} */ exports.describeRequestFromResponse = function (response) { - // Uppercase is used to allow finding logs corresponding to the same request - response pair, - // by matching identical options' descriptions. - return exports.describeRequestData({ method: response.config?.method.toUpperCase(), url: response.config?.url }); + let methodUppercase = undefined; + let url = undefined; + let guid = undefined; + const responseConfig = response.config; + if (responseConfig) { + // Uppercase is used to allow finding logs corresponding to the same request - response pair, + // by matching identical options' descriptions. + methodUppercase = responseConfig.method.toUpperCase(); + url = responseConfig.url; + guid = responseConfig.params?.[sfParams.paramsNames.SF_REQUEST_GUID]; + } + return exports.describeRequestData({ method: methodUppercase, url: url, guid: guid }); }; -exports.describeRequestData = function (requestData) { - return `[method: ${requestData.method}, url: ${requestData.url}]`; +exports.describeRequestData = function ({ method, url, guid } = {}) { + const guidDescription = guid ? `, guid: ${guid}` : ''; + return `[method: ${method}, url: ${url}` + guidDescription + ']'; }; \ No newline at end of file diff --git a/lib/services/sf.js b/lib/services/sf.js index 4a1ce42a9..2e339c204 100644 --- a/lib/services/sf.js +++ b/lib/services/sf.js @@ -60,6 +60,7 @@ const AuthenticationTypes = require('../authentication/authentication_types'); const AuthOkta = require('../authentication/auth_okta'); const AuthKeypair = require('../authentication/auth_keypair'); const Authenticator = require('../authentication/authentication'); +const sfParams = require('../constants/sf_params'); function isRetryableNetworkError(err) { // anything other than REVOKED error can be retryable. @@ -578,10 +579,16 @@ function StateAbstract(options) { * * @param {Object} requestOptions * @param {Object} httpClient - * + * @param {Object} auth * @returns {Object} the http request object. */ function sendHttpRequest(requestOptions, httpClient, auth) { + + const params = requestOptions.params || {}; + if (!requestOptions.excludeGuid) { + addGuidToParams(params); + } + const realRequestOptions = { method: requestOptions.method, @@ -589,6 +596,7 @@ function StateAbstract(options) { url: requestOptions.absoluteUrl, gzip: requestOptions.gzip, json: requestOptions.json, + params: params, callback: async function (err, response, body) { // if we got an error, wrap it into a network error if (err) { @@ -679,8 +687,8 @@ function StateAbstract(options) { }; if (requestOptions.retry > 2) { - const includeParam = requestOptions.url.includes('?'); - realRequestOptions.url += (includeParam ? '&' : '?'); + const includesParam = requestOptions.url.includes('?'); + realRequestOptions.url += (includesParam ? '&' : '?'); realRequestOptions.url += ('clientStartTime=' + requestOptions.startTime + '&' + 'retryCount=' + (requestOptions.retry - 1)); @@ -703,7 +711,7 @@ function StateAbstract(options) { /////////////////////////////////////////////////////////////////////////// /** - * Creates a new Request. + * Creates a new Request to Snowflake. * * @param {Object} requestOptions * @constructor @@ -721,18 +729,31 @@ function StateAbstract(options) { // pre-process the request options this.preprocessOptions(this.requestOptions); + const params = this.requestOptions.params || {}; + if (!this.requestOptions.excludeGuid) { + addGuidToParams(params); + } const options = { method: this.requestOptions.method, headers: this.requestOptions.headers, url: this.requestOptions.absoluteUrl, - json: this.requestOptions.json + json: this.requestOptions.json, + params: params }; // issue the async http request + //TODO: this should be wrapped with the same operations, as in the synchronous send method's callback. return await httpClient.requestAsync(options); }; + function addGuidToParams(params) { + // In case of repeated requests for the same request ID, + // the Global UID is added for better traceability. + const guid = uuidv4(); + params[sfParams.paramsNames.SF_REQUEST_GUID] = guid; + } + /** * Sends out the request. * @@ -765,6 +786,8 @@ function StateAbstract(options) { // augment the options with the absolute url requestOptions.absoluteUrl = this.buildFullUrl(requestOptions.url); + + requestOptions.excludeGuid = !Util.exists(requestOptions.excludeGuid) ? false : requestOptions.excludeGuid; }; /** diff --git a/test/integration/testRequestParams.js b/test/integration/testRequestParams.js new file mode 100644 index 000000000..f4eb0fd36 --- /dev/null +++ b/test/integration/testRequestParams.js @@ -0,0 +1,72 @@ +/* + * Copyright (c) 2015-2024 Snowflake Computing Inc. All rights reserved. + */ + +const testUtil = require('./testUtil'); +const assert = require('assert'); +const httpInterceptorUtils = require('./test_utils/httpInterceptorUtils'); +const Core = require('../../lib/core'); +const Util = require('../../lib/util'); + +describe('SF service tests', function () { + const selectPiTxt = 'select PI();'; + let interceptors; + let coreInstance; + + before(async function () { + interceptors = new httpInterceptorUtils.Interceptors(); + const HttpClientClassWithInterceptors = httpInterceptorUtils.getHttpClientWithInterceptorsClass(interceptors); + coreInstance = Core({ + httpClientClass: HttpClientClassWithInterceptors, + loggerClass: require('./../../lib/logger/node'), + client: { + version: Util.driverVersion, + name: Util.driverName, + environment: process.versions, + }, + }); + }); + + it('GUID called for all', async function () { + let guidAddedWhenExpected = true; + let totalCallsWithGUIDCount = 0; + let expectedCallsWithGUIDCount = 0; + const pathsExpectedToIncludeGuid = [ + 'session?delete=true&requestId', + 'queries/v1/query-request', + 'session/v1/login-request' + ]; + + function countCallsWithGuid(requestOptions) { + pathsExpectedToIncludeGuid.forEach((value) => { + if (requestOptions.url.includes(value)) { + // Counting is done instead of assertions, because we do not want to interrupt + // the flow of operations inside the HttpClient. Retries and other custom exception handling could be triggered. + if (!testUtil.isGuidInRequestOptions(requestOptions)) { + guidAddedWhenExpected = false; + } + expectedCallsWithGUIDCount++; + } + }); + + if (testUtil.isGuidInRequestOptions(requestOptions)) { + totalCallsWithGUIDCount++; + } + } + interceptors.add('request', httpInterceptorUtils.HOOK_TYPE.FOR_ARGS, countCallsWithGuid); + + const connection = testUtil.createConnection({}, coreInstance); + await testUtil.connectAsync(connection); + await testUtil.executeCmdAsync(connection, selectPiTxt); + + const guidCallsOccurred = totalCallsWithGUIDCount > 0; + assert.strictEqual(guidCallsOccurred, true, 'No GUID calls occurred'); + assert.strictEqual(guidAddedWhenExpected, true, `GUID not found in all requests with paths: ${pathsExpectedToIncludeGuid}`); + assert.strictEqual(expectedCallsWithGUIDCount === totalCallsWithGUIDCount, true, `GUID was added to requests not included in the expected paths: ${pathsExpectedToIncludeGuid}.` + + `Total calls with guid: ${totalCallsWithGUIDCount}. Expected calls with guid: ${expectedCallsWithGUIDCount}.` + ); + + await testUtil.destroyConnectionAsync(connection); + interceptors.clear(); + }); +}); diff --git a/test/integration/testUtil.js b/test/integration/testUtil.js index c15b61449..dd836cc40 100644 --- a/test/integration/testUtil.js +++ b/test/integration/testUtil.js @@ -12,19 +12,31 @@ const Logger = require('../../lib/logger'); const path = require('path'); const os = require('os'); -module.exports.createConnection = function (validConnectionOptionsOverride = {}) { - return snowflake.createConnection({ +module.exports.createConnection = function (validConnectionOptionsOverride = {}, coreInstance) { + coreInstance = coreInstance ? coreInstance : snowflake; + + return coreInstance.createConnection({ ...connOptions.valid, ...validConnectionOptionsOverride, }); }; -module.exports.createProxyConnection = function () { - return snowflake.createConnection(connOptions.connectionWithProxy); +module.exports.createProxyConnection = function (validConnectionOptionsOverride, coreInstance) { + coreInstance = coreInstance ? coreInstance : snowflake; + + return coreInstance.createConnection({ + ...connOptions.connectionWithProxy, + ...validConnectionOptionsOverride + }); }; -module.exports.createConnectionPool = function () { - return snowflake.createPool(connOptions.valid, { max: 10, min: 0, testOnBorrow: true }); +module.exports.createConnectionPool = function (validConnectionOptionsOverride, coreInstance) { + coreInstance = coreInstance ? coreInstance : snowflake; + + return coreInstance.createPool({ + ...connOptions.valid, + ...validConnectionOptionsOverride + }, { max: 10, min: 0, testOnBorrow: true }); }; module.exports.connect = function (connection, callback) { @@ -355,6 +367,9 @@ module.exports.assertActiveConnectionDestroyedCorrectlyAsync = async function (c module.exports.assertConnectionInactive(connection); }; - module.exports.normalizeRowObject = normalizeRowObject; -module.exports.normalizeValue = normalizeValue; \ No newline at end of file +module.exports.normalizeValue = normalizeValue; + +module.exports.isGuidInRequestOptions = function (requestOptions) { + return requestOptions.url.includes('request_guid') || 'request_guid' in requestOptions.params; +}; \ No newline at end of file diff --git a/test/integration/test_utils/httpInterceptorUtils.js b/test/integration/test_utils/httpInterceptorUtils.js new file mode 100644 index 000000000..408896938 --- /dev/null +++ b/test/integration/test_utils/httpInterceptorUtils.js @@ -0,0 +1,112 @@ +const Logger = require('../../../lib/logger'); +const { NodeHttpClient } = require('../../../lib/http/node'); +const Util = require('../../../lib/util'); + +const HOOK_TYPE = { + FOR_ARGS: 'args', + FOR_RETURNED_VALUE: 'returned', +}; + +module.exports.HOOK_TYPE = HOOK_TYPE; + +class Interceptor { + constructor(methodName, hookType, callback) { + this.methodName = methodName; + this.hookType = hookType || HOOK_TYPE.FOR_ARGS; + this.callback = callback; + } + + execute(...args) { + this.callback(...args); + } +} + +class Interceptors { + constructor(initialInterceptors) { + this.interceptorsMap = this.createInterceptorsMap(initialInterceptors); + } + + add(methodName, hookType, callback, interceptor = undefined) { + if (!interceptor) { + interceptor = new Interceptor(methodName, hookType, callback); + } + this.interceptorsMap[interceptor.methodName][interceptor.hookType] = interceptor; + } + + get(methodName, hookType) { + return this.interceptorsMap[methodName][hookType]; + } + + intercept(methodName, hookType, ...args) { + // When no interceptor registered - ignores and does not raise any error + try { + return this.get(methodName, hookType)?.execute(...args); + } catch (e) { + throw 'Unable to execute interceptor method in tests. Error: ' + e; + } + } + + clear() { + this.interceptorsMap = this.createInterceptorsMap(); + } + + createInterceptorsMap(initialInterceptors = {}) { + if (initialInterceptors instanceof Interceptors) { + return initialInterceptors.interceptorsMap; + } + // Map creating another map for each accessed key not present in the map + // (analogy - DefaultDict from Python). + return new Proxy(initialInterceptors, { + get: (target, prop) => { + if (prop in target) { + return target[prop]; + } else { + // Create an empty object, store it in target, and return it + const newObj = {}; + target[prop] = newObj; + return newObj; + } + } + }); + } +} + +module.exports.Interceptors = Interceptors; + +function HttpClientWithInterceptors(connectionConfig, initialInterceptors) { + Logger.getInstance().trace('Initializing HttpClientWithInterceptors with Connection Config[%s]', + connectionConfig.describeIdentityAttributes()); + this.interceptors = new Interceptors(initialInterceptors); + NodeHttpClient.apply(this, [connectionConfig]); +} + +Util.inherits(HttpClientWithInterceptors, NodeHttpClient); + + +HttpClientWithInterceptors.prototype.requestAsync = async function (url, options) { + this.interceptors.intercept('requestAsync', HOOK_TYPE.FOR_ARGS, url, options); + const response = await NodeHttpClient.prototype.requestAsync.call(this, url, options); + this.interceptors.intercept('requestAsync', HOOK_TYPE.FOR_RETURNED_VALUE, response); + return response; +}; + +HttpClientWithInterceptors.prototype.request = function (url, options) { + this.interceptors.intercept('request', HOOK_TYPE.FOR_ARGS, url, options); + const response = NodeHttpClient.prototype.request.call(this, url, options); + this.interceptors.intercept('request', HOOK_TYPE.FOR_RETURNED_VALUE, response); + return response; +}; + +// Factory method for HttpClientWithInterceptors to be able to partially initialize class +// with interceptors used in fully instantiated object. +function getHttpClientWithInterceptorsClass(interceptors) { + function HttpClientWithInterceptorsWrapper(connectionConfig) { + HttpClientWithInterceptors.apply(this, [connectionConfig, interceptors]); + } + Util.inherits(HttpClientWithInterceptorsWrapper, HttpClientWithInterceptors); + + return HttpClientWithInterceptorsWrapper; +} + + +module.exports.getHttpClientWithInterceptorsClass = getHttpClientWithInterceptorsClass; diff --git a/test/unit/mock/mock_http_client.js b/test/unit/mock/mock_http_client.js index afd2f2a93..73546a7d3 100644 --- a/test/unit/mock/mock_http_client.js +++ b/test/unit/mock/mock_http_client.js @@ -36,14 +36,13 @@ MockHttpClient.prototype.request = function (request) { this._mapRequestToOutput = buildRequestToOutputMap(buildRequestOutputMappings(this._clientInfo)); } + removeParamFromRequestUrl(request, 'request_guid'); + removeParamFromRequestParams(request, 'request_guid'); // Closing a connection includes a requestID as a query parameter in the url // Example: http://fake504.snowflakecomputing.com/session?delete=true&requestId=a40454c6-c3bb-4824-b0f3-bae041d9d6a2 if (request.url.includes('session?delete=true') || request.url.includes('session/heartbeat?requestId=')) { - // Offset for the query character preceding the 'requestId=' string in URL (either '?' or '&') - const PRECEDING_QUERY_CHAR_OFFSET = 1; - // Remove the requestID query parameter for the mock HTTP client - request.url = request.url.substring(0, request.url.indexOf('requestId=') - PRECEDING_QUERY_CHAR_OFFSET); + removeParamFromRequestUrl(request, 'requestId'); } // get the output of the specified request from the map @@ -84,14 +83,13 @@ MockHttpClient.prototype.requestAsync = function (request) { this._mapRequestToOutput = buildRequestToOutputMap(buildRequestOutputMappings(this._clientInfo)); } + removeParamFromRequestUrl(request, 'request_guid'); + removeParamFromRequestParams(request, 'request_guid'); // Closing a connection includes a requestID as a query parameter in the url // Example: http://fake504.snowflakecomputing.com/session?delete=true&requestId=a40454c6-c3bb-4824-b0f3-bae041d9d6a2 if (request.url.includes('session?delete=true') || request.url.includes('session/heartbeat?requestId=')) { - // Offset for the query character preceding the 'requestId=' string in URL (either '?' or '&') - const PRECEDING_QUERY_CHAR_OFFSET = 1; - // Remove the requestID query parameter for the mock HTTP client - request.url = request.url.substring(0, request.url.indexOf('requestId=') - PRECEDING_QUERY_CHAR_OFFSET); + removeParamFromRequestUrl(request, 'requestId'); } // get the output of the specified request from the map @@ -175,6 +173,35 @@ function createSortedClone(target) { return sortedClone; } +function removeParamFromRequestUrl(request, paramName) { + try { + // Use the URL constructor to parse the URL + const urlObj = new URL(request.url); + urlObj.searchParams.delete(paramName); + request.url = urlObj.toString(); + } catch (error) { + // Handle invalid URLs or other errors + throw `Invalid URL: ${request.url} Error: ${error}`; + } +} + +/** + * Removes a parameter from the params object of a request. + * If the params object becomes empty after the deletion, removes the params property entirely. + */ +function removeParamFromRequestParams(request, paramName) { + if (request && request.params && typeof request.params === 'object') { + // Delete the specified parameter + delete request.params[paramName]; + + // Check if params is now empty + if (Object.keys(request.params).length === 0) { + // Remove the entire params property + delete request.params; + } + } +} + /** * Returns an array of objects, each of which has a request and output property. *