diff --git a/.changeset/8767-object-grid-refuses-string-sort.md b/.changeset/8767-object-grid-refuses-string-sort.md new file mode 100644 index 0000000000..5ab4c24690 --- /dev/null +++ b/.changeset/8767-object-grid-refuses-string-sort.md @@ -0,0 +1,62 @@ +--- +'@object-ui/plugin-grid': minor +--- + +`object-grid` REFUSES the retired string `sort` clause at its own read site +(objectui#8767, maintainer ruling 2026-09-10 — route C). + +**Breaking, deliberately.** A `sort: "name desc"` on an `object-grid` node no +longer reaches `$orderby`. It is reported once per spelling with the diagnostic +objectui#8221 / PR #8758 already ship — the message names the offending value +and prescribes the array form — and the query goes out carrying no ordering at +all, exactly as the same key already behaves on `object-view`. + +**Why it was still lowering.** The #8221 ruling retired the legacy string +clause: one spelling, the array, everywhere. PR #8758 narrowed the shared sink +`convertSortToQueryParams` and every declaration that published a string arm, +but `ObjectGrid` never used that sink — it reads `schema.sort` and lowers it +with private code, so a bare grid went on honouring at runtime a spelling +`object-view` refuses. One key, two meanings, chosen by which block you are on +— which is the per-block divergence the ruling declined by name when it +rejected option A. + +**Migration.** Write the array: `sort: [{ field: 'name', order: 'desc' }]`. +**Both keys are required.** `SortConfig.order` carries no `?` in +`@object-ui/types` (`packages/types/src/objectql.ts`) and no `.optional()` in +its zod mirror, and the protocol's own reusable `SortItemSchema` requires +`order` as well — measured: that schema refuses `[{ field: 'name' }]` with +`invalid_value` at `0.order`. Do not omit it: this block's array arm +interpolates whatever is present, so an omitted `order` lowers to +`$orderby: 'name undefined'` today. That is pre-existing behaviour on the arm +this change does not touch, and it is filed as a successor card rather than +widened into here. + +**What the spec face does and does not say.** Measured against the installed +`@objectstack/spec@17.4.0`: `ui.ObjectGridPropsSchema` is **value-agnostic** on +this key — `sort` is `z.unknown().optional()`, so `safeParse` accepts +`'name desc'`, `'name'`, `42`, `['name desc']` and `{ name: 'desc' }` alike, +while an undeclared `bogusProp` is refused with `unrecognized_keys` (the +control that shows those parse readings are real and not a schema that accepts +everything). So the protocol's **validator** does not refuse the string, and +this change is not a narrowing the validator already performed. + +What the protocol **declares and documents** is the array, in three places: +that same key's own `describe` reads `Initial sort (array of { field, order })`; +the sibling `ElementRecordPickerPropsSchema.sort` spells the identical intent as +a typed `z.array(SortItemSchema)`, which refuses a string outright; and the +`defaultSort` retirement text instructs authors to rename the key to `sort` and +`wrap the value in an array`. The array is likewise the only spelling +`ObjectGridSchema.sort` has declared since #8221, the only one the registered +`sort` input publishes (`type: 'array'`), and the only one +`convertSortToQueryParams` lowers. So type-checked metadata is already on it, +and only untyped JSON or a stored `sys_metadata` row can still carry the string +— which is exactly why the refusal is a loud runtime diagnostic rather than a +type change. + +**What is deliberately unchanged.** The wire shape. The array arm still lowers +to this block's own `"field order[, field order]"` join string, and the export +path and the header-arrow reader `parseSchemaSort` still read the key exactly as +before. Routing the whole key through the shared sink would send its +`{ field: direction }` map where every grid today sends a string; that is a +separate change with its own blast radius, and this ruling explicitly did not +take it. diff --git a/packages/plugin-grid/src/ObjectGrid.tsx b/packages/plugin-grid/src/ObjectGrid.tsx index 95d8fbbab5..064b293dcd 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, leadWithNameField, buildExpandFields, buildExportFileName, columnIdentity, collectPredicateFieldRefs, collectGroupingFieldRefs, listViewPredicates, isObjectInlineEditable, isProjectableField, isExpandableFieldType, isUnmaterializedFieldType, readObjectSortability, isPlatformSortableField, filterPlatformSortableSort, toFilterNode, ROW_HEIGHT_TO_DENSITY_MODE, resolveRecordSourceConfig, resolveRecordSourceObjectName } from '@object-ui/core'; +import { resolveConditionalFormatting, leadWithNameField, buildExpandFields, buildExportFileName, columnIdentity, collectPredicateFieldRefs, collectGroupingFieldRefs, listViewPredicates, isObjectInlineEditable, isProjectableField, isExpandableFieldType, isUnmaterializedFieldType, readObjectSortability, isPlatformSortableField, filterPlatformSortableSort, toFilterNode, convertSortToQueryParams, type QuerySortEntry, ROW_HEIGHT_TO_DENSITY_MODE, resolveRecordSourceConfig, resolveRecordSourceObjectName } from '@object-ui/core'; import { usePermissions } from '@object-ui/permissions'; import { ChevronRight, ChevronLeft, ChevronsLeft, ChevronsRight, Download, Rows2, Rows3, Rows4, AlignJustify, Type, Hash, Calendar, CheckSquare, User, Tag, Clock, Loader2 } from 'lucide-react'; import { useRowColor } from './useRowColor'; @@ -59,13 +59,22 @@ import type { BulkActionDef } from '@object-ui/types'; /** * A view's declared `sort` → the shape the table's header indicators read. * - * `@objectstack/spec` allows `"name desc"`, `["name desc", …]` and - * `[{ field, order }, …]`, and this grid's own fetch path already reads all - * three. The headers have to agree with it: a view that arrives sorted by + * `[{ field, order }, …]` is the ONE spelling `@objectstack/spec` still + * declares (objectui#8221 retired the string clauses). The headers have to + * agree with the fetch path on it: a view that arrives sorted by * `created_at desc` should show that arrow before anyone clicks anything — * otherwise the first click on that column produces `asc` while the list was * already `desc`, and the arrow tells the truth only from the second click on. * + * ⚠️ This reader is WIDER than the fetch path as of objectui#8767, and the + * sentence this docblock used to carry — that the fetch path reads all three + * spellings — is no longer true. The fetch path now REFUSES a string and sends + * no `$orderby` at all, while this reader still parses `"name desc"` and + * `["name desc", …]`, so a grid authored with a retired spelling shows an + * arrow for an ordering its query does not carry. Narrowing this reader moves + * the wire shape and takes the export path with it — the route the #8767 + * ruling deliberately did not take. It is NOT fixed here. + * * Exported for the test that pins it against the fetch path's own reading. */ export function parseSchemaSort(sort: unknown): TableSortItem[] { @@ -1851,7 +1860,30 @@ export const ObjectGrid: React.FC = ({ params.$orderby = headerSort.map((s) => ({ field: s.field, order: s.order })); } else if (schemaSort) { if (typeof schemaSort === 'string') { - params.$orderby = schemaSort; + // objectui#8767 — the legacy string `sort` clause is RETIRED + // (objectui#8221, decision batch #77) and is REFUSED here rather + // than lowered. This block owns a PRIVATE lowering, so #8758's + // narrowing of the shared sink never reached it: a bare + // `object-grid` went on honouring a spelling `object-view` + // already refuses — one key meaning two things depending on which + // block you are on, which is the per-block divergence the #8221 + // ruling declined by name. + // + // The refusal is #8758's OWN, not a second one: calling the + // shared sink on this arm reports the retired spelling once per + // spelling and answers `undefined`, so the query carries no + // `$orderby`. Its return value is deliberately unused — the wire + // shape stays this block's, and the array arm below is untouched + // (with it the export path and `parseSchemaSort`). Routing the + // whole key through the sink is a different card: it would send + // the sink's `{field: direction}` map where every grid today + // sends a `"field order"` string. + // + // Read through `unknown`, exactly as the sink does: types are + // erased, so the array-only `ObjectGridSchema.sort` declaration + // cannot stop a string arriving from authored JSON, a stored + // `sys_metadata` row or an `as any` bag. + convertSortToQueryParams(schemaSort as unknown as QuerySortEntry[]); } else if (Array.isArray(schemaSort)) { params.$orderby = schemaSort .map((s: any) => `${s.field} ${s.order}`) @@ -4023,6 +4055,11 @@ export const ObjectGrid: React.FC = ({ // arrow, and the first click on that column would ask for `asc` on a list // that was already `desc`. // + // ⚠️ One spelling now escapes that agreement: since objectui#8767 the fetch + // path REFUSES a string `sort` and sends no `$orderby`, while + // {@link parseSchemaSort} still parses one. Closing that gap narrows this + // reader and moves the wire shape with it — the route #8767 did not take. + // // A plain expression, not a `useMemo`: this sits below the component's early // returns, where a hook would be skipped on some renders and change the hook // order. Parsing at most a handful of sort keys costs nothing worth a hook. diff --git a/packages/plugin-grid/src/__tests__/gridRetiredStringSort-8767.test.tsx b/packages/plugin-grid/src/__tests__/gridRetiredStringSort-8767.test.tsx new file mode 100644 index 0000000000..555c67b3b9 --- /dev/null +++ b/packages/plugin-grid/src/__tests__/gridRetiredStringSort-8767.test.tsx @@ -0,0 +1,173 @@ +/** + * 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. + * + * `object-grid` REFUSES the retired string `sort` clause (objectui#8767). + * + * ## What was wrong + * + * The #8221 ruling (decision batch #77) retired the legacy OData-ish string + * clause: one spelling, the array, everywhere. PR #8758 narrowed the shared + * sink `convertSortToQueryParams` so a string that still arrives at runtime is + * refused out loud and the query carries no `$orderby`. + * + * `ObjectGrid` never went through that sink. It reads `schema.sort` at its own + * site and lowers it with private code, so after #8758 a bare `object-grid` + * still forwarded a string verbatim to `$orderby` while the SAME key routed + * through `object-view` was refused with a diagnostic — one key meaning two + * things depending on which block you are on. That is precisely the shape the + * ruling rejected when it declined option A by name: + * + * > per-block arms would make one key mean different things on different + * > blocks and keep a spelling the spec already refuses on one of them. + * + * ## What these tests pin, and what they deliberately do NOT + * + * Route C, as ruled: the string arm is refused with #8758's own reporter, and + * NOTHING else about this block moves. So the file has two halves, and the + * second is what stops it passing by refusing everything: + * + * 1. a string `sort` reaches no `$orderby`, and says so once per spelling; + * 2. the array arm still lowers to the very same `"field order"` join string + * this block has always sent — byte for byte, single- and multi-key. + * + * The join string is the wire shape route B would change and route C keeps. + * The header-arrow reader `parseSchemaSort` and the export path read the same + * key and are untouched here; they belong to that other card. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, waitFor } from '@testing-library/react'; +import React from 'react'; +import { SchemaRenderer, SchemaRendererProvider } from '@object-ui/react'; +import { resetRetiredSortSpellingReports } from '@object-ui/core'; +// Registers `object-grid` and its `view:grid` alias. +import '../index'; + +function makeAdapter() { + return { + find: vi.fn().mockResolvedValue({ + data: [{ id: '1', name: 'Acme', status: 'active' }], + total: 1, + }), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + getObjectSchema: vi.fn().mockResolvedValue({ + name: 'account', + fields: { id: { type: 'text' }, name: { type: 'text' }, status: { type: 'text' } }, + }), + }; +} + +/** + * Render the block and return the params of its first `find` call — the same + * harness shape as the sibling `gridDefaultFiltersLowering.test.tsx`, so both + * legs of this read site are observed through one lens. + */ +async function findParamsFor(schema: Record) { + const adapter = makeAdapter(); + render( + + + , + ); + await waitFor(() => expect(adapter.find).toHaveBeenCalled()); + return (adapter.find.mock.calls[0] as [string, any])[1]; +} + +const BASE = { type: 'object-grid', objectName: 'account', columns: [{ field: 'name' }] }; + +/** + * The retired spelling, reached the only way it still can be. The declaration + * is `SortConfig[]` since #8221, so a string arrives from authored JSON, a + * stored `sys_metadata` row or an `as any` bag — never from a typed caller. + */ +const authoredAtRuntime = (sort: unknown) => ({ ...BASE, sort }) as Record; + +let errorSpy: ReturnType; + +beforeEach(() => { + // The reporter dedupes per spelling in MODULE state. Without this reset the + // second test to assert the diagnostic would observe silence and pass for + // the wrong reason. + resetRetiredSortSpellingReports(); + errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); +}); + +afterEach(() => { + errorSpy.mockRestore(); +}); + +describe('object-grid — the retired string `sort` clause is REFUSED at this block’s own read site (objectui#8767)', () => { + it('carries NO `$orderby` for a string `sort`, and names the array form', async () => { + const params = await findParamsFor(authoredAtRuntime('name desc')); + + // The defect in one line: this used to be the string `'name desc'`, i.e. + // the grid honoured at runtime what `object-view` refuses. + expect(params.$orderby).toBeUndefined(); + expect(Object.prototype.hasOwnProperty.call(params, '$orderby')).toBe(false); + + expect(errorSpy).toHaveBeenCalled(); + const message = String(errorSpy.mock.calls[0][0]); + // #8758's OWN diagnostic, not a second one written here: it quotes the + // offending spelling and prescribes the array form, because a refusal with + // no prescription only moves the author's problem. + expect(message).toContain('"name desc"'); + expect(message).toContain("[{ field: 'name', order: 'desc' }]"); + expect(message).toContain('objectui#8221'); + }); + + it('refuses a bare field string too — every retired spelling, not just the two-word one', async () => { + const params = await findParamsFor(authoredAtRuntime('name')); + expect(params.$orderby).toBeUndefined(); + expect(errorSpy).toHaveBeenCalled(); + }); + + it('reports once per spelling, not once per grid', async () => { + await findParamsFor(authoredAtRuntime('name desc')); + await findParamsFor(authoredAtRuntime('name desc')); + // Two blocks inheriting the same bad view sort print one line between them + // — the dedupe is the reporter's, and reusing it is what keeps this read + // site from becoming a second, differently-behaved diagnostic. + expect(errorSpy).toHaveBeenCalledTimes(1); + }); + + it('does not fall through to the legacy `defaultSort` leg when it refuses', async () => { + // A refusal that quietly handed the query to the next arm would be a + // silent substitution — a different ordering than either the author asked + // for or the refusal announced. + const params = await findParamsFor({ + ...authoredAtRuntime('name desc'), + defaultSort: { field: 'status', order: 'asc' }, + }); + expect(params.$orderby).toBeUndefined(); + }); +}); + +describe('CONTROL — the array arm lowers UNCHANGED (route C keeps this block’s wire shape)', () => { + it('still sends the single-key `"field order"` join string', async () => { + const params = await findParamsFor({ ...BASE, sort: [{ field: 'name', order: 'desc' }] }); + // Byte-identical to what this block sent before #8767. Route B would send + // the shared sink's `{ name: 'desc' }` map here; that is a different card, + // and this line is what would catch it arriving by accident. + expect(params.$orderby).toBe('name desc'); + expect(errorSpy).not.toHaveBeenCalled(); + }); + + it('still joins a multi-key array with `, `', async () => { + const params = await findParamsFor({ + ...BASE, + sort: [ + { field: 'status', order: 'asc' }, + { field: 'name', order: 'desc' }, + ], + }); + expect(params.$orderby).toBe('status asc, name desc'); + expect(errorSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/plugin-grid/src/__tests__/serverSorting.test.tsx b/packages/plugin-grid/src/__tests__/serverSorting.test.tsx index 468d09856e..79021cbff5 100644 --- a/packages/plugin-grid/src/__tests__/serverSorting.test.tsx +++ b/packages/plugin-grid/src/__tests__/serverSorting.test.tsx @@ -22,6 +22,7 @@ import React from 'react'; import { ObjectGrid, parseSchemaSort } from '../ObjectGrid'; import { registerAllFields } from '@object-ui/fields'; +import { resetRetiredSortSpellingReports } from '@object-ui/core'; import { ActionProvider } from '@object-ui/react'; registerAllFields(); @@ -138,9 +139,14 @@ describe('ObjectGrid — column-header sorting is server-side (#3106)', () => { it('replaces the view\'s declared sort rather than stacking on it', async () => { const ds = makeDataSource(); - const { container } = renderGrid(ds, { sort: 'name desc' }); + const { container } = renderGrid(ds, { sort: [{ field: 'name', order: 'desc' }] }); await waitFor(() => expect(screen.getByText('Row 0')).toBeInTheDocument()); - // The declared sort goes out as the string form it was authored in. + // Authored in the ONE declared spelling. This case used to author the + // retired string clause and assert that it went out verbatim; objectui#8767 + // made this block REFUSE a string (objectui#8221), so a string here would + // reach no `$orderby` at all and the click would have nothing to replace. + // The array arm still lowers to this block's own `"field order"` string — + // unchanged, which is the point of route C. expect(lastFindParams(ds).$orderby).toBe('name desc'); fireEvent.click(headerCell(container, 'Status')); @@ -153,8 +159,14 @@ describe('ObjectGrid — column-header sorting is server-side (#3106)', () => { it('shows the view\'s declared sort before anyone clicks', async () => { // Otherwise the first click on that column asks for `asc` on a list that is // already `desc`, and the arrow only tells the truth from click two on. + // + // Authored in the ONE declared spelling. This case used to author the + // retired string clause; the subject is the header behaviour and the + // spelling was incidental, so it moves to the array for the same reason + // the sibling case above did. What a string does to this same schema is + // pinned deliberately, in the case below. const ds = makeDataSource(); - const { container } = renderGrid(ds, { sort: 'status desc' }); + const { container } = renderGrid(ds, { sort: [{ field: 'status', order: 'desc' }] }); await waitFor(() => expect(screen.getByText('Row 0')).toBeInTheDocument()); expect(headerCell(container, 'Status').querySelector('[class*="chevron-down"]')).not.toBeNull(); @@ -166,6 +178,48 @@ describe('ObjectGrid — column-header sorting is server-side (#3106)', () => { }); }); + it('DIVERGENCE, pinned not tolerated (objectui#8961): a retired STRING sort still draws the arrow while the wire carries NO ordering', async () => { + // objectui#8767 made the fetch path REFUSE a string `sort`. The header + // indicators are fed by `parseSchemaSort`, a second private reader that + // the ruling deliberately left alone — and it still parses a string. So + // the two readers of one key now disagree, and this asserts BOTH halves of + // that disagreement. + // + // Asserting only the arrow (which is what this file did before) leaves a + // green test that reads as "string sort ⇒ arrow, as intended". A green + // test does not make a divergence visible; it certifies it. Naming both + // halves is what makes the state a recorded defect instead of an expected + // one, and makes THIS the case that has to change when objectui#8961 + // closes the gap — narrowing the reader moves the wire shape and takes the + // export path with it, the route objectui#8767 did not take. + resetRetiredSortSpellingReports(); + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + const ds = makeDataSource(); + const { container } = renderGrid(ds, { sort: 'status desc' }); + await waitFor(() => expect(screen.getByText('Row 0')).toBeInTheDocument()); + + // Half one — the header still tells the user this list is `status desc`. + expect( + headerCell(container, 'Status').querySelector('[class*="chevron-down"]'), + ).not.toBeNull(); + + // Half two — and the query it was fetched with carries no ordering at + // all. `hasOwnProperty`, not `toBeUndefined`: the key is absent, which is + // a different claim from "present and undefined". + const params = lastFindParams(ds); + expect(Object.prototype.hasOwnProperty.call(params, '$orderby')).toBe(false); + + // …and the disagreement is announced once, by PR #8758's own reporter. + const retired = errorSpy.mock.calls.filter((c) => + String(c[0]).includes('objectui#8221'), + ); + expect(retired).toHaveLength(1); + } finally { + errorSpy.mockRestore(); + } + }); + it('withholds the sort affordance from a relational column (#3096)', async () => { const ds = makeDataSource(); const { container } = renderGrid(ds); @@ -182,7 +236,16 @@ describe('ObjectGrid — column-header sorting is server-side (#3106)', () => { }); }); -describe('parseSchemaSort — the header reads what the fetch path reads', () => { +/** + * ⚠️ This block pins the header reader's OWN contract, which since + * objectui#8767 is WIDER than the fetch path's — hence the renamed title. The + * cases below still admit the retired string spellings because this reader + * still admits them; the fetch path refuses them (see the divergence pin + * above). Re-judging these inputs belongs to objectui#8961, which narrows this + * reader and moves the wire shape with it. Until then they are read as "what + * `parseSchemaSort` accepts", never as "what the grid queries with". + */ +describe('parseSchemaSort — the header reader\'s own contract, wider than the fetch path (objectui#8961)', () => { it('reads the bare-string form', () => { expect(parseSchemaSort('name desc')).toEqual([{ field: 'name', order: 'desc' }]); });