Skip to content

Commit ceb4a93

Browse files
fix(objectql)!: having and the per-aggregation filter refuse a { $field } against a column with no comparison class, as where does; applyInMemoryAggregation takes the reference rules (#21406)
Fixes #21299 Clause-②: no (narrowing) **Status: the seat answered the open question with C (5952549615 on #21299).** The no-class sentence stays `objectql`-local (`noClassReason`) beside `crossClassReason`, its precedent. The REST twin pin asserts that each engine position's reason equals the real `where` twin's diagnostic, verbatim, and that is the drift guard. The dispatch's no-third-copy clause is relaxed for this sentence only. Patch round 1 (`b397060136`) replaced the PENDING docblock with the decision record. A spec-side home (option A below) is recorded as a candidate for the spec lane in the seat's hand-over ledger, not filed. Dispatched by `domain:engine#1` (seat post #6367), claim 5949513813, branch `claude/issue-21299-having-no-class-refusal`. Base `db0cf2231b`, `origin/main` merged at `69a12a0952`, head `b397060136` (patch round 1; round 0 was `466e632c33`). ## What changes - **`packages/objectql/src/having-filter.ts`** - The seam `crossClassReferenceViolation` now acts on the spec verdict's `no-class` answer as well as `cross-class`. Its reason is `where`'s sentence for the same pair: the referenced column first, then the target, as `driver-sql`'s `applyCrossFieldComparison` asks them. No second rule: both answers come from the one `crossFieldComparisonVerdict` call. - Both callers ask it of every reference. With `addDays`, only the `no-class` answer is taken from the seam (`where` asks it before it reads an offset); the cross-class half of an `addDays` pair stays `offsetPairViolation`'s, unchanged. - `assertAggregationFilterReferencesAreDeclared` is exported (module-level only; neither package entry re-exports `having-filter.ts`). `AggregationFilterDeclaration.object` becomes optional, so a caller with no object name gets a diagnostic that names the field alone. - **`packages/objectql/src/in-memory-aggregation.ts`**: `applyInMemoryAggregation`, when handed `fields`, runs `assertAggregationFilterReferencesAreDeclared` on each per-aggregation filter before the JSON-column rule, before any row. That is the same function and the same order `engine.aggregate` uses. The signature is unchanged. Without `fields`, nothing new is judged. - **Pins** - `packages/objectql/src/engine-aggregate-reference-verdict-positions.test.ts` (new): the enumeration pin and the card's shapes at the engine door. - `packages/objectql/src/engine-aggregate-filter.test.ts`: #21242's direct `applyInMemoryAggregation` pin is split in two. With no field map it still answers 1 of 4. With a field map the cross-class pair is now refused, which is the ruled direction ("one door, whichever caller enters it"). - `packages/rest/src/aggregation-filter-where-doors.test.ts`: the positions × verdicts parity table against the real `where` twin on `SqlDriver`. The reasons are read from the twin's own withheld diagnostic, never retyped. `boot()` now takes an optional object and rows. - **`.changeset/21299-having-no-class-reference.md`**: `@objectstack/objectql` `minor`, BREAKING, `Clause-②: no (narrowing)`, `adr-0087: not-required (no-migration-prescription)`. ## Baseline and after (H1), through `engine.aggregate` on `SqlDriver` / better-sqlite3 Probe fixture: 6 rows, `photo` image, `tags` multiselect, `due_f` formula (`returnType: date`). Base `db0cf2231b`; after = `be8d2c428b` (built dists). | shape | base: populated | base: empty | after (both) | `where` twin (base and after) | |---|---|---|---|---| | per-aggregation `{ customer_id: { $ne: { $field: 'photo' } } }` (text vs image) | counted 6 of 6 | 0 | `INVALID_FILTER` / 400 | 400, `"photo" (type "image") has no scalar stored column a comparison can read.` | | per-aggregation `{ amount: { $ne: { $field: 'tags' } } }` (number vs multiselect) | 400 from the per-row array floor (row-dependent; 6 of 6 when `tags` is null) | 0 | `INVALID_FILTER` / 400 | 400, `"tags" (type "multiselect") ...` | | `having: { photo: { $ne: { $field: 'n' } } }` on an image groupBy | kept all 6 groups | `[]` | `INVALID_FILTER` / 400 | twin `photo` vs `amount`: 400, `the target field "photo" (type "image") ...` | | per-aggregation `{ closed_at: { $lte: { $field: 'due_f' } } }` (datetime vs formula) | counted 0 of 6 | 0 | `INVALID_FILTER` / 400 | 400, `"due_f" (type "formula") ...` | | control: `{ due_on: { $lte: { $field: 'due_on' } } }` | 6 | 0 | 6 / 0, unchanged | 6 / 0 | After the change, each per-aggregation refusal logs `where`'s reason verbatim, and the REST pin compares it against the twin's diagnostic. ## Hypotheses - **H1, confirmed.** On the base, `crossClassReferenceViolation` acted only on `cross-class`, through its two callers. `CROSS_FIELD_NO_CLASS_REASONS` is `list-or-object`, `file`, `formula`. `where`'s words are `driver-sql`'s, under `INVALID_FILTER` / 400. - **H2, none exists.** The sentence "has no scalar stored column a comparison can read" is written twice, inline, in `driver-sql`'s `applyCrossFieldComparison` (referent arm and target arm), and exported nowhere. `objectql` does not depend on `driver-sql`. `@objectstack/spec` and `@objectstack/core` hold no no-class sentence. Two other no-class wordings exist, neither of them `where`'s: `@objectstack/formula`'s `describeColumn` (behind the exported `findCrossFieldClassRefusal`) and `@objectstack/lint`'s RLS / sharing-rule doors. Per the dispatch, adding one to `packages/spec` is a spec edit, so this run stopped before it. The seat answered with C (5952549615); see the decision record below. - **H3, no changed answer for the existing callers.** `packages/verify`'s `checkDateBucketParity` calls `applyInMemoryAggregation(rows, ast)` with no `fields`, so the reference rules have nothing to judge. - Read, not run: no other in-repo caller outside `objectql` passes `fields` either (the dogfood parity tests, `service-analytics`' bucket-key test, the `driver-mongodb` parity test). - Measured against the rebuilt dists: `@objectstack/verify` 16 files / 120 tests passed, and dogfood 168 files passed (1 skipped), 1374 tests passed (9 skipped). - The one answer that moved is #21242's own with-field-map pin, and it moved by the ruling. - **H4, posture kept, and recorded as rows.** - Registry-less host: not judged at the per-aggregation filter, at `having`, or at `applyInMemoryAggregation` without `fields`. - Audit-opt-out object's row-carried `created_at` / `updated_at`: not judged at the engine positions. Measured on `SqlDriver`: the `where` twin refuses it 400 ("the target field "created_at" is not a declared field"), while the per-aggregation filter counts 6 of 6. That divergence is pinned in the REST table as the recorded posture. - #21336's pins stay green: the registry-less `picked: 3`, `['c1']` and the audit-opt-out 1 of 4. - **H5, unreachable, and pinned.** A `multiple: true` select, lookup or user is refused `INVALID_FIELD` / 400 by the engine's groupBy door and by its `min` / `max` door before `having` is read. Measured on `SqlDriver` and in the pin, for a groupBy, `min` and `max` of each. The same holds for `multiselect` and `json`. The only no-class columns that reach `having` are a file-family groupBy (every driver) and a formula groupBy on a driver whose door does not refuse it (the engine's in-memory path; `driver-sql` refuses a formula groupBy itself). ## The enumeration pin Spec verdict names: the dispatch's `same-class` is the spec's `comparable`. | verdict | `where` (driver-sql) | `having` | `aggregations[i].filter` | `applyInMemoryAggregation` (with `fields`) | |---|---|---|---|---| | `comparable` | answered | answered | answered | answered | | `cross-class` | 400 | 400 | 400 | 400 | | `no-class` | 400 | 400; list-or-object is refused earlier by the groupBy door | 400; a formula KEY is refused earlier by the materializable-filter door (`INVALID_FIELD`), as in `where` | 400 | | `unjudged` (type outside `FieldType`) | judged by the driver's own aliases | unreachable: the registry refuses the object | unreachable: same | not judged | | no declaration | 400 | not judged | not judged | not judged | - **objectql pin.** It sweeps all 15 × 15 ordered pairs at each of the three engine-side positions. The columns are one per row of `CROSS_FIELD_COMPARISON_TYPE_CLASSES`, plus `select` with `multiple: true`, plus a type outside `FieldType`. Each cell's expected answer is computed from `crossFieldComparisonVerdict` itself. A refused cell must be 400 before any read, and must name both classes or the no-class column with its declared type; the filter positions must keep those names off the wire. - **What makes it fail until placed.** - The verdict posture is a `Record` keyed by the spec's verdict union, so a new verdict is a type error. It is also a runtime failure: the set of verdicts the sweep meets must equal the record's keys. - The no-class reasons the sweep covers must equal `CROSS_FIELD_NO_CLASS_REASONS`. - The filter slots of `EngineAggregateOptionsSchema` must equal `where`, `having` and `aggregations[i].filter`. - The published exports of the two evaluator modules must equal `applyInMemoryAggregation` (a position) and `bucketDateValue` (not a position). - **REST pin.** One row per verdict this object can give: comparable, cross-class, and no-class for file, list-or-object and formula. Each row runs at all four positions, empty and populated. Every position's answer (refusal envelope, or total count) must equal the `where` twin's. For the no-class rows, the twin's reason, read from its withheld diagnostic, must appear verbatim in the per-aggregation log line, in `applyInMemoryAggregation`'s diagnostic and, for the file row, in the `having` message. ## Reverse verification (at `623b7474bd`, before the merge) The mutation was made with `node scripts/ablation-replace.mjs` and its EXIT / INT / TERM trap restore. - **Mutation.** It replaced only the seam's no-class arm, `if (verdict.verdict === 'no-class') return noClassReason(...)`, with an always-false guard carrying the marker `ABLATION_21299`. Anchor 1 → 0; blob `f4c9c878df05` → `eb68be65ef10`. - **Build.** `objectql` was rebuilt (exit 0). `ablation-dist-preflight` found the marker in 4 built files. - **Prediction, then reading.** The no-class refusal pins go red and the controls stay green. That is what happened: - `objectql`: **10 failed / 303 passed** (313). The 10 are the three sweeps, the three filter shapes, the `having` image shape, the two `addDays`-against-a-formula rows (per-aggregation and `having`, both answered with the arm off), and the audit-on `created_at` vs image control. The comparable, cross-class, census, no-declaration, `multiple`-seam and same-class rows stayed green, as did all of `engine-aggregate-filter.test.ts` and `engine-aggregate-having-comparand-shape.test.ts`. - `rest`: **3 failed / 23 passed** (26). The 3 are the no-class rows (file, list-or-object, formula). The comparable and cross-class rows, the audit-opt-out posture row and every earlier test stayed green. - **Restore.** Proven four ways: the blob equals HEAD (`f4c9c878df05`), `git diff HEAD` is empty, `--absent` preflight passes after a rebuild ("marker absent from all 14 built files", whole tree clean), and `git status --porcelain` is empty. The re-run was 313 / 26 passed. ## Verification (all heavy runs through `os-verify-lock.sh`) The suites below ran at `be8d2c428b` (the merge). Two commits followed. `251922e411` gives the pin file's two single-choice columns their `options`, the legal declaration `FieldSchema` now requires; `466e632c33` corrects one changeset sentence. At `251922e411` that file was re-run (26 passed) and `objectql` was re-typechecked (exit 0, test layer held at 40 / 234). The whole gate union was re-run at `466e632c33` (below). - Full `@objectstack/objectql`: `--project local` 365 files, **7381 passed**; `--project repo` 1 file, 5 passed. `VERDICT command-exit 0`. - `@objectstack/rest` `aggregation-filter-where-doors.test.ts`: 26 passed. `@objectstack/verify`: 16 files, 120 passed. Dogfood (`pnpm --filter @objectstack/dogfood exec vitest run`) against the rebuilt dists: 168 passed + 1 skipped files, 1374 passed + 9 skipped tests. Each run's verdict was `command-exit 0`. - Typecheck: - `pnpm --filter @objectstack/objectql run typecheck` exit 0. Its test layer held at 40 files / 234 errors, unchanged, so the new pin file adds none. - `pnpm --filter @objectstack/rest run typecheck` exit 0, test layer 0 errors. - Build after the merge: `turbo run build --filter='@objectstack/dogfood^...' --filter=@objectstack/rest --filter=@objectstack/verify`, 63 of 63 tasks. - Gates, at `466e632c33`: - `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands` derived 65 commands. All 65 ran with exit 0, each exit code recorded beside the printed command. - `--ran` reports "65 derived, 65 run, 0 NOT-MEASURED, 0 UNRUN (a DERIVED zero)". - In the earlier pass at `251922e411`, `check:dual-build-cjs-loads` answered `PREREQUISITE NOT MET` (exit 3: 8 unrelated packages had no `dist/`). Those packages were built (41 of 41 turbo tasks from cache), and the gate exited 0 there and in the final pass. - The four roster gates whose rosters sit under my paths also exited 0: `check-changeset-fixed`, `check:authz-resolver`, `check:error-code-casing`, `check:filter-alias-parity`. - NOT MEASURED, because their argv takes workflow values and CI runs them: `check-issue-citations --census`, `check-shard-attestation`, `check-test-completeness`. - Lint, a proven narrowing: - Command: `pnpm exec eslint --no-inline-config --format json` on the 5 changed `.ts` files gives 5 results, 0 errors, 0 warnings. - Why the narrowing holds: `eslint.config.mjs` is not type-aware (no `parserOptions.project`; header near `:328`), so this diff cannot move any untouched file's verdict. - The full `pnpm lint` is CI's. ## Deviations (both accepted by the seat, 5952549615; the `rest` test file is declared on the cli seat post, 5952559844) - `packages/rest/src/aggregation-filter-where-doors.test.ts` is outside the claim's file surface, and it is a test only. The `where` twin can only be refused on a real `driver-sql`, which `objectql` does not depend on. This is the same harness and the same reason as #21255's deviation 2. - The seam judges the `no-class` answer for an `addDays` pair too. Before, an `addDays` pair against a formula was answered at both positions. One against a file or list column was refused, but in the `addDays` pair rule's words: that rule reads those types as `text`, so it answered with a cross-class or an offset sentence (read from the source, not measured). It now gets the no-class sentence. This stays within "no-class references at these positions", and the changeset says so. ## Decision record (H2): C The seat chose **C** in 5952549615. `objectql` keeps the local copy (`noClassReason`), on the `crossClassReason` precedent, which landed with #21255 (PR #21297, contract review PASS 5944780154). The REST twin pin is the drift guard. Two options were not taken: - **A**, a pure sentence function beside the verdict in `packages/spec`: it adds a published spec export (`Clause-②: yes`, spec-lane work) and a `driver-sql` edit outside this claim. It would also move the cross-class sentence. It is recorded as a candidate for the spec lane, not filed, because no reader gets a wrong answer today. - **B** strands `formula`, which depends on `spec` alone. **Patch round 1** (`b397060136`, report 5953205116) changed only `noClassReason`'s docblock, +8/−5, every line inside the JSDoc block. At that head: - the pins pass: `objectql` 313 and `rest` 26; - `objectql` typecheck exits 0; - gates: 65 derived, 65 run, all exit 0; - the full suites stand from `be8d2c428b`, because no source line outside comments changed since then (proven by the filtered diff). ## Acceptance notes (nothing filed) - `having` refusal messages exceed the REST envelope's 500-character bound, so the wire cuts the closing clause about how a column's class is read. The no-class reason sits in the surviving prefix (the REST pin asserts it on the wire). This predates the PR (#21255's note). Carrier: 承接者:无. - The `where` words for a cross-class pair say "is stored as" where the engine positions say "is" (#20127's convention for a computed column). The no-class sentence needs no such reading and is byte-identical. --- _Generated by [Claude Code](https://claude.ai/code/session_017xfMoEjKUuSh2xYB8sCozp)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 5e58193 commit ceb4a93

6 files changed

Lines changed: 958 additions & 55 deletions
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
---
2+
"@objectstack/objectql": minor
3+
---
4+
5+
fix(objectql)!: `having` and the per-aggregation `filter` refuse a `{ $field }` comparison against a column with no comparison class (a file field, a list, a formula) with `INVALID_FILTER` / 400, as `where` refuses it; `applyInMemoryAggregation` takes the same reference rules
6+
7+
Clause-②: no (narrowing)
8+
9+
<!-- adr-0087: not-required (no-migration-prescription) a refusal of filter STRUCTURE at the engine's query door and at its published in-memory aggregation function: a { $field } comparison against a column the spec's crossFieldComparisonVerdict answers no-class for, at having and at a per-aggregation filter, and the reference rules engine.aggregate already applied, now applied by applyInMemoryAggregation when its caller passes a field map. No authorable key, spelling, export or stored shape moves: FieldReferenceSchema, every query shape and every object definition parse as before, the package entries export nothing new and nothing less, applyInMemoryAggregation keeps its signature, and no stored row is read or rewritten. What is refused is a comparison the same query's where already refuses on driver-sql, and which comparable column the caller meant is not something a ledger entry can decide. The other categories are closed on facts: the package publishes (not unpublished); no ADR-0087 id covers a filter's comparison class (not registered / already-registered); and the change is runtime behaviour, not a declaration (not runtime-interface-only / type-surface-only). -->
10+
11+
**BREAKING**: this narrows what a `{ $field }` reference may pair at two positions of `engine.aggregate`, and what `applyInMemoryAggregation` accepts when it is handed a field map. It ships as `minor` under the launch-window convention for accept-set narrowings. No export or published type changes.
12+
13+
**What was accepted before.** The spec's comparison-class verdict (`crossFieldComparisonVerdict`) answers `no-class` for a pair in which either column has no comparison class: a list or an object (a structured-JSON type, a multi-option type, a multi-capable type flagged `multiple: true`), a file field (`FILE_REFERENCE_TYPES`), or a formula. `having` and a per-aggregation `filter` (`aggregations[i].filter`) did not judge that answer. Measured on `SqlDriver` over better-sqlite3 through `engine.aggregate`, beside a `where` twin that `driver-sql` refused `INVALID_FILTER` / 400 each time:
14+
15+
- a per-aggregation `{ customer_id: { $ne: { $field: 'photo' } } }` (text against an image) counted 6 of 6 rows;
16+
- a per-aggregation `{ closed_at: { $lte: { $field: 'due_f' } } }` (datetime against a formula) counted 0 of 6;
17+
- a per-aggregation `{ amount: { $ne: { $field: 'tags' } } }` (number against a multiselect) counted 6 of 6 when the column held no value, 0 on an empty table, and was refused by the per-row array check, in other words, when the column held a list;
18+
- `having: { photo: { $ne: { $field: 'n' } } }` over a groupBy on an image field kept all 6 groups.
19+
20+
A `{ $field, addDays }` pair against a formula was answered at both positions. `applyInMemoryAggregation`, called with a field map, applied none of `engine.aggregate`'s reference rules: a reference to a field the map does not declare, a pair across two classes and a pair against a column with no class were all counted.
21+
22+
**What is refused now.** At `having` and at a per-aggregation `filter`, a scalar comparison (`$eq`, `$ne`, `$gt`, `$gte`, `$lt`, `$lte`) whose `{ $field }` comparand, or whose own column, has no comparison class, with or without `addDays`. The refusal is `INVALID_FILTER` / 400, raised before any driver is asked for a row, on an empty set as on a populated one, in the reason `driver-sql`'s `where` logs for the same pair: the column it names (the referenced one first, as `where` asks it first) "has no scalar stored column a comparison can read". The per-aggregation `filter` withholds the fields, the operator and the reason from the message and writes them to the server log, as `where` does; `having` names the two columns of the query's own projection. A `{ $field, addDays }` pair against a file or list column was already refused, in the `addDays` pair rule's words (that rule reads those types as text, so it answered with a cross-class or an offset sentence); it is now refused in this one.
23+
24+
`applyInMemoryAggregation(rows, ast, timezone, fields, reportWithheld)`, when `fields` is passed, judges each per-aggregation `filter` by the reference rules `engine.aggregate` applies at that position, through the same function, before any row is judged: the referenced column (and an `addDays` offset column) is declared, a pair across two classes or against a column with no class is refused, and an `addDays` pair follows its class rule. The refusal is `INVALID_FILTER` / 400; the withheld diagnostic goes to `reportWithheld`, and it names no object (this function is not told one).
25+
26+
**The remedy.** Compare two columns that each have a comparison class, and the same one: a file field, a list and a formula have no stored scalar a comparison can read. Compare the scalar column the value is derived from, or filter the column with a literal.
27+
28+
**Unchanged.** A reference between two columns of one class answers as before, and the cross-class refusal keeps its words. A side with no declaration is not judged at any of the three positions: a host with no registered object, a column the field map does not carry (`id`, and an audit-opt-out object's row-carried `created_at` / `updated_at`), an aggregation over an undeclared field. A declared type outside `FieldType` is not judged either. `applyInMemoryAggregation` called without `fields` judges nothing it did not judge before.

‎packages/objectql/src/engine-aggregate-filter.test.ts‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -958,12 +958,26 @@ describe('[#21242] per-aggregation filter — a { $field } pair no seam lowers i
958958
}
959959
});
960960

961-
it('a direct applyInMemoryAggregation call — no seam, no class rule — 1 of 4, was 3, with or without a field map', () => {
962-
// The class rule is `engine.aggregate`'s; this published function applies
963-
// the JSON-column rule alone, so a cross-class pair reaches the matcher.
961+
it('a direct applyInMemoryAggregation call with no field map — no declaration, no class rule — 1 of 4, was 3', () => {
962+
// No declaration, so the pair reaches the matcher, compared as written.
964963
const ast = withFilter({ closed_at: { $lte: { $field: 'due_on' } } }) as never;
965964
expect(applyInMemoryAggregation([...DAY_ROWS], ast)).toEqual([{ opp_count: 4, picked: 1 }]);
966-
expect(applyInMemoryAggregation([...DAY_ROWS], ast, undefined, FIELDS)).toEqual([{ opp_count: 4, picked: 1 }]);
965+
});
966+
967+
it('[#21299] …with a field map, the same call takes engine.aggregate\'s reference rules: a datetime against a date is refused, before any row', () => {
968+
// Was 1 of 4 at #21242's head: this published function applied the
969+
// JSON-column rule alone. It now runs the reference rules through the same
970+
// function `engine.aggregate` does, so one filter gets one answer whichever
971+
// door it enters by.
972+
const ast = withFilter({ closed_at: { $lte: { $field: 'due_on' } } }) as never;
973+
const withheld: string[] = [];
974+
for (const rows of [[], [...DAY_ROWS]]) {
975+
const err = syncRefusalOf(() => applyInMemoryAggregation(rows, ast, undefined, FIELDS, (d) => withheld.push(d)));
976+
expect(err?.code).toBe('INVALID_FILTER');
977+
expect(err?.status).toBe(400);
978+
}
979+
expect(withheld).toHaveLength(2);
980+
for (const line of withheld) expect(line).toContain('"closed_at" is datetime but "due_on" is date');
967981
});
968982
});
969983

0 commit comments

Comments
 (0)