fix(metadata-protocol): sort audit history and global search by order, not direction (#4674)

claude · claude · commit 0183be7dfa14 · 2026-08-02T21:30:21.000Z
Two internal `engine.find` calls wrote their sort as `{ field, direction: 'desc' }`. The QueryAST sort shape is `SortNodeSchema` = `{ field, order }`, and both real drivers normalize off `.order` with no fallback — `sql-driver` maps `item.order === 'desc'`, `mongodb-driver` the same. With `order` absent, `undefined === 'desc'` is false and both queries ran ASCENDING. `direction` is `IReportService`'s vocabulary, a genuinely different contract, which is how the wrong spelling looked plausible. Because both queries carry a `limit`, the wrong direction did not merely reorder a page — it changed WHICH ROWS CAME BACK: - Metadata audit history returned the OLDEST `limit` events: the beginning of an object's life and never its recent changes. On a long-lived object an editor would never see what they came for. - Global search returned the STALEST `perObject` matches, truncating away the recently-edited records — the ones a searcher is most likely to want. The `as any` / `: any` at both sites go with it. `EngineQueryOptions.orderBy` is `SortNodeSchema[]` and would have rejected `direction`; the erasure is what let this through, and for an internal caller `tsc` IS the enforced channel — the protocol's `INVALID_SORT` normalizer does not run on calls the protocol makes to `this.engine.find` directly, and it rejects bad VALUES rather than unknown KEYS anyway (the schema is not `.strict()`, so `direction` was dropped, not flagged). Also fixes the `sys_metadata_history` double in `protocol-publish-rollback.test.ts`, which destructured `{ field, direction }` off `opts.orderBy` — an ARRAY. Both names read `undefined`, so it spoke the vocabulary the engine does not read AND sorted nothing at all; a test built on it would have ratified either failure. The regression tests assert on row IDENTITY rather than sequence, with `limit` below the fixture size, so a wrong direction returns a disjoint set — an order-only assertion would pass against a double that ignored `orderBy` entirely. Verified they fail against the old code: `['a1','a2']` instead of `['a5','a4']` for audit history, `['c1','c2']` instead of `['c4','c3']` for search. Swept the tree for the same mistake: the three other `direction` sites are all correct — `IReportService`'s own contract, plugin-auth's explicit `direction` → `order` translation, and sql-driver's post-normalization internal vocabulary. These two were the only wrong ones. Item 4 of the issue (make the mismatch fail for EXTERNAL callers too) is deliberately not here: it is a separate decision, and it protects a different population — filed separately. Closes #4674 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NKcGqCYCCpMkB5UW8jNPXx
diff --git a/.changeset/orderby-direction-vocabulary.md b/.changeset/orderby-direction-vocabulary.md
@@ -0,0 +1,14 @@
+---
+"@objectstack/metadata-protocol": patch
+---
+
+fix(metadata-protocol): 元数据审计历史与全局搜索按 `order` 排序,不再按 `direction` (#4674)
+
+`protocol.ts` 里两处内部 `engine.find` 调用把排序写成 `{ field, direction: 'desc' }`。QueryAST 的排序形状是 `SortNodeSchema` = `{ field, order }`,两个真实驱动都只认 `.order` 且没有 `direction` 回退——`undefined === 'desc'` 为假,于是两个查询实际都在**升序**运行。`direction` 是 `IReportService` 的词汇,是另一份契约,这正是错误拼写看起来合理的原因。
+
+由于两个查询都带 `limit`,方向错误不只是把一页重排,而是**改变了哪些行会被返回**:
+
+- **元数据审计历史**取到的是最旧的 `limit` 条事件——一个对象生命的开头,而永远不是它最近的变更。在长期存在的对象上,编辑者要找的东西一条也看不到。
+- **全局搜索**取到的是最陈旧的 `perObject` 条匹配,最近编辑过的记录恰好被 `limit` 截断掉——而那正是搜索者最可能想要的。
+
+两处的 `as any` / `: any` 一并去掉:`EngineQueryOptions.orderBy` 是 `SortNodeSchema[]`,本来就会拒绝 `direction`,而类型擦除正是让它溜过去的原因。恢复类型是这次改动价值的大头,因为对内部调用方来说 `tsc` 就是那条被执行的渠道。
diff --git a/packages/metadata-protocol/src/protocol.orderby-vocabulary.test.ts b/packages/metadata-protocol/src/protocol.orderby-vocabulary.test.ts
@@ -0,0 +1,132 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+//
+// Two internal `engine.find` calls sorted by `direction`, a key nothing on that
+// path reads (#4674).
+//
+// The QueryAST sort shape is `SortNodeSchema` = `{ field, order }`
+// (`packages/spec/src/data/query.zod.ts`), and both real drivers normalize off
+// `.order` with no fallback — `sql-driver` maps `item.order === 'desc'`,
+// `mongodb-driver` the same. With `order` absent, `undefined === 'desc'` is
+// false and both land on ASCENDING. `direction` is `IReportService`'s
+// vocabulary; it is a genuinely different contract, which is how the wrong
+// spelling looked plausible.
+//
+// Because both queries carry a `limit`, the wrong direction did not merely
+// reorder a page — it changed WHICH ROWS CAME BACK. So these tests assert on
+// identity, not sequence: with a limit smaller than the fixture, sorting the
+// wrong way returns a disjoint set. An order-only assertion would have passed
+// against a fake that ignored `orderBy` entirely.
+//
+// Nothing caught this because both sites erased their types (`} as any)` and
+// `const opts: any`), the protocol's `INVALID_SORT` normalizer does not run on
+// calls the protocol makes to `this.engine.find` directly, and that normalizer
+// rejects bad VALUES rather than unknown KEYS — the schema is not `.strict()`,
+// so `direction` was dropped rather than flagged.
+
+import { describe, it, expect, vi } from 'vitest';
+import { ObjectStackProtocolImplementation } from './protocol.js';
+
+/**
+ * A `find` that honours the QueryAST contract for sort and limit, and nothing
+ * else. Filtering is deliberately not implemented: what is under test is which
+ * rows survive `orderBy` + `limit`, and a double that also filtered would let a
+ * sort bug hide behind a `where` that happened to select the right rows.
+ *
+ * It reads `order` — the shape the drivers read. A double that read `direction`
+ * would agree with the bug instead of catching it, which is exactly what the
+ * publish-rollback double did until this change.
+ */
+function makeFind(rowsByObject: Record<string, any[]>) {
+    return vi.fn(async (object: string, opts: any = {}) => {
+        const rows = [...(rowsByObject[object] ?? [])];
+        for (const { field, order } of [...(opts.orderBy ?? [])].reverse()) {
+            rows.sort((a, b) => {
+                const av = a[field], bv = b[field];
+                if (av === bv) return 0;
+                return (av < bv ? -1 : 1) * (order === 'desc' ? -1 : 1);
+            });
+        }
+        return typeof opts.limit === 'number' ? rows.slice(0, opts.limit) : rows;
+    });
+}
+
+/** The options the protocol handed to `engine.find` on its first call. */
+const optionsFrom = (find: any) => find.mock.calls[0][1];
+
+const AUDIT_ROWS = ['2024-01-01', '2024-02-01', '2024-03-01', '2024-04-01', '2024-05-01'].map(
+    (d, i) => ({
+        id: `a${i + 1}`,
+        occurred_at: `${d}T00:00:00.000Z`,
+        actor: 'someone',
+        operation: 'save',
+        outcome: 'allowed',
+        code: 'OK',
+    }),
+);
+
+describe('auditMetaItem sorts newest-first (#4674)', () => {
+    function makeProtocol() {
+        const find = makeFind({ sys_metadata_audit: AUDIT_ROWS });
+        const engine = { registry: { getObject: () => undefined }, find };
+        return { p: new ObjectStackProtocolImplementation(engine as any), find };
+    }
+
+    it('returns the NEWEST `limit` events, not the oldest', async () => {
+        const { p } = makeProtocol();
+        const { events } = await p.auditMetaItem({ type: 'objects', name: 'invoice', limit: 2 });
+
+        // The whole defect in one assertion: ascending returns a1/a2 here.
+        expect(events.map(e => e.id)).toEqual(['a5', 'a4']);
+    });
+
+    it('asks for `order`, never `direction`', async () => {
+        const { p, find } = makeProtocol();
+        await p.auditMetaItem({ type: 'objects', name: 'invoice', limit: 2 });
+
+        const sort = optionsFrom(find).orderBy;
+        expect(sort).toEqual([{ field: 'occurred_at', order: 'desc' }]);
+        // Named explicitly: `direction` reads as a well-formed "sort by
+        // occurred_at, direction unspecified" and passes every existing check.
+        expect(sort[0]).not.toHaveProperty('direction');
+    });
+});
+
+const SEARCH_ROWS = ['2024-01-01', '2024-02-01', '2024-03-01', '2024-04-01'].map((d, i) => ({
+    id: `c${i + 1}`,
+    name: `Acme ${i + 1}`,
+    updated_at: `${d}T00:00:00.000Z`,
+}));
+
+const CONTACT = {
+    name: 'contact',
+    fields: { name: { name: 'name', type: 'text', searchable: true } },
+};
+
+describe('searchAll sorts newest-first (#4674)', () => {
+    function makeProtocol() {
+        const find = makeFind({ contact: SEARCH_ROWS });
+        const engine = {
+            registry: { getObject: (n: string) => (n === 'contact' ? CONTACT : undefined), getAllObjects: () => [CONTACT] },
+            find,
+        };
+        return { p: new ObjectStackProtocolImplementation(engine as any), find };
+    }
+
+    it('returns the most recently updated matches, not the stalest', async () => {
+        const { p } = makeProtocol();
+        const { hits } = await p.searchAll({ q: 'Acme', perObject: 2 });
+
+        // Ascending returned c1/c2 — the stalest rows, with the recently-edited
+        // ones truncated away by `perObject`.
+        expect(hits.map(h => h.id)).toEqual(['c4', 'c3']);
+    });
+
+    it('asks for `order`, never `direction`', async () => {
+        const { p, find } = makeProtocol();
+        await p.searchAll({ q: 'Acme', perObject: 2 });
+
+        const sort = optionsFrom(find).orderBy;
+        expect(sort).toEqual([{ field: 'updated_at', order: 'desc' }]);
+        expect(sort[0]).not.toHaveProperty('direction');
+    });
+});
diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts
@@ -24,7 +24,7 @@ import {
     SEARCHABLE_TEXTUAL_TYPES, SEARCHABLE_ENUM_TYPES, SEARCH_AUTO_EXCLUDED_FIELDS,
     RPC_QUERY_ALIAS_SLOTS, foldQueryAliasSlots,
     type QueryAliasConflict, type QueryAliasSlot,
-    type DroppedFieldsEvent, type QueryAST,
+    type DroppedFieldsEvent, type QueryAST, type EngineQueryOptions,
 } from '@objectstack/spec/data';
 import { PLURAL_TO_SINGULAR, SINGULAR_TO_PLURAL } from '@objectstack/spec/shared';
 import { applyConversionsToStoredItem, type ConversionNotice } from '@objectstack/spec';
@@ -3311,11 +3311,20 @@ export class ObjectStackProtocolImplementation implements
                 type: singular,
                 name: request.name,
             };
+            // `order`, NOT `direction`: the QueryAST sort shape is
+            // `SortNodeSchema` = `{ field, order }`, and both drivers normalize
+            // off `.order` with no fallback. `direction` is `IReportService`'s
+            // vocabulary and is silently DROPPED here (the schema is not
+            // `.strict()`), which left this query running ascending — the
+            // OLDEST `limit` audit events, i.e. the beginning of an object's
+            // life and never its recent changes (#4674). The `as any` is gone
+            // for the same reason: `EngineQueryOptions` rejects the wrong key,
+            // and erasing the type is what let it through.
             const rows = await this.engine.find('sys_metadata_audit', {
                 where,
-                orderBy: [{ field: 'occurred_at', direction: 'desc' }],
+                orderBy: [{ field: 'occurred_at', order: 'desc' }],
                 limit,
-            } as any);
+            });
             const events = (Array.isArray(rows) ? rows : []).map((r: any) => ({
                 id: r.id,
                 occurredAt:
@@ -5098,10 +5107,15 @@ export class ObjectStackProtocolImplementation implements
             const where = andClauses.length === 1 ? andClauses[0] : { $and: andClauses };
 
             try {
-                const opts: any = {
+                // `order`, NOT `direction` — see the audit-history query above.
+                // Ascending here returned the STALEST `perObject` matches and
+                // truncated away the recently-edited records a searcher is most
+                // likely to want (#4674). Typed rather than `any` so the
+                // contract rejects the wrong key at the call site.
+                const opts: EngineQueryOptions = {
                     where,
                     limit: perObject,
-                    orderBy: [{ field: 'updated_at', direction: 'desc' }],
+                    orderBy: [{ field: 'updated_at', order: 'desc' }],
                 };
                 if (request.context !== undefined) opts.context = request.context;
 
diff --git a/packages/objectql/src/protocol-publish-rollback.test.ts b/packages/objectql/src/protocol-publish-rollback.test.ts
@@ -82,12 +82,21 @@ function makeStubEngine() {
         async find(table: string, opts: { where: Record<string, unknown> }) {
             if (table === 'sys_metadata_history') {
                 const out = historyRows.filter((h) => matchesHistory(h, opts.where));
-                if (opts && (opts as any).orderBy) {
-                    const { field, direction } = (opts as any).orderBy;
+                // QueryAST shape: `orderBy` is an ARRAY of `{ field, order }`
+                // (SortNodeSchema). This double used to destructure
+                // `{ field, direction }` off the array itself, so both names
+                // read `undefined` — it spoke the `direction` vocabulary the
+                // engine does not read (#4674) AND, because an array has no
+                // `.field`, sorted nothing at all. Either way a test built on
+                // it would have ratified the broken behaviour.
+                const orderBy = (opts as any).orderBy;
+                if (Array.isArray(orderBy) && orderBy.length > 0) {
                     out.sort((a: any, b: any) => {
-                        const av = a[field]; const bv = b[field];
-                        if (av < bv) return direction === 'desc' ? 1 : -1;
-                        if (av > bv) return direction === 'desc' ? -1 : 1;
+                        for (const { field, order } of orderBy) {
+                            const av = a[field]; const bv = b[field];
+                            if (av < bv) return order === 'desc' ? 1 : -1;
+                            if (av > bv) return order === 'desc' ? -1 : 1;
+                        }
                         return 0;
                     });
                 }
