From 6cee3d99a97b7c35e3fea6248797190df6db276b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 03:28:53 +0000 Subject: [PATCH 1/3] feat(objectql,cli): os migrate summary-nulls backfills pre-seed NULL count/sum roll-ups (#6063) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #6013 (#5749) seeds a roll-up's empty-set value at parent INSERT, which reaches new rows only: a database upgraded in place keeps pre-upgrade parents at NULL, because the recompute that would fix them runs only when one of their children is written. Those rows keep vanishing from `= 0` filters, sorts, GROUP BY and formulas. This adds the one-off, explicit data migration for them. - `backfillSummaryNulls` (packages/objectql/src/summary-backfill.ts): walk each object owning a count/sum roll-up, and recompute every row whose column is stored NULL. A pre-upgrade parent WITH children is NULL too and its correct value is the real aggregate, so `SET col = 0 WHERE col IS NULL` is wrong, not merely coarse. Dry run by default; idempotent; driver-agnostic (values are read and tested in JS, no null predicate pushed down); one row's failure is recorded and the run continues. - min/max/avg are never touched: undefined on an empty set, so a stored null there is the correct reading of "no child rows". The report names them as deliberately skipped. - `summary-aggregate.ts`: SummaryDescriptor, summaryEmptySetValue and the single-descriptor aggregate lifted out of engine.ts unchanged, so the seed, the recompute and the backfill share ONE computation instead of three that agree until one is edited. The descriptor gains `childObject` so the parent-side index is usable on its own. - `os migrate summary-nulls`: thin oclif shell over the migration, following the files-to-references precedent (occupancy gate, --apply/--yes, --object, --max-records, --json). No deployment flag — nothing is gated on this run. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --- .changeset/summary-null-backfill-migration.md | 51 +++ content/docs/deployment/cli.mdx | 38 ++ .../cli/src/commands/migrate/summary-nulls.ts | 242 ++++++++++++ packages/objectql/src/engine.ts | 80 ++-- packages/objectql/src/index.ts | 19 + packages/objectql/src/summary-aggregate.ts | 122 ++++++ .../objectql/src/summary-backfill.test.ts | 321 +++++++++++++++ packages/objectql/src/summary-backfill.ts | 372 ++++++++++++++++++ scripts/query-options-erasure-baseline.json | 3 +- 9 files changed, 1193 insertions(+), 55 deletions(-) create mode 100644 .changeset/summary-null-backfill-migration.md create mode 100644 packages/cli/src/commands/migrate/summary-nulls.ts create mode 100644 packages/objectql/src/summary-aggregate.ts create mode 100644 packages/objectql/src/summary-backfill.test.ts create mode 100644 packages/objectql/src/summary-backfill.ts diff --git a/.changeset/summary-null-backfill-migration.md b/.changeset/summary-null-backfill-migration.md new file mode 100644 index 0000000000..98aad7d1ff --- /dev/null +++ b/.changeset/summary-null-backfill-migration.md @@ -0,0 +1,51 @@ +--- +"@objectstack/objectql": patch +"@objectstack/cli": patch +--- + +feat(objectql,cli): `os migrate summary-nulls` backfills roll-up count/sum columns left NULL by pre-seed inserts (#6063) + +#5749 / PR #6013 fixed the **producer**: a parent row created from that release +on has its `count` / `sum` roll-up columns seeded to the empty-set value at +insert, so `filter ["task_count", "=", 0]`, sorting, `GROUP BY` and formulas +over the column stop silently dropping parents that never had a child. + +Being a create-time fix, it reaches **new rows only**. A database upgraded **in +place** still holds parents stored before the upgrade, and the recompute that +would otherwise correct them runs only when one of their **children** is +written — so those rows keep their `NULL` indefinitely and keep disappearing +from the same queries. A freshly seeded deployment is correct; an upgraded one +is not. This release ships the other half: a one-off, explicit data migration. + +```bash +os migrate summary-nulls # dry run: full report, writes nothing +os migrate summary-nulls --apply # recompute and write (prompts) +os migrate summary-nulls --apply --yes --json # CI / scripts +os migrate summary-nulls --object project # restrict to one object (repeatable) +``` + +**Every NULL row is recomputed, never blanket-set to 0.** A pre-upgrade parent +that *does* have children is `NULL` too — nothing ever recomputed it — and its +correct value is the real aggregate. `UPDATE ... SET col = 0 WHERE col IS NULL` +would replace a visibly-missing value with a confidently-wrong one, which the +next child write would then silently change back. The run computes each value +through the same code path the engine's own child-write recompute uses +(`aggregateSummaryValue`), over the descriptors the engine itself maintains, so +a backfilled column and a recomputed one can never mean different things. + +**`min` / `max` / `avg` are never touched.** They are undefined on an empty set +— which is why the insert-time seed leaves them `null` — so a stored `null` +there is the correct reading of "no child rows", not a defect. The report names +them as deliberately skipped rather than omitting them silently. + +Other properties: dry run by default and a dry run writes nothing at all; +idempotent, so re-running is safe and a clean report is the operator's own +verification; driver-agnostic (it reads values and tests them in JS rather than +pushing a null predicate down, since null-predicate compilation is precisely +where drivers diverge); one row's failure is recorded and the run carries on. +It records no deployment flag — unlike its `os migrate` siblings, nothing is +gated on it having run. + +Never running it is safe in the sense that nothing breaks *further*: the +affected rows simply stay missing from `= 0` filters until a child of theirs is +written. diff --git a/content/docs/deployment/cli.mdx b/content/docs/deployment/cli.mdx index ff226f20b1..9100dc0ce6 100644 --- a/content/docs/deployment/cli.mdx +++ b/content/docs/deployment/cli.mdx @@ -659,6 +659,7 @@ where the data lives. |---------|-------------| | `os migrate files-to-references` | Convert legacy file-field values to `sys_file` references, verify the ownership ledger, and record the deployment's migration flag | | `os migrate value-shapes` | Scan stored reference and structured-JSON field values against the platform's value contract, and record the deployment's migration flag when clean | +| `os migrate summary-nulls` | Backfill roll-up `count` / `sum` columns still stored as `NULL` on parent rows created before the insert-time seed. Repairs values; no flag, nothing depends on it having run | | `os migrate meta --stored` | Replay the metadata conversion chain over this deployment's `sys_metadata` rows and rewrite the ones still carrying a pre-protocol shape. Hygiene, not a gate — nothing depends on it having run | ```bash @@ -755,6 +756,43 @@ Same writing rules as its sibling: a dry run writes **nothing**, `--apply` is the only writing mode, a later failing run clears the verified state, and a running server reads the flag once — **restart it** after a successful apply. +#### `os migrate summary-nulls` + +A roll-up `summary` field of function `count` or `sum` is **0** over an empty +child collection — zero children is zero, not "unknown" — and since #5749 a +parent row is created holding that value. Rows created *before* that are the +exception: nothing seeded them, and the recompute that maintains a roll-up runs +only when one of the parent's **children** is written, so a parent that has +never had a child keeps its `NULL` indefinitely. `filter ["task_count", "=", 0]` +then silently omits it, and so do sorting, `GROUP BY` and any formula reading +the column (null propagation). + +```bash +os migrate summary-nulls # Dry run: full report, writes nothing +os migrate summary-nulls --apply # Recompute and write (prompts) +os migrate summary-nulls --apply --yes --json # CI / scripts +os migrate summary-nulls --object project # Restrict to one object (repeatable) +``` + +**Each affected row is recomputed, not set to 0.** A pre-upgrade parent that +*does* have children is `NULL` too, and its correct value is the aggregate over +them — writing 0 there would replace a missing value with a wrong one, and the +next child write would change it back. The report separates the two: `N NULL +row(s), M with real child data`. + +`min` / `max` / `avg` are **never touched**. They are undefined on an empty set, +so a `null` there is the correct reading of "no child rows"; the report lists +them as deliberately skipped. + +Idempotent — every write turns a `NULL` into a number, so a second run finds +nothing and writes nothing. Re-running until the report says zero *is* the +verification, which is why this command records no flag: it repairs values and +changes no behaviour, so there is no posture for a flag to attest. + +A deployment whose database was seeded fresh on this version has nothing to do +here: its parents were created with the value already in place, and the run +reports zero. + #### A database created by this version needs no migration A deployment whose database the platform **creates from empty** records these diff --git a/packages/cli/src/commands/migrate/summary-nulls.ts b/packages/cli/src/commands/migrate/summary-nulls.ts new file mode 100644 index 0000000000..1f772d1adc --- /dev/null +++ b/packages/cli/src/commands/migrate/summary-nulls.ts @@ -0,0 +1,242 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { Command, Flags } from '@oclif/core'; +import chalk from 'chalk'; +import { createInterface } from 'node:readline'; +import { + printHeader, + printSuccess, + printWarning, + printError, + printInfo, + printStep, + createTimer, + emitJson, + isExitSignal, +} from '../../utils/format.js'; +import { bootSchemaStack } from '../../utils/schema-migrate.js'; +import { OCCUPANCY_HINT, probeMigrationTarget } from '../../utils/migrate-occupancy-gate.js'; +import { describeOccupancy } from '../../utils/sqlite-occupancy.js'; +import { buildDataMigrationPlugins } from '../../utils/data-migration-plugins.js'; + +async function confirm(question: string): Promise { + if (!process.stdin.isTTY) return false; // non-interactive → require --yes + const rl = createInterface({ input: process.stdin, output: process.stdout }); + try { + const answer: string = await new Promise((resolve) => rl.question(question, resolve)); + return /^y(es)?$/i.test(answer.trim()); + } finally { + rl.close(); + } +} + +/** + * `os migrate summary-nulls` — the one-off backfill of roll-up `count`/`sum` + * columns left `NULL` by inserts predating PR #6013 (#6063, the second half of + * #5749). + * + * PR #6013 fixed the producer: a parent created from that release on starts its + * `count`/`sum` roll-ups at 0. It cannot reach rows that already exist, and the + * recompute only ever visits a parent when one of its CHILDREN is written — so + * a database upgraded IN PLACE keeps pre-upgrade parents at `NULL`, and every + * `= 0` filter, sort, GROUP BY and formula over the column silently drops them. + * A freshly seeded database has no such rows; this command is for the other + * kind, and is needed exactly once per deployment. + * + * Dry run by default (writes nothing at all), `--apply` to backfill. Each + * `NULL` row is RECOMPUTED — a pre-upgrade parent that has children is `NULL` + * too and its correct value is the real aggregate, so writing 0 everywhere + * would swap a missing value for a wrong one. Idempotent: a second run finds + * nothing left to do. + * + * `min`/`max`/`avg` are never touched — undefined on an empty set, so a `null` + * there is the correct reading of "no child rows", not a defect. + * + * ## No deployment flag, deliberately + * + * Its siblings (`files-to-references`, `value-shapes`) record a `sys_migration` + * flag because that flag is what later OPENS irreversible behaviour on the + * deployment. Nothing is gated on this run: it repairs values and changes no + * posture, and its own idempotence is the verification (re-run; a clean report + * is the evidence). A flag here would be a fact nothing reads. + */ +export default class MigrateSummaryNulls extends Command { + static override description = + 'Backfill roll-up count/sum summary columns still stored as NULL on parent rows created before the ' + + 'insert-time seed (#5749). Dry-run by default; --apply recomputes and writes each affected row.'; + + static override examples = [ + '$ os migrate summary-nulls', + '$ os migrate summary-nulls --apply', + '$ os migrate summary-nulls --apply --yes --json', + '$ os migrate summary-nulls --object project', + ]; + + static override flags = { + 'database-url': Flags.string({ + description: 'Database URL to migrate (defaults to $OS_DATABASE_URL / the project DB)', + env: 'OS_DATABASE_URL', + }), + apply: Flags.boolean({ + description: 'Write the recomputed values (default is a read-only dry run)', + default: false, + }), + yes: Flags.boolean({ char: 'y', description: 'Skip the --apply confirmation prompt', default: false }), + force: Flags.boolean({ + description: 'Apply even when another process is using the database (SQLite occupancy check)', + default: false, + }), + object: Flags.string({ + description: 'Restrict to this object (repeatable; default: every object owning a count/sum roll-up)', + multiple: true, + }), + 'max-records': Flags.integer({ + description: 'Safety bound on parent rows read per object — exceeding it truncates the walk', + }), + json: Flags.boolean({ description: 'Output as JSON (implies non-interactive; requires --yes to apply)' }), + }; + + async run(): Promise { + const { flags } = await this.parse(MigrateSummaryNulls); + const timer = createTimer(); + const apply = flags.apply; + + if (!flags.json) printHeader('Migrate · summary-nulls'); + + // Occupancy gate, like `files-to-references`: an apply run rewrites ROWS, + // so a second writer on the same SQLite file is a real hazard. Probed + // before boot (afterwards our own pool is what the probe finds) and before + // the prompt, so an operator is never asked to confirm a run we refuse. + const occupancy = await probeMigrationTarget(flags['database-url']); + if (occupancy.status === 'busy' && apply && !flags.force) { + if (flags.json) { + await emitJson({ + error: 'database_busy', + database: occupancy.filename, + signal: occupancy.signal, + detail: occupancy.detail, + hint: OCCUPANCY_HINT, + }, 0, { compact: true }); + this.exit(1); + return; + } + printError(describeOccupancy(occupancy)); + printWarning(OCCUPANCY_HINT); + this.exit(1); + return; + } + if (occupancy.status === 'busy' && !flags.json) { + printWarning(apply + ? `--force: ${describeOccupancy(occupancy)} Backfilling anyway — the live process may write rows mid-walk.` + : `${describeOccupancy(occupancy)} The dry run below writes nothing, but its counts may shift while that process is running.`); + } + if (occupancy.status === 'unknown' && !flags.json) { + printWarning(`Could not check whether the database is in use — ${occupancy.detail}`); + } + + if (apply && !flags.yes) { + if (flags.json || !process.stdin.isTTY) { + if (flags.json) { + await emitJson({ error: 'confirmation_required', hint: 'pass --yes' }, 0, { compact: true }); + this.exit(1); + return; + } + printWarning('Apply mode rewrites record data. Re-run with --yes to confirm, or run without --apply to preview.'); + this.exit(1); + return; + } + const ok = await confirm( + chalk.bold('\nRecompute and write every NULL count/sum roll-up value on this database? [y/N] '), + ); + if (!ok) { + printInfo('Aborted — no changes made.'); + return; + } + } + + if (!flags.json) { + printStep(apply ? 'Booting data stack (APPLY mode)…' : 'Booting data stack (dry run)…'); + } + + let stack; + try { + stack = await bootSchemaStack({ + databaseUrl: flags['database-url'], + extraPlugins: await buildDataMigrationPlugins(), + }); + } catch (error: any) { + if (flags.json) { await emitJson({ error: error.message }, 0, { compact: true }); this.exit(1); } + printError(error.message || String(error)); + this.exit(1); + return; + } + + try { + const engine: any = stack.kernel.getService('objectql'); + if (typeof engine?.getOwnedSummaryDescriptors !== 'function') { + throw new Error('No ObjectQL engine on this stack — cannot read the roll-up index.'); + } + // An empty walk is indistinguishable from a clean one, and "clean" is the + // answer an operator will act on — so refuse to run when no app metadata + // is loaded (missing artifact / wrong directory) rather than report a + // database the walk never looked at. + const loadedObjects: string[] = + typeof engine.getConfigs === 'function' ? Object.keys(engine.getConfigs()) : []; + if (!loadedObjects.some((name) => !name.startsWith('sys_'))) { + throw new Error( + 'No app objects are loaded, so the walk would examine nothing. ' + + 'Run "os build" in your project root first (the migration reads dist/objectstack.json), then re-run.', + ); + } + + const { backfillSummaryNulls, formatSummaryBackfillReport } = await import('@objectstack/objectql'); + + // In JSON mode keep stdout parseable — route warnings to stderr. + const logger = flags.json + ? { info: (m: string) => console.error(m), warn: (m: string) => console.error(m) } + : { info: (m: string) => printInfo(m), warn: (m: string) => printWarning(m) }; + + const report = await backfillSummaryNulls(engine, logger, { + apply, + objects: flags.object, + maxRecordsPerObject: flags['max-records'], + }); + + if (flags.json) { + await emitJson({ database: stack.dbLabel, apply, report, duration: timer.elapsed() }); + if (report.failures.length > 0) this.exit(1); + return; + } + + printInfo(`Database: ${chalk.white(stack.dbLabel)}`); + console.log(''); + console.log(formatSummaryBackfillReport(report).join('\n')); + console.log(''); + + if (report.failures.length > 0) { + printError(`${report.failures.length} row(s) could not be recomputed — re-run to finish them.`); + } else if (apply && report.filled > 0) { + printSuccess( + `Backfilled ${report.filled} roll-up value(s) across ${report.fields.length} column(s). ` + + 'Re-run any time — it only ever revisits rows still holding NULL.', + ); + } else if (apply) { + printSuccess('Nothing to backfill — every count/sum roll-up already holds a value.'); + } else if (report.nullRows > 0) { + printInfo(`Dry run only — ${report.nullRows} value(s) would be recomputed. Re-run with --apply.`); + } else { + printSuccess('Nothing to backfill — every count/sum roll-up already holds a value.'); + } + console.log(chalk.dim(` ${timer.display()}`)); + console.log(''); + if (report.failures.length > 0) this.exit(1); + } catch (error: any) { + if (isExitSignal(error)) throw error; + if (flags.json) { await emitJson({ error: error.message }, 0, { compact: true }); this.exit(1); } + printError(error.message || String(error)); + this.exit(1); + } finally { + await stack.shutdown(); + } + } +} diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 0d234a2369..e1b750f5b9 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -41,6 +41,11 @@ import { resolveFilterTokens, } from '@objectstack/core'; import { SummaryRecomputeError, type SummaryRecomputeFailure } from './summary-errors.js'; +import { + aggregateSummaryValue, + summaryEmptySetValue, + type SummaryDescriptor, +} from './summary-aggregate.js'; import { ReadonlyFieldRejectedError } from './readonly-strict-errors.js'; import { DriverConnectError, @@ -787,39 +792,12 @@ function resolveMetadataItemName(key: string, item: any): string | undefined { * - CoreServiceName.data (CRUD) * - CoreServiceName.metadata (Schema Registry) */ -/** A roll-up `summary` field on a parent object that aggregates a child. */ -interface SummaryDescriptor { - parentObject: string; - summaryField: string; - /** FK field on the child pointing back to the parent. */ - fkField: string; - fn: 'count' | 'sum' | 'min' | 'max' | 'avg'; - /** Child field aggregated (unused for count). */ - sourceField: string; - /** - * Optional predicate (a query `where` FilterCondition) restricting which child - * rows are aggregated. ANDed with the parent-FK match when the aggregate runs. - * Undefined ⇒ aggregate every child of the parent. - */ - 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; -} +// [#6063] `SummaryDescriptor`, `summaryEmptySetValue` and the single-descriptor +// aggregate moved to `./summary-aggregate.js` — unchanged, and still the one +// place each is written down. The move exists so the one-off backfill of +// pre-#6013 `NULL` rows computes its value through the SAME code this engine +// does, instead of a second implementation that agrees only until one of them +// is edited. // `implements IObjectQLEngine` is the verification step of #4251 B3: every // member the `objectql` slot's contract declares is checked against this class @@ -4239,7 +4217,7 @@ export class ObjectQL implements IObjectQLEngine { ? so.filter as Record : undefined; const descriptor: SummaryDescriptor = { - parentObject: parent.name, summaryField, fkField, fn, sourceField: so.field, filter, + parentObject: parent.name, summaryField, childObject, fkField, fn, sourceField: so.field, filter, }; const list = index.get(childObject) ?? []; list.push(descriptor); @@ -4294,8 +4272,14 @@ export class ObjectQL implements IObjectQLEngine { } /** 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[] { + * i.e. the ones a NEW row of `parentObject` must have seeded. + * + * Public since #6063: the one-off backfill of pre-#6013 `NULL` rows + * (`os migrate summary-nulls`) iterates parents, and reading the engine's + * OWN index is what makes it see exactly the roll-ups the engine maintains — + * same FK resolution, same filter, same staleness rule. Re-deriving them + * would be a second index that disagrees the first time either moves. */ + getOwnedSummaryDescriptors(parentObject: string): SummaryDescriptor[] { this.ensureSummaryIndexes(); return this.summaryIndexByParent!.get(parentObject) ?? []; } @@ -4369,24 +4353,12 @@ export class ObjectQL implements IObjectQLEngine { // aggregate/update) with backoff — a network blip here used to leave // the parent summary silently stale (framework#3147). await withTransientRetry(async () => { - // AND the parent-FK match with the optional per-summary filter so - // only matching child rows are aggregated (e.g. received receipts). - const fkMatch = { [desc.fkField]: parentId }; - const where = desc.filter ? { $and: [fkMatch, desc.filter] } : fkMatch; - const rows = await this.aggregate(childObject, { - where, - aggregations: [{ - function: desc.fn, - ...(desc.fn === 'count' ? {} : { field: desc.sourceField }), - alias: 'value', - }], - context: execCtx, - } as any); - let value = rows?.[0]?.value; - // 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); + // The aggregate — parent-FK match ANDed with the optional + // per-summary filter, empty-set fallback included — is + // `aggregateSummaryValue` (#6063). Behaviour unchanged; it simply + // lives where the insert-time seed and the one-off NULL backfill + // can read the identical computation instead of copying it. + const value = await aggregateSummaryValue(this, desc, parentId, execCtx); await this.update(desc.parentObject, { id: parentId, [desc.summaryField]: value }, { context: execCtx } as any); }, this.summaryRetryOptions); } catch (err) { diff --git a/packages/objectql/src/index.ts b/packages/objectql/src/index.ts index cf8dc5fed9..6fce791553 100644 --- a/packages/objectql/src/index.ts +++ b/packages/objectql/src/index.ts @@ -144,6 +144,25 @@ export type { ValueShapeScanOptions, ValueShapeScanLogger, } from './validation/scan-value-shapes.js'; +// [#6063 / #5749] The one-off backfill behind `os migrate summary-nulls`, and +// the roll-up core it shares with the engine. Same package as the roll-up it +// repairs (the owning-package half of the migration-family shape: the CLI holds +// only the command shell). +export { + backfillSummaryNulls, + formatSummaryBackfillReport, + summaryBackfillComplete, +} from './summary-backfill.js'; +export type { + SummaryBackfillReport, + SummaryBackfillFieldOutcome, + SummaryBackfillFailure, + SummaryBackfillEngine, + SummaryBackfillLogger, + SummaryBackfillOptions, +} from './summary-backfill.js'; +export { summaryEmptySetValue, summaryNullIsBackfillable, aggregateSummaryValue } from './summary-aggregate.js'; +export type { SummaryDescriptor, SummaryAggregateEngine } from './summary-aggregate.js'; export { evaluateValidationRules, needsPriorRecord, legalNextStates } from './validation/rule-validator.js'; export type { EvaluateRulesOptions } from './validation/rule-validator.js'; export { diff --git a/packages/objectql/src/summary-aggregate.ts b/packages/objectql/src/summary-aggregate.ts new file mode 100644 index 0000000000..5055a9ddfd --- /dev/null +++ b/packages/objectql/src/summary-aggregate.ts @@ -0,0 +1,122 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The roll-up `summary` core: what a roll-up IS (the descriptor), what it reads + * over an empty child collection, and how ONE descriptor's value is computed + * for ONE parent. + * + * Lifted out of `engine.ts` verbatim by #6063 for a single reason: the one-off + * backfill of pre-#6013 `NULL` rows (`os migrate summary-nulls`) must write the + * value the engine itself would write — not a second implementation that agrees + * today. Three call sites now share this file: + * + * 1. {@link ObjectQL.initializeSummaryFields} — the insert-time seed (#5749 / + * PR #6013): a brand-new parent starts at {@link summaryEmptySetValue}. + * 2. `ObjectQL.recomputeSummaries` — the child-write recompute: aggregate, + * then the same empty-set fallback. + * 3. `backfillSummaryNulls` (`./summary-backfill.js`, #6063) — the existing + * rows the insert-time seed structurally cannot reach. + * + * The same reasoning `scan-value-shapes.ts` gives for importing the write-path + * predicate applies here: a second "what does this roll-up equal" drifting by + * one clause would let a migration write values the engine then disagrees with + * on the next child write — a column that changes under a user who changed + * nothing. + */ + +/** A roll-up `summary` field on a parent object that aggregates a child. */ +export interface SummaryDescriptor { + parentObject: string; + summaryField: string; + /** + * The child object aggregated — `summaryOperations.object`. + * + * [#6063] Present on the descriptor itself so the PARENT-side view is usable + * on its own. The child-side index is keyed by this name, so the recompute + * path never needed it as a field; the backfill iterates parents and does. + */ + childObject: string; + /** FK field on the child pointing back to the parent. */ + fkField: string; + fn: 'count' | 'sum' | 'min' | 'max' | 'avg'; + /** Child field aggregated (unused for count). */ + sourceField: string; + /** + * Optional predicate (a query `where` FilterCondition) restricting which child + * rows are aggregated. ANDed with the parent-FK match when the aggregate runs. + * Undefined ⇒ aggregate every child of the parent. + */ + 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 aggregateSummaryValue} uses it for + * the post-aggregate fallback (an aggregate over no rows returns + * `null`/`undefined` on every driver), `ObjectQL.initializeSummaryFields` uses + * it to seed a brand-new parent row with the same value the first recompute + * would have produced, and `backfillSummaryNulls` (#6063) reads it to decide + * which columns a stored `NULL` is even a defect in. Three 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. + */ +export function summaryEmptySetValue(fn: SummaryDescriptor['fn']): number | null { + return fn === 'count' || fn === 'sum' ? 0 : null; +} + +/** + * Does a stored `null` in this roll-up's column mean "never computed"? + * + * Only for the functions that HAVE an empty-set value: a `null` `count`/`sum` + * is a hole (the correct value is 0 or the real aggregate), while a `null` + * `min`/`max`/`avg` is the legitimate reading of "no child rows" and must be + * left exactly as it is. #6063's scope narrowing is this predicate, and it is + * derived from {@link summaryEmptySetValue} rather than re-listing the + * functions, so the two can never disagree. + */ +export function summaryNullIsBackfillable(fn: SummaryDescriptor['fn']): boolean { + return summaryEmptySetValue(fn) !== null; +} + +/** + * The engine surface one roll-up aggregate needs — deliberately the single + * verb, so nothing here can write. + */ +export interface SummaryAggregateEngine { + aggregate(object: string, query: any, options?: any): Promise; +} + +/** + * Compute ONE roll-up descriptor's value for ONE parent id. + * + * The parent-FK match is ANDed with the descriptor's optional predicate so only + * matching child rows are aggregated (e.g. sum receipts where + * `{ status: 'received' }`), and an aggregate over no rows — `null`/`undefined` + * on every driver — falls back to {@link summaryEmptySetValue}. + * + * Driver-agnostic by construction: it speaks the engine's aggregate contract, + * never SQL, so a deployment on any driver gets the same value (#6063 ruling 3). + */ +export async function aggregateSummaryValue( + engine: SummaryAggregateEngine, + desc: SummaryDescriptor, + parentId: string, + execCtx?: unknown, +): Promise { + const fkMatch = { [desc.fkField]: parentId }; + const where = desc.filter ? { $and: [fkMatch, desc.filter] } : fkMatch; + const rows = await engine.aggregate(desc.childObject, { + where, + aggregations: [{ + function: desc.fn, + ...(desc.fn === 'count' ? {} : { field: desc.sourceField }), + alias: 'value', + }], + context: execCtx, + } as any); + const value = rows?.[0]?.value; + return value == null ? summaryEmptySetValue(desc.fn) : value; +} diff --git a/packages/objectql/src/summary-backfill.test.ts b/packages/objectql/src/summary-backfill.test.ts new file mode 100644 index 0000000000..150919aa62 --- /dev/null +++ b/packages/objectql/src/summary-backfill.test.ts @@ -0,0 +1,321 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// `os migrate summary-nulls` — the one-off backfill of roll-up `count`/`sum` +// columns left `NULL` by inserts predating PR #6013 (#6063, second half of +// #5749). +// +// Every fixture here builds the state the migration exists for: rows written +// STRAIGHT INTO THE DRIVER STORE, bypassing the engine's insert path, which is +// exactly what a database upgraded in place holds — parents stored before the +// insert-time seed existed, never revisited because no child of theirs was +// ever written. Inserting them through the engine would seed them to 0 and +// there would be nothing left to test. + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { backfillSummaryNulls, formatSummaryBackfillReport } from './summary-backfill.js'; + +function makeDriver() { + const stores = new Map>(); + const writes: Array<{ object: string; id: string; data: Record }> = []; + const storeFor = (o: string) => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + // Minimal FilterCondition matcher — implicit equality, the comparison + // operators the keyset walk emits (`$gt` on `id`), and the `$and`/`$or`/`$not` + // the engine emits when a summary carries a filter. + const checkOp = (value: any, cond: any): boolean => { + if (cond === null || typeof cond !== 'object' || Array.isArray(cond) || cond instanceof Date) { + return value === cond; + } + return Object.entries(cond).every(([op, target]: [string, any]) => { + switch (op) { + case '$eq': return value === target; + case '$ne': return value !== target; + case '$gt': return value > target; + case '$gte': return value >= target; + case '$lt': return value < target; + case '$lte': return value <= target; + case '$in': return Array.isArray(target) && target.includes(value); + case '$nin': return Array.isArray(target) && !target.includes(value); + default: return true; + } + }); + }; + const matches = (row: any, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + return Object.entries(where).every(([k, v]: [string, any]) => { + if (k === '$and') return (v as any[]).every((w) => matches(row, w)); + if (k === '$or') return (v as any[]).some((w) => matches(row, w)); + if (k === '$not') return !matches(row, v); + return checkOp(row?.[k], v); + }); + }; + let n = 0; + /** Set by a test to make ONE parent's update fail, pinning failure isolation. */ + let failUpdateFor: string | null = null; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find(object: string, ast: any) { + const rows = Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where)); + rows.sort((a, b) => String(a.id).localeCompare(String(b.id))); + return typeof ast?.limit === 'number' ? rows.slice(0, ast.limit) : rows; + }, + async findOne(object: string, ast: any) { + for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + n += 1; + const id = (data.id as string) ?? `r_${n}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + if (failUpdateFor && id === failUpdateFor) throw new Error('driver refused this row'); + writes.push({ object, id, data }); + const s = storeFor(object); + const row = { ...s.get(id), ...data, id }; + s.set(id, row); + return row; + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count() { return 0; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r, undefined))); + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { + driver, + storeFor, + writes, + failUpdateFor: (id: string | null) => { failUpdateFor = id; }, + }; +} + +const quietLogger = { info: () => {}, warn: () => {} }; + +describe('backfillSummaryNulls — pre-#6013 NULL roll-ups (#6063)', () => { + let engine: ObjectQL; + let d: ReturnType; + + /** A parent row as an IN-PLACE UPGRADED database holds it: no summary values + * at all, because the insert that wrote it predates the seed. */ + const legacyParent = (id: string, extra: Record = {}) => { + d.storeFor('project').set(id, { id, name: id, ...extra }); + }; + /** A child row written the same way — so no recompute ever ran for it. */ + const legacyTask = (id: string, project: string, extra: Record = {}) => { + d.storeFor('task').set(id, { id, title: id, project, ...extra }); + }; + const project = (id: string) => d.storeFor('project').get(id); + + beforeEach(async () => { + engine = new ObjectQL(); + d = makeDriver(); + 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' } }, + done_count: { + type: 'summary', + summaryOperations: { object: 'task', field: 'id', function: 'count', filter: { status: 'done' } }, + }, + // No empty-set value — a stored null here means "no child rows" and is + // NOT a defect. Deliberately out of this migration's scope. + 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' }, + status: { type: 'text' }, + estimate: { type: 'number' }, + project: { type: 'master_detail', reference: 'project' }, + }, + } as any); + }); + + it('gives a NULL parent WITH children its real aggregate — not 0', async () => { + // The case the cheap `UPDATE … SET col = 0 WHERE col IS NULL` answers + // wrongly: nothing ever recomputed this parent, so it is NULL, but it has + // children and its correct value is the aggregate over them. + legacyParent('p_busy'); + legacyTask('t1', 'p_busy', { estimate: 10, status: 'done' }); + legacyTask('t2', 'p_busy', { estimate: 32, status: 'todo' }); + + const report = await backfillSummaryNulls(engine, quietLogger, { apply: true }); + + expect(project('p_busy').task_count).toBe(2); + expect(project('p_busy').total_estimate).toBe(42); + expect(project('p_busy').done_count).toBe(1); // the per-summary filter is honoured + expect(report.filled).toBe(3); + expect(report.nullRows).toBe(3); + // Every one of the three columns held real child data — the report says so, + // which is the evidence that 0 would have been wrong here. + expect(report.fields.every((f) => f.nonEmpty === 1)).toBe(true); + }); + + it('gives a NULL parent with NO children the empty-set value 0', async () => { + legacyParent('p_quiet'); + + await backfillSummaryNulls(engine, quietLogger, { apply: true }); + + expect(project('p_quiet').task_count).toBe(0); + expect(project('p_quiet').total_estimate).toBe(0); + expect(project('p_quiet').done_count).toBe(0); + }); + + it('leaves min/max/avg NULL exactly as they are, and reports them as out of scope', async () => { + legacyParent('p_busy'); + legacyTask('t1', 'p_busy', { estimate: 10 }); + + const report = await backfillSummaryNulls(engine, quietLogger, { apply: true }); + + // Undefined on an empty set — and undefined is what the column keeps here: + // this migration does not decide anything about them either way. + expect(project('p_busy').avg_estimate ?? null).toBeNull(); + expect(project('p_busy').max_estimate ?? null).toBeNull(); + expect(report.skippedUndefinedOnEmpty).toEqual( + expect.arrayContaining(['project.avg_estimate (avg)', 'project.max_estimate (max)']), + ); + // …and no write ever named them. + expect(d.writes.every((w) => !('avg_estimate' in w.data) && !('max_estimate' in w.data))).toBe(true); + }); + + it('is idempotent — the second run finds nothing and writes nothing', async () => { + legacyParent('p_busy'); + legacyTask('t1', 'p_busy', { estimate: 10 }); + legacyParent('p_quiet'); + + const first = await backfillSummaryNulls(engine, quietLogger, { apply: true }); + expect(first.filled).toBeGreaterThan(0); + const writesAfterFirst = d.writes.length; + + const second = await backfillSummaryNulls(engine, quietLogger, { apply: true }); + + expect(second.nullRows).toBe(0); + expect(second.filled).toBe(0); + expect(second.fields).toEqual([]); + expect(d.writes.length).toBe(writesAfterFirst); // not one further write + }); + + it('is a no-op on a database whose rows were all created with the seed (a fresh install)', async () => { + // Nothing legacy at all: every parent went through the engine's insert + // path, so #6013 already gave it 0. + const p = await engine.insert('project', { name: 'Apollo' }); + await engine.insert('task', { title: 't', estimate: 5, project: p.id }); + const writesBefore = d.writes.length; + + const report = await backfillSummaryNulls(engine, quietLogger, { apply: true }); + + expect(report.nullRows).toBe(0); + expect(report.filled).toBe(0); + expect(report.failures).toEqual([]); + expect(report.truncated).toBe(false); + expect(d.writes.length).toBe(writesBefore); + expect(formatSummaryBackfillReport(report)).toEqual( + expect.arrayContaining([expect.stringContaining('No NULL count/sum roll-up values found')]), + ); + }); + + it('dry run reports the same rows and writes nothing', async () => { + legacyParent('p_busy'); + legacyTask('t1', 'p_busy', { estimate: 10 }); + + const dry = await backfillSummaryNulls(engine, quietLogger, {}); + + expect(dry.applied).toBe(false); + expect(dry.nullRows).toBe(3); + expect(dry.filled).toBe(0); + expect(d.writes).toEqual([]); + expect(project('p_busy').task_count ?? null).toBeNull(); + + const applied = await backfillSummaryNulls(engine, quietLogger, { apply: true }); + // What the dry run said it would do is what the apply run did. + expect(applied.nullRows).toBe(dry.nullRows); + expect(applied.filled).toBe(dry.nullRows); + }); + + it('never overwrites a value already stored — including a deliberate 0', async () => { + legacyParent('p_imported', { task_count: 7, total_estimate: 0, done_count: 3 }); + legacyTask('t1', 'p_imported', { estimate: 10, status: 'done' }); + + const report = await backfillSummaryNulls(engine, quietLogger, { apply: true }); + + expect(project('p_imported').task_count).toBe(7); + expect(project('p_imported').total_estimate).toBe(0); + expect(project('p_imported').done_count).toBe(3); + expect(report.nullRows).toBe(0); + }); + + it('writes the SAME value the engine\'s own child-write recompute would', async () => { + // The point of sharing `aggregateSummaryValue`: after the backfill, the very + // next child write must not move the column. A second implementation that + // merged the summary filter differently, or fell back differently on an + // empty aggregate, would show up here as a column that changes under a user + // who changed nothing. + legacyParent('p_busy'); + legacyTask('t1', 'p_busy', { estimate: 10, status: 'done' }); + legacyTask('t2', 'p_busy', { estimate: 32, status: 'todo' }); + + await backfillSummaryNulls(engine, quietLogger, { apply: true }); + const backfilled = { + task_count: project('p_busy').task_count, + total_estimate: project('p_busy').total_estimate, + done_count: project('p_busy').done_count, + }; + + // A child write of a zero-valued, non-matching task: the recompute runs and + // must land on exactly the same numbers except the one it really changes. + await engine.insert('task', { title: 't3', estimate: 0, status: 'todo', project: 'p_busy' }); + + expect(project('p_busy').total_estimate).toBe(backfilled.total_estimate); + expect(project('p_busy').done_count).toBe(backfilled.done_count); + expect(project('p_busy').task_count).toBe(backfilled.task_count! + 1); + }); + + it('restricts to the objects it is given', async () => { + legacyParent('p_quiet'); + + const report = await backfillSummaryNulls(engine, quietLogger, { apply: true, objects: ['task'] }); + + expect(report.scannedObjects).toEqual([]); // `task` owns no roll-up + expect(report.nullRows).toBe(0); + expect(project('p_quiet').task_count ?? null).toBeNull(); + }); + + it('records a row it cannot write and carries on with the rest', async () => { + legacyParent('p_bad'); + legacyParent('p_good'); + d.failUpdateFor('p_bad'); + + const report = await backfillSummaryNulls(engine, quietLogger, { + apply: true, + // One attempt, no sleeping — this failure is deterministic, not transient. + retry: { maxRetries: 1, sleep: async () => {} }, + }); + + expect(report.failures.length).toBeGreaterThan(0); + expect(report.failures.every((f) => f.recordId === 'p_bad')).toBe(true); + // The healthy parent is still backfilled — one row's failure is not the + // run's failure. + expect(project('p_good').task_count).toBe(0); + expect(formatSummaryBackfillReport(report)).toEqual( + expect.arrayContaining([expect.stringContaining('could not be recomputed')]), + ); + }); +}); diff --git a/packages/objectql/src/summary-backfill.ts b/packages/objectql/src/summary-backfill.ts new file mode 100644 index 0000000000..131c1ab6a9 --- /dev/null +++ b/packages/objectql/src/summary-backfill.ts @@ -0,0 +1,372 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The one-off backfill of roll-up `count`/`sum` columns left `NULL` by inserts + * that predate PR #6013 — `os migrate summary-nulls` (#6063, the second half of + * #5749). + * + * ## The hole this fills + * + * PR #6013 seeds a roll-up's empty-set value at PARENT INSERT time, so from + * that release on a `count`/`sum` column starts at 0 and only ever moves to + * another number. It is a producer-side fix and therefore reaches new rows + * only: `ObjectQL.initializeSummaryFields` is create-time, and the recompute + * that would otherwise correct a stored `NULL` visits a parent only when one of + * its CHILDREN is written. A parent stored before the upgrade that has never + * had a child is named by neither path, so it keeps its `NULL` indefinitely — + * and `filter ["task_count", "=", 0]`, `ORDER BY`, `GROUP BY` and every formula + * reading the column silently drop it (null propagation). A re-seeded database + * is correct; an IN-PLACE upgraded one is not. That asymmetry is what this run + * removes, once. + * + * ## Why per-row recompute and not `SET col = 0 WHERE col IS NULL` + * + * The cheap `UPDATE` is not merely coarse, it is **wrong**: a pre-upgrade + * parent that DOES have children is `NULL` too — nothing ever recomputed it — + * and its correct value is the real aggregate, not 0. Writing 0 there would + * replace a visibly-missing value with a confidently-wrong one, and the next + * child write would silently change it back. So every `NULL` row is recomputed + * through {@link aggregateSummaryValue} — the identical computation the engine's + * own child-write recompute uses, over the identical descriptors the engine + * maintains (maintainer ruling on #6063, 2026-08-07). + * + * ## Scope: `count`/`sum` only + * + * A `NULL` `min`/`max`/`avg` is the LEGITIMATE reading of "no child rows" — + * those aggregates are undefined on the empty set, which is exactly why #6013 + * leaves them `null` at insert. They are not a defect and are never written + * here; {@link summaryNullIsBackfillable} is the single predicate, derived from + * the same empty-set list. + * + * ## Driver-agnostic, and no `IS NULL` pushdown + * + * The walk reads `id` + the roll-up columns and tests `== null` in JS. It does + * NOT push a null predicate down to the driver: null-comparison compilation is + * exactly where drivers diverge (`sql-driver-null-operators.test.ts`, + * `sql-driver-out-of-contract-filter-input.test.ts`), and a migration whose + * coverage depends on which driver it happens to run against is a migration + * that silently skips rows. Reading the value and testing it in JS is the one + * form that means the same thing everywhere (#6063 ruling 3). + * + * ## Idempotent + * + * Every write turns a `NULL` into a number, so the second run's walk finds no + * `NULL` rows and writes nothing. A partially-completed run is safe to repeat, + * and "re-run until it reports zero" is the operator's own verification. + */ + +import { keysetWalk, type KeysetPageQuery } from '@objectstack/types'; +import { withTransientRetry, type RetryOptions } from '@objectstack/core'; + +import { + aggregateSummaryValue, + summaryNullIsBackfillable, + type SummaryDescriptor, +} from './summary-aggregate.js'; + +/** Roll-up column examined by one run, with what the run did to it. */ +export interface SummaryBackfillFieldOutcome { + object: string; + field: string; + fn: 'count' | 'sum'; + /** The child object aggregated — so a report names the whole relationship. */ + childObject: string; + /** Parent rows found holding `NULL` in this column. */ + nullRows: number; + /** Of those, the ones whose recomputed value is a real aggregate (> 0 rows + * of children) — the rows a `SET col = 0` shortcut would have corrupted. */ + nonEmpty: number; + /** Values written (equal to `nullRows` on an apply run with no failures; + * always 0 on a dry run — a dry run writes nothing). */ + filled: number; + /** A few affected record ids, so an operator can go and look. */ + sampleRecordIds: string[]; +} + +/** One parent row whose recompute could not be completed. */ +export interface SummaryBackfillFailure { + object: string; + field: string; + recordId: string; + error: string; +} + +export interface SummaryBackfillReport { + /** Objects walked — those owning at least one backfillable roll-up column. */ + scannedObjects: string[]; + scannedRecords: number; + fields: SummaryBackfillFieldOutcome[]; + /** Parent rows found holding `NULL` in a `count`/`sum` roll-up. */ + nullRows: number; + /** Values actually written (0 on a dry run). */ + filled: number; + /** + * `object.field (fn)` roll-ups deliberately NOT touched: `min`/`max`/`avg` + * have no empty-set value, so a stored `null` there is the correct reading of + * "no child rows". Reported rather than silently omitted — the scope + * narrowing is a decision an operator should be able to SEE. + */ + skippedUndefinedOnEmpty: string[]; + /** False on a dry run — no writes were made. */ + applied: boolean; + /** A per-object cap or an unreadable object cut the walk short. */ + truncated: boolean; + /** Objects that could not be read at all — reported, and they truncate. */ + unreadableObjects: string[]; + /** Rows whose recompute failed after retries; the run continues past them. */ + failures: SummaryBackfillFailure[]; +} + +/** Engine surface the backfill needs: the engine's OWN roll-up index, a read, + * an aggregate, and the same update verb the recompute writes through. */ +export interface SummaryBackfillEngine { + getOwnedSummaryDescriptors(parentObject: string): SummaryDescriptor[]; + getConfigs?(): Record; + find(object: string, options: Record): Promise>>; + aggregate(object: string, query: any, options?: any): Promise; + update(object: string, data: Record, options?: any): Promise; +} + +export interface SummaryBackfillLogger { + info(msg: string, meta?: unknown): void; + warn(msg: string, meta?: unknown): void; +} + +export interface SummaryBackfillOptions { + /** Write the recomputed values. Omit for a read-only dry run. */ + apply?: boolean; + /** Restrict to these objects (default: every object owning a roll-up). */ + objects?: string[]; + /** Safety bound on parent rows read per object. */ + maxRecordsPerObject?: number; + /** Retry policy for one row's aggregate + update; defaults to the transient + * defaults the engine's own recompute uses (framework#3147). */ + retry?: RetryOptions; +} + +const SCAN_PAGE_SIZE = 500; +const MAX_SAMPLE_IDS = 5; +/** Unscoped by design: a deployment-wide backfill must see every org's rows. + * `isSystem` with no `tenantId` is what `buildDriverOptions` reads as "do not + * tenant-scope this" — the same context the other `os migrate` data runs use. */ +const SYSTEM_CTX = { isSystem: true } as const; + +/** The `count`/`sum` roll-ups `object` owns, and the `min`/`max`/`avg` ones it + * owns that are deliberately out of scope. */ +function partitionDescriptors( + engine: SummaryBackfillEngine, + object: string, +): { backfillable: SummaryDescriptor[]; skipped: string[] } { + let owned: SummaryDescriptor[] = []; + try { + owned = engine.getOwnedSummaryDescriptors(object) ?? []; + } catch { + owned = []; + } + const backfillable: SummaryDescriptor[] = []; + const skipped: string[] = []; + for (const desc of owned) { + if (summaryNullIsBackfillable(desc.fn)) backfillable.push(desc); + else skipped.push(`${desc.parentObject}.${desc.summaryField} (${desc.fn})`); + } + return { backfillable, skipped }; +} + +/** + * Recompute every `count`/`sum` roll-up column still stored as `NULL`. + * + * Dry run unless `options.apply` — and a dry run writes nothing at all, so the + * report can be reviewed (and diffed against what actually happened) before any + * row changes. + */ +export async function backfillSummaryNulls( + engine: SummaryBackfillEngine, + logger: SummaryBackfillLogger, + options: SummaryBackfillOptions = {}, +): Promise { + const apply = options.apply === true; + const maxPerObject = options.maxRecordsPerObject ?? 100_000; + const candidates = + options.objects ?? + (typeof engine.getConfigs === 'function' ? Object.keys(engine.getConfigs()) : []); + + const scannedObjects: string[] = []; + const outcomes = new Map(); + const skippedUndefinedOnEmpty: string[] = []; + const unreadableObjects: string[] = []; + const failures: SummaryBackfillFailure[] = []; + let scannedRecords = 0; + let truncated = false; + + for (const object of candidates) { + const { backfillable, skipped } = partitionDescriptors(engine, object); + skippedUndefinedOnEmpty.push(...skipped); + if (backfillable.length === 0) continue; + scannedObjects.push(object); + + for (const desc of backfillable) { + outcomes.set(`${object}.${desc.summaryField}`, { + object, + field: desc.summaryField, + fn: desc.fn as 'count' | 'sum', + childObject: desc.childObject, + nullRows: 0, + nonEmpty: 0, + filled: 0, + sampleRecordIds: [], + }); + } + + // Seek by `id` rather than offset (#4363): this run's claim is about EVERY + // stored row, and an offset walk cannot promise it visited them all. + const walk = keysetWalk>( + (q: KeysetPageQuery) => engine.find(object, { + ...q, + fields: ['id', ...backfillable.map((d) => d.summaryField)], + context: { ...SYSTEM_CTX }, + }), + { pageSize: SCAN_PAGE_SIZE, max: maxPerObject }, + ); + + try { + for await (const page of walk.pages()) { + for (const row of page) { + scannedRecords++; + const parentId = row.id; + if (parentId == null || parentId === '') continue; + for (const desc of backfillable) { + // The stored value is what this run judges: `null` (and the + // `undefined` a document driver returns for an absent column) is + // the hole; a real number — including a 0 the seed or a recompute + // already wrote, and including an author-supplied value — is left + // exactly as it is. + if (row[desc.summaryField] != null) continue; + const outcome = outcomes.get(`${object}.${desc.summaryField}`)!; + outcome.nullRows++; + if (outcome.sampleRecordIds.length < MAX_SAMPLE_IDS) { + outcome.sampleRecordIds.push(String(parentId)); + } + try { + // Counters are bumped AFTER the retry settles, never inside the + // retried closure — a retried attempt would otherwise count the + // same row twice and the report would overstate the run. + let computed: unknown; + await withTransientRetry(async () => { + computed = await aggregateSummaryValue( + engine, desc, String(parentId), { ...SYSTEM_CTX }, + ); + if (!apply) return; + await engine.update( + desc.parentObject, + { id: parentId, [desc.summaryField]: computed }, + { context: { ...SYSTEM_CTX } }, + ); + }, options.retry ?? {}); + if (typeof computed === 'number' && computed !== 0) outcome.nonEmpty++; + if (apply) outcome.filled++; + } catch (err) { + // One row's failure must not abort the rest — the same rule the + // engine's recompute follows. Recorded so the caller can surface + // it and the operator can re-run (the run is idempotent, so a + // repeat only revisits what is still `NULL`). + const message = (err as any)?.message ?? String(err); + logger.warn( + `[summary-backfill] ${object}.${desc.summaryField} record ${String(parentId)}: ${message}`, + ); + failures.push({ + object, field: desc.summaryField, recordId: String(parentId), error: message, + }); + } + } + } + } + } catch (err) { + // An object we cannot read is an object whose rows this run cannot vouch + // for. Record it AND truncate rather than skip it quietly. + logger.warn( + `[summary-backfill] cannot read ${object} (${(err as Error)?.message ?? err}) — ` + + 'reported as unreadable; this run does not cover it', + ); + unreadableObjects.push(object); + truncated = true; + continue; + } + if (walk.truncated) truncated = true; + } + + const fields = [...outcomes.values()] + .filter((f) => f.nullRows > 0) + .sort((a, b) => b.nullRows - a.nullRows); + + return { + scannedObjects, + scannedRecords, + fields, + nullRows: fields.reduce((n, f) => n + f.nullRows, 0), + filled: fields.reduce((n, f) => n + f.filled, 0), + skippedUndefinedOnEmpty, + applied: apply, + truncated, + unreadableObjects, + failures, + }; +} + +/** Human-readable report body, shared by the CLI's dry-run and apply output. */ +export function formatSummaryBackfillReport(report: SummaryBackfillReport): string[] { + const lines: string[] = []; + lines.push( + `Scanned ${report.scannedRecords} parent row(s) across ${report.scannedObjects.length} object(s) ` + + 'for count/sum roll-up columns still stored as NULL.', + ); + if (report.fields.length === 0) { + lines.push('✓ No NULL count/sum roll-up values found — nothing to backfill.'); + } else { + const verb = report.applied ? 'Backfilled' : 'Would backfill'; + lines.push(`${verb} ${report.applied ? report.filled : report.nullRows} value(s) in ${report.fields.length} column(s):`); + for (const f of report.fields) { + lines.push( + ` • ${f.object}.${f.field} (${f.fn} over ${f.childObject}) — ${f.nullRows} NULL row(s), ` + + `${f.nonEmpty} with real child data` + + `\n e.g. ${f.sampleRecordIds.join(', ')}`, + ); + } + if (report.fields.some((f) => f.nonEmpty > 0)) { + lines.push( + 'Rows "with real child data" are why this run recomputes instead of writing 0:', + 'their correct value is the aggregate over their children, not the empty-set value.', + ); + } + } + if (report.skippedUndefinedOnEmpty.length > 0) { + lines.push( + `· Untouched by design (no empty-set value — a null there means "no child rows"): ` + + report.skippedUndefinedOnEmpty.join(', '), + ); + } + if (report.failures.length > 0) { + lines.push(`✗ ${report.failures.length} row(s) could not be recomputed:`); + for (const f of report.failures.slice(0, MAX_SAMPLE_IDS)) { + lines.push(` • ${f.object}.${f.field} ${f.recordId}: ${f.error}`); + } + if (report.failures.length > MAX_SAMPLE_IDS) { + lines.push(` … and ${report.failures.length - MAX_SAMPLE_IDS} more`); + } + lines.push('Re-running is safe: the backfill only ever revisits rows still holding NULL.'); + } + if (report.unreadableObjects.length > 0) { + lines.push(`⚠ Unreadable object(s): ${report.unreadableObjects.join(', ')}`); + } + if (report.truncated) { + lines.push('⚠ Walk incomplete — some rows were not examined; re-run without the cap to finish.'); + } + if (!report.applied && report.nullRows > 0) { + lines.push('Dry run — nothing was written. Re-run with --apply to backfill.'); + } + return lines; +} + +/** Did this run leave the deployment with no known NULL roll-up left? */ +export function summaryBackfillComplete(report: SummaryBackfillReport): boolean { + return report.failures.length === 0 && !report.truncated && (report.applied || report.nullRows === 0); +} diff --git a/scripts/query-options-erasure-baseline.json b/scripts/query-options-erasure-baseline.json index 1b195743a3..670530bb9a 100644 --- a/scripts/query-options-erasure-baseline.json +++ b/scripts/query-options-erasure-baseline.json @@ -36,7 +36,8 @@ "packages/metadata-protocol/src/protocol.ts": 6, "packages/metadata-protocol/src/seed-loader.ts": 3, "packages/metadata/src/loaders/database-loader.ts": 6, - "packages/objectql/src/engine.ts": 13, + "packages/objectql/src/engine.ts": 12, + "packages/objectql/src/summary-aggregate.ts": 1, "packages/plugins/plugin-approvals/src/approval-service.ts": 10, "packages/plugins/plugin-approvals/src/approver-org-scope.ts": 2, "packages/plugins/plugin-approvals/src/lifecycle-hooks.ts": 4, From 1cb6db7a29a2c76356d416b3d61f3576396405cb Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 03:57:43 +0000 Subject: [PATCH 2/3] fix(objectql): type the roll-up aggregate call instead of grandfathering it (#6063) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check:query-options-erasure` rejects a NEW file in its baseline — the grandfather list only shrinks. The `as any` came along with the code lifted out of engine.ts and is not needed there: `SummaryAggregateEngine.aggregate` takes the query directly, so the cast is dropped and the baseline records only engine.ts's own count falling 13 -> 12. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --- packages/objectql/src/summary-aggregate.ts | 2 +- scripts/query-options-erasure-baseline.json | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/objectql/src/summary-aggregate.ts b/packages/objectql/src/summary-aggregate.ts index 5055a9ddfd..8b6c878dcf 100644 --- a/packages/objectql/src/summary-aggregate.ts +++ b/packages/objectql/src/summary-aggregate.ts @@ -116,7 +116,7 @@ export async function aggregateSummaryValue( alias: 'value', }], context: execCtx, - } as any); + }); const value = rows?.[0]?.value; return value == null ? summaryEmptySetValue(desc.fn) : value; } diff --git a/scripts/query-options-erasure-baseline.json b/scripts/query-options-erasure-baseline.json index 670530bb9a..75661a7673 100644 --- a/scripts/query-options-erasure-baseline.json +++ b/scripts/query-options-erasure-baseline.json @@ -37,7 +37,6 @@ "packages/metadata-protocol/src/seed-loader.ts": 3, "packages/metadata/src/loaders/database-loader.ts": 6, "packages/objectql/src/engine.ts": 12, - "packages/objectql/src/summary-aggregate.ts": 1, "packages/plugins/plugin-approvals/src/approval-service.ts": 10, "packages/plugins/plugin-approvals/src/approver-org-scope.ts": 2, "packages/plugins/plugin-approvals/src/lifecycle-hooks.ts": 4, From c92d007ca63b5a868f1008e05d1d03956eeb8d22 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 04:07:19 +0000 Subject: [PATCH 3/3] fix(cli): type the objectql lookup in summary-nulls instead of erasing it to any (#6063) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The slot-lookup rule (#4168/#4251) rejects `const engine: any = getService('objectql')` — the sibling migrate commands are silent only because they are grandfathered by file, and that baseline only shrinks. The command's real requirement is `SummaryBackfillEngine` (the slot contract plus the one member the backfill reads), so name that type: the call site keeps its checking and the lookup is not erased. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --- packages/cli/src/commands/migrate/summary-nulls.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/commands/migrate/summary-nulls.ts b/packages/cli/src/commands/migrate/summary-nulls.ts index 1f772d1adc..d9177d30cf 100644 --- a/packages/cli/src/commands/migrate/summary-nulls.ts +++ b/packages/cli/src/commands/migrate/summary-nulls.ts @@ -18,6 +18,12 @@ import { bootSchemaStack } from '../../utils/schema-migrate.js'; import { OCCUPANCY_HINT, probeMigrationTarget } from '../../utils/migrate-occupancy-gate.js'; import { describeOccupancy } from '../../utils/sqlite-occupancy.js'; import { buildDataMigrationPlugins } from '../../utils/data-migration-plugins.js'; +// Type-only, so the heavy engine package is still loaded lazily below: this is +// the surface the migration actually needs, which is wider than the `objectql` +// slot contract by exactly one member (`getOwnedSummaryDescriptors`). Naming it +// here keeps the call site checked instead of erasing the lookup to `any` +// (#4168/#4251). +import type { SummaryBackfillEngine } from '@objectstack/objectql'; async function confirm(question: string): Promise { if (!process.stdin.isTTY) return false; // non-interactive → require --yes @@ -172,7 +178,7 @@ export default class MigrateSummaryNulls extends Command { } try { - const engine: any = stack.kernel.getService('objectql'); + const engine: SummaryBackfillEngine = stack.kernel.getService('objectql'); if (typeof engine?.getOwnedSummaryDescriptors !== 'function') { throw new Error('No ObjectQL engine on this stack — cannot read the roll-up index.'); }