Skip to content

Commit a6d8b8d

Browse files
os-warrenclaude
andauthored
fix(grid,list,core): union a grid's grouping fields into the query projection (#7214)
A grid view declaring `grouping` on a field absent from its `columns` rendered one group labelled `(empty)` holding every row. `$select` was built from the view's `columns` alone, so the grouping field was never requested and was `undefined` on every row by the time grouping ran. The fix is in the projection, at both sites it is built (`ObjectGrid` when it fetches for itself, `ListView` when it fetches for the grid), plus `$expand` so a lookup grouping field groups by name rather than by raw foreign key. The union is gated: grouping fields are intersected with the object's declared fields and pass the same FLS gate as columns, because an unknown `$select` key zeroes the whole list on backends that reject rather than ignore it. Co-authored-by: Claude <noreply@anthropic.com>
1 parent 3561bd2 commit a6d8b8d

8 files changed

Lines changed: 911 additions & 9 deletions

File tree

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
---
2+
'@object-ui/core': patch
3+
'@object-ui/plugin-grid': patch
4+
'@object-ui/plugin-list': patch
5+
---
6+
7+
Fix: a grid grouped by a field it does not also show as a column no longer collapses
8+
every row into one `(empty)` group (objectui#7179).
9+
10+
`$select` was built from the view's `columns` and nothing else, so a view declaring
11+
`grouping: { fields: [{ field: 'business_unit' }] }` on a field absent from its columns
12+
never asked the server for that field. It was `undefined` on every row by the time
13+
grouping ran, and the grouping label builder — correctly, for a genuinely empty value —
14+
answered `(empty)` for all of them. The result was one collapsible group holding every
15+
record, with no error, no warning and no empty state: a grid that looked like it grouped
16+
and did not, reading as "these records have no value for this field".
17+
18+
The grouping fields are now unioned into the projection, at both places it is built —
19+
`ObjectGrid` when it fetches for itself, and `ListView` when it fetches and hands the
20+
rows down. Lookup grouping fields are unioned into `$expand` as well: a `select` that
21+
fetches a bare foreign key without populating it buckets by raw id instead of by name,
22+
which is a different wrong answer rather than a fix.
23+
24+
Authors do not need to mirror a grouping field in `columns` any more. That was never
25+
required by `@objectstack/spec` — `grouping` is a sibling of `columns`, not a subset of
26+
it — and the neighbouring view kinds (kanban, gantt, timeline) already unioned their
27+
`groupByField` with no column needed. Refusing the configuration at author time was
28+
considered and rejected: it would make the grid the odd one out and reject working
29+
intent that the schema explicitly allows.
30+
31+
The union is guarded, and the guard is as load-bearing as the fix. A `grouping.fields[]`
32+
entry carries a bare string that has never been through column validation, and some
33+
backends answer an unknown `$select` key with an empty result set rather than ignoring
34+
it. Unioned unguarded, a grouping field naming something the object does not declare
35+
would have turned this bug into a strictly worse one — no rows at all, equally silently.
36+
Grouping fields are therefore intersected with the object's declared fields and passed
37+
through the same field-level-security gate as columns and predicate operands before they
38+
reach the query.

‎packages/core/src/index.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,10 @@ export * from './utils/predicate-record.js';
9898
// The other half of a view's field appetite: the fields its PREDICATES read,
9999
// which the column-derived `$select` never asked the server for.
100100
export * from './utils/predicate-fields.js';
101+
// The THIRD source of a view's field appetite: the fields it GROUPS BY. The
102+
// spec's `grouping` block is a sibling of `columns`, not a subset, so a grid
103+
// may group by a field it never shows (objectui#7179).
104+
export * from './utils/grouping-fields.js';
101105
export * from './utils/normalize-list-view.js';
102106
// The single home for the VALUE fallback prettifier (a stored value becomes a
103107
// display string when nothing resolves it). `@object-ui/fields` and
Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* objectui#7179 — the grouping-field harvester's own contract.
11+
*
12+
* The two renderer suites (`plugin-grid`'s `groupingProjection-7179` and
13+
* `plugin-list`'s `ListView.groupingProjection-7179`) pin what reaches the
14+
* QUERY. This pins the harvest itself, which is the half those two cannot
15+
* fully reach: a malformed entry that crashes a consumer's own unrelated code
16+
* before the projection is built is invisible to them, and the `null` case
17+
* below is exactly that — `ObjectGrid`'s `groupValueFormatter` memo throws on
18+
* it today, so the only place the harvester's handling of `null` is observable
19+
* is here.
20+
*/
21+
import { describe, it, expect } from 'vitest';
22+
import { collectGroupingFieldRefs } from '../grouping-fields';
23+
24+
describe('collectGroupingFieldRefs (objectui#7179)', () => {
25+
it('harvests the field name from the spec shape', () => {
26+
expect(
27+
collectGroupingFieldRefs({ fields: [{ field: 'business_unit', order: 'asc', collapsed: false }] }),
28+
).toEqual(['business_unit']);
29+
});
30+
31+
it('preserves first-seen order across a multi-level block', () => {
32+
expect(
33+
collectGroupingFieldRefs({ fields: [{ field: 'business_unit' }, { field: 'region' }] }),
34+
).toEqual(['business_unit', 'region']);
35+
});
36+
37+
it('deduplicates a repeated field', () => {
38+
expect(
39+
collectGroupingFieldRefs({ fields: [{ field: 'region' }, { field: 'region' }] }),
40+
).toEqual(['region']);
41+
});
42+
43+
it('trims surrounding whitespace so a padded name is not sent as an unknown key', () => {
44+
expect(collectGroupingFieldRefs({ fields: [{ field: ' region ' }] })).toEqual(['region']);
45+
});
46+
47+
it.each([
48+
['undefined', undefined],
49+
['null', null],
50+
['an empty block', {}],
51+
['a non-array `fields`', { fields: 'business_unit' }],
52+
['an empty `fields`', { fields: [] }],
53+
])('harvests nothing from %s', (_label, input) => {
54+
expect(collectGroupingFieldRefs(input)).toEqual([]);
55+
});
56+
57+
it('contributes nothing for a NULL entry rather than throwing', () => {
58+
// The shape no consumer survives today. The harvester must not be the
59+
// thing that throws, so the crash stays attributable to its real owner.
60+
expect(collectGroupingFieldRefs({ fields: [null, { field: 'region' }] })).toEqual(['region']);
61+
});
62+
63+
it('contributes nothing for an entry with no `field`', () => {
64+
expect(collectGroupingFieldRefs({ fields: [{}, { field: 'region' }] })).toEqual(['region']);
65+
});
66+
67+
it('contributes nothing for a non-string `field`', () => {
68+
expect(collectGroupingFieldRefs({ fields: [{ field: 42 }, { field: 'region' }] })).toEqual(['region']);
69+
});
70+
71+
it('contributes nothing for an empty or whitespace-only `field`', () => {
72+
expect(collectGroupingFieldRefs({ fields: [{ field: '' }, { field: ' ' }] })).toEqual([]);
73+
});
74+
75+
it('REFUSES a bare string entry — the shorthand the spec does not accept', () => {
76+
// `GroupingConfigSchema.fields` is an array of `$strict` OBJECTS. Reading a
77+
// bare string anyway would be the lenient renderer-side alias AGENTS.md
78+
// #0.1 forbids: it fossilizes a second de-facto contract instead of having
79+
// the producer rejected at publish. It also could not work end to end —
80+
// `useGroupedData` reads `f.field` off each entry, so a bare string groups
81+
// by `undefined` no matter what the projection asks for.
82+
expect(collectGroupingFieldRefs({ fields: ['business_unit'] })).toEqual([]);
83+
});
84+
});
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* The THIRD half of a view's field appetite (objectui#7179).
11+
*
12+
* A list view projects what it DISPLAYS — `$select` is built from `columns`.
13+
* `predicate-fields.ts` covers the fields a view's PREDICATES read. This covers
14+
* the fields it GROUPS BY, which is a third, independent source of demand: the
15+
* spec's `grouping` block is a sibling of `columns`, not a subset of it, so a
16+
* view may legitimately group by a field it never shows.
17+
*
18+
* ## Why it needed a fix rather than a gate
19+
*
20+
* `GroupingConfigSchema` accepts `grouping` with no matching column, and the
21+
* neighbouring view kinds (kanban / gantt / timeline) already union their
22+
* `groupByField` into the projection with no column required. Grouping by a
23+
* field you do not want on screen is an ordinary thing to want. Refusing it at
24+
* author time would make the grid the odd one out and reject working intent.
25+
*
26+
* ## The failure this closes
27+
*
28+
* With the grouping field absent from `$select` the server never returns it,
29+
* so `useGroupedData` reads `undefined` on every row and `buildSegmentLabel`
30+
* answers `(empty)` for all of them: ONE group holding every record, with no
31+
* error, no warning and no empty state. It reads as "these records have no
32+
* value for this field" — a plausible, wrong, actionable conclusion about the
33+
* data rather than a visible bug in the view.
34+
*
35+
* ## ⛔ THE RESULT IS CANDIDATES, NOT VERIFIED FIELDS — the caller MUST gate it
36+
*
37+
* `GroupingFieldSchema.field` is a bare `z.ZodString`. Nothing in the schema
38+
* requires it to name a field the object declares, and the whole premise of
39+
* this harvest is that it has NOT been through column validation. Some backends
40+
* answer an unknown `$select` key with an EMPTY RESULT SET rather than ignoring
41+
* it — the cloud multi-tenant runtime does exactly that — so a single unknown
42+
* grouping field put in the projection unguarded silently zeroes the whole
43+
* list. That would convert this card's bug (one `(empty)` group holding every
44+
* row) into a strictly worse one (no rows at all, still silent).
45+
*
46+
* So every caller intersects this result with the object's declared fields —
47+
* {@link isProjectableField}, or the caller's equivalent known-field set —
48+
* exactly as {@link collectPredicateFieldRefs}'s callers do, and for the same
49+
* measured reason. Callers additionally FLS-gate it: a grouping field names a
50+
* field just as capable of being denied as a column is, and the projection is
51+
* what goes on the wire (objectui#6898).
52+
*
53+
* @param grouping - The view's `grouping` block in any authored state
54+
* (`undefined`, malformed, or the spec shape). Anything that is not an entry
55+
* carrying a non-empty string `field` contributes nothing, so a malformed
56+
* block yields an empty harvest instead of a plausible wrong name.
57+
* @returns Grouping field names in first-seen order, deduplicated.
58+
*/
59+
export function collectGroupingFieldRefs(grouping: unknown): string[] {
60+
const fields = (grouping as { fields?: unknown } | null | undefined)?.fields;
61+
if (!Array.isArray(fields)) return [];
62+
const out: string[] = [];
63+
const seen = new Set<string>();
64+
for (const entry of fields) {
65+
// The spec shape is `{ field, order, collapsed }`. A bare string is NOT
66+
// accepted here even though it would be a natural shorthand: `grouping` is
67+
// a `$strict` object schema in `@objectstack/spec`, so a bare string is
68+
// off-spec metadata, and reading it anyway would be exactly the lenient
69+
// renderer-side alias AGENTS.md #0.1 forbids — it fossilizes a second
70+
// de-facto contract instead of having the producer rejected at publish.
71+
const name = (entry as { field?: unknown } | null | undefined)?.field;
72+
if (typeof name !== 'string') continue;
73+
const trimmed = name.trim();
74+
if (!trimmed || seen.has(trimmed)) continue;
75+
seen.add(trimmed);
76+
out.push(trimmed);
77+
}
78+
return out;
79+
}

0 commit comments

Comments
 (0)