fix(clickhouse): refuse values with no SQL literal instead of writing String(value)

.changeset/untyped-literal-refuses-non-scalars.md

@@ -0,0 +1,5 @@
+---
+"@maple-dev/effect-orm": patch
+---
+
+Refuse values with no SQL literal instead of writing `String(value)`. A value compared against, or passed to, an expression with no column type (a `defineFn` result, `untypedExpr`, an untyped `dynamicColumn`) used to be written raw when it was not a string, number, boolean or date: an array or object from a request became part of the SQL (`toFloat64OrNull(x) > 1) OR (1=1`). It now fails compilation with a `QueryBuilderError` (`InvalidLiteral`). `null` is written as `NULL` and a bigint as its digits, as before.

docs/expressions.md

@@ -45,11 +45,16 @@ $.Count.eq("lots")
 // QueryBuilderError { code: "InvalidLiteral" }: column Count: string "lots" is not a valid value
 ```
 
-Expressions with no type to read — `untypedExpr`, an untyped `dynamicColumn` — fall back to
-guessing from the JavaScript value, which handles strings, numbers, booleans and dates but
-nothing structured.
-
-_(Backed by `src/ch/literal.test.ts`.)_
+Expressions with no type to read — `untypedExpr`, an untyped `dynamicColumn`, a function
+declared with `defineFn` — fall back to guessing from the JavaScript value, which handles
+strings, numbers, bigints, booleans, dates and `null`; a string is written as an escaped
+literal. Anything structured (an array, an object), a symbol or a function fails compilation
+with a `QueryBuilderError` (`InvalidLiteral`) rather than being written as `String(value)`,
+so an unsupported value that arrives untyped is never written as raw SQL. `undefined` fails
+too: as a function argument with `InvalidLiteral`, and compared against an expression
+(`.gt(undefined)`) with `InvalidArguments`, like any comparison with a missing value.
+
+_(Backed by `src/ch/literal.test.ts` and `src/ch/failure.test.ts`.)_
 
 ## Comparisons
 

src/ch/expr.ts

@@ -307,9 +307,24 @@ export function toFragment(value: unknown): SqlFragment {
 	// `String(value)` produces.
 	if (DateTime.isDateTime(value)) return known(() => dateTimeLiteral(DateTime.toUtc(value)))
 	if (value instanceof Date) return known(() => dateTimeLiteral(DateTime.makeUnsafe(value)))
-	return raw(String(value))
+	if (value === null) return raw("NULL")
+	if (typeof value === "bigint") return raw(String(value))
+	// Anything else has no literal here. `String(value)` used to be written as
+	// SQL, so an array or object from a request became part of the query.
+	return known(() =>
+		fail(
+			new QueryBuilderError({
+				code: "InvalidLiteral",
+				message: `cannot write ${describeUntyped(value)} as a SQL literal; compare against a typed column or pass a string, number, boolean or date`,
+			}),
+			"NULL",
+		),
+	)
 }
 
+const describeUntyped = (value: unknown): string =>
+	Array.isArray(value) ? "an array" : value === undefined ? "undefined" : `a ${typeof value}`
+
 /** A value with no column type to encode it, in the active dialect's syntax.
  *  ClickHouse writes booleans as `1`/`0`, which is also the rendering outside
  *  a compile. */

src/ch/failure.test.ts

@@ -31,6 +31,50 @@ describe("the builder never throws", () => {
 		expect(error.message).toMatch(/limit\(-1\)/)
 	})
 
+	// A value with no column type to encode it was written as `String(value)`:
+	// an array or object from a request became SQL. It is a typed failure now.
+	it.each([
+		["an array", ["1) OR (1=1"]],
+		["an object", { toString: (): string => "1) OR (1=1" }],
+		["a symbol", Symbol("x")],
+		["a function", () => "1"],
+	])("refuses %s compared against an untyped expression", (_label, value) => {
+		const toFloat = CH.defineFn<[CH.Expr<string>], number>("toFloat64OrNull", CH.float64)
+		const query = CH.from(Events)
+			.select(($) => ({ id: $.Id }))
+			.where(($) => [toFloat($.Name).gt(value as never)])
+		const error = Effect.runSync(Effect.flip(CH.compile(query)))
+		expect(error).toBeInstanceOf(QueryBuilderError)
+		expect(error.code).toBe("InvalidLiteral")
+	})
+
+	it.each([
+		["an object", { toString: (): string => "1) OR (1=1" }],
+		["an array", ["1) OR (1=1"]],
+		["undefined", undefined],
+	])("refuses %s as a function argument", (_label, value) => {
+		const twice = CH.defineFn<[unknown], number>("twice", CH.float64)
+		const query = CH.from(Events).select(() => ({ n: twice(value) }))
+		const error = Effect.runSync(Effect.flip(CH.compile(query)))
+		expect(error.code).toBe("InvalidLiteral")
+	})
+
+	it("still writes null, a bigint and the scalars it always wrote", () => {
+		const toFloat = CH.defineFn<[CH.Expr<string>], number>("toFloat64OrNull", CH.float64)
+		const fn = CH.defineFn<[unknown], number>("f", CH.float64)
+		const sql = CH.compileUnsafe(
+			CH.from(Events)
+				.select(() => ({ a: fn(null), b: fn(12345678901234567890n), c: fn("x'y"), d: fn(1.5), e: fn(true) }))
+				.where(($) => [toFloat($.Name).gt(3)]),
+		).sql
+		expect(sql).toContain("f(NULL) AS a")
+		expect(sql).toContain("f(12345678901234567890) AS b")
+		expect(sql).toContain("f('x\\'y') AS c")
+		expect(sql).toContain("f(1.5) AS d")
+		expect(sql).toContain("f(1) AS e")
+		expect(sql).toContain("toFloat64OrNull(events.Name) > 3")
+	})
+
 	it("dies on a misuse no input could fix", () => {
 		const query = CH.from(Events).select(($) => ({ id: $.Id })).where(($) => [$.Id.eq(CH.param.string("bad__name"))])
 		const exit = Effect.runSyncExit(CH.compile(query, { bad__name: "x" }))