From c9239a9be15fc63890d570415845f75e80fff6d7 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" <3815206+peco-engineer-bot[bot]@users.noreply.github.com> Date: Fri, 6 Nov 2026 22:07:37 +0000 Subject: [PATCH 01/10] Batch INSERT (using array of arrays) results in the application hanging (#261) Signed-off-by: peco-engineer-bot[bot] <3815206+peco-engineer-bot[bot]@users.noreply.github.com> Signed-off-by: Vu Anh Phung --- lib/thrift-backend/ThriftSessionBackend.ts | 7 ++- tests/e2e/query_parameters.test.ts | 66 ++++++++++++++++++++++ tests/unit/DBSQLSession.test.ts | 50 ++++++++++++++++ 3 files changed, 121 insertions(+), 2 deletions(-) diff --git a/lib/thrift-backend/ThriftSessionBackend.ts b/lib/thrift-backend/ThriftSessionBackend.ts index 7b562979..c0caec98 100644 --- a/lib/thrift-backend/ThriftSessionBackend.ts +++ b/lib/thrift-backend/ThriftSessionBackend.ts @@ -38,6 +38,7 @@ import escapeCatalogName from '../utils/escapeCatalogName'; import EmptyOperationBackend from './EmptyOperationBackend'; import isObjectNotFound from './metadataErrors'; import createPrimaryKeysSchema from './primaryKeysSchema'; +import assertBindableValue from '../kernel/KernelInputValidation'; interface OperationResponseShape { status: TStatus; @@ -118,16 +119,18 @@ function getQueryParameters( if (namedParameters !== undefined) { for (const name of Object.keys(namedParameters)) { const value = namedParameters[name]; + assertBindableValue(value, `namedParameters[${name}]`); const param = value instanceof DBSQLParameter ? value : new DBSQLParameter({ value }); result.push(param.toSparkParameter({ name })); } } if (ordinalParameters !== undefined) { - for (const value of ordinalParameters) { + ordinalParameters.forEach((value, i) => { + assertBindableValue(value, `ordinalParameters[${i}]`); const param = value instanceof DBSQLParameter ? value : new DBSQLParameter({ value }); result.push(param.toSparkParameter()); - } + }); } return result; diff --git a/tests/e2e/query_parameters.test.ts b/tests/e2e/query_parameters.test.ts index 61022758..9b94e6e0 100644 --- a/tests/e2e/query_parameters.test.ts +++ b/tests/e2e/query_parameters.test.ts @@ -231,4 +231,70 @@ describe('Query parameters', () => { expect(error).to.be.instanceof(ParameterError); } }); + + // https://github.com/databricks/databricks-sql-nodejs/issues/261 + describe('non-scalar parameter values (issue #261)', () => { + const HANG_TIMEOUT_MS = 30000; + + // Run the statement end-to-end against the live warehouse and return whatever error it + // produced. A hang (the reported bug) surfaces as a sentinel error after HANG_TIMEOUT_MS + // instead of stalling the suite. + async function runAndCaptureError( + statement: string, + options: Parameters>['executeStatement']>[1], + ): Promise { + const session = await openSession(); + let timer: NodeJS.Timeout | undefined; + try { + const run = (async () => { + const operation = await session.executeStatement(statement, options); + try { + await operation.fetchAll(); + } finally { + await operation.close().catch(() => {}); + } + })(); + const hang = new Promise((_, reject) => { + timer = setTimeout( + () => reject(new Error(`executeStatement hung for more than ${HANG_TIMEOUT_MS}ms`)), + HANG_TIMEOUT_MS, + ); + }); + await Promise.race([run, hang]); + return undefined; + } catch (error) { + return error; + } finally { + if (timer) clearTimeout(timer); + // Bound cleanup too: when the bug reproduces, closing the session can hang as well. + await Promise.race([ + session.close().catch(() => {}), + new Promise((resolve) => { + setTimeout(resolve, 5000).unref(); + }), + ]); + } + } + + it('should reject an array-of-arrays of ordinal parameters with ParameterError instead of hanging', async function () { + this.timeout(HANG_TIMEOUT_MS + 30000); + const error = await runAndCaptureError('SELECT ? AS c', { ordinalParameters: [[1], [2], [3]] as never }); + expect(error).to.be.instanceof(ParameterError); + expect((error as Error).message).to.match(/array/); + }); + + it('should reject an array value for a named parameter with ParameterError', async function () { + this.timeout(HANG_TIMEOUT_MS + 30000); + const error = await runAndCaptureError('SELECT :p AS c', { namedParameters: { p: [1, 2] as never } }); + expect(error).to.be.instanceof(ParameterError); + expect((error as Error).message).to.match(/array/); + }); + + it('should reject a plain-object ordinal parameter value with ParameterError', async function () { + this.timeout(HANG_TIMEOUT_MS + 30000); + const error = await runAndCaptureError('SELECT ? AS c', { ordinalParameters: [{ a: 1 } as never] }); + expect(error).to.be.instanceof(ParameterError); + expect((error as Error).message).to.match(/object/); + }); + }); }); diff --git a/tests/unit/DBSQLSession.test.ts b/tests/unit/DBSQLSession.test.ts index 11f6142f..54e518d8 100644 --- a/tests/unit/DBSQLSession.test.ts +++ b/tests/unit/DBSQLSession.test.ts @@ -263,6 +263,56 @@ describe('DBSQLSession', () => { }); }); + // https://github.com/databricks/databricks-sql-nodejs/issues/261 + describe('executeStatement with non-scalar parameter values', () => { + const invalidCases: Array<{ desc: string; options: object; pattern: RegExp }> = [ + { desc: 'ordinal array-of-arrays', options: { ordinalParameters: [[1], [2], [3]] }, pattern: /array/ }, + { desc: 'named array value', options: { namedParameters: { p: [1, 2] } }, pattern: /array/ }, + { desc: 'ordinal plain object', options: { ordinalParameters: [{ a: 1 }] }, pattern: /object/ }, + { desc: 'named plain object', options: { namedParameters: { p: { a: 1 } } }, pattern: /object/ }, + ]; + + for (const { desc, options, pattern } of invalidCases) { + it(`should throw ParameterError and not call the driver for ${desc}`, async () => { + const context = new ClientContextStub(); + const driver = sinon.spy(context.driver); + const session = createSessionForTest({ + handle: sessionHandleStub, + context, + serverProtocolVersion: TProtocolVersion.SPARK_CLI_SERVICE_PROTOCOL_V8, + }); + + try { + await session.executeStatement('SELECT ?', options as never); + expect.fail('It should throw an error'); + } catch (error) { + if (error instanceof AssertionError) { + throw error; + } + expect(error).instanceOf(ParameterError); + expect((error as Error).message).to.match(pattern); + } + expect(driver.executeStatement.called).to.be.false; + }); + } + + it('should keep accepting valid scalar values', async () => { + const context = new ClientContextStub(); + const driver = sinon.spy(context.driver); + const session = createSessionForTest({ + handle: sessionHandleStub, + context, + serverProtocolVersion: TProtocolVersion.SPARK_CLI_SERVICE_PROTOCOL_V8, + }); + + await session.executeStatement('SELECT ?, ?, ?, ?, ?', { + ordinalParameters: [new Date('2023-09-06T03:14:27.843Z'), new Int64(1234), BigInt(1234), null, 'x'], + }); + expect(driver.executeStatement.callCount).to.eq(1); + expect(driver.executeStatement.firstCall.args[0].parameters?.length).to.eq(5); + }); + }); + describe('executeStatement with queryTags', () => { it('should set confOverlay with query_tags when queryTags are provided', async () => { const context = new ClientContextStub(); From 76d93a1cfa0629f1b407cde64d41222b4bb21b4f Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 6 Nov 2026 22:14:07 +0000 Subject: [PATCH 02/10] ai: apply changes for #568 (2 review threads) Addresses: - #4498148804 at lib/thrift-backend/ThriftSessionBackend.ts:122 - #4498148811 at lib/thrift-backend/ThriftSessionBackend.ts:41 Signed-off-by: peco-engineer-bot[bot] Signed-off-by: Vu Anh Phung --- lib/kernel/KernelInputValidation.ts | 34 +------------- lib/kernel/KernelPositionalParams.ts | 2 +- lib/thrift-backend/ThriftSessionBackend.ts | 2 +- lib/utils/assertBindableValue.ts | 54 ++++++++++++++++++++++ tests/unit/kernel/inputValidation.test.ts | 17 ++++++- 5 files changed, 74 insertions(+), 35 deletions(-) create mode 100644 lib/utils/assertBindableValue.ts diff --git a/lib/kernel/KernelInputValidation.ts b/lib/kernel/KernelInputValidation.ts index a397542b..c8d96487 100644 --- a/lib/kernel/KernelInputValidation.ts +++ b/lib/kernel/KernelInputValidation.ts @@ -12,35 +12,5 @@ // See the License for the specific language governing permissions and // limitations under the License. -import Int64 from 'node-int64'; -import { DBSQLParameter, DBSQLParameterValue } from '../DBSQLParameter'; -import ParameterError from '../errors/ParameterError'; - -/** - * Reject a parameter value that cannot be bound as a scalar. Arrays and plain - * objects stringify to garbage (e.g. `[1,2,3]` → `"1,2,3"`) that the server - * fails to coerce — on the Thrift path the operation never returns to - * FINISHED (a DoS hazard), and on kernel it surfaces an opaque server error. We - * fail fast at bind time instead, mirroring the kernel's compound-type - * rejection. `DBSQLParameter`, `Int64`, `Date`, and JS primitives are allowed. - * - * Parameter-marker counting and arity validation are intentionally NOT done - * here: the kernel's `statement::params` codec owns that check and binds - * exactly one placeholder style per statement, so duplicating the SQL-walk - * JS-side would only risk drift. The driver's sole bind-time job is this - * cheap, type-shape gate before the value crosses the napi boundary. - */ -export default function assertBindableValue(value: DBSQLParameter | DBSQLParameterValue, label: string): void { - if (value instanceof DBSQLParameter) return; - if (value === null || value === undefined) return; - if (Array.isArray(value)) { - throw new ParameterError( - `${label} is an array; compound types (ARRAY/MAP/STRUCT) are not bindable as a parameter value`, - ); - } - if (typeof value === 'object' && !(value instanceof Date) && !(value instanceof Int64)) { - throw new ParameterError( - `${label} is an object; only scalar values (string/number/bigint/boolean), Date, and Int64 are bindable`, - ); - } -} +// Moved to a backend-neutral location; kept as a re-export for compatibility. +export { default } from '../utils/assertBindableValue'; diff --git a/lib/kernel/KernelPositionalParams.ts b/lib/kernel/KernelPositionalParams.ts index 61faa460..05db1343 100644 --- a/lib/kernel/KernelPositionalParams.ts +++ b/lib/kernel/KernelPositionalParams.ts @@ -15,7 +15,7 @@ import { DBSQLParameter, DBSQLParameterValue } from '../DBSQLParameter'; import ParameterError from '../errors/ParameterError'; import { KernelNativeRawParameterInput } from './KernelNativeLoader'; -import assertBindableValue from './KernelInputValidation'; +import assertBindableValue from '../utils/assertBindableValue'; /** * Derive `(precision,scale)` from a decimal value string for the kernel diff --git a/lib/thrift-backend/ThriftSessionBackend.ts b/lib/thrift-backend/ThriftSessionBackend.ts index c0caec98..8ccb1b96 100644 --- a/lib/thrift-backend/ThriftSessionBackend.ts +++ b/lib/thrift-backend/ThriftSessionBackend.ts @@ -38,7 +38,7 @@ import escapeCatalogName from '../utils/escapeCatalogName'; import EmptyOperationBackend from './EmptyOperationBackend'; import isObjectNotFound from './metadataErrors'; import createPrimaryKeysSchema from './primaryKeysSchema'; -import assertBindableValue from '../kernel/KernelInputValidation'; +import assertBindableValue from '../utils/assertBindableValue'; interface OperationResponseShape { status: TStatus; diff --git a/lib/utils/assertBindableValue.ts b/lib/utils/assertBindableValue.ts new file mode 100644 index 00000000..d315c45d --- /dev/null +++ b/lib/utils/assertBindableValue.ts @@ -0,0 +1,54 @@ +// Copyright (c) 2026 Databricks, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import Int64 from 'node-int64'; +import { DBSQLParameter, DBSQLParameterType, DBSQLParameterValue } from '../DBSQLParameter'; +import ParameterError from '../errors/ParameterError'; + +/** + * Backend-neutral bind-time gate shared by the Thrift and kernel backends. + * + * Rejects a parameter value that cannot be bound as a scalar. Arrays and plain + * objects stringify to garbage (e.g. `[1,2,3]` → `"1,2,3"`) that the server + * fails to coerce — on the Thrift path the operation never returns to + * FINISHED (a DoS hazard), and on the kernel path it surfaces an opaque server + * error. We fail fast at bind time instead, with the same error on both + * backends. `DBSQLParameter`, `Int64`, `Date`, and JS primitives are allowed. + * + * Parameter-marker counting and arity validation are intentionally NOT done + * here: that is owned by the server (Thrift) or the kernel's + * `statement::params` codec, so duplicating the SQL-walk JS-side would only + * risk drift. This is purely a cheap type-shape check. + */ +export default function assertBindableValue(value: DBSQLParameter | DBSQLParameterValue, label: string): void { + if (value instanceof DBSQLParameter) { + // An explicit VOID type ignores the wrapped value (binds NULL), so there is + // nothing to validate. Otherwise validate the unwrapped value: a + // non-scalar wrapped in `DBSQLParameter` hits the same STRING fallback. + if (value.type === DBSQLParameterType.VOID) return; + assertBindableValue(value.value, label); + return; + } + if (value === null || value === undefined) return; + if (Array.isArray(value)) { + throw new ParameterError( + `${label} is an array; compound types (ARRAY/MAP/STRUCT) are not bindable as a parameter value`, + ); + } + if (typeof value === 'object' && !(value instanceof Date) && !(value instanceof Int64)) { + throw new ParameterError( + `${label} is an object; only scalar values (string/number/bigint/boolean), Date, and Int64 are bindable`, + ); + } +} diff --git a/tests/unit/kernel/inputValidation.test.ts b/tests/unit/kernel/inputValidation.test.ts index a5e11af1..e5bc52c3 100644 --- a/tests/unit/kernel/inputValidation.test.ts +++ b/tests/unit/kernel/inputValidation.test.ts @@ -14,7 +14,7 @@ import { expect } from 'chai'; import Int64 from 'node-int64'; -import assertBindableValue from '../../../lib/kernel/KernelInputValidation'; +import assertBindableValue from '../../../lib/utils/assertBindableValue'; import { DBSQLParameter, DBSQLParameterType } from '../../../lib/DBSQLParameter'; import ParameterError from '../../../lib/errors/ParameterError'; @@ -39,4 +39,19 @@ describe('KernelInputValidation.assertBindableValue', () => { it('rejects plain objects', () => { expect(() => assertBindableValue({ a: 1 } as never, 'p')).to.throw(ParameterError, /object/); }); + + it('rejects non-scalar values wrapped in DBSQLParameter', () => { + expect(() => + assertBindableValue(new DBSQLParameter({ value: [1, 2] as never }), 'ordinalParameters[0]'), + ).to.throw(ParameterError, /ordinalParameters\[0\] is an array/); + expect(() => + assertBindableValue(new DBSQLParameter({ type: DBSQLParameterType.STRING, value: { a: 1 } as never }), 'p'), + ).to.throw(ParameterError, /object/); + }); + + it('accepts DBSQLParameter with explicit VOID type regardless of value', () => { + expect(() => + assertBindableValue(new DBSQLParameter({ type: DBSQLParameterType.VOID, value: [1, 2] as never }), 'p'), + ).to.not.throw(); + }); }); From 29944db3e7991f9ec30fd885f63f4a317b7233ff Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 6 Nov 2026 23:24:22 +0000 Subject: [PATCH 03/10] engineer-bot: address @mention on PR #568 Signed-off-by: peco-engineer-bot[bot] Signed-off-by: Vu Anh Phung --- tests/unit/kernel/inputValidation.test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/unit/kernel/inputValidation.test.ts b/tests/unit/kernel/inputValidation.test.ts index e5bc52c3..dfe3770a 100644 --- a/tests/unit/kernel/inputValidation.test.ts +++ b/tests/unit/kernel/inputValidation.test.ts @@ -41,9 +41,10 @@ describe('KernelInputValidation.assertBindableValue', () => { }); it('rejects non-scalar values wrapped in DBSQLParameter', () => { - expect(() => - assertBindableValue(new DBSQLParameter({ value: [1, 2] as never }), 'ordinalParameters[0]'), - ).to.throw(ParameterError, /ordinalParameters\[0\] is an array/); + expect(() => assertBindableValue(new DBSQLParameter({ value: [1, 2] as never }), 'ordinalParameters[0]')).to.throw( + ParameterError, + /ordinalParameters\[0\] is an array/, + ); expect(() => assertBindableValue(new DBSQLParameter({ type: DBSQLParameterType.STRING, value: { a: 1 } as never }), 'p'), ).to.throw(ParameterError, /object/); From 13407525053b867b0023c7140536f07c5895e9f9 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 6 Nov 2026 23:28:56 +0000 Subject: [PATCH 04/10] ai: apply changes for #568 (1 review thread) Addresses: - #4498696701 at lib/utils/assertBindableValue.ts:36 Signed-off-by: peco-engineer-bot[bot] Signed-off-by: Vu Anh Phung --- tests/unit/kernel/positionalParams.test.ts | 24 ++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/unit/kernel/positionalParams.test.ts b/tests/unit/kernel/positionalParams.test.ts index 960b2689..4e4e4009 100644 --- a/tests/unit/kernel/positionalParams.test.ts +++ b/tests/unit/kernel/positionalParams.test.ts @@ -102,6 +102,22 @@ describe('KernelPositionalParams.buildKernelPositionalParams', () => { { sqlType: 'TIMESTAMP', value: '2024-01-15 10:30:00' }, ]); }); + + it('rejects a non-scalar wrapped in DBSQLParameter with the same ParameterError as Thrift', () => { + expect(() => + buildKernelPositionalParams([new DBSQLParameter({ type: DBSQLParameterType.STRING, value: { a: 1 } as never })]), + ).to.throw(ParameterError, /ordinalParameters\[0\] is an object/); + expect(() => buildKernelPositionalParams([new DBSQLParameter({ value: [1, 2] as never })])).to.throw( + ParameterError, + /ordinalParameters\[0\] is an array/, + ); + }); + + it('allows an explicit VOID DBSQLParameter regardless of its wrapped value', () => { + expect( + buildKernelPositionalParams([new DBSQLParameter({ type: DBSQLParameterType.VOID, value: [1, 2] as never })]), + ).to.deep.equal([{ sqlType: 'VOID' }]); + }); }); describe('KernelPositionalParams.buildKernelNamedParams', () => { @@ -135,4 +151,12 @@ describe('KernelPositionalParams.buildKernelNamedParams', () => { }), ).to.deep.equal([{ name: 'duration', sqlType: 'INTERVAL MONTH', value: '2-6' }]); }); + + it('rejects a named non-scalar wrapped in DBSQLParameter with ParameterError', () => { + expect(() => + buildKernelNamedParams({ + p: new DBSQLParameter({ type: DBSQLParameterType.STRING, value: { a: 1 } as never }), + }), + ).to.throw(ParameterError, /namedParameters\[p\] is an object/); + }); }); From cdf2adf4a4bf26f877d991280a7bb3943299547b Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 6 Nov 2026 23:32:39 +0000 Subject: [PATCH 05/10] ai: apply changes for #568 (1 review thread) Addresses: - #4498725321 at lib/utils/assertBindableValue.ts:40 Signed-off-by: peco-engineer-bot[bot] Signed-off-by: Vu Anh Phung --- lib/utils/assertBindableValue.ts | 15 ++++++++++++++- tests/unit/kernel/inputValidation.test.ts | 15 +++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/lib/utils/assertBindableValue.ts b/lib/utils/assertBindableValue.ts index d315c45d..e8c5d17f 100644 --- a/lib/utils/assertBindableValue.ts +++ b/lib/utils/assertBindableValue.ts @@ -37,6 +37,11 @@ export default function assertBindableValue(value: DBSQLParameter | DBSQLParamet // nothing to validate. Otherwise validate the unwrapped value: a // non-scalar wrapped in `DBSQLParameter` hits the same STRING fallback. if (value.type === DBSQLParameterType.VOID) return; + // A nested `DBSQLParameter` is not a valid wrapped value: the outer + // parameter would fall through to the STRING default with an object value. + if ((value.value as unknown) instanceof DBSQLParameter) { + throw new ParameterError(`${label} wraps a nested DBSQLParameter; the wrapped value must be a scalar`); + } assertBindableValue(value.value, label); return; } @@ -46,9 +51,17 @@ export default function assertBindableValue(value: DBSQLParameter | DBSQLParamet `${label} is an array; compound types (ARRAY/MAP/STRUCT) are not bindable as a parameter value`, ); } - if (typeof value === 'object' && !(value instanceof Date) && !(value instanceof Int64)) { + if (typeof value === 'object') { + if (value instanceof Date || value instanceof Int64) return; throw new ParameterError( `${label} is an object; only scalar values (string/number/bigint/boolean), Date, and Int64 are bindable`, ); } + // Allow-list the remaining primitives; rejects e.g. functions and symbols. + const kind = typeof value; + if (kind !== 'string' && kind !== 'number' && kind !== 'bigint' && kind !== 'boolean') { + throw new ParameterError( + `${label} is a ${kind}; only scalar values (string/number/bigint/boolean), Date, and Int64 are bindable`, + ); + } } diff --git a/tests/unit/kernel/inputValidation.test.ts b/tests/unit/kernel/inputValidation.test.ts index dfe3770a..e4b9601b 100644 --- a/tests/unit/kernel/inputValidation.test.ts +++ b/tests/unit/kernel/inputValidation.test.ts @@ -50,6 +50,21 @@ describe('KernelInputValidation.assertBindableValue', () => { ).to.throw(ParameterError, /object/); }); + it('rejects a DBSQLParameter nested inside a DBSQLParameter', () => { + expect(() => + assertBindableValue(new DBSQLParameter({ value: new DBSQLParameter({ value: 1 }) as never }), 'p'), + ).to.throw(ParameterError, /nested DBSQLParameter/); + }); + + it('rejects functions and symbols', () => { + expect(() => assertBindableValue((() => 1) as never, 'p')).to.throw(ParameterError, /p is a function/); + expect(() => assertBindableValue(Symbol('s') as never, 'p')).to.throw(ParameterError, /p is a symbol/); + expect(() => assertBindableValue(new DBSQLParameter({ value: (() => 1) as never }), 'p')).to.throw( + ParameterError, + /function/, + ); + }); + it('accepts DBSQLParameter with explicit VOID type regardless of value', () => { expect(() => assertBindableValue(new DBSQLParameter({ type: DBSQLParameterType.VOID, value: [1, 2] as never }), 'p'), From bc07f5ded5461b29b928f0d50a79b3b33be4bc9d Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 6 Nov 2026 23:36:03 +0000 Subject: [PATCH 06/10] ai: apply changes for #568 (2 review threads) Addresses: - #4498750916 at tests/e2e/query_parameters.test.ts:242 - #4498750926 at lib/utils/assertBindableValue.ts:55 Signed-off-by: peco-engineer-bot[bot] Signed-off-by: Vu Anh Phung --- CHANGELOG.md | 1 + tests/e2e/query_parameters.test.ts | 64 ++++++++++++------------------ 2 files changed, 26 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 957853dc..04174c6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased - Report untyped NULL columns as STRING in kernel result metadata, matching Thrift. (#470) +- Reject non-scalar query parameter values at bind time with a `ParameterError` on both the Thrift and kernel backends, instead of stringifying them (which could leave a Thrift operation stuck before FINISHED). Newly rejected on the Thrift backend: arrays, plain objects, boxed primitives (e.g. `new String('x')`, `new Number(1)`), functions, symbols, and a `DBSQLParameter` that wraps another `DBSQLParameter`. Primitives, `Date`, `Int64`, and `DBSQLParameter` are still accepted. ## 3.0.0 diff --git a/tests/e2e/query_parameters.test.ts b/tests/e2e/query_parameters.test.ts index 9b94e6e0..d6d47b6b 100644 --- a/tests/e2e/query_parameters.test.ts +++ b/tests/e2e/query_parameters.test.ts @@ -234,67 +234,53 @@ describe('Query parameters', () => { // https://github.com/databricks/databricks-sql-nodejs/issues/261 describe('non-scalar parameter values (issue #261)', () => { - const HANG_TIMEOUT_MS = 30000; - - // Run the statement end-to-end against the live warehouse and return whatever error it - // produced. A hang (the reported bug) surfaces as a sentinel error after HANG_TIMEOUT_MS - // instead of stalling the suite. - async function runAndCaptureError( + // Non-scalar values are rejected client-side (before any RPC is sent), so executeStatement + // rejects immediately; this exercises the real session open/close round-trip around it. + async function executeAndCaptureError( statement: string, options: Parameters>['executeStatement']>[1], ): Promise { const session = await openSession(); - let timer: NodeJS.Timeout | undefined; try { - const run = (async () => { - const operation = await session.executeStatement(statement, options); - try { - await operation.fetchAll(); - } finally { - await operation.close().catch(() => {}); - } - })(); - const hang = new Promise((_, reject) => { - timer = setTimeout( - () => reject(new Error(`executeStatement hung for more than ${HANG_TIMEOUT_MS}ms`)), - HANG_TIMEOUT_MS, - ); - }); - await Promise.race([run, hang]); + await session.executeStatement(statement, options); return undefined; } catch (error) { return error; } finally { - if (timer) clearTimeout(timer); - // Bound cleanup too: when the bug reproduces, closing the session can hang as well. - await Promise.race([ - session.close().catch(() => {}), - new Promise((resolve) => { - setTimeout(resolve, 5000).unref(); - }), - ]); + await session.close(); } } - it('should reject an array-of-arrays of ordinal parameters with ParameterError instead of hanging', async function () { - this.timeout(HANG_TIMEOUT_MS + 30000); - const error = await runAndCaptureError('SELECT ? AS c', { ordinalParameters: [[1], [2], [3]] as never }); + it('should reject an array-of-arrays of ordinal parameters with ParameterError instead of hanging', async () => { + const error = await executeAndCaptureError('SELECT ? AS c', { ordinalParameters: [[1], [2], [3]] as never }); expect(error).to.be.instanceof(ParameterError); expect((error as Error).message).to.match(/array/); }); - it('should reject an array value for a named parameter with ParameterError', async function () { - this.timeout(HANG_TIMEOUT_MS + 30000); - const error = await runAndCaptureError('SELECT :p AS c', { namedParameters: { p: [1, 2] as never } }); + it('should reject an array value for a named parameter with ParameterError', async () => { + const error = await executeAndCaptureError('SELECT :p AS c', { namedParameters: { p: [1, 2] as never } }); expect(error).to.be.instanceof(ParameterError); expect((error as Error).message).to.match(/array/); }); - it('should reject a plain-object ordinal parameter value with ParameterError', async function () { - this.timeout(HANG_TIMEOUT_MS + 30000); - const error = await runAndCaptureError('SELECT ? AS c', { ordinalParameters: [{ a: 1 } as never] }); + it('should reject a plain-object ordinal parameter value with ParameterError', async () => { + const error = await executeAndCaptureError('SELECT ? AS c', { ordinalParameters: [{ a: 1 } as never] }); expect(error).to.be.instanceof(ParameterError); expect((error as Error).message).to.match(/object/); }); + + it('should support multi-row inserts with flattened scalar ordinal parameters', async () => { + const session = await openSession(); + try { + const operation = await session.executeStatement('SELECT * FROM VALUES (?), (?), (?) AS t(c) ORDER BY c', { + ordinalParameters: [1, 2, 3], + }); + const result = await operation.fetchAll(); + await operation.close(); + expect(result).to.deep.equal([{ c: 1 }, { c: 2 }, { c: 3 }]); + } finally { + await session.close(); + } + }); }); }); From 5eda06f1506c513b15f77d40f2795102057887eb Mon Sep 17 00:00:00 2001 From: Vu Anh Phung Date: Sat, 7 Nov 2026 00:04:07 +0000 Subject: [PATCH 07/10] fix: keep parameter validation compatibility export lint clean Signed-off-by: Vu Anh Phung --- lib/kernel/KernelInputValidation.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/kernel/KernelInputValidation.ts b/lib/kernel/KernelInputValidation.ts index c8d96487..43f51634 100644 --- a/lib/kernel/KernelInputValidation.ts +++ b/lib/kernel/KernelInputValidation.ts @@ -12,5 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +import assertBindableValue from '../utils/assertBindableValue'; + // Moved to a backend-neutral location; kept as a re-export for compatibility. -export { default } from '../utils/assertBindableValue'; +export default assertBindableValue; From 3eb9ae998e95a8bc917ca5afa2343a1f86f47f69 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Sat, 7 Nov 2026 00:08:26 +0000 Subject: [PATCH 08/10] ai: apply changes for #568 (1 review thread) Addresses: - #4498942089 at lib/utils/assertBindableValue.ts:54 Signed-off-by: peco-engineer-bot[bot] --- tests/unit/DBSQLSession.test.ts | 10 ++++++++++ tests/unit/kernel/inputValidation.test.ts | 9 +++++++++ 2 files changed, 19 insertions(+) diff --git a/tests/unit/DBSQLSession.test.ts b/tests/unit/DBSQLSession.test.ts index 54e518d8..ce782207 100644 --- a/tests/unit/DBSQLSession.test.ts +++ b/tests/unit/DBSQLSession.test.ts @@ -8,6 +8,7 @@ import DBSQLOperation from '../../lib/DBSQLOperation'; import { PrimaryKeysRequest } from '../../lib/contracts/IDBSQLSession'; import ISessionBackend from '../../lib/contracts/ISessionBackend'; import ParameterError from '../../lib/errors/ParameterError'; +import { DBSQLParameter } from '../../lib/DBSQLParameter'; import { TSessionHandle, TProtocolVersion } from '../../thrift/TCLIService_types'; import ClientContextStub from './.stubs/ClientContextStub'; import { createSessionForTest } from './.stubs/createSessionForTest'; @@ -270,6 +271,15 @@ describe('DBSQLSession', () => { { desc: 'named array value', options: { namedParameters: { p: [1, 2] } }, pattern: /array/ }, { desc: 'ordinal plain object', options: { ordinalParameters: [{ a: 1 }] }, pattern: /object/ }, { desc: 'named plain object', options: { namedParameters: { p: { a: 1 } } }, pattern: /object/ }, + { + desc: 'ordinal array wrapped in DBSQLParameter', + options: { ordinalParameters: [new DBSQLParameter({ value: [1, 2] as never })] }, + pattern: /array/, + }, + // eslint-disable-next-line no-new-wrappers + { desc: 'ordinal boxed String', options: { ordinalParameters: [new String('x')] }, pattern: /object/ }, + // eslint-disable-next-line no-new-wrappers + { desc: 'named boxed Number', options: { namedParameters: { p: new Number(1) } }, pattern: /object/ }, ]; for (const { desc, options, pattern } of invalidCases) { diff --git a/tests/unit/kernel/inputValidation.test.ts b/tests/unit/kernel/inputValidation.test.ts index e4b9601b..c1b2e088 100644 --- a/tests/unit/kernel/inputValidation.test.ts +++ b/tests/unit/kernel/inputValidation.test.ts @@ -40,6 +40,15 @@ describe('KernelInputValidation.assertBindableValue', () => { expect(() => assertBindableValue({ a: 1 } as never, 'p')).to.throw(ParameterError, /object/); }); + it('rejects boxed primitives', () => { + // eslint-disable-next-line no-new-wrappers + expect(() => assertBindableValue(new String('x') as never, 'p')).to.throw(ParameterError, /p is an object/); + // eslint-disable-next-line no-new-wrappers + expect(() => assertBindableValue(new Number(1) as never, 'p')).to.throw(ParameterError, /p is an object/); + // eslint-disable-next-line no-new-wrappers + expect(() => assertBindableValue(new Boolean(true) as never, 'p')).to.throw(ParameterError, /p is an object/); + }); + it('rejects non-scalar values wrapped in DBSQLParameter', () => { expect(() => assertBindableValue(new DBSQLParameter({ value: [1, 2] as never }), 'ordinalParameters[0]')).to.throw( ParameterError, From cd421452cfa081a23147f8a73fed3b4fc75d8b76 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Sat, 7 Nov 2026 00:12:23 +0000 Subject: [PATCH 09/10] ai: apply changes for #568 (1 review thread) Addresses: - #4498972272 at lib/kernel/KernelInputValidation.ts:18 Signed-off-by: peco-engineer-bot[bot] --- lib/kernel/KernelInputValidation.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/kernel/KernelInputValidation.ts b/lib/kernel/KernelInputValidation.ts index 43f51634..eb8a0268 100644 --- a/lib/kernel/KernelInputValidation.ts +++ b/lib/kernel/KernelInputValidation.ts @@ -14,5 +14,12 @@ import assertBindableValue from '../utils/assertBindableValue'; -// Moved to a backend-neutral location; kept as a re-export for compatibility. -export default assertBindableValue; +/** + * @deprecated Moved to a backend-neutral location — import the default export + * of `lib/utils/assertBindableValue` instead. Kept only as a thin shim because + * the package has no `exports` map, so `dist/kernel/KernelInputValidation` is a + * reachable deep-import path; it will be removed in a future release. + */ +export default function kernelAssertBindableValue(...args: Parameters): void { + assertBindableValue(...args); +} From b5c82763d1e830015ccab1f487584c2c9177eb98 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Sat, 7 Nov 2026 00:15:36 +0000 Subject: [PATCH 10/10] ai: apply changes for #568 (1 review thread) Addresses: - #4498992434 at tests/unit/kernel/inputValidation.test.ts:17 Signed-off-by: peco-engineer-bot[bot] --- tests/unit/kernel/inputValidation.test.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/unit/kernel/inputValidation.test.ts b/tests/unit/kernel/inputValidation.test.ts index c1b2e088..0f86647f 100644 --- a/tests/unit/kernel/inputValidation.test.ts +++ b/tests/unit/kernel/inputValidation.test.ts @@ -15,10 +15,21 @@ import { expect } from 'chai'; import Int64 from 'node-int64'; import assertBindableValue from '../../../lib/utils/assertBindableValue'; +import kernelAssertBindableValue from '../../../lib/kernel/KernelInputValidation'; import { DBSQLParameter, DBSQLParameterType } from '../../../lib/DBSQLParameter'; import ParameterError from '../../../lib/errors/ParameterError'; -describe('KernelInputValidation.assertBindableValue', () => { +describe('KernelInputValidation (deprecated shim)', () => { + it('still delegates to assertBindableValue', () => { + expect(() => kernelAssertBindableValue(42, 'p')).to.not.throw(); + expect(() => kernelAssertBindableValue([1, 2, 3] as never, 'ordinalParameters[0]')).to.throw( + ParameterError, + /ordinalParameters\[0\] is an array/, + ); + }); +}); + +describe('utils/assertBindableValue', () => { it('accepts scalars, Date, Int64, bigint, null, and DBSQLParameter', () => { expect(() => assertBindableValue(42, 'p')).to.not.throw(); expect(() => assertBindableValue('x', 'p')).to.not.throw();