Skip to content

Commit 5d0ada3

Browse files
authored
fix: address 7 P1 findings from v0.5.16 release evaluation (#591)
* fix: address 7 P1 findings from v0.5.16 release evaluation (#590) ClickHouse driver: - Silently ignore `binds` param (was throwing, inconsistent with other drivers) - Strip SQL comments before LIMIT check to prevent `-- LIMIT` bypass - Remove redundant `hasDML` regex (dead code after `isDDL` routing) - Replace fragile `position(type, 'Nullable')` SQL with TS regex on type string - Add connection guard: `execute()` before `connect()` throws clear error Query history: - Rename `{days:UInt32}` placeholders to `__DAYS__`/`__LIMIT__` to avoid confusion with ClickHouse native query parameter syntax Docs: - Update warehouse count from 10 to 12 Tests: - Add 39 unit tests for ClickHouse driver (DDL routing, LIMIT injection, truncation, nullable, connection lifecycle, binds, column mapping) - Remove stale "ClickHouse unsupported" test * fix: address code review findings for ClickHouse driver (#592) SQL parsing hardening (from 6-model consensus review): - Strip string literals before comments to prevent false matches - Run `supportsLimit` and `isDDL` on cleaned SQL (fixes leading comment bypass) - Append LIMIT with newline instead of space (fixes trailing comment bypass) Nullable detection: - Fix `LowCardinality(Nullable(T))` regression — `LowCardinality` is a storage optimization, not a semantic wrapper; column IS nullable Tests: - Fix `LowCardinality(Nullable(String))` assertion to `true` - Add `LowCardinality(String)` non-nullable test - Add trailing comment, leading comment, string literal LIMIT bypass tests - Remove residual no-op comment in `driver-security.test.ts` * fix: address CodeRabbit PR comments — docs, test assertions, await - Add Oracle and SQLite docs sections to warehouses.md (were supported but undocumented, making "12 warehouse types" claim incomplete) - Use strict `toBe` assertions for LIMIT comment bypass tests to verify exact query shape (not just substring) - Add missing `await` on `rejects.toThrow` to prevent flaky test * fix: add connection guards to listSchemas, listTables, describeTable Add consistent not-connected guard to all ClickHouse query methods, matching the guard already present in execute(). Without this, calling these methods before connect() would throw an unhelpful TypeError instead of a clear error message. * fix: handle doubled-quote string escaping in SQL cleaning regex The string literal stripping regex now handles ClickHouse's doubled-quote escape convention (`'it''s'`) in addition to backslash escaping (`\'`). * test: add WITH...SELECT LIMIT injection tests Cover the CTE branch of LIMIT injection logic — both appending LIMIT to a bare WITH...SELECT and skipping when one already exists.
1 parent 99270e5 commit 5d0ada3

File tree

5 files changed

+446
-28
lines changed

5 files changed

+446
-28
lines changed

docs/docs/configure/warehouses.md

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# Warehouses
22

3-
Altimate Code connects to 10 warehouse types. Configure them in `.altimate-code/connections.json` (project-local) or `~/.altimate-code/connections.json` (global).
3+
Altimate Code connects to 12 warehouse types. Configure them in `.altimate-code/connections.json` (project-local) or `~/.altimate-code/connections.json` (global).
44

55
## Configuration
66

@@ -348,6 +348,53 @@ If you're already authenticated via `gcloud`, omit `credentials_path`:
348348
!!! info "Server compatibility"
349349
The ClickHouse driver supports ClickHouse server versions 23.3 and later, covering all non-EOL releases. This includes LTS releases 23.8, 24.3, 24.8, and all stable releases through the current version.
350350

351+
## Oracle
352+
353+
```json
354+
{
355+
"oracle-prod": {
356+
"type": "oracle",
357+
"host": "localhost",
358+
"port": 1521,
359+
"service_name": "ORCL",
360+
"user": "analyst",
361+
"password": "{env:ORACLE_PASSWORD}"
362+
}
363+
}
364+
```
365+
366+
| Field | Required | Description |
367+
|-------|----------|-------------|
368+
| `connection_string` | No | Full connect string (alternative to individual fields, e.g. `host:1521/ORCL`) |
369+
| `host` | No | Hostname (default: `127.0.0.1`) |
370+
| `port` | No | Port (default: `1521`) |
371+
| `service_name` | No | Oracle service name (default: `ORCL`) |
372+
| `database` | No | Alias for `service_name` |
373+
| `user` | No | Username |
374+
| `password` | No | Password |
375+
376+
!!! info "Pure JavaScript driver"
377+
The Oracle driver uses `oracledb` in thin mode (pure JavaScript) — no Oracle Instant Client installation is required.
378+
379+
## SQLite
380+
381+
```json
382+
{
383+
"dev-sqlite": {
384+
"type": "sqlite",
385+
"path": "./dev.sqlite"
386+
}
387+
}
388+
```
389+
390+
| Field | Required | Description |
391+
|-------|----------|-------------|
392+
| `path` | No | Database file path. Omit or use `":memory:"` for in-memory |
393+
| `readonly` | No | Open in read-only mode (default: `false`) |
394+
395+
!!! note
396+
SQLite uses Bun's built-in `bun:sqlite` driver. WAL journal mode is enabled automatically for writable databases.
397+
351398
## SQL Server
352399

353400
```json

packages/drivers/src/clickhouse.ts

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -60,17 +60,25 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
6060
client = createClient(clientConfig)
6161
},
6262

63-
async execute(sql: string, limit?: number, binds?: any[]): Promise<ConnectorResult> {
64-
if (binds && binds.length > 0) {
65-
throw new Error("ClickHouse driver does not support parameterized binds — use ClickHouse query parameters instead")
63+
async execute(sql: string, limit?: number, _binds?: any[]): Promise<ConnectorResult> {
64+
if (!client) {
65+
throw new Error("ClickHouse client not connected — call connect() first")
6666
}
6767
const effectiveLimit = limit === undefined ? 1000 : limit
6868
let query = sql
69+
70+
// Strip string literals, then comments, for accurate SQL heuristic checks.
71+
// This prevents comment-like content inside strings from fooling detection,
72+
// and ensures leading/trailing comments don't hide keywords.
73+
const sqlCleaned = sql
74+
.replace(/'(?:[^'\\]|\\.|\'{2})*'/g, "") // strip single-quoted strings (handles \' and '' escaping)
75+
.replace(/--[^\n]*/g, "") // strip line comments
76+
.replace(/\/\*[\s\S]*?\*\//g, "") // strip block comments
77+
6978
// Only SELECT and WITH...SELECT support LIMIT — SHOW/DESCRIBE/EXPLAIN/EXISTS do not
70-
const supportsLimit = /^\s*(SELECT|WITH)\b/i.test(sql)
79+
const supportsLimit = /^\s*(SELECT|WITH)\b/i.test(sqlCleaned)
7180
const isDDL =
72-
/^\s*(INSERT|CREATE|DROP|ALTER|TRUNCATE|RENAME|ATTACH|DETACH|OPTIMIZE|SYSTEM|SET|USE|GRANT|REVOKE)\b/i.test(sql)
73-
const hasDML = /\b(INSERT|CREATE|DROP|ALTER|TRUNCATE|RENAME|ATTACH|DETACH|OPTIMIZE|SYSTEM)\b/i.test(sql)
81+
/^\s*(INSERT|CREATE|DROP|ALTER|TRUNCATE|RENAME|ATTACH|DETACH|OPTIMIZE|SYSTEM|SET|USE|GRANT|REVOKE)\b/i.test(sqlCleaned)
7482

7583
// DDL/DML: use client.command() — no result set expected
7684
if (isDDL) {
@@ -79,9 +87,9 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
7987
}
8088

8189
// Read queries: use client.query() with JSONEachRow format
82-
// Only append LIMIT for SELECT/WITH queries (not SHOW/DESCRIBE/EXPLAIN/EXISTS)
83-
if (supportsLimit && !hasDML && effectiveLimit > 0 && !/\bLIMIT\b/i.test(sql)) {
84-
query = `${sql.replace(/;\s*$/, "")} LIMIT ${effectiveLimit + 1}`
90+
// Only append LIMIT for SELECT/WITH queries that don't already have one.
91+
if (supportsLimit && effectiveLimit > 0 && !/\bLIMIT\b/i.test(sqlCleaned)) {
92+
query = `${sql.replace(/;\s*$/, "")}\nLIMIT ${effectiveLimit + 1}`
8593
}
8694

8795
const resultSet = await client.query({
@@ -108,6 +116,9 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
108116
},
109117

110118
async listSchemas(): Promise<string[]> {
119+
if (!client) {
120+
throw new Error("ClickHouse client not connected — call connect() first")
121+
}
111122
const resultSet = await client.query({
112123
query: "SHOW DATABASES",
113124
format: "JSONEachRow",
@@ -117,6 +128,9 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
117128
},
118129

119130
async listTables(schema: string): Promise<Array<{ name: string; type: string }>> {
131+
if (!client) {
132+
throw new Error("ClickHouse client not connected — call connect() first")
133+
}
120134
const resultSet = await client.query({
121135
query: `SELECT name, engine
122136
FROM system.tables
@@ -133,9 +147,11 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
133147
},
134148

135149
async describeTable(schema: string, table: string): Promise<SchemaColumn[]> {
150+
if (!client) {
151+
throw new Error("ClickHouse client not connected — call connect() first")
152+
}
136153
const resultSet = await client.query({
137-
query: `SELECT name, type,
138-
position(type, 'Nullable') > 0 AS is_nullable
154+
query: `SELECT name, type
139155
FROM system.columns
140156
WHERE database = {db:String}
141157
AND table = {tbl:String}
@@ -147,7 +163,9 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
147163
return rows.map((r) => ({
148164
name: r.name as string,
149165
data_type: r.type as string,
150-
nullable: r.is_nullable === 1 || r.is_nullable === true || r.is_nullable === "1",
166+
// Detect Nullable from the type string directly — stable across all versions.
167+
// LowCardinality(Nullable(T)) is also nullable (LowCardinality is a storage optimization).
168+
nullable: /^(?:LowCardinality\(\s*)?Nullable\b/i.test((r.type as string) ?? ""),
151169
}))
152170
},
153171

0 commit comments

Comments
 (0)