From a0e536069b29b71758f58833d050491ee783d302 Mon Sep 17 00:00:00 2001 From: Dylan Frankland Date: Wed, 15 Jul 2026 23:28:09 +0000 Subject: [PATCH] Bind Tuple parameters and integer Map keys, matching @clickhouse/client `serializeValue` (the JS-value serializer behind `formatParamValue`, used by the `queryBindAsync` server-side binding path) mishandled two compound-type parameter shapes, so `@clickhouse/client` code passing them through `ChdbConnection` failed with engine parse errors: - A `@clickhouse/client-common` `TupleParam` fell through to the generic-object branch and serialized as the map literal `{'values':[...]}`, which the engine rejects for a `Tuple(...)` param. It now serializes as the positional tuple literal `(a,b,c)`. Detected by shape (constructor name + `values` array) rather than `instanceof`: the instance a caller passes comes from their own `@clickhouse/client(-common)` copy, which under pnpm/nested installs may be a different module instance than ours, so `instanceof` would spuriously miss it. - `Map` keys were force-stringified and quoted (`{'42':...}`), which the engine rejects for `Map(Int32, ...)`. Keys are now serialized by their JS type (numeric keys unquoted, string keys quoted), matching `@clickhouse/client-common`'s formatter. Verified against the real engine: added unit + `queryBind` round-trip tests, and confirmed the upstream `select_query_binding.test.ts` tuple/map cases now pass, so their entries are removed from the clickhouse-js parity skip list. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/serialize.ts | 34 +++++++++++++++++++++++++++-- test/v3/querybind.test.ts | 34 +++++++++++++++++++++++++++++ test/v3/serialize.test.ts | 20 +++++++++++++++++ tests/clickhouse-js/skip_list.json | 35 ------------------------------ 4 files changed, 86 insertions(+), 37 deletions(-) diff --git a/src/serialize.ts b/src/serialize.ts index 3bb0d00..63541e3 100644 --- a/src/serialize.ts +++ b/src/serialize.ts @@ -143,7 +143,8 @@ function serializeNumber(value: number): string { * - string -> escaped single-quoted literal * - Date -> 'YYYY-MM-DD HH:MM:SS' (UTC) * - Array / TypedArray -> [a, b, c] - * - Map -> {k: v, ...} (keys escaped as string literals) + * - TupleParam -> (a, b, c) (tuple literal, elements quoted) + * - Map -> {k: v, ...} (keys serialized by JS type) * - plain object -> {k: v, ...} (keys escaped as string literals) * * @throws ChdbBindError on non-finite numbers, invalid Dates, or unsupported types. @@ -174,10 +175,15 @@ export function serializeValue(value: unknown): string { if (Array.isArray(value)) return serializeArray(value) + if (isTupleParam(value)) return serializeTuple(value.values) + if (value instanceof Map) { const parts: string[] = [] for (const [k, v] of value) { - parts.push(`${escapeStringLiteral(String(k))}:${serializeValue(v)}`) + // Serialize the key by its JS type, not force-quoted: a numeric key binds + // as `42` (an `{'42':…}` string-quoted key is rejected for `Map(Int32,…)`), + // a string key as `'k'`. Matches @clickhouse/client-common formatting. + parts.push(`${serializeValue(k)}:${serializeValue(v)}`) } return `{${parts.join(',')}}` } @@ -205,6 +211,30 @@ function serializeArray(arr: ReadonlyArray): string { return `[${arr.map((x) => serializeValue(x)).join(',')}]` } +function serializeTuple(values: ReadonlyArray): string { + return `(${values.map((x) => serializeValue(x)).join(',')})` +} + +/** + * Detect `@clickhouse/client-common`'s `TupleParam` (members in `.values`, binds + * as the tuple literal `(a,b,c)` — not the `{k:v}` form the generic-object + * branch would otherwise produce). + * + * Matched by shape, not `instanceof`: the instance a caller passes originates + * from their own `@clickhouse/client(-common)` copy, which pnpm (or any nested + * install) may resolve to a different module instance than ours, so an + * `instanceof` against our imported class would spuriously miss it. The class is + * shipped un-minified with its name intact, so the constructor-name check is + * reliable for real instances while staying a private, non-`TupleParam` object + * (which keeps the `{k:v}` map form) untouched. + */ +function isTupleParam(value: object): value is { values: ReadonlyArray } { + return ( + value.constructor?.name === 'TupleParam' && + Array.isArray((value as { values?: unknown }).values) + ) +} + /** * Format a JS value as a ClickHouse **parameter** string for server-side * binding via `chdb_query_with_params` ({name:Type} placeholders). This is NOT diff --git a/test/v3/querybind.test.ts b/test/v3/querybind.test.ts index 0da88f0..a5cabf6 100644 --- a/test/v3/querybind.test.ts +++ b/test/v3/querybind.test.ts @@ -27,6 +27,40 @@ describe('queryBind path A (server-side chdb_query_with_params)', () => { expect(row.m).toEqual({ abc: [1, 2, 3] }) }) + it('binds Tuple and Array(Tuple) params via a TupleParam-shaped value', () => { + // @clickhouse/client-common's TupleParam { values }, reproduced by shape so + // the test needs no dependency (serializeValue matches it structurally). + class TupleParam { + constructor(readonly values: unknown[]) {} + } + expect( + t('SELECT ({pair:Tuple(String, String)}).1', { + pair: new TupleParam(['main*', '^main.*$']), + }), + ).toBe('main*') + // The Array(Tuple(String, String)) case from the trunk check-branch-patterns + // query — previously failed with "cannot be parsed as Array(Tuple(...))". + const rows = queryBind( + `SELECT pair.1 AS glob, pair.2 AS re + FROM (SELECT arrayJoin({pairs:Array(Tuple(String, String))}) AS pair) + ORDER BY glob`, + { + pairs: [ + new TupleParam(['dev*', '^dev.*$']), + new TupleParam(['main*', '^main.*$']), + ], + }, + 'JSONEachRow', + ) + .trim() + .split('\n') + .map((l) => JSON.parse(l)) + expect(rows).toEqual([ + { glob: 'dev*', re: '^dev.*$' }, + { glob: 'main*', re: '^main.*$' }, + ]) + }) + it('treats injection payloads as inert bound data (no interpolation)', () => { const payloads = ["'; DROP TABLE x; --", 'a\\b', "a' OR 1=1 --", 'tab\tend'] for (const p of payloads) { diff --git a/test/v3/serialize.test.ts b/test/v3/serialize.test.ts index e55e972..5fc54b4 100644 --- a/test/v3/serialize.test.ts +++ b/test/v3/serialize.test.ts @@ -96,9 +96,29 @@ describe('serializeValue (string assertions)', () => { expect(serializeValue(['a', "b'c"])).toBe("['a','b\\'c']") expect(serializeValue(new Int32Array([1, 2]))).toBe('[1,2]') expect(serializeValue(new Map([['k', 1]]))).toBe("{'k':1}") + // Numeric Map key binds unquoted (Map(Int32, …)), string key stays quoted. + expect(serializeValue(new Map([[42, 'v']]))).toBe("{42:'v'}") expect(serializeValue({ a: 1, b: 'x' })).toBe("{'a':1,'b':'x'}") expect(serializeValue([[1, 2], [3]])).toBe('[[1,2],[3]]') }) + + it('serializes a TupleParam as a tuple literal, not an object', () => { + // @clickhouse/client-common's TupleParam { values }. Reproduce its shape + // (matched structurally, not by instanceof) so the test needs no dependency. + class TupleParam { + constructor(readonly values: unknown[]) {} + } + expect(serializeValue(new TupleParam(['main*', '^main.*$']))).toBe( + "('main*','^main.*$')", + ) + expect(serializeValue(new TupleParam([42, 'foo', null]))).toBe("(42,'foo',NULL)") + // Nested inside an Array — the Array(Tuple(...)) param case. + expect( + serializeValue([new TupleParam(['a', 'b']), new TupleParam(['c', 'd'])]), + ).toBe("[('a','b'),('c','d')]") + // A private object literally named-differently keeps the {k:v} map form. + expect(serializeValue({ values: ['a', 'b'] })).toBe("{'values':['a','b']}") + }) }) describe('formatParamValue (server-side {name:Type} binding)', () => { diff --git a/tests/clickhouse-js/skip_list.json b/tests/clickhouse-js/skip_list.json index c7c0124..02a2167 100644 --- a/tests/clickhouse-js/skip_list.json +++ b/tests/clickhouse-js/skip_list.json @@ -226,41 +226,6 @@ "file": "packages/client-node/__tests__/integration/node_streaming_e2e.test.ts", "test": "[Node.js] streaming e2e > should stream a Parquet file", "reason": "Streams a Parquet file into chdb via `INSERT FORMAT Parquet`; chdb's `ParquetV3BlockInputFormat` rejects this specific fixture with `apache::thrift::protocol::TProtocolException: invalid TType`. The server's older Parquet input format accepts it. Engine-version divergence in Parquet decoder." - }, - { - "file": "packages/client-common/__tests__/integration/select_query_binding.test.ts", - "test": "select with query binding > handles tuples in a parametrized query", - "reason": "chdb's `queryBindAsync` serializes Tuple parameter values using `{'field':value}` map-literal form. The engine's parser expects `(value1, value2, ...)` positional form. Real chdb-engine parameter-binding gap for compound types." - }, - { - "file": "packages/client-common/__tests__/integration/select_query_binding.test.ts", - "test": "select with query binding > handles arrays of tuples in a parametrized query", - "reason": "Same chdb parameter-binding gap for Tuple — applies to Array(Tuple(...))." - }, - { - "file": "packages/client-common/__tests__/integration/select_query_binding.test.ts", - "test": "select with query binding > handles maps with tuples in a parametrized query", - "reason": "Same chdb parameter-binding gap for Tuple — applies to Map(K, Tuple(...))." - }, - { - "file": "packages/client-common/__tests__/integration/select_query_binding.test.ts", - "test": "select with query binding > handles maps with nested arrays in a parametrized query", - "reason": "chdb parameter-binding for Map(K, Array(...)) generates a serialization the engine parser rejects." - }, - { - "file": "packages/client-common/__tests__/integration/select_query_binding.test.ts", - "test": "select with query binding > handles maps with nullable values in a parametrized query", - "reason": "chdb parameter-binding for Map(K, Nullable(V)) generates a serialization the engine parser rejects." - }, - { - "file": "packages/client-common/__tests__/integration/select_query_binding.test.ts", - "test": "select with query binding > Nested boolean types > handles boolean in a tuple", - "reason": "Boolean inside Tuple parameter binding hits the same Tuple-serialization divergence above." - }, - { - "file": "packages/client-common/__tests__/integration/select_query_binding.test.ts", - "test": "select with query binding > Nested boolean types > handles boolean in a mixed nested structure", - "reason": "Boolean inside nested Tuple/Map parameter binding — same root cause." } ] }