fix(data-objectstack): a string $orderby reaches the server as a sort, not a list of character indices (#3106)

.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<field, direction>`. 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.

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<field, direction>` — 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<T = unknown> implements DataSource<T> {
     }
 
     // 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<T = unknown> implements DataSource<T> {
       }
     }
 
-    // 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) {

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<field, direction>` 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<string | undefined> {
+  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<field, direction>', { 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');
+  });
+});