Skip to content

Commit beac798

Browse files
fix(driver-sql): a bare comparand nested under $and/$or/$not gets the top-level refusal and compilation (#19908)
Fixes #19885 Clause-②: no ## PostgreSQL reading (triage's mandatory first step) **The nested shape is a SILENT WRONG ANSWER on PostgreSQL, not an error.** Measured on a live Postgres 16.13 against `origin/main` `029d8a4710`, text column `tags`, with a decoy row whose text is `{"a"}`: | filter | SQLite | Postgres 16.13 | |---|---|---| | `{ tags: ['a'] }` (top level, no operator anywhere) | 400 `INVALID_FILTER` | 400 `INVALID_FILTER` | | `{ $or: [{ tags: ['a'] }] }` (same for `$and`, and two deep) | 500 `DATABASE_ERROR` | resolves `["r_decoy"]` | | `{ $not: { tags: ['a'] } }` | 500 `DATABASE_ERROR` | resolves every row except the decoy, including `r_a` | | `{ tags: ['a'], name: { $ne: 'zz' } }` (no combinator, one operator sibling) | 500 `DATABASE_ERROR` | resolves `["r_decoy"]` | `pg` serialises a JS array as its array-literal text (`{"a"}`), so the text comparison selects the one row storing that text and nothing the caller meant. Reach on this tree: `parseFilterAST` and the engine's object-form comparand walk (`normalizeFilterComparandTypes`) both **accept** the array leaf in every position above (measured on `029d8a4710`). The shared-face refusal from the #19757 ruling has not landed yet, so today this shape reaches the driver through the platform doors. By triage's criterion that re-grades the card to **p2**. MySQL was not measurable here (no server binary, no docker daemon); CI's `Temporal Conformance (live PG + MySQL)` job runs the new suite's MySQL cell. ## What was wrong A filter with no operator anywhere compiles through `compileFilters`' plain `{ field: value }` loop, which runs `assertCompilableComparand` on every value. Any other filter (a combinator, or a single sibling key carrying an operator) routes the whole node through `applyFilterCondition`. That path's leaf handling differed from the loop in two ways: 1. **Its bare-value branch carried the column gate (`assertOperatorAppliesToColumn`) but not the comparand gate.** `assertOperatorAppliesToColumn`'s own docblock says the two gates are called from the same three positions, and this was the third position, missing the comparand gate. That is the card's defect, and it covers the operator-sibling case too, which the card did not name. 2. **It took "any non-array object" to be an operator map, with a different wrong answer per shape.** A `Date` has no own entries, so it emitted nothing and the leaf was dropped from the WHERE clause. A non-empty binary comparand (`Buffer` / `Uint8Array`) had its byte indices read as operator names, so it was refused with `INVALID_FILTER` / 400 `Unsupported filter operator "0" on field "blob"`, although the same leaf at top level compiles to `blob = ?`. An empty binary comparand had no entries and was dropped like the `Date`. `nullGuardForFieldSpec`, which builds the `$not` NULL guard, made the same reading. The fix gives it the same `isFilterNode` test. Before that, an empty buffer under `$not` got no guard and compiled to a bare `NOT (blob = ?)`, which drops the NULL row. Measured on the same tree: | filter | SQLite | Postgres 16.13 | |---|---|---| | `{ at: someDate }` (top level) | `["r_a"]` | `["r_a"]` | | `{ $and: [{ at: someDate }] }`, `$or`, beside an operator sibling | **every row** | **every row** | | `{ $not: { at: someDate } }` | only the NULL rows (the guard without its leaf) | same | Binary, measured on SQLite against the base blob of `sql-driver.ts` (`029d8a4710`), `blob` column, rows `ab` / `c` / empty / NULL: | filter | before | after (`fb4f11dcf6`) | |---|---|---| | `{ blob: Buffer ab }` (top level) | `["r_ab"]`, `blob = ?` | unchanged | | the same leaf under `$and` / `$or` / `$not` / beside an operator sibling | 400 `INVALID_FILTER`, `Unsupported filter operator "0" on field "blob"` | compiles `blob = ?`; `$and` / `$or` / sibling answer `["r_ab"]`, `$not` answers `["r_c","r_empty","r_null"]` | | `{ blob: empty Buffer }` (top level) | `["r_empty"]`, `blob = ?` | unchanged | | the same leaf under `$and` / `$or` / `$not` | every row (no WHERE clause at all) | compiles `blob = ?`; `$and` / `$or` answer `["r_empty"]`, `$not` answers `["r_ab","r_c","r_null"]` | The engine's object-form walk keeps a `Date` leaf a `Date`, so server-side engine callers reach the `Date` half. `parseFilterAST` and the engine walk both refuse a binary comparand in every position, so the binary half reaches direct driver callers only. ## The fix (`packages/drivers/driver-sql/src/sql-driver.ts`: `applyFilterCondition` and `nullGuardForFieldSpec`) - The field-key branch now tests `isFilterNode(value)`, the predicate the validating walk (`classifyFilterKey`) and the top-level loop already use, instead of "non-array object". A `Date` or binary comparand now takes the bare-value branch, which compiles it exactly as the top-level loop does. - The bare-value branch now calls `assertCompilableComparand(field, '=', value, condition)` before the column gate, in the same order the top-level loop uses. - (Patch round 1.) `nullGuardForFieldSpec`, which builds the `$not` NULL guard, now reads a comparand with the same `isFilterNode` test, so a binary comparand under `$not` returns the rows whose column is NULL, as every other negated equality does. Only non-plain-object comparands move; scalars, `Date`, arrays and operator maps are classified exactly as before. On A2: the check runs inside the one walk that compiles every leaf, which is the emitter. No second pre-walk was added. On A3: the top-level refusal carries no filter path (its caller-visible message is redacted, and the server-side diagnostic names only the operator and the field), and the #8197 ruling withholds the filter path. So the nested refusal is the top-level one, unchanged: same code, status, message and diagnostic. The suite asserts that equality. The dispatch's Zone 1 wording "with the path of the offending leaf" conflicts with A3 plus that ruling. I followed A3. **In-place fix of the `Date` half, declared.** It is outside the card's literal text (an array leaf). I fixed it here because all four in-place conditions hold: ① same defect class (a bare equality-slot comparand under a combinator compiled differently from the same comparand at top level, in the same branch decision); ② the fix is mechanical and its target shape is already pinned (the top-level loop's compilation, and the walk's own `isFilterNode`); ③ the file is inside this card's claimed surface, and no other claim holds it (the neighbour #19868 branch at `4afdb8c170` touches only driver-turso; `git merge-tree --write-tree` of this head against it exits 0); ④ it is covered by the same gate family (driver-sql's own suite), with no new verification surface. One predicate closes both halves. **Clause-② declaration.** `Clause-②: no`. Two accept-set movements need a citation. First, a non-empty binary comparand nested under a combinator moves from refused (`Unsupported filter operator "0"`) to accepted. Second, an empty one moves from silently dropped to compiled. Both are removed misreadings, not a widening, and the published filter reference already negated the refusal. `content/docs/references/data/filter.mdx` on `origin/main` (`d1ca8741dd`), the `where` row, reads verbatim: "A field-keyed entry is a condition on that field — a bare value is implicit equality, an object is a map of field operators — and `$and` / `$or` / `$not` combine conditions." The `$eq` row reads: "the DEFAULT operator: a bare value written against a field key is the same condition as this one". A `Buffer` in a field-value position is a bare value under that text, not an object in its operator-map sense. The spec's own walkers classify it that way: on this tree `parseFilterAST` and `normalizeFilterComparandTypes` both answer `{ $and: [{ blob: Buffer }] }` with "Filter comparand at where.$and[0].blob is a Buffer instance". That names it a comparand, refused on type, never read as an operator map named `0`. The driver's own top-level loop and its validating walk (`isFilterNode`) read it the same way. So the nested refusal contradicted the published sentence, and the change brings the nested positions into line with it. The platform accept set does not move: both walkers still refuse a binary comparand in every position. Only the direct-driver surface changes, and there only to the driver's existing top-level binding (`isBindableComparand` admits `ArrayBuffer.isView`). The `$not` NULL-guard change is covered by the same `where` row: "`$not` is NULL-safe: a row whose compared column is null does NOT satisfy the negated condition and IS returned." ## Tests New `packages/drivers/driver-sql/src/sql-driver-19885-nested-bare-comparand.test.ts`, run on every cell of the D-A3 driver axis via `declareDialectCell`: - The array leaf under `$and`, `$or`, `$not`, two deep (`$or` over `$and`, and `$not` over `$or`), and beside an operator-carrying sibling: each gets `INVALID_FILTER` / 400, with the **same** message and withheld diagnostic as the top-level refusal. There is a top-level control. - Valid nested scalar controls (`$and`, `$or`, `$not`, depth 2, sibling, a null comparand) keep their row sets on every cell. On SQLite, each compiles to the **exact SQL captured from the unfixed driver**. The probe diffed pre- and post-fix SQL on SQLite and Postgres and found it byte-identical. - A nested `Date` leaf under `$and`, `$or`, depth 2, a sibling and `$not` answers what the top level answers. Patch round 1 at `fb4f11dcf6`: five SQLite-only binary pins were added (a non-empty `Buffer` under `$and` with SQL, binding and rows; the same leaf under `$or`, beside an operator sibling and under `$not`; an empty `Buffer` under `$and`; an empty `Buffer` under `$not` keeping the NULL row; a fixture control). The dialect-matrix fixture has no binary column, and the platform doors refuse binary before any driver. New file on SQLite: 25 passed, 2 skipped. Full driver-sql suite (Test Core shape): 180 files passed, 11 skipped; 2670 tests passed, 170 skipped. Typecheck: exit 0. Ablation, leg A (both emitter hunks reverted): 15 failed / 10 passed / 2 skipped, all four binary behaviour pins red. Leg B (only the guard line reverted): 1 failed / 24 passed / 2 skipped, the only red being the empty-`Buffer`-under-`$not` pin. Both restores proven by blob equality. Live Postgres was not re-run this round: the change moves only non-plain-object comparands and the matrix fixture has none; CI's live job runs the matrix at this head. Round 1, runs at `8bde954772`: - New file on SQLite plus live Postgres 16.13: **39 passed, 1 skipped** (the unprovisioned MySQL cell, a named skip). - Full driver-sql suite (the Test Core shape, no live URLs): `vitest run --maxWorkers=2` gives **180 files passed, 11 skipped; 2665 tests passed, 170 skipped**. - `pnpm --filter @objectstack/driver-sql typecheck`: exit 0. `tsc --listFiles` includes the new test file. **Ablation (once, both hunks reverted, then restored).** The mutation went through `scripts/ablation-replace.mjs`: two nested wrap-mode calls, each anchor hitting once and landing on disk (anchor counts 1 to 0, blob `9f204f7468` to `4721560ff2` to `041310cdc0`), plus a bash `trap` restore on EXIT/INT/TERM. The subject is imported by relative path from `src/`, so no `dist/` leg exists. - Result: **22 failed, 17 passed, 1 skipped**. Every array-leaf refusal went red on both cells: SQLite answered `DATABASE_ERROR` instead of `INVALID_FILTER`, and Postgres resolved `["r_decoy"]`, or `["r_a","r_b","r_null"]` under `$not`. Every nested `Date` case went red (every row, or only the NULL rows under `$not`). All controls, row sets and SQL pins stayed green. - Restore was proven by blob equality: `9f204f74680a27857b13c780fbd146aa16c882ad` equals HEAD's, `git diff HEAD` is empty, and porcelain is clean. ## Gates At `fb4f11dcf6` (patch round 1): `--commands` re-derived the same 62 commands; all were re-run, 60 exited 0 and the same 2 exited 3 (NOT MEASURED, declared to CI); `--ran` ✓. `check-issue-citations`, `check-changeset-no-major --base origin/main` and `check:nul-bytes`: exit 0. Round 1, at `8bde954772`: - `node scripts/pm/dispatch-gates.mjs --commands` (no paths) derived 62 commands. I ran all of them with exit codes captured before any pipe. **60 exited 0 and 2 exited 3 (`PREREQUISITE NOT MET`, NOT MEASURED):** - `check:dual-build-cjs-loads` needs every package's `dist/`. As a targeted probe, driver-sql's own CJS build loads. - `check:type-check-debt` needs the whole `./packages/*` build closure. - Both are declared to CI. The diff changes no exported type: both hunks are inside a method body, and `dist/index.d.ts` carries none of the new text. - `check:lean-entry-closure` first exited 3, then 0 after building `@objectstack/objectql`. - `node scripts/pm/dispatch-gates.mjs --ran` gives **62 derived, 60 run, 2 NOT MEASURED, 0 UNRUN**, verdict ✓, exit 0. - `node scripts/check-issue-citations.mjs` (live, by hand, because the derived family runs only `--self-test`): exit 0, "every citation this change adds resolves". - `pnpm check:driver-conformance`: exit 0. - Lint, narrowed and proven: `eslint --no-inline-config --format json` on the two touched `.ts` files gives 2 files, 0 errors, 0 warnings. The changeset `.md` matches no eslint config (`--print-config` prints `undefined`), so it is outside the population. `eslint.config.mjs` enables no type-aware linting (the printed configs carry no `parserOptions.project` or `projectService`), so this diff cannot move a verdict on any untouched file. The repo-wide `pnpm lint` is CI's. ## Acceptance notes - **Boundary, not filed.** A malformed leaf inside a branch that a boolean identity settles, for example `{ $or: [{}, { tags: ['a'] }] }`, is still answered by the identity (every row) rather than refused. The operator spelling `{ $or: [{}, { tags: { $eq: ['a'] } }] }` answers the same way on `main` today (measured on SQLite, post-fix). `assertCompilableComparand` has always been an emitter-side gate, and the value returned is the logically correct TRUE. Moving the comparand gate onto the validating walk would be a separate design change. - The new file's MySQL cell is unmeasured locally. The fixture uses whole-second instants on a driver-created `datetime` field, the shape the temporal conformance corpus already runs on every cell. - The PR body is written once. Any later edit is requested through the report. --- _Generated by [Claude Code](https://claude.ai/code/session_01TEhopqrWQYBycZzyJHpAZr)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent a34c27c commit beac798

3 files changed

Lines changed: 455 additions & 3 deletions

File tree

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
---
2+
'@objectstack/driver-sql': patch
3+
---
4+
5+
A bare comparand nested under `$and` / `$or` / `$not` now gets the same compilation and the same refusal it gets at top level
6+
7+
A filter with no operator anywhere compiles through one loop; any other filter (a
8+
combinator, or a single sibling key carrying an operator) compiles through
9+
`applyFilterCondition`. That second path handled a bare `{ field: value }` leaf in
10+
two ways the first one did not:
11+
12+
- **An array in the equality slot was not refused.** `{ tags: ['a'] }` at top level
13+
answers `INVALID_FILTER` / 400, but under `$and` / `$or` / `$not`, or beside an
14+
operator-carrying sibling (`{ tags: ['a'], name: { $ne: 'x' } }`), it was bound as
15+
it stood. SQLite refused the bind, so the caller got a 500 `DATABASE_ERROR` for a
16+
filter it can fix. Postgres bound the array as its array-literal text (`{"a"}`)
17+
and silently returned the wrong rows. Every such leaf now gets the top-level
18+
refusal: the same code, status, message and server-side diagnostic.
19+
- **A `Date` comparand was dropped, and a binary one was refused or dropped.** That
20+
path treated any non-array object as an operator map. A `Date` has no entries, so
21+
`{ $and: [{ closed_at: someDate }] }` emitted no predicate for the leaf and
22+
answered every row on SQLite and Postgres, while `{ closed_at: someDate }`
23+
answered the matching rows. A non-empty binary comparand (`Buffer` / `Uint8Array`)
24+
had its byte indices read as operator names, so it was refused with
25+
`INVALID_FILTER` / 400 `Unsupported filter operator "0"`, although the same leaf
26+
at top level compiles. An empty one was dropped like the `Date`. Only a plain
27+
object is now read as an operator map (the same test the filter-validating walk
28+
already uses), so all of these compile as the equality they are at top level.
29+
The `$not` NULL guard now reads a comparand the same way, so a binary comparand
30+
under `$not` returns the rows whose column is NULL, as every other negated
31+
equality does; an empty one used to get no guard at all.
32+
33+
Valid filters are unchanged. A scalar leaf in any of these positions compiles to
34+
byte-identical SQL, and the new suite pins that SQL against the output captured
35+
before the fix. Neither shape is stopped before the driver today: `parseFilterAST`
36+
and the engine's object-form comparand walk both pass the array leaf through, and
37+
the engine's walk also keeps a `Date` leaf a `Date`. Both refuse a binary
38+
comparand in every position, so the binary half reaches direct driver callers only.

0 commit comments

Comments
 (0)