Skip to content

Commit 97ba590

Browse files
feat(spec,cli): validate dataset measure aggregation by field semantics (#2208)
A measure that SUMs a percentage/rate field produces a meaningless total (it can exceed 100%); rates must AVG. Authoring tools and `os validate` had no notion of this, so a hand-authored — or AI-authored — dataset could summon a "win-probability" measure as SUM and pass every check. - @objectstack/spec/data: new aggregation-policy — `defaultAggregateFor` (rates AVG, amounts SUM) and `isIncoherentAggregate`. The single source of truth for field→aggregation semantics, shared by authoring (dataset derivation) and validation so the two cannot drift. - @objectstack/cli validateWidgetBindings: new `measure-aggregate-incoherent` advisory — checks every dataset's measures against its object's field types and flags SUM/count_distinct of a percent field. Runs at validate/compile/build through the existing widget-binding pass; never false-positives when the object's field types are unknown. Tests: spec policy unit tests + cli validator cases (sum-on-percent flagged, avg clean, currency-sum clean, no-objects no-op, count_distinct flagged). Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com>
1 parent 94d2161 commit 97ba590

5 files changed

Lines changed: 200 additions & 0 deletions

File tree

packages/cli/src/utils/validate-widget-bindings.test.ts

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
WIDGET_MEASURE_UNKNOWN,
88
CHART_FIELD_UNKNOWN,
99
CHART_CONFIG_MISSING,
10+
MEASURE_AGGREGATE_INCOHERENT,
1011
} from './validate-widget-bindings.js';
1112

1213
/** The downstream repro from issue #1719 — dataset with a count AND a sum
@@ -320,3 +321,62 @@ describe('validateWidgetBindings (table-count-only, issue #1719)', () => {
320321
expect(validateWidgetBindings({ dashboards: [], datasets: [] })).toHaveLength(0);
321322
});
322323
});
324+
325+
describe('validateWidgetBindings (measure-aggregate-incoherent — rate aggregation)', () => {
326+
/** A dataset whose `probability` measure aggregates a percent field. */
327+
function crmStack(aggregate: string) {
328+
return {
329+
objects: [{
330+
name: 'opportunity',
331+
fields: [
332+
{ name: 'amount', type: 'currency' },
333+
{ name: 'probability', type: 'percent' },
334+
{ name: 'stage', type: 'select' },
335+
],
336+
}],
337+
datasets: [{
338+
name: 'opportunity_ds',
339+
object: 'opportunity',
340+
measures: [
341+
{ name: 'count', aggregate: 'count' },
342+
{ name: 'total_amount', aggregate: 'sum', field: 'amount' },
343+
{ name: 'win_probability', aggregate, field: 'probability' },
344+
],
345+
dimensions: [{ name: 'stage', field: 'stage' }],
346+
}],
347+
};
348+
}
349+
350+
it('warns when a measure SUMs a percentage field', () => {
351+
const findings = validateWidgetBindings(crmStack('sum'));
352+
expect(findings).toHaveLength(1);
353+
expect(findings[0].severity).toBe('warning');
354+
expect(findings[0].rule).toBe(MEASURE_AGGREGATE_INCOHERENT);
355+
expect(findings[0].where).toContain('opportunity_ds');
356+
expect(findings[0].where).toContain('win_probability');
357+
expect(findings[0].path).toBe('datasets[0].measures[2]');
358+
expect(findings[0].message).toContain('percent field "probability"');
359+
expect(findings[0].hint).toMatch(/avg/i);
360+
});
361+
362+
it('is clean when the percentage field is AVG’d', () => {
363+
expect(validateWidgetBindings(crmStack('avg'))).toHaveLength(0);
364+
});
365+
366+
it('also flags count_distinct of a percentage field', () => {
367+
const findings = validateWidgetBindings(crmStack('count_distinct'));
368+
expect(findings).toHaveLength(1);
369+
expect(findings[0].rule).toBe(MEASURE_AGGREGATE_INCOHERENT);
370+
});
371+
372+
it('does not flag SUM of a currency/amount field', () => {
373+
// total_amount sums `amount` (currency) — additive, perfectly fine.
374+
expect(validateWidgetBindings(crmStack('avg')).filter((f) => f.rule === MEASURE_AGGREGATE_INCOHERENT)).toHaveLength(0);
375+
});
376+
377+
it('cannot judge — and never false-positives — without the object field types', () => {
378+
const stack = crmStack('sum');
379+
delete (stack as { objects?: unknown }).objects;
380+
expect(validateWidgetBindings(stack)).toHaveLength(0);
381+
});
382+
});

packages/cli/src/utils/validate-widget-bindings.ts

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
22

3+
import { isIncoherentAggregate } from '@objectstack/spec/data';
4+
35
/**
46
* Build-time dashboard widget binding diagnostics (issues #1719, #1721).
57
*
@@ -37,6 +39,11 @@
3739
* means the author wanted a per-record listing, which is not an
3840
* analytics dataset at all (model it as an object-bound ListView,
3941
* ADR-0017). Evaluated on the WIDGET's binding, not the dataset.
42+
* - `measure-aggregate-incoherent` — a dataset measure aggregates its field
43+
* in a way that produces a meaningless number: today, SUM (or
44+
* `count_distinct`) of a `percent`/rate field, whose total routinely
45+
* exceeds 100%. Rates must AVG. Checked once per dataset (independent of
46+
* any widget) when the bound object's field types are known.
4047
*
4148
* Warnings can be deliberately suppressed per widget via
4249
* `suppressWarnings: ['<rule-id>']`; errors cannot — they describe a
@@ -49,6 +56,7 @@ export const WIDGET_MEASURE_UNKNOWN = 'widget-measure-unknown';
4956
export const CHART_FIELD_UNKNOWN = 'chart-field-unknown';
5057
export const CHART_CONFIG_MISSING = 'chart-config-missing';
5158
export const TABLE_COUNT_ONLY = 'table-count-only';
59+
export const MEASURE_AGGREGATE_INCOHERENT = 'measure-aggregate-incoherent';
5260

5361
export type WidgetBindingSeverity = 'error' | 'warning';
5462

@@ -159,6 +167,50 @@ export function validateWidgetBindings(stack: AnyRec): WidgetBindingFinding[] {
159167
if (typeof ds.name === 'string') datasets.set(ds.name, ds);
160168
}
161169

170+
// ── (0) dataset measures aggregate their field coherently ──
171+
// A measure that SUMs a percentage/rate field produces a meaningless total
172+
// (it can exceed 100%); rates must AVG. This is a dataset-level defect (it
173+
// does not depend on any widget), so it is checked once over every dataset
174+
// whose object's field types are known. Advisory — the page still renders.
175+
const objectFieldTypes = new Map<string, Map<string, string>>();
176+
for (const o of asArray(stack.objects)) {
177+
if (typeof o.name !== 'string') continue;
178+
const fm = new Map<string, string>();
179+
for (const f of asArray(o.fields)) {
180+
if (typeof f.name === 'string' && typeof f.type === 'string') fm.set(f.name, f.type);
181+
}
182+
objectFieldTypes.set(o.name, fm);
183+
}
184+
const datasetList = asArray(stack.datasets);
185+
for (let i = 0; i < datasetList.length; i++) {
186+
const ds = datasetList[i];
187+
const fieldTypes = typeof ds.object === 'string' ? objectFieldTypes.get(ds.object) : undefined;
188+
if (!fieldTypes) continue; // cannot judge without the object's field types
189+
const dsMeasures = asArray(ds.measures);
190+
for (let k = 0; k < dsMeasures.length; k++) {
191+
const m = dsMeasures[k];
192+
const field = typeof m.field === 'string' ? m.field : undefined;
193+
const aggregate = typeof m.aggregate === 'string' ? m.aggregate : undefined;
194+
if (!field || !aggregate) continue; // count(*) and underivable measures are fine
195+
const ftype = fieldTypes.get(field);
196+
if (ftype && isIncoherentAggregate(aggregate, ftype)) {
197+
findings.push({
198+
severity: 'warning',
199+
rule: MEASURE_AGGREGATE_INCOHERENT,
200+
where: `dataset "${typeof ds.name === 'string' ? ds.name : `(dataset ${i})`}" › measure "${typeof m.name === 'string' ? m.name : `(measure ${k})`}"`,
201+
path: `datasets[${i}].measures[${k}]`,
202+
message:
203+
`measure "${m.name}" applies ${aggregate} to ${ftype} field "${field}" — ` +
204+
`summed percentages are meaningless (they can exceed 100%).`,
205+
hint:
206+
`Use aggregate "avg" for percentage/rate fields (or "count" of records). ` +
207+
`If a running total is genuinely intended, suppress with: ` +
208+
`suppressWarnings: ['${MEASURE_AGGREGATE_INCOHERENT}'] on the measure.`,
209+
});
210+
}
211+
}
212+
}
213+
162214
const dashboards = asArray(stack.dashboards);
163215
for (let i = 0; i < dashboards.length; i++) {
164216
const dash = dashboards[i];
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, it, expect } from 'vitest';
4+
import { defaultAggregateFor, isIncoherentAggregate, MEASURE_FIELD_TYPES } from './aggregation-policy';
5+
6+
describe('defaultAggregateFor', () => {
7+
it('SUMs additive amounts', () => {
8+
expect(defaultAggregateFor('currency')).toBe('sum');
9+
expect(defaultAggregateFor('number')).toBe('sum');
10+
});
11+
12+
it('AVGs rates (percent)', () => {
13+
expect(defaultAggregateFor('percent')).toBe('avg');
14+
});
15+
16+
it('defaults to sum for unknown/undefined types', () => {
17+
expect(defaultAggregateFor(undefined)).toBe('sum');
18+
expect(defaultAggregateFor('text')).toBe('sum');
19+
});
20+
});
21+
22+
describe('isIncoherentAggregate', () => {
23+
it('flags SUM and count_distinct of a percentage', () => {
24+
expect(isIncoherentAggregate('sum', 'percent')).toBe(true);
25+
expect(isIncoherentAggregate('count_distinct', 'percent')).toBe(true);
26+
});
27+
28+
it('allows AVG / min / max / count of a percentage', () => {
29+
expect(isIncoherentAggregate('avg', 'percent')).toBe(false);
30+
expect(isIncoherentAggregate('min', 'percent')).toBe(false);
31+
expect(isIncoherentAggregate('max', 'percent')).toBe(false);
32+
expect(isIncoherentAggregate('count', 'percent')).toBe(false);
33+
});
34+
35+
it('allows SUM of additive amounts', () => {
36+
expect(isIncoherentAggregate('sum', 'currency')).toBe(false);
37+
expect(isIncoherentAggregate('sum', 'number')).toBe(false);
38+
});
39+
40+
it('never false-positives on an unknown field type', () => {
41+
expect(isIncoherentAggregate('sum', undefined)).toBe(false);
42+
expect(isIncoherentAggregate('sum', 'text')).toBe(false);
43+
});
44+
});
45+
46+
describe('MEASURE_FIELD_TYPES', () => {
47+
it('covers the numeric measure-backing field types', () => {
48+
expect([...MEASURE_FIELD_TYPES].sort()).toEqual(['currency', 'number', 'percent']);
49+
});
50+
});
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Field → aggregation semantics. The single source of truth for "how should a
5+
* numeric field be aggregated into a measure", shared by authoring (dataset
6+
* derivation) and validation (build-time coherence checks) so the two cannot
7+
* drift.
8+
*
9+
* The motivating defect: additive amounts (currency/number) SUM correctly, but
10+
* a RATE — a `percent` field such as a win-probability or conversion rate — must
11+
* AVG. Summing percentages is meaningless: the total routinely exceeds 100%.
12+
*/
13+
14+
/** Numeric field types that can back a value measure. */
15+
export const MEASURE_FIELD_TYPES: ReadonlySet<string> = new Set(['number', 'currency', 'percent']);
16+
17+
/**
18+
* The aggregation a derived value measure should use for a field of this type:
19+
* rates (`percent`) AVG, every other additive amount SUMs.
20+
*/
21+
export function defaultAggregateFor(fieldType: string | undefined): 'sum' | 'avg' {
22+
return fieldType === 'percent' ? 'avg' : 'sum';
23+
}
24+
25+
/**
26+
* Is applying `aggregate` to a field of `fieldType` semantically incoherent?
27+
* True only for cases that produce a meaningless number — today: SUM (or
28+
* COUNT_DISTINCT) of a percentage/rate. Returns false when the field type is
29+
* unknown (cannot judge) so callers never raise a false positive.
30+
*/
31+
export function isIncoherentAggregate(aggregate: string, fieldType: string | undefined): boolean {
32+
if (fieldType === 'percent' && (aggregate === 'sum' || aggregate === 'count_distinct')) return true;
33+
return false;
34+
}

packages/spec/src/data/index.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,10 @@ export * from './external-catalog.zod';
3939
// Analytics Protocol (Semantic Layer)
4040
export * from './analytics.zod';
4141

42+
// Field → aggregation semantics (rates AVG, amounts SUM) — shared by authoring
43+
// and build-time coherence validation.
44+
export * from './aggregation-policy';
45+
4246
// Feed & Activity Protocol
4347
export * from './feed.zod';
4448

0 commit comments

Comments
 (0)