SNOW-3055676 Remove smkId string conversion

CHANGELOG.md

@@ -17,6 +17,7 @@ Internal:
 - Extended login-request `PLATFORM` telemetry to detect cloud VMs, managed identities, and GitHub Actions in addition to serverless environments (snowflakedb/snowflake-connector-nodejs#1386)
 - The login-request now requests `sessionId` as a string to avoid precision loss (snowflakedb/snowflake-connector-nodejs#1384)
 - Removed dead browser-related code and `browser-request` dependency (snowflakedb/snowflake-connector-nodejs#1387)
+- Removed `smkId` string conversion; the driver now requests the server to return it as a string (snowflakedb/snowflake-connector-nodejs#1344)
 
 ## 2.4.0
 

lib/http/base.js

@@ -186,15 +186,6 @@ function parseIfJSONData(response) {
   // SNOW-3157996 content-type can also have an optional parameter, type/subtype;parameter=value
   const isJson = response['headers']['content-type']?.toLowerCase().includes('application/json');
   if (Util.isString(response['data']) && isJson) {
-    // NOTE:
-    // A super hacky solution to patch /queries/v1/query-request response for PUT/GET operations
-    // Returned encryptionMaterial.smkId is an integer, that might be higher than Number.MAX_SAFE_INTEGER
-    // Treating it as integer will result in uploaded files being UNRECOVERABLE
-    //
-    // This should be replaced with new API field that is a string
-    if (response['data'].includes('smkId')) {
-      response['data'] = Util.convertSmkIdToString(response['data']);
-    }
     response['json'] = response['data'];
     response['data'] = JSON.parse(response['data']);
   }

lib/services/sf.js

@@ -1088,6 +1088,7 @@ StateConnecting.prototype.continue = async function () {
     CLIENT_APP_ID: this.connectionConfig.getClientType(),
     CLIENT_APP_VERSION: this.connectionConfig.getClientVersion(),
     CLIENT_CAPABILITIES: {
+      SMK_ID_AS_STRING: true,
       SESSION_ID_AS_STRING: true,
     },
     CLIENT_ENVIRONMENT: {

lib/util.ts

@@ -355,17 +355,6 @@ export function isGetCommand(sqlText: string) {
   return sqlText.trim().substring(0, 3).toUpperCase() === 'GET';
 }
 
-/**
- * Add double quotes to smkId's value to parse it as a string instead of integer
- * to preserve precision of numbers exceeding JavaScript's max safe integer
- * e.g (inputting 32621973126123526	outputs 32621973126123530)
- *
- * @param body the data in JSON
- */
-export function convertSmkIdToString(body: string) {
-  return body.replace(/"smkId"(\s*):(\s*)([0-9]+)/g, '"smkId"$1:$2"$3"');
-}
-
 /**
  * Under some circumstances the object passed to JSON.stringify in exception handling
  * can contain circular reference, on which JSON.stringify bails out

test/integration/testLoginRequestBody.ts

@@ -144,6 +144,13 @@ describe('/login-request body', () => {
     });
   });
 
+  it('includes required capabilities', async () => {
+    await initConnection();
+    assert.deepStrictEqual(getLoginRequestData().CLIENT_CAPABILITIES, {
+      SMK_ID_AS_STRING: true,
+    });
+  });
+
   describe('SPCS_TOKEN', () => {
     it('is not included when SNOWFLAKE_RUNNING_INSIDE_SPCS is not set', async () => {
       await initConnection();

test/integration/testPutSmkIdString.ts

@@ -0,0 +1,96 @@
+import assert from 'assert';
+import rewiremock from 'rewiremock/node';
+import os from 'os';
+import path from 'path';
+import fs from 'fs';
+import zlib from 'zlib';
+
+const OriginalFileTransferAgent = require('../../lib/file_transfer_agent/file_transfer_agent');
+
+// NOTE:
+// Keeping this test just in case we get issues with CLIENT_CAPABILITIES.
+// DO NOT migrate this test to the Universal Driver.
+describe('smkId in PUT statements', () => {
+  let testUtil: any;
+  let fileTransferAgentUsedContext: any;
+
+  const FILE_CONTENT = 'smkId-string-test-content\n';
+  const STAGE_PATH = '@~/test_smkId_in_put';
+
+  function toPlatformPath(filePath: string) {
+    return process.platform === 'win32'
+      ? `${process.env.USERPROFILE}\\AppData\\Local\\Temp\\${path.basename(filePath)}`
+      : filePath;
+  }
+
+  function createTmpFileWithContent() {
+    const tmpFile = testUtil.createTempFile(
+      os.tmpdir(),
+      testUtil.createRandomFileName(),
+      FILE_CONTENT,
+    );
+    return toPlatformPath(tmpFile);
+  }
+
+  function createTmpDir() {
+    const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'get-smkId-'));
+    return { realPath: tmpDir, platformPath: toPlatformPath(tmpDir) };
+  }
+
+  before(() => {
+    rewiremock('../../lib/file_transfer_agent/file_transfer_agent').with(function (context: any) {
+      // Context mutates, so we deep clone it
+      fileTransferAgentUsedContext = JSON.parse(JSON.stringify(context));
+      return new OriginalFileTransferAgent(context);
+    });
+    rewiremock.enable();
+    testUtil = require('./testUtil');
+  });
+
+  after(() => {
+    rewiremock.disable();
+  });
+
+  it('FileTransferAgent receives smkId as string', async () => {
+    const connection = testUtil.createConnection();
+    const putFilePath = createTmpFileWithContent();
+    const { realPath: getDir, platformPath: getDirForQuery } = createTmpDir();
+    const fileName = path.basename(putFilePath);
+
+    await testUtil.connectAsync(connection);
+    try {
+      await testUtil.executeCmdAsync(connection, `PUT file://${putFilePath} ${STAGE_PATH}`);
+      assert.strictEqual(
+        typeof fileTransferAgentUsedContext.fileMetadata.data.encryptionMaterial.smkId,
+        'string',
+        'smkId should be a string',
+      );
+
+      await testUtil.executeCmdAsync(
+        connection,
+        `GET ${STAGE_PATH}/${fileName}.gz file://${getDirForQuery}`,
+      );
+      assert.strictEqual(
+        typeof fileTransferAgentUsedContext.fileMetadata.data.encryptionMaterial[0].smkId,
+        'string',
+        'smkId should be a string on GET as well',
+      );
+
+      const downloadedFile = path.join(getDir, `${fileName}.gz`);
+      assert.ok(fs.existsSync(downloadedFile), `Downloaded file should exist at ${downloadedFile}`);
+
+      const decompressed = zlib.gunzipSync(fs.readFileSync(downloadedFile)).toString();
+      assert.strictEqual(
+        decompressed,
+        FILE_CONTENT,
+        'Downloaded file content should match the uploaded content',
+      );
+    } finally {
+      await testUtil
+        .executeCmdAsync(connection, `REMOVE ${STAGE_PATH}/${fileName}.gz`)
+        .catch(() => undefined);
+      testUtil.deleteFileSyncIgnoringErrors?.(putFilePath);
+      fs.rmSync(getDir, { recursive: true, force: true });
+    }
+  });
+});

test/unit/util_test.js

@@ -962,27 +962,4 @@ describe('Util', function () {
       }
     });
   });
-
-  describe('Util.convertSmkIdToString()', function () {
-    it('should convert smkId numbers to strings in JSON', function () {
-      const input = '{"smkId" : 1234567890123456789, "otherKey" : "value"}';
-      const expectedOutput = '{"smkId" : "1234567890123456789", "otherKey" : "value"}';
-      const result = Util.convertSmkIdToString(input);
-      assert.strictEqual(result, expectedOutput);
-    });
-
-    it('should not modify JSON without smkId', function () {
-      const input = '{"otherKey" : "value"}';
-      const expectedOutput = '{"otherKey" : "value"}';
-      const result = Util.convertSmkIdToString(input);
-      assert.strictEqual(result, expectedOutput);
-    });
-
-    it('should convert smkId numbers to strings in JSON whitespace insensitive', function () {
-      const input = '{"smkId":1234567890123456789,"otherKey":"value"}';
-      const expectedOutput = '{"smkId":"1234567890123456789","otherKey":"value"}';
-      const result = Util.convertSmkIdToString(input);
-      assert.strictEqual(result, expectedOutput);
-    });
-  });
 });