Skip to content

Commit 6e8fd3c

Browse files
authored
fix(charts): a fieldless count aggregate keyed its value column undefined (framework#3701) (#2878)
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. Three of the four row builders honoured it. The odd one out was `count`, the one function that may legitimately omit `field`, because every 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 had returned. 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.
1 parent 9b4b952 commit 6e8fd3c

4 files changed

Lines changed: 168 additions & 25 deletions

File tree

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
---
2+
"@object-ui/plugin-charts": patch
3+
"@object-ui/data-objectstack": patch
4+
---
5+
6+
fix(charts): a fieldless `count` aggregate keyed its value column `undefined`, so the chart plotted nothing (framework#3701)
7+
8+
framework#3701 pinned down what an OBJECT-bound chart aggregate names its result
9+
columns — the raw field names it was given (`groupBy` for the category, `field`
10+
for the value; no `sum_`-style decoration, unlike a dataset measure), plus the
11+
literal `count` when a `count` omits `field`, which is the alias the engine
12+
projects `COUNT(*)` under. `os validate` now lints page sources against that
13+
convention, so the paths that build these rows have to honour it exactly.
14+
15+
Three of the four did. The odd one out was `count` — the one function that may
16+
legitimately omit `field` — because every row builder read `params.field`
17+
directly:
18+
19+
- `aggregateRecords` / `ObjectDataSource.aggregateClientSide` emitted
20+
`{ [groupBy]: key, [undefined]: value }`, i.e. a column literally named
21+
`undefined` that no axis binding could ever name;
22+
- the legacy analytics path was worse: it remapped the server's `count` measure
23+
onto `params.field` and **deleted** the original key, so the value the server
24+
did return was thrown away before the chart saw it.
25+
26+
All of them now resolve the column through one helper (`aggregateValueKey`) so a
27+
fieldless count lands under `count`, matching the framework contract. The
28+
comparison-overlay column is derived from the same key (`count__comparison`
29+
instead of `undefined__comparison`), and `aggregate.field` is typed optional to
30+
match the spec's `ChartAggregateSchema`. Charts that name a field are unchanged.

packages/data-objectstack/src/index.ts

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2603,6 +2603,12 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
26032603
const measureName = params.function === 'count'
26042604
? 'count'
26052605
: `${params.field}_${params.function}`;
2606+
// The column the caller expects the value under — the raw `field`, or the
2607+
// literal `count` when a count names no field (framework#3701). Reading
2608+
// `params.field` directly here keyed the row `undefined` for a fieldless
2609+
// count and deleted the `count` the server sent, so the chart plotted
2610+
// nothing.
2611+
const valueKey = this.aggregateValueKey(params);
26062612

26072613
const payload: Record<string, unknown> = {
26082614
cube: resource,
@@ -2634,7 +2640,7 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
26342640
const measureMissing = rawRows.length > 0 && rawRows.every((row: any) => {
26352641
if (row == null) return true;
26362642
if (measureName in row && row[measureName] != null) return false;
2637-
if (params.field in row && row[params.field] != null) return false;
2643+
if (valueKey in row && row[valueKey] != null) return false;
26382644
return true;
26392645
});
26402646
if (measureMissing) {
@@ -2644,14 +2650,15 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
26442650
return this.aggregateClientSide(records, params);
26452651
}
26462652

2647-
// Map measure keys back to the original field name so that consumers
2648-
// (ObjectChart, DashboardRenderer, etc.) can access values by field name.
2649-
// This includes count → field (e.g. 'count' → 'amount') to match the
2650-
// output format of aggregateClientSide() which always uses params.field.
2653+
// Map measure keys back to the object-bound result column so consumers
2654+
// (ObjectChart, DashboardRenderer, …) read values by the name the
2655+
// convention promises: `field`, or `count` for a fieldless count
2656+
// (framework#3701). This includes count → field (e.g. 'count' →
2657+
// 'amount'), matching aggregateClientSide()'s output.
26512658
return rawRows.map((row: any) => {
26522659
const mapped = { ...row };
2653-
if (measureName !== params.field && measureName in mapped) {
2654-
mapped[params.field] = mapped[measureName];
2660+
if (measureName !== valueKey && measureName in mapped) {
2661+
mapped[valueKey] = mapped[measureName];
26552662
delete mapped[measureName];
26562663
}
26572664
return mapped;
@@ -2776,8 +2783,19 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
27762783
}
27772784

27782785
/** Client-side aggregation fallback */
2779-
private aggregateClientSide(records: any[], params: { field: string; function: string; groupBy: string }): any[] {
2786+
/**
2787+
* The result column an object-bound `aggregate` projects its value under
2788+
* (framework#3701, `chartAggregateValueKey` in `@objectstack/spec/ui`): the
2789+
* raw `field` name — no `sum_`-style decoration, unlike a dataset measure —
2790+
* or the literal `count` when a count names no field.
2791+
*/
2792+
private aggregateValueKey(params: { field?: string; function?: string }): string {
2793+
return params.field || params.function || 'count';
2794+
}
2795+
2796+
private aggregateClientSide(records: any[], params: { field?: string; function: string; groupBy: string }): any[] {
27802797
const { field, function: aggFn, groupBy } = params;
2798+
const valueKey = this.aggregateValueKey(params);
27812799
const groups: Record<string, any[]> = {};
27822800

27832801
for (const record of records) {
@@ -2787,7 +2805,7 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
27872805
}
27882806

27892807
return Object.entries(groups).map(([key, group]) => {
2790-
const values = group.map(r => Number(r[field]) || 0);
2808+
const values = field ? group.map(r => Number(r[field]) || 0) : [];
27912809
let result: number;
27922810

27932811
switch (aggFn) {
@@ -2798,7 +2816,7 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
27982816
case 'sum': default: result = values.reduce((a, b) => a + b, 0); break;
27992817
}
28002818

2801-
return { [groupBy]: key, [field]: result };
2819+
return { [groupBy]: key, [valueKey]: result };
28022820
});
28032821
}
28042822

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* The object-bound aggregate RESULT-COLUMN convention (framework#3701).
6+
*
7+
* An object-bound chart (`objectName` + an inline `aggregate`) returns rows
8+
* keyed by the RAW FIELD NAMES it was given — `groupBy` for the category,
9+
* `field` for the value — with NO `sum_`-style decoration. That is the opposite
10+
* of a dataset, whose rows are keyed by the declared measure `name`, and it is
11+
* what `xAxisKey` / `series[].dataKey` resolve against. The framework now pins
12+
* the rule (`chartAggregateResultKeys` in `@objectstack/spec/ui`) and lints
13+
* page sources against it, so the row builders here have to honour it exactly.
14+
*
15+
* Regression: `count` is the one function that may omit `field`, and every
16+
* builder read `params.field` directly — so a fieldless count keyed its value
17+
* `undefined` and the chart plotted nothing.
18+
*/
19+
import { describe, it, expect } from 'vitest';
20+
import { aggregateRecords, aggregateValueKey } from './ObjectChart';
21+
22+
const rows = [
23+
{ status: 'open', total: 100 },
24+
{ status: 'open', total: 50 },
25+
{ status: 'paid', total: 20 },
26+
];
27+
28+
describe('aggregateValueKey', () => {
29+
it('is the raw field — never a decorated measure name', () => {
30+
expect(aggregateValueKey({ field: 'total', function: 'sum' })).toBe('total');
31+
});
32+
33+
it('is the literal "count" when a count names no field', () => {
34+
expect(aggregateValueKey({ function: 'count' })).toBe('count');
35+
});
36+
37+
it('prefers an explicit field even for count', () => {
38+
expect(aggregateValueKey({ field: 'total', function: 'count' })).toBe('total');
39+
});
40+
});
41+
42+
describe('aggregateRecords — result columns', () => {
43+
it('keys rows by groupBy and the raw field', () => {
44+
expect(aggregateRecords(rows, { field: 'total', function: 'sum', groupBy: 'status' })).toEqual([
45+
{ status: 'open', total: 150 },
46+
{ status: 'paid', total: 20 },
47+
]);
48+
});
49+
50+
it('keys a fieldless count under "count", not undefined', () => {
51+
const out = aggregateRecords(rows, { function: 'count', groupBy: 'status' });
52+
expect(out).toEqual([
53+
{ status: 'open', count: 2 },
54+
{ status: 'paid', count: 1 },
55+
]);
56+
// The pre-fix shape: a column literally named "undefined", which no axis
57+
// binding could ever name.
58+
expect(Object.keys(out[0])).not.toContain('undefined');
59+
});
60+
61+
it('still counts rows when a field IS named', () => {
62+
expect(aggregateRecords(rows, { field: 'total', function: 'count', groupBy: 'status' })).toEqual([
63+
{ status: 'open', total: 2 },
64+
{ status: 'paid', total: 1 },
65+
]);
66+
});
67+
});

packages/plugin-charts/src/ObjectChart.tsx

Lines changed: 43 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,34 @@ export function humanizeLabel(value: string): string {
1515
return value.replace(/[_-]/g, ' ').replace(/\b\w/g, c => c.toUpperCase());
1616
}
1717

18+
/**
19+
* The result column an object-bound `aggregate` projects its value under
20+
* (framework#3701, `chartAggregateValueKey` in `@objectstack/spec/ui`).
21+
*
22+
* The raw `field` name — an object-bound aggregate does NOT decorate it the way
23+
* a dataset measure is named (`sum_amount`). Only `count` may omit `field`, and
24+
* it then lands under the literal `'count'`, which is the alias the engine
25+
* projects `COUNT(*)` under. Exported so every path that builds these rows
26+
* agrees on one key instead of each re-deriving it.
27+
*/
28+
export function aggregateValueKey(aggregate: { field?: string; function?: string }): string {
29+
return aggregate.field || aggregate.function || 'count';
30+
}
31+
32+
/** Suffix the previous-window value carries under a compareTo overlay. */
33+
export const COMPARISON_SUFFIX = '__comparison';
34+
1835
/**
1936
* Client-side aggregation for fetched records.
2037
* Groups records by `groupBy` field and applies the aggregation function
2138
* to the `field` values in each group.
2239
*/
2340
export function aggregateRecords(
2441
records: any[],
25-
aggregate: { field: string; function: string; groupBy: string }
42+
aggregate: { field?: string; function: string; groupBy: string }
2643
): any[] {
2744
const { field, function: aggFn, groupBy } = aggregate;
45+
const valueKey = aggregateValueKey(aggregate);
2846
const groups: Record<string, any[]> = {};
2947

3048
for (const record of records) {
@@ -34,7 +52,7 @@ export function aggregateRecords(
3452
}
3553

3654
return Object.entries(groups).map(([key, group]) => {
37-
const values = group.map(r => Number(r[field]) || 0);
55+
const values = field ? group.map(r => Number(r[field]) || 0) : [];
3856
let result: number;
3957

4058
switch (aggFn) {
@@ -56,7 +74,7 @@ export function aggregateRecords(
5674
break;
5775
}
5876

59-
return { [groupBy]: key, [field]: result };
77+
return { [groupBy]: key, [valueKey]: result };
6078
});
6179
}
6280

@@ -369,8 +387,9 @@ export const ObjectChart = (props: any) => {
369387
const aggField = schema.aggregate.field;
370388
const aggFn = schema.aggregate.function;
371389
// Project the measure under its plain field name so downstream
372-
// (xAxisKey + series.dataKey lookups) finds it unchanged.
373-
const alias = aggField || aggFn;
390+
// (xAxisKey + series.dataKey lookups) finds it unchanged — the
391+
// object-bound result-column convention (framework#3701).
392+
const alias = aggregateValueKey(schema.aggregate);
374393
// For `count`, omit `field` so the engine emits `count(*)` /
375394
// `COUNT(*)`. The upstream dashboard wiring defaults `field: 'value'`
376395
// for charts without an explicit valueField, which crashes on SQL
@@ -479,17 +498,22 @@ export const ObjectChart = (props: any) => {
479498
if (wantsComparison && comparisonRows.length > 0 && schema.aggregate) {
480499
const aggField = schema.aggregate.field;
481500
const aggFn = schema.aggregate.function;
501+
// The column this aggregate projects its value under — `field`,
502+
// or `count` for a fieldless count (framework#3701).
503+
const valueKey = aggregateValueKey(schema.aggregate);
482504
const readValue = (row: Record<string, any>): number | null => {
483505
if (row == null) return null;
484-
const suffixed = `${aggField}_${aggFn}`;
485-
if (suffixed in row) return Number(row[suffixed]);
486-
if (aggFn === 'count' && `${aggField}_count` in row) return Number(row[`${aggField}_count`]);
487-
if (aggField in row) return Number(row[aggField]);
506+
if (aggField) {
507+
const suffixed = `${aggField}_${aggFn}`;
508+
if (suffixed in row) return Number(row[suffixed]);
509+
if (aggFn === 'count' && `${aggField}_count` in row) return Number(row[`${aggField}_count`]);
510+
}
511+
if (valueKey in row) return Number(row[valueKey]);
488512
if ('value' in row) return Number(row.value);
489513
if ('count' in row) return Number(row.count);
490514
return null;
491515
};
492-
const comparisonKey = `${aggField}__comparison`;
516+
const comparisonKey = `${valueKey}${COMPARISON_SUFFIX}`;
493517
const gb = groupByField;
494518
if (gb && data.some((r: any) => r[gb] != null) && comparisonRows.some((r: any) => r[gb] != null)) {
495519
const cmpByKey = new Map<string, number | null>();
@@ -601,16 +625,20 @@ export const ObjectChart = (props: any) => {
601625
// for a supported chart type, also synthesize a second series so the
602626
// chart implementation renders the comparison overlay (dashed / muted).
603627
const compareToConfig: CompareToConfig | undefined = (schema as any).compareTo;
628+
// The result column this aggregate projects its value under, and the column
629+
// the comparison overlay arrives in (framework#3701).
630+
const valueKey = schema.aggregate ? aggregateValueKey(schema.aggregate) : undefined;
631+
const comparisonKey = valueKey ? `${valueKey}${COMPARISON_SUFFIX}` : undefined;
604632
const enableComparisonSeries =
605633
!!compareToConfig &&
606634
supportsCompareTo(schema.chartType) &&
607-
!!schema.aggregate &&
608-
finalData.some((row: Record<string, any>) => row[`${schema.aggregate!.field}__comparison`] != null);
635+
!!comparisonKey &&
636+
finalData.some((row: Record<string, any>) => row[comparisonKey] != null);
609637

610638
const augmentedSeries = useMemo(() => {
611639
const existing = Array.isArray((schema as any).series) ? (schema as any).series : null;
612640
if (!enableComparisonSeries) return existing;
613-
const primary = existing || [{ dataKey: schema.aggregate!.field }];
641+
const primary = existing || [{ dataKey: valueKey }];
614642
const labelMap: Record<string, string> = {
615643
vsLastWeek: 'Previous week',
616644
vsLastMonth: 'Previous month',
@@ -624,12 +652,12 @@ export const ObjectChart = (props: any) => {
624652
return [
625653
...primary.map((s: any) => ({ ...s, variant: s.variant || 'current' })),
626654
{
627-
dataKey: `${schema.aggregate!.field}__comparison`,
655+
dataKey: comparisonKey,
628656
label: friendlyLabel,
629657
variant: 'comparison',
630658
},
631659
];
632-
}, [enableComparisonSeries, (schema as any).series, schema.aggregate, schema.filter, compareToConfig]);
660+
}, [enableComparisonSeries, (schema as any).series, valueKey, comparisonKey, schema.filter, compareToConfig]);
633661

634662
// ADR-0021 (#1759): when the chart binds to a dataset, derive data/xAxisKey/
635663
// series from its dimensions/measures via the shared buildChartSeries helper —

0 commit comments

Comments
 (0)