diff --git a/.changeset/rollup-non-numeric-aggregand.md b/.changeset/rollup-non-numeric-aggregand.md new file mode 100644 index 0000000000..1673c5baed --- /dev/null +++ b/.changeset/rollup-non-numeric-aggregand.md @@ -0,0 +1,14 @@ +--- +"@objectstack/lint": minor +--- + +`os lint` now refuses a `min`/`max` roll-up whose answer cannot be stored in the column it rolls up into — `rollup/non-numeric-aggregand`, at `error`. + +`FieldSchema.summaryOperations` admits `min`/`max` over ANY child field, and the engine's `aggregateSummaryValue` returns the driver's answer verbatim (only an empty-set fallback stands between the backend and the stored value). A `summary` field is a member of the spec's `NUMERIC_VALUE_TYPES`, so `valueSchemaFor` answers `z.number().finite()` for it and `driver-sql`'s `createColumn` emits a float column. An ordinary "latest shipment" roll-up — `max` over a `datetime` child field — therefore computes an instant into a column the value contract says holds a finite number, and nothing between author and driver correlated the two. It is refused at authoring time rather than tolerated in a consumer (Prime Directive #12). + +- **The accept set** is the numeric class union the boolean class, read from `NUMERIC_VALUE_TYPES` and `BOOLEAN_VALUE_TYPES` rather than typed out. The first is the set that DEFINES the criterion — it is the membership `valueSchemaFor` consults to answer `z.number().finite()`, so a type joining it moves the value contract and this door together. The second is admitted on the authority of the `min(flag)=0` / `max(flag)=1` ruling pinned by the spec's own `AGGREGATION_CASES` (#11152): the answer is a number, so it fits. +- **It is NOT `isAggregateCompatibleWithFieldType`.** That table deliberately accepts `min`/`max` over the temporal class, because there the answer is returned to a caller and "return[s] a value of the field's OWN type" (#15768). Reusing it here would accept the very declaration this rule exists to refuse. The two questions look alike and are not — "can every backend give one answer" versus "does that answer fit the column this roll-up is stored into" — so this predicate is that table's `min`/`max` row narrowed by exactly the temporal class, and a test pins the disagreement. +- **Scope.** `min`/`max` only. `count` reads no value off the field; `sum`/`avg` over a non-numeric child is a different shape, whose accept set the aggregate table's own rows already exclude, and is not widened into here. +- **Silent where it cannot resolve.** An unknown child object, a field the child does not declare, or a field with no declared type produce no finding — the aggregate table's own consumer tier ("a consumer that cannot resolve a field's type must NOT call the predicate with a guess"). A partially-loaded model cannot draw a false refusal. + +No export moves: the rule id is an inline literal inside the already-exported `lintDataModel`, beside `rollup/missing-summary`. Measured across this repository, no declaration trips the new refusal — all three `min`/`max` roll-ups aggregate a `number` child field — so this adds a door rather than migrating anything. diff --git a/packages/lint/src/data-model-rules.summary-rollup.test.ts b/packages/lint/src/data-model-rules.summary-rollup.test.ts new file mode 100644 index 0000000000..aec6893e22 --- /dev/null +++ b/packages/lint/src/data-model-rules.summary-rollup.test.ts @@ -0,0 +1,186 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// `rollup/non-numeric-aggregand` — the roll-up door. +// +// `FieldSchema.summaryOperations` admits `min`/`max` over ANY child field, and +// `aggregateSummaryValue` (objectql) returns the driver's answer verbatim with +// only an empty-set fallback. A `summary` field is a member of the spec's +// `NUMERIC_VALUE_TYPES`, so `valueSchemaFor` answers `z.number().finite()` for +// it and `driver-sql`'s `createColumn` emits `table.float(name)`. An ordinary +// "latest shipment" roll-up — `max` over a `datetime` child field — therefore +// computes an INSTANT into a column the value contract says holds a finite +// number, and nothing between author and driver correlated the two. +// +// The load-bearing test in this file is `discriminates from the analytics +// table`. The refusal CANNOT be `isAggregateCompatibleWithFieldType`: that +// table deliberately accepts `min`/`max` over the temporal class, because +// there the answer is RETURNED to a caller rather than stored, and it "return[s] +// a value of the field's OWN type (#15768)". Reusing it here would accept the +// very declaration this rule exists to refuse, and the rule would be green +// because it never fires. The two questions look alike and are not: +// "can every backend give one answer" vs "does that answer fit the column this +// roll-up is stored into". +import { describe, expect, it } from 'vitest'; +import { + BOOLEAN_VALUE_TYPES, + FieldType, + NUMERIC_VALUE_TYPES, + isAggregateCompatibleWithFieldType, +} from '@objectstack/spec/data'; + +import { lintDataModel } from './data-model-rules.js'; + +const RULE = 'rollup/non-numeric-aggregand'; + +/** A parent rolling up `fn(child.)`, and a child declaring that field as `childType`. */ +const model = (childType: string | undefined, fn = 'max', overrides: Record = {}) => [ + { + name: 'invoice', + fields: { + name: { type: 'text' }, + rolled_up: { + type: 'summary', + summaryOperations: { object: 'invoice_line', field: 'shipped_at', function: fn, ...overrides }, + }, + }, + }, + { + name: 'invoice_line', + fields: { + name: { type: 'text' }, + invoice: { type: 'master_detail', reference: 'invoice', required: true, deleteBehavior: 'cascade' }, + ...(childType ? { shipped_at: { type: childType } } : {}), + }, + }, +]; + +const findings = (objects: unknown[]) => lintDataModel(objects as any[]).filter((i) => i.rule === RULE); + +describe('rollup/non-numeric-aggregand — refuses a min/max roll-up whose answer cannot fit the column', () => { + it('refuses the card\'s own example: max over a datetime child field', () => { + const found = findings(model('datetime')); + expect(found).toHaveLength(1); + const issue = found[0]; + expect(issue.severity).toBe('error'); + expect(issue.rule).toBe(RULE); + // The message must let an author act without opening the source: WHICH + // child object, WHICH field, its DECLARED type, and why the answer cannot + // be stored. + expect(issue.message).toContain('invoice_line'); + expect(issue.message).toContain('shipped_at'); + expect(issue.message).toContain('datetime'); + expect(issue.message).toContain('finite number'); + expect(issue.path).toBe('objects[0].fields.rolled_up.summaryOperations.field'); + expect(issue.fix).toBeTruthy(); + }); + + it('refuses every member of the temporal class, under both min and max', () => { + for (const childType of ['date', 'datetime', 'time']) { + for (const fn of ['min', 'max']) { + expect(findings(model(childType, fn)), `${fn}(${childType})`).toHaveLength(1); + } + } + }); + + it('refuses a text / option / reference child field too', () => { + for (const childType of ['text', 'select', 'lookup', 'json', 'autonumber']) { + expect(findings(model(childType)), childType).toHaveLength(1); + } + }); +}); + +describe('rollup/non-numeric-aggregand — accepts the classes whose answer IS a number', () => { + it('accepts the numeric class', () => { + for (const childType of NUMERIC_VALUE_TYPES) { + expect(findings(model(childType)), childType).toEqual([]); + } + }); + + // #11152 (maintainer ruling, 2026-08-28), pinned by the spec's own + // `AGGREGATION_CASES`: `min(flag)=0` / `max(flag)=1` on six backends. A + // careless predicate — "numeric only" — breaks exactly this leg. + it('accepts the boolean class', () => { + for (const childType of BOOLEAN_VALUE_TYPES) { + expect(findings(model(childType)), childType).toEqual([]); + } + }); + + it('accepts exactly the numeric ∪ boolean classes over every declared FieldType', () => { + const accepted = new Set([...NUMERIC_VALUE_TYPES, ...BOOLEAN_VALUE_TYPES]); + const refused = FieldType.options.filter((t) => findings(model(t)).length > 0); + expect(refused.sort()).toEqual(FieldType.options.filter((t) => !accepted.has(t)).sort()); + // A floor, so a future edit that stops the rule firing at all cannot make + // the equality above vacuously true. + expect(refused.length).toBeGreaterThan(10); + }); +}); + +describe('rollup/non-numeric-aggregand — discriminates from the analytics table', () => { + // ⭐ The assertion that stops someone "simplifying" this rule back into + // `isAggregateCompatibleWithFieldType`. If this ever fails because the table + // started refusing the temporal class, that is a spec change to read, not a + // test to update: the two predicates would then answer the same question. + it('the analytics table ACCEPTS the temporal pair this rule refuses', () => { + for (const childType of ['date', 'datetime', 'time']) { + for (const fn of ['min', 'max']) { + expect(isAggregateCompatibleWithFieldType(fn, childType), `${fn}(${childType})`).toBe(true); + expect(findings(model(childType, fn)), `${fn}(${childType})`).toHaveLength(1); + } + } + }); + + it('the two agree everywhere else on min/max — the difference is exactly the temporal class', () => { + const TEMPORAL = new Set(['date', 'datetime', 'time']); + for (const childType of FieldType.options) { + const tableAccepts = isAggregateCompatibleWithFieldType('max', childType); + const doorAccepts = findings(model(childType)).length === 0; + if (TEMPORAL.has(childType)) continue; + expect(doorAccepts, `max(${childType})`).toBe(tableAccepts); + } + }); +}); + +describe('rollup/non-numeric-aggregand — stays silent where it cannot resolve, and outside its scope', () => { + // "A consumer that cannot resolve a field's type must NOT call the predicate + // with a guess" — `aggregate-field-type-compatibility.ts`. A refusal fired on + // a partially-loaded model would redden an app over metadata never seen. + it('is silent when the child object is not in the pass\'s object set', () => { + const objects = model('datetime').slice(0, 1); // parent only — no `invoice_line` + expect(findings(objects)).toEqual([]); + }); + + it('is silent when the named field is not declared on the child', () => { + expect(findings(model(undefined))).toEqual([]); + }); + + it('is silent when the child field declares no type', () => { + const objects = model('datetime') as any[]; + objects[1].fields.shipped_at = { label: 'Shipped At' }; + expect(findings(objects)).toEqual([]); + }); + + it('is silent when summaryOperations names no object or no field', () => { + expect(findings(model('datetime', 'max', { object: undefined }))).toEqual([]); + expect(findings(model('datetime', 'max', { field: undefined }))).toEqual([]); + expect(findings(model('datetime', 'max', { object: '' }))).toEqual([]); + }); + + it('is scoped to min/max — count reads no value off the field', () => { + expect(findings(model('datetime', 'count'))).toEqual([]); + }); + + // `sum` / `avg` over a non-numeric child is a different shape, whose accept + // set the analytics table's own rows already exclude, and is deliberately not + // this rule's. (Nothing in this tree consults that table — an open gap + // reported with this change, not one this rule silently absorbs.) + it('does not fire for sum or avg', () => { + expect(findings(model('datetime', 'sum'))).toEqual([]); + expect(findings(model('datetime', 'avg'))).toEqual([]); + }); + + it('is silent for a field that is not a summary at all', () => { + const objects = model('datetime') as any[]; + objects[0].fields.rolled_up.type = 'number'; + expect(findings(objects)).toEqual([]); + }); +}); diff --git a/packages/lint/src/data-model-rules.ts b/packages/lint/src/data-model-rules.ts index 4b3c419928..ac2309969c 100644 --- a/packages/lint/src/data-model-rules.ts +++ b/packages/lint/src/data-model-rules.ts @@ -16,6 +16,8 @@ * schema-valid AND lint-clean here. */ +import { BOOLEAN_VALUE_TYPES, NUMERIC_VALUE_TYPES } from '@objectstack/spec/data'; + export type Severity = 'error' | 'warning' | 'suggestion'; export interface LintIssue { @@ -105,6 +107,57 @@ const NUMERIC_TYPES = new Set([ 'number', 'currency', 'integer', 'decimal', 'percent', 'float', 'double', ]); const OPTION_FIELD_TYPES = new Set(['select', 'multiselect', 'radio', 'enum']); + +/** + * Does a `min` / `max` roll-up over a child field of `childFieldType` produce + * an answer that fits the column a `summary` field is stored in? + * + * This is the roll-up door's OWN predicate, and it is deliberately NOT + * `isAggregateCompatibleWithFieldType` + * (`packages/spec/src/data/aggregate-field-type-compatibility.ts`). The two + * answer different questions about the same pair: + * + * - That table asks **"can every backend give one answer"** — and for + * `min` / `max` it ACCEPTS the temporal class on purpose, because there + * they "return a value of the field's OWN type (#15768)". A `DatasetMeasure` + * hands that answer straight to the caller, so a temporal answer is fine. + * - This rule asks **"does that answer fit the column this roll-up is + * STORED into"**. A roll-up's answer is not returned, it is persisted in + * the `summary` field, whose value contract is `z.number().finite()` + * (`valueSchemaFor`, ADR-0104 D1) because `summary` is a member of + * `NUMERIC_VALUE_TYPES`. + * + * So this predicate is the analytics table's `min` / `max` row NARROWED by + * exactly the temporal class. Reusing that table here would accept + * `max(child.shipped_at)` — the very declaration this rule exists to refuse — + * and the gate would be green because it never fires. ⛔ Do not "simplify" + * the two into one call; `data-model-rules.summary-rollup.test.ts` pins the + * disagreement. + * + * Membership is READ from the spec's own value classes rather than typed out + * here, and from these two specifically: + * + * - `NUMERIC_VALUE_TYPES` is the set that DEFINES the acceptance criterion — + * it is the very membership `valueSchemaFor` consults to answer + * `z.number().finite()`. A type joining that class changes the `summary` + * value contract and this door with it, in one edit. + * - `BOOLEAN_VALUE_TYPES` is admitted on the authority of maintainer ruling + * #11152: booleans aggregate as NUMBERS on every backend with no + * per-aggregate exception, pinned by the spec's own `AGGREGATION_CASES` + * (`min(flag)=0`, `max(flag)=1`, enrolled on six backends) and implemented + * by `driver-sql`'s `int` cast on Postgres (#11635). The answer is a + * number, so it fits. + * + * ⛔ Deliberately NOT read: `NON_TEXT_STORED_VALUE_TYPES`, whose membership is + * these same two classes TODAY. It is defined by a third question — "is the + * stored value never text" (#14079) — and excludes the temporal class for a + * DIALECT reason, not for this one. Composing the union here states why each + * half is in, so a future member of that set cannot widen this door as a side + * effect. + */ +function summaryRollupAnswerFitsColumn(childFieldType: string): boolean { + return NUMERIC_VALUE_TYPES.has(childFieldType) || BOOLEAN_VALUE_TYPES.has(childFieldType); +} /** * Field names that give an object a title FACE, for R9 * (`object/missing-name-field`). @@ -491,6 +544,12 @@ export function lintDataModel(objects: any[]): LintIssue[] { ]; if (!Array.isArray(objects) || objects.length === 0) return issues; + // Index: object name → the object, for resolving a roll-up's child object. + const objectsByName: Record = {}; + for (const o of objects) { + if (o?.name && !(o.name in objectsByName)) objectsByName[o.name] = o; + } + // Index: parent object name → child relationships pointing at it. const childrenByParent: Record> = {}; for (const child of objects) { @@ -574,6 +633,71 @@ export function lintDataModel(objects: any[]): LintIssue[] { } } + // R13 — a `min`/`max` roll-up must aggregate a child field whose ANSWER + // fits the column the roll-up is stored into. + // + // `FieldSchema.summaryOperations` admits `min`/`max` over ANY child + // field, and `aggregateSummaryValue` (objectql) returns the driver's + // answer verbatim — only an empty-set fallback stands between the + // backend and the stored value. So an ordinary "latest shipment" + // roll-up, `max` over a `datetime` child field, computes an INSTANT into + // a field whose value contract says finite number. Nothing between + // author and driver correlated the two, so it is refused here, at + // authoring time, at `error` (Prime Directive #12: reject at authoring, + // never tolerate in a consumer). + // + // Scoped to `min`/`max` deliberately: `count` ignores the field + // entirely, and `sum`/`avg` over a non-numeric child is a different + // shape, whose accept set the analytics table's own rows already + // EXCLUDE. Measured on this tree, nothing consults that table on the + // roll-up door (or anywhere else), so that is an open gap, not a + // refusal this rule may lean on — and widening here to cover it would + // be a second account of a pair the table already rules on. + if (type === 'summary') { + const ops = def.summaryOperations; + const fn = ops?.function; + const childName = ops?.object; + const childFieldName = ops?.field; + if ( + (fn === 'min' || fn === 'max') && + typeof childName === 'string' && childName !== '' && + typeof childFieldName === 'string' && childFieldName !== '' + ) { + // SILENCE, never a guess, on anything this pass cannot resolve: a + // child object contributed by another package, a partially-loaded + // stack, or a field name that resolves to no declaration. The spec's + // own aggregate table states the tier — "a consumer that cannot + // resolve a field's type … must NOT call the predicate with a guess; + // 'cannot answer, do not block' is the consumer's tier". A refusal + // fired on an unresolvable model would redden an app for metadata + // this pass simply never saw. + const child = objectsByName[childName]; + const childField = child + ? fieldEntries(child.fields).find((f) => f.name === childFieldName) + : undefined; + const childType = childField?.def?.type; + if (typeof childType === 'string' && !summaryRollupAnswerFitsColumn(childType)) { + issues.push({ + severity: 'error', + rule: 'rollup/non-numeric-aggregand', + message: + `summary field "${obj.name}.${fieldName}" rolls up ` + + `${fn}(${childName}.${childFieldName}), but "${childName}.${childFieldName}" is ` + + `a ${childType} field — ${fn} answers with a value of the CHILD field's own type, ` + + `while a summary field's value contract is a finite number (it is a member of the ` + + `spec's NUMERIC_VALUE_TYPES class), so the answer does not fit the column the ` + + `roll-up is stored in`, + path: `${fieldPath}.summaryOperations.field`, + fix: + `Aggregate a numeric or boolean child field instead (min/max over those answer ` + + `with a number), or use function: 'count' — which reads no value off the field. ` + + `To carry a ${childType} on "${obj.name}", declare a ${childType} field and ` + + `maintain it from a flow; a roll-up cannot store one.`, + }); + } + } + } + if (!RELATIONSHIP_TYPES.has(type)) continue; const parent = refOf(def);