Skip to content

Refuse invalid queries before SQL is sent: typed params, GROUP BY and dialect checks - #18

Merged
Makisuo merged 5 commits into
mainfrom
feat/query-soundness
Oct 4, 2026
Merged

Makisuo merged 5 commits into
mainfrom
feat/query-soundness

Conversation

@Makisuo

@Makisuo Makisuo commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Compared the builder's type safety with Kysely (0.29.6) by writing ~45 deliberately invalid queries and checking each against tsc, compile, and PGlite. Many type-checked and either emitted invalid SQL or SQL that cannot mean what it says. This PR closes those holes. The goal it enforces: every invalid query is either a type error or a typed QueryBuilderError / QueryBuilderDefect from compile, before any SQL is sent.

Three commits, one per tier:

1. Refuse invalid queries the builder used to accept

  • A second where() / having() ANDs with the first instead of replacing it (as Kysely does), on queries and writes, so a shared base keeps its tenant filter.
  • Comparisons refuse null (type error, plus a QueryBuilderError at runtime); use isNull(). Empty in_() / notIn() render as 1 = 0 / 1 = 1 instead of IN ().
  • like / ilike accept nullable strings (previously rejected).
  • limit / offset reject negative or fractional literals at the type level and NaN / negative / fractional runtime values at compile, instead of emitting LIMIT NaN or rounding.
  • Join aliases must not repeat, shadow a FROM column, or equal the FROM alias; CTE names must be unique (previously ambiguous SQL or a raw TypeError).
  • A query with no select() cannot be compiled, run, joined, used in FROM, a CTE, EXISTS, or INSERT ... SELECT.
  • unionAll branches must agree on aliases and compatible column types; an empty union is a type error.
  • inSubquery / notInSubquery need exactly one column of a comparable type.
  • update().set({}) is a type error; an UPDATE / DELETE without where() or allRows() cannot be compiled or run (phantom ready-state).

2. Aggregates, GROUP BY, and dialect function sets

  • Each clause renders under a small render tracker (src/sql/render-tracker.ts) that records columns read outside an aggregate and whether the clause aggregates. compile refuses an aggregate in WHERE or a join's ON, a selected/HAVING column that is neither grouped nor aggregated, and grouping by an aggregate.
  • Only SQL the builder writes is counted. Raw SQL, CH.sql templates, windows, and caller-declared functions are opaque, so they can hide an error but never cause a false one.
  • Built-in functions belong to a function set (Dialect.functions): CH.count() (which emits count()) in a Postgres compile is a defect, and the reverse. coalesce, nullIf, and lower stay portable.

3. Params in the type

  • Expr<T, P> / Condition<P> carry the param.* placeholders inside them, via a contravariant phantom, so Expr<T, P> still goes wherever Expr<T> does.
  • Queries, unions, inserts, updates, and deletes collect params from every clause, subquery, join, and CTE. compile, compileUnion, and Database.run require each param with a value of its type, and the error spells out paramsRequired: { … }. Extra keys are allowed.
  • Built-in functions, defineFn / defineCondFn, subquery predicates, window specs, and CH.sql pass their arguments' params on.
  • Insert rows and SET records also reject columns the table cannot write.

Reviewer notes

  • Breaking. Listed in CHANGELOG.md under Unreleased; docs/queries.md, expressions.md, updates-and-deletes.md, tenant-scoping.md, postgres.md, params-and-compilation.md, and extending.md are updated.
  • Downstream impact. Expect new errors in Maple code. The GROUP BY check already found one invalid fixture in this repo (compile.test.ts, an ungrouped column next to GROUP BY). Code passing params as an untyped Record<string, unknown> will need a typed object.
  • Known gaps. A custom function built with makeExpr drops param tracking unless its signature carries Q (shown in docs/extending.md). untypedSubqueryExpr<T>(…) with an explicit type argument does the same. In both cases compile still checks the params at runtime. The IN (subquery) column-count check is type-level only.
  • Tests that pinned the old runtime failures (missing or mistyped params, empty SET, a write without where) are now also marked @ts-expect-error, so they cover both layers.
  • New tests: src/ch/soundness.test-d.ts, src/ch/soundness.test.ts, src/ch/params-propagation.test-d.ts.

Testing

  • tsc --noEmit (both configs): clean
  • vitest run: 584 passed, 193 skipped (integration suites needing a live ClickHouse/Postgres; not run here)
  • Doc example, citation, and export-catalog checks: pass
  • tsdown build: pass

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Summary by CodeRabbit

  • New Features
    • Query parameters are tracked through queries and expressions, with compile-time checks for missing or mismatched values.
    • Query builders validate selections, joins, unions, grouping, and write operations, catching more invalid queries before execution.
    • Repeated where() and having() calls now combine conditions with AND; empty IN lists compile to predictable true/false results.
    • Dialect-specific functions are checked during compilation, and nullable strings are supported by like and ilike.
  • Bug Fixes
    • Invalid LIMIT and OFFSET values and comparisons against null or undefined now produce clear errors rather than generating invalid SQL.

Makisuo and others added 3 commits October 4, 2026 16:37
- A second where()/having() ANDs with the first instead of replacing it,
  so a shared base keeps its tenant filter.
- Comparisons refuse null (type error, and a QueryBuilderError at runtime);
  an empty IN list renders as 1 = 0 / 1 = 1 instead of IN ().
- like/notLike/ilike accept a nullable string column.
- limit/offset reject negative or fractional literals, and NaN, negative or
  fractional runtime values, instead of emitting or rounding them.
- Join aliases must not repeat or shadow a FROM column (type error), nor
  repeat the FROM alias; CTE names must be unique (compile-time defect).
- A query with no select() cannot be run, compiled, joined, used in FROM,
  a CTE, EXISTS or INSERT ... SELECT.
- unionAll branches must agree on aliases and column types; an empty union
  is a type error.
- inSubquery/notInSubquery require exactly one column of a comparable type.
- update().set({}) is a type error; an UPDATE or DELETE without where() or
  allRows() cannot be compiled or run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…er dialect

Each clause is rendered under a render track that records the columns it
reads outside an aggregate and whether it aggregates. compile uses it to
refuse an aggregate in WHERE or a join's ON, a selected or HAVING column that
is neither grouped nor aggregated, and grouping by an aggregate. Only SQL the
builder writes is counted; raw SQL, templates, windows and caller-declared
functions are opaque, so they can hide an error but never cause a false one.

Built-in functions now belong to a function set. A ClickHouse function in a
Postgres compile (count() instead of count(*)) is a defect, and the reverse;
coalesce, nullIf and lower stay portable.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Expr and Condition carry the params inside them (a contravariant phantom,
so an Expr<T, P> still goes wherever an Expr<T> does). Queries, unions,
inserts, updates and deletes collect them from every clause, subquery,
join and CTE; compile, compileUnion and Database.run require each one with
a value of its type. Built-in functions, defineFn/defineCondFn, subquery
predicates and CH.sql pass their arguments' params on.

Insert rows and SET records also reject columns the table cannot write,
now that their types are inferred.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 4, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8db044a5-5e08-46b9-9639-89ef19e9b82c
📥 Commits

Reviewing files that changed from the base of the PR and between 90b846b and 2ebe906.

📒 Files selected for processing (48)
  • CHANGELOG.md
  • docs/expressions.md
  • docs/extending.md
  • docs/params-and-compilation.md
  • docs/postgres.md
  • docs/queries.md
  • docs/tenant-scoping.md
  • docs/updates-and-deletes.md
  • src/ch/compilation-regressions.test.ts
  • src/ch/compile.test.ts
  • src/ch/compile.ts
  • src/ch/core-dsl.test.ts
  • src/ch/define-fn.ts
  • src/ch/dialect.test.ts
  • src/ch/dialect.ts
  • src/ch/expr.ts
  • src/ch/functions/aggregate.ts
  • src/ch/functions/array.ts
  • src/ch/functions/builtin.ts
  • src/ch/functions/conditional.ts
  • src/ch/functions/date-time.ts
  • src/ch/functions/json.ts
  • src/ch/functions/map.ts
  • src/ch/functions/numeric.ts
  • src/ch/functions/string.ts
  • src/ch/functions/window.ts
  • src/ch/insert.test.ts
  • src/ch/insert.ts
  • src/ch/literal.test.ts
  • src/ch/param.ts
  • src/ch/params-propagation.test-d.ts
  • src/ch/publish-readiness.test.ts
  • src/ch/query.ts
  • src/ch/soundness.test-d.ts
  • src/ch/soundness.test.ts
  • src/ch/sql-template.ts
  • src/ch/subquery.ts
  • src/ch/union.ts
  • src/ch/update.test.ts
  • src/ch/update.ts
  • src/database/database.test.ts
  • src/database/database.ts
  • src/pg/dialect.ts
  • src/pg/functions.ts
  • src/pg/postgres.test.ts
  • src/schema/define.ts
  • src/sql/render-tracker.ts
  • src/sql/sql-fragment.ts
 _____________________________________________________________
< I'm sorry, Dave. I'm afraid I can't let you write that bug. >
 -------------------------------------------------------------
  \
   \   (\__/)
       (•ㅅ•)
       /   づ
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Makisuo and others added 2 commits October 4, 2026 22:59
- makeExpr / makeUntypedExpr / makeCond take the expressions they
  interpolate as `uses` and carry their params; compiling one whose SQL
  holds a param no `uses` entry carries is a defect, so a param can no
  longer reach a query without being in its type.
- An explicit type argument on makeExpr, subqueryExpr or
  compileTypedFnCall is now an error (the inferred parameter comes first)
  instead of silently dropping params; untypedSubqueryExpr returns
  Expr<unknown>.
- inSubquery / notInSubquery check at compile time that the subquery
  selects exactly one column, for callers past the types.
- param.dateTimeString / dateTimeSeconds accept a Date or DateTime.Utc in
  the type, as they do at runtime.
- Integration fixtures and the docs behaviour check no longer rely on a
  second where() replacing the first.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The tarball check compiles a query without its param to exercise the
runtime failure; params are now in the type, so that call is a type error
too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Makisuo
Makisuo merged commit be1a3ec into main Oct 4, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant