From 0183be7dfa14c36ed0288f077752aafbb417f73c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 21:30:21 +0000 Subject: [PATCH] fix(metadata-protocol): sort audit history and global search by `order`, not `direction` (#4674) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01NKcGqCYCCpMkB5UW8jNPXx --- .changeset/orderby-direction-vocabulary.md | 14 ++ .../src/protocol.orderby-vocabulary.test.ts | 132 ++++++++++++++++++ packages/metadata-protocol/src/protocol.ts | 24 +++- .../src/protocol-publish-rollback.test.ts | 19 ++- 4 files changed, 179 insertions(+), 10 deletions(-) create mode 100644 .changeset/orderby-direction-vocabulary.md create mode 100644 packages/metadata-protocol/src/protocol.orderby-vocabulary.test.ts diff --git a/.changeset/orderby-direction-vocabulary.md b/.changeset/orderby-direction-vocabulary.md new file mode 100644 index 0000000000..0e7fb47d50 --- /dev/null +++ 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 new file mode 100644 index 0000000000..c89d7ea2ee --- /dev/null +++ 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) { + 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 index ef69f1b65a..66dfc6361a 100644 --- 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 index d2d25a351d..770bc654d0 100644 --- 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 }) { 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; }); }