diff --git a/.changeset/typed-errors-not-throws.md b/.changeset/typed-errors-not-throws.md new file mode 100644 index 0000000..d247486 --- /dev/null +++ b/.changeset/typed-errors-not-throws.md @@ -0,0 +1,5 @@ +--- +"@maple-dev/effect-orm": patch +--- + +Values that cannot become SQL fail compilation with a `QueryBuilderError` (`InvalidLiteral`) instead of throwing: a bigint compared against a non-integer column, a non-string `like`/`notLike`/`ilike` pattern, `lit()` of anything but a string or number (it wrote `[object Object]`), a non-numeric arithmetic operand, an invalid `Date`, and an unparseable string or invalid `Date` for a Postgres timestamp. A non-expression in a `select` record is a `QueryBuilderDefect` naming the alias, like a malformed `orderBy`. diff --git a/src/ch/compile.ts b/src/ch/compile.ts index add77b4..f345c95 100644 --- a/src/ch/compile.ts +++ b/src/ch/compile.ts @@ -914,6 +914,12 @@ function compileInner< // GROUP BY rules below know what each part reads and aggregates. const selected = selectKeys.map((alias) => { const expr = selectExprs[alias] + if (!isExprLike(expr)) { + const [sql, found] = track(() => + fail(new QueryBuilderDefect({ message: `CHQuery: select(($) => ...) must map each alias to an expression; ${JSON.stringify(alias)} is not one` }), "NULL"), + ) + return { alias, sql, found, column: undefined } + } const fragment = expr.toFragment() const [sql, found] = track(() => compileSqlFragment(fragment)) return { alias, sql, found, column: plainColumn(fragment) } diff --git a/src/ch/expr.ts b/src/ch/expr.ts index 59a1fb5..9de1b38 100644 --- a/src/ch/expr.ts +++ b/src/ch/expr.ts @@ -306,7 +306,13 @@ export function toFragment(value: unknown): SqlFragment { // be the dialect's own form (ClickHouse's is tz-less) rather than whatever // `String(value)` produces. if (DateTime.isDateTime(value)) return known(() => dateTimeLiteral(DateTime.toUtc(value))) - if (value instanceof Date) return known(() => dateTimeLiteral(DateTime.makeUnsafe(value))) + if (value instanceof Date) { + return known(() => + Number.isNaN(value.getTime()) + ? fail(new QueryBuilderError({ code: "InvalidLiteral", message: "cannot write an invalid Date as a SQL literal" }), "NULL") + : dateTimeLiteral(DateTime.makeUnsafe(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 @@ -342,9 +348,19 @@ const boundString = (value: string): SqlFragment => return bind !== undefined ? bind(value) : compile(str(value)) }) -/** A LIKE pattern: an expression as it renders, a string as `boundString`. */ -const likePattern = (pattern: string | Expr): string => - compile(typeof pattern === "string" ? boundString(pattern) : pattern.toFragment()) +/** A LIKE pattern: an expression as it renders, a string as `boundString`. A + * pattern often comes from a request, so anything else is a failure. */ +const likePattern = (pattern: string | Expr): string => { + if (typeof pattern === "string") return compile(boundString(pattern)) + if (isExprLike(pattern)) return compile(pattern.toFragment()) + return fail( + new QueryBuilderError({ + code: "InvalidLiteral", + message: `a LIKE pattern must be a string or a string expression, not ${describeUntyped(pattern)}`, + }), + "NULL", + ) +} const dateTimeLiteral = (value: DateTime.Utc): string => activeSqlSyntax()?.dateTimeLiteral(value) ?? compile(str(chDateTimeLiteral(value))) @@ -407,7 +423,8 @@ const arith = ( rhs: number | null | Expr, lhsSchema?: Schema.Codec, ): Expr => { - const rhsSchema = typeof rhs === "number" || rhs === null ? undefined : rhs.schema + const operand = typeof rhs === "number" || rhs === null || isExprLike(rhs) + const rhsSchema = isExprLike(rhs) ? rhs.schema : undefined // `x / 1000000` is finite whenever `x` is. A literal below 1 in magnitude // can overflow a large dividend (`1 / 5e-324` is `inf`), so only |d| >= 1 // keeps the strict codec — the same rule `Quotient` applies to the type. @@ -422,6 +439,13 @@ const arith = ( const grouped = typeof rhs === "object" && rhs !== null && infix.has(rhs) const expr = makeExpr( known(() => { + // The operand is often a number from a request; anything else is a failure, not `'x'` or `NULL` in SQL. + if (!operand) { + return fail( + new QueryBuilderError({ code: "InvalidLiteral", message: `arithmetic operand must be a number or a numeric expression, not ${describeUntyped(rhs)}` }), + "NULL", + ) + } const right = compile(toFragment(rhs)) return `${compile(lhs)} ${op} ${grouped ? `(${right})` : right}` }), @@ -657,7 +681,18 @@ export function lit(value: string | number): Expr | Expr { // a single `CH.lit("all")` in a SELECT costs the whole query its row schema, // since derivation is all-or-nothing. if (typeof value === "string") return makeExpr(str(value), chString.schema) - return makeExpr(typeof value === "number" ? numberFragment(value) : raw(String(value)), CHNumber as Schema.Codec) + if (typeof value === "number") return makeExpr(numberFragment(value), CHNumber as Schema.Codec) + if (typeof value === "bigint") return makeExpr(raw(String(value)), CHNumber as Schema.Codec) + // `String(value)` used to be written as SQL, so `lit(obj)` emitted `[object Object]`. + return makeExpr( + known(() => + fail( + new QueryBuilderError({ code: "InvalidLiteral", message: `lit: cannot write ${describeUntyped(value)} as a SQL literal; pass a string or a number` }), + "NULL", + ), + ), + CHNumber as Schema.Codec, + ) } // Subquery expressions diff --git a/src/ch/failure.test.ts b/src/ch/failure.test.ts index 0883baf..15aaffe 100644 --- a/src/ch/failure.test.ts +++ b/src/ch/failure.test.ts @@ -13,6 +13,41 @@ const Events = CH.table("events", { const PgEvents = PG.table("events", { columns: { id: PG.text, score: PG.nullable(PG.float8) }, primaryKey: ["id"] }) +describe("values a request could supply fail as QueryBuilderError, never a throw", () => { + const PgTimed = PG.table("timed", { columns: { id: PG.text, at: PG.timestamptz }, primaryKey: ["id"] }) + const failure = (effect: Effect.Effect) => { + const exit = Effect.runSyncExit(effect) + expect(Exit.isFailure(exit) && Cause.squash(exit.cause)).toBeInstanceOf(QueryBuilderError) + return Exit.isFailure(exit) ? String(Cause.squash(exit.cause)) : "" + } + + it("a bigint compared against a String column", () => { + expect(failure(CH.compile(CH.from(Events).select(($) => ({ id: $.Id })).where(($) => [$.Id.eq(0n as never)])))).toMatch(/0n is not a valid value/) + }) + + it.each([[1], [{}], [new Map()]])("a LIKE pattern of %o", (pattern) => { + expect(failure(CH.compile(CH.from(Events).select(($) => ({ id: $.Id })).where(($) => [$.Name.like(pattern as never)])))).toMatch(/LIKE pattern/) + }) + + it.each([[{}], [Symbol("x")], [true]])("lit(%o)", (value) => { + expect(failure(CH.compile(CH.from(Events).select(() => ({ v: CH.lit(value as never) }))))).toMatch(/lit: cannot write/) + }) + + it.each([[undefined], ["5"], [{}], [new Date(Number.NaN)]])("an arithmetic operand of %o", (operand) => { + expect(failure(CH.compile(CH.from(Events).select(() => ({ v: CH.lit(1).add(operand as never) }))))).toMatch(/operand|invalid Date/) + }) + + it.each([[""], ["2026-13-45 99:99:99"], [new Date(Number.NaN)]])("a Postgres timestamp of %o", (value) => { + const query = PG.from(PgTimed).select(($) => ({ id: $.id })).where(($) => [$.at.gte(CH.param.dateTime("from"))]) + expect(failure(PG.compile(query, { from: value as never }))).toMatch(/not a valid value/) + }) + + it("a non-expression in a select record is a defect: it is written in source", () => { + const exit = Effect.runSyncExit(CH.compile(CH.from(Events).select(() => ({ v: 1 as never })))) + expect(Exit.isFailure(exit) && Cause.squash(exit.cause)).toBeInstanceOf(QueryBuilderDefect) + }) +}) + describe("the builder never throws", () => { it("builds a bad query without throwing; compiling it reports the failure", () => { const build = () => diff --git a/src/ch/literal.ts b/src/ch/literal.ts index 053ff25..d03a415 100644 --- a/src/ch/literal.ts +++ b/src/ch/literal.ts @@ -30,6 +30,7 @@ const describe = (value: unknown): string => { if (typeof value === "function") return "a function" if (typeof value === "symbol") return "a symbol" if (typeof value === "number" || typeof value === "boolean") return `${value}` + if (typeof value === "bigint") return `${value}n` if (value instanceof Date) return "a Date" if (Array.isArray(value)) return "an array" if (typeof value === "object") return "an object" diff --git a/src/pg/types.ts b/src/pg/types.ts index 2c066d7..adb6c5e 100644 --- a/src/pg/types.ts +++ b/src/pg/types.ts @@ -69,6 +69,13 @@ const PgTimestamptz: Schema.Codec = Schema.Union([ * receives epoch milliseconds. Unlike a zoneless literal, an instant cannot be * read in the session's time zone. */ +const isValidDate = Schema.makeFilter((value: Date) => (Number.isNaN(value.getTime()) ? "an invalid Date" : undefined), { + title: "validDate", +}) + +// The checks sit on the value side, which is what encoding reads: a string or +// Date that is not an instant fails there as a schema issue, instead of reaching +// `format` as NaN and throwing from `toISOString`. export const timestampLiteral = (format: (epochMillis: number) => string): Schema.Codec => Schema.Union([ Schema.String.pipe( @@ -78,14 +85,14 @@ export const timestampLiteral = (format: (epochMillis: number) => string): Schem }), ), Schema.String.pipe( - Schema.decodeTo(Schema.Date, { + Schema.decodeTo(Schema.Date.pipe(Schema.check(isValidDate)), { decode: SchemaGetter.transform((value: string) => new Date(value)), encode: SchemaGetter.transform((value: Date) => format(value.getTime())), }), ), Schema.String.pipe( Schema.check(isTimestamp), - Schema.decodeTo(Schema.String, { + Schema.decodeTo(Schema.String.pipe(Schema.check(isTimestamp)), { decode: SchemaGetter.transform((value: string) => value), encode: SchemaGetter.transform((value: string) => format(Date.parse(pgTimestampToIso(value)))), }), diff --git a/tests/fuzz/compile.fuzz.test.ts b/tests/fuzz/compile.fuzz.test.ts index c4edeed..036c6a8 100644 --- a/tests/fuzz/compile.fuzz.test.ts +++ b/tests/fuzz/compile.fuzz.test.ts @@ -77,7 +77,7 @@ const hostile: fc.Arbitrary = fc.oneof( fc.constantFrom(Number.NaN, Infinity, -Infinity, -0, 2 ** 53, 1e308, 5e-324, new Date(Number.NaN), Symbol("s"), () => 1, undefined, null, "", "2026-13-45 99:99:99", { _brand: "Expr" }, { _brand: "Expr", toFragment: 1 }), ) -type HostileSite = "eq" | "neq" | "gt" | "in_" | "notIn" | "between" | "like" | "param.int" | "param.float" | "param.string" | "param.bool" | "param.dateTime" | "param.of" | "select" +type HostileSite = "lit" | "add" | "eq" | "neq" | "gt" | "in_" | "notIn" | "between" | "like" | "param.int" | "param.float" | "param.string" | "param.bool" | "param.dateTime" | "param.of" | "select" export interface HostileInput { readonly dialect: "clickhouse" | "postgres" readonly column: OrderColumn @@ -90,6 +90,8 @@ const hostileQuery = (ctx: CoreContext, input: HostileInput) => { const params: Record = { orgId: "org_1", h: value } let q: any = ctx.from(ctx.orders) if (site === "select") return { query: q.select(() => ({ id: CH.lit(1), bad: value })), params } + if (site === "lit") return { query: q.select(() => ({ id: CH.lit(value as never) })), params } + if (site === "add") return { query: q.select(($: any) => ({ id: $.Amount.add(value) })), params } q = q.select(($: any) => ({ id: $.Id })) if (site.startsWith("param.")) { const kind = site.slice("param.".length) @@ -106,7 +108,7 @@ const hostileInput = (dialect: "clickhouse" | "postgres"): fc.Arbitrary("eq", "neq", "gt", "in_", "notIn", "between", "like", "param.int", "param.float", "param.string", "param.bool", "param.dateTime", "param.of", "select"), + site: fc.constantFrom("lit", "add", "eq", "neq", "gt", "in_", "notIn", "between", "like", "param.int", "param.float", "param.string", "param.bool", "param.dateTime", "param.of", "select"), value: hostile, }) @@ -123,7 +125,13 @@ const neverThrows = (ctx: CoreContext, input: HostileInput) => { }) } if (Exit.isFailure(exit)) { - const typed = exit.cause.reasons.every((reason) => reason._tag === "Fail" && String((reason as any).error?._tag).endsWith("QueryBuilderError")) + // src/ch/errors.ts: a value a request could supply fails with a QueryBuilderError; what is only ever + // written in source (the shape of a select record) may die with a QueryBuilderDefect. Nothing else. + const sourceSite = input.site === "select" + const typed = exit.cause.reasons.every((reason) => { + const tag = String(((reason as any).error ?? (reason as any).defect)?._tag) + return (reason._tag === "Fail" && tag.endsWith("QueryBuilderError")) || (sourceSite && reason._tag === "Die" && tag.endsWith("QueryBuilderDefect")) + }) if (!typed) throw new FuzzFailure("compile died or failed with something other than a QueryBuilderError", { cause: String(exit.cause) }) } expect(true).toBe(true) diff --git a/tests/fuzz/known-issues.ts b/tests/fuzz/known-issues.ts index d969158..c93502c 100644 --- a/tests/fuzz/known-issues.ts +++ b/tests/fuzz/known-issues.ts @@ -58,51 +58,11 @@ export const knownLiteralIssue = (input: LiteralInput): string | undefined => import type { HostileInput } from "./compile.fuzz.test" -const containsBigInt = (value: unknown, seen = new Set()): boolean => { - if (typeof value === "bigint") return true - if (value === null || typeof value !== "object" || seen.has(value)) return false - seen.add(value) - try { - return Reflect.ownKeys(value).some((key) => containsBigInt((value as any)[key], seen)) - } catch { - return false - } -} - const hostileIssues: ReadonlyArray> = [ - { - id: "describe-bigint", - reason: - "literal.ts `describe()` formats an unexpected value with JSON.stringify, which throws on a bigint, so " + - "`$.OrgId.eq(0n)` dies with `TypeError: Do not know how to serialize a BigInt` instead of an InvalidLiteral error.", - matches: ({ value }) => containsBigInt(value), - }, - { - id: "like-non-string-pattern", - reason: - "`like`/`notLike`/`ilike` call `pattern.toFragment()` on anything that is not a string, so a number, " + - "object or Map pattern dies with `TypeError: pattern.toFragment is not a function`.", - matches: ({ site, value }) => site === "like" && typeof value !== "string", - }, - { - id: "select-non-expression", - reason: - "A select callback returning a non-expression (`{ bad: 1 }`, `{ bad: undefined }`) dies with " + - "`TypeError: selectExprs[alias].toFragment is not a function` instead of a QueryBuilderError naming the alias.", - matches: ({ site, value }) => - site === "select" && !(value !== null && typeof value === "object" && typeof (value as any).toFragment === "function"), - }, - { - id: "pg-timestamp-invalid-string-dies", - reason: - "The Postgres timestamp literal codec formats an unparseable string or invalid Date with toISOString(), which " + - "throws `RangeError: Invalid time value` inside the transform: compile dies instead of failing InvalidLiteral.", - matches: ({ dialect, site, column, value }) => - dialect === "postgres" && - (site === "param.dateTime" || (column === "Created" && (site === "param.of" || !site.startsWith("param.")))) && - (typeof value === "string" || value instanceof Date) && - Number.isNaN(new Date(value as string).getTime()), - }, + + + + ] export const knownHostileIssue = (input: HostileInput): string | undefined => diff --git a/tests/fuzz/regressions.ts b/tests/fuzz/regressions.ts index 7d8d870..a21260e 100644 --- a/tests/fuzz/regressions.ts +++ b/tests/fuzz/regressions.ts @@ -56,6 +56,9 @@ export const pinned: Record> = { { dialect: "clickhouse", column: "OrgId", site: "eq", value: 0n }, { dialect: "clickhouse", column: "OrgId", site: "like", value: {} }, { dialect: "clickhouse", column: "OrgId", site: "select", value: Number.NaN }, + { dialect: "clickhouse", column: "OrgId", site: "lit", value: 0n }, + { dialect: "clickhouse", column: "OrgId", site: "add", value: undefined }, + { dialect: "clickhouse", column: "OrgId", site: "add", value: new Date(Number.NaN) }, ], "hostile values are refused, never thrown (postgres)": [ { dialect: "postgres", column: "OrgId", site: "param.dateTime", value: "" },