diff --git a/.changeset/export-matches-the-searched-view.md b/.changeset/export-matches-the-searched-view.md new file mode 100644 index 000000000..e88674e64 --- /dev/null +++ b/.changeset/export-matches-the-searched-view.md @@ -0,0 +1,43 @@ +--- +"@object-ui/types": patch +"@object-ui/data-objectstack": patch +"@object-ui/plugin-list": patch +--- + +fix(list,data-objectstack,types): exporting a searched list no longer downloads the unsearched superset + +The server-streamed export mirrored the view's `filter` and `sort`, and the +code comment claimed that made the file match the screen: + +> Mirrors the active view's filter + sort so the exported file matches what the +> user sees. + +It mirrored one half. There was no way to carry the term a user had typed into +the search box — `ExportDownloadRequest` had no field for one — so exporting +during a search produced **more rows than the list showed**, in a file that +looks authoritative, with nothing indicating the difference. The client-side +fallback was always correct (it serializes the already-searched `data`); only +the server path was wrong, and it is the one that handles xlsx. + +Same family as a dropped filter (objectstack#3948, objectstack#4181): a +plausible answer that is quietly broader than the one asked for. + +- `ExportDownloadRequest` gains `search` / `searchFields`. +- `ObjectStackAdapter.exportDownload` sends them as `search=` / `searchFields=`, + trimming the term and omitting both when it is blank (`searchFields` alone + means nothing). +- `ListView` passes the active `searchTerm` and the view's `searchableFields`, + and both are now in the export callback's dependency array — a stale closure + would export the wrong row set. + +Requires a server with objectstack#4230. Older servers ignore unknown query +params on this route, so they keep today's behaviour rather than erroring. + +**Also: the filter merge is no longer written twice.** The three filter sources +(view filter, filter-panel group, per-field user filters) were merged by +verbatim copies in the data fetch and in the export — two copies that must +agree, deciding respectively what the user *sees* and what they *download*. +Both now call `buildEffectiveFilter`. This is a pure extraction: the copies did +agree, and the four parity tests added for it pass against the old code too. +They exist to keep it that way — the adapter's duplicated filter-shape check +had already drifted apart unnoticed (#3072). diff --git a/packages/data-objectstack/src/exportDownload.test.ts b/packages/data-objectstack/src/exportDownload.test.ts index eb700003b..429d9724a 100644 --- a/packages/data-objectstack/src/exportDownload.test.ts +++ b/packages/data-objectstack/src/exportDownload.test.ts @@ -97,3 +97,45 @@ describe('ObjectStackAdapter.exportDownload', () => { }); }); }); + +/** + * `search` — the half of a list this request could not carry. + * + * The route mirrored `filter` and `orderby` only, so an export taken while a + * search was active downloaded the UNSEARCHED superset: more rows than the + * screen showed, in a file that looks authoritative. Server half: + * objectstack#4230. + */ +describe('ObjectStackAdapter.exportDownload — search', () => { + const paramsFor = async (request: Record) => { + const fetchImpl = vi.fn(async (_url: string, _init: RequestInit) => csvResponse()); + await makeDS(fetchImpl).exportDownload('task', request); + return new URL(fetchImpl.mock.calls[0][0]).searchParams; + }; + + it('sends the term as `search`', async () => { + expect((await paramsFor({ search: 'acme' })).get('search')).toBe('acme'); + }); + + it('sends `searchFields` as a comma list alongside the term', async () => { + const p = await paramsFor({ search: 'acme', searchFields: ['name', 'stage'] }); + expect(p.get('searchFields')).toBe('name,stage'); + }); + + it('omits both when no term is given — `searchFields` alone means nothing', async () => { + const p = await paramsFor({ searchFields: ['name'] }); + expect(p.get('search')).toBeNull(); + expect(p.get('searchFields')).toBeNull(); + }); + + it('treats a whitespace-only term as absent, and trims a real one', async () => { + expect((await paramsFor({ search: ' ' })).get('search')).toBeNull(); + expect((await paramsFor({ search: ' acme ' })).get('search')).toBe('acme'); + }); + + it('carries search and filter together — neither replaces the other', async () => { + const p = await paramsFor({ search: 'acme', filter: [['status', '=', 'open']] }); + expect(p.get('search')).toBe('acme'); + expect(JSON.parse(p.get('filter')!)).toEqual([['status', '=', 'open']]); + }); +}); diff --git a/packages/data-objectstack/src/index.ts b/packages/data-objectstack/src/index.ts index 4a60cce93..67e242632 100644 --- a/packages/data-objectstack/src/index.ts +++ b/packages/data-objectstack/src/index.ts @@ -2130,6 +2130,16 @@ export class ObjectStackAdapter implements DataSource { queryParams.set('filter', JSON.stringify(translated)); } } + // Search — the other half of what a list is showing. Without it an export + // taken while a search is active returns the unsearched superset + // (objectstack#4230). Servers predating that ignore the param. + const searchTerm = typeof request.search === 'string' ? request.search.trim() : ''; + if (searchTerm) { + queryParams.set('search', searchTerm); + if (request.searchFields && request.searchFields.length > 0) { + queryParams.set('searchFields', request.searchFields.join(',')); + } + } const baseUrl = this.baseUrl.replace(/\/$/, ''); // Avoid doubling /api/v1 if baseUrl already includes the version suffix. diff --git a/packages/plugin-list/src/ListView.tsx b/packages/plugin-list/src/ListView.tsx index c8a34b06d..d911e6480 100644 --- a/packages/plugin-list/src/ListView.tsx +++ b/packages/plugin-list/src/ListView.tsx @@ -218,6 +218,42 @@ export function normalizeFilters(filters: any[]): any[] { .filter(f => Array.isArray(f) && f.length > 0); } +/** + * Merge the three filter sources a list can have — the view's own `filter`, the + * filter-panel group, and the per-field user filters — into one AST node. + * + * ONE definition on purpose. The data fetch and the export used to carry + * verbatim copies of this, and those two decide, respectively, **what the user + * sees** and **what they download**. Two copies that must agree, where a + * divergence is a file that silently disagrees with the screen — the same shape + * of bug that had already bitten the adapter, whose duplicated filter-shape + * check drifted apart unnoticed (#3072). + * + * Returns `undefined` when nothing is active, so callers can skip `$filter` + * entirely rather than sending an empty array. + * + * Known gap, preserved deliberately: a MongoDB-style object `schema.filter` has + * no `.length` and is dropped here, as it always has been. Nothing in this repo + * produces one for a list view (they come from dashboard widgets, which query + * directly), so this stays a pure extraction rather than a behaviour change. + */ +export function buildEffectiveFilter( + baseFilter: unknown, + currentFilters: FilterGroup, + userFilterConditions: any[], +): any[] | undefined { + const base = Array.isArray(baseFilter) ? baseFilter : []; + const userFilter = convertFilterGroupToAST(currentFilters); + const allFilters = [ + ...(base.length > 0 ? [base] : []), + ...(userFilter.length > 0 ? [userFilter] : []), + // Normalize the per-field conditions (expands `in` into an `or` of `=`). + ...normalizeFilters(userFilterConditions), + ].filter((f: any) => Array.isArray(f) && f.length > 0); + if (allFilters.length === 0) return undefined; + return allFilters.length === 1 ? allFilters[0] : ['and', ...allFilters]; +} + export function convertFilterGroupToAST(group: FilterGroup): any[] { if (!group || !group.conditions || group.conditions.length === 0) return []; @@ -1044,28 +1080,10 @@ export const ListView = React.forwardRef(({ setLoading(true); setLoadError(null); try { - // Construct filter - let finalFilter: any = []; - const baseFilter = schema.filter || []; - const userFilter = convertFilterGroupToAST(currentFilters); - - - // Normalize userFilter conditions (convert `in` to `or` of `=`) - const normalizedUserFilterConditions = normalizeFilters(userFilterConditions); - - // Merge all filter sources with consistent structure - const allFilters = [ - ...(baseFilter.length > 0 ? [baseFilter] : []), - ...(userFilter.length > 0 ? [userFilter] : []), - ...normalizedUserFilterConditions, - ].filter(f => Array.isArray(f) && f.length > 0); - - if (allFilters.length > 1) { - finalFilter = ['and', ...allFilters]; - } else if (allFilters.length === 1) { - finalFilter = allFilters[0]; - } - + // Construct filter — shared with the export path so the file a user + // downloads is built from the same three sources as the rows on screen. + const finalFilter = buildEffectiveFilter(schema.filter, currentFilters, userFilterConditions); + // Convert sort to query format // Use array format to ensure order is preserved (Object keys are not guaranteed ordered) const sort: any = currentSort.length > 0 @@ -1183,12 +1201,11 @@ export const ListView = React.forwardRef(({ return Array.from(required); })(); - // Only send $filter when it is a non-empty AST. Sending an empty - // array results in `?filter=%5B%5D` which is wasted bandwidth and - // can defeat server-side query parsing/caching. - const hasFilter = Array.isArray(finalFilter) - ? finalFilter.length > 0 - : !!finalFilter && Object.keys(finalFilter).length > 0; + // Only send $filter when there is one. Sending an empty array results in + // `?filter=%5B%5D` which is wasted bandwidth and can defeat server-side + // query parsing/caching. `buildEffectiveFilter` returns a non-empty AST + // or `undefined`, so this is the whole test. + const hasFilter = finalFilter !== undefined; // Window the request only for the flat grid view. Grouped grids and the // visual views (kanban/calendar/gantt/gallery) consume the whole batch, @@ -1750,7 +1767,12 @@ export const ListView = React.forwardRef(({ // Server-streamed path: csv / xlsx / json via dataSource.exportDownload. // XLSX is server-only; type-aware value formatting, field resolution and // permission enforcement all happen server-side. Mirrors the active view's - // filter + sort so the exported file matches what the user sees. + // filter + SEARCH + sort so the exported file matches what the user sees. + // + // The search half was missing, and this comment asserted the match anyway: + // exporting during a search downloaded the unsearched superset. The client + // fallback below was always right (it serializes `data`, already searched); + // only this path was wrong, and it is the one that handles xlsx. const serverEligible = (format === 'csv' || format === 'xlsx' || format === 'json') && typeof dataSource?.exportDownload === 'function' && !!schema.objectName @@ -1760,18 +1782,8 @@ export const ListView = React.forwardRef(({ .map((f: any) => typeof f === 'string' ? f : (f.name || f.fieldName || f.field)) .filter(Boolean); - // Merge the same filter sources as the data fetch (base + user + conditions). - const baseFilter = schema.filter || []; - const userFilter = convertFilterGroupToAST(currentFilters); - const normalizedUserFilterConditions = normalizeFilters(userFilterConditions); - const allFilters = [ - ...(baseFilter.length > 0 ? [baseFilter] : []), - ...(userFilter.length > 0 ? [userFilter] : []), - ...normalizedUserFilterConditions, - ].filter((f: any) => Array.isArray(f) && f.length > 0); - const finalFilter = allFilters.length > 1 - ? ['and', ...allFilters] - : allFilters.length === 1 ? allFilters[0] : undefined; + // The same three filter sources as the data fetch, from the same function. + const finalFilter = buildEffectiveFilter(schema.filter, currentFilters, userFilterConditions); const sort = currentSort.length > 0 ? currentSort @@ -1787,6 +1799,16 @@ export const ListView = React.forwardRef(({ format: format as 'csv' | 'xlsx' | 'json', fields: fields.length ? fields : undefined, filter: finalFilter, + // The other half of what the list is showing. Without it an export + // taken during a search returned the UNSEARCHED superset — more rows + // than the screen, in a file that looks authoritative. Needs a server + // with objectstack#4230; older ones ignore it and behave as before. + ...(searchTerm ? { + search: searchTerm, + ...(schema.searchableFields && schema.searchableFields.length > 0 + ? { searchFields: schema.searchableFields } + : {}), + } : {}), sort, includeHeaders, limit: maxRecords > 0 ? maxRecords : undefined, @@ -1861,7 +1883,9 @@ export const ListView = React.forwardRef(({ URL.revokeObjectURL(url); } setShowExport(false); - }, [data, effectiveFields, resolvedExportOptions, schema.objectName, schema.filter, exportPermitted, dataSource, currentFilters, userFilterConditions, currentSort, objectDef, resolveObjectLabel]); + // `searchTerm` / `searchableFields` belong here: the export now narrows by + // the active search, so a stale closure would export the wrong row set. + }, [data, effectiveFields, resolvedExportOptions, schema.objectName, schema.filter, schema.searchableFields, exportPermitted, dataSource, currentFilters, userFilterConditions, currentSort, searchTerm, objectDef, resolveObjectLabel]); // All available fields for hide/show (with i18n) const allFields = React.useMemo(() => { diff --git a/packages/plugin-list/src/__tests__/ListView.exportMatchesView.test.tsx b/packages/plugin-list/src/__tests__/ListView.exportMatchesView.test.tsx new file mode 100644 index 000000000..a77d88571 --- /dev/null +++ b/packages/plugin-list/src/__tests__/ListView.exportMatchesView.test.tsx @@ -0,0 +1,141 @@ +/** + * 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. + */ + +/** + * "Export what the user is looking at." + * + * The server-streamed export mirrored the view's FILTER and sort, and the code + * comment claimed that made the file match the screen. It did not: there was no + * way to carry the search term, and `ExportDownloadRequest` had no field for + * one. Exporting during a search downloaded the UNSEARCHED superset — more rows + * than the list showed, in a file that looks authoritative, with nothing saying + * so. (The client-side fallback was always right; it serializes the already + * searched `data`. Only the server path was wrong — and it is the one that + * handles xlsx.) Server half: objectstack#4230. + * + * The filter half was built twice — once for the fetch, once for the export — + * as verbatim copies that had to stay in sync. Those two decide what the user + * SEES and what they DOWNLOAD, so the parity test below compares the real + * arguments of the two real calls rather than asserting a shape twice. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, fireEvent, screen } from '@testing-library/react'; +import { ListView } from '../ListView'; +import { SchemaRendererProvider } from '@object-ui/react'; +import type { ListViewSchema } from '@object-ui/types'; + +const RECORDS = [{ id: '1', name: 'Acme', stage: 'won' }]; + +function harness(schema: ListViewSchema, props: Record = {}) { + const exportDownload = vi.fn().mockResolvedValue(new Blob(['x'], { type: 'text/csv' })); + const find = vi.fn().mockResolvedValue({ data: RECORDS, total: 1 }); + const ds: any = { find, findOne: vi.fn(), create: vi.fn(), update: vi.fn(), delete: vi.fn(), exportDownload }; + render( + + + , + ); + return { exportDownload, find }; +} + +async function clickExportCsv() { + fireEvent.click(screen.getByRole('button', { name: /export/i })); + fireEvent.click(await screen.findByRole('button', { name: /export as csv/i })); +} + +const BASE: ListViewSchema = { + type: 'list-view', + objectName: 'account', + viewType: 'grid', + fields: ['name'], + exportOptions: { formats: ['csv'] }, +}; + +describe('ListView export mirrors the active view', () => { + beforeEach(() => { + vi.spyOn(URL, 'createObjectURL').mockReturnValue('blob:x'); + vi.spyOn(URL, 'revokeObjectURL').mockImplementation(() => {}); + }); + afterEach(() => vi.restoreAllMocks()); + + it('carries the active search term', async () => { + const { exportDownload } = harness(BASE, { initialSearchTerm: 'acme' }); + await clickExportCsv(); + await vi.waitFor(() => expect(exportDownload).toHaveBeenCalledTimes(1)); + expect(exportDownload.mock.calls[0][1]).toMatchObject({ search: 'acme' }); + }); + + it('forwards the view’s searchableFields alongside the term', async () => { + const { exportDownload } = harness( + { ...BASE, searchableFields: ['name', 'stage'] }, + { initialSearchTerm: 'acme' }, + ); + await clickExportCsv(); + await vi.waitFor(() => expect(exportDownload).toHaveBeenCalledTimes(1)); + expect(exportDownload.mock.calls[0][1]).toMatchObject({ + search: 'acme', + searchFields: ['name', 'stage'], + }); + }); + + it('sends no search key when nothing is searched', async () => { + const { exportDownload } = harness(BASE); + await clickExportCsv(); + await vi.waitFor(() => expect(exportDownload).toHaveBeenCalledTimes(1)); + expect(exportDownload.mock.calls[0][1]).not.toHaveProperty('search'); + expect(exportDownload.mock.calls[0][1]).not.toHaveProperty('searchFields'); + }); + + it('omits searchFields when the view declares none', async () => { + const { exportDownload } = harness(BASE, { initialSearchTerm: 'acme' }); + await clickExportCsv(); + await vi.waitFor(() => expect(exportDownload).toHaveBeenCalledTimes(1)); + expect(exportDownload.mock.calls[0][1]).not.toHaveProperty('searchFields'); + }); +}); + +describe('the fetch and the export build the SAME filter', () => { + beforeEach(() => { + vi.spyOn(URL, 'createObjectURL').mockReturnValue('blob:x'); + vi.spyOn(URL, 'revokeObjectURL').mockImplementation(() => {}); + }); + afterEach(() => vi.restoreAllMocks()); + + /** + * Derived, not restated: compare what the two real calls actually carried. + * An assertion that spelled out the expected AST twice would go on passing if + * both copies drifted the same wrong way. + */ + const cases: Array<[string, ListViewSchema]> = [ + ['a view filter alone', { ...BASE, filter: [{ field: 'stage', operator: 'eq', value: 'won' }] as any }], + ['a multi-rule view filter', { + ...BASE, + filter: [ + { field: 'stage', operator: 'eq', value: 'won' }, + { field: 'amount', operator: 'gt', value: 10 }, + ] as any, + }], + ['an AST-shaped view filter', { ...BASE, filter: [['stage', '=', 'won']] as any }], + ['no filter at all', BASE], + ]; + + for (const [name, schema] of cases) { + it(`agree for ${name}`, async () => { + const { exportDownload, find } = harness(schema); + await vi.waitFor(() => expect(find).toHaveBeenCalled()); + const fetched = find.mock.calls[0][1]?.$filter; + + await clickExportCsv(); + await vi.waitFor(() => expect(exportDownload).toHaveBeenCalledTimes(1)); + const exported = exportDownload.mock.calls[0][1]?.filter; + + expect(exported).toEqual(fetched); + }); + } +}); diff --git a/packages/types/src/data.ts b/packages/types/src/data.ts index 9289a263d..c6cdac341 100644 --- a/packages/types/src/data.ts +++ b/packages/types/src/data.ts @@ -1007,8 +1007,13 @@ export interface ListImportJobsOptions { /** * Request payload for `DataSource.exportDownload` (synchronous streamed export). * - * Mirrors the active list view: pass the same `filter` / `sort` the list is - * showing so the exported file matches what the user sees. + * Mirrors the active list view: pass the same `filter` / `search` / `sort` the + * list is showing so the exported file matches what the user sees. + * + * `search` was missing until objectstack#4230, and the omission was not + * visible: exporting a searched list quietly produced the UNSEARCHED superset — + * more rows than the screen showed, in a file that looks authoritative. A + * caller that mirrors only `filter` still has that bug. */ export interface ExportDownloadRequest { /** Output file format. Defaults to 'csv'. */ @@ -1017,6 +1022,14 @@ export interface ExportDownloadRequest { fields?: string[]; /** Server-side filter (engine-specific shape, often the active view filter). */ filter?: unknown; + /** + * Full-text term, same semantics as the list read's `$search`. Composes with + * `filter` server-side rather than replacing it. Requires a server with + * objectstack#4230; older servers ignore it (and export the wider set). + */ + search?: string; + /** Optional override for which fields `search` scans (ADR-0061). */ + searchFields?: string[]; /** Sort instructions; multiple keys allowed, order preserved. */ sort?: Array<{ field: string; direction?: 'asc' | 'desc' }>; /** Hard cap on records exported (server enforces its own ceiling too). */