From 5d4e3f049fc3ce110d72e0a8cfa9285f5e1ad831 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 09:04:15 +0000 Subject: [PATCH 1/2] fix(objectql): buildSummaryIndex skips LOUDLY when a roll-up's `reference` carrier is unreadable The child->parent FK was resolved by comparing the child field's `reference` carrier to the parent's name. A carrier no reader can read -- a non-string, where `FieldSchema.reference` declares an optional string -- compared false against every name, `fkField` stayed unset, and the `continue` dropped a DECLARED `summary` field out of both indexes with no diagnostic anywhere. The parent's stored summary value then kept whatever it held through every insert / update / delete of the child, while each write reported success. The resolution RULE is deliberately unchanged (a looser comparison would trade a silent stall for a mis-matched foreign key, which is more expensive). The carrier is read through the one arbiter, `referenceCarrierOf`, and the skip now reports itself at `error` with both the consequence and the fix. Absence (undefined/null/'') stays silent, every readable carrier resolves exactly as before, and the arbiter's refusal is caught rather than propagated so an unreadable sibling cannot hide the readable field that IS the foreign key. Claude-Session: https://claude.ai/code/session_01NcPSwnmJHczmTu6FG7NMjE Co-authored-by: Claude --- ...e-summary-index-unreadable-carrier.test.ts | 307 ++++++++++++++++++ packages/objectql/src/engine.ts | 97 +++++- 2 files changed, 401 insertions(+), 3 deletions(-) create mode 100644 packages/objectql/src/engine-summary-index-unreadable-carrier.test.ts diff --git a/packages/objectql/src/engine-summary-index-unreadable-carrier.test.ts b/packages/objectql/src/engine-summary-index-unreadable-carrier.test.ts new file mode 100644 index 0000000000..c27f4ca457 --- /dev/null +++ b/packages/objectql/src/engine-summary-index-unreadable-carrier.test.ts @@ -0,0 +1,307 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#19082] `buildSummaryIndex()` must not answer an UNREADABLE `reference` + * carrier with a silent *"this parent declares no roll-up"*. + * + * The child→parent foreign key is resolved by scanning the child's + * `master_detail` / `lookup` fields for one whose `reference` names the parent. + * The comparison used to read the carrier raw, so a carrier no reader can read + * — a non-string, where `FieldSchema.reference` declares an optional string — + * compared false against every name, `fkField` stayed unset, and + * + * ```ts + * if (!fkField) continue; // can't resolve the relationship — skip + * ``` + * + * dropped a DECLARED `summary` field out of both indexes with no diagnostic + * anywhere. `recomputeSummaries()` then found nothing to do after every insert + * / update / delete of the child, so the parent's stored summary value kept + * whatever it held while every one of those writes reported success. It is the + * SECOND way this one function invents "nothing to recompute" — the first, its + * registry read, is #9154 (`engine-summary-index-registry-read-failure.test.ts`). + * + * ## What is pinned here, and why BOTH halves are required + * + * ⛔ The remedy is deliberately NOT a looser comparison: that would trade a + * silent stall for a MIS-MATCHED foreign key, which is more expensive. So the + * skip itself is unchanged and two pins are needed to tell "fixed" apart from + * "this path was closed off": + * + * 1. an unreadable carrier makes the skip OBSERVABLE (§2 below); + * 2. a normal `reference` still resolves `fkField` (§1 below) — without this + * one, a change that simply stopped resolving anything would pass §2. + * + * §3 runs both in ONE index build, which is the shape a real registry has. + * + * ## Why the zero counts below are readings and not a dead instrument + * + * Every case drives the SAME `RecordingLogger` through the SAME public handle + * (`getOwnedSummaryDescriptors`, the engine's own parent-side read of the + * index — #6063 — which reaches `buildSummaryIndex()` with no driver in the + * path). §2 measures that recorder at **1**, so the **0** in §1, §4 and §5 is + * this instrument reporting silence rather than this instrument being unable + * to report at all. + * + * ⚠️ Scope, stated so the next reader of the skip branch does not re-file it: + * PR #18503 recorded this site in its **C2** list and #18550 left it there + * deliberately. This change does not move that boundary — the RESOLUTION RULE + * is untouched, and only the SILENCE is closed. + */ + +import { describe, it, expect } from 'vitest'; +import type { ServiceObject } from '@objectstack/spec/data'; +import type { Logger } from '@objectstack/spec/contracts'; +import { ObjectQL } from './engine.js'; + +/** The package id every fixture below is registered under. */ +const OWNER_PACKAGE = 'test-19082'; + +/** + * A `Logger` that keeps what it was told. `error` is a real method, not an + * optional one: the engine reaches for `error` and falls back to `warn`, and a + * recorder missing `error` would silently measure the fallback instead of the + * level this card is about. + */ +class RecordingLogger implements Logger { + readonly errors: string[] = []; + readonly warns: string[] = []; + debug(): void { /* not read by these cases */ } + info(): void { /* not read by these cases */ } + warn(message: string): void { this.warns.push(message); } + error(message: string): void { this.errors.push(message); } +} + +/* + * Fixtures are typed as `ServiceObject` (and registered WITH their + * `packageId`) rather than left to inference, so this file adds nothing to + * `@objectstack/objectql`'s TEST_DEBT ledger — a shrink-only ratchet (#5278). + * The one exception is `badLine`, whose whole point is a `reference` the type + * forbids; it is cast once, at its declaration, and the cast is the statement + * that this value never came through a parse. + */ + +/** Parent whose roll-up resolves — the control. */ +const inv: ServiceObject = { + name: 'inv', + label: 'Invoice', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + line_total: { + name: 'line_total', + label: 'Line total', + type: 'summary' as const, + summaryOperations: { object: 'inv_line', field: 'amount', function: 'sum' as const }, + }, + }, +}; + +/** Child with a READABLE carrier. */ +const invLine: ServiceObject = { + name: 'inv_line', + label: 'Invoice line', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + amount: { name: 'amount', label: 'Amount', type: 'number' as const }, + inv: { name: 'inv', label: 'Invoice', type: 'master_detail' as const, reference: 'inv' }, + }, +}; + +/** Parent whose roll-up cannot resolve — the defect. */ +const bad: ServiceObject = { + name: 'bad', + label: 'Bad invoice', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + line_total: { + name: 'line_total', + label: 'Line total', + type: 'summary' as const, + summaryOperations: { object: 'bad_line', field: 'amount', function: 'sum' as const }, + }, + }, +}; + +/** + * Child whose carrier NO READER CAN READ. `{ object: 'bad' }` is the shape an + * author reaches for when they think `reference` takes a descriptor; + * `ObjectSchema.safeParse` refuses it with a located `invalid_type`, so a + * definition in this shape reached the registry around the parse seam — a raw + * `registerObject`, or a metadata row stored before that tightening. + */ +const badLine = { + name: 'bad_line', + label: 'Bad invoice line', + fields: { + id: { name: 'id', label: 'ID', type: 'text' }, + amount: { name: 'amount', label: 'Amount', type: 'number' }, + bad: { name: 'bad', label: 'Invoice', type: 'master_detail', reference: { object: 'bad' } }, + }, +} as unknown as ServiceObject; + +/** Child that names NO target at all — absence, which is legal and silent. */ +const absentLine: ServiceObject = { + name: 'bad_line', + label: 'Bad invoice line', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + amount: { name: 'amount', label: 'Amount', type: 'number' as const }, + bad: { name: 'bad', label: 'Invoice', type: 'lookup' as const }, + }, +}; + +/** + * Child carrying BOTH an unreadable carrier and, after it, the readable + * `master_detail` that really is the foreign key. Key order matters: the + * unreadable one is declared FIRST, so a scan that let the arbiter's refusal + * propagate would never reach the field below it. + */ +const mixedLine = { + name: 'bad_line', + label: 'Bad invoice line', + fields: { + id: { name: 'id', label: 'ID', type: 'text' }, + stale_ref: { name: 'stale_ref', label: 'Stale', type: 'lookup', reference: ['bad'] }, + amount: { name: 'amount', label: 'Amount', type: 'number' }, + bad: { name: 'bad', label: 'Invoice', type: 'master_detail', reference: 'bad' }, + }, +} as unknown as ServiceObject; + +/** A fresh engine with `objects` registered and a recorder on the log sink. */ +function makeEngine(objects: ServiceObject[]): { engine: ObjectQL; logger: RecordingLogger } { + const logger = new RecordingLogger(); + const engine = new ObjectQL({ logger }); + for (const o of objects) engine.registry.registerObject(o, OWNER_PACKAGE); + return { engine, logger }; +} + +/** Errors this card's diagnostic is responsible for, isolated from any other. */ +const skipDiagnostics = (logger: RecordingLogger): string[] => + logger.errors.filter((m) => m.startsWith('[summary-index]')); + +describe('[#19082] buildSummaryIndex — an unreadable `reference` carrier skips LOUDLY', () => { + describe('§1 control — a readable carrier still resolves `fkField`', () => { + it('indexes the roll-up and says nothing', () => { + const { engine, logger } = makeEngine([inv, invLine]); + + const owned = engine.getOwnedSummaryDescriptors('inv'); + + // Without this half, a change that simply stopped resolving + // anything would satisfy §2 — "fixed" and "closed this path off" + // would be indistinguishable. + expect(owned).toHaveLength(1); + expect(owned[0].summaryField).toBe('line_total'); + expect(owned[0].childObject).toBe('inv_line'); + expect(owned[0].fkField).toBe('inv'); + expect(skipDiagnostics(logger)).toEqual([]); + expect(logger.warns.filter((m) => m.startsWith('[summary-index]'))).toEqual([]); + }); + }); + + describe('§2 the defect — an unreadable carrier emits an observable skip signal', () => { + it('reports the skip once, naming the consequence and the fix', () => { + const { engine, logger } = makeEngine([bad, badLine]); + + const owned = engine.getOwnedSummaryDescriptors('bad'); + + // ⛔ The skip is NOT repaired by guessing the foreign key: a looser + // comparison would index a MIS-MATCHED FK, which is worse than the + // stall. The descriptor stays absent; what ends is the silence. + expect(owned).toEqual([]); + + const reported = skipDiagnostics(logger); + expect(reported).toHaveLength(1); + const [msg] = reported; + // WHO: the declared summary field that is not being maintained. + expect(msg).toContain('bad.line_total'); + // WHERE: the field whose carrier could not be read. + expect(msg).toContain('bad_line.bad'); + // THE CONSEQUENCE, concretely — the half a bare "could not resolve" + // leaves out, and the reason this is an `error` and not a `warn`. + expect(msg).toContain('will NOT recompute'); + expect(msg).toContain('reports success'); + // THE FIX, both spellings the author can reach for. + expect(msg).toContain("reference: 'bad'"); + expect(msg).toContain('summaryOperations.relationshipField'); + }); + + it('speaks at `error`, not at `warn`', () => { + const { engine, logger } = makeEngine([bad, badLine]); + engine.getOwnedSummaryDescriptors('bad'); + + // A persisted summary silently stops tracking its children while + // every write keeps reporting success — the durability class, whose + // level is `error` by AGENTS.md's own question. A `warn` here is the + // one level at which an operator is never told. + expect(skipDiagnostics(logger)).toHaveLength(1); + expect(logger.warns.filter((m) => m.startsWith('[summary-index]'))).toEqual([]); + }); + }); + + describe('§3 both in ONE build — the control and the defect do not interfere', () => { + it('the readable roll-up is indexed while the unreadable one is reported', () => { + const { engine, logger } = makeEngine([inv, invLine, bad, badLine]); + + const good = engine.getOwnedSummaryDescriptors('inv'); + const broken = engine.getOwnedSummaryDescriptors('bad'); + + expect(good.map((d) => d.fkField)).toEqual(['inv']); + expect(broken).toEqual([]); + expect(skipDiagnostics(logger)).toHaveLength(1); + expect(skipDiagnostics(logger)[0]).toContain('bad.line_total'); + }); + }); + + describe('§4 absence is not unreadability — and stays silent', () => { + it('a relation field that names no target skips without a diagnostic', () => { + const { engine, logger } = makeEngine([bad, absentLine]); + + // `FieldSchema.reference` is `.optional()` and `StrictField` + // declares it nullable: naming no target is a legal thing for a + // field to say, it was silent before this change, and it is silent + // after. Only UNREADABILITY is new. + expect(engine.getOwnedSummaryDescriptors('bad')).toEqual([]); + expect(skipDiagnostics(logger)).toEqual([]); + }); + }); + + describe('§5 an unreadable SIBLING must not hide the real foreign key', () => { + it('resolves the readable `master_detail` declared after it, silently', () => { + const { engine, logger } = makeEngine([bad, mixedLine]); + + // The arbiter THROWS on an unreadable carrier, and the two cascade + // seams #19080 routed through it let that throw propagate. This + // scan cannot: it is looking FOR the foreign key across every + // relation field, so a propagating refusal on `stale_ref` would + // turn a roll-up that works today into a hard failure of every + // write to `bad_line`. Nothing is dropped here, so nothing is + // reported here either. + const owned = engine.getOwnedSummaryDescriptors('bad'); + expect(owned).toHaveLength(1); + expect(owned[0].fkField).toBe('bad'); + expect(skipDiagnostics(logger)).toEqual([]); + }); + }); + + describe('§6 said once per index BUILD, never once per read', () => { + it('repeats only when the registry moves, not on every consult', () => { + const { engine, logger } = makeEngine([bad, badLine]); + + for (let i = 0; i < 5; i++) engine.getOwnedSummaryDescriptors('bad'); + + // `ensureSummaryIndexes()` memoises the built pair against the + // registry's `objectRevision`, which moves on a metadata MUTATION + // and never on a data write — so this diagnostic cannot become a + // per-write log line. + expect(skipDiagnostics(logger)).toHaveLength(1); + + // A metadata mutation invalidates the stamp, the index rebuilds, + // and the condition — still present — is reported again. Measured + // rather than assumed: without this leg, a "1" above could equally + // mean the diagnostic is emitted exactly once per process. + engine.registry.registerObject(inv, OWNER_PACKAGE); + engine.getOwnedSummaryDescriptors('bad'); + expect(skipDiagnostics(logger)).toHaveLength(2); + }); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 4ca2839cae..1fa48b7183 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -9005,7 +9005,24 @@ export class ObjectQL implements IObjectQLEngine { * — which returns `undefined` on every failure branch it models — over a * fold that is spreads and comparisons. No I/O, no driver, no `throw` on the * measured path. The pin lives in - * `engine-summary-index-registry-read-failure.test.ts`. */ + * `engine-summary-index-registry-read-failure.test.ts`. + * + * [#19082] The SECOND way this one function invented *"nothing to + * recompute"* — a different input, the same outcome. The child→parent FK + * was resolved by comparing the child field's `reference` carrier to the + * parent's name, so a carrier no reader can read (a non-string where + * `FieldSchema.reference` declares an optional string) compared false + * against every name, `fkField` stayed unset, and the `continue` below + * dropped a DECLARED roll-up out of both indexes with nothing logged. + * + * ⛔ The fix is NOT a looser comparison — that would trade a silent + * stall for a MIS-MATCHED foreign key, which is more expensive. The + * carrier is read through the one arbiter and the skip now SAYS SO, at + * `error` (a persisted summary silently stops tracking its children while + * every write keeps reporting success — the durability class). Absence and + * every readable carrier behave exactly as before; the resolution rule + * itself is deliberately left where PR #18503's C2 boundary put it. Pinned + * in `engine-summary-index-unreadable-carrier.test.ts`, both directions. */ private buildSummaryIndex(): { byChild: Map; byParent: Map; @@ -9025,18 +9042,92 @@ export class ObjectQL implements IObjectQLEngine { if (!childObject || !fn) continue; // Resolve the FK on the child pointing back to this parent. let fkField: string | undefined = so.relationshipField; + // [#19082] Candidate relation fields on the child whose `reference` + // carrier no reader can read, spelled `.`. Collected + // rather than thrown on — see the two comments below. + const unreadableCarriers: string[] = []; if (!fkField) { const child = this._registry.getObject(childObject) as any; const cfields = child?.fields || {}; for (const [cfName, cdef] of Object.entries(cfields)) { const cd: any = cdef; - if ((cd?.type === 'master_detail' || cd?.type === 'lookup') && cd?.reference === parent.name) { + if (cd?.type !== 'master_detail' && cd?.type !== 'lookup') continue; + // [#19082] The carrier is read through the ONE arbiter, the same + // one {@link ObjectQL.planCascadeAtomicity} and + // {@link ObjectQL.cascadeDeleteRelations} were routed through — + // but its refusal is CAUGHT here instead of propagated, for a + // reason those two seams do not have: this is a SCAN over every + // relation field on the child, looking for the one that points + // back. Letting the first unreadable carrier throw would hide a + // perfectly readable sibling that IS the foreign key, turning a + // roll-up that works today into a hard failure of every write. + // The three answers stay distinct: + // + // - ABSENCE (`undefined` / `null` / `''`) is unchanged and still + // silent — `referenceCarrierOf` answers `undefined` for all + // three, this `continue` skips the field, and a field that + // names no target is a legal thing to declare. + // - A READABLE carrier compares exactly as `cd.reference === + // parent.name` compared it, so every roll-up that resolved + // before resolves now. + // - UNREADABILITY is recorded, and it is what the skip below + // stops being silent about. + let carrier: string | undefined; + try { + carrier = referenceCarrierOf(cd, 'ObjectQL.buildSummaryIndex'); + } catch { + unreadableCarriers.push(`${childObject}.${cfName}`); + continue; + } + if (carrier === parent.name) { fkField = cfName; break; } } } - if (!fkField) continue; // can't resolve the relationship — skip + if (!fkField) { + // [#19082] The skip is unchanged; its SILENCE is what ends here. + // + // An unreadable carrier compares false against every name, so + // `fkField` stayed unset and this `continue` dropped a DECLARED + // roll-up out of both indexes with no diagnostic anywhere — the + // second way this one function invents "nothing to recompute" (the + // first, the registry read, is #9154 above). It is a durability + // degradation by the repo's own test: every write keeps reporting + // success while a persisted summary value silently stops tracking + // its children, so it is an `error`, not a `warn`, and it owes both + // the consequence and the fix in the line it prints. + // + // Said once per INDEX BUILD, never once per write: + // {@link ensureSummaryIndexes} memoises the result against the + // registry's `objectRevision`, which moves only on a metadata + // mutation. And only the unreadable case speaks — a roll-up whose + // child genuinely declares no relation field at all is a different + // (and much louder at authoring time) condition, left exactly as it + // was rather than widened into here. + if (unreadableCarriers.length > 0) { + const msg = + `[summary-index] ${parent.name}.${summaryField} is NOT indexed and will NOT recompute: ` + + `its roll-up over '${childObject}' has no resolvable foreign key, because ` + + `${unreadableCarriers.length} candidate relation field(s) on '${childObject}' carry a ` + + `\`reference\` no reader can read (${unreadableCarriers.join(', ')}). ` + + `\`FieldSchema.reference\` is an optional STRING — the target object's name — and a ` + + `non-string carrier is refused by \`ObjectSchema.safeParse\`, so such a definition ` + + `reached this registry around the parse seam (a raw \`registerObject\`, or a metadata ` + + `row stored before that tightening). CONSEQUENCE: ${parent.name}.${summaryField} keeps ` + + `whatever value it holds now through EVERY insert / update / delete of '${childObject}', ` + + `while every one of those writes reports success and nothing else reports this. FIX: ` + + `spell the carrier as the target object name (reference: '${parent.name}') on the ` + + `child's master_detail/lookup field, or name the foreign key explicitly with ` + + `summaryOperations.relationshipField on ${parent.name}.${summaryField}.`; + // Sanctioned logger shape (PR #9750): reach for `error`, fall back + // to `warn` — ⛔ never an optional call like `logger.error?.()`, + // which emits nothing against a sink that has no `error`. + if (typeof this.logger.error === 'function') this.logger.error(msg); + else this.logger.warn(msg); + } + continue; // can't resolve the relationship — skip + } // Optional per-summary predicate: only child rows matching it are // aggregated (e.g. sum receipts where { status: 'received' }). ANDed with // the parent-FK match at recompute time. Ignore a non-object filter. From d413c5e8d21d56ffe72140c19ceed47bb8958b29 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 09:12:54 +0000 Subject: [PATCH 2/2] chore(changeset): patch @objectstack/objectql for the loud summary-index skip Claude-Session: https://claude.ai/code/session_01NcPSwnmJHczmTu6FG7NMjE Co-authored-by: Claude --- ...mary-index-unreadable-carrier-loud-skip.md | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 .changeset/19082-summary-index-unreadable-carrier-loud-skip.md diff --git a/.changeset/19082-summary-index-unreadable-carrier-loud-skip.md b/.changeset/19082-summary-index-unreadable-carrier-loud-skip.md new file mode 100644 index 0000000000..25e70116b0 --- /dev/null +++ b/.changeset/19082-summary-index-unreadable-carrier-loud-skip.md @@ -0,0 +1,20 @@ +--- +"@objectstack/objectql": patch +--- + +`buildSummaryIndex` no longer drops a declared `summary` field silently when the roll-up's `reference` carrier cannot be read — the skip now reports itself at `error`, naming the field, the consequence and the fix (#19082). + +The child→parent foreign key is resolved by scanning the child object's `master_detail` / `lookup` fields for one whose `reference` names the parent. That comparison read the carrier raw (`cd.reference === parent.name`), so a carrier **no reader can read** — a non-string, where `FieldSchema.reference` declares an optional string — compared `false` against every name, `fkField` stayed unset, and + +```ts +if (!fkField) continue; // can't resolve the relationship — skip +``` + +removed the roll-up from **both** summary indexes. `recomputeSummaries()` then had nothing to do after every insert / update / delete of the child, so the parent's stored summary value kept whatever it held while each of those writes reported success, and nothing anywhere said so. It is the second way this one function invents *"nothing to recompute"*; the first, its registry read, was closed as #9154. + +- **⛔ The resolution rule is deliberately unchanged.** Loosening the comparison would trade a silent stall for a **mis-matched foreign key**, which is more expensive: a roll-up quietly aggregating the wrong children reads exactly like a correct one. PR #18503 recorded this site in its C2 list and the #18550 round left it there on purpose; that boundary still stands. What ends is only the silence. +- **The carrier is read through the one arbiter**, `referenceCarrierOf` — the same accessor #19080 routed the two delete-cascade seams through. Its refusal is **caught** here rather than propagated, because this is a *scan* looking for the foreign key across every relation field: a propagating refusal on one unreadable field would hide a readable sibling that really is the FK, turning a roll-up that works today into a hard failure of every write to that child. +- **`error`, not `warn`**, and said once per index build rather than once per write. A persisted summary that silently stops tracking its children while every write keeps reporting success is the durability class, and the line it prints carries both halves an operator needs: what is not being maintained and will not recompute, and the two ways to fix it — spell the carrier as the target object's name, or name the FK explicitly with `summaryOperations.relationshipField`. +- **Absence is untouched.** `undefined`, `null` and `''` mean "this field names no target", which is a legal thing to declare; they skip silently exactly as before. Every readable carrier resolves exactly as before. + +No schema changed, no key was added or removed, and nothing that resolved before resolves differently now. `engine-summary-index-unreadable-carrier.test.ts` pins both directions — the unreadable carrier reporting its skip, and a normal `reference` still resolving `fkField` — because without the second one, a change that simply stopped resolving anything would look identical to a fix.