Skip to content

Commit f6a481e

Browse files
committed
feat(lint): refuse a min/max roll-up whose answer cannot fit the summary column
`FieldSchema.summaryOperations` admits `min`/`max` over ANY child field, and `aggregateSummaryValue` returns the driver's answer verbatim (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. 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. Add `rollup/non-numeric-aggregand` (error) to `lintDataModel`, beside `rollup/missing-summary`. Its predicate is the roll-up door's OWN: the numeric class union the boolean class, read from `NUMERIC_VALUE_TYPES` and `BOOLEAN_VALUE_TYPES` — the analytics table's min/max row narrowed by exactly the temporal class, because that table answers "can every backend give one answer" while this door answers "does that answer fit the column this roll-up is stored into". Silent on anything the pass cannot resolve (unknown child object, undeclared field, missing type), per the aggregate table's own consumer tier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016N6xmWt5hYm94ffVEwGH8x
1 parent fe2b755 commit f6a481e

2 files changed

Lines changed: 304 additions & 0 deletions

File tree

Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,184 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// `rollup/non-numeric-aggregand` — the roll-up door.
4+
//
5+
// `FieldSchema.summaryOperations` admits `min`/`max` over ANY child field, and
6+
// `aggregateSummaryValue` (objectql) returns the driver's answer verbatim with
7+
// only an empty-set fallback. A `summary` field is a member of the spec's
8+
// `NUMERIC_VALUE_TYPES`, so `valueSchemaFor` answers `z.number().finite()` for
9+
// it and `driver-sql`'s `createColumn` emits `table.float(name)`. An ordinary
10+
// "latest shipment" roll-up — `max` over a `datetime` child field — therefore
11+
// computes an INSTANT into a column the value contract says holds a finite
12+
// number, and nothing between author and driver correlated the two.
13+
//
14+
// The load-bearing test in this file is `discriminates from the analytics
15+
// table`. The refusal CANNOT be `isAggregateCompatibleWithFieldType`: that
16+
// table deliberately accepts `min`/`max` over the temporal class, because
17+
// there the answer is RETURNED to a caller rather than stored, and it "return[s]
18+
// a value of the field's OWN type (#15768)". Reusing it here would accept the
19+
// very declaration this rule exists to refuse, and the rule would be green
20+
// because it never fires. The two questions look alike and are not:
21+
// "can every backend give one answer" vs "does that answer fit the column this
22+
// roll-up is stored into".
23+
import { describe, expect, it } from 'vitest';
24+
import {
25+
BOOLEAN_VALUE_TYPES,
26+
FieldType,
27+
NUMERIC_VALUE_TYPES,
28+
isAggregateCompatibleWithFieldType,
29+
} from '@objectstack/spec/data';
30+
31+
import { lintDataModel } from './data-model-rules.js';
32+
33+
const RULE = 'rollup/non-numeric-aggregand';
34+
35+
/** A parent rolling up `fn(child.<field>)`, and a child declaring that field as `childType`. */
36+
const model = (childType: string | undefined, fn = 'max', overrides: Record<string, unknown> = {}) => [
37+
{
38+
name: 'invoice',
39+
fields: {
40+
name: { type: 'text' },
41+
rolled_up: {
42+
type: 'summary',
43+
summaryOperations: { object: 'invoice_line', field: 'shipped_at', function: fn, ...overrides },
44+
},
45+
},
46+
},
47+
{
48+
name: 'invoice_line',
49+
fields: {
50+
name: { type: 'text' },
51+
invoice: { type: 'master_detail', reference: 'invoice', required: true, deleteBehavior: 'cascade' },
52+
...(childType ? { shipped_at: { type: childType } } : {}),
53+
},
54+
},
55+
];
56+
57+
const findings = (objects: unknown[]) => lintDataModel(objects as any[]).filter((i) => i.rule === RULE);
58+
59+
describe('rollup/non-numeric-aggregand — refuses a min/max roll-up whose answer cannot fit the column', () => {
60+
it('refuses the card\'s own example: max over a datetime child field', () => {
61+
const found = findings(model('datetime'));
62+
expect(found).toHaveLength(1);
63+
const issue = found[0];
64+
expect(issue.severity).toBe('error');
65+
expect(issue.rule).toBe(RULE);
66+
// The message must let an author act without opening the source: WHICH
67+
// child object, WHICH field, its DECLARED type, and why the answer cannot
68+
// be stored.
69+
expect(issue.message).toContain('invoice_line');
70+
expect(issue.message).toContain('shipped_at');
71+
expect(issue.message).toContain('datetime');
72+
expect(issue.message).toContain('finite number');
73+
expect(issue.path).toBe('objects[0].fields.rolled_up.summaryOperations.field');
74+
expect(issue.fix).toBeTruthy();
75+
});
76+
77+
it('refuses every member of the temporal class, under both min and max', () => {
78+
for (const childType of ['date', 'datetime', 'time']) {
79+
for (const fn of ['min', 'max']) {
80+
expect(findings(model(childType, fn)), `${fn}(${childType})`).toHaveLength(1);
81+
}
82+
}
83+
});
84+
85+
it('refuses a text / option / reference child field too', () => {
86+
for (const childType of ['text', 'select', 'lookup', 'json', 'autonumber']) {
87+
expect(findings(model(childType)), childType).toHaveLength(1);
88+
}
89+
});
90+
});
91+
92+
describe('rollup/non-numeric-aggregand — accepts the classes whose answer IS a number', () => {
93+
it('accepts the numeric class', () => {
94+
for (const childType of NUMERIC_VALUE_TYPES) {
95+
expect(findings(model(childType)), childType).toEqual([]);
96+
}
97+
});
98+
99+
// #11152 (maintainer ruling, 2026-08-28), pinned by the spec's own
100+
// `AGGREGATION_CASES`: `min(flag)=0` / `max(flag)=1` on six backends. A
101+
// careless predicate — "numeric only" — breaks exactly this leg.
102+
it('accepts the boolean class', () => {
103+
for (const childType of BOOLEAN_VALUE_TYPES) {
104+
expect(findings(model(childType)), childType).toEqual([]);
105+
}
106+
});
107+
108+
it('accepts exactly the numeric ∪ boolean classes over every declared FieldType', () => {
109+
const accepted = new Set([...NUMERIC_VALUE_TYPES, ...BOOLEAN_VALUE_TYPES]);
110+
const refused = FieldType.options.filter((t) => findings(model(t)).length > 0);
111+
expect(refused.sort()).toEqual(FieldType.options.filter((t) => !accepted.has(t)).sort());
112+
// A floor, so a future edit that stops the rule firing at all cannot make
113+
// the equality above vacuously true.
114+
expect(refused.length).toBeGreaterThan(10);
115+
});
116+
});
117+
118+
describe('rollup/non-numeric-aggregand — discriminates from the analytics table', () => {
119+
// ⭐ The assertion that stops someone "simplifying" this rule back into
120+
// `isAggregateCompatibleWithFieldType`. If this ever fails because the table
121+
// started refusing the temporal class, that is a spec change to read, not a
122+
// test to update: the two predicates would then answer the same question.
123+
it('the analytics table ACCEPTS the temporal pair this rule refuses', () => {
124+
for (const childType of ['date', 'datetime', 'time']) {
125+
for (const fn of ['min', 'max']) {
126+
expect(isAggregateCompatibleWithFieldType(fn, childType), `${fn}(${childType})`).toBe(true);
127+
expect(findings(model(childType, fn)), `${fn}(${childType})`).toHaveLength(1);
128+
}
129+
}
130+
});
131+
132+
it('the two agree everywhere else on min/max — the difference is exactly the temporal class', () => {
133+
const TEMPORAL = new Set(['date', 'datetime', 'time']);
134+
for (const childType of FieldType.options) {
135+
const tableAccepts = isAggregateCompatibleWithFieldType('max', childType);
136+
const doorAccepts = findings(model(childType)).length === 0;
137+
if (TEMPORAL.has(childType)) continue;
138+
expect(doorAccepts, `max(${childType})`).toBe(tableAccepts);
139+
}
140+
});
141+
});
142+
143+
describe('rollup/non-numeric-aggregand — stays silent where it cannot resolve, and outside its scope', () => {
144+
// "A consumer that cannot resolve a field's type must NOT call the predicate
145+
// with a guess" — `aggregate-field-type-compatibility.ts`. A refusal fired on
146+
// a partially-loaded model would redden an app over metadata never seen.
147+
it('is silent when the child object is not in the pass\'s object set', () => {
148+
const objects = model('datetime').slice(0, 1); // parent only — no `invoice_line`
149+
expect(findings(objects)).toEqual([]);
150+
});
151+
152+
it('is silent when the named field is not declared on the child', () => {
153+
expect(findings(model(undefined))).toEqual([]);
154+
});
155+
156+
it('is silent when the child field declares no type', () => {
157+
const objects = model('datetime') as any[];
158+
objects[1].fields.shipped_at = { label: 'Shipped At' };
159+
expect(findings(objects)).toEqual([]);
160+
});
161+
162+
it('is silent when summaryOperations names no object or no field', () => {
163+
expect(findings(model('datetime', 'max', { object: undefined }))).toEqual([]);
164+
expect(findings(model('datetime', 'max', { field: undefined }))).toEqual([]);
165+
expect(findings(model('datetime', 'max', { object: '' }))).toEqual([]);
166+
});
167+
168+
it('is scoped to min/max — count reads no value off the field', () => {
169+
expect(findings(model('datetime', 'count'))).toEqual([]);
170+
});
171+
172+
// `sum` / `avg` over a non-numeric child is a different shape (the analytics
173+
// table already refuses those pairs) and is deliberately not this rule's.
174+
it('does not fire for sum or avg', () => {
175+
expect(findings(model('datetime', 'sum'))).toEqual([]);
176+
expect(findings(model('datetime', 'avg'))).toEqual([]);
177+
});
178+
179+
it('is silent for a field that is not a summary at all', () => {
180+
const objects = model('datetime') as any[];
181+
objects[0].fields.rolled_up.type = 'number';
182+
expect(findings(objects)).toEqual([]);
183+
});
184+
});

packages/lint/src/data-model-rules.ts

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
* schema-valid AND lint-clean here.
1717
*/
1818

19+
import { BOOLEAN_VALUE_TYPES, NUMERIC_VALUE_TYPES } from '@objectstack/spec/data';
20+
1921
export type Severity = 'error' | 'warning' | 'suggestion';
2022

2123
export interface LintIssue {
@@ -105,6 +107,57 @@ const NUMERIC_TYPES = new Set([
105107
'number', 'currency', 'integer', 'decimal', 'percent', 'float', 'double',
106108
]);
107109
const OPTION_FIELD_TYPES = new Set(['select', 'multiselect', 'radio', 'enum']);
110+
111+
/**
112+
* Does a `min` / `max` roll-up over a child field of `childFieldType` produce
113+
* an answer that fits the column a `summary` field is stored in?
114+
*
115+
* This is the roll-up door's OWN predicate, and it is deliberately NOT
116+
* `isAggregateCompatibleWithFieldType`
117+
* (`packages/spec/src/data/aggregate-field-type-compatibility.ts`). The two
118+
* answer different questions about the same pair:
119+
*
120+
* - That table asks **"can every backend give one answer"** — and for
121+
* `min` / `max` it ACCEPTS the temporal class on purpose, because there
122+
* they "return a value of the field's OWN type (#15768)". A `DatasetMeasure`
123+
* hands that answer straight to the caller, so a temporal answer is fine.
124+
* - This rule asks **"does that answer fit the column this roll-up is
125+
* STORED into"**. A roll-up's answer is not returned, it is persisted in
126+
* the `summary` field, whose value contract is `z.number().finite()`
127+
* (`valueSchemaFor`, ADR-0104 D1) because `summary` is a member of
128+
* `NUMERIC_VALUE_TYPES`.
129+
*
130+
* So this predicate is the analytics table's `min` / `max` row NARROWED by
131+
* exactly the temporal class. Reusing that table here would accept
132+
* `max(child.shipped_at)` — the very declaration this rule exists to refuse —
133+
* and the gate would be green because it never fires. ⛔ Do not "simplify"
134+
* the two into one call; `data-model-rules.summary-rollup.test.ts` pins the
135+
* disagreement.
136+
*
137+
* Membership is READ from the spec's own value classes rather than typed out
138+
* here, and from these two specifically:
139+
*
140+
* - `NUMERIC_VALUE_TYPES` is the set that DEFINES the acceptance criterion —
141+
* it is the very membership `valueSchemaFor` consults to answer
142+
* `z.number().finite()`. A type joining that class changes the `summary`
143+
* value contract and this door with it, in one edit.
144+
* - `BOOLEAN_VALUE_TYPES` is admitted on the authority of maintainer ruling
145+
* #11152: booleans aggregate as NUMBERS on every backend with no
146+
* per-aggregate exception, pinned by the spec's own `AGGREGATION_CASES`
147+
* (`min(flag)=0`, `max(flag)=1`, enrolled on six backends) and implemented
148+
* by `driver-sql`'s `int` cast on Postgres (#11635). The answer is a
149+
* number, so it fits.
150+
*
151+
* ⛔ Deliberately NOT read: `NON_TEXT_STORED_VALUE_TYPES`, whose membership is
152+
* these same two classes TODAY. It is defined by a third question — "is the
153+
* stored value never text" (#14079) — and excludes the temporal class for a
154+
* DIALECT reason, not for this one. Composing the union here states why each
155+
* half is in, so a future member of that set cannot widen this door as a side
156+
* effect.
157+
*/
158+
function summaryRollupAnswerFitsColumn(childFieldType: string): boolean {
159+
return NUMERIC_VALUE_TYPES.has(childFieldType) || BOOLEAN_VALUE_TYPES.has(childFieldType);
160+
}
108161
/**
109162
* Field names that give an object a title FACE, for R9
110163
* (`object/missing-name-field`).
@@ -491,6 +544,12 @@ export function lintDataModel(objects: any[]): LintIssue[] {
491544
];
492545
if (!Array.isArray(objects) || objects.length === 0) return issues;
493546

547+
// Index: object name → the object, for resolving a roll-up's child object.
548+
const objectsByName: Record<string, any> = {};
549+
for (const o of objects) {
550+
if (o?.name && !(o.name in objectsByName)) objectsByName[o.name] = o;
551+
}
552+
494553
// Index: parent object name → child relationships pointing at it.
495554
const childrenByParent: Record<string, Array<{ child: any; fieldName: string; def: any }>> = {};
496555
for (const child of objects) {
@@ -574,6 +633,67 @@ export function lintDataModel(objects: any[]): LintIssue[] {
574633
}
575634
}
576635

636+
// R13 — a `min`/`max` roll-up must aggregate a child field whose ANSWER
637+
// fits the column the roll-up is stored into.
638+
//
639+
// `FieldSchema.summaryOperations` admits `min`/`max` over ANY child
640+
// field, and `aggregateSummaryValue` (objectql) returns the driver's
641+
// answer verbatim — only an empty-set fallback stands between the
642+
// backend and the stored value. So an ordinary "latest shipment"
643+
// roll-up, `max` over a `datetime` child field, computes an INSTANT into
644+
// a field whose value contract says finite number. Nothing between
645+
// author and driver correlated the two, so it is refused here, at
646+
// authoring time, at `error` (Prime Directive #12: reject at authoring,
647+
// never tolerate in a consumer).
648+
//
649+
// Scoped to `min`/`max` deliberately: `count` ignores the field
650+
// entirely, and `sum`/`avg` over a non-numeric child is a different
651+
// shape — one the analytics table already refuses — judged elsewhere.
652+
if (type === 'summary') {
653+
const ops = def.summaryOperations;
654+
const fn = ops?.function;
655+
const childName = ops?.object;
656+
const childFieldName = ops?.field;
657+
if (
658+
(fn === 'min' || fn === 'max') &&
659+
typeof childName === 'string' && childName !== '' &&
660+
typeof childFieldName === 'string' && childFieldName !== ''
661+
) {
662+
// SILENCE, never a guess, on anything this pass cannot resolve: a
663+
// child object contributed by another package, a partially-loaded
664+
// stack, or a field name that resolves to no declaration. The spec's
665+
// own aggregate table states the tier — "a consumer that cannot
666+
// resolve a field's type … must NOT call the predicate with a guess;
667+
// 'cannot answer, do not block' is the consumer's tier". A refusal
668+
// fired on an unresolvable model would redden an app for metadata
669+
// this pass simply never saw.
670+
const child = objectsByName[childName];
671+
const childField = child
672+
? fieldEntries(child.fields).find((f) => f.name === childFieldName)
673+
: undefined;
674+
const childType = childField?.def?.type;
675+
if (typeof childType === 'string' && !summaryRollupAnswerFitsColumn(childType)) {
676+
issues.push({
677+
severity: 'error',
678+
rule: 'rollup/non-numeric-aggregand',
679+
message:
680+
`summary field "${obj.name}.${fieldName}" rolls up ` +
681+
`${fn}(${childName}.${childFieldName}), but "${childName}.${childFieldName}" is ` +
682+
`a ${childType} field — ${fn} answers with a value of the CHILD field's own type, ` +
683+
`while a summary field's value contract is a finite number (it is a member of the ` +
684+
`spec's NUMERIC_VALUE_TYPES class), so the answer does not fit the column the ` +
685+
`roll-up is stored in`,
686+
path: `${fieldPath}.summaryOperations.field`,
687+
fix:
688+
`Aggregate a numeric or boolean child field instead (min/max over those answer ` +
689+
`with a number), or use function: 'count' — which reads no value off the field. ` +
690+
`To carry a ${childType} on "${obj.name}", declare a ${childType} field and ` +
691+
`maintain it from a flow; a roll-up cannot store one.`,
692+
});
693+
}
694+
}
695+
}
696+
577697
if (!RELATIONSHIP_TYPES.has(type)) continue;
578698
const parent = refOf(def);
579699

0 commit comments

Comments
 (0)