diff --git a/.changeset/10223-lookup-candidates-expand.md b/.changeset/10223-lookup-candidates-expand.md new file mode 100644 index 0000000000..9c7fb95d56 --- /dev/null +++ b/.changeset/10223-lookup-candidates-expand.md @@ -0,0 +1,40 @@ +--- +'@object-ui/fields': patch +--- + +fix(fields): a lookup's candidate queries expand the reference columns they display + +Opening a lookup field's dropdown sent the candidate query without `$expand`. +Every reference column the dropdown previews under each candidate (by default +the referenced object's leading `highlightFields`) therefore arrived as a bare +foreign key, and the lookup cell renderer fetched each related record on its +own: one extra request per candidate per such column, on every open. The +browse-all picker (`RecordPickerDialog`) did the same once per table row. + +Both queries now ask for `$expand` on the reference columns they display, +chosen by `buildExpandFields` from `@object-ui/core` (the rule the list views +apply to their visible columns): the dropdown's previewed columns, and the +picker's columns other than its id column. The dropdown's recently-used rail +asks for the same expansion. Related records in the expanded columns now +render with no request per row. + +Field-level security gates that list the way it gates the other +`buildExpandFields` call sites: once the permission policy has loaded, a +reference column the user may not read on the referenced object is left out of +`$expand`; before the policy loads, nothing is filtered. `@object-ui/fields` +now depends on `@object-ui/permissions` for that check. A column left out of +`$expand`, like any column from a backend that ignores the parameter, still +arrives as a bare id and is resolved one by one as before. + +`buildExpandFields` covers `user` columns as well as `lookup`, +`master_detail` and `tree`. A previewed `user` column therefore now shows the +person, as the picker table and the list views do, where it used to show the +raw id marked as unresolved. + +What the dropdown and the picker hand onward is built from each row with its +expanded relations collapsed back to ids, through `toPredicateRecord` from +`@object-ui/core`. That covers option labels, `titleFormat` titles, the +committed value, and the records passed to `onSelectRecord` and +`onSelectRecords`, so none of them carries an expanded object. One difference +remains: `toPredicateRecord` returns ids as strings, so a numeric foreign key +in an expanded column reaches those callbacks in its string form. diff --git a/.changeset/lookup-dropdown-cell-renderer-5492.md b/.changeset/lookup-dropdown-cell-renderer-5492.md index d43f0e19a2..802c997fbc 100644 --- a/.changeset/lookup-dropdown-cell-renderer-5492.md +++ b/.changeset/lookup-dropdown-cell-renderer-5492.md @@ -27,11 +27,14 @@ there is a single renderer left to drift from. The dropdown's extra columns are rendered into the option row itself; the row's `title` keeps the full option label, which is what a truncated label needs, instead of a raw-value dump. -No query changed and no contract widened. `lookupColumns` entries stay bare -field names — no dot paths, no populate/expand semantics — because neither -surface's request carries populate to begin with: the picker resolves a +This change touched no query and widened no contract. `lookupColumns` entries +stay bare field names — no dot paths, no populate/expand semantics — because +at the time neither surface's request carried populate: the picker resolved a foreign-key id to a name client-side, in the lookup cell renderer, and the -dropdown now inherits exactly that. An unresolved reference therefore renders +dropdown inherited exactly that. A later change (objectui#10223) has both +requests ask for `$expand` on the reference columns they display, minus any +the loaded permission policy denies; a value that still arrives as a bare id +is resolved by that same cell renderer. An unresolved reference therefore renders what the picker renders for it, and keeps its column: a slot is dropped only when the record holds no value for the field, decided on the raw value and never on what the renderer makes of it, so an unresolved id can never degrade diff --git a/packages/fields/package.json b/packages/fields/package.json index 8177869d7c..564873eccd 100644 --- a/packages/fields/package.json +++ b/packages/fields/package.json @@ -33,6 +33,7 @@ "@object-ui/components": "workspace:*", "@object-ui/core": "workspace:*", "@object-ui/i18n": "workspace:*", + "@object-ui/permissions": "workspace:*", "@object-ui/providers": "workspace:*", "@object-ui/react": "workspace:*", "@object-ui/types": "workspace:*", diff --git a/packages/fields/src/widgets/LookupField.candidateExpand-10223.test.tsx b/packages/fields/src/widgets/LookupField.candidateExpand-10223.test.tsx new file mode 100644 index 0000000000..a5c77aa633 --- /dev/null +++ b/packages/fields/src/widgets/LookupField.candidateExpand-10223.test.tsx @@ -0,0 +1,537 @@ +/** + * 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. + */ + +/** + * One dropdown open, one candidate query — objectui#10223. + * + * Reported from a real deployment: a scheduling form's lookup to a + * `task_version` object, whose `highlightFields` include the master_detail + * field `task`. The lookup declares no `lookup_columns`, so the dropdown + * previews the first highlight columns — `task` among them. The candidate + * query sent no `$expand`, every candidate's `task` came back as a bare + * foreign key, and the lookup cell renderer resolved each one with its own + * `findOne`: one request per candidate on every open, on top of the list + * query. The display was right; the load was not. + * + * The fixture mirrors that shape: a full page of candidates, one master_detail + * column inside the derived preview columns, no `lookup_columns`. What is + * pinned: + * + * - the open costs ONE candidate query, and it carries `$expand` for exactly + * the previewed reference column — no per-row `findOne` follows, and the + * subtitle still names each related record; + * - a previewed `user` column rides the same rule (`buildExpandFields` is the + * one reference-bearing family), and now names the person; + * - the control: with no reference column previewed the query carries no + * `$expand` key at all — nor with no column previewed at all (the display + * field alone), where an empty list would otherwise mean "every relation"; + * - the browse-all picker behind the dropdown, the same way; + * - field-level security gates the expansion, as at every other + * `buildExpandFields` call site: a relation the loaded policy denies is not + * asked for (dropdown, recents rail, picker), a readable one is, and with + * no policy loaded nothing is filtered; + * - expansion changes nothing a host or a user reads besides the request + * count. The option labels (including a `titleFormat` template that names + * the expanded field), the preview text, and the record handed to + * `onSelectRecord` / `onSelectRecords` are the same whether or not the + * backend honours `$expand` — the second backend returns bare ids, which is + * also the fallback for a backend that ignores the parameter. + */ + +import * as React from 'react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, cleanup, waitFor, act, fireEvent } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { SchemaRendererContext } from '@object-ui/react'; +import { PermissionProvider } from '@object-ui/permissions'; +import { LookupField } from './LookupField'; +import { RecordPickerDialog } from './RecordPickerDialog'; +import { pushRecentLookupId } from './recentLookups'; +import { getCellRenderer } from '../index'; + +/** A full dropdown page — the page size the inline dropdown asks for. */ +const CANDIDATES = 50; + +const TASK_VERSION_FIELDS: Record = { + name: { type: 'text', label: 'Name' }, + code: { type: 'text', label: 'Code' }, + task: { type: 'master_detail', label: 'Task', reference_to: 'task' }, + version: { type: 'number', label: 'Version' }, + owner: { type: 'user', label: 'Owner', reference_to: 'sys_user' }, +}; + +interface BackendOptions { + /** + * Unique per test. The lookup cell renderer's name cache is module-level, so + * a task id resolved by one test would read as "no fetch needed" in the next. + */ + prefix: string; + highlightFields: string[]; + /** A backend that ignores `$expand` returns bare foreign keys. */ + honoursExpand?: boolean; + titleFormat?: string; + /** The referenced object's declared fields (default `TASK_VERSION_FIELDS`). */ + fields?: Record; +} + +function makeBackend({ + prefix, + highlightFields, + honoursExpand = true, + titleFormat, + fields = TASK_VERSION_FIELDS, +}: BackendOptions) { + const tasks: Record = {}; + const users: Record = {}; + const rows: Record[] = []; + for (let i = 0; i < CANDIDATES; i++) { + const taskId = `${prefix}_task_${i}`; + const userId = `${prefix}_user_${i}`; + tasks[taskId] = { id: taskId, name: `Assembly step ${i}` }; + users[userId] = { id: userId, name: `Owner ${i}` }; + rows.push({ id: `${prefix}_tv_${i}`, name: `TV-${i}`, code: `C-${i}`, task: taskId, version: i + 1, owner: userId }); + } + /** What `$expand` puts in place of each relation's key. */ + const related: Record> = { task: tasks, owner: users }; + + const find = vi.fn(async (objectName: string, params?: Record) => { + if (objectName !== 'task_version') return { data: [], total: 0 }; + const expand: string[] = honoursExpand && Array.isArray(params?.$expand) ? params!.$expand : []; + const skip = Number(params?.$skip ?? 0); + const top = Number(params?.$top ?? rows.length); + const data = rows.slice(skip, skip + top).map((r) => { + const row = { ...r }; + for (const f of expand) if (related[f]) row[f] = related[f][r[f]]; + return row; + }); + return { data, total: rows.length }; + }); + const findOne = vi.fn(async (objectName: string, id: string) => + objectName === 'task' ? tasks[id] ?? null : null, + ); + const getObjectSchema = vi.fn(async (objectName: string) => { + if (objectName === 'task_version') { + return { + name: 'task_version', + fields, + highlightFields, + ...(titleFormat ? { titleFormat } : {}), + }; + } + if (objectName === 'task') { + return { name: 'task', fields: { name: { type: 'text', label: 'Name' } } }; + } + return undefined; + }); + + return { dataSource: { find, findOne, getObjectSchema } as any, rows }; +} + +type Backend = ReturnType; + +/** The candidate queries — `find` on the referenced object itself. */ +function candidateQueries(backend: Backend): Array> { + return backend.dataSource.find.mock.calls + .filter(([objectName]: [string]) => objectName === 'task_version') + .map(([, params]: [string, Record]) => params); +} + +/** Every read that is NOT a candidate query: per-row resolution of `task`. */ +function perRowReads(backend: Backend): number { + const finds = backend.dataSource.find.mock.calls.filter( + ([objectName]: [string]) => objectName !== 'task_version', + ).length; + return finds + backend.dataSource.findOne.mock.calls.length; +} + +/** + * Macrotask flushes, as in `LookupField.pickerAgreement.test.tsx`: the schema + * fetches, any per-id resolution and the re-render that carries a resolved + * name all settle, without a predicate that would have to encode the answer. + */ +async function settle(): Promise { + for (let i = 0; i < 6; i++) { + await act(async () => { + await new Promise((r) => setTimeout(r, 0)); + }); + } +} + +/** + * Mount the lookup the way a form does and wait for the referenced schema, + * which lands at mount — before any user can reach the trigger. + */ +/** Wraps the rendered tree — a permission provider, or nothing. */ +type Wrap = (node: React.ReactElement) => React.ReactElement; +const bare: Wrap = (node) => node; + +async function mountLookup( + backend: Backend, + extra: Record = {}, + wrap: Wrap = bare, +): Promise { + render( + wrap( + + {}} + dataSource={backend.dataSource} + field={{ reference_to: 'task_version' } as never} + {...(extra as object)} + /> + , + ), + ); + await waitFor(() => expect(backend.dataSource.getObjectSchema).toHaveBeenCalledWith('task_version')); + await settle(); +} + +async function openDropdown( + backend: Backend, + extra: Record = {}, + wrap: Wrap = bare, +): Promise { + await mountLookup(backend, extra, wrap); + await act(async () => { + fireEvent.click(screen.getByTestId('lookup-trigger')); + }); + await waitFor(() => expect(screen.getAllByRole('option')).toHaveLength(CANDIDATES)); + await settle(); +} + +function previewTexts(field: string): string[] { + return Array.from(document.querySelectorAll(`[data-lookup-preview="${field}"]`)).map( + (el) => el.textContent ?? '', + ); +} + +afterEach(() => { + cleanup(); + // Selecting an option records it as "recently used"; keep tests independent. + try { + localStorage.clear(); + } catch { + /* no storage in this environment */ + } +}); + +describe('LookupField — the dropdown expands the reference columns it previews (objectui#10223)', () => { + it('one open costs one candidate query carrying `$expand`, and no per-row read', async () => { + const backend = makeBackend({ prefix: 'card', highlightFields: ['code', 'task', 'version'] }); + await openDropdown(backend); + + const queries = candidateQueries(backend); + expect(queries).toHaveLength(1); + expect(queries[0].$expand).toEqual(['task']); + expect(perRowReads(backend)).toBe(0); + + // The subtitle still names every related record. + const tasks = previewTexts('task'); + expect(tasks).toHaveLength(CANDIDATES); + expect(tasks[0]).toBe('Assembly step 0'); + expect(tasks[CANDIDATES - 1]).toBe(`Assembly step ${CANDIDATES - 1}`); + }); + + it('a previewed `user` column is expanded by the same rule, and names the person', async () => { + const backend = makeBackend({ prefix: 'usr', highlightFields: ['code', 'owner'] }); + await openDropdown(backend); + + const queries = candidateQueries(backend); + expect(queries).toHaveLength(1); + expect(queries[0].$expand).toEqual(['owner']); + expect(perRowReads(backend)).toBe(0); + // The user cell renderer names an expanded record; a bare id it can only + // mark as unresolved. + const owners = previewTexts('owner'); + expect(owners[0]).toContain('Owner 0'); + expect(owners[0]).not.toContain('usr_user_0'); + }); + + it('control: with no reference column previewed, the query carries no `$expand` key', async () => { + const backend = makeBackend({ prefix: 'ctrl', highlightFields: ['code', 'version'] }); + await openDropdown(backend); + + const queries = candidateQueries(backend); + expect(queries).toHaveLength(1); + expect('$expand' in queries[0]).toBe(false); + expect(perRowReads(backend)).toBe(0); + expect(previewTexts('task')).toHaveLength(0); + expect(previewTexts('code')[0]).toBe('C-0'); + }); + + it('the browse-all picker behind it expands the same column, and reads nothing per row', async () => { + const backend = makeBackend({ prefix: 'pick', highlightFields: ['code', 'task', 'version'] }); + await mountLookup(backend); + await act(async () => { + fireEvent.click(screen.getByTestId('browse-all-records')); + }); + await waitFor(() => expect(screen.getByText('TV-0')).toBeInTheDocument()); + await settle(); + + const queries = candidateQueries(backend); + expect(queries).toHaveLength(1); + expect(queries[0].$expand).toEqual(['task']); + expect(perRowReads(backend)).toBe(0); + + const cells = Array.from(document.querySelectorAll('[data-lookup-cell="task"]')); + expect(cells.length).toBeGreaterThan(0); + expect(cells[0].textContent).toBe('Assembly step 0'); + }); +}); + +describe('LookupField — no displayed reference column, no `$expand` (objectui#10223)', () => { + /** + * `buildExpandFields` reads an EMPTY column list as "no restriction" and + * returns every relation the object declares. A dropdown whose picker + * columns are the display field alone previews nothing, so it must not + * hand that function an empty list: the object below declares audit and + * ownership relations (`created_by`, `owner_id`) plus the master_detail + * `task` and the `user` field `owner`, and the dropdown renders none of them. + */ + const WITH_AUDIT_RELATIONS: Record = { + ...TASK_VERSION_FIELDS, + created_by: { type: 'user', label: 'Created By', reference_to: 'sys_user' }, + owner_id: { type: 'lookup', label: 'Owner', reference_to: 'sys_user' }, + }; + + it('dropdown and recents rail: `highlightFields` naming only the display field send no `$expand` key', async () => { + const backend = makeBackend({ prefix: 'nameonly', highlightFields: ['name'], fields: WITH_AUDIT_RELATIONS }); + pushRecentLookupId('task_version', 'nameonly_tv_1'); + await openDropdown(backend); + + const queries = candidateQueries(backend); + const recents = queries.filter((p) => JSON.stringify(p.$filter ?? {}).includes('$in')); + const main = queries.filter((p) => !recents.includes(p)); + expect(main).toHaveLength(1); + expect(recents).toHaveLength(1); + expect('$expand' in main[0]).toBe(false); + expect('$expand' in recents[0]).toBe(false); + expect(previewTexts('task')).toHaveLength(0); + }); + + it('picker: no rendered column besides the id sends no `$expand` key', async () => { + const backend = makeBackend({ prefix: 'idonly', highlightFields: ['name'], fields: WITH_AUDIT_RELATIONS }); + render( + + {}} + dataSource={backend.dataSource} + objectName="task_version" + displayField="name" + columns={['id']} + onSelect={() => {}} + cellRenderer={getCellRenderer} + fieldsMeta={WITH_AUDIT_RELATIONS} + /> + , + ); + await waitFor(() => expect(screen.getByTestId('record-row-idonly_tv_0')).toBeInTheDocument()); + const queries = candidateQueries(backend); + expect(queries).toHaveLength(1); + expect('$expand' in queries[0]).toBe(false); + }); +}); + +describe('LookupField — expansion changes the request count and nothing else (objectui#10223)', () => { + /** + * `titleFormat` names the expanded field on purpose: a template that + * printed an expanded record would print an object where it used to print + * the key. + */ + const TITLE_FORMAT = '{code} - {task}'; + + interface DropdownReading { + labels: string[]; + previews: string[]; + picked: Record; + } + + async function readDropdown(honoursExpand: boolean): Promise { + const backend = makeBackend({ + prefix: 'same', + highlightFields: ['code', 'task', 'version'], + honoursExpand, + titleFormat: TITLE_FORMAT, + }); + const onSelectRecord = vi.fn(); + await openDropdown(backend, { onSelectRecord }); + const options = screen.getAllByRole('option').slice(0, 3); + const labels = options.map((o) => o.getAttribute('title') ?? ''); + const previews = previewTexts('task').slice(0, 3); + await act(async () => { + fireEvent.click(options[0]); + }); + expect(onSelectRecord).toHaveBeenCalledTimes(1); + const picked = onSelectRecord.mock.calls[0][0] as Record; + cleanup(); + localStorage.clear(); + return { labels, previews, picked }; + } + + it('dropdown: same labels, same subtitles, same record handed to onSelectRecord', async () => { + // The expanding backend first: it resolves nothing per row, so it leaves + // the module-level name cache empty for the bare-id run that follows. + const expanded = await readDropdown(true); + const bare = await readDropdown(false); + + expect(expanded).toEqual(bare); + expect(expanded.labels[0]).toBe('C-0 - same_task_0'); + expect(expanded.previews[0]).toBe('Assembly step 0'); + expect(expanded.picked.task).toBe('same_task_0'); + }); + + interface PickerReading { + titles: string[]; + tasks: string[]; + picked: Record[]; + } + + async function readPicker(honoursExpand: boolean): Promise { + const backend = makeBackend({ + prefix: 'samepick', + highlightFields: ['code', 'task', 'version'], + honoursExpand, + }); + const onSelectRecords = vi.fn(); + render( + + {}} + dataSource={backend.dataSource} + objectName="task_version" + displayField="name" + titleFormat={TITLE_FORMAT} + columns={['name', 'code', 'task', 'version']} + onSelect={() => {}} + onSelectRecords={onSelectRecords} + cellRenderer={getCellRenderer} + fieldsMeta={TASK_VERSION_FIELDS} + /> + , + ); + await waitFor(() => expect(screen.getByTestId('record-row-samepick_tv_0')).toBeInTheDocument()); + await settle(); + const read = (field: string) => + Array.from(document.querySelectorAll(`[data-lookup-cell="${field}"]`)) + .slice(0, 3) + .map((el) => el.textContent ?? ''); + const titles = read('name'); + const tasks = read('task'); + await act(async () => { + fireEvent.click(screen.getByTestId('record-row-samepick_tv_0')); + }); + expect(onSelectRecords).toHaveBeenCalledTimes(1); + const picked = onSelectRecords.mock.calls[0][0] as Record[]; + cleanup(); + return { titles, tasks, picked }; + } + + it('picker: same title cells, same task cells, same records handed to onSelectRecords', async () => { + const expanded = await readPicker(true); + const bare = await readPicker(false); + + expect(expanded).toEqual(bare); + expect(expanded.titles[0]).toBe('C-0 - samepick_task_0'); + expect(expanded.tasks[0]).toBe('Assembly step 0'); + expect(expanded.picked).toEqual([ + { + id: 'samepick_tv_0', + name: 'TV-0', + code: 'C-0', + task: 'samepick_task_0', + version: 1, + owner: 'samepick_user_0', + }, + ]); + }); +}); + +describe('LookupField — field-level security gates the expansion (objectui#10223)', () => { + /** + * The real provider, not a stub: `checkField` answers from a policy that + * denies `task_version.task` to the `viewer` role and says nothing about + * `owner`. Both relations are previewed, so the pin reads a FILTER — one + * name removed, the other kept — rather than an expansion that vanished. + */ + function withPolicy(taskReadable: boolean): Wrap { + return (node) => ( + + {node} + + ); + } + + const HIGHLIGHTS = ['code', 'task', 'owner']; + + it('dropdown: a relation the policy denies is not expanded; the readable one still is', async () => { + const backend = makeBackend({ prefix: 'flsdeny', highlightFields: HIGHLIGHTS }); + await openDropdown(backend, {}, withPolicy(false)); + const queries = candidateQueries(backend); + expect(queries).toHaveLength(1); + expect(queries[0].$expand).toEqual(['owner']); + }); + + it('dropdown: a readable relation is expanded as before', async () => { + const backend = makeBackend({ prefix: 'flsallow', highlightFields: HIGHLIGHTS }); + await openDropdown(backend, {}, withPolicy(true)); + expect(candidateQueries(backend)[0].$expand).toEqual(['task', 'owner']); + }); + + it('dropdown: no policy loaded (no provider) filters nothing', async () => { + const backend = makeBackend({ prefix: 'flsnone', highlightFields: HIGHLIGHTS }); + await openDropdown(backend); + expect(candidateQueries(backend)[0].$expand).toEqual(['task', 'owner']); + }); + + it('recents rail: the same gate applies to its query', async () => { + const backend = makeBackend({ prefix: 'flsrecent', highlightFields: HIGHLIGHTS }); + pushRecentLookupId('task_version', 'flsrecent_tv_1'); + pushRecentLookupId('task_version', 'flsrecent_tv_0'); + await openDropdown(backend, {}, withPolicy(false)); + const recents = candidateQueries(backend).filter((p) => JSON.stringify(p.$filter ?? {}).includes('$in')); + expect(recents).toHaveLength(1); + expect(recents[0].$expand).toEqual(['owner']); + }); + + it('picker: a relation the policy denies is not expanded; the readable one still is', async () => { + const backend = makeBackend({ prefix: 'flspick', highlightFields: HIGHLIGHTS }); + render( + withPolicy(false)( + + {}} + dataSource={backend.dataSource} + objectName="task_version" + displayField="name" + columns={['name', 'code', 'task', 'owner']} + onSelect={() => {}} + cellRenderer={getCellRenderer} + fieldsMeta={TASK_VERSION_FIELDS} + /> + , + ), + ); + await waitFor(() => expect(screen.getByTestId('record-row-flspick_tv_0')).toBeInTheDocument()); + const queries = candidateQueries(backend); + expect(queries).toHaveLength(1); + expect(queries[0].$expand).toEqual(['owner']); + }); +}); diff --git a/packages/fields/src/widgets/LookupField.pickerAgreement.test.tsx b/packages/fields/src/widgets/LookupField.pickerAgreement.test.tsx index 47dd56dcff..8a4bbbdbe3 100644 --- a/packages/fields/src/widgets/LookupField.pickerAgreement.test.tsx +++ b/packages/fields/src/widgets/LookupField.pickerAgreement.test.tsx @@ -31,15 +31,17 @@ * declaration is driven through both, and the rendered values must match * column for column. * - * They also pin the fallback. Neither surface's query carries populate/expand - * — that is unchanged, and widening `lookupColumns` with dot-path/populate - * semantics is explicitly NOT what fixes this — so a lookup value can - * legitimately arrive as an unresolved foreign-key id. The rule is that the - * dropdown adopts whatever the picker shows for it, and that the column is - * never silently dropped: a held value keeps its slot and renders the lookup - * cell renderer's own placeholder, because a blank column is worse than a - * bare id (the field report records a dot-path attempt producing exactly that - * silent-empty outcome). + * They also pin the fallback. Both surfaces' queries now ask for `$expand` on + * the reference columns they show (objectui#10223), but this file's backend + * ignores that parameter and returns the bare foreign key, as any backend that + * does not honour it would — and widening `lookupColumns` with + * dot-path/populate semantics is explicitly NOT what fixes this — so a lookup + * value can legitimately arrive as an unresolved foreign-key id. The rule is + * that the dropdown adopts whatever the picker shows for it, and that the + * column is never silently dropped: a held value keeps its slot and renders + * the lookup cell renderer's own placeholder, because a blank column is worse + * than a bare id (the field report records a dot-path attempt producing + * exactly that silent-empty outcome). */ import * as React from 'react'; @@ -234,9 +236,9 @@ describe('LookupField — inline dropdown agrees with the browse-all picker (obj const dropdown = await readInlineDropdown(UNRESOLVED_STEP_ID); const picker = await readBrowseAllPicker(UNRESOLVED_STEP_ID); - // The dropdown's query carries no populate/expand, so an id that resolves - // to nothing is a legitimate state. Whatever it renders, it renders the - // picker's answer — the two surfaces do not get to disagree here either. + // This backend ignores `$expand` and serves the bare key, so an id that + // resolves to nothing is a legitimate state. Whatever it renders, it renders + // the picker's answer — the two surfaces do not get to disagree here either. expect(dropdown).toEqual(picker); // The column must still be THERE, and it must not have degraded into the diff --git a/packages/fields/src/widgets/LookupField.tsx b/packages/fields/src/widgets/LookupField.tsx index f00c2301d3..17a4686616 100644 --- a/packages/fields/src/widgets/LookupField.tsx +++ b/packages/fields/src/widgets/LookupField.tsx @@ -18,7 +18,7 @@ import type { RecordPickerFilterColumn } from './RecordPickerDialog.js'; import { PeoplePicker } from './PeoplePicker.js'; import { useRecordQuery } from './useRecordQuery.js'; import { deriveLookupColumns } from './deriveLookupColumns.js'; -import { getRecordDisplayName, mergeFilterNodes } from '@object-ui/core'; +import { buildExpandFields, getRecordDisplayName, mergeFilterNodes, toPredicateRecord } from '@object-ui/core'; import { getRecentLookupIds, pushRecentLookupId } from './recentLookups.js'; import { getPersonInitials } from './personDisplay.js'; import { getCellRendererResolver } from './_cell-renderer-bridge.js'; @@ -33,6 +33,7 @@ import { } from './lookupColumnDisplay.js'; import { useSafeFieldLabel, useDisplayLocale } from '@object-ui/i18n'; import { SchemaRendererContext as ImportedSchemaRendererContext, useAction, useHasActionProvider } from '@object-ui/react'; +import { usePermissions } from '@object-ui/permissions'; import { useFieldTranslation } from './useFieldTranslation.js'; export interface LookupOption { @@ -496,6 +497,48 @@ export function LookupField({ value, onChange, field, readonly, error: fieldErro [previewColumns, refObjectSchema, referenceTo, translateOptions], ); + /** + * `$expand` for the candidate queries (objectui#10223): the reference columns + * among the ones this dropdown PREVIEWS, by `buildExpandFields`' rule — the + * same one the list views use for their visible columns. + * + * Without it every previewed `lookup` / `master_detail` column arrived as a + * bare foreign key, and the lookup cell renderer resolved each one with its + * own `findOne` — one request per candidate per such column, on every open. + * An expanded value is rendered by that same cell renderer with no fetch, so + * the preview reads the same and the per-row requests go. + * + * Expansion is a DISPLAY concern here and stays one: options are built from + * the row with its relations collapsed back to ids (`toPredicateRecord`), so + * the label, the committed value and the record `onSelectRecord` hands a host + * are what they were before any column was expanded. Only the preview reads + * the expanded row (`previewRows` below). A backend that ignores `$expand` + * returns bare ids, and the cell renderer's per-id resolution takes over as + * before. + * + * Field-level security gates the OUTPUT, in the shape the objectui#7429 sweep + * applied at every other `buildExpandFields` call site: once the policy has + * loaded, a relation the user may not read on the referenced object is not + * asked for; before it loads, nothing is filtered and `perms` in the deps + * rebuilds the list when the answer arrives. Every name judged here is one + * the referenced object declares, so the "`checkField` answers false for an + * undeclared key" trap cannot be reached. + * + * No previewed column ⇒ no `$expand`. `buildExpandFields` reads an EMPTY + * column list as "no column restriction" and returns every relation the + * object declares; a dropdown that previews only its display field (a + * `highlightFields` naming just that field, or every other field + * system-managed) would then ask for `created_by`, `owner_id`, … — none of + * which it renders. + */ + const perms = usePermissions(); + const candidateExpand = useMemo(() => { + if (previewColumns.length === 0) return []; + const expandable = buildExpandFields(refObjectSchema?.fields, previewColumns); + if (!perms.isLoaded || !referenceTo) return expandable; + return expandable.filter((f) => perms.checkField(referenceTo, f, 'read')); + }, [refObjectSchema, previewColumns, perms, referenceTo]); + // Derive filter-bar columns from any typed picker columns. const filterColumns = useMemo(() => { if (!pickerColumns) return undefined; @@ -548,6 +591,7 @@ export function LookupField({ value, onChange, field, readonly, error: fieldErro enabled: isOpen && hasDataSource && !dependenciesMissing, pageSize: LOOKUP_PAGE_SIZE, filter: popoverFilter, + expand: candidateExpand, }); // Re-source the popover's fetch state from the kernel; all existing read sites @@ -556,10 +600,15 @@ export function LookupField({ value, onChange, field, readonly, error: fieldErro const loading = popoverQuery.loading; const totalCount = popoverQuery.total; const error = popoverQuery.error ?? createError; + // Built from the row with its relations collapsed to ids — see + // `candidateExpand` for why the option never sees the expanded form. const fetchedOptions = useMemo( () => popoverQuery.records.map(r => - recordToOption(r, displayField, idField, effectiveDescriptionField, refTitleFormat, refObjectSchema), + recordToOption( + toPredicateRecord(r, refObjectSchema?.fields), + displayField, idField, effectiveDescriptionField, refTitleFormat, refObjectSchema, + ), ), [popoverQuery.records, displayField, idField, effectiveDescriptionField, refTitleFormat, refObjectSchema], ); @@ -894,12 +943,20 @@ export function LookupField({ value, onChange, field, readonly, error: fieldErro // filtered response; ids it does not return are dropped, whether they fail // the filters or no longer exist. It is also one request instead of up to // MAX_RECENT serial round-trips. - const [recentOptions, setRecentOptions] = useState([]); + // + // The rail previews the same columns as the main list, so it asks for the + // same `$expand` (objectui#10223). It keeps the rows it was served — the + // preview reads them — and derives its options from them exactly as the main + // list does, relations collapsed to ids. + const [recentRows, setRecentRows] = useState[]>([]); + // The expansion as a primitive, for the effect below: a memoised array's + // identity is not a dependency to key a fetch on (AGENTS.md #10). + const candidateExpandKey = candidateExpand.join(','); useEffect(() => { if (!isOpen || !hasDataSource || !dataSource || !referenceTo || searchQuery) return; - if (dependenciesMissing) { setRecentOptions([]); return; } + if (dependenciesMissing) { setRecentRows([]); return; } const ids = getRecentLookupIds(referenceTo); - if (!ids.length) { setRecentOptions([]); return; } + if (!ids.length) { setRecentRows([]); return; } let cancelled = false; (async () => { try { @@ -909,29 +966,53 @@ export function LookupField({ value, onChange, field, readonly, error: fieldErro ? mergeFilterNodes(popoverFilter, idRestriction) : idRestriction, $top: ids.length, + ...(candidateExpand.length > 0 ? { $expand: candidateExpand } : {}), } as QueryParams); const rows = (res as any)?.data ?? res ?? []; const byId = new Map(); if (Array.isArray(rows)) { for (const row of rows) { - const rid = row?.[idField] ?? row?.id ?? row?._id; + const plain = toPredicateRecord(row, refObjectSchema?.fields); + const rid = plain?.[idField] ?? plain?.id ?? plain?._id; if (rid !== undefined && rid !== null) byId.set(String(rid), row); } } // Most-recent-first order is preserved; the response only decides // WHICH ids survive, never their order. - const recs = ids - .map((id) => byId.get(String(id))) - .filter(Boolean) - .map((r) => - recordToOption(r, displayField, idField, effectiveDescriptionField, refTitleFormat, refObjectSchema), - ); - if (!cancelled) setRecentOptions(recs); - } catch { if (!cancelled) setRecentOptions([]); } + const kept = ids.map((id) => byId.get(String(id))).filter(Boolean); + if (!cancelled) setRecentRows(kept); + } catch { if (!cancelled) setRecentRows([]); } })(); return () => { cancelled = true; }; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [isOpen, hasDataSource, referenceTo, searchQuery, dependenciesMissing, popoverFilter, idField]); + }, [isOpen, hasDataSource, referenceTo, searchQuery, dependenciesMissing, popoverFilter, idField, candidateExpandKey]); + const recentOptions = useMemo( + () => + recentRows.map((r) => + recordToOption( + toPredicateRecord(r, refObjectSchema?.fields), + displayField, idField, effectiveDescriptionField, refTitleFormat, refObjectSchema, + ), + ), + [recentRows, displayField, idField, effectiveDescriptionField, refTitleFormat, refObjectSchema], + ); + + /** + * The rows previews render from (objectui#10223): each as the server returned + * it, with any `$expand`-ed relation still expanded, so the lookup cell + * renderer names it without a fetch. Keyed by the option's value (a + * primitive, never an option object's identity). An option with no served + * row — a static option, a just-created record — previews from itself. + */ + const previewRows = useMemo(() => { + const byValue = new Map>(); + for (const raw of [...recentRows, ...popoverQuery.records]) { + const plain = toPredicateRecord(raw, refObjectSchema?.fields); + const v = plain?.[idField] ?? plain?.id ?? plain?._id ?? plain?.externalId; + if (v !== undefined && v !== null && !byValue.has(String(v))) byValue.set(String(v), raw); + } + return byValue; + }, [recentRows, popoverQuery.records, refObjectSchema, idField]); // Recently-used first (only before the user types), then live results — one // de-duped list that drives BOTH rendering and arrow-key navigation. @@ -1028,8 +1109,12 @@ export function LookupField({ value, onChange, field, readonly, error: fieldErro */ const previewOf = useCallback( (option: LookupOption): React.ReactNode => { + // The option with its relations as the server served them — every other + // key reads exactly as it did before `$expand` (objectui#10223). + const served = previewRows.get(String(option.value)); + const row = served ? { ...option, ...served } : option; const cols = previewColumns.filter((c) => { - const v = (option as any)[c.field]; + const v = (row as any)[c.field]; return v !== null && v !== undefined && v !== ''; }); if (cols.length === 0) return null; @@ -1039,7 +1124,7 @@ export function LookupField({ value, onChange, field, readonly, error: fieldErro {`${col.label || fieldToLabel(col.field)}:`} - {renderLookupColumnValue(option, col, { + {renderLookupColumnValue(row, col, { descriptors: previewDescriptors, cellRenderer: getCellRendererResolver(), displayLocale, @@ -1049,7 +1134,7 @@ export function LookupField({ value, onChange, field, readonly, error: fieldErro )); }, - [previewColumns, previewDescriptors, displayLocale], + [previewRows, previewColumns, previewDescriptors, displayLocale], ); // Keyboard handler for the search input — arrow keys + Enter diff --git a/packages/fields/src/widgets/RecordPickerDialog.tsx b/packages/fields/src/widgets/RecordPickerDialog.tsx index 123a3efa22..132060ef9a 100644 --- a/packages/fields/src/widgets/RecordPickerDialog.tsx +++ b/packages/fields/src/widgets/RecordPickerDialog.tsx @@ -40,8 +40,9 @@ import type { DataSource, LookupColumnDef, LookupFilterDef } from '@object-ui/ty // The repo's single filter sink (`packages/core/src/utils/filter-converter.ts`) // — shared with plugin-list's `buildEffectiveFilter` and plugin-view's // ObjectView, so a spec `ViewFilterRule[]` lowers in exactly one place. -import { mergeFilterNodes } from '@object-ui/core'; +import { buildExpandFields, mergeFilterNodes, toPredicateRecord } from '@object-ui/core'; import { useSafeFieldLabel, useDisplayLocale } from '@object-ui/i18n'; +import { usePermissions } from '@object-ui/permissions'; import { useFieldTranslation } from './useFieldTranslation.js'; import { useRecordQuery } from './useRecordQuery.js'; // The one place a lookup column's display value is decided — shared with the @@ -499,6 +500,37 @@ export function RecordPickerDialog({ [resolvedColumns, fieldsMeta, objectName, translateOptions], ); + /** + * `$expand` for the picker's query (objectui#10223): the reference columns + * among the ones this table renders, by `buildExpandFields`' rule — the one + * LookupField's inline dropdown applies to its previewed columns. Without it + * each such cell arrived as a bare foreign key and the lookup cell renderer + * resolved it with its own `findOne`, one request per row per column. + * + * The id column is left out: the row's identity (`getRecordId`) reads it + * raw, so it must stay the key it always was. + * + * The table renders the rows as served. What leaves it — the records + * `onSelectRecords` hands a host, and the `titleFormat` template's reading + * of a row — sees the row with its relations collapsed to ids + * (`toPredicateRecord`), as it did before any column was expanded. + * + * Field-level security gates the OUTPUT, in the objectui#7429 sweep's shape + * — the same gate as LookupField's `candidateExpand`: once the policy has + * loaded, a relation the user may not read on `objectName` is not asked for. + * + * No rendered column besides the id ⇒ no `$expand`: `buildExpandFields` + * reads an EMPTY column list as "every relation the object declares". + */ + const perms = usePermissions(); + const expand = useMemo(() => { + const rendered = resolvedColumns.filter((c) => c.field !== idField); + if (rendered.length === 0) return []; + const expandable = buildExpandFields(fieldsMeta, rendered); + if (!perms.isLoaded) return expandable; + return expandable.filter((f) => perms.checkField(objectName, f, 'read')); + }, [fieldsMeta, resolvedColumns, idField, perms, objectName]); + // Auto-generate filter columns from lookupFilters when no explicit filterColumns given. // Each LookupFilterDef becomes a filterable field with inferred type. const baseFilterColumns = useMemo(() => { @@ -600,6 +632,7 @@ export function RecordPickerDialog({ pageSize, paginate: true, filter: mergedFilter, + expand, }); // Preserve the previous local names so the handlers and render below are // unchanged (the migration is a pure refactor). @@ -670,6 +703,8 @@ export function RecordPickerDialog({ const handleRowClick = useCallback( (record: any) => { const rid = getRecordId(record); + // A host receives the row as it was before `$expand` (objectui#10223). + const selected = toPredicateRecord(record, fieldsMeta); if (multiple) { setPendingSelection(prev => { @@ -679,18 +714,18 @@ export function RecordPickerDialog({ selectedRecordsMap.current.delete(rid); } else { next.add(rid); - selectedRecordsMap.current.set(rid, record); + selectedRecordsMap.current.set(rid, selected); } return next; }); } else { // Single select — immediately close onSelect(rid); - onSelectRecords?.([record]); + onSelectRecords?.([selected]); onOpenChange(false); } }, - [multiple, getRecordId, onSelect, onSelectRecords, onOpenChange], + [multiple, getRecordId, fieldsMeta, onSelect, onSelectRecords, onOpenChange], ); // Confirm multi-select @@ -762,16 +797,24 @@ export function RecordPickerDialog({ // renderer (objectui#5492) — the inline dropdown in LookupField calls the // very same function, so this table and that popover cannot answer one // `lookup_columns` declaration two different ways. + // + // The display column's `titleFormat` template reads the row with its + // relations collapsed to ids, so an expanded reference it names prints what + // it printed before `$expand` rather than an object (objectui#10223). const renderCellContent = useCallback( (record: any, col: LookupColumnDef): React.ReactNode => - renderLookupColumnValue(record, col, { - descriptors: columnFieldDescriptors, - cellRenderer, - titleFormat, - displayField, - displayLocale, - }), - [cellRenderer, titleFormat, displayField, columnFieldDescriptors, displayLocale], + renderLookupColumnValue( + titleFormat && col.field === displayField ? toPredicateRecord(record, fieldsMeta) : record, + col, + { + descriptors: columnFieldDescriptors, + cellRenderer, + titleFormat, + displayField, + displayLocale, + }, + ), + [cellRenderer, titleFormat, displayField, fieldsMeta, columnFieldDescriptors, displayLocale], ); // Render sort indicator for a column diff --git a/packages/fields/src/widgets/lookupColumnDisplay.tsx b/packages/fields/src/widgets/lookupColumnDisplay.tsx index 390d3877dd..d05ac7e7cd 100644 --- a/packages/fields/src/widgets/lookupColumnDisplay.tsx +++ b/packages/fields/src/widgets/lookupColumnDisplay.tsx @@ -29,7 +29,10 @@ * paths, no populate/expand semantics. A lookup column whose value arrives as * an unresolved foreign-key id is resolved the same way the picker has always * resolved it — client-side, by the lookup cell renderer — so the two surfaces - * agree without either query changing. + * agree whether or not the related record came back expanded: both queries ask + * for `$expand` on the readable reference columns they display + * (objectui#10223), and an expanded value renders through that same cell + * renderer without a fetch. */ import React from 'react'; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 4abe68a87f..5044319a45 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1273,6 +1273,9 @@ importers: '@object-ui/i18n': specifier: workspace:* version: link:../i18n + '@object-ui/permissions': + specifier: workspace:* + version: link:../permissions '@object-ui/providers': specifier: workspace:* version: link:../providers