From 56122292c65dc57f870bb0a2136dbba15e64126a Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Mon, 22 Jun 2026 21:13:54 +0800 Subject: [PATCH] feat(spec,cli): validate dataset measure aggregation by field semantics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- .../utils/validate-widget-bindings.test.ts | 60 +++++++++++++++++++ .../cli/src/utils/validate-widget-bindings.ts | 52 ++++++++++++++++ .../spec/src/data/aggregation-policy.test.ts | 50 ++++++++++++++++ packages/spec/src/data/aggregation-policy.ts | 34 +++++++++++ packages/spec/src/data/index.ts | 4 ++ 5 files changed, 200 insertions(+) create mode 100644 packages/spec/src/data/aggregation-policy.test.ts create mode 100644 packages/spec/src/data/aggregation-policy.ts diff --git a/packages/cli/src/utils/validate-widget-bindings.test.ts b/packages/cli/src/utils/validate-widget-bindings.test.ts index a82e61759e..16949d4594 100644 --- a/packages/cli/src/utils/validate-widget-bindings.test.ts +++ b/packages/cli/src/utils/validate-widget-bindings.test.ts @@ -7,6 +7,7 @@ import { WIDGET_MEASURE_UNKNOWN, CHART_FIELD_UNKNOWN, CHART_CONFIG_MISSING, + MEASURE_AGGREGATE_INCOHERENT, } from './validate-widget-bindings.js'; /** The downstream repro from issue #1719 — dataset with a count AND a sum @@ -320,3 +321,62 @@ describe('validateWidgetBindings (table-count-only, issue #1719)', () => { expect(validateWidgetBindings({ dashboards: [], datasets: [] })).toHaveLength(0); }); }); + +describe('validateWidgetBindings (measure-aggregate-incoherent — rate aggregation)', () => { + /** A dataset whose `probability` measure aggregates a percent field. */ + function crmStack(aggregate: string) { + return { + objects: [{ + name: 'opportunity', + fields: [ + { name: 'amount', type: 'currency' }, + { name: 'probability', type: 'percent' }, + { name: 'stage', type: 'select' }, + ], + }], + datasets: [{ + name: 'opportunity_ds', + object: 'opportunity', + measures: [ + { name: 'count', aggregate: 'count' }, + { name: 'total_amount', aggregate: 'sum', field: 'amount' }, + { name: 'win_probability', aggregate, field: 'probability' }, + ], + dimensions: [{ name: 'stage', field: 'stage' }], + }], + }; + } + + it('warns when a measure SUMs a percentage field', () => { + const findings = validateWidgetBindings(crmStack('sum')); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('warning'); + expect(findings[0].rule).toBe(MEASURE_AGGREGATE_INCOHERENT); + expect(findings[0].where).toContain('opportunity_ds'); + expect(findings[0].where).toContain('win_probability'); + expect(findings[0].path).toBe('datasets[0].measures[2]'); + expect(findings[0].message).toContain('percent field "probability"'); + expect(findings[0].hint).toMatch(/avg/i); + }); + + it('is clean when the percentage field is AVG’d', () => { + expect(validateWidgetBindings(crmStack('avg'))).toHaveLength(0); + }); + + it('also flags count_distinct of a percentage field', () => { + const findings = validateWidgetBindings(crmStack('count_distinct')); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(MEASURE_AGGREGATE_INCOHERENT); + }); + + it('does not flag SUM of a currency/amount field', () => { + // total_amount sums `amount` (currency) — additive, perfectly fine. + expect(validateWidgetBindings(crmStack('avg')).filter((f) => f.rule === MEASURE_AGGREGATE_INCOHERENT)).toHaveLength(0); + }); + + it('cannot judge — and never false-positives — without the object field types', () => { + const stack = crmStack('sum'); + delete (stack as { objects?: unknown }).objects; + expect(validateWidgetBindings(stack)).toHaveLength(0); + }); +}); diff --git a/packages/cli/src/utils/validate-widget-bindings.ts b/packages/cli/src/utils/validate-widget-bindings.ts index 056c633bb5..9cda00ff3e 100644 --- a/packages/cli/src/utils/validate-widget-bindings.ts +++ b/packages/cli/src/utils/validate-widget-bindings.ts @@ -1,5 +1,7 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +import { isIncoherentAggregate } from '@objectstack/spec/data'; + /** * Build-time dashboard widget binding diagnostics (issues #1719, #1721). * @@ -37,6 +39,11 @@ * means the author wanted a per-record listing, which is not an * analytics dataset at all (model it as an object-bound ListView, * ADR-0017). Evaluated on the WIDGET's binding, not the dataset. + * - `measure-aggregate-incoherent` — a dataset measure aggregates its field + * in a way that produces a meaningless number: today, SUM (or + * `count_distinct`) of a `percent`/rate field, whose total routinely + * exceeds 100%. Rates must AVG. Checked once per dataset (independent of + * any widget) when the bound object's field types are known. * * Warnings can be deliberately suppressed per widget via * `suppressWarnings: ['']`; errors cannot — they describe a @@ -49,6 +56,7 @@ export const WIDGET_MEASURE_UNKNOWN = 'widget-measure-unknown'; export const CHART_FIELD_UNKNOWN = 'chart-field-unknown'; export const CHART_CONFIG_MISSING = 'chart-config-missing'; export const TABLE_COUNT_ONLY = 'table-count-only'; +export const MEASURE_AGGREGATE_INCOHERENT = 'measure-aggregate-incoherent'; export type WidgetBindingSeverity = 'error' | 'warning'; @@ -159,6 +167,50 @@ export function validateWidgetBindings(stack: AnyRec): WidgetBindingFinding[] { if (typeof ds.name === 'string') datasets.set(ds.name, ds); } + // ── (0) dataset measures aggregate their field coherently ── + // A measure that SUMs a percentage/rate field produces a meaningless total + // (it can exceed 100%); rates must AVG. This is a dataset-level defect (it + // does not depend on any widget), so it is checked once over every dataset + // whose object's field types are known. Advisory — the page still renders. + const objectFieldTypes = new Map>(); + for (const o of asArray(stack.objects)) { + if (typeof o.name !== 'string') continue; + const fm = new Map(); + for (const f of asArray(o.fields)) { + if (typeof f.name === 'string' && typeof f.type === 'string') fm.set(f.name, f.type); + } + objectFieldTypes.set(o.name, fm); + } + const datasetList = asArray(stack.datasets); + for (let i = 0; i < datasetList.length; i++) { + const ds = datasetList[i]; + const fieldTypes = typeof ds.object === 'string' ? objectFieldTypes.get(ds.object) : undefined; + if (!fieldTypes) continue; // cannot judge without the object's field types + const dsMeasures = asArray(ds.measures); + for (let k = 0; k < dsMeasures.length; k++) { + const m = dsMeasures[k]; + const field = typeof m.field === 'string' ? m.field : undefined; + const aggregate = typeof m.aggregate === 'string' ? m.aggregate : undefined; + if (!field || !aggregate) continue; // count(*) and underivable measures are fine + const ftype = fieldTypes.get(field); + if (ftype && isIncoherentAggregate(aggregate, ftype)) { + findings.push({ + severity: 'warning', + rule: MEASURE_AGGREGATE_INCOHERENT, + where: `dataset "${typeof ds.name === 'string' ? ds.name : `(dataset ${i})`}" › measure "${typeof m.name === 'string' ? m.name : `(measure ${k})`}"`, + path: `datasets[${i}].measures[${k}]`, + message: + `measure "${m.name}" applies ${aggregate} to ${ftype} field "${field}" — ` + + `summed percentages are meaningless (they can exceed 100%).`, + hint: + `Use aggregate "avg" for percentage/rate fields (or "count" of records). ` + + `If a running total is genuinely intended, suppress with: ` + + `suppressWarnings: ['${MEASURE_AGGREGATE_INCOHERENT}'] on the measure.`, + }); + } + } + } + const dashboards = asArray(stack.dashboards); for (let i = 0; i < dashboards.length; i++) { const dash = dashboards[i]; diff --git a/packages/spec/src/data/aggregation-policy.test.ts b/packages/spec/src/data/aggregation-policy.test.ts new file mode 100644 index 0000000000..054eacdef1 --- /dev/null +++ b/packages/spec/src/data/aggregation-policy.test.ts @@ -0,0 +1,50 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { defaultAggregateFor, isIncoherentAggregate, MEASURE_FIELD_TYPES } from './aggregation-policy'; + +describe('defaultAggregateFor', () => { + it('SUMs additive amounts', () => { + expect(defaultAggregateFor('currency')).toBe('sum'); + expect(defaultAggregateFor('number')).toBe('sum'); + }); + + it('AVGs rates (percent)', () => { + expect(defaultAggregateFor('percent')).toBe('avg'); + }); + + it('defaults to sum for unknown/undefined types', () => { + expect(defaultAggregateFor(undefined)).toBe('sum'); + expect(defaultAggregateFor('text')).toBe('sum'); + }); +}); + +describe('isIncoherentAggregate', () => { + it('flags SUM and count_distinct of a percentage', () => { + expect(isIncoherentAggregate('sum', 'percent')).toBe(true); + expect(isIncoherentAggregate('count_distinct', 'percent')).toBe(true); + }); + + it('allows AVG / min / max / count of a percentage', () => { + expect(isIncoherentAggregate('avg', 'percent')).toBe(false); + expect(isIncoherentAggregate('min', 'percent')).toBe(false); + expect(isIncoherentAggregate('max', 'percent')).toBe(false); + expect(isIncoherentAggregate('count', 'percent')).toBe(false); + }); + + it('allows SUM of additive amounts', () => { + expect(isIncoherentAggregate('sum', 'currency')).toBe(false); + expect(isIncoherentAggregate('sum', 'number')).toBe(false); + }); + + it('never false-positives on an unknown field type', () => { + expect(isIncoherentAggregate('sum', undefined)).toBe(false); + expect(isIncoherentAggregate('sum', 'text')).toBe(false); + }); +}); + +describe('MEASURE_FIELD_TYPES', () => { + it('covers the numeric measure-backing field types', () => { + expect([...MEASURE_FIELD_TYPES].sort()).toEqual(['currency', 'number', 'percent']); + }); +}); diff --git a/packages/spec/src/data/aggregation-policy.ts b/packages/spec/src/data/aggregation-policy.ts new file mode 100644 index 0000000000..27ebe29f85 --- /dev/null +++ b/packages/spec/src/data/aggregation-policy.ts @@ -0,0 +1,34 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Field → aggregation semantics. The single source of truth for "how should a + * numeric field be aggregated into a measure", shared by authoring (dataset + * derivation) and validation (build-time coherence checks) so the two cannot + * drift. + * + * The motivating defect: additive amounts (currency/number) SUM correctly, but + * a RATE — a `percent` field such as a win-probability or conversion rate — must + * AVG. Summing percentages is meaningless: the total routinely exceeds 100%. + */ + +/** Numeric field types that can back a value measure. */ +export const MEASURE_FIELD_TYPES: ReadonlySet = new Set(['number', 'currency', 'percent']); + +/** + * The aggregation a derived value measure should use for a field of this type: + * rates (`percent`) AVG, every other additive amount SUMs. + */ +export function defaultAggregateFor(fieldType: string | undefined): 'sum' | 'avg' { + return fieldType === 'percent' ? 'avg' : 'sum'; +} + +/** + * Is applying `aggregate` to a field of `fieldType` semantically incoherent? + * True only for cases that produce a meaningless number — today: SUM (or + * COUNT_DISTINCT) of a percentage/rate. Returns false when the field type is + * unknown (cannot judge) so callers never raise a false positive. + */ +export function isIncoherentAggregate(aggregate: string, fieldType: string | undefined): boolean { + if (fieldType === 'percent' && (aggregate === 'sum' || aggregate === 'count_distinct')) return true; + return false; +} diff --git a/packages/spec/src/data/index.ts b/packages/spec/src/data/index.ts index 48a4d9d251..84afb283be 100644 --- a/packages/spec/src/data/index.ts +++ b/packages/spec/src/data/index.ts @@ -39,6 +39,10 @@ export * from './external-catalog.zod'; // Analytics Protocol (Semantic Layer) export * from './analytics.zod'; +// Field → aggregation semantics (rates AVG, amounts SUM) — shared by authoring +// and build-time coherence validation. +export * from './aggregation-policy'; + // Feed & Activity Protocol export * from './feed.zod';