From 00d9ddd6ad92d44389c00f671129827021740574 Mon Sep 17 00:00:00 2001 From: slvrtrn Date: Thu, 12 Sep 2024 19:37:43 +0200 Subject: [PATCH 1/2] Fix unhandled ResultSet.json errors --- .../__tests__/unit/node_result_set.test.ts | 77 ++++++++++++++++++- packages/client-node/src/result_set.ts | 16 ++-- packages/client-node/src/utils/stream.ts | 10 +-- 3 files changed, 83 insertions(+), 20 deletions(-) diff --git a/packages/client-node/__tests__/unit/node_result_set.test.ts b/packages/client-node/__tests__/unit/node_result_set.test.ts index a627f68f..217b1fd8 100644 --- a/packages/client-node/__tests__/unit/node_result_set.test.ts +++ b/packages/client-node/__tests__/unit/node_result_set.test.ts @@ -1,4 +1,4 @@ -import type { Row } from '@clickhouse/client-common' +import type { DataFormat, Row } from '@clickhouse/client-common' import { guid } from '@test/utils' import Stream, { Readable } from 'stream' import { ResultSet } from '../../src' @@ -63,10 +63,81 @@ describe('[Node.js] ResultSet', () => { expect(row.json()).toEqual({ foo: 'bar' }) }) - function makeResultSet(stream: Stream.Readable) { + describe('unhandled exceptions with streamable JSON formats', () => { + const logAndQuit = (err: Error | unknown, prefix: string) => { + console.error(prefix, err) + process.exit(1) + } + const uncaughtExceptionListener = (err: Error) => + logAndQuit(err, 'uncaughtException:') + const unhandledRejectionListener = (err: unknown) => + logAndQuit(err, 'unhandledRejection:') + + const invalidJSON = 'invalid":"foo"}\n' + + beforeAll(() => { + process.on('uncaughtException', uncaughtExceptionListener) + process.on('unhandledRejection', unhandledRejectionListener) + }) + afterAll(() => { + process.off('uncaughtException', uncaughtExceptionListener) + process.off('unhandledRejection', unhandledRejectionListener) + }) + + describe('Streamable JSON formats - JSONEachRow', () => { + it('should not be produced (ResultSet.text)', async () => { + const rs = makeResultSet( + Stream.Readable.from([Buffer.from(invalidJSON)]), + ) + const text = await rs.text() + expect(text).toEqual(invalidJSON) + }) + + it('should not be produced (ResultSet.json)', async () => { + const rs = makeResultSet( + Stream.Readable.from([Buffer.from(invalidJSON)]), + ) + const jsonPromise = rs.json() + await expectAsync(jsonPromise).toBeRejectedWith( + jasmine.objectContaining({ + name: 'SyntaxError', + }), + ) + }) + }) + + describe('Non-streamable JSON formats - JSON', () => { + it('should not be produced (ResultSet.text)', async () => { + const rs = makeResultSet( + Stream.Readable.from([Buffer.from(invalidJSON)]), + 'JSON', + ) + const text = await rs.text() + expect(text).toEqual(invalidJSON) + }) + + it('should not be produced (ResultSet.json)', async () => { + const rs = makeResultSet( + Stream.Readable.from([Buffer.from(invalidJSON)]), + 'JSON', + ) + const jsonPromise = rs.json() + await expectAsync(jsonPromise).toBeRejectedWith( + jasmine.objectContaining({ + name: 'SyntaxError', + }), + ) + }) + }) + }) + + function makeResultSet( + stream: Stream.Readable, + format: DataFormat = 'JSONEachRow', + ) { return ResultSet.instance({ stream, - format: 'JSONEachRow', + format, query_id: guid(), log_error: (err) => { console.error(err) diff --git a/packages/client-node/src/result_set.ts b/packages/client-node/src/result_set.ts index 0096944a..43aceb28 100644 --- a/packages/client-node/src/result_set.ts +++ b/packages/client-node/src/result_set.ts @@ -82,16 +82,12 @@ export class ResultSet // JSONEachRow, etc. if (isStreamableJSONFamily(this.format as DataFormat)) { const result: T[] = [] - await new Promise((resolve, reject) => { - const stream = this.stream() - stream.on('data', (rows: Row[]) => { - for (const row of rows) { - result.push(row.json()) - } - }) - stream.on('end', resolve) - stream.on('error', reject) - }) + const stream = this.stream() + for await (const rows of stream) { + for (const row of rows) { + result.push(row.json()) + } + } return result as any } // JSON, JSONObjectEachRow, etc. diff --git a/packages/client-node/src/utils/stream.ts b/packages/client-node/src/utils/stream.ts index f9a68689..7523828b 100644 --- a/packages/client-node/src/utils/stream.ts +++ b/packages/client-node/src/utils/stream.ts @@ -8,13 +8,9 @@ export async function getAsText(stream: Stream.Readable): Promise { let text = '' const textDecoder = new TextDecoder() - await new Promise((resolve, reject) => { - stream.on('data', (chunk) => { - text += textDecoder.decode(chunk, { stream: true }) - }) - stream.on('end', resolve) - stream.on('error', reject) - }) + for await (const chunk of stream) { + text += textDecoder.decode(chunk, { stream: true }) + } // flush const last = textDecoder.decode() From 2248487d26bf5e0ddb4fedca5563f3c7c3e130a4 Mon Sep 17 00:00:00 2001 From: slvrtrn Date: Thu, 12 Sep 2024 19:41:05 +0200 Subject: [PATCH 2/2] Add CHANGELOG entry, bump versions --- CHANGELOG.md | 6 +++++- packages/client-common/src/version.ts | 2 +- packages/client-node/src/version.ts | 2 +- packages/client-web/src/version.ts | 2 +- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f38c1f71..0f2ee2b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,9 +1,13 @@ -# Unreleased +# 1.6.0 (Common, Node.js, Web) ## New features - Added optional `real_time_microseconds` field to the `ClickHouseSummary` interface (see https://github.com/ClickHouse/ClickHouse/pull/69032) +## Bug fixes + +- Fixed unhandled exceptions produced when calling `ResultSet.json` if the response data was not in fact a valid JSON. ([#311](https://github.com/ClickHouse/clickhouse-js/pull/311)) + # 1.5.0 (Node.js) ## New features diff --git a/packages/client-common/src/version.ts b/packages/client-common/src/version.ts index b7c78f16..d6574191 100644 --- a/packages/client-common/src/version.ts +++ b/packages/client-common/src/version.ts @@ -1 +1 @@ -export default '1.5.0' +export default '1.6.0' diff --git a/packages/client-node/src/version.ts b/packages/client-node/src/version.ts index b7c78f16..d6574191 100644 --- a/packages/client-node/src/version.ts +++ b/packages/client-node/src/version.ts @@ -1 +1 @@ -export default '1.5.0' +export default '1.6.0' diff --git a/packages/client-web/src/version.ts b/packages/client-web/src/version.ts index b7c78f16..d6574191 100644 --- a/packages/client-web/src/version.ts +++ b/packages/client-web/src/version.ts @@ -1 +1 @@ -export default '1.5.0' +export default '1.6.0'