From c684bf4b0dcb9c90860a5a05d5834d58b46c037c Mon Sep 17 00:00:00 2001
From: Vijay Yadav <vjyadav194@gmail.com>
Date: Mon, 30 Mar 2026 19:16:11 -0400
Subject: [PATCH] fix: address ClickHouse driver review findings

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>
---
 packages/drivers/test/clickhouse-unit.test.ts | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/packages/drivers/test/clickhouse-unit.test.ts b/packages/drivers/test/clickhouse-unit.test.ts
index 77ff48dcd..8620cfa74 100644
--- a/packages/drivers/test/clickhouse-unit.test.ts
+++ b/packages/drivers/test/clickhouse-unit.test.ts
@@ -203,6 +203,38 @@ describe("ClickHouse driver unit tests", () => {
       await connector.execute("SELECT 'it''s -- LIMIT 5' FROM t", 10)
       expect(mockQueryCalls[0].query).toContain("LIMIT 11")
     })
+
+    test("leading block comment does NOT bypass LIMIT injection", async () => {
+      mockQueryResult = [{ id: 1 }]
+      await connector.execute("/* analytics query */ SELECT * FROM t", 10)
+      expect(mockQueryCalls[0].query).toContain("LIMIT 11")
+    })
+
+    test("multi-line block comment before SELECT does NOT bypass LIMIT", async () => {
+      mockQueryResult = [{ id: 1 }]
+      await connector.execute("/*\n * Report query\n * Author: test\n */\nSELECT * FROM t", 10)
+      expect(mockQueryCalls[0].query).toContain("LIMIT 11")
+    })
+
+    test("backslash-escaped string does NOT break LIMIT check", async () => {
+      mockQueryResult = [{ id: 1 }]
+      await connector.execute("SELECT 'path\\\\to\\'s -- LIMIT 5' FROM t", 10)
+      expect(mockQueryCalls[0].query).toContain("LIMIT 11")
+    })
+
+    test("leading comment does NOT misroute SELECT as DDL", async () => {
+      mockQueryResult = [{ id: 1 }]
+      await connector.execute("-- INSERT note\nSELECT * FROM t", 10)
+      expect(mockQueryCalls.length).toBe(1)
+      expect(mockCommandCalls.length).toBe(0)
+    })
+
+    test("block comment containing DDL keyword does NOT misroute SELECT", async () => {
+      mockQueryResult = [{ id: 1 }]
+      await connector.execute("/* DROP TABLE note */ SELECT * FROM t", 10)
+      expect(mockQueryCalls.length).toBe(1)
+      expect(mockCommandCalls.length).toBe(0)
+    })
   })
 
   // --- Truncation detection ---
@@ -285,6 +317,31 @@ describe("ClickHouse driver unit tests", () => {
       const cols = await connector.describeTable("default", "t")
       expect(cols[0].nullable).toBe(false)
     })
+
+    test("LowCardinality(Nullable(FixedString(32))) IS nullable — nested inner type", async () => {
+      mockQueryResult = [{ name: "col1", type: "LowCardinality(Nullable(FixedString(32)))" }]
+      const cols = await connector.describeTable("default", "t")
+      expect(cols[0].nullable).toBe(true)
+    })
+
+    test("Map(String, Nullable(UInt32)) is NOT nullable at column level", async () => {
+      // The map values are nullable, but the column itself is not
+      mockQueryResult = [{ name: "col1", type: "Map(String, Nullable(UInt32))" }]
+      const cols = await connector.describeTable("default", "t")
+      expect(cols[0].nullable).toBe(false)
+    })
+
+    test("Tuple(Nullable(String), UInt32) is NOT nullable at column level", async () => {
+      mockQueryResult = [{ name: "col1", type: "Tuple(Nullable(String), UInt32)" }]
+      const cols = await connector.describeTable("default", "t")
+      expect(cols[0].nullable).toBe(false)
+    })
+
+    test("handles empty/missing type gracefully", async () => {
+      mockQueryResult = [{ name: "col1", type: undefined }]
+      const cols = await connector.describeTable("default", "t")
+      expect(cols[0].nullable).toBe(false)
+    })
   })
 
   // --- Connection guard ---