From 5a36f35e3ba3602d997b9b17ea83b62e12bc3a89 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 14:48:55 +0000 Subject: [PATCH] fix(objectql): seed count/sum roll-up summaries at parent insert (#5749) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `recomputeSummaries()` only ever visits parents named by a CHILD write (`recs`/`prevs` -> `desc.fkField`), so a parent that has never had a child is never visited and its summary column keeps insert's `null`. Delete the last child and the parent IS visited (via `previous`) and lands on 0 — one logical state, two values. The consequence is not cosmetic: `= 0` / `< 1` filters compare in the database and silently DROP every parent that never had a child; sorting, GROUP BY and formula fields reading it inherit the same null. Fixed at the producer. `buildSummaryIndex` now publishes the identical descriptors under a second, parent-side view, and `insert` seeds the count/sum summaries a new row OWNS with the empty-collection value right after `applyFieldDefaults`. The empty-set function list is extracted to `summaryEmptySetValue` so the insert seed and the recompute fallback read ONE list — min/max/avg have no empty-set value and stay `null`, unchanged. Boundaries: author-supplied values are never overwritten (same `!= null` rule as `applyFieldDefaults`, #2706) and `beforeInsert` still has the final say; a roll-up whose relationship cannot be resolved is not seeded, so "seeded" and "maintained by recompute" stay the same set; existing rows are untouched — this is create-time only and backfill is a separate decision. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --- .../summary-count-zero-on-parent-insert.md | 17 ++ packages/objectql/src/engine.ts | 152 ++++++++++++++-- packages/objectql/src/summary-rollup.test.ts | 166 ++++++++++++++++++ 3 files changed, 321 insertions(+), 14 deletions(-) create mode 100644 .changeset/summary-count-zero-on-parent-insert.md diff --git a/.changeset/summary-count-zero-on-parent-insert.md b/.changeset/summary-count-zero-on-parent-insert.md new file mode 100644 index 0000000000..876bbbb631 --- /dev/null +++ b/.changeset/summary-count-zero-on-parent-insert.md @@ -0,0 +1,17 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): 新建父行时把 count/sum 型 `Field.summary` 汇总字段初始化为 0 + +`recomputeSummaries()` 只会重算「本次子记录写入所指向的父行」,所以一个**从未有过子记录**的父行永远不会被访问到,汇总列停在 insert 时的 `null`;而把最后一条子记录**删掉**的父行反而会被访问到(经由 `previous`)并落到 `0`。同一个「零个子记录」的逻辑状态因此读出两个不同的值。 + +后果不是显示难看,而是**筛选静默漏行**:`["task_count","=",0]` / `["task_count","<",1]`(库内比对)会把「从来没建过任务的项目」整行丢掉,没有任何报错;排序、GROUP BY、以及以该字段为输入的公式字段(null 传播)同样受影响。 + +本次在**生产端**修:父行 insert 时,按父对象取一份 summary descriptor,把该行自己拥有的 `count` / `sum` 汇总字段落成空集合的值 `0` —— 与 `recomputeSummaries` 的空集兜底共用同一份函数清单(现已提取为单一来源),所以「从未有过子记录」与「删光子记录」必然读到同一个值。`min` / `max` / `avg` 在空集上没有定义,仍然保持 `null`,口径不变。 + +边界: + +- 作者显式提供的值不会被覆盖(与 `applyFieldDefaults` 同口径:insert 时 `undefined` 与显式 `null` 都算「未提供」);`beforeInsert` 钩子仍有最终决定权。 +- 关系无法解析的汇总字段不落初值 ——「落了初值」与「会被重算维护」是同一个集合,不会出现一个没人维护的 `0`。 +- **存量数据不受本 PR 影响**:这是 create-time 初始化,已经存成 `null` 的老父行仍然是 `null`,直到某次子记录写入把它重算。存量回填是独立取舍,另行处理。 diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index d326e157d6..0d234a2369 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -804,6 +804,23 @@ interface SummaryDescriptor { filter?: Record; } +/** + * The value a roll-up summary takes over an **empty** child collection (#5749). + * + * `count` and `sum` are defined on the empty set — zero children is zero, not + * "unknown" — while `min`/`max`/`avg` are not, so those stay `null`. This is the + * ONE place that list is written down: {@link ObjectQL.recomputeSummaries} uses + * it for the post-aggregate fallback (an aggregate over no rows returns + * `null`/`undefined` on every driver), and the insert-time initialiser + * {@link ObjectQL.initializeSummaryFields} uses it to seed a brand-new parent + * row with the same value the first recompute would have produced. Two sites, + * one list — a parent that has never had a child and a parent whose last child + * was deleted are the SAME logical state and must read the same value. + */ +function summaryEmptySetValue(fn: SummaryDescriptor['fn']): number | null { + return fn === 'count' || fn === 'sum' ? 0 : null; +} + // `implements IObjectQLEngine` is the verification step of #4251 B3: every // member the `objectql` slot's contract declares is checked against this class // on every build, so the seven consumer-local surface declarations the contract @@ -4156,6 +4173,15 @@ export class ObjectQL implements IObjectQLEngine { * parent objects that aggregate it. Invalidated when packages register. */ private summaryIndex: Map | null = null; + /** The SAME descriptors, indexed the other way: parent object name → the + * roll-up summary fields that object OWNS. Built in the same pass as + * {@link summaryIndex} and invalidated with it. The child index answers + * "whose summaries must I recompute after writing this row"; this one answers + * "which of my own summary fields must be seeded when I create this row" + * (#5749) — the question the child index structurally cannot answer, because + * a parent that has never had a child appears in no child write. */ + private summaryIndexByParent: Map | null = null; + /** * Retry options for roll-up summary recompute (framework#3147). Public so a * test can inject a no-op sleep for deterministic backoff; production uses @@ -4166,12 +4192,20 @@ export class ObjectQL implements IObjectQLEngine { /** Invalidate the cached roll-up summary index (call when metadata changes). */ private invalidateSummaryIndex(): void { this.summaryIndex = null; - } - - /** Scan all registered objects for `summary` fields and index them by the - * child object they aggregate, resolving the child→parent FK field. */ - private buildSummaryIndex(): Map { + this.summaryIndexByParent = null; + } + + /** Scan all registered objects for `summary` fields and index them BOTH ways + * — by the child object they aggregate and by the parent object that owns + * them — resolving the child→parent FK field. One scan, two views of the + * identical descriptor objects, so the two indexes can never disagree about + * which roll-ups exist. */ + private buildSummaryIndex(): { + byChild: Map; + byParent: Map; + } { const index = new Map(); + const byParent = new Map(); let objects: any[] = []; try { objects = (this._registry as any).getAllObjects?.() ?? []; } catch { objects = []; } for (const parent of objects) { @@ -4204,18 +4238,34 @@ export class ObjectQL implements IObjectQLEngine { const filter = so.filter && typeof so.filter === 'object' && !Array.isArray(so.filter) ? so.filter as Record : undefined; + const descriptor: SummaryDescriptor = { + parentObject: parent.name, summaryField, fkField, fn, sourceField: so.field, filter, + }; const list = index.get(childObject) ?? []; - list.push({ parentObject: parent.name, summaryField, fkField, fn, sourceField: so.field, filter }); + list.push(descriptor); index.set(childObject, list); + // Same descriptor, parent-side view. Only descriptors that made it this + // far are indexed either way, so "seeded at insert" and "maintained by + // recompute" are the same set by construction — a roll-up whose + // relationship could not be resolved (the `continue` above) is left + // untouched on both paths rather than seeded with a 0 nothing updates. + const owned = byParent.get(parent.name) ?? []; + owned.push(descriptor); + byParent.set(parent.name, owned); } } - return index; + return { byChild: index, byParent }; } /** `registry.objectRevision` the cached {@link summaryIndex} was built at. */ private summaryIndexRevision = -1; - private getSummaryDescriptors(childObject: string): SummaryDescriptor[] { + /** + * Ensure both roll-up indexes are present and current. Split out of + * {@link getSummaryDescriptors} so the parent-side view (#5749) shares the + * exact same staleness rule instead of re-deriving one. + */ + private ensureSummaryIndexes(): void { // Rebuild whenever the REGISTRY's object set has moved since the index was // built — not only when someone remembered to call // `invalidateSummaryIndex`. That single site (`registerApp`) is bypassed by @@ -4228,11 +4278,67 @@ export class ObjectQL implements IObjectQLEngine { // "已完成任务数" shipped empty over correct metadata (cloud#970). const revision = (this._registry as unknown as { objectRevision?: number })?.objectRevision; const stale = typeof revision === 'number' && revision !== this.summaryIndexRevision; - if (!this.summaryIndex || stale) { - this.summaryIndex = this.buildSummaryIndex(); + if (!this.summaryIndex || !this.summaryIndexByParent || stale) { + const built = this.buildSummaryIndex(); + this.summaryIndex = built.byChild; + this.summaryIndexByParent = built.byParent; if (typeof revision === 'number') this.summaryIndexRevision = revision; } - return this.summaryIndex.get(childObject) ?? []; + } + + /** Roll-up descriptors for summaries that aggregate `childObject` — i.e. the + * ones a write to `childObject` must recompute. Semantics unchanged. */ + private getSummaryDescriptors(childObject: string): SummaryDescriptor[] { + this.ensureSummaryIndexes(); + return this.summaryIndex!.get(childObject) ?? []; + } + + /** Roll-up descriptors for the summary fields `parentObject` OWNS (#5749) — + * i.e. the ones a NEW row of `parentObject` must have seeded. */ + private getOwnedSummaryDescriptors(parentObject: string): SummaryDescriptor[] { + this.ensureSummaryIndexes(); + return this.summaryIndexByParent!.get(parentObject) ?? []; + } + + /** + * Seed the roll-up `summary` fields a freshly-created row owns (#5749). + * + * `recomputeSummaries` only ever visits parents named by a child write, so a + * parent that has NEVER had a child is never visited and its summary column + * keeps whatever insert put there — `null`. Delete the last child and the + * parent DOES get visited (via `previous`) and lands on 0. Same logical state, + * two different values: `filter ["task_count","=",0]` silently skipped every + * parent that never had a child, and so did sorting, GROUP BY and any formula + * reading the field (null propagation). + * + * The fix is at the producer: write the empty-collection value at create time, + * so `count`/`sum` start at 0 and only ever move to another number. + * `min`/`max`/`avg` have no empty-set value and deliberately stay `null` — + * {@link summaryEmptySetValue} is the single list both this and the recompute + * fallback read. + * + * Author-supplied values are never overwritten. The `!= null` test matches + * {@link applyFieldDefaults} exactly (#2706): on INSERT an explicit `null` is + * "no value supplied", any real value — including a deliberate 0 or a seeded + * count — is respected. Runs before the `beforeInsert` hooks for the same + * reason defaults do, so a hook still has the final say. + * + * Existing rows are untouched: this is create-time only, so parents already + * stored with `null` stay `null` until a child write recomputes them. + */ + private initializeSummaryFields(object: string, record: any): any { + const descriptors = this.getOwnedSummaryDescriptors(object); + if (descriptors.length === 0) return record; + if (!record || typeof record !== 'object' || Array.isArray(record)) return record; + let out: Record = record; + for (const desc of descriptors) { + const seed = summaryEmptySetValue(desc.fn); + if (seed == null) continue; // min/max/avg — undefined on an empty set + if (out[desc.summaryField] != null) continue; // author supplied a value + if (out === record) out = { ...record }; + out[desc.summaryField] = seed; + } + return out; } /** @@ -4277,7 +4383,10 @@ export class ObjectQL implements IObjectQLEngine { context: execCtx, } as any); let value = rows?.[0]?.value; - if (value == null) value = (desc.fn === 'count' || desc.fn === 'sum') ? 0 : null; + // An aggregate over no rows returns null/undefined on every driver. + // Behaviour unchanged — the empty-set list simply moved to the one + // place the insert-time seed reads it from too (#5749). + if (value == null) value = summaryEmptySetValue(desc.fn); await this.update(desc.parentObject, { id: parentId, [desc.summaryField]: value }, { context: execCtx } as any); }, this.summaryRetryOptions); } catch (err) { @@ -5037,13 +5146,28 @@ export class ObjectQL implements IObjectQLEngine { // (#2703). The hook still has final say — it runs after and may override // any defaulted field. `applyFieldDefaults` returns a fresh copy and only // fills fields left `undefined`, so client-supplied values are untouched. + // + // [#5749] Roll-up `summary` fields this object OWNS are seeded in the same + // pass, right after the declared defaults: `count`/`sum` over the empty + // child collection is 0, and a brand-new parent HAS an empty child + // collection. Without it the row stored `null` and stayed there until some + // child write happened to name it — so "never had a child" (null) and + // "had one, deleted it" (0) read differently and `= 0` filters dropped + // rows. Same placement rules as the defaults above: caller-supplied values + // untouched, hooks run after and may override. const nowSnap = new Date(); const isBatch = Array.isArray(opCtx.data); const defaultedData = isBatch ? (opCtx.data as any[]).map((row) => - this.applyFieldDefaults(object, row as Record, opCtx.context, nowSnap), + this.initializeSummaryFields( + object, + this.applyFieldDefaults(object, row as Record, opCtx.context, nowSnap), + ), ) - : this.applyFieldDefaults(object, opCtx.data as Record, opCtx.context, nowSnap); + : this.initializeSummaryFields( + object, + this.applyFieldDefaults(object, opCtx.data as Record, opCtx.context, nowSnap), + ); // Batch inserts trigger beforeInsert/afterInsert PER ROW, each with the // exact single-record context shape (`input.data` = one row, `result` = diff --git a/packages/objectql/src/summary-rollup.test.ts b/packages/objectql/src/summary-rollup.test.ts index 0e0c4b0542..35fef6b073 100644 --- a/packages/objectql/src/summary-rollup.test.ts +++ b/packages/objectql/src/summary-rollup.test.ts @@ -282,3 +282,169 @@ describe('roll-up summary index — a roll-up registered at RUNTIME still comput expect(parent.completed_task_count).toBe(1); }); }); + +describe('roll-up summary seeding on the PARENT insert (#5749)', () => { + // The bug: `recomputeSummaries` only ever visits parents named by a CHILD + // write (`recs`/`prevs` → `desc.fkField`), so a parent that has never had a + // child is never visited and its summary column keeps insert's `null`. Delete + // the last child and the parent IS visited (via `previous`) and lands on 0 — + // so "never had a child" and "had one, deleted it" are the same logical state + // read back as two different values, and a `= 0` filter silently drops the + // first kind. Seeding at parent-insert time is the producer-side fix: `count` + // and `sum` start at the empty-collection value they will always have. + let engine: ObjectQL; + let storeFor: ReturnType['storeFor']; + + beforeEach(async () => { + engine = new ObjectQL(); + const d = makeDriver(); + storeFor = d.storeFor; + engine.registerDriver(d.driver, true); + await engine.init(); + engine.registry.registerObject({ + name: 'project', + fields: { + name: { type: 'text' }, + task_count: { type: 'summary', summaryOperations: { object: 'task', field: 'id', function: 'count' } }, + total_estimate: { type: 'summary', summaryOperations: { object: 'task', field: 'estimate', function: 'sum' } }, + // No empty-set value — these must stay `null`, same list the recompute + // fallback uses. + avg_estimate: { type: 'summary', summaryOperations: { object: 'task', field: 'estimate', function: 'avg' } }, + max_estimate: { type: 'summary', summaryOperations: { object: 'task', field: 'estimate', function: 'max' } }, + }, + } as any); + engine.registry.registerObject({ + name: 'task', + fields: { + title: { type: 'text' }, + estimate: { type: 'number' }, + project: { type: 'master_detail', reference: 'project' }, + }, + } as any); + }); + + const row = (id: string) => storeFor('project').get(id); + + it('reads the SAME value for "never had a child" (A) and "had one, deleted it" (C)', async () => { + // A — created, never had a child. The issue measured `null` here. + const a = await engine.insert('project', { name: 'Legacy Sunset' }); + expect(a.task_count).toBe(0); // the record handed back to the caller + expect(row(a.id).task_count).toBe(0); // and what was actually stored + expect(row(a.id).total_estimate).toBe(0); + + // B — one child. + const b = await engine.insert('project', { name: 'Apollo' }); + const t = await engine.insert('task', { title: 't1', estimate: 8, project: b.id }); + expect(row(b.id).task_count).toBe(1); + expect(row(b.id).total_estimate).toBe(8); + + // C — that child deleted again. + await engine.delete('task', { where: { id: t.id } }); + expect(row(b.id).task_count).toBe(0); + expect(row(b.id).total_estimate).toBe(0); + + // The point of the whole change: A and C are one logical state, one value. + expect(row(a.id).task_count).toBe(row(b.id).task_count); + expect(row(a.id).total_estimate).toBe(row(b.id).total_estimate); + }); + + it('`= 0` and `< 1` filters no longer drop the parent that never had a child', async () => { + // Exactly the showcase repro: `Legacy Sunset` (never had a task) used to be + // missing from both result sets while `ROLLUP PROBE` (had one, deleted it) + // was returned — the same query, two answers for one state, and the miss is + // a silently absent ROW, not an error. + const never = await engine.insert('project', { name: 'Legacy Sunset' }); + const probe = await engine.insert('project', { name: 'ROLLUP PROBE' }); + const seeded = await engine.insert('task', { title: 'seed', estimate: 3, project: probe.id }); + await engine.delete('task', { where: { id: seeded.id } }); + const busy = await engine.insert('project', { name: 'Apollo' }); + await engine.insert('task', { title: 'live', estimate: 5, project: busy.id }); + + const eqZero = await engine.find('project', { where: [['task_count', '=', 0]] }); + expect(eqZero.map((r: any) => r.name).sort()).toEqual(['Legacy Sunset', 'ROLLUP PROBE']); + expect(eqZero.map((r: any) => r.id)).toContain(never.id); + + const ltOne = await engine.find('project', { where: [['task_count', '<', 1]] }); + expect(ltOne.map((r: any) => r.name).sort()).toEqual(['Legacy Sunset', 'ROLLUP PROBE']); + + // And the parent that DOES have a task is still excluded by both. + expect(eqZero.map((r: any) => r.id)).not.toContain(busy.id); + expect(ltOne.map((r: any) => r.id)).not.toContain(busy.id); + }); + + it('leaves avg/max null — undefined on an empty set, before AND after children', async () => { + const p = await engine.insert('project', { name: 'No tasks yet' }); + expect(row(p.id).avg_estimate ?? null).toBeNull(); + expect(row(p.id).max_estimate ?? null).toBeNull(); + + const t = await engine.insert('task', { title: 't', estimate: 6, project: p.id }); + expect(row(p.id).avg_estimate).toBe(6); + expect(row(p.id).max_estimate).toBe(6); + + // Back to the empty collection: the recompute fallback puts them back to + // null. Seeding reads the same list, so A and C agree here too. + await engine.delete('task', { where: { id: t.id } }); + expect(row(p.id).avg_estimate ?? null).toBeNull(); + expect(row(p.id).max_estimate ?? null).toBeNull(); + }); + + it('never overwrites a value the author supplied on insert', async () => { + const p = await engine.insert('project', { name: 'Imported', task_count: 7, total_estimate: 42 }); + expect(row(p.id).task_count).toBe(7); + expect(row(p.id).total_estimate).toBe(42); + }); + + it('seeds every row of a batch insert, and only the unsupplied ones', async () => { + const written = await engine.insert('project', [{ name: 'P1' }, { name: 'P2', task_count: 3 }]); + expect(row(written[0].id).task_count).toBe(0); + expect(row(written[0].id).total_estimate).toBe(0); + expect(row(written[1].id).task_count).toBe(3); + }); + + it('lets a beforeInsert hook still have the final say', async () => { + engine.registerHook('beforeInsert', async (ctx: any) => { + ctx.input.data.task_count = 99; + }, { object: 'project' }); + const p = await engine.insert('project', { name: 'Hooked' }); + expect(row(p.id).task_count).toBe(99); + }); + + it('does NOT seed a roll-up whose relationship cannot be resolved', async () => { + // Seeded ⇔ maintained: `buildSummaryIndex` skips a descriptor whose child→ + // parent FK it cannot resolve, so the recompute would never maintain this + // field. A 0 nothing ever updates would be a worse lie than the null. + engine.registry.registerObject({ + name: 'orphan_parent', + fields: { + name: { type: 'text' }, + ghost_count: { type: 'summary', summaryOperations: { object: 'ghost', field: 'id', function: 'count' } }, + }, + } as any); + engine.registry.registerObject({ name: 'ghost', fields: { label: { type: 'text' } } } as any); + + const p = await engine.insert('orphan_parent', { name: 'x' }); + expect(storeFor('orphan_parent').get(p.id).ghost_count ?? null).toBeNull(); + }); + + it('seeds a parent published AFTER the summary index was already warmed', async () => { + // The parent-side index must share the child-side staleness rule (cloud#970): + // a runtime publish registers straight into the registry, so an index warmed + // by an earlier write must still see the new roll-up. + await engine.insert('project', { name: 'warms the index' }); + + engine.registry.registerObject({ + name: 'sprint', + fields: { + name: { type: 'text' }, + story_count: { type: 'summary', summaryOperations: { object: 'story', field: 'id', function: 'count' } }, + }, + } as any); + engine.registry.registerObject({ + name: 'story', + fields: { title: { type: 'text' }, sprint: { type: 'master_detail', reference: 'sprint' } }, + } as any); + + const s = await engine.insert('sprint', { name: 'S1' }); + expect(storeFor('sprint').get(s.id).story_count).toBe(0); + }); +});