diff --git a/.changeset/orderby-string-serialization.md b/.changeset/orderby-string-serialization.md new file mode 100644 index 000000000..71391d411 --- /dev/null +++ b/.changeset/orderby-string-serialization.md @@ -0,0 +1,23 @@ +--- +"@object-ui/data-objectstack": patch +--- + +fix(data-objectstack): a string `$orderby` reaches the server as a sort instead of a list of character indices — #3106 + +`QueryParams['$orderby']` declares four shapes — `string`, `string[]`, +`SortNode[]`, `Record`. Both of this adapter's `find()` routes +(`convertQueryParams` for a plain read, `rawFindWithPopulate` for one carrying +`$expand`/`$search`) carried their own copy of the fold that serializes it, and +both copies handled the same three. The bare string fell through to the +`Record` branch, where `Object.entries('name asc')` enumerates the string's +character indices — so the request went out as `sort=0,1,2,3,4,5,6,7`. + +Since `objectstack#4226` the server refuses a sort it cannot read +(`400 INVALID_SORT`) rather than dropping it silently, so this was not a +degraded ordering but a list that failed to load outright — and `"${field} +${order}"` is exactly the shape `ObjectGrid` builds from its view metadata's +`sort`, making every standalone grid with a configured sort a broken one. + +Both routes now share one exported `serializeOrderBy`, for the same reason the +filter path already shares one: two copies of a fold can only agree by +inspection, and these two did not. diff --git a/packages/data-objectstack/src/index.ts b/packages/data-objectstack/src/index.ts index d267b1185..af80e77f7 100644 --- a/packages/data-objectstack/src/index.ts +++ b/packages/data-objectstack/src/index.ts @@ -300,6 +300,55 @@ function translateFilterToAST(filter: unknown): unknown | undefined { return undefined; } +/** + * Serialize a `$orderby` to the server's `sort` shorthand + * (`field,-other_field`), for every shape `QueryParams['$orderby']` declares. + * + * The type declares four — `string`, `string[]`, `SortNode[]`, + * `Record` — and the two `find()` routes each open-coded a + * fold that handled three of them. The missing one was the bare string, and it + * did not degrade quietly: `Object.entries('name asc')` enumerates a string's + * character indices, so the request went out as `sort=0,1,2,3,4,5,6,7`. Against + * a server that rejects an unreadable sort rather than ignoring it + * (objectstack#4226), that is a `400 INVALID_SORT` and an empty list — so a + * standalone `ObjectGrid` with a `sort` in its metadata, which is exactly the + * shape it builds (`ObjectGrid.tsx`: `` `${field} ${order}` ``), failed to load + * at all. + * + * One serializer for both routes, for the reason the filter path already has + * one: two copies of a fold can only agree by inspection, and these two did not. + * + * Returns `undefined` when nothing is sortable, so callers skip the parameter + * entirely rather than sending an empty one. + */ +export function serializeOrderBy(orderby: QueryParams['$orderby']): string | undefined { + if (orderby === undefined || orderby === null) return undefined; + + // `field asc` / `-field` / a comma-separated list of either — already the + // wire shorthand the server parses, so it rides through untouched. + if (typeof orderby === 'string') { + const trimmed = orderby.trim(); + return trimmed.length > 0 ? trimmed : undefined; + } + + const shorthand = (field: string, order?: unknown) => + String(order).toLowerCase() === 'desc' ? `-${field}` : field; + + if (Array.isArray(orderby)) { + const parts = orderby + .map((item) => (typeof item === 'string' ? item.trim() : shorthand(item.field, item.order))) + .filter((s) => s.length > 0); + return parts.length > 0 ? parts.join(',') : undefined; + } + + if (typeof orderby === 'object') { + const parts = Object.entries(orderby).map(([field, order]) => shorthand(field, order)); + return parts.length > 0 ? parts.join(',') : undefined; + } + + return undefined; +} + // Module-level discovery cache. Multiple ObjectStackAdapter instances pointed // at the same baseUrl (e.g. ConditionalAuthWrapper's throwaway adapter + // AdapterProvider's main adapter) would otherwise each fire `/discovery`. By @@ -2057,22 +2106,8 @@ export class ObjectStackAdapter implements DataSource { } // Sorting - if (params.$orderby) { - if (Array.isArray(params.$orderby)) { - const sortStr = params.$orderby.map(item => { - if (typeof item === 'string') return item; - const field = item.field; - const order = item.order || 'asc'; - return order === 'desc' ? `-${field}` : field; - }).join(','); - queryParams.set('sort', sortStr); - } else { - const sortStr = Object.entries(params.$orderby) - .map(([field, order]) => order === 'desc' ? `-${field}` : field) - .join(','); - queryParams.set('sort', sortStr); - } - } + const sortStr = serializeOrderBy(params.$orderby); + if (sortStr) queryParams.set('sort', sortStr); // Filter — translate ViewFilterRule[] (`[{field, operator, value}]`) // and other shapes into AST tuples the server understands. Without this, @@ -2229,25 +2264,11 @@ export class ObjectStackAdapter implements DataSource { } } - // Sorting - convert to ObjectStack format - if (params.$orderby) { - if (Array.isArray(params.$orderby)) { - // Handle array format ['name', '-age'] or [{ field: 'name', order: 'asc' }] - options.sort = params.$orderby.map(item => { - if (typeof item === 'string') return item; - // Handle object format { field: 'name', order: 'desc' } - const field = item.field; - const order = item.order || 'asc'; - return order === 'desc' ? `-${field}` : field; - }); - } else { - // Handle Record format { name: 'asc', age: 'desc' } - const sortArray = Object.entries(params.$orderby).map(([field, order]) => { - return order === 'desc' ? `-${field}` : field; - }); - options.sort = sortArray; - } - } + // Sorting — the same serializer the raw GET route uses, so the two `find()` + // paths cannot disagree about one stored sort. The client SDK's + // `QueryOptions.sort` accepts the shorthand string directly. + const sort = serializeOrderBy(params.$orderby); + if (sort) options.sort = sort; // Pagination if (params.$skip !== undefined) { diff --git a/packages/data-objectstack/src/orderby-serialization.test.ts b/packages/data-objectstack/src/orderby-serialization.test.ts new file mode 100644 index 000000000..099ca52e7 --- /dev/null +++ b/packages/data-objectstack/src/orderby-serialization.test.ts @@ -0,0 +1,115 @@ +/** + * 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. + */ + +/** + * Every shape `QueryParams['$orderby']` declares reaches the wire as a sort. + * + * `find()` has TWO routes to the server — a plain read goes through + * `convertQueryParams` and the client SDK, a read with `$expand`/`$search` + * through `rawFindWithPopulate` — and each used to carry its own copy of the + * "which `$orderby` shape is this?" fold. Both copies handled three of the four + * declared shapes; both missed the bare string, and missed it the same + * spectacular way: the string fell into the `Record` branch, + * where `Object.entries('name asc')` enumerates character indices and the + * request went out as `sort=0,1,2,3,4,5,6,7`. + * + * That is not a cosmetic mistranslation. Since objectstack#4226 the server + * refuses a sort it cannot read (`400 INVALID_SORT`) rather than dropping it, + * so the whole list fails to load — and `"${field} ${order}"` is exactly what a + * standalone `ObjectGrid` builds from its view metadata's `sort`. + * + * Both routes are asserted for every shape, because "the two routes agree" is + * the property that was actually missing; one serializer now backs both. + */ + +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { ObjectStackAdapter, clearSharedDiscoveryCache, serializeOrderBy } from './index'; + +function makeAdapter() { + const calls: string[] = []; + const fetchImpl = vi.fn(async (url: any) => { + const u = String(url); + calls.push(u); + if (u.includes('/api/v1/discovery')) { + return { + ok: true, status: 200, statusText: 'OK', + json: async () => ({ success: true, data: { version: 'v1', routes: {} } }), + } as any; + } + return { + ok: true, status: 200, statusText: 'OK', + json: async () => ({ success: true, data: { object: 'account', records: [], total: 0 } }), + } as any; + }); + const adapter = new ObjectStackAdapter({ + baseUrl: 'http://localhost:3000', token: 't', autoReconnect: false, fetch: fetchImpl as any, + }); + return { adapter, calls }; +} + +/** The `sort=` this `$orderby` produced on the wire, or `undefined` if none was sent. */ +async function sortOnWire($orderby: unknown, route: 'plain' | 'expand'): Promise { + const { adapter, calls } = makeAdapter(); + await adapter.find('account', { + $orderby, + ...(route === 'expand' ? { $expand: ['owner'] } : {}), + } as any); + const dataCall = calls.filter((u) => u.includes('/data/account')).pop(); + const raw = dataCall ? new URL(dataCall).searchParams.get('sort') : null; + return raw === null ? undefined : raw; +} + +/** Run one input down both `find()` routes and assert they agree. */ +function bothRoutes(name: string, $orderby: unknown, expected: string | undefined) { + for (const route of ['plain', 'expand'] as const) { + it(`${name} (${route} route)`, async () => { + expect(await sortOnWire($orderby, route)).toBe(expected); + }); + } +} + +describe('$orderby reaches the wire for every declared shape', () => { + beforeEach(() => clearSharedDiscoveryCache()); + + // The regression: ObjectGrid's own `"${field} ${order}"`. + bothRoutes('a bare "field order" string', 'name asc', 'name asc'); + bothRoutes('a bare string with a descending key', 'created_at desc', 'created_at desc'); + bothRoutes('a bare string already in `-field` shorthand', '-created_at', '-created_at'); + bothRoutes('a bare multi-key string', 'status asc,-created_at', 'status asc,-created_at'); + + bothRoutes('a SortNode[]', [{ field: 'name', order: 'asc' }], 'name'); + bothRoutes( + 'a SortNode[] with a descending key', + [{ field: 'status', order: 'asc' }, { field: 'created_at', order: 'desc' }], + 'status,-created_at', + ); + bothRoutes('a string[]', ['name', '-age'], 'name,-age'); + bothRoutes('a Record', { name: 'asc', age: 'desc' }, 'name,-age'); + + bothRoutes('no sort at all', undefined, undefined); + bothRoutes('an empty array', [], undefined); + bothRoutes('an empty object', {}, undefined); + bothRoutes('a whitespace-only string', ' ', undefined); +}); + +describe('serializeOrderBy', () => { + it('never enumerates a string as an object — the defect this replaced', () => { + // `Object.entries('name asc')` → [['0','n'],['1','a'],…]. The old fold + // reached that branch for every string input. + expect(serializeOrderBy('name asc')).toBe('name asc'); + expect(serializeOrderBy('name asc')).not.toMatch(/^0,1,2/); + }); + + it('reads `order` case-insensitively', () => { + expect(serializeOrderBy([{ field: 'name', order: 'DESC' as any }])).toBe('-name'); + }); + + it('defaults a missing direction to ascending', () => { + expect(serializeOrderBy([{ field: 'name' }])).toBe('name'); + }); +});