diff --git a/.changeset/7179-grid-grouping-projection.md b/.changeset/7179-grid-grouping-projection.md new file mode 100644 index 0000000000..b59fb5a296 --- /dev/null +++ b/.changeset/7179-grid-grouping-projection.md @@ -0,0 +1,38 @@ +--- +'@object-ui/core': patch +'@object-ui/plugin-grid': patch +'@object-ui/plugin-list': patch +--- + +Fix: a grid grouped by a field it does not also show as a column no longer collapses +every row into one `(empty)` group (objectui#7179). + +`$select` was built from the view's `columns` and nothing else, so a view declaring +`grouping: { fields: [{ field: 'business_unit' }] }` on a field absent from its columns +never asked the server for that field. It was `undefined` on every row by the time +grouping ran, and the grouping label builder — correctly, for a genuinely empty value — +answered `(empty)` for all of them. The result was one collapsible group holding every +record, with no error, no warning and no empty state: a grid that looked like it grouped +and did not, reading as "these records have no value for this field". + +The grouping fields are now unioned into the projection, at both places it is built — +`ObjectGrid` when it fetches for itself, and `ListView` when it fetches and hands the +rows down. Lookup grouping fields are unioned into `$expand` as well: a `select` that +fetches a bare foreign key without populating it buckets by raw id instead of by name, +which is a different wrong answer rather than a fix. + +Authors do not need to mirror a grouping field in `columns` any more. That was never +required by `@objectstack/spec` — `grouping` is a sibling of `columns`, not a subset of +it — and the neighbouring view kinds (kanban, gantt, timeline) already unioned their +`groupByField` with no column needed. Refusing the configuration at author time was +considered and rejected: it would make the grid the odd one out and reject working +intent that the schema explicitly allows. + +The union is guarded, and the guard is as load-bearing as the fix. A `grouping.fields[]` +entry carries a bare string that has never been through column validation, and some +backends answer an unknown `$select` key with an empty result set rather than ignoring +it. Unioned unguarded, a grouping field naming something the object does not declare +would have turned this bug into a strictly worse one — no rows at all, equally silently. +Grouping fields are therefore intersected with the object's declared fields and passed +through the same field-level-security gate as columns and predicate operands before they +reach the query. diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 20407143dc..8cbcdb6678 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -98,6 +98,10 @@ export * from './utils/predicate-record.js'; // The other half of a view's field appetite: the fields its PREDICATES read, // which the column-derived `$select` never asked the server for. export * from './utils/predicate-fields.js'; +// The THIRD source of a view's field appetite: the fields it GROUPS BY. The +// spec's `grouping` block is a sibling of `columns`, not a subset, so a grid +// may group by a field it never shows (objectui#7179). +export * from './utils/grouping-fields.js'; export * from './utils/normalize-list-view.js'; // The single home for the VALUE fallback prettifier (a stored value becomes a // display string when nothing resolves it). `@object-ui/fields` and diff --git a/packages/core/src/utils/__tests__/grouping-fields.test.ts b/packages/core/src/utils/__tests__/grouping-fields.test.ts new file mode 100644 index 0000000000..c316ad5d60 --- /dev/null +++ b/packages/core/src/utils/__tests__/grouping-fields.test.ts @@ -0,0 +1,84 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#7179 — the grouping-field harvester's own contract. + * + * The two renderer suites (`plugin-grid`'s `groupingProjection-7179` and + * `plugin-list`'s `ListView.groupingProjection-7179`) pin what reaches the + * QUERY. This pins the harvest itself, which is the half those two cannot + * fully reach: a malformed entry that crashes a consumer's own unrelated code + * before the projection is built is invisible to them, and the `null` case + * below is exactly that — `ObjectGrid`'s `groupValueFormatter` memo throws on + * it today, so the only place the harvester's handling of `null` is observable + * is here. + */ +import { describe, it, expect } from 'vitest'; +import { collectGroupingFieldRefs } from '../grouping-fields'; + +describe('collectGroupingFieldRefs (objectui#7179)', () => { + it('harvests the field name from the spec shape', () => { + expect( + collectGroupingFieldRefs({ fields: [{ field: 'business_unit', order: 'asc', collapsed: false }] }), + ).toEqual(['business_unit']); + }); + + it('preserves first-seen order across a multi-level block', () => { + expect( + collectGroupingFieldRefs({ fields: [{ field: 'business_unit' }, { field: 'region' }] }), + ).toEqual(['business_unit', 'region']); + }); + + it('deduplicates a repeated field', () => { + expect( + collectGroupingFieldRefs({ fields: [{ field: 'region' }, { field: 'region' }] }), + ).toEqual(['region']); + }); + + it('trims surrounding whitespace so a padded name is not sent as an unknown key', () => { + expect(collectGroupingFieldRefs({ fields: [{ field: ' region ' }] })).toEqual(['region']); + }); + + it.each([ + ['undefined', undefined], + ['null', null], + ['an empty block', {}], + ['a non-array `fields`', { fields: 'business_unit' }], + ['an empty `fields`', { fields: [] }], + ])('harvests nothing from %s', (_label, input) => { + expect(collectGroupingFieldRefs(input)).toEqual([]); + }); + + it('contributes nothing for a NULL entry rather than throwing', () => { + // The shape no consumer survives today. The harvester must not be the + // thing that throws, so the crash stays attributable to its real owner. + expect(collectGroupingFieldRefs({ fields: [null, { field: 'region' }] })).toEqual(['region']); + }); + + it('contributes nothing for an entry with no `field`', () => { + expect(collectGroupingFieldRefs({ fields: [{}, { field: 'region' }] })).toEqual(['region']); + }); + + it('contributes nothing for a non-string `field`', () => { + expect(collectGroupingFieldRefs({ fields: [{ field: 42 }, { field: 'region' }] })).toEqual(['region']); + }); + + it('contributes nothing for an empty or whitespace-only `field`', () => { + expect(collectGroupingFieldRefs({ fields: [{ field: '' }, { field: ' ' }] })).toEqual([]); + }); + + it('REFUSES a bare string entry — the shorthand the spec does not accept', () => { + // `GroupingConfigSchema.fields` is an array of `$strict` OBJECTS. Reading a + // bare string anyway would be the lenient renderer-side alias AGENTS.md + // #0.1 forbids: it fossilizes a second de-facto contract instead of having + // the producer rejected at publish. It also could not work end to end — + // `useGroupedData` reads `f.field` off each entry, so a bare string groups + // by `undefined` no matter what the projection asks for. + expect(collectGroupingFieldRefs({ fields: ['business_unit'] })).toEqual([]); + }); +}); diff --git a/packages/core/src/utils/grouping-fields.ts b/packages/core/src/utils/grouping-fields.ts new file mode 100644 index 0000000000..49ceaee9bd --- /dev/null +++ b/packages/core/src/utils/grouping-fields.ts @@ -0,0 +1,79 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * The THIRD half of a view's field appetite (objectui#7179). + * + * A list view projects what it DISPLAYS — `$select` is built from `columns`. + * `predicate-fields.ts` covers the fields a view's PREDICATES read. This covers + * the fields it GROUPS BY, which is a third, independent source of demand: the + * spec's `grouping` block is a sibling of `columns`, not a subset of it, so a + * view may legitimately group by a field it never shows. + * + * ## Why it needed a fix rather than a gate + * + * `GroupingConfigSchema` accepts `grouping` with no matching column, and the + * neighbouring view kinds (kanban / gantt / timeline) already union their + * `groupByField` into the projection with no column required. Grouping by a + * field you do not want on screen is an ordinary thing to want. Refusing it at + * author time would make the grid the odd one out and reject working intent. + * + * ## The failure this closes + * + * With the grouping field absent from `$select` the server never returns it, + * so `useGroupedData` reads `undefined` on every row and `buildSegmentLabel` + * answers `(empty)` for all of them: ONE group holding every record, with no + * error, no warning and no empty state. It reads as "these records have no + * value for this field" — a plausible, wrong, actionable conclusion about the + * data rather than a visible bug in the view. + * + * ## ⛔ THE RESULT IS CANDIDATES, NOT VERIFIED FIELDS — the caller MUST gate it + * + * `GroupingFieldSchema.field` is a bare `z.ZodString`. Nothing in the schema + * requires it to name a field the object declares, and the whole premise of + * this harvest is that it has NOT been through column validation. Some backends + * answer an unknown `$select` key with an EMPTY RESULT SET rather than ignoring + * it — the cloud multi-tenant runtime does exactly that — so a single unknown + * grouping field put in the projection unguarded silently zeroes the whole + * list. That would convert this card's bug (one `(empty)` group holding every + * row) into a strictly worse one (no rows at all, still silent). + * + * So every caller intersects this result with the object's declared fields — + * {@link isProjectableField}, or the caller's equivalent known-field set — + * exactly as {@link collectPredicateFieldRefs}'s callers do, and for the same + * measured reason. Callers additionally FLS-gate it: a grouping field names a + * field just as capable of being denied as a column is, and the projection is + * what goes on the wire (objectui#6898). + * + * @param grouping - The view's `grouping` block in any authored state + * (`undefined`, malformed, or the spec shape). Anything that is not an entry + * carrying a non-empty string `field` contributes nothing, so a malformed + * block yields an empty harvest instead of a plausible wrong name. + * @returns Grouping field names in first-seen order, deduplicated. + */ +export function collectGroupingFieldRefs(grouping: unknown): string[] { + const fields = (grouping as { fields?: unknown } | null | undefined)?.fields; + if (!Array.isArray(fields)) return []; + const out: string[] = []; + const seen = new Set(); + for (const entry of fields) { + // The spec shape is `{ field, order, collapsed }`. A bare string is NOT + // accepted here even though it would be a natural shorthand: `grouping` is + // a `$strict` object schema in `@objectstack/spec`, so a bare string is + // off-spec metadata, and reading it anyway would be exactly the lenient + // renderer-side alias AGENTS.md #0.1 forbids — it fossilizes a second + // de-facto contract instead of having the producer rejected at publish. + const name = (entry as { field?: unknown } | null | undefined)?.field; + if (typeof name !== 'string') continue; + const trimmed = name.trim(); + if (!trimmed || seen.has(trimmed)) continue; + seen.add(trimmed); + out.push(trimmed); + } + return out; +} diff --git a/packages/plugin-grid/src/ObjectGrid.tsx b/packages/plugin-grid/src/ObjectGrid.tsx index 971f23f2ac..0a3f9248cc 100644 --- a/packages/plugin-grid/src/ObjectGrid.tsx +++ b/packages/plugin-grid/src/ObjectGrid.tsx @@ -36,7 +36,7 @@ import { RefreshIndicator, } from '@object-ui/components'; import { usePullToRefresh } from '@object-ui/mobile'; -import { resolveConditionalFormatting, buildExpandFields, buildExportFileName, columnIdentity, collectPredicateFieldRefs, listViewPredicates, isObjectInlineEditable, isProjectableField, isExpandableFieldType, isUnmaterializedFieldType, readObjectSortability, isPlatformSortableField, filterPlatformSortableSort, toFilterNode, ROW_HEIGHT_TO_DENSITY_MODE } from '@object-ui/core'; +import { resolveConditionalFormatting, buildExpandFields, buildExportFileName, columnIdentity, collectPredicateFieldRefs, collectGroupingFieldRefs, listViewPredicates, isObjectInlineEditable, isProjectableField, isExpandableFieldType, isUnmaterializedFieldType, readObjectSortability, isPlatformSortableField, filterPlatformSortableSort, toFilterNode, ROW_HEIGHT_TO_DENSITY_MODE } from '@object-ui/core'; import { usePermissions } from '@object-ui/permissions'; import { ChevronRight, ChevronDown, ChevronLeft, ChevronsLeft, ChevronsRight, Download, Rows2, Rows3, Rows4, AlignJustify, Type, Hash, Calendar, CheckSquare, User, Tag, Clock, Loader2 } from 'lucide-react'; import { useRowColor } from './useRowColor'; @@ -1341,6 +1341,22 @@ export const ObjectGrid: React.FC = ({ const schemaFields = schema.fields; const schemaColumns = schema.columns; + // [objectui#7179] The fetch effect's dep on the grouping block, as a CONTENT + // key over the field NAMES alone. + // + // Not `schema.grouping` itself: hosts rebuild that object literal every + // render (`plugin-list` spreads a fresh `{ grouping: groupingConfig }` into + // the node it hands down), so naming it here would re-issue the query on + // every render — the identity-churn refetch storm objectui#6697 recorded for + // `expandFields`. + // + // Names ONLY, not the whole block: `order` and `collapsed` are render-time + // concerns the projection cannot see, so a user collapsing a group must not + // cost a round trip. What the query depends on is exactly this list. + const groupingProjectionKey = useMemo( + () => JSON.stringify(collectGroupingFieldRefs(schema.grouping)), + [schema.grouping], + ); // The view's declared filter, lowered ONCE through the repo's single filter // sink for both consumers below (the fetch and the server-side export). // @@ -1485,6 +1501,19 @@ export const ObjectGrid: React.FC = ({ // --- Step 2: Fetch data --- if (dataSource && objectName) { + // [objectui#7179] The fields the view GROUPS BY. `grouping` is a + // sibling of `columns` in the spec, not a subset of it, so a grid may + // legitimately group by a field it never shows — and until this + // harvest existed, `$select` (built from `columns` alone) never asked + // for it, so every row carried `undefined` and `useGroupedData` + // labelled ONE group `(empty)` holding every record. + // + // Raw here, GATED at each of its two use sites below, because the two + // sites need different gates: the projection has to intersect with + // the declared fields (an unknown `$select` key ZEROES the list on + // backends that reject rather than ignore it), while `$expand` is + // gated structurally by `buildExpandFields` itself. + const groupingFieldRefs = collectGroupingFieldRefs(schema.grouping); const getSelectFields = () => { // Always include 'id' so row click / navigation handlers can resolve // the record key — without it `record.id` is undefined and the @@ -1639,21 +1668,50 @@ export const ObjectGrid: React.FC = ({ // this principal may not read. .filter((f) => passesProjectionGate(f)) : []; - const withPredicates = (list: any[]): any[] => { - if (predicateFields.length === 0) return list; + // [objectui#7179] The GROUPING fields, through the SAME two gates + // the predicate operands take, for the same two measured reasons. + // + // `isProjectableField` first: a `grouping.fields[]` entry is + // SPECULATIVE in exactly the sense that gate exists for. Its + // `field` is a bare `z.ZodString` and this card's whole premise is + // that it is NOT among the columns, so nothing has validated it. + // Some backends answer an unknown `$select` key with an EMPTY + // RESULT SET rather than ignoring it (the cloud multi-tenant + // runtime does exactly that), so an unguarded union would trade + // this card's bug — one `(empty)` group holding all the rows — for + // a strictly worse one: NO rows, equally silently. The current bug + // at least shows the data. + // + // `passesProjectionGate` second (objectui#6898): a grouping field + // names a field just as capable of being denied as a column is, and + // this is the half that goes on the WIRE. Gating here rather than + // after the union is what keeps that card closed. + const groupingFields = declared + ? groupingFieldRefs + .filter((f) => isProjectableField(f, declared as Record)) + .filter((f) => passesProjectionGate(f)) + : []; + // A UNION, never an append: a grouping field that IS also a column + // must not produce a duplicate `$select` entry. + const extraFields = [...predicateFields, ...groupingFields]; + const withHarvestedFields = (list: any[]): any[] => { + if (extraFields.length === 0) return list; const names = new Set(list.map((f: any) => columnIdentity(f))); - const extra = predicateFields.filter((f) => !names.has(f)); + const seen = new Set(); + const extra = extraFields.filter( + (f) => !names.has(f) && !seen.has(f) && (seen.add(f), true), + ); return extra.length > 0 ? [...list, ...extra] : list; }; if (schemaFields) { - return withPredicates(ensureId((schemaFields as any[]).filter(passesProjectionGate))); + return withHarvestedFields(ensureId((schemaFields as any[]).filter(passesProjectionGate))); } if (schemaColumns && Array.isArray(schemaColumns)) { const fields = schemaColumns .filter(passesProjectionGate) .map((c: any) => columnIdentity(c)) .filter((v): v is string => !!v); - return withPredicates(ensureId(fields)); + return withHarvestedFields(ensureId(fields)); } return undefined; }; @@ -1741,7 +1799,27 @@ export const ObjectGrid: React.FC = ({ } // Auto-inject $expand for lookup/master_detail fields - const expand = buildExpandFields(resolvedSchema?.fields, schemaColumns ?? schemaFields); + // + // [objectui#7179] The grouping fields ride along. `$select` alone + // fetches a lookup as its BARE FOREIGN KEY, so a grid grouped by a + // lookup would bucket by raw id ("8UY9zHWBfjYjYor4") instead of by + // name — better than one `(empty)` bucket, still not right, and the + // identical failure `expandFields` in `plugin-list` already records + // for kanban. No `isProjectableField` gate is needed on THIS half: + // `buildExpandFields` returns a subset of the object's declared + // reference-bearing fields, so an unknown or non-relational grouping + // field is dropped structurally and cannot reach the query. + // + // Only augment when a column list actually narrows the expansion — + // with no columns, `buildExpandFields` already expands every relation + // and the grouping fields are covered by that superset. Passing an + // array here unconditionally would NARROW that case to the grouping + // fields alone. + const expandColumns = schemaColumns ?? schemaFields; + const expand = buildExpandFields( + resolvedSchema?.fields, + expandColumns ? [...(expandColumns as any[]), ...groupingFieldRefs] : undefined, + ); if (expand.length > 0) { params.$expand = expand; } @@ -1783,7 +1861,13 @@ export const ObjectGrid: React.FC = ({ // context object's identity would re-fetch the grid on every render. // `PermissionProvider` reports `true` synchronously and the no-provider // default stays `false` forever, so neither of those pays anything. - }, [objectName, schemaFields, schemaColumns, schemaFilter, schemaSort, headerSort, searchTerm, schemaPagination, schemaPageSize, serverPage, serverPageSize, dataSource, hasInlineData, dataConfig, refreshKey, perms.isLoaded]); + // `groupingProjectionKey` (objectui#7179): the grouping fields are part of + // the projection now, and grouping is RUNTIME-MUTABLE (the toolbar popover + // rewrites it), so without this dep switching the grouping field would leave + // the query asking for the OLD one and the new grouping would read + // `undefined` on every row — the very `(empty)` bucket this card fixes, + // reachable a second way. + }, [objectName, schemaFields, schemaColumns, schemaFilter, schemaSort, headerSort, searchTerm, schemaPagination, schemaPageSize, serverPage, serverPageSize, dataSource, hasInlineData, dataConfig, refreshKey, perms.isLoaded, groupingProjectionKey]); // The same reset, for the path the loader above never runs on (objectui#4501 // clause 2). "All N matching are selected" is a claim about ONE query, so it diff --git a/packages/plugin-grid/src/__tests__/groupingProjection-7179.test.tsx b/packages/plugin-grid/src/__tests__/groupingProjection-7179.test.tsx new file mode 100644 index 0000000000..c507ffeee6 --- /dev/null +++ b/packages/plugin-grid/src/__tests__/groupingProjection-7179.test.tsx @@ -0,0 +1,343 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#7179 — the fields a grid GROUPS BY reach the query. + * + * ## The defect + * + * `$select` was built from `columns` alone. A view declaring + * `grouping: { fields: [{ field: 'business_unit' }] }` on a field absent from + * its columns therefore never asked the server for that field, so it was + * `undefined` on every row by the time grouping ran and `buildSegmentLabel` + * answered `(empty)` for all of them: ONE group holding every record, with no + * error, no warning and no empty state. + * + * ## The `(empty)` guard is NOT the bug and is deliberately untouched + * + * `useGroupedData`'s first line is right for a genuinely empty value and cannot + * distinguish it from a field that was never fetched. The defect is upstream of + * it, in the projection. Nothing here asserts on that guard. + * + * ## Both directions of the hazard are pinned, because they point OPPOSITE ways + * + * The naive union — concatenate every grouping field into `$select` — is + * STRICTLY WORSE than the bug it fixes on any backend that answers an unknown + * select key with an empty result set rather than ignoring it (the cloud + * multi-tenant runtime does exactly that). It would trade one `(empty)` group + * holding all the rows for NO rows, equally silently. So PIN 4 is as + * load-bearing as PIN 1: the fix must widen the projection for a field the + * object HAS and must refuse to widen it for one the object LACKS. + * + * ## `populate`, not just `select` (PIN 6) + * + * A `select` that fetches a bare foreign key without expanding it groups into + * raw id buckets rather than names — better than one `(empty)` bucket, still + * not right. It is the identical failure `plugin-list`'s `expandFields` memo + * already records for kanban. So a lookup grouping field has to reach `$expand` + * too, and that is a separate assertion from the `$select` one. + * + * ## Test-source note + * + * This file imports `../ObjectGrid` relatively and the root vitest config + * aliases `@object-ui/*` to each package's `src`, so no build step stands + * between the edit and the run — the ablation recorded in the PR body reads + * source directly. (Same standing arrangement `projectionFls-6898.test.tsx` + * documents; re-stated rather than cross-referenced so this file is readable + * on its own.) + */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, cleanup } from '@testing-library/react'; +import React from 'react'; + +/** Stable stub identity — `ObjectGrid` carries `perms` in memo dep arrays. */ +const { permsStub, state } = vi.hoisted(() => { + const state: { isLoaded: boolean; readable: string[] } = { + isLoaded: false, + readable: [], + }; + return { + state, + permsStub: { + get isLoaded() { return state.isLoaded; }, + checkField: (_object: string, field: string, action: string) => + action === 'read' ? state.readable.includes(field) : true, + check: () => ({ allowed: true }), + getFieldPermissions: () => [], + getRowFilter: () => undefined, + getObjectApiOperations: () => undefined, + roles: [], + userId: null, + systemPermissions: undefined, + hasCapabilities: () => true, + can: () => true, + cannot: () => false, + }, + }; +}); + +vi.mock('@object-ui/permissions', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, usePermissions: () => permsStub as any }; +}); + +import { ObjectGrid } from '../ObjectGrid'; +import { ActionProvider } from '@object-ui/react'; + +const OBJECT = 'duly_task'; + +/** + * `business_unit` is the grouping field under test — declared, and deliberately + * NOT a column in most cases here. `owner` is the LOOKUP one, for the + * `populate` half. `secret_band` is declared and used for the FLS pin. + * `ghost_field` is deliberately absent from this map: it is the unknown key + * whose job is to be REFUSED. + */ +const OBJECT_FIELDS = { + id: { type: 'text', label: 'Id' }, + subject: { type: 'text', label: 'Subject' }, + status: { type: 'select', label: 'Status' }, + business_unit: { type: 'text', label: 'Business Unit' }, + region: { type: 'text', label: 'Region' }, + secret_band: { type: 'text', label: 'Secret Band' }, + owner: { type: 'lookup', reference: 'sys_user', label: 'Owner' }, +}; + +const makeDataSource = () => ({ + find: vi.fn().mockResolvedValue({ data: [], total: 0 }), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + getObjectSchema: vi.fn(async () => ({ name: OBJECT, fields: OBJECT_FIELDS })), +}); + +/** Render a grid and return the params it actually asked the server for. */ +const paramsFor = async (schemaExtra: Record) => { + const ds = makeDataSource(); + const schema: any = { type: 'object-grid', objectName: OBJECT, ...schemaExtra }; + render( + + + , + ); + await vi.waitFor(() => expect(ds.find).toHaveBeenCalled()); + const call = ds.find.mock.calls.at(-1)?.[1] ?? {}; + return { + select: (call.$select ?? []) as string[], + expand: (call.$expand ?? []) as string[], + }; +}; + +const selectFor = async (schemaExtra: Record) => + (await paramsFor(schemaExtra)).select; + +const grouping = (...fields: string[]) => ({ + fields: fields.map((field) => ({ field, order: 'asc', collapsed: false })), +}); + +beforeEach(() => { + vi.clearAllMocks(); + // Default OFF, so the projection-shape pins below measure the union and not + // the FLS gate. The FLS pin turns it on explicitly. + state.isLoaded = false; + state.readable = []; +}); +afterEach(() => cleanup()); + +describe('ObjectGrid — `grouping` fields reach the projection (objectui#7179)', () => { + // ── PIN 1: THE DEFECT ─────────────────────────────────────────────────── + it('asks the server for a grouping field that is NOT one of the columns', async () => { + const select = await selectFor({ + columns: ['subject', 'status'], + grouping: grouping('business_unit'), + }); + expect( + select, + 'the grid declared `grouping` on `business_unit` and never asked the server for it, ' + + 'so it is `undefined` on every row and every record lands in one `(empty)` group', + ).toContain('business_unit'); + }); + + it('still asks for the columns themselves — grouping widens, it never replaces', async () => { + const select = await selectFor({ + columns: ['subject', 'status'], + grouping: grouping('business_unit'), + }); + expect(select).toContain('subject'); + expect(select).toContain('status'); + }); + + // ── PIN 2: A UNION, NOT AN APPEND ─────────────────────────────────────── + it('does not duplicate a grouping field that is already a column', async () => { + const select = await selectFor({ + columns: ['subject', 'business_unit'], + grouping: grouping('business_unit'), + }); + expect( + select.filter((f) => f === 'business_unit'), + 'the projection must be a UNION — an append would send `business_unit` twice', + ).toHaveLength(1); + }); + + it('leaves the projection unchanged when the grouping field is already a column', async () => { + const withGrouping = await selectFor({ + columns: ['subject', 'business_unit'], + grouping: grouping('business_unit'), + }); + const withoutGrouping = await selectFor({ columns: ['subject', 'business_unit'] }); + expect([...withGrouping].sort()).toEqual([...withoutGrouping].sort()); + }); + + // ── PIN 3: `fields` IS AN ARRAY — multi-level grouping ────────────────── + it('covers every entry of a multi-level `grouping.fields[]`, not just the first', async () => { + const select = await selectFor({ + columns: ['subject'], + grouping: grouping('business_unit', 'region'), + }); + expect(select).toContain('business_unit'); + expect(select, 'the key is an ARRAY — a first-entry-only read regresses nested grouping') + .toContain('region'); + }); + + // ── PIN 4: THE HAZARD — an unknown key must NOT reach the query ───────── + it('REFUSES a grouping field the object does not declare', async () => { + const select = await selectFor({ + columns: ['subject', 'status'], + grouping: grouping('ghost_field'), + }); + expect( + select, + 'some backends answer an unknown `$select` key with an EMPTY RESULT SET rather than ' + + 'ignoring it, so an unguarded union would turn one `(empty)` group holding every row ' + + 'into NO rows — strictly worse than the bug being fixed, and equally silent', + ).not.toContain('ghost_field'); + }); + + it('still returns a usable projection when the grouping field is unknown — the list is not zeroed', async () => { + const select = await selectFor({ + columns: ['subject', 'status'], + grouping: grouping('ghost_field'), + }); + // The positive half of PIN 4: refusing the unknown key must not collapse + // the projection to nothing, which would be its own way of blanking a view. + expect(select).toContain('subject'); + expect(select).toContain('status'); + expect(select).toContain('id'); + }); + + it('keeps the KNOWN entries of a mixed grouping block and drops only the unknown one', async () => { + const select = await selectFor({ + columns: ['subject'], + grouping: grouping('business_unit', 'ghost_field'), + }); + expect(select).toContain('business_unit'); + expect(select).not.toContain('ghost_field'); + }); + + // ── PIN 5: FLS (objectui#6898 must stay closed) ───────────────────────── + it('does NOT ask for a grouping field the principal cannot read', async () => { + state.isLoaded = true; + state.readable = ['id', 'subject', 'status', 'business_unit']; + const select = await selectFor({ + columns: ['subject', 'status'], + grouping: grouping('secret_band'), + }); + expect( + select, + 'a grouping field names a field just as capable of being denied as a column is, and the ' + + 'projection is the half that goes on the WIRE — unioning after the FLS filter reopens ' + + 'objectui#6898 through a new door', + ).not.toContain('secret_band'); + }); + + it('still asks for a readable grouping field when the gate is live', async () => { + state.isLoaded = true; + state.readable = ['id', 'subject', 'status', 'business_unit']; + const select = await selectFor({ + columns: ['subject', 'status'], + grouping: grouping('business_unit'), + }); + expect(select, 'the gate narrows, it never empties').toContain('business_unit'); + }); + + // ── PIN 6: `populate`, not just `select` ──────────────────────────────── + it('EXPANDS a lookup grouping field that is not a column', async () => { + const { expand } = await paramsFor({ + columns: ['subject', 'status'], + grouping: grouping('owner'), + }); + expect( + expand, + 'a `select` that fetches a bare FK without populating it groups into raw id buckets ' + + 'instead of names — better than one `(empty)` bucket, still the wrong answer', + ).toContain('owner'); + }); + + it('does not expand a NON-relational grouping field', async () => { + const { expand } = await paramsFor({ + columns: ['subject'], + grouping: grouping('business_unit'), + }); + expect(expand).not.toContain('business_unit'); + }); + + it('does not expand an UNKNOWN grouping field', async () => { + const { expand } = await paramsFor({ + columns: ['subject'], + grouping: grouping('ghost_field'), + }); + expect( + expand, + '`buildExpandFields` returns a subset of the declared reference-bearing fields, so this ' + + 'is structural — the pin exists so a future refactor cannot make it accidental', + ).not.toContain('ghost_field'); + }); + + // ── PIN 7: THE `fields` ARM, not only the `columns` arm ───────────────── + it('unions the grouping field into the `fields` arm too', async () => { + const select = await selectFor({ + fields: ['subject', 'status'], + grouping: grouping('business_unit'), + }); + expect( + select, + 'the projection has TWO arms and a fix landing on only one leaves the other blind — ' + + 'the exact failure mode this card was dispatched to avoid', + ).toContain('business_unit'); + }); + + // ── PIN 8: a malformed block contributes nothing, and never throws ────── + it('is inert for a grid with no grouping at all', async () => { + const withNoGrouping = await selectFor({ columns: ['subject', 'status'] }); + expect([...withNoGrouping].sort()).toEqual(['id', 'status', 'subject']); + }); + + it('survives a malformed `grouping` block without poisoning the projection', async () => { + const select = await selectFor({ + columns: ['subject', 'status'], + // Every entry here is off-spec: a bare string (the tempting shorthand + // `GroupingFieldSchema` does NOT accept), an entry with no `field`, and a + // non-string `field`. None may contribute a name. + // + // ⚠️ A `null` ENTRY IS DELIBERATELY ABSENT FROM THIS FIXTURE, and its + // absence is a finding rather than an oversight. The harvester handles + // `null` (pinned directly in `core`'s `grouping-fields.test.ts`, which + // needs no grid to mount), but this component's `groupValueFormatter` + // memo dereferences `gf.field` with no null guard, so a `null` entry + // throws a TypeError and takes the whole grid down before any projection + // is built. That is a pre-existing crash on `origin/main`, a different + // defect class from this card's silent wrong answer, and it is filed + // separately rather than ridden here. + grouping: { fields: ['business_unit', {}, { field: 42 }] }, + }); + expect(select).toContain('subject'); + expect(select).not.toContain('42'); + expect(select).not.toContain('business_unit'); + }); +}); diff --git a/packages/plugin-list/src/ListView.tsx b/packages/plugin-list/src/ListView.tsx index c188d6eadb..8a7418a77a 100644 --- a/packages/plugin-list/src/ListView.tsx +++ b/packages/plugin-list/src/ListView.tsx @@ -21,7 +21,7 @@ import { useDensityMode } from '@object-ui/react'; import type { ListViewSchema, ObjectMapConfig } from '@object-ui/types'; import { detectStatusField } from '@object-ui/types'; import { usePullToRefresh } from '@object-ui/mobile'; -import { resolveConditionalFormatting, buildExpandFields, buildExportFileName, resolveEffectiveCrudAffordances, isObjectInlineEditable, partitionRowsByPredicate, normalizeListViewSchema, rowHeightToDensityMode, mergeFilterNodes, columnIdentity, collectPredicateFieldRefs, listViewPredicates, PLATFORM_RECORD_COLUMNS, EXPANDABLE_FIELD_TYPES, UNMATERIALIZED_FIELD_TYPES, readObjectSortability, isPlatformSortableField, filterPlatformSortableSort } from '@object-ui/core'; +import { resolveConditionalFormatting, buildExpandFields, buildExportFileName, resolveEffectiveCrudAffordances, isObjectInlineEditable, partitionRowsByPredicate, normalizeListViewSchema, rowHeightToDensityMode, mergeFilterNodes, columnIdentity, collectPredicateFieldRefs, collectGroupingFieldRefs, listViewPredicates, PLATFORM_RECORD_COLUMNS, EXPANDABLE_FIELD_TYPES, UNMATERIALIZED_FIELD_TYPES, readObjectSortability, isPlatformSortableField, filterPlatformSortableSort } from '@object-ui/core'; import { useObjectTranslation, useObjectLabel, useSafeFieldLabel, createSafeTranslation, useDisplayLocale } from '@object-ui/i18n'; // Two resolvers, two vocabularies — the repo spells the distinction into the // NAMES (objectui#4167). `resolveInlineI18nLabel` is the spec's own @@ -1475,10 +1475,26 @@ export const ListView = React.forwardRef(({ collectViewFields((schema as any).options?.timeline); collectViewFields((schema as any).gantt); collectViewFields((schema as any).options?.gantt); + // [objectui#7179] The GRID's grouping block, which this collector had no + // arm for: it reads `groupByField` (kanban / gantt / timeline) but the grid + // groups through `grouping.fields[]`, a different key with a different + // shape. Without this, a grid grouped by a LOOKUP gets the field into + // `$select` but never into `populate`, so every row carries the bare + // foreign key and the groups bucket by raw id instead of by name — the + // exact failure the comment above this memo records for kanban + // ("list view shows 'Initech Solutions' but kanban used to show + // '8UY9zHWBfjYjYor4'"). Better than one `(empty)` bucket, still wrong. + // + // Unguarded is safe HERE and only here: `buildExpandFields` returns a + // subset of the object's declared reference-bearing fields, so a grouping + // field the object does not have — or has as a non-relation — is dropped + // structurally. The `$select` half below needs a real gate, and takes one. + for (const f of collectGroupingFieldRefs(groupingConfig)) collected.add(f); const augmented = collected.size > 0 ? Array.from(collected) : undefined; return buildExpandFields(objectDef?.fields, augmented); }, [ objectDef?.fields, + groupingConfig, schema.columns, (schema as any).kanban, (schema as any).calendar, @@ -1704,6 +1720,38 @@ export const ListView = React.forwardRef(({ // everyone. Routed through addSpeculative for the same reason the view // bindings are: a typo'd predicate must not put an unknown column in // `$select` and zero the whole list. + // [objectui#7179] The GRID's grouping fields. `collectViewFields` + // above covers every OTHER view kind's grouping — they all spell it + // `groupByField`, a plain string — but the grid spells it + // `grouping.fields[]`, so it had no arm here and its projection was + // built from `columns` alone. The measured symptom: one group + // labelled `(empty)` holding every row, no error, no warning. + // + // Through `addSpeculative` for the reason stated at its definition, + // which applies to a `grouping.fields[]` entry more sharply than to + // anything else routed through it: this card's whole premise is that + // the grouping field is NOT a column, so it has never been through + // column validation, and `GroupingFieldSchema.field` is a bare + // string. Unioned unguarded on a backend that rejects unknown + // `$select` keys with an empty result set, it would turn one + // `(empty)` group holding 186 rows into ZERO rows, just as silently. + // + // FLS-gated on top (objectui#6898): the grid half of this same fix + // takes `passesProjectionGate`, and a grouping field can name a + // denied field exactly as a column can. Safe to ask `checkField` + // here precisely BECAUSE `addSpeculative` ran first — everything + // reaching this point is a field the object declares, so the + // "`checkField` answers false for an undeclared key" trap that makes + // this gate wrong on derived columns cannot fire. + const addGroupingField = (f: string) => { + if (perms?.isLoaded && schema.objectName + && knownObjectFields?.has(f) + && !PLATFORM_RECORD_COLUMNS.has(f) + && !perms.checkField(schema.objectName, f, 'read')) return; + addSpeculative(f); + }; + for (const f of collectGroupingFieldRefs(groupingConfig)) addGroupingField(f); + for (const f of collectPredicateFieldRefs(listViewPredicates({ conditionalFormatting: schema.conditionalFormatting as unknown[] | undefined, rowActionDefs: (schema as any).rowActionDefs, diff --git a/packages/plugin-list/src/__tests__/ListView.groupingProjection-7179.test.tsx b/packages/plugin-list/src/__tests__/ListView.groupingProjection-7179.test.tsx new file mode 100644 index 0000000000..437497ca36 --- /dev/null +++ b/packages/plugin-list/src/__tests__/ListView.groupingProjection-7179.test.tsx @@ -0,0 +1,222 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#7179 — the SECOND projection build site. + * + * The card was dispatched on the assumption that `$select` is assembled in ONE + * place. It is assembled in two, and they are independent: `ObjectGrid` builds + * its own when it fetches for itself, and `ListView` builds one when it fetches + * and hands the rows down to the grid. `plugin-grid`'s + * `groupingProjection-7179.test.tsx` pins the first. This pins the second, and + * the two files exist separately because a fix landing on only one of them is + * the specific failure mode this card was dispatched to avoid — a grid that + * groups correctly on one mounting path and shows one `(empty)` bucket on the + * other. + * + * This builder had an arm for every OTHER view kind's grouping already: + * `collectViewFields` reads `groupByField` for kanban, gantt and timeline. The + * grid was the only projection-blind path of the four because it spells the + * same intent differently — `grouping.fields[]`, an array of objects — so it + * matched none of the candidate keys. + * + * Both halves of the query are pinned here, because `$select` alone is not the + * whole fix: this builder's `expandFields` memo carries a comment recording the + * exact failure a `select` without a `populate` produces for a lookup ("list + * view shows 'Initech Solutions' but kanban used to show '8UY9zHWBfjYjYor4'"), + * and a grid grouped by a lookup would land in it. + */ +import React from 'react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, waitFor, cleanup } from '@testing-library/react'; +import { ListView } from '../ListView'; +import { SchemaRendererProvider } from '@object-ui/react'; + +const OBJECT = 'duly_task'; + +/** + * `ghost_field` is deliberately absent: it is the unknown key whose job is to + * be refused. `owner` is the lookup, for the `populate` half. + */ +const objectDef = { + name: OBJECT, + label: 'Task', + fields: { + id: { name: 'id', type: 'text' }, + subject: { name: 'subject', type: 'text', label: 'Subject' }, + status: { name: 'status', type: 'select', label: 'Status' }, + business_unit: { name: 'business_unit', type: 'text', label: 'Business Unit' }, + region: { name: 'region', type: 'text', label: 'Region' }, + owner: { name: 'owner', type: 'lookup', reference: 'sys_user', label: 'Owner' }, + }, +}; + +const makeDataSource = () => + ({ + find: vi.fn(async () => ({ data: [], total: 0 })), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + getObjectSchema: vi.fn(async () => objectDef), + }) as any; + +/** Mount a list view and return the params it actually asked the server for. */ +async function paramsFor(schemaExtra: Record) { + const dataSource = makeDataSource(); + const schema: any = { + type: 'list-view', + objectName: OBJECT, + viewType: 'grid', + ...schemaExtra, + }; + render( + + + , + ); + await waitFor(() => expect(dataSource.find).toHaveBeenCalled()); + const call = dataSource.find.mock.calls.at(-1)?.[1] ?? {}; + return { + select: (call.$select ?? []) as string[], + expand: (call.$expand ?? []) as string[], + }; +} + +const selectFor = async (schemaExtra: Record) => + (await paramsFor(schemaExtra)).select; + +const grouping = (...fields: string[]) => ({ + fields: fields.map((field) => ({ field, order: 'asc', collapsed: false })), +}); + +beforeEach(() => vi.clearAllMocks()); +afterEach(() => cleanup()); + +describe('ListView — `grouping` fields reach the projection (objectui#7179)', () => { + // ── PIN 1: THE DEFECT, on the second build site ───────────────────────── + it('asks the server for a grouping field that is NOT one of the columns', async () => { + const select = await selectFor({ + columns: ['subject', 'status'], + grouping: grouping('business_unit'), + }); + expect( + select, + 'this builder already unions `groupByField` for kanban / gantt / timeline; the grid ' + + 'spells the same intent as `grouping.fields[]` and matched none of the candidate keys', + ).toContain('business_unit'); + }); + + it('still asks for the columns themselves', async () => { + const select = await selectFor({ + columns: ['subject', 'status'], + grouping: grouping('business_unit'), + }); + expect(select).toContain('subject'); + expect(select).toContain('status'); + }); + + // ── PIN 2: the `groupBy` shorthand the view designer writes ───────────── + it('covers the `groupBy` shorthand, which normalizes into the same block', async () => { + const select = await selectFor({ columns: ['subject'], groupBy: 'business_unit' }); + expect( + select, + '`groupBy` / `groupBy2` are what the visual view editor writes; they are normalized into ' + + 'a GroupingConfig before render, so the projection must follow the normalized value ' + + 'rather than the authored key', + ).toContain('business_unit'); + }); + + it('covers the second level of the `groupBy2` shorthand too', async () => { + const select = await selectFor({ + columns: ['subject'], + groupBy: 'business_unit', + groupBy2: 'region', + }); + expect(select).toContain('business_unit'); + expect(select).toContain('region'); + }); + + // ── PIN 3: A UNION, NOT AN APPEND ─────────────────────────────────────── + it('does not duplicate a grouping field that is already a column', async () => { + const select = await selectFor({ + columns: ['subject', 'business_unit'], + grouping: grouping('business_unit'), + }); + expect(select.filter((f) => f === 'business_unit')).toHaveLength(1); + }); + + it('leaves the projection unchanged when the grouping field is already a column', async () => { + const withGrouping = await selectFor({ + columns: ['subject', 'business_unit'], + grouping: grouping('business_unit'), + }); + const withoutGrouping = await selectFor({ columns: ['subject', 'business_unit'] }); + expect([...withGrouping].sort()).toEqual([...withoutGrouping].sort()); + }); + + // ── PIN 4: THE HAZARD — an unknown key must NOT zero the list ─────────── + it('REFUSES a grouping field the object does not declare', async () => { + const select = await selectFor({ + columns: ['subject', 'status'], + grouping: grouping('ghost_field'), + }); + expect( + select, + 'this builder states the reason in terms: "some backends reject unknown select keys with ' + + 'an empty result set rather than ignoring them ... a single unknown column in $select ' + + 'silently zeroes the whole list"', + ).not.toContain('ghost_field'); + }); + + it('still returns a usable projection when the grouping field is unknown', async () => { + const select = await selectFor({ + columns: ['subject', 'status'], + grouping: grouping('ghost_field'), + }); + expect(select).toContain('subject'); + expect(select).toContain('status'); + expect(select).toContain('id'); + }); + + it('keeps the KNOWN entries of a mixed grouping block and drops only the unknown one', async () => { + const select = await selectFor({ + columns: ['subject'], + grouping: grouping('business_unit', 'ghost_field'), + }); + expect(select).toContain('business_unit'); + expect(select).not.toContain('ghost_field'); + }); + + // ── PIN 5: `populate`, not just `select` ──────────────────────────────── + it('EXPANDS a lookup grouping field that is not a column', async () => { + const { expand } = await paramsFor({ + columns: ['subject', 'status'], + grouping: grouping('owner'), + }); + expect( + expand, + 'without this the server returns the bare FK and the grid buckets by raw id instead of ' + + 'by name — the failure this memo already records for kanban', + ).toContain('owner'); + }); + + it('does not expand an unknown grouping field', async () => { + const { expand } = await paramsFor({ + columns: ['subject'], + grouping: grouping('ghost_field'), + }); + expect(expand).not.toContain('ghost_field'); + }); + + // ── PIN 6: inert without grouping ─────────────────────────────────────── + it('leaves a grid with no grouping exactly as it was', async () => { + const select = await selectFor({ columns: ['subject', 'status'] }); + expect([...select].sort()).toEqual(['id', 'status', 'subject']); + }); +});