Fix query(): insert FORMAT before a trailing SETTINGS clause

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<string> {
+  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);
+  });
+});

packages/client-common/src/client.ts

@@ -259,7 +259,10 @@ export class ClickHouseClient<Stream = unknown> {
    * 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 <format>` to the end of {@link QueryParams.query}.
+   * this method appends `FORMAT <format>` to {@link QueryParams.query}. If the query ends with a
+   * top-level `SETTINGS` clause, `FORMAT` is inserted right before it (producing
+   * `... FORMAT <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 <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 <name> = ...` 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 `<name> = ...` 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 <name> = ...` 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
+ *  `<name> = ...` 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--) {

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
 

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