Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions packages/cli/src/utils/validate-widget-bindings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
});
});
52 changes: 52 additions & 0 deletions packages/cli/src/utils/validate-widget-bindings.ts
Original file line number Diff line number Diff line change
@@ -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).
*
Expand Down Expand Up @@ -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: ['<rule-id>']`; errors cannot — they describe a
Expand All @@ -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';

Expand Down Expand Up @@ -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<string, Map<string, string>>();
for (const o of asArray(stack.objects)) {
if (typeof o.name !== 'string') continue;
const fm = new Map<string, string>();
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];
Expand Down
50 changes: 50 additions & 0 deletions packages/spec/src/data/aggregation-policy.test.ts
Original file line number Diff line number Diff line change
@@ -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']);
});
});
34 changes: 34 additions & 0 deletions packages/spec/src/data/aggregation-policy.ts
Original file line number Diff line number Diff line change
@@ -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<string> = 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;
}
4 changes: 4 additions & 0 deletions packages/spec/src/data/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down
Loading