perf(security,protocol): stop asking the same question twice within one request (#10824)

* perf(security,protocol): stop asking the same question twice per request

Refs #10757

* test(security,protocol): pin the dedupe and its invalidation; changeset

Refs #10757

---------

Co-authored-by: Jack Zhuang <zhuangjianguo@gmail.com>

Author: os-zhuang

.changeset/count-opt-out-and-permission-set-memo.md

@@ -0,0 +1,57 @@
+---
+"@objectstack/metadata-protocol": minor
+"@objectstack/plugin-security": patch
+---
+
+Stop issuing two DB queries for questions already answered earlier in the same
+request (#10757). One authenticated `GET /data/:object?$top=1` measured **24 DB
+queries before, 23 after** — **22** when the caller opts out of the count.
+Measured with `X-OS-Debug-Timing: json` on `pnpm dev:crm`, whose `Server-Timing`
+carries `db;dur=…;desc="N queries"`.
+
+**`$count=false` now skips the COUNT query** (`@objectstack/metadata-protocol`).
+The parameter has been declared (`ODataQuerySchema.$count`), aliased on the wire
+(`$count` → `count`), reserved out of the implicit-field-filter bucket,
+arity-checked and boolean-coerced for a long time — and then deleted unread, so
+every paginated list ran `engine.count()` whether or not the caller wanted a
+total. It is honoured now:
+
+```
+GET /data/task?$top=25              → { records, total, hasMore }   (unchanged)
+GET /data/task?$top=25&$count=true  → { records, total, hasMore }   (unchanged)
+GET /data/task?$top=25&$count=false → { records, hasMore }          (no COUNT query)
+```
+
+Read the shape of that carefully before adopting it:
+
+- **Only an explicit `false` opts out.** An ABSENT `$count` still counts and
+  still reports `total`. OData reads absent as "omit the count", and taking that
+  reading here would silently strip `total` from every existing caller — none of
+  them send the parameter, all of them read the number. The asymmetry is
+  deliberate and pinned by tests.
+- **`total` is OMITTED, never estimated.** `FindDataResponse.total` is declared
+  optional ("if requested"), so absent is the declared shape for "not
+  requested". A caller that opted out and then reads `total` gets `undefined`,
+  not a plausible-looking guess — guard the read (`total ?? undefined`) or do
+  not send `$count=false`.
+- **`hasMore` is still answered**, from the page alone: a full page means there
+  may be more. Same page-local rule the `$search` path already uses.
+
+**A find and its COUNT resolve permission sets once, not twice**
+(`@objectstack/plugin-security`). `findData` answers a paginated list with two
+engine operations, and the security middleware runs on both; each pass re-read
+`sys_permission_set` for the same context with identical bindings. The
+resolution is now memoized per execution context — a `WeakMap` keyed on the
+context object, which is built once per request and collected with it, so
+nothing outlives the caller it was resolved for — and **retired by any write**:
+a process-wide epoch is bumped on every `insert`/`update`/`delete` the engine
+middleware sees, ahead of the `isSystem` bypass so a seeder, a package publish
+or an auto-org-admin grant invalidates too. A context whose grants are rewritten
+in place re-resolves as well (the memo key covers `positions`, `permissions`,
+`principalKind` and the presence of `userId`). No authorization answer is reused
+across a write, across a context, or across a request.
+
+Not a fix for the whole cost: the remaining ~22 queries per authenticated
+request are session resolution, grant resolution, localization and metadata
+reads that repeat on every request. Removing those needs cross-request caching
+with an invalidation design, which is deliberately not in this change.

packages/metadata-protocol/src/protocol.count-opt-out.test.ts

@@ -0,0 +1,168 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * #10757 — `$count=false` skips the COUNT query.
+ *
+ * ## What was wrong
+ *
+ * `$count` has been a fully-plumbed parameter for a long time: declared in the
+ * spec (`ODataQuerySchema.$count`, `packages/spec/src/api/odata.zod.ts`),
+ * aliased on the wire (`$count` → `count`, `WIRE_DOLLAR_ALIASES`), reserved out
+ * of the implicit-field-filter bucket (`RESERVED_LIST_QUERY_PARAMS`),
+ * arity-checked (`protocol.query-param-arity.test.ts`) and boolean-coerced —
+ * and then DELETED unread by the protocol-key strip in `findData`. So every
+ * paginated list issued `engine.count()` whether or not the caller wanted a
+ * `total`, which on a remote database is a whole round trip per request. The
+ * measured trace on a real stack put it at query 24 of 24 for one
+ * `GET /data/:object?$top=1`.
+ *
+ * ## The two directions this suite pins, and why both are needed
+ *
+ *  1. **The OPT-OUT works** — an explicit `false` (either spelling) means no
+ *     `engine.count()` call and no `total` key. `expect(count).not.toHaveBeenCalled()`
+ *     is the load-bearing assertion; asserting only the absent `total` would
+ *     stay green if a future edit ran the query and merely dropped the number,
+ *     which is the whole cost with none of the saving.
+ *
+ *  2. **Nothing else changed** — absent `$count`, and explicit `$count=true`,
+ *     both still count and still report `total`. This is the direction that
+ *     makes the opt-out safe to ship: OData reads an ABSENT `$count` as "omit
+ *     the count", and taking that reading here would silently strip `total`
+ *     from every existing caller (none of them send the parameter, all of them
+ *     read the number). The asymmetry is deliberate, so it is pinned rather
+ *     than left to be "tidied up" later.
+ *
+ * `total` is OMITTED rather than estimated — `FindDataResponseSchema` declares
+ * it optional ("if requested"), and a page-local guess handed back to a caller
+ * who declined the real number is how an estimate ends up rendered as a record
+ * count. `hasMore` is still answered from the page alone.
+ */
+
+import { describe, it, expect, vi } from 'vitest';
+import { ObjectStackProtocolImplementation } from './protocol.js';
+
+const SCHEMA = {
+    name: 'invoice',
+    nameField: 'name',
+    fields: {
+        name: { name: 'name', type: 'text' },
+        status: { name: 'status', type: 'text' },
+    },
+};
+
+function makeProtocol(pageSize: number) {
+    const find = vi.fn(async () => Array.from({ length: pageSize }, (_, i) => ({ id: `r${i}` })));
+    const count = vi.fn(async () => 3125);
+    const engine = {
+        registry: { getObject: (n: string) => (n === 'invoice' ? SCHEMA : undefined) },
+        find,
+        count,
+        aggregate: vi.fn(async () => [] as unknown[]),
+    };
+    return { p: new ObjectStackProtocolImplementation(engine as any), find, count };
+}
+
+describe('[#10757] findData honours $count=false', () => {
+    describe('opt-out — the COUNT query is not issued', () => {
+        // Both wire spellings reach the same normalized `count` slot; a fix that
+        // read only one of them would leave the other paying for the query.
+        for (const spelling of ['$count', 'count'] as const) {
+            it(`?${spelling}=false skips engine.count() and omits total`, async () => {
+                const { p, count } = makeProtocol(1);
+
+                const result = await p.findData({
+                    object: 'invoice',
+                    query: { $top: 1, [spelling]: 'false' },
+                } as never);
+
+                expect(count).not.toHaveBeenCalled();
+                expect('total' in (result as object)).toBe(false);
+            });
+        }
+
+        it('accepts the already-boolean form a POST body carries', async () => {
+            const { p, count } = makeProtocol(1);
+
+            const result = await p.findData({
+                object: 'invoice',
+                query: { $top: 1, count: false },
+            } as never);
+
+            expect(count).not.toHaveBeenCalled();
+            expect('total' in (result as object)).toBe(false);
+        });
+
+        it('still answers hasMore from the page: a FULL page means there may be more', async () => {
+            const { p } = makeProtocol(10);
+
+            const result = await p.findData({
+                object: 'invoice',
+                query: { $top: 10, $count: 'false' },
+            } as never);
+
+            expect(result.hasMore).toBe(true);
+        });
+
+        it('…and a SHORT page means there are not', async () => {
+            const { p } = makeProtocol(3);
+
+            const result = await p.findData({
+                object: 'invoice',
+                query: { $top: 10, $count: 'false' },
+            } as never);
+
+            expect(result.hasMore).toBe(false);
+        });
+
+        it('leaves `count` off the engine option bag (it is a protocol-layer flag)', async () => {
+            const { p, find } = makeProtocol(1);
+
+            await p.findData({ object: 'invoice', query: { $top: 1, $count: 'false' } } as never);
+
+            const bag = (find.mock.calls[0] as unknown[])[1] as Record<string, unknown>;
+            expect('count' in bag).toBe(false);
+            expect('$count' in bag).toBe(false);
+        });
+    });
+
+    describe('unchanged for every caller that does not opt out', () => {
+        it('an ABSENT $count still counts and still reports total', async () => {
+            const { p, count } = makeProtocol(1);
+
+            const result = await p.findData({ object: 'invoice', query: { $top: 1 } } as never);
+
+            expect(count).toHaveBeenCalledTimes(1);
+            expect(result.total).toBe(3125);
+            expect(result.hasMore).toBe(true);
+        });
+
+        it('an explicit $count=true still counts and still reports total', async () => {
+            const { p, count } = makeProtocol(1);
+
+            const result = await p.findData({
+                object: 'invoice',
+                query: { $top: 1, $count: 'true' },
+            } as never);
+
+            expect(count).toHaveBeenCalledTimes(1);
+            expect(result.total).toBe(3125);
+        });
+
+        it('$count=false without a limit is a no-op — the full set is already the total', async () => {
+            // No `limit` ⇒ the whole result set came back, so `records.length` IS
+            // the total and `engine.count()` was never called even before #10757.
+            // Pinned so the opt-out cannot accidentally start suppressing a total
+            // that costs nothing.
+            const { p, count } = makeProtocol(4);
+
+            const result = await p.findData({
+                object: 'invoice',
+                query: { $count: 'false' },
+            } as never);
+
+            expect(count).not.toHaveBeenCalled();
+            expect(result.total).toBe(4);
+            expect(result.hasMore).toBe(false);
+        });
+    });
+});

packages/metadata-protocol/src/protocol.ts

@@ -8881,7 +8881,8 @@ export class ObjectStackProtocolImplementation implements
         //   ride the spread into the engine AST and OVERRIDE the resolved
         //   object, splitting `ast.object` from the table actually queried —
         //   a mismatch is refused, never resolved by picking a winner.
-        // - `count`: a response-shape flag this method consumed above.
+        // - `count`: a response-shape flag this method consumed above (and,
+        //   since #10757, one it actually HONOURS — see `countOptOut` below).
         // - QueryAST tombstones (`cursor`/`joins`/`windowFunctions`/
         //   `distinct`): reserved at the wire gate so they are not read as
         //   field filters; on the wire they stay ignored-with-tombstone-docs
@@ -8899,6 +8900,22 @@ export class ObjectStackProtocolImplementation implements
             err.code = 'QUERY_OBJECT_MISMATCH';
             throw err;
         }
+        // [#10757] `$count=false` — read the flag BEFORE the strip below deletes
+        // it, because the strip is what kept it from ever being honoured: the
+        // parameter has been declared (`ODataQuerySchema.$count`,
+        // `packages/spec/src/api/odata.zod.ts`), aliased (`$count` → `count`,
+        // {@link WIRE_DOLLAR_ALIASES}), reserved from the implicit-field-filter
+        // bucket ({@link RESERVED_LIST_QUERY_PARAMS}), arity-checked and boolean-
+        // coerced — and then deleted unread, so every list request paid for the
+        // COUNT query below whether or not the caller wanted a `total`.
+        //
+        // Only an EXPLICIT `false` opts out. An absent `$count` keeps today's
+        // behaviour (count runs, `total` is reported) rather than taking OData's
+        // "absent means omit" reading: every existing caller sends nothing and
+        // reads `total`, so the OData default would silently break all of them.
+        // The parameter is therefore an opt-OUT here, and that asymmetry is
+        // deliberate — see the changeset for the wording that ships to consumers.
+        const countOptOut = options.count === false;
         for (const k of ['object', 'count', 'joins', 'windowFunctions', 'cursor', 'distinct', 'having']) {
             delete options[k];
         }
@@ -8915,26 +8932,55 @@ export class ObjectStackProtocolImplementation implements
         // reporting a wrong total.
         const pageLimit = typeof options.limit === 'number' && options.limit > 0 ? options.limit : undefined;
         const pageOffset = typeof options.offset === 'number' && options.offset > 0 ? options.offset : 0;
-        let total = records.length;
+        let total: number | undefined = records.length;
         let hasMore = false;
         if (pageLimit !== undefined) {
             // `distinct` used to suppress the count here too — #4286 finding 2:
             // the flag's ONLY observable effect platform-wide, on a capability
             // that never deduplicated a row. Removed with `query.distinct`
             // (tombstoned in spec 17); `total`/`hasMore` are truthful again.
             const countable = options.search == null;
-            if (countable) {
+            if (countOptOut) {
+                // [#10757] The caller said it does not need `total`, so the
+                // COUNT query is not issued at all — that is the whole point of
+                // the parameter, and on a remote database it is a full round
+                // trip saved per list request.
+                //
+                // `total` is OMITTED rather than estimated. `FindDataResponse`
+                // declares it optional ("Total number of records matching the
+                // filter (IF REQUESTED)",
+                // `packages/spec/src/api/protocol.zod.ts`), so absent is the
+                // declared shape for "not requested" — and it is the only
+                // honest one: the `search` branch below reports an estimate
+                // because it has no better number to give, while here a real
+                // number was available and the caller declined it. Handing back
+                // a plausible-looking guess under those circumstances is how a
+                // page-local estimate ends up rendered as a record count.
+                //
+                // `hasMore` is still answered, from the page alone: a FULL page
+                // means there may be more. Same page-local rule the search
+                // branch uses, and it never over-reports the data — it can only
+                // say "maybe more" on an exactly-full last page.
+                hasMore = records.length === pageLimit;
+                total = undefined;
+            } else if (countable) {
+                // [#10757] `counted` is a separate, always-assigned local so
+                // `hasMore` below compares against a `number`: `total` became
+                // optional when `$count=false` gained the right to omit it, and
+                // TypeScript cannot narrow it back across the try/catch.
+                let counted: number;
                 try {
-                    total = await this.engine.count(request.object, {
+                    counted = await this.engine.count(request.object, {
                         where: options.where,
                         context: options.context,
                     } as any);
                 } catch {
                     // engine.count() has its own find().length fallback; if it still
                     // throws, degrade to a page-local total rather than failing the list.
-                    total = pageOffset + records.length;
+                    counted = pageOffset + records.length;
                 }
-                hasMore = pageOffset + records.length < total;
+                total = counted;
+                hasMore = pageOffset + records.length < counted;
             } else {
                 hasMore = records.length === pageLimit;
                 total = pageOffset + records.length + (hasMore ? 1 : 0);
@@ -8943,7 +8989,11 @@ export class ObjectStackProtocolImplementation implements
         return {
             object: request.object,
             records,
-            total,
+            // [#10757] Omitted, not `undefined`-valued: a JSON body carrying
+            // `"total": null` (or a key some serializers keep) reads as "the
+            // total is nothing", which is a different claim from "no total was
+            // requested". The key is simply absent.
+            ...(total === undefined ? {} : { total }),
             hasMore,
         };
     }