From 1bae9ce4e7769bb803154924121f817ee14caba1 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf <115584722+sfc-gh-ext-simba-lf@users.noreply.github.com> Date: Fri, 6 Oct 2023 09:37:26 -0700 Subject: [PATCH] SNOW-728803: Do not replace sqlText if it is specified by the user (#639) * SNOW-728803: Do not replace sqlText if it is specified by the user * SNOW-728803: Reformat with eslint * SNOW-728803: Add test for special case when resubmitting requets * SNOW-728803: Modify request to include queryContextDTO property * SNOW-728803: Replace done(err) with callback(err) * SNOW-728803: Wrap in else clause * SNOW-728803: Set requestId the same for QA and non-QA mode * SNOW-728803: Fix spacing of bracket * SNOW-728803: Add test case for sqlText being overwritten * SNOW-728803: Clean up tests * SNOW-728803: Remove async series --- lib/connection/statement.js | 38 ++++------------ test/unit/mock/mock_http_client.js | 73 ++++++++++++++++++++++++++++++ test/unit/snowflake_test.js | 67 +++++++++++++++++++++++++++ 3 files changed, 150 insertions(+), 28 deletions(-) diff --git a/lib/connection/statement.js b/lib/connection/statement.js index 3245fefd4..130d6bc30 100644 --- a/lib/connection/statement.js +++ b/lib/connection/statement.js @@ -397,29 +397,14 @@ function createContextPreExec( Errors.assertInternal(Util.isObject(services)); Errors.assertInternal(Util.isObject(connectionConfig)); - // if we're not in qa mode, use a random uuid for the statement request id - // or use request id passed by user - if (!connectionConfig.isQaMode()) - { - if (statementOptions.requestId) - { - statementContext.requestId = statementOptions.requestId; - statementContext.resubmitRequest = true; - } - else - { - statementContext.requestId = uuidv4(); - } + // use request id passed by user + if (statementOptions.requestId) { + statementContext.requestId = statementOptions.requestId; + statementContext.resubmitRequest = true; } - else // we're in qa mode - { - // if a request id or sequence id are specified in the statement options, - // use them as is; this is to facilitate testing by making things more - // deterministic - if (Util.isString(statementOptions.requestId)) - { - statementContext.requestId = statementOptions.requestId; - } + else { + // use a random uuid for the statement request id + statementContext.requestId = uuidv4(); } return statementContext; @@ -1310,12 +1295,9 @@ function sendRequestPreExec(statementContext, onResultAvailable) { disableOfflineChunks: false, }; - if (!statementContext.resubmitRequest) - { - json.sqlText = statementContext.sqlText; - } - else - { + json.sqlText = statementContext.sqlText; + + if (statementContext.resubmitRequest && !json.sqlText) { json.sqlText = `SELECT 'Error retrieving query results for request id: ${statementContext.requestId}, ` + `please use RESULT_SCAN instead' AS ErrorMessage;`; } diff --git a/test/unit/mock/mock_http_client.js b/test/unit/mock/mock_http_client.js index 47de2823a..05d3c346b 100644 --- a/test/unit/mock/mock_http_client.js +++ b/test/unit/mock/mock_http_client.js @@ -700,6 +700,79 @@ function buildRequestOutputMappings(clientInfo) } } }, + { + request: + { + method: 'POST', + url: 'http://fakeaccount.snowflakecomputing.com/queries/v1/query-request?requestId=SNOW-728803-requestId', + headers: + { + 'Accept': 'application/snowflake', + 'Authorization': 'Snowflake Token="SESSION_TOKEN"', + 'Content-Type': 'application/json' + }, + json: + { + disableOfflineChunks: false, + sqlText: 'select 1;', + queryContextDTO: { entries: [] } + } + }, + output: + { + err: null, + response: + { + statusCode: 200, + statusMessage: 'OK', + body: + { + 'data': + { + 'parameters': [], + 'rowtype': [], + 'rowset': [['1']], + 'total': 1, + 'returned': 1 + }, + 'message': null, + 'code': null, + 'success': true + } + } + } + }, + { + request: + { + method: 'POST', + url: 'http://fakeaccount.snowflakecomputing.com/queries/v1/query-request?requestId=SNOW-728803-requestId', + headers: + { + 'Accept': 'application/snowflake', + 'Authorization': 'Snowflake Token="SESSION_TOKEN"', + 'Content-Type': 'application/json' + }, + json: + { + disableOfflineChunks: false, + sqlText: 'SELECT \'Error retrieving query results for request id: SNOW-728803-requestId, please use RESULT_SCAN instead\' AS ErrorMessage;', + queryContextDTO: { entries: [] } + } + }, + output: + { + err: null, + response: + { + body: + { + 'message': 'The specified sqlText should not be overwritten when resubmitting the request', + 'success': false + } + } + } + }, { request: { diff --git a/test/unit/snowflake_test.js b/test/unit/snowflake_test.js index a50c7de05..cebf544ed 100644 --- a/test/unit/snowflake_test.js +++ b/test/unit/snowflake_test.js @@ -936,6 +936,73 @@ describe('connection.execute() statement failure', function () }); }); +describe('connection.execute() with requestId', function () { + const connection = snowflake.createConnection(connectionOptions); + const sqlText = 'select 1;'; + const blankSqlText = ''; + const requestId = 'SNOW-728803-requestId'; + + before(function (done) { + connection.connect(function (err, conn) { + assert.ok(!err, 'there should be no error'); + assert.strictEqual(conn, connection, + 'the connect() callback should be invoked with the statement'); + + done(); + }); + }); + + it('keep original sqlText when resubmitting requests', function (done) { + // request with sqlText and requestId specified + const statement = connection.execute( + { + sqlText: sqlText, + requestId: requestId, + complete: function (err, stmt) { + // if there's an error, fail the test with the error + if (err) { + done(err); + } + else { + assert.ok(!err, 'there should be no error'); + assert.strictEqual(stmt, statement, + 'the execute() callback should be invoked with the statement'); + + // the sql text and request id should be the same as what was passed + // in + assert.strictEqual(statement.getSqlText(), sqlText); + assert.strictEqual(statement.getRequestId(), requestId); + + done(); + } + } + }); + }); + + it('sqlText is overwritten when resubmitting requests', function (done) { + // request with only requestId specified + const statement = connection.execute( + { + // intentionally leave sqlText blank to invoke the connector to overwrite the sqlText + sqlText: blankSqlText, + requestId: requestId, + complete: function (err, stmt) { + assert.ok(err, 'there should be an error'); + assert.strictEqual(stmt, statement, + 'the execute() callback should be invoked with the statement'); + + // the sql text and request id should be the same as what was passed + // in + assert.strictEqual(stmt.getRequestId(), requestId); + // the sqlText on the statement is unchanged but the sqlText on the request is different + assert.strictEqual(stmt.getSqlText(), blankSqlText); + + done(); + } + }); + }); +}); + describe('too many concurrent requests', function () { it('too many concurrent requests per user', function (done)