From 90e93ce21085ccc523d78c6f0c8e5707f3c88c34 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 16:33:11 +0000 Subject: [PATCH] fix(charts): a fieldless count aggregate keyed its value column undefined (framework#3701) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit framework#3701 pinned down what an OBJECT-bound chart aggregate names its result columns: the raw field names it was given — `groupBy` for the category, `field` for the value, with no `sum_`-style decoration (unlike a dataset measure) — plus the literal `count` when a `count` omits `field`, which is the alias the engine projects COUNT(*) under. `os validate` now lints page sources against that convention, so every path that builds these rows has to honour it exactly. Three of the four did. The odd one out was `count`, the one function that may legitimately omit `field`, because every row builder read `params.field` directly: `aggregateRecords` and `ObjectDataSource.aggregateClientSide` emitted a column literally named `undefined`, and the legacy analytics path remapped the server's `count` measure onto that key and deleted the original — throwing away the value the server did return. All of them now resolve the column through one helper (`aggregateValueKey`), so a fieldless count lands under `count`. The comparison-overlay column derives from the same key (`count__comparison`, not `undefined__comparison`), and `aggregate.field` is typed optional to match the spec's ChartAggregateSchema. Charts that name a field are unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UMHZBHTjH4rw8xmDYirFC7 --- .../objectchart-fieldless-count-column.md | 30 +++++++++ packages/data-objectstack/src/index.ts | 38 ++++++++--- ...ObjectChart.aggregateResultColumns.test.ts | 67 +++++++++++++++++++ packages/plugin-charts/src/ObjectChart.tsx | 58 +++++++++++----- 4 files changed, 168 insertions(+), 25 deletions(-) create mode 100644 .changeset/objectchart-fieldless-count-column.md create mode 100644 packages/plugin-charts/src/ObjectChart.aggregateResultColumns.test.ts diff --git a/.changeset/objectchart-fieldless-count-column.md b/.changeset/objectchart-fieldless-count-column.md new file mode 100644 index 000000000..8571f0c65 --- /dev/null +++ b/.changeset/objectchart-fieldless-count-column.md @@ -0,0 +1,30 @@ +--- +"@object-ui/plugin-charts": patch +"@object-ui/data-objectstack": patch +--- + +fix(charts): a fieldless `count` aggregate keyed its value column `undefined`, so the chart plotted nothing (framework#3701) + +framework#3701 pinned down what an OBJECT-bound chart aggregate names its result +columns — the raw field names it was given (`groupBy` for the category, `field` +for the value; no `sum_`-style decoration, unlike a dataset measure), plus the +literal `count` when a `count` omits `field`, which is the alias the engine +projects `COUNT(*)` under. `os validate` now lints page sources against that +convention, so the paths that build these rows have to honour it exactly. + +Three of the four did. The odd one out was `count` — the one function that may +legitimately omit `field` — because every row builder read `params.field` +directly: + +- `aggregateRecords` / `ObjectDataSource.aggregateClientSide` emitted + `{ [groupBy]: key, [undefined]: value }`, i.e. a column literally named + `undefined` that no axis binding could ever name; +- the legacy analytics path was worse: it remapped the server's `count` measure + onto `params.field` and **deleted** the original key, so the value the server + did return was thrown away before the chart saw it. + +All of them now resolve the column through one helper (`aggregateValueKey`) so a +fieldless count lands under `count`, matching the framework contract. The +comparison-overlay column is derived from the same key (`count__comparison` +instead of `undefined__comparison`), and `aggregate.field` is typed optional to +match the spec's `ChartAggregateSchema`. Charts that name a field are unchanged. diff --git a/packages/data-objectstack/src/index.ts b/packages/data-objectstack/src/index.ts index af9e8a318..5643dc75d 100644 --- a/packages/data-objectstack/src/index.ts +++ b/packages/data-objectstack/src/index.ts @@ -2603,6 +2603,12 @@ export class ObjectStackAdapter implements DataSource { const measureName = params.function === 'count' ? 'count' : `${params.field}_${params.function}`; + // The column the caller expects the value under — the raw `field`, or the + // literal `count` when a count names no field (framework#3701). Reading + // `params.field` directly here keyed the row `undefined` for a fieldless + // count and deleted the `count` the server sent, so the chart plotted + // nothing. + const valueKey = this.aggregateValueKey(params); const payload: Record = { cube: resource, @@ -2634,7 +2640,7 @@ export class ObjectStackAdapter implements DataSource { const measureMissing = rawRows.length > 0 && rawRows.every((row: any) => { if (row == null) return true; if (measureName in row && row[measureName] != null) return false; - if (params.field in row && row[params.field] != null) return false; + if (valueKey in row && row[valueKey] != null) return false; return true; }); if (measureMissing) { @@ -2644,14 +2650,15 @@ export class ObjectStackAdapter implements DataSource { return this.aggregateClientSide(records, params); } - // Map measure keys back to the original field name so that consumers - // (ObjectChart, DashboardRenderer, etc.) can access values by field name. - // This includes count → field (e.g. 'count' → 'amount') to match the - // output format of aggregateClientSide() which always uses params.field. + // Map measure keys back to the object-bound result column so consumers + // (ObjectChart, DashboardRenderer, …) read values by the name the + // convention promises: `field`, or `count` for a fieldless count + // (framework#3701). This includes count → field (e.g. 'count' → + // 'amount'), matching aggregateClientSide()'s output. return rawRows.map((row: any) => { const mapped = { ...row }; - if (measureName !== params.field && measureName in mapped) { - mapped[params.field] = mapped[measureName]; + if (measureName !== valueKey && measureName in mapped) { + mapped[valueKey] = mapped[measureName]; delete mapped[measureName]; } return mapped; @@ -2776,8 +2783,19 @@ export class ObjectStackAdapter implements DataSource { } /** Client-side aggregation fallback */ - private aggregateClientSide(records: any[], params: { field: string; function: string; groupBy: string }): any[] { + /** + * The result column an object-bound `aggregate` projects its value under + * (framework#3701, `chartAggregateValueKey` in `@objectstack/spec/ui`): the + * raw `field` name — no `sum_`-style decoration, unlike a dataset measure — + * or the literal `count` when a count names no field. + */ + private aggregateValueKey(params: { field?: string; function?: string }): string { + return params.field || params.function || 'count'; + } + + private aggregateClientSide(records: any[], params: { field?: string; function: string; groupBy: string }): any[] { const { field, function: aggFn, groupBy } = params; + const valueKey = this.aggregateValueKey(params); const groups: Record = {}; for (const record of records) { @@ -2787,7 +2805,7 @@ export class ObjectStackAdapter implements DataSource { } return Object.entries(groups).map(([key, group]) => { - const values = group.map(r => Number(r[field]) || 0); + const values = field ? group.map(r => Number(r[field]) || 0) : []; let result: number; switch (aggFn) { @@ -2798,7 +2816,7 @@ export class ObjectStackAdapter implements DataSource { case 'sum': default: result = values.reduce((a, b) => a + b, 0); break; } - return { [groupBy]: key, [field]: result }; + return { [groupBy]: key, [valueKey]: result }; }); } diff --git a/packages/plugin-charts/src/ObjectChart.aggregateResultColumns.test.ts b/packages/plugin-charts/src/ObjectChart.aggregateResultColumns.test.ts new file mode 100644 index 000000000..374075e92 --- /dev/null +++ b/packages/plugin-charts/src/ObjectChart.aggregateResultColumns.test.ts @@ -0,0 +1,67 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * The object-bound aggregate RESULT-COLUMN convention (framework#3701). + * + * An object-bound chart (`objectName` + an inline `aggregate`) returns rows + * keyed by the RAW FIELD NAMES it was given — `groupBy` for the category, + * `field` for the value — with NO `sum_`-style decoration. That is the opposite + * of a dataset, whose rows are keyed by the declared measure `name`, and it is + * what `xAxisKey` / `series[].dataKey` resolve against. The framework now pins + * the rule (`chartAggregateResultKeys` in `@objectstack/spec/ui`) and lints + * page sources against it, so the row builders here have to honour it exactly. + * + * Regression: `count` is the one function that may omit `field`, and every + * builder read `params.field` directly — so a fieldless count keyed its value + * `undefined` and the chart plotted nothing. + */ +import { describe, it, expect } from 'vitest'; +import { aggregateRecords, aggregateValueKey } from './ObjectChart'; + +const rows = [ + { status: 'open', total: 100 }, + { status: 'open', total: 50 }, + { status: 'paid', total: 20 }, +]; + +describe('aggregateValueKey', () => { + it('is the raw field — never a decorated measure name', () => { + expect(aggregateValueKey({ field: 'total', function: 'sum' })).toBe('total'); + }); + + it('is the literal "count" when a count names no field', () => { + expect(aggregateValueKey({ function: 'count' })).toBe('count'); + }); + + it('prefers an explicit field even for count', () => { + expect(aggregateValueKey({ field: 'total', function: 'count' })).toBe('total'); + }); +}); + +describe('aggregateRecords — result columns', () => { + it('keys rows by groupBy and the raw field', () => { + expect(aggregateRecords(rows, { field: 'total', function: 'sum', groupBy: 'status' })).toEqual([ + { status: 'open', total: 150 }, + { status: 'paid', total: 20 }, + ]); + }); + + it('keys a fieldless count under "count", not undefined', () => { + const out = aggregateRecords(rows, { function: 'count', groupBy: 'status' }); + expect(out).toEqual([ + { status: 'open', count: 2 }, + { status: 'paid', count: 1 }, + ]); + // The pre-fix shape: a column literally named "undefined", which no axis + // binding could ever name. + expect(Object.keys(out[0])).not.toContain('undefined'); + }); + + it('still counts rows when a field IS named', () => { + expect(aggregateRecords(rows, { field: 'total', function: 'count', groupBy: 'status' })).toEqual([ + { status: 'open', total: 2 }, + { status: 'paid', total: 1 }, + ]); + }); +}); diff --git a/packages/plugin-charts/src/ObjectChart.tsx b/packages/plugin-charts/src/ObjectChart.tsx index 4f5539780..c6dfcf193 100644 --- a/packages/plugin-charts/src/ObjectChart.tsx +++ b/packages/plugin-charts/src/ObjectChart.tsx @@ -15,6 +15,23 @@ export function humanizeLabel(value: string): string { return value.replace(/[_-]/g, ' ').replace(/\b\w/g, c => c.toUpperCase()); } +/** + * The result column an object-bound `aggregate` projects its value under + * (framework#3701, `chartAggregateValueKey` in `@objectstack/spec/ui`). + * + * The raw `field` name — an object-bound aggregate does NOT decorate it the way + * a dataset measure is named (`sum_amount`). Only `count` may omit `field`, and + * it then lands under the literal `'count'`, which is the alias the engine + * projects `COUNT(*)` under. Exported so every path that builds these rows + * agrees on one key instead of each re-deriving it. + */ +export function aggregateValueKey(aggregate: { field?: string; function?: string }): string { + return aggregate.field || aggregate.function || 'count'; +} + +/** Suffix the previous-window value carries under a compareTo overlay. */ +export const COMPARISON_SUFFIX = '__comparison'; + /** * Client-side aggregation for fetched records. * Groups records by `groupBy` field and applies the aggregation function @@ -22,9 +39,10 @@ export function humanizeLabel(value: string): string { */ export function aggregateRecords( records: any[], - aggregate: { field: string; function: string; groupBy: string } + aggregate: { field?: string; function: string; groupBy: string } ): any[] { const { field, function: aggFn, groupBy } = aggregate; + const valueKey = aggregateValueKey(aggregate); const groups: Record = {}; for (const record of records) { @@ -34,7 +52,7 @@ export function aggregateRecords( } return Object.entries(groups).map(([key, group]) => { - const values = group.map(r => Number(r[field]) || 0); + const values = field ? group.map(r => Number(r[field]) || 0) : []; let result: number; switch (aggFn) { @@ -56,7 +74,7 @@ export function aggregateRecords( break; } - return { [groupBy]: key, [field]: result }; + return { [groupBy]: key, [valueKey]: result }; }); } @@ -369,8 +387,9 @@ export const ObjectChart = (props: any) => { const aggField = schema.aggregate.field; const aggFn = schema.aggregate.function; // Project the measure under its plain field name so downstream - // (xAxisKey + series.dataKey lookups) finds it unchanged. - const alias = aggField || aggFn; + // (xAxisKey + series.dataKey lookups) finds it unchanged — the + // object-bound result-column convention (framework#3701). + const alias = aggregateValueKey(schema.aggregate); // For `count`, omit `field` so the engine emits `count(*)` / // `COUNT(*)`. The upstream dashboard wiring defaults `field: 'value'` // for charts without an explicit valueField, which crashes on SQL @@ -479,17 +498,22 @@ export const ObjectChart = (props: any) => { if (wantsComparison && comparisonRows.length > 0 && schema.aggregate) { const aggField = schema.aggregate.field; const aggFn = schema.aggregate.function; + // The column this aggregate projects its value under — `field`, + // or `count` for a fieldless count (framework#3701). + const valueKey = aggregateValueKey(schema.aggregate); const readValue = (row: Record): number | null => { if (row == null) return null; - const suffixed = `${aggField}_${aggFn}`; - if (suffixed in row) return Number(row[suffixed]); - if (aggFn === 'count' && `${aggField}_count` in row) return Number(row[`${aggField}_count`]); - if (aggField in row) return Number(row[aggField]); + if (aggField) { + const suffixed = `${aggField}_${aggFn}`; + if (suffixed in row) return Number(row[suffixed]); + if (aggFn === 'count' && `${aggField}_count` in row) return Number(row[`${aggField}_count`]); + } + if (valueKey in row) return Number(row[valueKey]); if ('value' in row) return Number(row.value); if ('count' in row) return Number(row.count); return null; }; - const comparisonKey = `${aggField}__comparison`; + const comparisonKey = `${valueKey}${COMPARISON_SUFFIX}`; const gb = groupByField; if (gb && data.some((r: any) => r[gb] != null) && comparisonRows.some((r: any) => r[gb] != null)) { const cmpByKey = new Map(); @@ -601,16 +625,20 @@ export const ObjectChart = (props: any) => { // for a supported chart type, also synthesize a second series so the // chart implementation renders the comparison overlay (dashed / muted). const compareToConfig: CompareToConfig | undefined = (schema as any).compareTo; + // The result column this aggregate projects its value under, and the column + // the comparison overlay arrives in (framework#3701). + const valueKey = schema.aggregate ? aggregateValueKey(schema.aggregate) : undefined; + const comparisonKey = valueKey ? `${valueKey}${COMPARISON_SUFFIX}` : undefined; const enableComparisonSeries = !!compareToConfig && supportsCompareTo(schema.chartType) && - !!schema.aggregate && - finalData.some((row: Record) => row[`${schema.aggregate!.field}__comparison`] != null); + !!comparisonKey && + finalData.some((row: Record) => row[comparisonKey] != null); const augmentedSeries = useMemo(() => { const existing = Array.isArray((schema as any).series) ? (schema as any).series : null; if (!enableComparisonSeries) return existing; - const primary = existing || [{ dataKey: schema.aggregate!.field }]; + const primary = existing || [{ dataKey: valueKey }]; const labelMap: Record = { vsLastWeek: 'Previous week', vsLastMonth: 'Previous month', @@ -624,12 +652,12 @@ export const ObjectChart = (props: any) => { return [ ...primary.map((s: any) => ({ ...s, variant: s.variant || 'current' })), { - dataKey: `${schema.aggregate!.field}__comparison`, + dataKey: comparisonKey, label: friendlyLabel, variant: 'comparison', }, ]; - }, [enableComparisonSeries, (schema as any).series, schema.aggregate, schema.filter, compareToConfig]); + }, [enableComparisonSeries, (schema as any).series, valueKey, comparisonKey, schema.filter, compareToConfig]); // ADR-0021 (#1759): when the chart binds to a dataset, derive data/xAxisKey/ // series from its dimensions/measures via the shared buildChartSeries helper —