Skip to content

Commit 9c9a85e

Browse files
fix: audit #24 — coalesce arity, MSSQL OUTPUT star, MySQL/SQLite JSONPath (#89)
* fix: audit #24 — coalesce arity, MSSQL OUTPUT star, MySQL/SQLite JSONPath HIGH #1: coalesce() silently accepted zero arguments `COALESCE()` is invalid on every dialect (PG / MySQL / SQLite / MSSQL all reject). The variadic signature had no min-args check; the runtime driver error ("requires at least one argument") pointed at the DB instead of the caller. Added a builder-time throw. HIGH #2: MSSQL OUTPUT emitted invalid three-part name `returning(star("orders"))` compiled to `OUTPUT INSERTED.[orders].*` on MSSQL — a three-part reference to a non-existent object. The INSERTED/DELETED pseudo-tables have all the target columns already and cannot be qualified by the base table name. Fix: strip the table qualifier when a star hits the MSSQL OUTPUT path; emit `INSERTED.*` / `DELETED.*`. HIGH #4: MySQL / SQLite `->` / `->>` required JSONPath, not bare keys Both engines' JSON operators expect the RHS to be a JSONPath string starting with `$` (e.g. `data->'$.name'`, `data->'$[0]'`). The base printer emitted PG's bare-key form (`data->'name'`) which MySQL rejects with ER_INVALID_JSON_PATH and SQLite with "JSON path error". Overrode `printJsonAccess` on both MysqlPrinter and SqlitePrinter to rewrite single-segment paths to `$.name` / `$[N]`, with standard single-quote escaping of embedded quotes. Updated audit5 MSSQL test expectations to match the new pseudo-table behavior and added audit24 regression tests for each fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: audit #24 review — document MySQL/SQLite JSONPath semantic scope Review of PR #89 noted that MySQL/SQLite's JSONPath rewrite treats `at("a.b")` as a two-level path (`$.a.b`) while PG's printer treats the same string as a literal single key. Add a docblock note explaining the cross-dialect semantic scope: `.at(path)` takes a single JSON selector string, and chained `.at("a").at("b")` is the recommended portable form. Users who need path traversal on PG should use `#>` with a segment array. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent faf3787 commit 9c9a85e

6 files changed

Lines changed: 143 additions & 15 deletions

File tree

src/builder/eb.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -454,6 +454,13 @@ export function max<T>(expr: Expression<T>): Expression<T> {
454454

455455
/** COALESCE(a, b, c, ...) — returns first non-null value */
456456
export function coalesce<T>(...args: Expression<T | null>[]): Expression<T> {
457+
if (args.length === 0) {
458+
// `COALESCE()` is invalid on every dialect (PG, MySQL, SQLite,
459+
// MSSQL all reject zero-arg COALESCE). Catch at build time — the
460+
// runtime driver error ("COALESCE requires at least one argument")
461+
// doesn't point at the caller.
462+
throw new Error("coalesce() requires at least one argument")
463+
}
457464
return wrap(
458465
rawFn(
459466
"COALESCE",

src/printer/mssql.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -361,8 +361,11 @@ export class MssqlPrinter extends BasePrinter {
361361
/**
362362
* Render a RETURNING list as MSSQL `OUTPUT` columns under the given
363363
* pseudo-table (`INSERTED` or `DELETED`). Handles `StarNode` bare and
364-
* table-qualified — previously a `printed === "*"` string check missed
365-
* the `"t".*` form and emitted invalid `OUTPUT INSERTED."t".*`.
364+
* table-qualified — the pseudo-tables are fixed names, so a user's
365+
* `returning(star("orders"))` (meaning "all columns of orders") maps
366+
* to `INSERTED.*` (the pseudo-table has every column of the target).
367+
* Emitting `INSERTED.[orders].*` produces an invalid three-part name
368+
* that SQL Server rejects at parse.
366369
*/
367370
private _outputCols(
368371
returning: readonly import("../ast/nodes.ts").ExpressionNode[],
@@ -371,7 +374,9 @@ export class MssqlPrinter extends BasePrinter {
371374
return returning
372375
.map((r) => {
373376
if (r.type === "star") {
374-
return r.table ? `${prefix}.${quoteIdentifier(r.table, this.dialect)}.*` : `${prefix}.*`
377+
// Drop any user-supplied table qualifier: OUTPUT targets the
378+
// INSERTED/DELETED pseudo-table, never the base table directly.
379+
return `${prefix}.*`
375380
}
376381
return `${prefix}.${this.printExpression(r)}`
377382
})

src/printer/mysql.ts

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import type {
1313
} from "../ast/nodes.ts"
1414
import { UnsupportedDialectFeatureError } from "../errors.ts"
1515
import { quoteIdentifier } from "../utils/identifier.ts"
16+
import { escapeStringLiteral } from "../utils/security.ts"
1617
import { BasePrinter } from "./base.ts"
1718

1819
export class MysqlPrinter extends BasePrinter {
@@ -197,9 +198,21 @@ export class MysqlPrinter extends BasePrinter {
197198
}
198199

199200
/**
200-
* MySQL supports `->` / `->>` (single key) but has no path operators
201-
* `#>` / `#>>`; those are PG-specific. Reject the path variants with
202-
* a pointer at the JSON_EXTRACT equivalent.
201+
* MySQL `->` / `->>` require the RHS to be a JSONPath starting with
202+
* `$` (e.g. `data->'$.name'`, `data->'$[0]'`). The base printer emits
203+
* PG's bare-key form (`data->'name'`), which MySQL rejects with
204+
* `ER_INVALID_JSON_PATH`. Rewrite the path literal here.
205+
*
206+
* Note on cross-dialect semantics: if a caller passes `at("a.b")` the
207+
* MySQL form becomes `$.a.b` — a two-level JSONPath. On PG the same
208+
* node emits `data->'a.b'` — a literal single key. Sumak treats
209+
* `.at(path)` as "the user-supplied JSON selector string"; dotted
210+
* keys are rare and chaining (`.at("a").at("b")`) is the
211+
* recommended portable form. Use `#>` with a segment array for
212+
* path traversal (PG only).
213+
*
214+
* `#>` / `#>>` path operators are PG-specific — reject with a pointer
215+
* at JSON_EXTRACT.
203216
*/
204217
protected override printJsonAccess(node: import("../ast/nodes.ts").JsonAccessNode): string {
205218
if (node.operator === "#>" || node.operator === "#>>") {
@@ -208,7 +221,13 @@ export class MysqlPrinter extends BasePrinter {
208221
`${node.operator} JSON path operator — use JSON_EXTRACT(expr, '$.a.b') or chained '->' on MySQL`,
209222
)
210223
}
211-
return super.printJsonAccess(node)
224+
// Rewrite single-segment path to JSONPath. `at("0")` → `$[0]`,
225+
// `at("name")` → `$.name`. Escape embedded quotes with the usual
226+
// single-quote doubling.
227+
const seg = /^\d+$/.test(node.path) ? `[${node.path}]` : `.${node.path}`
228+
const pathLiteral = `'$${escapeStringLiteral(seg)}'`
229+
const result = `${this.printExpression(node.expr)}${node.operator}${pathLiteral}`
230+
return node.alias ? `${result} AS ${quoteIdentifier(node.alias, this.dialect)}` : result
212231
}
213232

214233
/** MySQL does not support `DELETE … RETURNING` — PG / SQLite 3.35+ only. */

src/printer/sqlite.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import type {
1010
} from "../ast/nodes.ts"
1111
import { UnsupportedDialectFeatureError } from "../errors.ts"
1212
import { quoteIdentifier } from "../utils/identifier.ts"
13+
import { escapeStringLiteral } from "../utils/security.ts"
1314
import { BasePrinter } from "./base.ts"
1415

1516
export class SqlitePrinter extends BasePrinter {
@@ -94,9 +95,10 @@ export class SqlitePrinter extends BasePrinter {
9495
}
9596

9697
/**
97-
* SQLite supports `->` / `->>` (3.38+) but has no path operators
98-
* `#>` / `#>>`; those are PG-specific. Reject with a pointer at
99-
* json_extract / chained `->`.
98+
* SQLite `->` / `->>` (3.38+) require the RHS to be a JSONPath
99+
* starting with `$` (`data->'$.name'`, `data->'$[0]'`). The base
100+
* printer emits PG's bare-key form, which SQLite rejects. Rewrite
101+
* the path literal here. `#>` / `#>>` are PG-specific — reject.
100102
*/
101103
protected override printJsonAccess(node: import("../ast/nodes.ts").JsonAccessNode): string {
102104
if (node.operator === "#>" || node.operator === "#>>") {
@@ -105,7 +107,10 @@ export class SqlitePrinter extends BasePrinter {
105107
`${node.operator} JSON path operator — use json_extract(expr, '$.a.b') or chained '->' on SQLite`,
106108
)
107109
}
108-
return super.printJsonAccess(node)
110+
const seg = /^\d+$/.test(node.path) ? `[${node.path}]` : `.${node.path}`
111+
const pathLiteral = `'$${escapeStringLiteral(seg)}'`
112+
const result = `${this.printExpression(node.expr)}${node.operator}${pathLiteral}`
113+
return node.alias ? `${result} AS ${quoteIdentifier(node.alias, this.dialect)}` : result
109114
}
110115

111116
/**

test/audit24-regressions.test.ts

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
import { describe, expect, it } from "vitest"
2+
3+
import type { InsertNode, JsonAccessNode, SelectNode } from "../src/ast/nodes.ts"
4+
import { createInsertNode, createSelectNode } from "../src/ast/nodes.ts"
5+
import { coalesce } from "../src/builder/eb.ts"
6+
import { MssqlPrinter } from "../src/printer/mssql.ts"
7+
import { MysqlPrinter } from "../src/printer/mysql.ts"
8+
import { SqlitePrinter } from "../src/printer/sqlite.ts"
9+
10+
const selectJsonAccess = (op: "->" | "->>", path: string): SelectNode => ({
11+
...createSelectNode(),
12+
columns: [
13+
{
14+
type: "json_access",
15+
expr: { type: "column_ref", column: "data" },
16+
operator: op,
17+
path,
18+
} as JsonAccessNode,
19+
],
20+
from: { type: "table_ref", name: "t" },
21+
})
22+
23+
describe("Audit #24 regressions", () => {
24+
describe("coalesce() requires at least one argument", () => {
25+
it("coalesce() with zero args throws at builder time", () => {
26+
expect(() => coalesce()).toThrow(/requires at least one argument/)
27+
})
28+
29+
it("coalesce(x) with one arg still works", () => {
30+
const r = coalesce({ node: { type: "literal", value: 1 } } as any)
31+
expect((r as any).node).toEqual({
32+
type: "function_call",
33+
name: "COALESCE",
34+
args: [{ type: "literal", value: 1 }],
35+
})
36+
})
37+
})
38+
39+
describe("MSSQL OUTPUT drops table qualifier on star (pseudo-tables only)", () => {
40+
it("INSERT ... RETURNING star('orders') → OUTPUT INSERTED.*", () => {
41+
const node: InsertNode = {
42+
...createInsertNode({ type: "table_ref", name: "orders" }),
43+
columns: ["name"],
44+
values: [[{ type: "literal", value: "x" }]],
45+
returning: [{ type: "star", table: "orders" }],
46+
}
47+
const r = new MssqlPrinter().print(node)
48+
expect(r.sql).toContain("OUTPUT INSERTED.*")
49+
expect(r.sql).not.toContain("INSERTED.[orders].*")
50+
})
51+
52+
it("bare RETURNING * → OUTPUT INSERTED.*", () => {
53+
const node: InsertNode = {
54+
...createInsertNode({ type: "table_ref", name: "orders" }),
55+
columns: ["name"],
56+
values: [[{ type: "literal", value: "x" }]],
57+
returning: [{ type: "star" }],
58+
}
59+
const r = new MssqlPrinter().print(node)
60+
expect(r.sql).toContain("OUTPUT INSERTED.*")
61+
})
62+
})
63+
64+
describe("MySQL / SQLite JSON `->` uses JSONPath ($.path / $[n])", () => {
65+
it("MySQL at('name') → `data`->'$.name'", () => {
66+
const r = new MysqlPrinter().print(selectJsonAccess("->", "name"))
67+
expect(r.sql).toContain("->'$.name'")
68+
expect(r.sql).not.toMatch(/->'name'/)
69+
})
70+
71+
it("MySQL at('0') → `data`->'$[0]' (array index)", () => {
72+
const r = new MysqlPrinter().print(selectJsonAccess("->", "0"))
73+
expect(r.sql).toContain("->'$[0]'")
74+
expect(r.sql).not.toContain("->'0'")
75+
})
76+
77+
it("SQLite ->> name → \"data\"->>'$.name'", () => {
78+
const r = new SqlitePrinter().print(selectJsonAccess("->>", "name"))
79+
expect(r.sql).toContain("->>'$.name'")
80+
})
81+
82+
it("embedded single quote in path key is escape-doubled", () => {
83+
// e.g. `at("a'b")` should still land as a well-formed literal.
84+
const r = new MysqlPrinter().print(selectJsonAccess("->", "a'b"))
85+
expect(r.sql).toContain("->'$.a''b'")
86+
})
87+
})
88+
})

test/audit5-regressions.test.ts

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,18 +36,22 @@ describe("Audit #5 regressions", () => {
3636
returning: [{ type: "star", table: "users" }],
3737
}
3838
const r = printer.print(node)
39-
// Previously emitted `INSERTED."users".*` (wrong quote style + bug)
40-
expect(r.sql).toContain("OUTPUT INSERTED.[users].*")
39+
// Audit #24: MSSQL pseudo-tables (INSERTED/DELETED) cannot be
40+
// qualified by a base table name — three-part `INSERTED.<tbl>.*`
41+
// is a SQL Server parse error. Drop the caller's qualifier.
42+
expect(r.sql).toContain("OUTPUT INSERTED.*")
43+
expect(r.sql).not.toContain("INSERTED.[users].*")
4144
})
4245

43-
it("DELETE … OUTPUT DELETED.[t].* for table-qualified star", () => {
46+
it("DELETE … OUTPUT DELETED.* for table-qualified star", () => {
4447
const node: DeleteNode = {
4548
...createDeleteNode({ type: "table_ref", name: "users" }),
4649
returning: [{ type: "star", table: "users" }],
4750
where: eq(col("id"), param(0, 1)),
4851
}
4952
const r = printer.print(node)
50-
expect(r.sql).toContain("OUTPUT DELETED.[users].*")
53+
expect(r.sql).toContain("OUTPUT DELETED.*")
54+
expect(r.sql).not.toContain("DELETED.[users].*")
5155
})
5256

5357
it("INSERT OUTPUT with non-star column nodes still prefixes correctly", () => {

0 commit comments

Comments
 (0)