Skip to content

Commit 8f52e99

Browse files
VJ-yadavVijay Yadav
andauthored
fix: address ClickHouse driver review findings (#599)
Add comprehensive edge-case tests for all 4 findings from the 6-model code review panel (issue #592): - Nullable: LowCardinality(Nullable(FixedString(32))), Map/Tuple wrappers, undefined type fallback - SQL parsing: leading block comments, multi-line comments, backslash escapes in strings, DDL keywords inside comments Fixes #592 Co-authored-by: Vijay Yadav <vijay@studentsucceed.com>
1 parent 0203b6a commit 8f52e99

File tree

1 file changed

+57
-0
lines changed

1 file changed

+57
-0
lines changed

packages/drivers/test/clickhouse-unit.test.ts

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,38 @@ describe("ClickHouse driver unit tests", () => {
203203
await connector.execute("SELECT 'it''s -- LIMIT 5' FROM t", 10)
204204
expect(mockQueryCalls[0].query).toContain("LIMIT 11")
205205
})
206+
207+
test("leading block comment does NOT bypass LIMIT injection", async () => {
208+
mockQueryResult = [{ id: 1 }]
209+
await connector.execute("/* analytics query */ SELECT * FROM t", 10)
210+
expect(mockQueryCalls[0].query).toContain("LIMIT 11")
211+
})
212+
213+
test("multi-line block comment before SELECT does NOT bypass LIMIT", async () => {
214+
mockQueryResult = [{ id: 1 }]
215+
await connector.execute("/*\n * Report query\n * Author: test\n */\nSELECT * FROM t", 10)
216+
expect(mockQueryCalls[0].query).toContain("LIMIT 11")
217+
})
218+
219+
test("backslash-escaped string does NOT break LIMIT check", async () => {
220+
mockQueryResult = [{ id: 1 }]
221+
await connector.execute("SELECT 'path\\\\to\\'s -- LIMIT 5' FROM t", 10)
222+
expect(mockQueryCalls[0].query).toContain("LIMIT 11")
223+
})
224+
225+
test("leading comment does NOT misroute SELECT as DDL", async () => {
226+
mockQueryResult = [{ id: 1 }]
227+
await connector.execute("-- INSERT note\nSELECT * FROM t", 10)
228+
expect(mockQueryCalls.length).toBe(1)
229+
expect(mockCommandCalls.length).toBe(0)
230+
})
231+
232+
test("block comment containing DDL keyword does NOT misroute SELECT", async () => {
233+
mockQueryResult = [{ id: 1 }]
234+
await connector.execute("/* DROP TABLE note */ SELECT * FROM t", 10)
235+
expect(mockQueryCalls.length).toBe(1)
236+
expect(mockCommandCalls.length).toBe(0)
237+
})
206238
})
207239

208240
// --- Truncation detection ---
@@ -285,6 +317,31 @@ describe("ClickHouse driver unit tests", () => {
285317
const cols = await connector.describeTable("default", "t")
286318
expect(cols[0].nullable).toBe(false)
287319
})
320+
321+
test("LowCardinality(Nullable(FixedString(32))) IS nullable — nested inner type", async () => {
322+
mockQueryResult = [{ name: "col1", type: "LowCardinality(Nullable(FixedString(32)))" }]
323+
const cols = await connector.describeTable("default", "t")
324+
expect(cols[0].nullable).toBe(true)
325+
})
326+
327+
test("Map(String, Nullable(UInt32)) is NOT nullable at column level", async () => {
328+
// The map values are nullable, but the column itself is not
329+
mockQueryResult = [{ name: "col1", type: "Map(String, Nullable(UInt32))" }]
330+
const cols = await connector.describeTable("default", "t")
331+
expect(cols[0].nullable).toBe(false)
332+
})
333+
334+
test("Tuple(Nullable(String), UInt32) is NOT nullable at column level", async () => {
335+
mockQueryResult = [{ name: "col1", type: "Tuple(Nullable(String), UInt32)" }]
336+
const cols = await connector.describeTable("default", "t")
337+
expect(cols[0].nullable).toBe(false)
338+
})
339+
340+
test("handles empty/missing type gracefully", async () => {
341+
mockQueryResult = [{ name: "col1", type: undefined }]
342+
const cols = await connector.describeTable("default", "t")
343+
expect(cols[0].nullable).toBe(false)
344+
})
288345
})
289346

290347
// --- Connection guard ---

0 commit comments

Comments
 (0)