From 055b461b103b84538c1b80acd32a0635e37b77df Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" <3815206+peco-engineer-bot[bot]@users.noreply.github.com> Date: Thu, 5 Nov 2026 05:06:06 +0000 Subject: [PATCH 1/3] missing errorCode and statusCode on errors (#33) Signed-off-by: peco-engineer-bot[bot] <3815206+peco-engineer-bot[bot]@users.noreply.github.com> --- lib/errors/OperationStateError.ts | 8 +++ tests/e2e/operation_error_codes.test.ts | 59 +++++++++++++++++++ tests/unit/errors/OperationStateError.test.ts | 27 +++++++++ 3 files changed, 94 insertions(+) create mode 100644 tests/e2e/operation_error_codes.test.ts create mode 100644 tests/unit/errors/OperationStateError.test.ts diff --git a/lib/errors/OperationStateError.ts b/lib/errors/OperationStateError.ts index 794464d7..0e2a791e 100644 --- a/lib/errors/OperationStateError.ts +++ b/lib/errors/OperationStateError.ts @@ -22,10 +22,18 @@ export default class OperationStateError extends HiveDriverError { public response?: TGetOperationStatusResp; + // SQLSTATE reported by the server for the failed operation (if any) + public sqlState?: string; + constructor(errorCode: OperationStateErrorCode, response?: TGetOperationStatusResp) { super(response?.displayMessage ?? errorMessages[errorCode]); this.errorCode = errorCode; this.response = response; + + const sqlState = response?.sqlState ?? response?.status?.sqlState; + if (sqlState) { + this.sqlState = sqlState; + } } } diff --git a/tests/e2e/operation_error_codes.test.ts b/tests/e2e/operation_error_codes.test.ts new file mode 100644 index 00000000..7fbf32f3 --- /dev/null +++ b/tests/e2e/operation_error_codes.test.ts @@ -0,0 +1,59 @@ +import { expect, AssertionError } from 'chai'; +import { DBSQLClient } from '../../lib'; +import IDBSQLSession from '../../lib/contracts/IDBSQLSession'; +import OperationStateError, { OperationStateErrorCode } from '../../lib/errors/OperationStateError'; + +import config from './utils/config'; + +// Regression for https://github.com/databricks/databricks-sql-nodejs/issues/33 — +// errors thrown for failed operations should carry a machine-readable code +// (SQLSTATE) at the top level, not only a message buried in `response`. +describe('Operation error codes', () => { + let client: DBSQLClient; + let session: IDBSQLSession; + + before(async () => { + client = new DBSQLClient(); + const connection = await client.connect({ + host: config.host, + path: config.path, + token: config.token, + }); + session = await connection.openSession({ + initialCatalog: config.catalog, + initialSchema: config.schema, + }); + }); + + after(async () => { + await session?.close(); + await client?.close(); + }); + + async function expectOperationStateError(query: string) { + try { + const operation = await session.executeStatement(query); + await operation.fetchAll(); + expect.fail('It should throw an error'); + } catch (error) { + if (error instanceof AssertionError) { + throw error; + } + expect(error).to.be.instanceOf(OperationStateError); + return error as OperationStateError & { sqlState?: string }; + } + throw new Error('unreachable'); + } + + it('should expose server SQLSTATE on OperationStateError for runtime failures', async () => { + const error = await expectOperationStateError("SELECT raise_error('boom')"); + + expect(error.errorCode).to.be.eq(OperationStateErrorCode.Error); + + const wireSqlState = error.response?.sqlState ?? error.response?.status?.sqlState; + expect(wireSqlState, 'server should send a SQLSTATE on the wire').to.be.a('string'); + expect(wireSqlState).to.have.length(5); + + expect(error.sqlState).to.be.eq(wireSqlState); + }); +}); diff --git a/tests/unit/errors/OperationStateError.test.ts b/tests/unit/errors/OperationStateError.test.ts new file mode 100644 index 00000000..12026981 --- /dev/null +++ b/tests/unit/errors/OperationStateError.test.ts @@ -0,0 +1,27 @@ +import { expect } from 'chai'; +import OperationStateError, { OperationStateErrorCode } from '../../../lib/errors/OperationStateError'; +import { TOperationState, TStatusCode } from '../../../thrift/TCLIService_types'; + +describe('OperationStateError', () => { + it('should expose response.sqlState at top level', () => { + const error = new OperationStateError(OperationStateErrorCode.Error, { + status: { statusCode: TStatusCode.SUCCESS_STATUS, sqlState: '00000' }, + operationState: TOperationState.ERROR_STATE, + sqlState: '22012', + displayMessage: 'Division by zero', + }) as OperationStateError & { sqlState?: string }; + + expect(error.errorCode).to.be.eq(OperationStateErrorCode.Error); + expect(error.message).to.be.eq('Division by zero'); + expect(error.sqlState).to.be.eq('22012'); + }); + + it('should fall back to response.status.sqlState', () => { + const error = new OperationStateError(OperationStateErrorCode.Error, { + status: { statusCode: TStatusCode.ERROR_STATUS, sqlState: '42P01' }, + operationState: TOperationState.ERROR_STATE, + }) as OperationStateError & { sqlState?: string }; + + expect(error.sqlState).to.be.eq('42P01'); + }); +}); From bc8f9f179736ad05ee57a6ad853cf093b8ed04f1 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 5 Nov 2026 05:11:25 +0000 Subject: [PATCH 2/3] ai: apply changes for #563 (1 review thread) Addresses: - #4476822989 at lib/errors/OperationStateError.ts:36 Signed-off-by: peco-engineer-bot[bot] --- lib/errors/OperationStateError.ts | 10 ++++++- tests/unit/errors/OperationStateError.test.ts | 26 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/lib/errors/OperationStateError.ts b/lib/errors/OperationStateError.ts index 0e2a791e..e615e5a6 100644 --- a/lib/errors/OperationStateError.ts +++ b/lib/errors/OperationStateError.ts @@ -33,7 +33,15 @@ export default class OperationStateError extends HiveDriverError { const sqlState = response?.sqlState ?? response?.status?.sqlState; if (sqlState) { - this.sqlState = sqlState; + // Non-enumerable, matching how the kernel backend attaches `sqlState` + // (see `defineErrorMetadata` in lib/kernel/KernelErrorMapping.ts), so the + // same error class looks identical regardless of backend. + Object.defineProperty(this, 'sqlState', { + value: sqlState, + writable: true, + enumerable: false, + configurable: true, + }); } } } diff --git a/tests/unit/errors/OperationStateError.test.ts b/tests/unit/errors/OperationStateError.test.ts index 12026981..dd91fb85 100644 --- a/tests/unit/errors/OperationStateError.test.ts +++ b/tests/unit/errors/OperationStateError.test.ts @@ -1,5 +1,6 @@ import { expect } from 'chai'; import OperationStateError, { OperationStateErrorCode } from '../../../lib/errors/OperationStateError'; +import { mapKernelErrorToJsError } from '../../../lib/kernel/KernelErrorMapping'; import { TOperationState, TStatusCode } from '../../../thrift/TCLIService_types'; describe('OperationStateError', () => { @@ -24,4 +25,29 @@ describe('OperationStateError', () => { expect(error.sqlState).to.be.eq('42P01'); }); + + it('should attach sqlState with the same descriptor as the kernel backend', () => { + const thriftError = new OperationStateError(OperationStateErrorCode.Error, { + status: { statusCode: TStatusCode.ERROR_STATUS }, + operationState: TOperationState.ERROR_STATE, + sqlState: '22012', + }); + const kernelError = mapKernelErrorToJsError({ code: 'SqlError', message: 'x', sqlstate: '22012' }); + + expect(kernelError).to.be.instanceOf(OperationStateError); + expect(Object.getOwnPropertyDescriptor(thriftError, 'sqlState')).to.deep.eq( + Object.getOwnPropertyDescriptor(kernelError, 'sqlState'), + ); + expect(Object.keys(thriftError)).to.not.include('sqlState'); + expect(JSON.parse(JSON.stringify(thriftError))).to.not.have.property('sqlState'); + }); + + it('should not define sqlState when the server reports none', () => { + const error = new OperationStateError(OperationStateErrorCode.Error, { + status: { statusCode: TStatusCode.ERROR_STATUS }, + operationState: TOperationState.ERROR_STATE, + }); + + expect(Object.prototype.hasOwnProperty.call(error, 'sqlState')).to.be.false; + }); }); From 49c905818b5b5cba85b80c2103fb1685704b2630 Mon Sep 17 00:00:00 2001 From: Vu Anh Phung Date: Thu, 5 Nov 2026 05:47:59 +0000 Subject: [PATCH 3/3] Read sqlState only from the top-level operation status Status.assert throws StatusError for error RPC statuses before an OperationStateError is built, so `response.status` is always a successful GetOperationStatus status and its sqlState doesn't describe the failed query. Drop the fallback and move unit fixtures to SUCCESS_STATUS + ERROR_STATE. Co-authored-by: Isaac Signed-off-by: Vu Anh Phung --- lib/errors/OperationStateError.ts | 4 +++- tests/e2e/operation_error_codes.test.ts | 2 +- tests/unit/errors/OperationStateError.test.ts | 12 ++++++------ 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/errors/OperationStateError.ts b/lib/errors/OperationStateError.ts index e615e5a6..46f5024a 100644 --- a/lib/errors/OperationStateError.ts +++ b/lib/errors/OperationStateError.ts @@ -31,7 +31,9 @@ export default class OperationStateError extends HiveDriverError { this.errorCode = errorCode; this.response = response; - const sqlState = response?.sqlState ?? response?.status?.sqlState; + // Only the top-level field describes the failed operation: `status` is + // always a successful GetOperationStatus RPC status by the time this is thrown. + const sqlState = response?.sqlState; if (sqlState) { // Non-enumerable, matching how the kernel backend attaches `sqlState` // (see `defineErrorMetadata` in lib/kernel/KernelErrorMapping.ts), so the diff --git a/tests/e2e/operation_error_codes.test.ts b/tests/e2e/operation_error_codes.test.ts index 7fbf32f3..d1a2eb72 100644 --- a/tests/e2e/operation_error_codes.test.ts +++ b/tests/e2e/operation_error_codes.test.ts @@ -50,7 +50,7 @@ describe('Operation error codes', () => { expect(error.errorCode).to.be.eq(OperationStateErrorCode.Error); - const wireSqlState = error.response?.sqlState ?? error.response?.status?.sqlState; + const wireSqlState = error.response?.sqlState; expect(wireSqlState, 'server should send a SQLSTATE on the wire').to.be.a('string'); expect(wireSqlState).to.have.length(5); diff --git a/tests/unit/errors/OperationStateError.test.ts b/tests/unit/errors/OperationStateError.test.ts index dd91fb85..ac38c371 100644 --- a/tests/unit/errors/OperationStateError.test.ts +++ b/tests/unit/errors/OperationStateError.test.ts @@ -17,18 +17,18 @@ describe('OperationStateError', () => { expect(error.sqlState).to.be.eq('22012'); }); - it('should fall back to response.status.sqlState', () => { + it('should not take sqlState from the RPC status', () => { const error = new OperationStateError(OperationStateErrorCode.Error, { - status: { statusCode: TStatusCode.ERROR_STATUS, sqlState: '42P01' }, + status: { statusCode: TStatusCode.SUCCESS_STATUS, sqlState: '00000' }, operationState: TOperationState.ERROR_STATE, - }) as OperationStateError & { sqlState?: string }; + }); - expect(error.sqlState).to.be.eq('42P01'); + expect(Object.prototype.hasOwnProperty.call(error, 'sqlState')).to.be.false; }); it('should attach sqlState with the same descriptor as the kernel backend', () => { const thriftError = new OperationStateError(OperationStateErrorCode.Error, { - status: { statusCode: TStatusCode.ERROR_STATUS }, + status: { statusCode: TStatusCode.SUCCESS_STATUS }, operationState: TOperationState.ERROR_STATE, sqlState: '22012', }); @@ -44,7 +44,7 @@ describe('OperationStateError', () => { it('should not define sqlState when the server reports none', () => { const error = new OperationStateError(OperationStateErrorCode.Error, { - status: { statusCode: TStatusCode.ERROR_STATUS }, + status: { statusCode: TStatusCode.SUCCESS_STATUS }, operationState: TOperationState.ERROR_STATE, });