diff --git a/packages/client-common/__tests__/unit/format_query.test.ts b/packages/client-common/__tests__/unit/format_query.test.ts new file mode 100644 index 00000000..220c98ec --- /dev/null +++ b/packages/client-common/__tests__/unit/format_query.test.ts @@ -0,0 +1,193 @@ +import { describe, it, expect } from "vitest"; +import type { DataFormat } from "../../src/index"; +import { ClickHouseClient } from "../../src/client"; + +/** Builds a client whose connection records the final wire query string passed + * to `connection.query()`, so we can assert exactly how `client.query()` + * assembles the `FORMAT` clause — in particular, where it places `FORMAT` + * relative to a user-supplied trailing `SETTINGS` clause (see #970). */ +function createCapturingClient(): { + client: ClickHouseClient; + lastQuery: () => string; +} { + let captured = ""; + const client = new ClickHouseClient({ + url: "http://localhost:8123", + impl: { + make_connection: () => + ({ + query: async (params: { query: string }) => { + captured = params.query; + return { + stream: {} as any, + query_id: "q-1", + response_headers: {}, + }; + }, + close: async () => {}, + }) as any, + make_result_set: () => ({}) as any, + values_encoder: () => ({}) as any, + }, + }); + return { client, lastQuery: () => captured }; +} + +async function wireQuery(query: string, format?: DataFormat): Promise { + const { client, lastQuery } = createCapturingClient(); + await client.query({ query, format }); + return lastQuery(); +} + +describe("client.query FORMAT clause placement (#970)", () => { + it.each([ + { + name: "appends FORMAT at the end when there is no trailing SETTINGS clause", + query: "SELECT 1", + format: undefined, + expected: "SELECT 1 \nFORMAT JSON", + }, + { + name: "injects FORMAT before a trailing SETTINGS clause instead of after it", + query: "SELECT * FROM t WHERE a = 1 SETTINGS max_threads = 1", + format: "JSONEachRow" as DataFormat, + expected: + "SELECT * FROM t WHERE a = 1 \nFORMAT JSONEachRow SETTINGS max_threads = 1", + }, + { + name: "handles the DESC format(...) reproduction from the issue", + query: + "DESC format(JSONEachRow, '{\"id\":1}') SETTINGS schema_inference_hints = 'age LowCardinality(UInt8)', allow_suspicious_low_cardinality_types = 1", + format: "JSONEachRow" as DataFormat, + expected: + "DESC format(JSONEachRow, '{\"id\":1}') \nFORMAT JSONEachRow SETTINGS schema_inference_hints = 'age LowCardinality(UInt8)', allow_suspicious_low_cardinality_types = 1", + }, + { + name: "ignores the word SETTINGS inside a string literal", + query: "SELECT 'a SETTINGS b' AS x", + format: undefined, + expected: "SELECT 'a SETTINGS b' AS x \nFORMAT JSON", + }, + { + name: "ignores a settings identifier that is not a SETTINGS clause", + query: "SELECT name FROM system.settings", + format: undefined, + expected: "SELECT name FROM system.settings \nFORMAT JSON", + }, + { + name: "does not treat a subquery's own SETTINGS clause as trailing", + query: "SELECT * FROM (SELECT 1 SETTINGS max_threads = 1)", + format: undefined, + expected: + "SELECT * FROM (SELECT 1 SETTINGS max_threads = 1) \nFORMAT JSON", + }, + { + name: "injects FORMAT before the outer trailing SETTINGS, not the subquery's", + query: + "SELECT * FROM (SELECT 1 SETTINGS max_threads = 1) SETTINGS max_block_size = 100", + format: undefined, + expected: + "SELECT * FROM (SELECT 1 SETTINGS max_threads = 1) \nFORMAT JSON SETTINGS max_block_size = 100", + }, + { + name: "ignores SETTINGS inside a line comment", + query: "SELECT 1 -- SETTINGS x = 1", + format: undefined, + expected: "SELECT 1 -- SETTINGS x = 1 \nFORMAT JSON", + }, + { + name: "ignores SETTINGS inside a block comment but honors a real trailing clause after it", + query: "SELECT 1 /* SETTINGS y = 2 */ SETTINGS max_threads = 1", + format: undefined, + expected: + "SELECT 1 /* SETTINGS y = 2 */ \nFORMAT JSON SETTINGS max_threads = 1", + }, + { + name: "strips a trailing semicolon before placing the FORMAT clause", + query: "SELECT 1 SETTINGS max_threads = 1;", + format: undefined, + expected: "SELECT 1 \nFORMAT JSON SETTINGS max_threads = 1", + }, + { + name: "ignores SETTINGS inside a line comment", + query: "SELECT 1 # SETTINGS z = 3", + format: undefined, + expected: "SELECT 1 # SETTINGS z = 3 \nFORMAT JSON", + }, + { + name: "ignores SETTINGS inside a dollar-quoted (heredoc) string literal", + query: "SELECT $$a SETTINGS x = 1$$ AS c", + format: undefined, + expected: "SELECT $$a SETTINGS x = 1$$ AS c \nFORMAT JSON", + }, + { + name: "skips a tagged heredoc yet still injects before a real trailing SETTINGS clause", + query: "SELECT $q$a SETTINGS y = 2$q$ AS c SETTINGS max_threads = 1", + format: undefined, + expected: + "SELECT $q$a SETTINGS y = 2$q$ AS c \nFORMAT JSON SETTINGS max_threads = 1", + }, + { + name: "detects a SETTINGS clause with no spaces around the equals sign", + query: "SELECT 1 SETTINGS max_threads=1", + format: undefined, + expected: "SELECT 1 \nFORMAT JSON SETTINGS max_threads=1", + }, + { + name: "ignores SETTINGS inside a string literal that uses a backslash-escaped quote", + query: "SELECT 'a\\' SETTINGS b' AS x", + format: undefined, + expected: "SELECT 'a\\' SETTINGS b' AS x \nFORMAT JSON", + }, + { + name: "ignores SETTINGS inside a string literal that uses a doubled-quote escape", + query: "SELECT 'a'' SETTINGS b' AS x", + format: undefined, + expected: "SELECT 'a'' SETTINGS b' AS x \nFORMAT JSON", + }, + { + name: "treats an unterminated string literal as running to the end (SETTINGS inside is ignored)", + query: "SELECT 'no end SETTINGS x = 1", + format: undefined, + expected: "SELECT 'no end SETTINGS x = 1 \nFORMAT JSON", + }, + { + name: "treats a bare $ (not a dollar-quote opener) as an ordinary char and still finds a trailing SETTINGS", + query: "SELECT 1 $ SETTINGS max_threads = 1", + format: undefined, + expected: "SELECT 1 $ \nFORMAT JSON SETTINGS max_threads = 1", + }, + { + name: "treats a trailing $tag with no closing delimiter as an ordinary token", + query: "SELECT 1 AS $tag", + format: undefined, + expected: "SELECT 1 AS $tag \nFORMAT JSON", + }, + { + name: "treats an unterminated dollar-quoted string as running to the end (SETTINGS inside is ignored)", + query: "SELECT $$abc SETTINGS x = 1", + format: undefined, + expected: "SELECT $$abc SETTINGS x = 1 \nFORMAT JSON", + }, + { + name: "resumes scanning after a line comment's newline and finds a trailing SETTINGS on the next line", + query: "SELECT 1 -- c\n SETTINGS max_threads = 1", + format: undefined, + expected: "SELECT 1 -- c \nFORMAT JSON SETTINGS max_threads = 1", + }, + { + name: "treats an unterminated block comment as running to the end (SETTINGS inside is ignored)", + query: "SELECT 1 /* unterminated SETTINGS x = 1", + format: undefined, + expected: "SELECT 1 /* unterminated SETTINGS x = 1 \nFORMAT JSON", + }, + { + name: "tolerates an unbalanced closing bracket (depth never goes negative) and still finds a trailing SETTINGS", + query: "SELECT 1) SETTINGS max_threads = 1", + format: undefined, + expected: "SELECT 1) \nFORMAT JSON SETTINGS max_threads = 1", + }, + ])("$name", async ({ query, format, expected }) => { + expect(await wireQuery(query, format)).toBe(expected); + }); +}); diff --git a/packages/client-common/src/client.ts b/packages/client-common/src/client.ts index a9795501..862519a7 100644 --- a/packages/client-common/src/client.ts +++ b/packages/client-common/src/client.ts @@ -259,7 +259,10 @@ export class ClickHouseClient { * Returns an implementation of {@link BaseResultSet}. * * The `FORMAT` clause should be specified separately via {@link QueryParams.format} (default is `JSON`); - * this method will always append `FORMAT ` to the end of {@link QueryParams.query}. + * this method appends `FORMAT ` to {@link QueryParams.query}. If the query ends with a + * top-level `SETTINGS` clause, `FORMAT` is inserted right before it (producing + * `... FORMAT SETTINGS ...`), since ClickHouse requires the `SETTINGS` clause to stay last + * for some statements (e.g. `DESCRIBE` on older servers); otherwise `FORMAT` is appended at the very end. * If the query already contains a `FORMAT` clause, ClickHouse will return a syntax error due to a duplicate `FORMAT`. * This is intended behavior. * Use {@link ClickHouseClient.insert} for data insertion, {@link ClickHouseClient.command} for DDLs, @@ -700,9 +703,150 @@ function setResponseSpanAttributes( function formatQuery(query: string, format: DataFormat): string { query = query.trim(); query = removeTrailingSemi(query); + // A user-supplied query may end with a top-level `SETTINGS ...` clause. + // ClickHouse expects `FORMAT` to come *before* such a clause + // (`... FORMAT SETTINGS ...`); appending `FORMAT` after it produces + // invalid SQL on servers whose parser requires `SETTINGS` to be the trailing + // clause of the statement (e.g. `DESCRIBE` on ClickHouse < 24.x). + const settingsIndex = trailingSettingsClauseIndex(query); + if (settingsIndex !== -1) { + const head = query.slice(0, settingsIndex).trimEnd(); + const settingsClause = query.slice(settingsIndex); + return head + " \nFORMAT " + format + " " + settingsClause; + } return query + " \nFORMAT " + format; } +/** Matches a `SETTINGS = ...` clause anchored at a probe position. Used + * to distinguish a real trailing `SETTINGS` clause from a `settings` + * identifier/column (which is not followed by a ` = ...` settings list). + * The `y` (sticky) flag anchors the match at `lastIndex`, so a caller probes a + * position via {@link matchesSettingsClauseAt} without allocating a substring + * per probe — keeping the overall scan O(n). */ +const settingsClauseRe = /settings\s+\w+\s*=/iy; + +/** Tests whether a `SETTINGS = ...` clause begins exactly at `pos` in + * `query`. Uses the sticky {@link settingsClauseRe} anchored at `pos` via + * `lastIndex`, so no substring is allocated (unlike `re.test(query.slice(pos))`). + * `lastIndex` is reset on every call, so no state leaks between probes. */ +function matchesSettingsClauseAt(query: string, pos: number): boolean { + settingsClauseRe.lastIndex = pos; + return settingsClauseRe.test(query); +} + +/** Returns the index of the trailing top-level `SETTINGS` clause in `query`, or + * `-1` if there is none. Only a `SETTINGS` keyword that is outside string + * literals, comments, and brackets — and is immediately followed by a + * ` = ...` settings list — is treated as a clause. This deliberately + * ignores `SETTINGS` inside a string literal, a `settings` identifier/column, + * and a subquery's own `SETTINGS` clause (which lives inside parentheses). */ +function trailingSettingsClauseIndex(query: string): number { + let depth = 0; + let index = -1; + let i = 0; + const len = query.length; + while (i < len) { + const ch = query[i]; + if (ch === "'" || ch === '"' || ch === "`") { + i = skipQuoted(query, i); + continue; + } + if (ch === "$") { + const afterDollarQuote = skipDollarQuoted(query, i); + if (afterDollarQuote !== -1) { + i = afterDollarQuote; + continue; + } + } + if (ch === "-" && query[i + 1] === "-") { + i = skipToLineEnd(query, i + 2); + continue; + } + if (ch === "#") { + i = skipToLineEnd(query, i + 1); + continue; + } + if (ch === "/" && query[i + 1] === "*") { + i = skipBlockComment(query, i + 2); + continue; + } + if (ch === "(" || ch === "[" || ch === "{") { + depth++; + } else if (ch === ")" || ch === "]" || ch === "}") { + if (depth > 0) depth--; + } else if ( + depth === 0 && + (ch === "s" || ch === "S") && + !isWordChar(query[i - 1]) && + matchesSettingsClauseAt(query, i) + ) { + // Record the last top-level match; in a well-formed query there is at most + // one, and it is the trailing clause. + index = i; + } + i++; + } + return index; +} + +function isWordChar(ch: string | undefined): boolean { + return ch !== undefined && /[A-Za-z0-9_]/.test(ch); +} + +/** Given the index of an opening quote (`'`, `"`, or `` ` ``), returns the index + * just past the matching closing quote, skipping backslash escapes and doubled + * quotes (`''`, `""`, `` `` ``). An unterminated literal consumes to the end. */ +function skipQuoted(query: string, openIndex: number): number { + const quote = query[openIndex]; + const len = query.length; + let i = openIndex + 1; + while (i < len) { + const ch = query[i]; + if (ch === "\\") { + i += 2; + continue; + } + if (ch === quote) { + if (query[i + 1] === quote) { + i += 2; + continue; + } + return i + 1; + } + i++; + } + return len; +} + +/** If a dollar-quoted string literal (`$$...$$` or `$tag$...$tag$`, ClickHouse + * heredoc syntax) opens at `openIndex`, returns the index just past its closing + * delimiter; otherwise `-1` (the `$` is not a heredoc opener). An unterminated + * literal consumes to the end. */ +function skipDollarQuoted(query: string, openIndex: number): number { + const len = query.length; + let j = openIndex + 1; + while (j < len && query[j] !== "$") { + // A heredoc tag is an (optionally empty) identifier; anything else means + // this `$` does not open a dollar-quoted literal. + if (!isWordChar(query[j])) return -1; + j++; + } + if (j >= len) return -1; + const delimiter = query.slice(openIndex, j + 1); // e.g. `$$` or `$tag$` + const closeIndex = query.indexOf(delimiter, j + 1); + return closeIndex === -1 ? len : closeIndex + delimiter.length; +} + +function skipToLineEnd(query: string, from: number): number { + const newlineIndex = query.indexOf("\n", from); + return newlineIndex === -1 ? query.length : newlineIndex + 1; +} + +function skipBlockComment(query: string, from: number): number { + const endIndex = query.indexOf("*/", from); + return endIndex === -1 ? query.length : endIndex + 2; +} + function removeTrailingSemi(query: string) { let lastNonSemiIdx = query.length; for (let i = lastNonSemiIdx; i > 0; i--) { diff --git a/packages/client-node/CHANGELOG.md b/packages/client-node/CHANGELOG.md index 435f9b73..dccce664 100644 --- a/packages/client-node/CHANGELOG.md +++ b/packages/client-node/CHANGELOG.md @@ -15,8 +15,11 @@ ## Bug fixes - Fixed `Array(Date)` / `Array(Date32)` query-parameter binding (and other temporal element types nested in arrays, tuples, and maps). A JS `Date` inside a container was serialized as a bare Unix timestamp (e.g. `[1683244800]`), which the server's `Array(Date)` element parser rejects (`CANNOT_PARSE_INPUT_ASSERTION_FAILED`). Container-nested `Date` values are now emitted as a quoted UTC date string (e.g. `['2023-05-05']`), the one encoding every temporal element type accepts. Note: a `Date` used inside `Array(DateTime)` / `Array(DateTime64)` is now bound at day precision (the time-of-day is dropped), since date-only is the only form `Array(Date)` accepts; scalar `Date` / `DateTime` binding is unchanged. ([#947]) +- Fixed `query()` placing the `FORMAT` clause _after_ a user-supplied trailing `SETTINGS` clause, which produced invalid SQL on ClickHouse servers that require `SETTINGS` to be the last clause of the statement (e.g. `DESCRIBE` on servers older than 24.x). `FORMAT` is now inserted before a trailing top-level `SETTINGS` clause — e.g. `SELECT 1 SETTINGS max_threads = 1` is sent as `SELECT 1 FORMAT JSON SETTINGS max_threads = 1`. A `SETTINGS` keyword inside a string literal, comment, or subquery is left untouched. ([#970], [#972]) [#947]: https://github.com/ClickHouse/clickhouse-js/pull/947 +[#970]: https://github.com/ClickHouse/clickhouse-js/issues/970 +[#972]: https://github.com/ClickHouse/clickhouse-js/pull/972 # 1.23.1 diff --git a/packages/client-web/CHANGELOG.md b/packages/client-web/CHANGELOG.md index 12fb839c..291e856d 100644 --- a/packages/client-web/CHANGELOG.md +++ b/packages/client-web/CHANGELOG.md @@ -15,8 +15,11 @@ ## Bug fixes - Fixed `Array(Date)` / `Array(Date32)` query-parameter binding (and other temporal element types nested in arrays, tuples, and maps). A JS `Date` inside a container was serialized as a bare Unix timestamp (e.g. `[1683244800]`), which the server's `Array(Date)` element parser rejects (`CANNOT_PARSE_INPUT_ASSERTION_FAILED`). Container-nested `Date` values are now emitted as a quoted UTC date string (e.g. `['2023-05-05']`), the one encoding every temporal element type accepts. Note: a `Date` used inside `Array(DateTime)` / `Array(DateTime64)` is now bound at day precision (the time-of-day is dropped), since date-only is the only form `Array(Date)` accepts; scalar `Date` / `DateTime` binding is unchanged. ([#947]) +- Fixed `query()` placing the `FORMAT` clause _after_ a user-supplied trailing `SETTINGS` clause, which produced invalid SQL on ClickHouse servers that require `SETTINGS` to be the last clause of the statement (e.g. `DESCRIBE` on servers older than 24.x). `FORMAT` is now inserted before a trailing top-level `SETTINGS` clause — e.g. `SELECT 1 SETTINGS max_threads = 1` is sent as `SELECT 1 FORMAT JSON SETTINGS max_threads = 1`. A `SETTINGS` keyword inside a string literal, comment, or subquery is left untouched. ([#970], [#972]) [#947]: https://github.com/ClickHouse/clickhouse-js/pull/947 +[#970]: https://github.com/ClickHouse/clickhouse-js/issues/970 +[#972]: https://github.com/ClickHouse/clickhouse-js/pull/972 # 1.23.1