fix: address 7 P1 findings from v0.5.16 release evaluation #591
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,319 @@ | ||
| /** | ||
| * Unit tests for ClickHouse driver logic: | ||
| * - DDL vs SELECT routing (command vs query) | ||
| * - LIMIT injection and bypass prevention | ||
| * - Truncation detection | ||
| * - Nullable detection from type string | ||
| * - Connection guard (execute before connect) | ||
| * - Binds parameter is silently ignored | ||
| */ | ||
| import { describe, test, expect, mock, beforeEach } from "bun:test" | ||
|
|
||
| // --- Mock @clickhouse/client --- | ||
|
|
||
| let mockCommandCalls: any[] = [] | ||
| let mockQueryCalls: any[] = [] | ||
| let mockQueryResult: any[] = [] | ||
| let mockCloseCalls = 0 | ||
|
|
||
| function resetMocks() { | ||
| mockCommandCalls = [] | ||
| mockQueryCalls = [] | ||
| mockQueryResult = [] | ||
| mockCloseCalls = 0 | ||
| } | ||
|
|
||
| mock.module("@clickhouse/client", () => ({ | ||
| createClient: (_config: any) => ({ | ||
| command: async (opts: any) => { | ||
| mockCommandCalls.push(opts) | ||
| }, | ||
| query: async (opts: any) => { | ||
| mockQueryCalls.push(opts) | ||
| return { json: async () => mockQueryResult } | ||
| }, | ||
| close: async () => { | ||
| mockCloseCalls++ | ||
| }, | ||
| }), | ||
| })) | ||
|
|
||
| // Import after mocking | ||
| const { connect } = await import("../src/clickhouse") | ||
|
|
||
| describe("ClickHouse driver unit tests", () => { | ||
| let connector: Awaited<ReturnType<typeof connect>> | ||
|
|
||
| beforeEach(async () => { | ||
| resetMocks() | ||
| connector = await connect({ host: "localhost", port: 8123 }) | ||
| await connector.connect() | ||
| }) | ||
|
|
||
| // --- DDL vs SELECT routing --- | ||
|
|
||
| describe("DDL routing via client.command()", () => { | ||
| const ddlStatements = [ | ||
| "INSERT INTO t VALUES (1, 'a')", | ||
| "CREATE TABLE t (id UInt32) ENGINE = MergeTree()", | ||
| "DROP TABLE t", | ||
| "ALTER TABLE t ADD COLUMN x String", | ||
| "TRUNCATE TABLE t", | ||
| "OPTIMIZE TABLE t FINAL", | ||
| "SYSTEM RELOAD DICTIONARY", | ||
| "SET max_memory_usage = 1000000", | ||
| ] | ||
|
|
||
| for (const sql of ddlStatements) { | ||
| test(`routes "${sql.slice(0, 40)}..." to client.command()`, async () => { | ||
| const result = await connector.execute(sql) | ||
| expect(mockCommandCalls.length).toBe(1) | ||
| expect(mockQueryCalls.length).toBe(0) | ||
| expect(result.row_count).toBe(0) | ||
| }) | ||
| } | ||
|
|
||
| test("strips trailing semicolons from DDL", async () => { | ||
| await connector.execute("DROP TABLE t; ") | ||
| expect(mockCommandCalls[0].query).toBe("DROP TABLE t") | ||
| }) | ||
| }) | ||
|
|
||
| describe("SELECT routing via client.query()", () => { | ||
| test("routes SELECT to client.query()", async () => { | ||
| mockQueryResult = [{ id: 1, name: "test" }] | ||
| await connector.execute("SELECT id, name FROM t") | ||
| expect(mockQueryCalls.length).toBe(1) | ||
| expect(mockCommandCalls.length).toBe(0) | ||
| }) | ||
|
|
||
| test("routes SHOW to client.query()", async () => { | ||
| mockQueryResult = [{ name: "db1" }] | ||
| await connector.execute("SHOW DATABASES") | ||
| expect(mockQueryCalls.length).toBe(1) | ||
| }) | ||
|
|
||
| test("routes DESCRIBE to client.query()", async () => { | ||
| mockQueryResult = [{ name: "col1", type: "String" }] | ||
| await connector.execute("DESCRIBE TABLE t") | ||
| expect(mockQueryCalls.length).toBe(1) | ||
| }) | ||
|
|
||
| test("routes EXPLAIN to client.query()", async () => { | ||
| mockQueryResult = [{ explain: "ReadFromMergeTree" }] | ||
| await connector.execute("EXPLAIN SELECT 1") | ||
| expect(mockQueryCalls.length).toBe(1) | ||
| }) | ||
| }) | ||
|
|
||
| // --- LIMIT injection --- | ||
|
|
||
| describe("LIMIT injection", () => { | ||
| test("appends LIMIT to SELECT without one", async () => { | ||
| mockQueryResult = [{ id: 1 }] | ||
| await connector.execute("SELECT * FROM t", 10) | ||
| expect(mockQueryCalls[0].query).toContain("LIMIT 11") | ||
| }) | ||
|
|
||
| test("does NOT append LIMIT to SELECT that already has one", async () => { | ||
| mockQueryResult = [{ id: 1 }] | ||
| await connector.execute("SELECT * FROM t LIMIT 5", 10) | ||
| expect(mockQueryCalls[0].query).not.toContain("LIMIT 11") | ||
| }) | ||
|
|
||
| test("does NOT append LIMIT to SHOW/DESCRIBE/EXPLAIN/EXISTS", async () => { | ||
| mockQueryResult = [{ name: "t" }] | ||
|
|
||
| await connector.execute("SHOW TABLES", 10) | ||
| expect(mockQueryCalls[0].query).not.toContain("LIMIT") | ||
|
|
||
| mockQueryCalls = [] | ||
| await connector.execute("DESCRIBE TABLE t", 10) | ||
| expect(mockQueryCalls[0].query).not.toContain("LIMIT") | ||
|
|
||
| mockQueryCalls = [] | ||
| await connector.execute("EXISTS TABLE t", 10) | ||
| expect(mockQueryCalls[0].query).not.toContain("LIMIT") | ||
| }) | ||
|
|
||
| test("does NOT append LIMIT when limit=0 (unlimited)", async () => { | ||
| mockQueryResult = [{ id: 1 }, { id: 2 }] | ||
| await connector.execute("SELECT * FROM t", 0) | ||
| expect(mockQueryCalls[0].query).not.toContain("LIMIT") | ||
| }) | ||
|
|
||
| test("uses default limit=1000 when limit is undefined", async () => { | ||
| mockQueryResult = [{ id: 1 }] | ||
| await connector.execute("SELECT * FROM t") | ||
| expect(mockQueryCalls[0].query).toContain("LIMIT 1001") | ||
| }) | ||
|
|
||
| test("LIMIT in SQL comment does NOT prevent LIMIT injection", async () => { | ||
| mockQueryResult = [{ id: 1 }] | ||
| await connector.execute("SELECT * FROM t -- LIMIT 100", 10) | ||
| // Should still append LIMIT because the comment-stripped SQL has no LIMIT | ||
| expect(mockQueryCalls[0].query).toContain("LIMIT 11") | ||
| }) | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| test("LIMIT in block comment does NOT prevent LIMIT injection", async () => { | ||
| mockQueryResult = [{ id: 1 }] | ||
| await connector.execute("SELECT * FROM t /* LIMIT 50 */", 10) | ||
| expect(mockQueryCalls[0].query).toContain("LIMIT 11") | ||
| }) | ||
|
|
||
| test("real LIMIT in SQL still prevents double LIMIT", async () => { | ||
| mockQueryResult = [{ id: 1 }] | ||
| await connector.execute("SELECT * FROM t LIMIT 5 -- max rows", 10) | ||
| expect(mockQueryCalls[0].query).not.toContain("LIMIT 11") | ||
| }) | ||
| }) | ||
|
|
||
| // --- Truncation detection --- | ||
|
|
||
| describe("truncation detection", () => { | ||
| test("detects truncation when rows exceed limit", async () => { | ||
| mockQueryResult = Array.from({ length: 6 }, (_, i) => ({ id: i })) | ||
| const result = await connector.execute("SELECT * FROM t", 5) | ||
| expect(result.truncated).toBe(true) | ||
| expect(result.row_count).toBe(5) | ||
| expect(result.rows.length).toBe(5) | ||
| }) | ||
|
|
||
| test("no truncation when rows equal limit", async () => { | ||
| mockQueryResult = Array.from({ length: 5 }, (_, i) => ({ id: i })) | ||
| const result = await connector.execute("SELECT * FROM t", 5) | ||
| expect(result.truncated).toBe(false) | ||
| expect(result.row_count).toBe(5) | ||
| }) | ||
|
|
||
| test("no truncation when rows below limit", async () => { | ||
| mockQueryResult = [{ id: 1 }] | ||
| const result = await connector.execute("SELECT * FROM t", 10) | ||
| expect(result.truncated).toBe(false) | ||
| expect(result.row_count).toBe(1) | ||
| }) | ||
|
|
||
| test("limit=0 returns all rows without truncation", async () => { | ||
| mockQueryResult = Array.from({ length: 100 }, (_, i) => ({ id: i })) | ||
| const result = await connector.execute("SELECT * FROM t", 0) | ||
| expect(result.truncated).toBe(false) | ||
| expect(result.row_count).toBe(100) | ||
| }) | ||
|
|
||
| test("empty result returns correctly", async () => { | ||
| mockQueryResult = [] | ||
| const result = await connector.execute("SELECT * FROM t", 10) | ||
| expect(result.row_count).toBe(0) | ||
| expect(result.columns).toEqual([]) | ||
| expect(result.truncated).toBe(false) | ||
| }) | ||
| }) | ||
|
|
||
| // --- Nullable detection --- | ||
|
|
||
| describe("describeTable nullable detection", () => { | ||
| test("detects Nullable(String) as nullable", async () => { | ||
| mockQueryResult = [{ name: "col1", type: "Nullable(String)" }] | ||
| const cols = await connector.describeTable("default", "t") | ||
| expect(cols[0].nullable).toBe(true) | ||
| }) | ||
|
|
||
| test("detects String as non-nullable", async () => { | ||
| mockQueryResult = [{ name: "col1", type: "String" }] | ||
| const cols = await connector.describeTable("default", "t") | ||
| expect(cols[0].nullable).toBe(false) | ||
| }) | ||
|
|
||
| test("detects Nullable(UInt32) as nullable", async () => { | ||
| mockQueryResult = [{ name: "col1", type: "Nullable(UInt32)" }] | ||
| const cols = await connector.describeTable("default", "t") | ||
| expect(cols[0].nullable).toBe(true) | ||
| }) | ||
|
|
||
| test("Array(Nullable(String)) is NOT nullable at column level", async () => { | ||
| // The column itself isn't Nullable — the array elements are | ||
| mockQueryResult = [{ name: "col1", type: "Array(Nullable(String))" }] | ||
| const cols = await connector.describeTable("default", "t") | ||
| expect(cols[0].nullable).toBe(false) | ||
| }) | ||
|
|
||
| test("LowCardinality(Nullable(String)) is NOT nullable at column level", async () => { | ||
| // Nullable is nested inside LowCardinality — column-level is LowCardinality | ||
| mockQueryResult = [{ name: "col1", type: "LowCardinality(Nullable(String))" }] | ||
| const cols = await connector.describeTable("default", "t") | ||
| expect(cols[0].nullable).toBe(false) | ||
| }) | ||
| }) | ||
|
|
||
| // --- Connection guard --- | ||
|
|
||
| describe("connection lifecycle", () => { | ||
| test("execute before connect throws clear error", async () => { | ||
| const freshConnector = await connect({ host: "localhost" }) | ||
| // Don't call connect() | ||
| expect(freshConnector.execute("SELECT 1")).rejects.toThrow("not connected") | ||
| }) | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| test("close is idempotent", async () => { | ||
| await connector.close() | ||
| await connector.close() // should not throw | ||
| expect(mockCloseCalls).toBe(1) // only called once | ||
| }) | ||
| }) | ||
|
|
||
| // --- Binds parameter --- | ||
|
|
||
| describe("binds parameter", () => { | ||
| test("binds parameter is silently ignored", async () => { | ||
| mockQueryResult = [{ id: 1 }] | ||
| // Should not throw — binds are ignored | ||
| const result = await connector.execute("SELECT 1", 10, ["unused", "binds"]) | ||
| expect(result.row_count).toBe(1) | ||
| }) | ||
|
|
||
| test("empty binds array works fine", async () => { | ||
| mockQueryResult = [{ id: 1 }] | ||
| const result = await connector.execute("SELECT 1", 10, []) | ||
| expect(result.row_count).toBe(1) | ||
| }) | ||
| }) | ||
|
|
||
| // --- Column mapping --- | ||
|
|
||
| describe("result format", () => { | ||
| test("maps rows to column-ordered arrays", async () => { | ||
| mockQueryResult = [ | ||
| { id: 1, name: "alice", age: 30 }, | ||
| { id: 2, name: "bob", age: 25 }, | ||
| ] | ||
| const result = await connector.execute("SELECT * FROM t", 10) | ||
| expect(result.columns).toEqual(["id", "name", "age"]) | ||
| expect(result.rows).toEqual([ | ||
| [1, "alice", 30], | ||
| [2, "bob", 25], | ||
| ]) | ||
| }) | ||
| }) | ||
|
|
||
| // --- listTables type detection --- | ||
|
|
||
| describe("listTables engine-to-type mapping", () => { | ||
| test("MergeTree engines map to table", async () => { | ||
| mockQueryResult = [{ name: "t1", engine: "MergeTree" }] | ||
| const tables = await connector.listTables("default") | ||
| expect(tables[0].type).toBe("table") | ||
| }) | ||
|
|
||
| test("MaterializedView maps to view", async () => { | ||
| mockQueryResult = [{ name: "v1", engine: "MaterializedView" }] | ||
| const tables = await connector.listTables("default") | ||
| expect(tables[0].type).toBe("view") | ||
| }) | ||
|
|
||
| test("View maps to view", async () => { | ||
| mockQueryResult = [{ name: "v2", engine: "View" }] | ||
| const tables = await connector.listTables("default") | ||
| expect(tables[0].type).toBe("view") | ||
| }) | ||
| }) | ||
| }) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The headline count now exceeds what this page documents.
This page still only contains 10 concrete warehouse sections, so the new opener leaves two supported types undocumented here. Either add the missing sections or rephrase the intro so it matches what this page actually covers.
🤖 Prompt for AI Agents