Skip to content

fix: replace escapeSqlString with parameterized query binds in finops/schema#277

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
suryaiyer95 merged 2 commits into from
Mar 19, 2026
Merged

fix: replace escapeSqlString with parameterized query binds in finops/schema #277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dcdd1ec
fix: replace escapeSqlString with parameterized query binds in finops…
mdesmet Mar 18, 2026
95942d4
fix: migrate PR #283 — Snowflake column lowercasing, warehouse-adviso…
mdesmet Mar 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 8 additions & 9 deletions packages/drivers/src/bigquery.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -37,19 +37,18 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
client = new BigQuery(options)
},

async execute(sql: string, limit?: number): Promise<ConnectorResult> {
async execute(sql: string, limit?: number, binds?: any[]): Promise<ConnectorResult> {
const effectiveLimit = limit ?? 1000
let query = sql
const query = sql.replace(/;\s*$/, "")
const isSelectLike = /^\s*(SELECT|WITH|VALUES)\b/i.test(sql)
if (
isSelectLike &&
effectiveLimit &&
!/\bLIMIT\b/i.test(sql)
) {
query = `${sql.replace(/;\s*$/, "")} LIMIT ${effectiveLimit + 1}`
}

// BigQuery does not allow appending LIMIT to parameterized queries.
// Use maxResults instead — it limits rows returned at the API level.
const options: Record<string, unknown> = { query }
if (isSelectLike && effectiveLimit && !/\bLIMIT\b/i.test(sql)) {
options.maxResults = effectiveLimit + 1
}
if (binds?.length) options.params = binds
Comment on lines 47 to +51

This comment was marked as outdated.

Sign in to view

if (config.dataset) {
Comment on lines 47 to 52

@sentrysentryBotMar 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: BigQuery queries will fail because LIMIT ? is not supported. The logic for detecting truncated results is also bypassed because the LIMIT keyword is now present in the SQL string.
Severity: CRITICAL

Suggested Fix

Remove the LIMIT ? placeholder from the BigQuery SQL templates. Instead, rely on the driver's existing mechanism which appends LIMIT and sets maxResults correctly. This will ensure the queries are valid and that truncation detection works as intended.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/drivers/src/bigquery.ts#L37-L42
Potential issue: The BigQuery driver's `execute` method checks if a `LIMIT` clause is
present in the SQL to decide whether to set `options.maxResults` for truncation
detection. However, recent changes introduced SQL templates with a parameterized `LIMIT
?` clause. According to BigQuery documentation, using a parameter for the `LIMIT` clause
is not supported, which will cause all BigQuery finops queries (credit analysis,
expensive queries, query history) to fail at runtime. Additionally, because the SQL now
contains a `LIMIT` keyword, the driver's logic to detect truncated results by fetching
one extra row (`maxResults = effectiveLimit + 1`) is bypassed. This means that even if
the query were valid, the `truncated` flag would always be `false`.

options.defaultDataset = {
datasetId: config.dataset,
Expand Down
16 changes: 12 additions & 4 deletions packages/drivers/src/databricks.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -18,6 +18,10 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
let client: any
let session: any

function escapeIdentifier(value: string): string {
return value.replace(/`/g, "``")
}

return {
async connect() {
const DBSQLClient = databricksModule.DBSQLClient ?? databricksModule
Expand All@@ -40,7 +44,7 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
})
},

async execute(sql: string, limit?: number): Promise<ConnectorResult> {
async execute(sql: string, limit?: number, binds?: any[]): Promise<ConnectorResult> {
const effectiveLimit = limit ?? 1000
let query = sql
const isSelectLike = /^\s*(SELECT|WITH|VALUES)\b/i.test(sql)
Expand All@@ -52,7 +56,11 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
query = `${sql.replace(/;\s*$/, "")} LIMIT ${effectiveLimit + 1}`
}

const operation = await session.executeStatement(query)
const stmtOptions: Record<string, any> = {}
if (binds?.length) {
stmtOptions.ordinalParameters = binds
}
const operation = await session.executeStatement(query, stmtOptions)
Comment on lines +59 to +63

This comment was marked as outdated.

Sign in to view

const rows = await operation.fetchAll()
await operation.close()

Expand DownExpand Up@@ -84,7 +92,7 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
schema: string,
): Promise<Array<{ name: string; type: string }>> {
const operation = await session.executeStatement(
`SHOW TABLES IN \`${schema}\``,
`SHOW TABLES IN \`${escapeIdentifier(schema)}\``,
)
const rows = await operation.fetchAll()
await operation.close()
Expand All@@ -102,7 +110,7 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
table: string,
): Promise<SchemaColumn[]> {
const operation = await session.executeStatement(
`DESCRIBE TABLE \`${schema}\`.\`${table}\``,
`DESCRIBE TABLE \`${escapeIdentifier(schema)}\`.\`${escapeIdentifier(table)}\``,
)
const rows = await operation.fetchAll()
await operation.close()
Expand Down
33 changes: 19 additions & 14 deletions packages/drivers/src/duckdb.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2,7 +2,6 @@
* DuckDB driver using the `duckdb` package.
*/

import { escapeSqlString } from "./sql-escape"
import type { ConnectionConfig, Connector, ConnectorResult, SchemaColumn } from "./types"

export async function connect(config: ConnectionConfig): Promise<Connector> {
Expand All@@ -27,6 +26,15 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
})
}

function queryWithParams(sql: string, params: any[]): Promise<any[]> {
return new Promise((resolve, reject) => {
connection.all(sql, ...params, (err: Error | null, rows: any[]) => {
if (err) reject(err)
else resolve(rows ?? [])
})
})
}

return {
async connect() {
db = await new Promise<any>((resolve, reject) => {
Expand All@@ -51,7 +59,7 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
connection = db.connect()
},

async execute(sql: string, limit?: number): Promise<ConnectorResult> {
async execute(sql: string, limit?: number, binds?: any[]): Promise<ConnectorResult> {
const effectiveLimit = limit ?? 1000

let finalSql = sql
Expand All@@ -64,7 +72,9 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
finalSql = `${sql.replace(/;\s*$/, "")} LIMIT ${effectiveLimit + 1}`
}

const rows = await query(finalSql)
const rows = binds?.length
? await queryWithParams(finalSql, binds)
: await query(finalSql)
const columns =
rows.length > 0 ? Object.keys(rows[0]) : []
const truncated = rows.length > effectiveLimit
Expand All@@ -90,11 +100,9 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
async listTables(
schema: string,
): Promise<Array<{ name: string; type: string }>> {
const rows = await query(
`SELECT table_name, table_type
FROM information_schema.tables
WHERE table_schema = '${escapeSqlString(schema)}'
ORDER BY table_name`,
const rows = await queryWithParams(
`SELECT table_name, table_type FROM information_schema.tables WHERE table_schema = $1 ORDER BY table_name`,
[schema],
)
return rows.map((r) => ({
name: r.table_name as string,
Expand All@@ -106,12 +114,9 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
schema: string,
table: string,
): Promise<SchemaColumn[]> {
const rows = await query(
`SELECT column_name, data_type, is_nullable
FROM information_schema.columns
WHERE table_schema = '${escapeSqlString(schema)}'
AND table_name = '${escapeSqlString(table)}'
ORDER BY ordinal_position`,
const rows = await queryWithParams(
`SELECT column_name, data_type, is_nullable FROM information_schema.columns WHERE table_schema = $1 AND table_name = $2 ORDER BY ordinal_position`,
[schema, table],
)
return rows.map((r) => ({
name: r.column_name as string,
Expand Down
2 changes: 0 additions & 2 deletions packages/drivers/src/index.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
// Re-export types
export type { Connector, ConnectorResult, SchemaColumn, ConnectionConfig } from "./types"

// Re-export escape utilities
export { escapeSqlString, escapeSqlIdentifier } from "./sql-escape"

// Re-export config normalization
export { normalizeConfig } from "./normalize"
Expand Down
2 changes: 1 addition & 1 deletion packages/drivers/src/mysql.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -41,7 +41,7 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
pool = mysql.createPool(poolConfig)
},

async execute(sql: string, limit?: number): Promise<ConnectorResult> {
async execute(sql: string, limit?: number, _binds?: any[]): Promise<ConnectorResult> {
const effectiveLimit = limit ?? 1000
let query = sql
const isSelectLike = /^\s*(SELECT|WITH|VALUES)\b/i.test(sql)
Expand Down
2 changes: 1 addition & 1 deletion packages/drivers/src/oracle.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -37,7 +37,7 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
})
},

async execute(sql: string, limit?: number): Promise<ConnectorResult> {
async execute(sql: string, limit?: number, _binds?: any[]): Promise<ConnectorResult> {
const effectiveLimit = limit ?? 1000
let query = sql
const isSelectLike = /^\s*(SELECT|WITH)\b/i.test(sql)
Expand Down
2 changes: 1 addition & 1 deletion packages/drivers/src/postgres.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -39,7 +39,7 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
pool = new Pool(poolConfig)
},

async execute(sql: string, limit?: number): Promise<ConnectorResult> {
async execute(sql: string, limit?: number, _binds?: any[]): Promise<ConnectorResult> {
const client = await pool.connect()
try {
if (config.statement_timeout) {
Expand Down
2 changes: 1 addition & 1 deletion packages/drivers/src/redshift.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -40,7 +40,7 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
pool = new Pool(poolConfig)
},

async execute(sql: string, limit?: number): Promise<ConnectorResult> {
async execute(sql: string, limit?: number, _binds?: any[]): Promise<ConnectorResult> {
const client = await pool.connect()
try {
const effectiveLimit = limit ?? 1000
Expand Down
25 changes: 16 additions & 9 deletions packages/drivers/src/snowflake.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -28,22 +28,29 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {

let connection: any

function executeQuery(sql: string): Promise<{ columns: string[]; rows: any[][] }> {
function escapeSqlIdentifier(value: string): string {
return value.replace(/"/g, '""')
}

function executeQuery(sql: string, binds?: any[]): Promise<{ columns: string[]; rows: any[][] }> {
return new Promise((resolve, reject) => {
connection.execute({
const options: Record<string, any> = {
sqlText: sql,
complete(err: Error | null, _stmt: any, rows: any[]) {
if (err) return reject(err)
if (!rows || rows.length === 0) {
return resolve({ columns: [], rows: [] })
}
const columns = Object.keys(rows[0])
const rawColumns = Object.keys(rows[0])
const columns = rawColumns.map((col) => col.toLowerCase())
const mapped = rows.map((row) =>
columns.map((col) => row[col]),
rawColumns.map((col) => row[col]),
)
resolve({ columns, rows: mapped })
},
})
}
if (binds && binds.length > 0) options.binds = binds
connection.execute(options)
})
}

Expand DownExpand Up@@ -222,7 +229,7 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
})
},

async execute(sql: string, limit?: number): Promise<ConnectorResult> {
async execute(sql: string, limit?: number, binds?: any[]): Promise<ConnectorResult> {
const effectiveLimit = limit ?? 1000
let query = sql
const isSelectLike = /^\s*(SELECT|WITH|VALUES|SHOW)\b/i.test(sql)
Expand All@@ -234,7 +241,7 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
query = `${sql.replace(/;\s*$/, "")} LIMIT ${effectiveLimit + 1}`
}

const result = await executeQuery(query)
const result = await executeQuery(query, binds)
const truncated = result.rows.length > effectiveLimit
const rows = truncated
? result.rows.slice(0, effectiveLimit)
Expand All@@ -260,7 +267,7 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
schema: string,
): Promise<Array<{ name: string; type: string }>> {
const result = await executeQuery(
`SHOW TABLES IN SCHEMA "${schema.replace(/"/g, '""')}"`,
`SHOW TABLES IN SCHEMA "${escapeSqlIdentifier(schema)}"`,
)
const nameIdx = result.columns.indexOf("name")
const kindIdx = result.columns.indexOf("kind")
Expand All@@ -277,7 +284,7 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
table: string,
): Promise<SchemaColumn[]> {
const result = await executeQuery(
`SHOW COLUMNS IN TABLE "${schema.replace(/"/g, '""')}"."${table.replace(/"/g, '""')}"`,
`SHOW COLUMNS IN TABLE "${escapeSqlIdentifier(schema)}"."${escapeSqlIdentifier(table)}"`,
)
const nameIdx = result.columns.indexOf("column_name")
const typeIdx = result.columns.indexOf("data_type")
Expand Down
21 changes: 0 additions & 21 deletions packages/drivers/src/sql-escape.ts
View file
Open in desktop

This file was deleted.

5 changes: 2 additions & 3 deletions packages/drivers/src/sqlite.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -3,7 +3,6 @@
* Synchronous API wrapped in async interface.
*/

import { escapeSqlIdentifier } from "./sql-escape"
import type { ConnectionConfig, Connector, ConnectorResult, SchemaColumn } from "./types"

export async function connect(config: ConnectionConfig): Promise<Connector> {
Expand All@@ -28,7 +27,7 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
db.pragma("journal_mode = WAL")
},

async execute(sql: string, limit?: number): Promise<ConnectorResult> {
async execute(sql: string, limit?: number, _binds?: any[]): Promise<ConnectorResult> {
const effectiveLimit = limit ?? 1000

// Determine if this is a SELECT-like statement
Expand DownExpand Up@@ -98,7 +97,7 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
_schema: string,
table: string,
): Promise<SchemaColumn[]> {
const rows = db.prepare(`PRAGMA table_info("${escapeSqlIdentifier(table)}")`).all()
const rows = db.prepare('SELECT * FROM pragma_table_info(?) ORDER BY cid').all(table)
return rows.map((r: any) => ({
name: r.name as string,
data_type: r.type as string,
Expand Down
2 changes: 1 addition & 1 deletion packages/drivers/src/sqlserver.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -42,7 +42,7 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
pool = await mssql.connect(mssqlConfig)
},

async execute(sql: string, limit?: number): Promise<ConnectorResult> {
async execute(sql: string, limit?: number, _binds?: any[]): Promise<ConnectorResult> {
const effectiveLimit = limit ?? 1000

let query = sql
Expand Down
2 changes: 1 addition & 1 deletion packages/drivers/src/types.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -22,7 +22,7 @@ export interface SchemaColumn {

export interface Connector {
connect(): Promise<void>
execute(sql: string, limit?: number): Promise<ConnectorResult>
execute(sql: string, limit?: number, binds?: any[]): Promise<ConnectorResult>
listSchemas(): Promise<string[]>
listTables(schema: string): Promise<Array<{ name: string; type: string }>>
describeTable(schema: string, table: string): Promise<SchemaColumn[]>
Expand Down
Loading
Loading