From 73a0fb34d989d6113559269d5f493164a61d15d8 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Tue, 28 Jul 2026 13:42:01 +0800 Subject: [PATCH] test(service-storage): give the attachment read-visibility harness real Filter Protocol semantics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `attachment-read-visibility.test.ts` faked its engine with a matcher that understood implicit equality and `$in` and nothing else — no `$and`, no `$or`, no `$not`. Every assertion could therefore only check the *shape* of the filter `computeParentVisibilityFilter` emits, never the rows that filter selects. That is a poor bargain for a predicate whose whole job is narrowing: it pairs a discriminator with an id list per branch, and anything that widens `$or` loosens that pairing while leaving shape assertions green. - The matcher now implements the protocol for real, mirroring driver-memory's `memory-matcher.ts` and formula's `matches-filter.ts`: every key in a filter object ANDs, `$or` ORs its branches, a branch's own contents still AND. Unimplemented operators now throw rather than being ignored — a silently under-implemented test double is the failure mode this change exists to close. - New case asserts the ROWS a multi-parent-type scope returns: rows whose discriminator matches a branch but whose id is absent from that branch's id list — including one borrowing the sibling branch's id — are excluded, pinning the per-branch pairing itself. - A conformance block pins the harness against the same 2x2 fixture and expectations as `memory-matcher-or-semantics.test.ts`, `matches-filter-or-semantics.test.ts` and `sql-driver-or-filter.test.ts`, so the four cannot drift apart, plus a case proving the matcher tells a correctly paired scope from a widened one. Test-only; no runtime change. service-storage: 212/212 green. Co-Authored-By: Claude --- ...t-read-visibility-real-filter-semantics.md | 30 +++ .../src/attachment-read-visibility.test.ts | 219 +++++++++++++++++- 2 files changed, 238 insertions(+), 11 deletions(-) create mode 100644 .changeset/attachment-read-visibility-real-filter-semantics.md diff --git a/.changeset/attachment-read-visibility-real-filter-semantics.md b/.changeset/attachment-read-visibility-real-filter-semantics.md new file mode 100644 index 0000000000..48236c0264 --- /dev/null +++ b/.changeset/attachment-read-visibility-real-filter-semantics.md @@ -0,0 +1,30 @@ +--- +--- + +test(service-storage): give the attachment read-visibility harness real Filter Protocol semantics + +Test-only — releases nothing. + +`attachment-read-visibility.test.ts` faked its engine with a matcher that +understood implicit equality and `$in` and nothing else: no `$and`, no `$or`, +no `$not`. Every assertion in the file could therefore only check the *shape* +of the filter `computeParentVisibilityFilter` emits, never the rows that filter +selects — a poor bargain for a predicate whose whole job is narrowing. + +Changes: + +- The harness matcher now implements the protocol for real, mirroring + `driver-memory/memory-matcher.ts` and `formula/matches-filter.ts`: every key + in a filter object ANDs, `$or` ORs its branches, and a branch's own contents + still AND. Operators it does not implement now **throw** instead of being + ignored — a silently under-implemented test double is the failure mode this + change exists to close. +- A new case asserts the **rows** a multi-parent-type scope returns, not just + its shape: rows whose discriminator matches a branch but whose id is absent + from that branch's id list (including one that borrows the sibling branch's + id) are excluded, so the per-branch pairing itself is pinned. +- A conformance block pins the harness matcher against the same 2x2 fixture and + expectations as `memory-matcher-or-semantics.test.ts`, + `matches-filter-or-semantics.test.ts` and `sql-driver-or-filter.test.ts`, so + the four cannot drift apart, plus a case proving the matcher actually + distinguishes a correctly paired scope from a widened one. diff --git a/packages/services/service-storage/src/attachment-read-visibility.test.ts b/packages/services/service-storage/src/attachment-read-visibility.test.ts index bb495fcc9c..3cfdd42ded 100644 --- a/packages/services/service-storage/src/attachment-read-visibility.test.ts +++ b/packages/services/service-storage/src/attachment-read-visibility.test.ts @@ -6,6 +6,77 @@ import type { AttachmentLifecycleEngine, AttachmentReadMiddlewareCtx } from './a const silentLogger = () => ({ info: vi.fn(), warn: vi.fn(), debug: vi.fn() }); +/** + * Filter Protocol evaluator for the fake engine below — real `$and` / `$or` / + * `$not` semantics, not a shape check. + * + * Mirrors `driver-memory`'s `memory-matcher.ts` and `formula`'s + * `matches-filter.ts`: **every key inside one filter object ANDs**, a `$or` + * array ORs its branches, and a branch's own contents still AND. + * + * This used to understand neither logical operators nor anything but `$in`, + * so these tests could only assert the *shape* of the filter the middleware + * emits, never the rows that filter selects. That is a poor bargain for a + * predicate whose whole job is narrowing: `computeParentVisibilityFilter` + * pairs a discriminator with an id list per branch, and any bug that widens + * `$or` — the class #3774 fixed in the SQL compiler — turns that pairing into + * something looser while leaving every shape assertion green. A fake that + * silently under-implements the protocol cannot see a widening bug, so this + * one implements it for real and **throws** on anything it does not, rather + * than quietly matching. + * + * Values compare as strings on purpose: `computeParentVisibilityFilter` + * stringifies every `parent_id` when it builds the `$in` list, so a numeric + * row value still has to match its stringified filter value. + */ +function matchWhere(row: any, where: any): boolean { + if (where === null || where === undefined) return true; + if (typeof where !== 'object' || Array.isArray(where)) { + throw new Error(`harness matcher: a filter must be an object, got ${JSON.stringify(where)}`); + } + // A filter object is the AND of every one of its entries. + return Object.entries(where).every(([key, val]) => { + if (key === '$and') { + if (!Array.isArray(val)) throw new Error('harness matcher: $and expects an array'); + return val.every((branch) => matchWhere(row, branch)); + } + if (key === '$or') { + if (!Array.isArray(val)) throw new Error('harness matcher: $or expects an array'); + // Each branch is evaluated whole — the keys inside it still AND. + return val.some((branch) => matchWhere(row, branch)); + } + if (key === '$not') return !matchWhere(row, val); + if (key.startsWith('$')) { + throw new Error(`harness matcher: unsupported logical operator ${key}`); + } + return matchField(row[key], val); + }); +} + +/** One `field: ` entry. Multiple operators on the same field AND together. */ +function matchField(actual: any, spec: any): boolean { + if (spec === null || spec === undefined) return actual === null || actual === undefined; + if (Array.isArray(spec)) { + throw new Error('harness matcher: a bare array is not a field spec — use { $in: [...] }'); + } + if (typeof spec !== 'object') return String(actual) === String(spec); + return Object.entries(spec).every(([op, target]) => { + switch (op) { + case '$eq': + return String(actual) === String(target); + case '$ne': + return String(actual) !== String(target); + case '$in': + return (target as unknown[]).map(String).includes(String(actual)); + case '$nin': + return !(target as unknown[]).map(String).includes(String(actual)); + default: + // Loud on purpose — an ignored operator is how the original blind spot happened. + throw new Error(`harness matcher: unsupported operator ${op} — implement it, don't let it match silently`); + } + }); +} + /** * Fake engine modelling: a sys_attachment table (system pre-scan) + a * per-parent-object visibility map keyed by userId. A parent find under a @@ -19,16 +90,6 @@ function install(opts: { let mw!: (ctx: AttachmentReadMiddlewareCtx, next: () => Promise) => Promise; const calls = { parentFinds: [] as Array<{ object: string; ids: string[]; userId?: string }> }; - const matchWhere = (row: any, where: any): boolean => { - if (!where || typeof where !== 'object') return true; - return Object.entries(where).every(([k, v]) => { - if (v && typeof v === 'object' && Array.isArray((v as any).$in)) { - return (v as any).$in.map(String).includes(String(row[k])); - } - return String(row[k]) === String(v); - }); - }; - const engine: AttachmentLifecycleEngine = { registerHook: () => {}, registerMiddleware: (fn) => { @@ -51,7 +112,18 @@ function install(opts: { update: async () => ({}), }; installAttachmentReadVisibility(engine, silentLogger()); - return { mw, calls }; + return { + mw, + calls, + /** + * Ids the given `where` actually selects from the fixture — i.e. what a + * spec-conformant driver would return once the middleware has narrowed + * the query. Asserting on this (not just on the filter's shape) is what + * makes a widened scope visible. + */ + selectIds: (where: unknown): string[] => + opts.attachments.filter((r) => matchWhere(r, where)).map((r) => String(r.id)), + }; } /** Drive a read op through the middleware and return the resulting where. */ @@ -111,6 +183,34 @@ describe('installAttachmentReadVisibility', () => { }); }); + it('the multi-parent $or admits only rows matching BOTH keys of one branch', async () => { + // Each parent type has a visible and an invisible record, plus a row whose + // parent_object belongs to one branch while its parent_id appears only in + // the OTHER branch's id list. Nothing but a genuine per-branch AND excludes + // all three, so this pins the pairing itself rather than the filter's shape. + const { mw, selectIds } = install({ + attachments: [ + { id: 'a1', parent_object: 'att_case', parent_id: 'c1' }, // visible case + { id: 'a2', parent_object: 'att_case', parent_id: 'c2' }, // case u1 cannot read + { id: 'a3', parent_object: 'att_todo', parent_id: 't1' }, // visible todo + { id: 'a4', parent_object: 'att_todo', parent_id: 't2' }, // todo u1 cannot read + { id: 'a5', parent_object: 'att_case', parent_id: 't1' }, // object of branch 1, id of branch 2 + ], + visible: { att_case: { u1: ['c1'] }, att_todo: { u1: ['t1'] } }, + }); + const { where } = await runRead(mw, {}); + + expect(where).toEqual({ + $or: [ + { parent_object: 'att_case', parent_id: { $in: ['c1'] } }, + { parent_object: 'att_todo', parent_id: { $in: ['t1'] } }, + ], + }); + // The rows that filter actually returns — a2/a4 (right parent type, wrong + // id) and a5 (right type, an id borrowed from the sibling branch) are out. + expect(selectIds(where)).toEqual(['a1', 'a3']); + }); + it('denies all when no candidate parent is visible', async () => { const { mw } = install({ attachments: [{ id: 'a3', parent_object: 'att_secret', parent_id: 's1' }], @@ -178,3 +278,100 @@ describe('installAttachmentReadVisibility', () => { expect(calls.parentFinds).toHaveLength(0); }); }); + +/** + * Who tests the test double? The row assertions above are only worth as much + * as the matcher evaluating them, so it gets the same 2x2 fixture and the same + * expectations as the three production backends: + * `driver-memory/memory-matcher-or-semantics.test.ts`, + * `formula/matches-filter-or-semantics.test.ts`, and + * `driver-sql/sql-driver-or-filter.test.ts`. If this harness ever drifts from + * them, a read scope it declares safe would not be safe in the engine. + */ +describe('harness matcher — Filter Protocol $and/$or/$not semantics', () => { + const ROWS = [ + { id: '1', a: 'x', b: 'y', c: 'z' }, + { id: '2', a: 'x', b: 'zz', c: 'z' }, + { id: '3', a: 'qq', b: 'y', c: 'z' }, + { id: '4', a: 'qq', b: 'zz', c: 'z' }, + ]; + const ids = (filter: any): string[] => ROWS.filter((r) => matchWhere(r, filter)).map((r) => r.id); + + it('ANDs the keys of a single multi-key branch', () => { + expect(ids({ $or: [{ a: 'x', b: 'y' }] })).toEqual(['1']); + }); + + it('ANDs the keys of each branch independently', () => { + expect(ids({ $or: [{ a: 'x', b: 'y' }, { a: 'qq', b: 'zz' }] })).toEqual(['1', '4']); + }); + + it('ANDs operator-object keys within a branch', () => { + expect(ids({ $or: [{ a: { $eq: 'x' }, b: { $ne: 'zz' } }] })).toEqual(['1']); + }); + + it('ANDs keys inside a $or nested in a $or branch', () => { + expect(ids({ $or: [{ $or: [{ a: 'x', b: 'zz' }] }] })).toEqual(['2']); + }); + + it('ANDs multiple operators on ONE field within a branch', () => { + expect(ids({ $or: [{ a: { $ne: 'qq', $eq: 'x' }, b: 'y' }] })).toEqual(['1']); + }); + + it('OR-s a $and branch against a sibling multi-key branch', () => { + expect(ids({ $or: [{ $and: [{ a: 'x' }, { b: 'y' }] }, { a: 'qq', b: 'zz' }] })).toEqual(['1', '4']); + }); + + it('ANDs a $and with a sibling key in the same branch, either order', () => { + expect(ids({ $or: [{ c: 'nope' }, { $and: [{ a: 'qq' }], b: 'y' }] })).toEqual(['3']); + expect(ids({ $or: [{ c: 'nope' }, { b: 'y', $and: [{ a: 'qq' }] }] })).toEqual(['3']); + }); + + it('keeps single-key $or branches as a plain OR', () => { + expect(ids({ $or: [{ a: 'x' }, { b: 'y' }] })).toEqual(['1', '2', '3']); + }); + + it('ANDs a $or with a sibling top-level field key', () => { + expect(ids({ $or: [{ a: 'x' }, { b: 'y' }], b: 'zz' })).toEqual(['2']); + }); + + it('negates with $not', () => { + expect(ids({ $not: { a: 'x' } })).toEqual(['3', '4']); + }); + + it('tells a correctly paired scope apart from a widened one', () => { + // The proof this matcher can go red: the same clauses, once AND-ed per + // branch and once flattened to key-level ORs. If both returned the same + // rows, every row assertion in this file would be decorative. + const attachments = [ + { id: 'a1', parent_object: 'att_case', parent_id: 'c1' }, + { id: 'a2', parent_object: 'att_case', parent_id: 'c2' }, + { id: 'a3', parent_object: 'att_todo', parent_id: 't1' }, + { id: 'a4', parent_object: 'att_todo', parent_id: 't2' }, + ]; + const pick = (f: any) => attachments.filter((r) => matchWhere(r, f)).map((r) => r.id); + + const correct = { + $or: [ + { parent_object: 'att_case', parent_id: { $in: ['c1'] } }, + { parent_object: 'att_todo', parent_id: { $in: ['t1'] } }, + ], + }; + const widened = { + $or: [ + { parent_object: 'att_case' }, + { parent_id: { $in: ['c1'] } }, + { parent_object: 'att_todo' }, + { parent_id: { $in: ['t1'] } }, + ], + }; + expect(pick(correct)).toEqual(['a1', 'a3']); + expect(pick(widened)).toEqual(['a1', 'a2', 'a3', 'a4']); // every row in the tenant + }); + + it('throws instead of silently ignoring an operator it does not implement', () => { + // The original blind spot in one line: an unimplemented operator must not + // quietly evaluate to a match. + expect(() => ids({ a: { $gt: 'x' } })).toThrow(/unsupported operator \$gt/); + expect(() => ids({ $nor: [{ a: 'x' }] })).toThrow(/unsupported logical operator \$nor/); + }); +});