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/lib/kernel/KernelInputValidation.ts b/lib/kernel/KernelInputValidation.ts index a397542b..eb8a0268 100644 --- a/lib/kernel/KernelInputValidation.ts +++ b/lib/kernel/KernelInputValidation.ts @@ -12,35 +12,14 @@ // 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'; +import assertBindableValue from '../utils/assertBindableValue'; /** - * 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. + * @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 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`, - ); - } +export default function kernelAssertBindableValue(...args: Parameters): void { + assertBindableValue(...args); } 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 7b562979..8ccb1b96 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 '../utils/assertBindableValue'; 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/lib/utils/assertBindableValue.ts b/lib/utils/assertBindableValue.ts new file mode 100644 index 00000000..e8c5d17f --- /dev/null +++ b/lib/utils/assertBindableValue.ts @@ -0,0 +1,67 @@ +// 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; + // 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; + } + 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') { + 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/e2e/query_parameters.test.ts b/tests/e2e/query_parameters.test.ts index 61022758..d6d47b6b 100644 --- a/tests/e2e/query_parameters.test.ts +++ b/tests/e2e/query_parameters.test.ts @@ -231,4 +231,56 @@ 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)', () => { + // 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(); + try { + await session.executeStatement(statement, options); + return undefined; + } catch (error) { + return error; + } finally { + await session.close(); + } + } + + 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 () => { + 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 () => { + 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(); + } + }); + }); }); diff --git a/tests/unit/DBSQLSession.test.ts b/tests/unit/DBSQLSession.test.ts index 11f6142f..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'; @@ -263,6 +264,65 @@ 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/ }, + { + 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) { + 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(); diff --git a/tests/unit/kernel/inputValidation.test.ts b/tests/unit/kernel/inputValidation.test.ts index a5e11af1..0f86647f 100644 --- a/tests/unit/kernel/inputValidation.test.ts +++ b/tests/unit/kernel/inputValidation.test.ts @@ -14,11 +14,22 @@ import { expect } from 'chai'; import Int64 from 'node-int64'; -import assertBindableValue from '../../../lib/kernel/KernelInputValidation'; +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(); @@ -39,4 +50,44 @@ describe('KernelInputValidation.assertBindableValue', () => { it('rejects plain objects', () => { 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, + /ordinalParameters\[0\] is an array/, + ); + expect(() => + assertBindableValue(new DBSQLParameter({ type: DBSQLParameterType.STRING, value: { a: 1 } as never }), 'p'), + ).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'), + ).to.not.throw(); + }); }); 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/); + }); });