From 1331e03e2602c9031590e01f13849119490abe54 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 07:35:37 +0000 Subject: [PATCH 1/2] fix(objectql,driver-mongodb)!: findOne must say which record it wants, and executes every option it declares (#4419) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit findOne reads a single row, which makes its predicate the only thing between the caller and an arbitrary record. When the predicate is missing the result is not `null` — it is the object's FIRST ROW: a real, plausible-looking record with nothing to do with the request, which the `if (!row)` check every call site already has cannot catch, and which then propagates into whatever is computed next. A throw would have been caught in development; a `null` would have been caught by the null-check. A valid-looking wrong record defeats both. #4419 reported this against the `filter` key, which #4346 (fold on every entry point) and #4400 (unknown keys throw) already closed. This is what those left standing. BREAKING: findOne refuses a query that selects nothing in particular. findOne(o) / findOne(o, {}) / findOne(o, { where: {} }) -> findOne(o, { where: … }) the record matching this -> findOne(o, { search: 'Acme' }) the record this search finds -> findOne(o, { orderBy: [{ field, order }] }) the FIRST in this order -> find(o, { limit: 1 }) any row will do, said aloud The error names all four. `find` and `count` are unchanged — returning or counting every row is an honest answer; only findOne's implicit "just one of them" turns a missing predicate into a confidently wrong record. The guard reads the CALLER's predicate, before RLS/sharing middleware injects its own: a tenant filter narrows which rows are visible, it does not make "whichever comes first" something the caller asked for. Two silent drops that produced the same wrong record are fixed with it: - findOne({ search }) now applies the search. The ADR-0061 `search` -> cross-field $contains expansion lived inline in find() and nowhere else, while find and findOne are checked against the SAME legal-key set — so `search` passed the gate, rode onto the AST, and reached a driver. No driver reads ast.search. The read ran with no predicate at all and limit: 1 did the rest. The expansion is now one method both call. - MongoDBDriver.findOne now applies orderBy, fields and offset. It translated query.where and dropped the rest, so findOne({ orderBy }) did not return the newest record — it returned whichever document the scan reached first. This one matters beyond Mongo: the guard above tells an unpredicated caller to reach for orderBy, and an escape hatch one backend ignores is not an escape hatch. No ordering is IMPOSED when the caller supplies none — both drivers keep that carve-out (#4363), and SqlDriver's comment citing Mongo as already in agreement is corrected, since it cited the dropped parameter as agreement. And a gate so the class does not come back: a drift pin walks ENGINE_OPTION_KEY_SETS.findOne and requires each declared key to have an observable effect — on the AST the driver receives, on the driver options, or in an explicit "not executed, and here is why" entry (only `limit`, which the contract's limit: 1 overrides). `search` sat declared-but-unexecuted through two rounds of hardening because nothing asked that question. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Uy3zSSDGcJybydLSwGypFS --- .changeset/findone-requires-a-predicate.md | 70 +++ content/docs/releases/v17.mdx | 44 ++ .../src/engine-findone-contract.test.ts | 399 ++++++++++++++++++ packages/objectql/src/engine.test.ts | 9 +- packages/objectql/src/engine.ts | 152 +++++-- .../driver-mongodb/src/mongodb-driver.ts | 67 ++- .../src/mongodb-findone-options.test.ts | 130 ++++++ .../src/mongodb-findone-query.test.ts | 151 +++++++ packages/plugins/driver-sql/src/sql-driver.ts | 9 +- packages/spec/src/contracts/data-engine.ts | 14 + .../objectstack-data/references/data-hooks.md | 21 +- 11 files changed, 1019 insertions(+), 47 deletions(-) create mode 100644 .changeset/findone-requires-a-predicate.md create mode 100644 packages/objectql/src/engine-findone-contract.test.ts create mode 100644 packages/plugins/driver-mongodb/src/mongodb-findone-options.test.ts create mode 100644 packages/plugins/driver-mongodb/src/mongodb-findone-query.test.ts diff --git a/.changeset/findone-requires-a-predicate.md b/.changeset/findone-requires-a-predicate.md new file mode 100644 index 0000000000..826c8f8500 --- /dev/null +++ b/.changeset/findone-requires-a-predicate.md @@ -0,0 +1,70 @@ +--- +"@objectstack/objectql": major +"@objectstack/spec": patch +"@objectstack/driver-mongodb": patch +"@objectstack/driver-sql": patch +--- + +fix(objectql,driver-mongodb)!: `findOne` must say which record it wants, and executes every option it declares (#4419) + +`findOne` reads a single row, which makes its predicate the only thing between +the caller and *an arbitrary record*. When the predicate is missing the result is +not `null` — it is the object's **first row**: a real, plausible-looking record +with nothing to do with the request, which the `if (!row)` check every call site +already has cannot catch, and which then propagates into whatever is computed +next. Reported downstream: line items defaulting their price from the first +product in the catalog rather than the selected one, and "is this deal already +closed?" answered against an unrelated record while the write that followed +correctly targeted the intended id. A throw would have been caught in +development; a `null` would have been caught by the null-check. A valid-looking +wrong record defeats both. + +**Breaking — `findOne` now refuses a query that selects nothing in particular.** + +FROM → TO: + +| Was | Now write | Meaning | +|---|---|---| +| `findOne(o)`, `findOne(o, {})`, `findOne(o, { where: {} })` | `findOne(o, { where: … })` | the record matching this predicate | +| | `findOne(o, { search: 'Acme' })` | the record this search finds | +| | `findOne(o, { orderBy: [{ field: 'created_at', order: 'desc' }] })` | the FIRST record in this order — the newest | +| | `find(o, { limit: 1 })` | any row will genuinely do, said at the call site | + +One-line fix: add the `where` you meant, or `orderBy` if you meant "the newest +one", or switch to `find(o, { limit: 1 })` if any row will do. The error names +all four. `find` and `count` are unchanged — returning or counting every row is +an honest answer; only `findOne`'s implicit "just one of them" turns a missing +predicate into a confidently wrong record. The guard reads the CALLER's +predicate, before RLS/sharing middleware injects its own: a tenant filter +narrows which rows are visible, it does not make "whichever comes first" +something the caller asked for. + +**Two silent drops that produced the same wrong record are fixed with it.** + +- **`findOne({ search })` applies the search.** The ADR-0061 `search` → + cross-field `$contains` expansion lived inline in `find` and nowhere else, + while `find` and `findOne` are checked against the SAME legal-key set — so + `search` passed the gate, rode onto the AST, and reached a driver. No driver + reads `ast.search`. The read therefore ran with no predicate at all and + `limit: 1` did the rest. The expansion is now one method both call. +- **`MongoDBDriver.findOne` applies `orderBy`, `fields` and `offset`.** It + translated `query.where` and dropped the rest, so `findOne({ orderBy })` did + not return the newest record — it returned whichever document the scan reached + first. `find` and `_findStream` in the same driver had always handled all + three. This one matters beyond Mongo: the guard above tells an unpredicated + caller to reach for `orderBy`, and an escape hatch one backend ignores is not + an escape hatch. No ordering is IMPOSED when the caller supplies none — both + drivers keep that carve-out (#4363), and `SqlDriver`'s comment about Mongo + "never sorting" is corrected, since it cited the dropped parameter as + agreement. + +**And a gate so the class does not come back.** A drift pin walks +`ENGINE_OPTION_KEY_SETS.findOne` and requires each declared key to have an +observable effect — on the AST the driver receives, on the driver options, or in +an explicit "not executed, and here is why" entry (only `limit`, which the +contract's `limit: 1` overrides). `search` sat declared-but-unexecuted through +two rounds of hardening because nothing asked that question. + +Together with #4346 (`filter` → `where` folds on every entry point) and #4400 +(unknown option keys throw), a read parameter the engine does not execute now +fails at the call site instead of quietly changing the answer. diff --git a/content/docs/releases/v17.mdx b/content/docs/releases/v17.mdx index 3c3ac3da59..1674feba38 100644 --- a/content/docs/releases/v17.mdx +++ b/content/docs/releases/v17.mdx @@ -930,6 +930,50 @@ honoured — `updateManyData`, `deleteManyData` and `batchData` persisted regardless, so a caller sending it to *preview* a mutation got it executed. It is HTTP-only; stop sending it. +### `findOne` must say which record it wants (#4419) + +`findOne` reads a single row. That makes its predicate the only thing standing +between the caller and *an arbitrary record* — and when the predicate is missing +the result is not `null`, it is the object's **first row**: a real, +plausible-looking record with nothing to do with the request, which the +`if (!row)` check every call site already has cannot catch, and which then +propagates into whatever is computed next. Downstream of one such call, line +items defaulted their price from the first product in the catalog, and +"is this deal already closed?" was answered against an unrelated record while +the write that followed correctly targeted the intended id. + +So a query that selects nothing in particular is now **refused** rather than +answered. Say which record you want in one of three ways: + +| Instead of | Write | Meaning | +|---|---|---| +| `findOne(o)` / `findOne(o, {})` / `findOne(o, { where: {} })` | `findOne(o, { where: … })` | the record matching this predicate | +| | `findOne(o, { search: 'Acme' })` | the record this search finds | +| | `findOne(o, { orderBy: [{ field: 'created_at', order: 'desc' }] })` | the FIRST record in this order — the newest | +| | `find(o, { limit: 1 })` | any row will genuinely do — and the call site says so | + +The error names all four. `find` and `count` are unchanged: returning or counting +every row is an honest answer, and only `findOne`'s implicit "just one of them" +turns a missing predicate into a confidently wrong record. + +Two silent drops that produced the same wrong record are fixed with it: + +- **`findOne({ search })` now applies the search.** The ADR-0061 `search` → + cross-field `$contains` expansion ran in `find` only, while both methods are + checked against the same legal-key set — so `search` passed the gate, reached a + driver that does not read it, and the read came back unpredicated. The + expansion is now one function both call, and a drift pin requires every option + `findOne` declares to have an observable effect. +- **`MongoDBDriver.findOne` now applies `orderBy`, `fields` and `offset`.** It + translated `where` and dropped the rest, so "the newest record" returned + whichever document the scan reached first. No ordering is imposed when the + caller supplies none (#4363) — that part is unchanged, on both drivers. + +Together with the `filter` → `where` fold on every entry point and the +unknown-key rejection (both already in this release), a read parameter the engine +does not execute now fails at the call site instead of quietly changing the +answer. + ### Dead spec clusters removed **App shell (2026-06 liveness audit, #4001 app step).** `App.version`, diff --git a/packages/objectql/src/engine-findone-contract.test.ts b/packages/objectql/src/engine-findone-contract.test.ts new file mode 100644 index 0000000000..efc51ff604 --- /dev/null +++ b/packages/objectql/src/engine-findone-contract.test.ts @@ -0,0 +1,399 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4419 — `findOne` executes every option it declares, and refuses a query that + * selects no particular record. + * + * The reported failure: a read query carrying a predicate the engine does not + * execute is not rejected and not reported; the key is dropped and the query + * runs WITHOUT a predicate. On `findOne` the forced `limit: 1` then turns that + * into the object's FIRST ROW — a real, plausible-looking record unrelated to + * the request, which no caller's `if (!row)` can catch and which propagates + * into whatever is computed next. Downstream, one wrong key defaulted line-item + * prices from the first product in the catalog and evaluated "is this deal + * already closed?" against an unrelated record. + * + * `filter` — the key the issue was reported against — was closed by #4346 (fold + * on every entry point) and #4400 (unknown keys throw), both pinned in + * `engine-filter-alias.test.ts` / `engine-unknown-option.test.ts`. This suite + * covers what those left standing: + * + * 1. **`search` was the same bug under a different key.** `find()` expanded + * ADR-0061 `search` into `where`; `findOne()` did not — while both are + * checked against the SAME legal-key set, so `search` passed the gate, rode + * onto the AST, and reached a driver. No driver reads `ast.search`. So + * `findOne({ search })` returned the first row of the whole object. + * 2. **The empty predicate itself.** Even with every key folded and every + * unknown key refused, `findOne({})` still answered with an arbitrary row. + * It now throws, and names the three ways to be specific. + * 3. **The drift pin at the bottom** is the part that stops (1) recurring: it + * walks `ENGINE_OPTION_KEY_SETS.findOne` and requires each declared key to + * have an observable effect. `search` sat declared-but-unexecuted through + * two rounds of hardening because nothing asked that question. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL, ENGINE_OPTION_KEY_SETS } from './engine.js'; + +const account = { + name: 'crm_account', + label: 'Account', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', type: 'text' as const }, + industry: { name: 'industry', type: 'text' as const }, + annual_revenue: { name: 'annual_revenue', type: 'number' as const }, + owner: { name: 'owner', type: 'lookup' as const, reference: 'person' }, + }, +}; +const person = { + name: 'person', + label: 'Person', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', type: 'text' as const }, + }, +}; +interface SeenRead { ast: any; opts: any } + +/** Memory driver recording the AST and driver options of every read. */ +function makeRecordingDriver() { + const stores = new Map>>(); + const storeFor = (o: string) => { let s = stores.get(o); if (!s) { s = new Map(); stores.set(o, s); } return s; }; + const reads: SeenRead[] = []; + let nextId = 0; + const matches = (row: any, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k === '$and') return (v as any[]).every((w) => matches(row, w)); + if (k === '$or') return (v as any[]).some((w) => matches(row, w)); + if (k.startsWith('$')) continue; + if (v && typeof v === 'object' && '$contains' in (v as any)) { + const needle = String((v as any).$contains).toLowerCase(); + if (!String(row[k] ?? '').toLowerCase().includes(needle)) return false; + continue; + } + const exp = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (exp ?? null)) return false; + } + return true; + }; + const run = (o: string, ast: any) => { + let rows = Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + const ord = Array.isArray(ast?.orderBy) ? ast.orderBy : []; + if (ord.length > 0) { + rows = [...rows].sort((a: any, b: any) => { + for (const { field, order } of ord) { + const cmp = String(a?.[field] ?? '').localeCompare(String(b?.[field] ?? '')); + if (cmp !== 0) return order === 'desc' ? -cmp : cmp; + } + return 0; + }); + } + if (typeof ast?.offset === 'number' && ast.offset > 0) rows = rows.slice(ast.offset); + if (Array.isArray(ast?.fields) && ast.fields.length > 0) { + rows = rows.map((r) => Object.fromEntries(ast.fields.map((f: string) => [f, (r as any)[f]]))); + } + return typeof ast?.limit === 'number' && ast.limit > 0 ? rows.slice(0, ast.limit) : rows; + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find(o: string, ast: any, opts: any) { reads.push({ ast, opts }); return run(o, ast); }, + findStream() { throw new Error('ns'); }, + async findOne(o: string, ast: any, opts: any) { reads.push({ ast, opts }); return run(o, ast)[0] ?? null; }, + async create(o: string, data: Record) { + nextId += 1; const id = (data.id as string) ?? `r_${nextId}`; const row = { ...data, id }; storeFor(o).set(id, row); return row; + }, + async update(o: string, id: string, data: Record) { + const s = storeFor(o); const cur = s.get(id); if (!cur) throw new Error(`nf ${o}/${id}`); + const up = { ...cur, ...data, id }; s.set(id, up); return up; + }, + async delete(o: string, id: string) { return storeFor(o).delete(id); }, + async count(o: string, ast: any) { return run(o, ast).length; }, + async bulkCreate(o: string, rows: Record[]) { return Promise.all(rows.map((r) => this.create(o, r))); }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, async commit() {}, async rollback() {}, + }; + return { driver, stores, reads }; +} + +describe('findOne executes what it declares and refuses an empty predicate (#4419)', () => { + let engine: ObjectQL; + let reads: SeenRead[]; + let one: any, two: any, three: any; + + beforeEach(async () => { + engine = new ObjectQL(); + const mem = makeRecordingDriver(); + reads = mem.reads; + engine.registerDriver(mem.driver, true); + await engine.init(); + engine.registry.registerObject(account as any); + engine.registry.registerObject(person as any); + // The issue's own repro set. + one = await engine.insert('crm_account', { name: 'One', industry: 'Retail', annual_revenue: 100 }); + two = await engine.insert('crm_account', { name: 'Two', industry: 'Metals', annual_revenue: 200 }); + three = await engine.insert('crm_account', { name: 'Three', industry: 'Mining', annual_revenue: 200 }); + reads.length = 0; // drop insert-path reads; the pins below own this log + }); + + const lastRead = () => reads.at(-1)!; + + // ── (1) `search` is a predicate on findOne, not a dropped key ──────── + + it('findOne({search}) matches the searched record, not the first row', async () => { + const row = await engine.findOne('crm_account', { search: 'Two' } as any); + expect(row?.name).toBe('Two'); + expect(row?.id).not.toBe(one.id); + }); + + it('the search term reaches the driver as a $contains predicate — `search` never does', async () => { + await engine.findOne('crm_account', { search: 'Two' } as any); + const { ast } = lastRead(); + expect(ast.where).toBeTruthy(); + expect(JSON.stringify(ast.where)).toContain('$contains'); + expect(ast.search).toBeUndefined(); + expect(ast.searchFields).toBeUndefined(); + }); + + it('findOne({search, searchFields}) narrows the scanned fields, as find() does', async () => { + // 'Two' lives in `name`, 'Metals' in `industry`. Narrowed to + // `industry`, only the latter can hit — and a narrowed miss must be a + // miss, not a fall-back to an unpredicated read. + const narrowed = { searchFields: ['industry'] }; + expect(await engine.findOne('crm_account', { search: 'Two', ...narrowed } as any)).toBeNull(); + expect((await engine.findOne('crm_account', { search: 'Metals', ...narrowed } as any))?.id) + .toBe(two.id); + }); + + it('find({search}) is unchanged — the expansion moved, it did not fork', async () => { + const rows = await engine.find('crm_account', { search: 'Two' } as any); + expect(rows.map((r: any) => r.name)).toEqual(['Two']); + }); + + it('a search that expands to NO filter is refused, not answered with an arbitrary row', async () => { + // A blank/whitespace term expands to nothing (`expandSearchToFilter` + // returns null), so the AST is left with no predicate at all — the + // "predicate resolved to empty" shape the issue names. Before #4419 the + // forced `limit: 1` turned it into the object's first row. + for (const term of ['', ' ']) { + await expect(engine.findOne('crm_account', { search: term } as any)) + .rejects.toThrow(/selects no particular record/); + } + expect(reads).toHaveLength(0); + }); + + // ── (2) the empty-predicate guard ─────────────────────────────────── + + it.each([ + ['no argument at all', undefined], + ['an empty bag', {}], + ['an empty where', { where: {} }], + ['an explicitly null where', { where: null }], + ['a withdrawn filter alias', { filter: null }], + ['projection without a predicate', { fields: ['id', 'name'] }], + ['expand without a predicate', { expand: { owner: { object: 'person' } } }], + ])('findOne with %s throws instead of returning the first row', async (_label, query) => { + await expect(engine.findOne('crm_account', query as any)).rejects.toThrow( + /findOne\('crm_account'\) selects no particular record/, + ); + expect(reads).toHaveLength(0); // refused before the driver was asked + }); + + it('the refusal names all three ways to be specific', async () => { + await expect(engine.findOne('crm_account', {} as any)).rejects.toThrow( + /Pass 'where'.*pass 'orderBy'.*find\('crm_account', \{ limit: 1 \}\)/s, + ); + }); + + it.each([ + ['where', { where: { id: () => two.id } }], + ['the filter alias', { filter: { id: () => two.id } }], + ])('findOne({%s}) still selects the record', async (_label, shape) => { + const key = Object.keys(shape)[0]; + const row = await engine.findOne('crm_account', { [key]: { id: two.id } } as any); + expect(row?.id).toBe(two.id); + }); + + it('orderBy alone is a legitimate findOne — "the first record in THIS order"', async () => { + const row = await engine.findOne('crm_account', { + orderBy: [{ field: 'name', order: 'desc' }], + } as any); + expect(row?.name).toBe('Two'); // Two > Three > One + }); + + it('an empty orderBy array is not an ordering, and does not satisfy the guard', async () => { + await expect(engine.findOne('crm_account', { orderBy: [] } as any)) + .rejects.toThrow(/selects no particular record/); + }); + + it('find() is NOT guarded — returning every row is an honest answer', async () => { + const rows = await engine.find('crm_account'); + expect(rows).toHaveLength(3); + expect(await engine.count('crm_account')).toBe(3); + }); + + it('a non-object where (an expression tree) is the driver\'s to interpret, not refused', async () => { + // The guard closes match-everything, not everything it cannot prove. + await expect(engine.findOne('crm_account', { where: [['name', '=', 'Two']] } as any)) + .resolves.not.toThrow(); + }); + + it('the guard reads the CALLER\'s predicate, before any middleware scoping', async () => { + // A read filter injected downstream narrows which rows are visible; it + // does not make "whichever comes first" something the caller asked for. + engine.registerMiddleware(async (opCtx: any, next: any) => { + if (opCtx.operation === 'findOne') { + opCtx.ast.where = { ...(opCtx.ast.where ?? {}), annual_revenue: 200 }; + } + return next(); + }); + await expect(engine.findOne('crm_account', {} as any)) + .rejects.toThrow(/selects no particular record/); + }); + + // ── the L2 hook surface the issue was found on ────────────────────── + + it('ctx.api.object(o).findOne({where}) works; the same call with no predicate throws', async () => { + const repo = engine.createContext({ userId: 'usr_1' }).object('crm_account'); + expect((await repo.findOne({ where: { id: two.id } }))?.id).toBe(two.id); + expect((await repo.findOne({ search: 'Three' }))?.name).toBe('Three'); + await expect(repo.findOne()).rejects.toThrow(/selects no particular record/); + await expect(repo.findOne({})).rejects.toThrow(/selects no particular record/); + }); + + it('a miss is still null — the guard did not turn "not found" into an error', async () => { + expect(await engine.findOne('crm_account', { where: { id: 'nope' } } as any)).toBeNull(); + expect(await engine.findOne('crm_account', { search: 'nope' } as any)).toBeNull(); + }); + + // ── (3) drift pin: every declared findOne option is executed ──────── + + /** + * One proof per key in `ENGINE_OPTION_KEY_SETS.findOne`: the call to make, + * and what must be observable afterwards. `na` records a key that is + * deliberately not executed, with the reason — the only honest way to leave + * one out, and the thing `search` never had. + * + * The table is asserted to cover the legal set EXACTLY, so a key added to + * the spec must be given a proof (or an explicit `na`) before it can ship. + */ + type Proof = + | { call: Record; expect: (seen: SeenRead) => void } + | { na: string }; + + const PROOFS: Record = { + where: { + call: { where: { name: 'Two' } }, + expect: ({ ast }) => expect(ast.where).toMatchObject({ name: 'Two' }), + }, + fields: { + call: { where: { name: 'Two' }, fields: ['id', 'name'] }, + expect: ({ ast }) => expect(ast.fields).toEqual(['id', 'name']), + }, + orderBy: { + call: { orderBy: [{ field: 'name', order: 'desc' }] }, + expect: ({ ast }) => expect(ast.orderBy).toEqual([{ field: 'name', order: 'desc' }]), + }, + offset: { + call: { where: { annual_revenue: 200 }, offset: 1 }, + expect: ({ ast }) => expect(ast.offset).toBe(1), + }, + search: { + call: { search: 'Two' }, + expect: ({ ast }) => { + expect(ast.search).toBeUndefined(); + expect(JSON.stringify(ast.where)).toContain('$contains'); + }, + }, + searchFields: { + call: { search: 'Two', searchFields: ['name'] }, + expect: ({ ast }) => { + expect(ast.searchFields).toBeUndefined(); + // Narrowed to the one requested field, not the auto-default set. + expect(JSON.stringify(ast.where)).toContain('name'); + }, + }, + expand: { + call: { where: { name: 'Two' }, expand: { owner: { object: 'person' } } }, + // Engine-side post-processing: the relation is resolved after the + // fetch, so the proof is that the key survives onto the AST for it. + expect: ({ ast }) => expect(ast.expand).toBeTruthy(), + }, + context: { + call: { where: { name: 'Two' }, context: { tenantId: 't-1' } }, + expect: ({ ast, opts }) => { + expect(ast.context).toBeUndefined(); // not a driver concern + expect(opts).toMatchObject({ tenantId: 't-1' }); + }, + }, + tenantId: { + call: { where: { name: 'Two' }, tenantId: 't-explicit' }, + expect: ({ opts }) => expect(opts).toMatchObject({ tenantId: 't-explicit' }), + }, + tenantIds: { + call: { where: { name: 'Two' }, tenantIds: ['t-1', 't-2'] }, + expect: ({ opts }) => expect(opts).toMatchObject({ tenantIds: ['t-1', 't-2'] }), + }, + timezone: { + call: { where: { name: 'Two' }, timezone: 'Asia/Shanghai' }, + expect: ({ opts }) => expect(opts).toMatchObject({ timezone: 'Asia/Shanghai' }), + }, + transaction: { + call: { where: { name: 'Two' }, transaction: { id: 'tx-1' } }, + expect: ({ opts }) => expect(opts).toMatchObject({ transaction: { id: 'tx-1' } }), + }, + bypassTenantAudit: { + call: { where: { name: 'Two' }, bypassTenantAudit: true }, + expect: ({ opts }) => expect(opts).toMatchObject({ bypassTenantAudit: true }), + }, + preserveAudit: { + call: { where: { name: 'Two' }, preserveAudit: true }, + expect: ({ opts }) => expect(opts).toMatchObject({ preserveAudit: true }), + }, + limit: { + na: 'findOne is single-row by contract — the literal `limit: 1` on the AST ' + + 'wins over any caller value (and over the folded `top` alias). Legal ' + + 'because the shared find/findOne schema declares it; overridden, not dropped.', + }, + }; + + it('every option findOne declares is one it executes', async () => { + const legal = ENGINE_OPTION_KEY_SETS.findOne; + expect(new Set(Object.keys(PROOFS)), 'PROOFS must cover the legal set exactly') + .toEqual(new Set(legal)); + + for (const [key, proof] of Object.entries(PROOFS)) { + if ('na' in proof) { + expect(proof.na.length, `${key} must state WHY it is not executed`).toBeGreaterThan(20); + continue; + } + reads.length = 0; + await engine.findOne('crm_account', proof.call as any); + expect(reads.length, `findOne({${key}}) must reach the driver`).toBeGreaterThan(0); + proof.expect(lastRead()); + } + }); + + it('the caller-supplied limit is overridden, not honoured — findOne stays single-row', async () => { + await engine.findOne('crm_account', { where: { annual_revenue: 200 }, limit: 5 } as any); + expect(lastRead().ast.limit).toBe(1); + // `top` folds to `limit` (#4346) and loses to the same override. + await engine.findOne('crm_account', { where: { annual_revenue: 200 }, top: 5 } as any); + expect(lastRead().ast.limit).toBe(1); + }); + + it('the issue\'s original repro table, end to end', async () => { + const o = () => engine.createContext({ isSystem: true }).object('crm_account'); + expect((await o().findOne({ where: { id: two.id } }))?.name).toBe('Two'); + expect((await o().findOne({ filter: { id: two.id } }))?.name).toBe('Two'); // #4346 + expect(await o().findOne({ where: { id: 'nope' } })).toBeNull(); + expect(await o().findOne({ filter: { id: 'nope' } })).toBeNull(); // #4346 + expect(await o().count({ where: { annual_revenue: 100 } })).toBe(1); + expect(await o().count({ filter: { annual_revenue: 100 } })).toBe(1); // #4346 + expect((await o().find({ filter: { annual_revenue: 200 } })).map((r: any) => r.id).sort()) + .toEqual([two.id, three.id].sort()); + expect((await o().findOne({ search: 'Two' }))?.name).toBe('Two'); // #4419 + await expect(o().findOne({})).rejects.toThrow(/selects no particular record/); // #4419 + }); +}); diff --git a/packages/objectql/src/engine.test.ts b/packages/objectql/src/engine.test.ts index 4257103691..c8960f56e9 100644 --- a/packages/objectql/src/engine.test.ts +++ b/packages/objectql/src/engine.test.ts @@ -668,7 +668,9 @@ describe('ObjectQL Engine', () => { it('findOne accepts context via the trailing options arg', async () => { (mockDriver.findOne as any).mockResolvedValue({ id: '1' }); - await engine.findOne('task', {}, { context: { tenantId: 't-fo' } as any }); + // `where` is not incidental: findOne refuses a query that selects no + // particular record (#4419). + await engine.findOne('task', { where: { id: '1' } }, { context: { tenantId: 't-fo' } as any }); expect((mockDriver.findOne as any).mock.calls.at(-1)?.[2]).toMatchObject({ tenantId: 't-fo' }); }); @@ -2053,7 +2055,10 @@ describe('ObjectQL Engine', () => { { id: 'u1', name: 'Alice' }, ]); - const result = await engine.findOne('task', { expand: { assignee: { object: 'assignee' } } }); + const result = await engine.findOne('task', { + where: { id: 't1' }, + expand: { assignee: { object: 'assignee' } }, + }); expect(result.assignee).toEqual({ id: 'u1', name: 'Alice' }); }); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 730ea3596e..f03f2dd6be 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -3236,6 +3236,97 @@ export class ObjectQL implements IObjectQLEngine { return resolved === options.where ? options : ({ ...options, where: resolved } as T); } + /** + * ADR-0061: expand `search` into a server-resolved cross-field `$or` of + * `$contains`, AND it with any caller `where`, then strip the search keys off + * the AST. + * + * Shared by `find` and `findOne` (#4419). It lived inline in `find` and + * nowhere else, while `ENGINE_FIND_OPTION_KEYS` — the one legal-key set BOTH + * methods are checked against (see {@link ENGINE_OPTION_KEY_SETS}) — declares + * `search`/`searchFields` for both. So `findOne({ search })` passed the gate, + * rode onto the AST verbatim, and reached a driver: no driver reads + * `ast.search` (the expansion is the engine's job by ADR-0061), so the + * predicate vanished and the forced `limit: 1` turned it into the first row of + * the WHOLE object — a real, plausible-looking record unrelated to the search. + * That is #4419's reported failure exactly, under a different key than the + * `filter` #4346 closed; one expander, called from both, is what stops the + * pair drifting again. + * + * Field resolution is server-side (declared `searchableFields` → + * auto-default); the optional `searchFields` override is intersected with the + * allowed set, never widened. All drivers already execute `$or`/`$contains`, + * so this needs no driver changes. + * + * The keys are deleted whether or not anything expanded — leaving them on + * would hand the driver a key it does not read, which is the same silent drop + * one layer down. + */ + private expandSearchOnAst(ast: QueryAST, schema: ServiceObject | undefined): void { + // The `$search`/`$searchFields` OData spellings are NOT read here: the + // protocol layer normalizes them to the bare keys before the engine + // (protocol.ts findData), and a direct engine call carrying one is an + // unknown option — rejected at the entry point, not silently dropped + // (#4371). + const raw = (ast as any).search; + if (raw != null && schema?.fields) { + const requestedFields = (ast as any).searchFields + ?? (typeof raw === 'object' ? raw?.fields : undefined); + const searchFilter = expandSearchToFilter(raw, { + fields: schema.fields as any, + searchableFields: (schema as any).searchableFields, + requestedFields, + // [ADR-0079] `nameField` is the canonical primary-title pointer; + // `displayNameField` is the deprecated alias (still honored). + displayField: (schema as any).nameField ?? (schema as any).displayNameField, + }); + if (searchFilter) { + ast.where = ast.where ? { $and: [ast.where, searchFilter] } : searchFilter; + } + } + delete (ast as any).search; + delete (ast as any).searchFields; + } + + /** + * Refuse a `findOne` that selects nothing in particular (#4419). + * + * The AST reaching here is the CALLER's own intent: aliases folded, unknown + * keys refused, `search` expanded — but the security/sharing middlewares have + * not run yet, and that ordering is the point. An injected RLS predicate + * narrows *which* rows are visible; it does not make "whichever of them comes + * first" a thing the caller asked for. Judging the post-middleware AST would + * pass every query on a scoped object and leave the hole open where it is + * most expensive. + * + * "Selects nothing" is read the same way #3896 read an empty sharing + * criteria: absent, `null`, or `{}` — the three shapes that mean "match every + * row". A `where` that is not a plain object (an expression tree) is the + * driver's to interpret, and counts as a predicate; this guard closes the one + * case that is unambiguously match-everything, not everything it cannot + * prove. + * + * `orderBy` is the other way to be specific, and a legitimate one — "the + * newest", "the highest priority". It is honored on this path by every + * driver, so it is a real answer and not a second silent drop. + */ + private requireFindOnePredicate(object: string, ast: QueryAST): void { + const where = ast.where as unknown; + const hasPredicate = + where != null && + (typeof where !== 'object' || Array.isArray(where) || Object.keys(where).length > 0); + if (hasPredicate) return; + if (Array.isArray(ast.orderBy) && ast.orderBy.length > 0) return; + throw new Error( + `findOne('${object}') selects no particular record: 'where' is absent or empty ` + + `and the query carries no 'orderBy'. findOne applies limit: 1, so this would return an ` + + `ARBITRARY row — a real, plausible-looking record unrelated to what was asked for, which ` + + `no caller's null-check can catch (#4419). Pass 'where' (or a 'search' that resolves to ` + + `one) to select the record; pass 'orderBy' if you mean "the first record in THIS order"; ` + + `or call find('${object}', { limit: 1 }) if any row will genuinely do.`, + ); + } + async find(object: string, query?: EngineQueryOptions, options?: EngineReadOptions): Promise { object = this.resolveObjectName(object); // Normalize the alias spellings (`filter`→`where`, `top`→`limit`) by the @@ -3263,35 +3354,7 @@ export class ObjectQL implements IObjectQLEngine { // fields needed to compute the formulas after fetch. const _findSchema = this._registry.getObject(object); - // ADR-0061: expand `$search` into a server-resolved cross-field `$or` - // of `$contains`. Field resolution is server-side (declared - // `searchableFields` -> auto-default); the optional `$searchFields` override - // is intersected with the allowed set. All drivers already execute - // `$or`/`$contains`, so this needs no driver changes. - { - // The `$search`/`$searchFields` OData spellings are NOT read here: the - // protocol layer normalizes them to the bare keys before the engine - // (protocol.ts findData), and a direct engine call carrying one is an - // unknown option — rejected above, not silently dropped (#4371). - const _searchRaw = (ast as any).search; - if (_searchRaw != null && _findSchema?.fields) { - const _reqFields = (ast as any).searchFields - ?? (typeof _searchRaw === 'object' ? _searchRaw?.fields : undefined); - const _searchFilter = expandSearchToFilter(_searchRaw, { - fields: _findSchema.fields as any, - searchableFields: (_findSchema as any).searchableFields, - requestedFields: _reqFields, - // [ADR-0079] `nameField` is the canonical primary-title pointer; - // `displayNameField` is the deprecated alias (still honored). - displayField: (_findSchema as any).nameField ?? (_findSchema as any).displayNameField, - }); - if (_searchFilter) { - ast.where = ast.where ? { $and: [ast.where, _searchFilter] } : _searchFilter; - } - } - delete (ast as any).search; - delete (ast as any).searchFields; - } + this.expandSearchOnAst(ast, _findSchema); const _findFormula = planFormulaProjection(_findSchema, ast.fields); if (_findFormula.projected) ast.fields = _findFormula.projected; @@ -3383,6 +3446,33 @@ export class ObjectQL implements IObjectQLEngine { return opCtx.result as any[]; } + /** + * Read the ONE record the query selects, or `null`. + * + * `findOne` applies `limit: 1` by contract — so unlike `find`, the query's + * predicate is the only thing standing between the caller and *an arbitrary + * row*. A query that selects nothing in particular does not return nothing; + * it returns the object's first row, which is a real, plausible-looking + * record that no caller's `if (!row)` check can catch, and that propagates + * into whatever is computed next (#4419). So this method REQUIRES the caller + * to say which record it wants: + * + * - `where` (or the `filter` alias, folded here), or a `search` that expands + * to one — the record is selected by predicate. + * - `orderBy` — "the FIRST record in this order" (the newest, the highest + * priority). Deterministic without a predicate, and honored by every + * driver on this path. + * + * Neither → throws. If any row genuinely will do, that is + * `find(object, { limit: 1 })`, which says so at the call site. + * + * No ordering is IMPOSED when the caller supplies none: `ORDER BY LIMIT + * 1` makes a planner abandon the predicate's own index (objectstack#4363, and + * see `SqlDriver.findRows`' `singleRowLookup`). `findOne` promises *a* + * matching record, never a position in a sequence. + * + * Fires the same `beforeFind`/`afterFind` hooks as `find` (#3195). + */ async findOne(objectName: string, query?: EngineQueryOptions, options?: EngineReadOptions): Promise { objectName = this.resolveObjectName(objectName); // Same alias fold as find() (#4346). Without it, `findOne({ filter })` @@ -3404,6 +3494,10 @@ export class ObjectQL implements IObjectQLEngine { // Plan formula projection (same as find): rewrite ast.fields so the driver // returns the raw dependency fields, then evaluate formulas after fetch. const _findOneSchema = this._registry.getObject(objectName); + // Before the guard below, so a `search` that resolves to a real filter + // counts as the predicate it is (#4419). + this.expandSearchOnAst(ast, _findOneSchema); + this.requireFindOnePredicate(objectName, ast); const _findOneFormula = planFormulaProjection(_findOneSchema, ast.fields); if (_findOneFormula.projected) ast.fields = _findOneFormula.projected; diff --git a/packages/plugins/driver-mongodb/src/mongodb-driver.ts b/packages/plugins/driver-mongodb/src/mongodb-driver.ts index f1d768025b..b18d5bf658 100644 --- a/packages/plugins/driver-mongodb/src/mongodb-driver.ts +++ b/packages/plugins/driver-mongodb/src/mongodb-driver.ts @@ -212,11 +212,29 @@ export class MongoDBDriver implements IDataDriver { // CRUD Operations // =========================================================================== - async find(object: string, query: QueryAST, options?: DriverOptions): Promise[]> { - const collection = this.getCollection(object); - const session = this.getSession(options); - - const filter = translateFilter(query.where, this.temporalKindFor(object)); + /** + * The projection / sort / pagination half of a read, shared by {@link find} + * and {@link findOne} (objectstack#4419). + * + * It was inline in `find` only, and `findOne` translated `query.where` and + * nothing else — so `orderBy`, `fields` and `offset` were accepted by the + * contract and silently dropped on the way to Mongo. `findOne({ orderBy })` + * therefore did not return the newest record; it returned whichever document + * the collection scan reached first, which is the same + * plausible-looking-wrong-record failure #4419 is about, one layer below the + * engine. + * + * `singleRowLookup` marks the caller as `findOne`; see {@link buildSortSpec}. + * + * Not used by `_findStream`, which deliberately projects the whole document + * regardless of `query.fields` — a separate divergence, and one that returns + * more data rather than the wrong data, so it is left as-is here. + */ + private buildFindOptions( + query: QueryAST, + session: FindOptions['session'], + opts?: { singleRowLookup?: boolean }, + ): FindOptions { const findOptions: FindOptions = { session }; // Field projection @@ -238,13 +256,23 @@ export class MongoDBDriver implements IDataDriver { } // Sorting - const sort = this.buildSortSpec(query); + const sort = this.buildSortSpec(query, opts); if (sort) findOptions.sort = sort; // Pagination if (query.offset !== undefined) findOptions.skip = query.offset; if (query.limit !== undefined) findOptions.limit = query.limit; + return findOptions; + } + + async find(object: string, query: QueryAST, options?: DriverOptions): Promise[]> { + const collection = this.getCollection(object); + const session = this.getSession(options); + + const filter = translateFilter(query.where, this.temporalKindFor(object)); + const findOptions = this.buildFindOptions(query, session); + const cursor = collection.find(filter, findOptions); const results = await cursor.toArray(); return results as Record[]; @@ -255,10 +283,14 @@ export class MongoDBDriver implements IDataDriver { const session = this.getSession(options); const filter = translateFilter(query.where, this.temporalKindFor(object)); - const result = await collection.findOne(filter, { - session, - projection: { _id: 0 }, - }); + // `singleRowLookup`: honour the caller's ordering, impose none of our own — + // the engine sends `limit: 1`, which is indistinguishable from "page one of + // a walk with page size 1", and the two want opposite things + // (objectstack#4363, and `SqlDriver.findRows` for the measured cost). + const result = await collection.findOne( + filter, + this.buildFindOptions(query, session, { singleRowLookup: true }), + ); return result as Record | null; } @@ -630,8 +662,18 @@ export class MongoDBDriver implements IDataDriver { * Returns `undefined` for a read that is neither sorted nor paged — nothing * is being sliced there, so a caller who asked for no order keeps none (the * contract's explicit carve-out). + * + * `singleRowLookup` puts {@link findOne} in that carve-out too. It arrives + * carrying the engine's `limit: 1`, which the `paged` test below cannot tell + * from "page one of a walk with page size 1" — but `findOne` promises *a* + * matching record, never a position in a sequence, so there is no partition + * to preserve and imposing an order only costs the plan the predicate earned + * (objectstack#4363; `SqlDriver.findRows` carries the same flag and the + * measured ~100× regression that motivated it). A caller-supplied `orderBy` + * is still honoured, tie-breaker and all — that is the half this driver used + * to drop entirely (objectstack#4419). */ - private buildSortSpec(query: QueryAST): Document | undefined { + private buildSortSpec(query: QueryAST, opts?: { singleRowLookup?: boolean }): Document | undefined { const sort: Document = {}; let lastDirection: 1 | -1 = 1; if (Array.isArray(query.orderBy)) { @@ -644,7 +686,8 @@ export class MongoDBDriver implements IDataDriver { } const requested = Object.keys(sort).length > 0; - const paged = query.limit !== undefined || query.offset !== undefined; + const paged = + !opts?.singleRowLookup && (query.limit !== undefined || query.offset !== undefined); if (!requested && !paged) return undefined; const idKey = this.mapFieldName('id'); diff --git a/packages/plugins/driver-mongodb/src/mongodb-findone-options.test.ts b/packages/plugins/driver-mongodb/src/mongodb-findone-options.test.ts new file mode 100644 index 0000000000..d5a3c99fa8 --- /dev/null +++ b/packages/plugins/driver-mongodb/src/mongodb-findone-options.test.ts @@ -0,0 +1,130 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectstack#4419 — the exact `FindOptions` `MongoDBDriver.findOne` hands to + * MongoDB, asserted without a server. + * + * The defect this pins was invisible in the rows: `findOne` used to call + * `collection.findOne(filter, { session, projection: { _id: 0 } })` — no + * `sort`, no `skip`, no field projection — so `orderBy`, `offset` and `fields` + * were dropped between the contract and the wire. An untouched three-document + * collection comes back in insertion order every time, which happens to be + * stable, so a row-level assertion can pass against a driver that sorts nothing + * at all. The options object is where the bug is legible, so that is what this + * asserts. + * + * The companion suite `mongodb-findone-query.test.ts` proves the same behaviour + * end to end against a real mongod, and skips when the binary cannot be + * fetched. This one has no such dependency and therefore always runs — which is + * the point: a driver defect that only a downloadable binary can catch is a + * defect nobody catches on a restricted network. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { MongoDBDriver } from './mongodb-driver.js'; + +interface Seen { filter: any; options: any } + +/** + * A driver wired to a fake `Db` — no connect(), no server. `getCollection` is + * `this.db.collection(name)`, so replacing `db` is enough to observe every call + * the real code path makes. + */ +function makeDriver() { + const seen: { findOne: Seen[]; find: Seen[] } = { findOne: [], find: [] }; + const rows: Record[] = []; + const collection = { + findOne: async (filter: any, options: any) => { seen.findOne.push({ filter, options }); return rows[0] ?? null; }, + find: (filter: any, options: any) => { + seen.find.push({ filter, options }); + return { toArray: async () => rows }; + }, + }; + const driver = new MongoDBDriver({ url: 'mongodb://unused/', database: 'unused' }); + (driver as any).db = { collection: () => collection }; + return { driver, seen, rows }; +} + +describe('MongoDBDriver.findOne hands Mongo the whole query (#4419)', () => { + let driver: MongoDBDriver; + let seen: { findOne: Seen[]; find: Seen[] }; + + beforeEach(() => { + const made = makeDriver(); + driver = made.driver; + seen = made.seen; + }); + + const lastFindOne = () => seen.findOne.at(-1)!; + + it('translates the predicate, as it always did', async () => { + await driver.findOne('account', { object: 'account', where: { id: 'a' }, limit: 1 } as any); + expect(lastFindOne().filter).toMatchObject({ id: 'a' }); + }); + + // ── the three parameters that used to be dropped ───────────────────── + + it('sends the caller ordering, with the id tie-breaker in the same direction', async () => { + await driver.findOne('account', { + object: 'account', orderBy: [{ field: 'rank', order: 'desc' }], limit: 1, + } as any); + expect(lastFindOne().options.sort).toEqual({ rank: -1, id: -1 }); + }); + + it('sends the field projection — `id` kept, `_id` dropped', async () => { + await driver.findOne('account', { + object: 'account', where: { id: 'a' }, fields: ['name'], limit: 1, + } as any); + expect(lastFindOne().options.projection).toEqual({ name: 1, id: 1, _id: 0 }); + }); + + it('sends the offset', async () => { + await driver.findOne('account', { + object: 'account', orderBy: [{ field: 'rank', order: 'asc' }], offset: 2, limit: 1, + } as any); + expect(lastFindOne().options.skip).toBe(2); + }); + + it('all three at once, on one query', async () => { + await driver.findOne('account', { + object: 'account', + where: { status: 'open' }, + orderBy: [{ field: 'created_at', order: 'desc' }], + fields: ['id', 'name'], + offset: 1, + limit: 1, + } as any); + const { filter, options } = lastFindOne(); + expect(filter).toMatchObject({ status: 'open' }); + expect(options.sort).toEqual({ created_at: -1, id: -1 }); + expect(options.projection).toEqual({ id: 1, name: 1, _id: 0 }); + expect(options.skip).toBe(1); + }); + + // ── and the half that must stay absent (#4363) ─────────────────────── + + it('imposes NO sort on an unordered findOne — its limit: 1 is not a page', async () => { + await driver.findOne('account', { object: 'account', where: { id: 'a' }, limit: 1 } as any); + expect(lastFindOne().options.sort).toBeUndefined(); + }); + + it('an unordered PAGED find still gets one — the carve-out is findOne-only', async () => { + await driver.find('account', { object: 'account', limit: 2, offset: 0 } as any); + expect(seen.find.at(-1)!.options.sort).toEqual({ id: 1 }); + }); + + it('an unordered, unpaged find still gets none — that rule is unchanged', async () => { + await driver.find('account', { object: 'account' } as any); + expect(seen.find.at(-1)!.options.sort).toBeUndefined(); + }); + + it('the transaction session still rides through', async () => { + const session = { id: 'sess-1' } as any; + await driver.findOne( + 'account', + { object: 'account', where: { id: 'a' }, limit: 1 } as any, + { transaction: session } as any, + ); + expect(lastFindOne().options.session).toBe(session); + }); +}); diff --git a/packages/plugins/driver-mongodb/src/mongodb-findone-query.test.ts b/packages/plugins/driver-mongodb/src/mongodb-findone-query.test.ts new file mode 100644 index 0000000000..7512903b07 --- /dev/null +++ b/packages/plugins/driver-mongodb/src/mongodb-findone-query.test.ts @@ -0,0 +1,151 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectstack#4419 — `MongoDBDriver.findOne` executes the whole QueryAST, not + * just its `where`. + * + * It used to issue `collection.findOne(translateFilter(query.where), { + * projection: { _id: 0 } })` and nothing else: `orderBy`, `fields` and `offset` + * were accepted by the contract and dropped on the floor. `find` and + * `_findStream` in the same file had always handled all three, so this was a + * per-method divergence exactly like the engine-level one #4419 is about — + * `findOne({ orderBy: [{ field: 'created_at', order: 'desc' }] })` did not + * return the newest record, it returned whichever document the scan reached + * first, with no error and nothing to grep for. + * + * It matters beyond Mongo: the engine's own findOne guard tells a caller with + * no predicate to pass `orderBy` instead ("the first record in THIS order"). An + * escape hatch one backend silently ignores is not an escape hatch. + * + * The other half is what must NOT happen: no ordering is imposed when the + * caller asks for none (objectstack#4363). The engine sends `limit: 1`, which + * the paged-read rule cannot tell from "page one with page size 1", and the two + * want opposite things — `findOne` promises *a* matching record, never a + * position in a sequence, so an imposed `sort` only costs it the plan its + * predicate earned. + * + * Runs against a real mongod via `mongodb-memory-server`, and skips itself when + * the binary cannot be fetched — the convention the other suites in this + * package already use. + */ + +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import { MongoMemoryServer } from 'mongodb-memory-server'; +import { MongoDBDriver } from './mongodb-driver.js'; + +let sharedMongod: MongoMemoryServer | undefined; +try { + sharedMongod = await MongoMemoryServer.create({ instance: { launchTimeout: 60_000 } }); +} catch (err) { + console.warn( + '[driver-mongodb] Skipping findOne query-execution suite — mongodb-memory-server could not ' + + `start: ${(err as Error)?.message ?? String(err)}`, + ); +} + +const ROWS = [ + { id: 'a', name: 'Alpha', rank: 3, secret: 'sa' }, + { id: 'b', name: 'Bravo', rank: 1, secret: 'sb' }, + { id: 'c', name: 'Charlie', rank: 2, secret: 'sc' }, +]; + +describe.skipIf(!sharedMongod)('driver-mongodb — findOne executes the whole query', () => { + const mongod = sharedMongod as MongoMemoryServer; + let driver: MongoDBDriver; + + beforeAll(async () => { + driver = new MongoDBDriver({ url: mongod.getUri(), database: 'findone_query' }); + await driver.connect(); + for (const row of ROWS) await driver.create('account', { ...row }); + }, 90_000); + + afterAll(async () => { + if (driver) await driver.disconnect(); + if (sharedMongod) await sharedMongod.stop(); + }); + + // ── orderBy: the half that used to vanish ─────────────────────────── + + it.each([ + ['asc', 'b'], + ['desc', 'a'], + ])('findOne({orderBy rank %s}) returns the extreme row, not an arbitrary one', async (order, expected) => { + const row = await driver.findOne('account', { + object: 'account', + orderBy: [{ field: 'rank', order: order as 'asc' | 'desc' }], + limit: 1, + } as any); + expect(row?.id).toBe(expected); + }); + + it('orderBy composes with a predicate — the newest of the MATCHING rows', async () => { + const row = await driver.findOne('account', { + object: 'account', + where: { id: { $in: ['a', 'c'] } }, + orderBy: [{ field: 'rank', order: 'asc' }], + limit: 1, + } as any); + expect(row?.id).toBe('c'); // rank 2 < rank 3 + }); + + // ── fields / offset ───────────────────────────────────────────────── + + it('findOne({fields}) projects — a column not asked for does not come back', async () => { + const row = await driver.findOne('account', { + object: 'account', where: { id: 'a' }, fields: ['name'], limit: 1, + } as any); + expect(row).toEqual({ id: 'a', name: 'Alpha' }); // `id` always kept, `_id` never + expect(row).not.toHaveProperty('secret'); + }); + + it('findOne({offset}) skips, so an ordered walk can step past the first match', async () => { + const row = await driver.findOne('account', { + object: 'account', orderBy: [{ field: 'rank', order: 'asc' }], offset: 1, limit: 1, + } as any); + expect(row?.id).toBe('c'); // ranks asc → b, c, a + }); + + // ── and the half that must stay absent (#4363) ─────────────────────── + + it('imposes NO sort when the caller asked for none — the limit: 1 is not a page', async () => { + const collection = (driver as any).getCollection('account'); + const spy = vi.spyOn(collection, 'findOne'); + try { + await driver.findOne('account', { object: 'account', where: { id: 'a' }, limit: 1 } as any); + expect(spy).toHaveBeenCalledTimes(1); + expect(spy.mock.calls[0][1]).not.toHaveProperty('sort'); + } finally { + spy.mockRestore(); + } + }); + + it('a caller-supplied sort still gets the id tie-breaker appended', async () => { + const collection = (driver as any).getCollection('account'); + const spy = vi.spyOn(collection, 'findOne'); + try { + await driver.findOne('account', { + object: 'account', orderBy: [{ field: 'rank', order: 'desc' }], limit: 1, + } as any); + expect(spy.mock.calls[0][1]?.sort).toEqual({ rank: -1, id: -1 }); + } finally { + spy.mockRestore(); + } + }); + + it('find() is unchanged — a paged read still gets its deterministic order', async () => { + const collection = (driver as any).getCollection('account'); + const spy = vi.spyOn(collection, 'find'); + try { + await driver.find('account', { object: 'account', limit: 2, offset: 0 } as any); + expect(spy.mock.calls[0][1]?.sort).toEqual({ id: 1 }); + } finally { + spy.mockRestore(); + } + }); + + it('a miss is still null', async () => { + expect(await driver.findOne('account', { + object: 'account', where: { id: 'nope' }, limit: 1, + } as any)).toBeNull(); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index 5f13a8a727..f00e6d96f1 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -1349,9 +1349,12 @@ export class SqlDriver implements IDataDriver { * bought for that: `findOne` promises *a* matching record, never a position * in a sequence, so there is no partition to preserve. * - * That also puts this driver back in step with `MongoDBDriver.findOne`, which - * issues `collection.findOne` and has never sorted. The obligation the - * contract states is on `find`, and this keeps it there. + * `MongoDBDriver.findOne` carries the same flag into its own `buildSortSpec`, + * so both drivers now read a `findOne` the same way: honour the caller's + * `orderBy`, impose nothing when there is none. (Mongo used to translate + * `where` and drop `orderBy` outright — an earlier version of this comment + * cited that as agreement, which it was not; objectstack#4419.) The + * obligation the contract states is on `find`, and this keeps it there. */ private async findRows( object: string, diff --git a/packages/spec/src/contracts/data-engine.ts b/packages/spec/src/contracts/data-engine.ts index 571d57704d..618ede8e04 100644 --- a/packages/spec/src/contracts/data-engine.ts +++ b/packages/spec/src/contracts/data-engine.ts @@ -68,6 +68,20 @@ export interface IDataEngine { * supported; when both are given, `options.context` wins. */ find(objectName: string, query?: EngineQueryOptions, options?: BaseEngineOptions): Promise; + /** + * Read the ONE record the query selects, or `null`. + * + * The query MUST say which record it wants: a `where` (or a `search` that + * expands to one), or an `orderBy` meaning "the FIRST record in this order". + * A query with neither is REJECTED (#4419) — `findOne` reads a single row, so + * an empty predicate does not return nothing, it returns the object's first + * row: a real, plausible-looking record unrelated to the request, which no + * caller's `if (!row)` can catch. When any row genuinely will do, that is + * `find(objectName, { limit: 1 })`, which says so at the call site. + * + * No ordering is imposed when the caller supplies none: `findOne` promises + * *a* matching record, never a position in a sequence (#4363). + */ findOne(objectName: string, query?: EngineQueryOptions, options?: BaseEngineOptions): Promise; insert(objectName: string, data: any | any[], options?: DataEngineInsertOptions & WriteObservabilityOptions): Promise; update(objectName: string, data: any, options?: EngineUpdateOptions & WriteObservabilityOptions): Promise; diff --git a/skills/objectstack-data/references/data-hooks.md b/skills/objectstack-data/references/data-hooks.md index 229cbb731c..07958757bb 100644 --- a/skills/objectstack-data/references/data-hooks.md +++ b/skills/objectstack-data/references/data-hooks.md @@ -324,7 +324,7 @@ org / user / transaction. Methods: | Method | Capability | Call | |:--|:--|:--| | `find(opts)` | `api.read` | `find({ where: { … }, fields, sort, limit })` → array | -| `findOne(opts)` | `api.read` | `findOne({ where: { id } })` → record \| `null` | +| `findOne(opts)` | `api.read` | `findOne({ where: { id } })` → record \| `null` — needs a predicate or an `orderBy`, see below | | `count(opts)` | `api.read` | `count({ where: { … } })` → number | | `insert(data)` | `api.write` | `insert({ … })` | | `update(data, opts?)` | `api.write` | **`update({ id, ...fields })`** — put the id **inside** `data` | @@ -346,6 +346,25 @@ await ctx.api.object('task').find({ where: { $and: [{ done: false }, { owner: ui > `[['id', '=', x]]` — that is not a supported value shape and silently matches > nothing. +**`findOne` must say which record it wants.** It reads a single row, so an +absent or empty predicate does not come back as `null` — it comes back as the +object's **first row**: a real, plausible-looking record with nothing to do with +what you asked for, which your `if (!row)` cannot catch. So `findOne()`, +`findOne({})` and `findOne({ where: {} })` **throw** (#4419). Be specific in one +of three ways: + +```js +await ctx.api.object('candidate').findOne({ where: { id } }); // by predicate +await ctx.api.object('candidate').findOne({ search: 'Acme' }); // by search +await ctx.api.object('audit') + .findOne({ orderBy: [{ field: 'created_at', order: 'desc' }] });// "the newest one" +await ctx.api.object('candidate').find({ limit: 1 }); // any row will do +``` + +An unpredicated `find` / `count` is fine — returning or counting every row is an +honest answer. It is `findOne`'s implicit "just one of them" that turns a missing +predicate into a confidently wrong record. + **Update by id.** `update` reads the primary key out of `data`, so the single-record form is `update({ id, ...fieldsToChange })` — e.g. `update({ id: pos, status: 'filled' })`. From 07139a8d3cc8ee5bbc994c5c9291dcfda4367262 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 08:34:37 +0000 Subject: [PATCH 2/2] fix(driver-mongodb): pin findOne's query execution from one shared case table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught three failures in the end-to-end Mongo suite this PR added — all in the test, none in the product. The four behavioural assertions (orderBy asc / desc, fields, offset) passed against a real mongod, so the driver fix itself is confirmed; the three spy-based ones could never have worked: const collection = (driver as any).getCollection('account'); vi.spyOn(collection, 'findOne'); // -> "called 0 times" `getCollection` is `this.db.collection(name)`, which builds a FRESH Collection per call, so the spy never saw the one the driver used. The server-free suite passed only because it replaces `db` with a fake whose `collection()` returns the same object. They are replaced rather than repaired, because the local environment cannot download a mongod and the first version shipped expectations nothing had run. The cases now live in one table, `mongodb-findone-cases.ts`, read by both halves: - `mongodb-findone-options.test.ts` (no server, always runs) asserts the `FindOptions` the driver emits — including the cases whose expectation is that NO sort was imposed (#4363), which is not observable in rows — and then executes the same cases against a controlled in-process collection so the expected ids are falsifiable without a binary download. That second pass is deliberately narrow: it checks the case table, never whether the driver is correct. - `mongodb-findone-query.test.ts` runs the same table against a real mongod and asserts rows only, answering the one question the other cannot. `b` and `d` now tie on `rank`, which is the only place the appended `id` tie-breaker is observable in the rows at all — so the sort spec is proven end to end without a spy. Reverse-verified both ways locally: a wrong expected id in the table fails the options suite (1 red), and reverting the driver fix fails it 12 red. Also updates the `findOne` section of the data-engine contract doc, which still described it as returning "the first record matching a query" — the reading that makes an empty query look legal. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Uy3zSSDGcJybydLSwGypFS --- content/docs/kernel/contracts/data-engine.mdx | 20 +- .../src/mongodb-findone-cases.ts | 125 +++++++++++++ .../src/mongodb-findone-options.test.ts | 177 +++++++++++------- .../src/mongodb-findone-query.test.ts | 129 +++---------- 4 files changed, 281 insertions(+), 170 deletions(-) create mode 100644 packages/plugins/driver-mongodb/src/mongodb-findone-cases.ts diff --git a/content/docs/kernel/contracts/data-engine.mdx b/content/docs/kernel/contracts/data-engine.mdx index cf8fff765f..12d31bd2ba 100644 --- a/content/docs/kernel/contracts/data-engine.mdx +++ b/content/docs/kernel/contracts/data-engine.mdx @@ -146,7 +146,7 @@ where: { ### findOne -Convenience method that returns the first record matching a query, or `null`. +Returns the ONE record the query selects, or `null`. ```typescript const task = await engine.findOne('task', { @@ -155,6 +155,24 @@ const task = await engine.findOne('task', { }); ``` +The query must say **which** record it wants — a `where` (or a `search` that +expands to one), or an `orderBy` meaning "the first record in this order". A +query with neither is rejected (#4419): + +```typescript +await engine.findOne('task', {}); // throws +await engine.findOne('task', { // the newest task + orderBy: [{ field: 'created_at', order: 'desc' }], +}); +await engine.find('task', { limit: 1 }); // any task will do +``` + +`findOne` reads a single row, so a missing predicate does not come back as +`null` — it comes back as the object's **first row**, a real record unrelated to +the request that no `if (!task)` check can catch. No ordering is imposed when +you supply none: `findOne` promises *a* matching record, never a position in a +sequence. + ### count Returns the number of records matching a filter without fetching data. diff --git a/packages/plugins/driver-mongodb/src/mongodb-findone-cases.ts b/packages/plugins/driver-mongodb/src/mongodb-findone-cases.ts new file mode 100644 index 0000000000..969922596c --- /dev/null +++ b/packages/plugins/driver-mongodb/src/mongodb-findone-cases.ts @@ -0,0 +1,125 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Shared cases for the two halves of the `findOne` query-execution proof + * (objectstack#4419), so neither half can drift from the other. + * + * The defect being pinned — `MongoDBDriver.findOne` translating `where` and + * dropping `orderBy` / `fields` / `offset` — needs BOTH halves, and each is + * blind to something the other sees: + * + * - `mongodb-findone-options.test.ts` asserts the `FindOptions` the driver + * emits, and executes the cases against a controlled in-process collection. + * It needs no server, so it always runs — which matters, because a driver + * defect only a downloadable binary can catch is a defect nobody catches on + * a restricted network. What it cannot prove is that MongoDB agrees. + * - `mongodb-findone-query.test.ts` runs the same cases against a real mongod, + * and skips when the binary cannot be fetched. It proves the server agrees; + * it proves nothing when it skips. + * + * One table, two readers. An expectation edited on one side moves both. + * + * `b` and `d` deliberately TIE on `rank`: equal sort keys have no defined + * relative order in MongoDB, so a tie is the only place the driver's appended + * `id` tie-breaker is observable in the ROWS at all. + */ + +export interface FindOneRow { + id: string; + name: string; + rank: number; + secret: string; +} + +export const FINDONE_ROWS: readonly FindOneRow[] = [ + { id: 'a', name: 'Alpha', rank: 3, secret: 'sa' }, + { id: 'b', name: 'Bravo', rank: 1, secret: 'sb' }, + { id: 'c', name: 'Charlie', rank: 2, secret: 'sc' }, + { id: 'd', name: 'Delta', rank: 1, secret: 'sd' }, +]; + +export interface FindOneCase { + /** Test name, used verbatim by both suites. */ + name: string; + /** The QueryAST `findOne` receives — the engine always supplies `limit: 1`. */ + query: Record; + /** `id` of the row that must come back, or `null` for a miss. */ + expectId: string | null; + /** + * The `sort` the driver must put on the wire. `undefined` asserts that NO + * ordering was imposed — the #4363 carve-out that makes `findOne` keep the + * plan its predicate earned. Only the options suite can check this; it is not + * observable in the rows. + */ + expectSort: Record | undefined; + /** Keys the returned row must have exactly, when the case projects. */ + expectKeys?: string[]; + /** `skip` the driver must put on the wire, when the case paginates. */ + expectSkip?: number; +} + +export const FINDONE_CASES: readonly FindOneCase[] = [ + // ── orderBy: the half that used to vanish entirely ────────────────── + { + name: 'orderBy rank asc returns the lowest-ranked row, not an arbitrary one', + query: { orderBy: [{ field: 'rank', order: 'asc' }], limit: 1 }, + expectId: 'b', + expectSort: { rank: 1, id: 1 }, + }, + { + name: 'orderBy rank desc returns the highest-ranked row', + query: { orderBy: [{ field: 'rank', order: 'desc' }], limit: 1 }, + expectId: 'a', + expectSort: { rank: -1, id: -1 }, + }, + { + name: 'orderBy composes with a predicate — the first of the MATCHING rows', + query: { where: { id: { $in: ['a', 'c'] } }, orderBy: [{ field: 'rank', order: 'asc' }], limit: 1 }, + expectId: 'c', // rank 2 < rank 3 + expectSort: { rank: 1, id: 1 }, + }, + + // ── the id tie-breaker, in the LAST requested direction ───────────── + { + name: 'a tie on the sort key is broken by id, ascending', + query: { where: { rank: 1 }, orderBy: [{ field: 'rank', order: 'asc' }], limit: 1 }, + expectId: 'b', + expectSort: { rank: 1, id: 1 }, + }, + { + name: 'a tie on the sort key is broken by id, descending', + query: { where: { rank: 1 }, orderBy: [{ field: 'rank', order: 'desc' }], limit: 1 }, + expectId: 'd', + expectSort: { rank: -1, id: -1 }, + }, + + // ── fields / offset ───────────────────────────────────────────────── + { + name: 'fields projects — a column not asked for does not come back', + query: { where: { id: 'a' }, fields: ['name'], limit: 1 }, + expectId: 'a', + expectSort: undefined, + expectKeys: ['id', 'name'], // `id` always kept, `_id` never + }, + { + name: 'offset skips, so an ordered walk can step past the first match', + query: { orderBy: [{ field: 'rank', order: 'asc' }], offset: 1, limit: 1 }, + expectId: 'd', // ranks asc → b, d (tied, id asc), c, a + expectSort: { rank: 1, id: 1 }, + expectSkip: 1, + }, + + // ── and the half that must stay ABSENT (#4363) ────────────────────── + { + name: 'an unsorted single-row lookup imposes no order — its limit: 1 is not a page', + query: { where: { id: 'a' }, limit: 1 }, + expectId: 'a', + expectSort: undefined, + }, + { + name: 'a miss is still null', + query: { where: { id: 'nope' }, limit: 1 }, + expectId: null, + expectSort: undefined, + }, +]; diff --git a/packages/plugins/driver-mongodb/src/mongodb-findone-options.test.ts b/packages/plugins/driver-mongodb/src/mongodb-findone-options.test.ts index d5a3c99fa8..73f84aa112 100644 --- a/packages/plugins/driver-mongodb/src/mongodb-findone-options.test.ts +++ b/packages/plugins/driver-mongodb/src/mongodb-findone-options.test.ts @@ -1,48 +1,101 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * objectstack#4419 — the exact `FindOptions` `MongoDBDriver.findOne` hands to - * MongoDB, asserted without a server. + * objectstack#4419 — what `MongoDBDriver.findOne` puts on the wire, asserted + * without a server. * * The defect this pins was invisible in the rows: `findOne` used to call * `collection.findOne(filter, { session, projection: { _id: 0 } })` — no * `sort`, no `skip`, no field projection — so `orderBy`, `offset` and `fields` - * were dropped between the contract and the wire. An untouched three-document + * were dropped between the contract and the wire. An untouched four-document * collection comes back in insertion order every time, which happens to be * stable, so a row-level assertion can pass against a driver that sorts nothing - * at all. The options object is where the bug is legible, so that is what this - * asserts. + * at all. The `FindOptions` object is where the bug is legible. * - * The companion suite `mongodb-findone-query.test.ts` proves the same behaviour - * end to end against a real mongod, and skips when the binary cannot be - * fetched. This one has no such dependency and therefore always runs — which is - * the point: a driver defect that only a downloadable binary can catch is a - * defect nobody catches on a restricted network. + * It runs the shared {@link FINDONE_CASES} twice over: + * + * 1. **What was emitted** — the `sort` / `projection` / `skip` handed to + * Mongo, including the cases whose expectation is that NO sort was imposed + * (#4363), which is not observable in rows at all. + * 2. **What those options select** — the same cases executed against an + * in-process collection that applies them by MongoDB's documented order + * (sort → skip → limit → project). This is what makes the expected ids + * falsifiable HERE rather than only on a CI runner that can download a + * mongod, and it is deliberately narrow: it is checking the case table, not + * standing in for the server. `mongodb-findone-query.test.ts` runs the same + * table against a real mongod for that. + * + * A stand-in more permissive than the real engine turns a suite into a green + * light for broken code — the hazard #4419 itself calls out. Hence the split: + * this file never decides whether the driver is correct, only whether the + * options and the expectations agree. */ import { describe, it, expect, beforeEach } from 'vitest'; import { MongoDBDriver } from './mongodb-driver.js'; +import { FINDONE_CASES, FINDONE_ROWS, type FindOneRow } from './mongodb-findone-cases.js'; interface Seen { filter: any; options: any } +/** Apply a Mongo sort spec (ordered keys, ±1) to a row list. */ +function applySort(rows: FindOneRow[], sort: Record | undefined): FindOneRow[] { + if (!sort) return rows; + return [...rows].sort((x, y) => { + for (const [key, dir] of Object.entries(sort)) { + const a = (x as any)[key]; + const b = (y as any)[key]; + if (a === b) continue; + return (a < b ? -1 : 1) * (dir as number); + } + return 0; + }); +} + +/** The `where` shapes {@link FINDONE_CASES} uses, as Mongo would match them. */ +function matches(row: FindOneRow, filter: any): boolean { + for (const [key, cond] of Object.entries(filter ?? {})) { + const value = (row as any)[key]; + if (cond && typeof cond === 'object' && '$in' in (cond as any)) { + if (!(cond as any).$in.includes(value)) return false; + } else if (value !== cond) { + return false; + } + } + return true; +} + /** * A driver wired to a fake `Db` — no connect(), no server. `getCollection` is * `this.db.collection(name)`, so replacing `db` is enough to observe every call - * the real code path makes. + * the real code path makes. (Spying on a Collection does NOT work: the real + * `getCollection` builds a fresh instance per call, which is what made the + * first version of this suite pass locally and fail on CI.) */ -function makeDriver() { +function makeDriver(rows: readonly FindOneRow[] = FINDONE_ROWS) { const seen: { findOne: Seen[]; find: Seen[] } = { findOne: [], find: [] }; - const rows: Record[] = []; + const run = (filter: any, options: any) => { + let out = applySort(rows.filter((r) => matches(r, filter)), options?.sort); + if (typeof options?.skip === 'number') out = out.slice(options.skip); + if (typeof options?.limit === 'number') out = out.slice(0, options.limit); + const projection = options?.projection ?? {}; + const keep = Object.keys(projection).filter((k) => projection[k] === 1); + if (keep.length === 0) return out.map((r) => ({ ...r })); + return out.map((r) => Object.fromEntries(keep.map((k) => [k, (r as any)[k]]))); + }; const collection = { - findOne: async (filter: any, options: any) => { seen.findOne.push({ filter, options }); return rows[0] ?? null; }, + findOne: async (filter: any, options: any) => { + seen.findOne.push({ filter, options }); + return run(filter, { ...options, limit: 1 })[0] ?? null; + }, find: (filter: any, options: any) => { seen.find.push({ filter, options }); - return { toArray: async () => rows }; + const out = run(filter, options); + return { toArray: async () => out }; }, }; const driver = new MongoDBDriver({ url: 'mongodb://unused/', database: 'unused' }); (driver as any).db = { collection: () => collection }; - return { driver, seen, rows }; + return { driver, seen }; } describe('MongoDBDriver.findOne hands Mongo the whole query (#4419)', () => { @@ -57,58 +110,56 @@ describe('MongoDBDriver.findOne hands Mongo the whole query (#4419)', () => { const lastFindOne = () => seen.findOne.at(-1)!; - it('translates the predicate, as it always did', async () => { - await driver.findOne('account', { object: 'account', where: { id: 'a' }, limit: 1 } as any); - expect(lastFindOne().filter).toMatchObject({ id: 'a' }); - }); + // ── 1. what the driver emitted ────────────────────────────────────── - // ── the three parameters that used to be dropped ───────────────────── + for (const c of FINDONE_CASES) { + it(`emits the right options — ${c.name}`, async () => { + await driver.findOne('account', { object: 'account', ...c.query } as any); + const { options } = lastFindOne(); - it('sends the caller ordering, with the id tie-breaker in the same direction', async () => { - await driver.findOne('account', { - object: 'account', orderBy: [{ field: 'rank', order: 'desc' }], limit: 1, - } as any); - expect(lastFindOne().options.sort).toEqual({ rank: -1, id: -1 }); - }); + // `undefined` here is a real assertion, not an absent one: it is the + // #4363 carve-out (no order imposed on an unsorted single-row lookup). + expect(options.sort).toEqual(c.expectSort); + expect(options.skip).toBe(c.expectSkip); - it('sends the field projection — `id` kept, `_id` dropped', async () => { - await driver.findOne('account', { - object: 'account', where: { id: 'a' }, fields: ['name'], limit: 1, - } as any); - expect(lastFindOne().options.projection).toEqual({ name: 1, id: 1, _id: 0 }); - }); + if (c.expectKeys) { + const projected = Object.entries(options.projection ?? {}) + .filter(([, v]) => v === 1) + .map(([k]) => k); + expect(new Set(projected)).toEqual(new Set(c.expectKeys)); + } + expect(options.projection?._id).toBe(0); // never leaks Mongo's own key + }); + } - it('sends the offset', async () => { - await driver.findOne('account', { - object: 'account', orderBy: [{ field: 'rank', order: 'asc' }], offset: 2, limit: 1, - } as any); - expect(lastFindOne().options.skip).toBe(2); + it('translates the predicate, as it always did', async () => { + await driver.findOne('account', { object: 'account', where: { id: 'a' }, limit: 1 } as any); + expect(lastFindOne().filter).toMatchObject({ id: 'a' }); }); - it('all three at once, on one query', async () => { - await driver.findOne('account', { - object: 'account', - where: { status: 'open' }, - orderBy: [{ field: 'created_at', order: 'desc' }], - fields: ['id', 'name'], - offset: 1, - limit: 1, - } as any); - const { filter, options } = lastFindOne(); - expect(filter).toMatchObject({ status: 'open' }); - expect(options.sort).toEqual({ created_at: -1, id: -1 }); - expect(options.projection).toEqual({ id: 1, name: 1, _id: 0 }); - expect(options.skip).toBe(1); + it('the transaction session still rides through', async () => { + const session = { id: 'sess-1' } as any; + await driver.findOne( + 'account', + { object: 'account', where: { id: 'a' }, limit: 1 } as any, + { transaction: session } as any, + ); + expect(lastFindOne().options.session).toBe(session); }); - // ── and the half that must stay absent (#4363) ─────────────────────── + // ── 2. what those options select ──────────────────────────────────── - it('imposes NO sort on an unordered findOne — its limit: 1 is not a page', async () => { - await driver.findOne('account', { object: 'account', where: { id: 'a' }, limit: 1 } as any); - expect(lastFindOne().options.sort).toBeUndefined(); - }); + for (const c of FINDONE_CASES) { + it(`selects the right row — ${c.name}`, async () => { + const row = await driver.findOne('account', { object: 'account', ...c.query } as any); + expect(row === null ? null : String((row as any).id)).toBe(c.expectId); + if (c.expectKeys && row) expect(new Set(Object.keys(row))).toEqual(new Set(c.expectKeys)); + }); + } - it('an unordered PAGED find still gets one — the carve-out is findOne-only', async () => { + // ── find() is unchanged: the carve-out is findOne-only ────────────── + + it('an unordered PAGED find still gets a deterministic order imposed', async () => { await driver.find('account', { object: 'account', limit: 2, offset: 0 } as any); expect(seen.find.at(-1)!.options.sort).toEqual({ id: 1 }); }); @@ -117,14 +168,4 @@ describe('MongoDBDriver.findOne hands Mongo the whole query (#4419)', () => { await driver.find('account', { object: 'account' } as any); expect(seen.find.at(-1)!.options.sort).toBeUndefined(); }); - - it('the transaction session still rides through', async () => { - const session = { id: 'sess-1' } as any; - await driver.findOne( - 'account', - { object: 'account', where: { id: 'a' }, limit: 1 } as any, - { transaction: session } as any, - ); - expect(lastFindOne().options.session).toBe(session); - }); }); diff --git a/packages/plugins/driver-mongodb/src/mongodb-findone-query.test.ts b/packages/plugins/driver-mongodb/src/mongodb-findone-query.test.ts index 7512903b07..10af39255e 100644 --- a/packages/plugins/driver-mongodb/src/mongodb-findone-query.test.ts +++ b/packages/plugins/driver-mongodb/src/mongodb-findone-query.test.ts @@ -1,8 +1,8 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * objectstack#4419 — `MongoDBDriver.findOne` executes the whole QueryAST, not - * just its `where`. + * objectstack#4419 — `MongoDBDriver.findOne` executes the whole QueryAST + * against a REAL mongod, not just its `where`. * * It used to issue `collection.findOne(translateFilter(query.where), { * projection: { _id: 0 } })` and nothing else: `orderBy`, `fields` and `offset` @@ -17,21 +17,21 @@ * no predicate to pass `orderBy` instead ("the first record in THIS order"). An * escape hatch one backend silently ignores is not an escape hatch. * - * The other half is what must NOT happen: no ordering is imposed when the - * caller asks for none (objectstack#4363). The engine sends `limit: 1`, which - * the paged-read rule cannot tell from "page one with page size 1", and the two - * want opposite things — `findOne` promises *a* matching record, never a - * position in a sequence, so an imposed `sort` only costs it the plan its - * predicate earned. + * The cases come from {@link FINDONE_CASES}, shared with + * `mongodb-findone-options.test.ts` so the two halves cannot drift. This half + * answers the one question the other cannot: does MongoDB agree? It therefore + * asserts ROWS only — the "no sort was imposed" cases are checked over there, + * because an untouched collection comes back in insertion order whether or not + * a sort went out. * - * Runs against a real mongod via `mongodb-memory-server`, and skips itself when - * the binary cannot be fetched — the convention the other suites in this - * package already use. + * Skips itself when the mongod binary cannot be fetched — the convention the + * other suites in this package already use. A skip is not a pass. */ -import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { MongoMemoryServer } from 'mongodb-memory-server'; import { MongoDBDriver } from './mongodb-driver.js'; +import { FINDONE_CASES, FINDONE_ROWS } from './mongodb-findone-cases.js'; let sharedMongod: MongoMemoryServer | undefined; try { @@ -43,12 +43,6 @@ try { ); } -const ROWS = [ - { id: 'a', name: 'Alpha', rank: 3, secret: 'sa' }, - { id: 'b', name: 'Bravo', rank: 1, secret: 'sb' }, - { id: 'c', name: 'Charlie', rank: 2, secret: 'sc' }, -]; - describe.skipIf(!sharedMongod)('driver-mongodb — findOne executes the whole query', () => { const mongod = sharedMongod as MongoMemoryServer; let driver: MongoDBDriver; @@ -56,7 +50,7 @@ describe.skipIf(!sharedMongod)('driver-mongodb — findOne executes the whole qu beforeAll(async () => { driver = new MongoDBDriver({ url: mongod.getUri(), database: 'findone_query' }); await driver.connect(); - for (const row of ROWS) await driver.create('account', { ...row }); + for (const row of FINDONE_ROWS) await driver.create('account', { ...row }); }, 90_000); afterAll(async () => { @@ -64,88 +58,21 @@ describe.skipIf(!sharedMongod)('driver-mongodb — findOne executes the whole qu if (sharedMongod) await sharedMongod.stop(); }); - // ── orderBy: the half that used to vanish ─────────────────────────── - - it.each([ - ['asc', 'b'], - ['desc', 'a'], - ])('findOne({orderBy rank %s}) returns the extreme row, not an arbitrary one', async (order, expected) => { - const row = await driver.findOne('account', { - object: 'account', - orderBy: [{ field: 'rank', order: order as 'asc' | 'desc' }], - limit: 1, - } as any); - expect(row?.id).toBe(expected); - }); - - it('orderBy composes with a predicate — the newest of the MATCHING rows', async () => { - const row = await driver.findOne('account', { - object: 'account', - where: { id: { $in: ['a', 'c'] } }, - orderBy: [{ field: 'rank', order: 'asc' }], - limit: 1, - } as any); - expect(row?.id).toBe('c'); // rank 2 < rank 3 - }); - - // ── fields / offset ───────────────────────────────────────────────── - - it('findOne({fields}) projects — a column not asked for does not come back', async () => { - const row = await driver.findOne('account', { - object: 'account', where: { id: 'a' }, fields: ['name'], limit: 1, - } as any); - expect(row).toEqual({ id: 'a', name: 'Alpha' }); // `id` always kept, `_id` never - expect(row).not.toHaveProperty('secret'); - }); - - it('findOne({offset}) skips, so an ordered walk can step past the first match', async () => { - const row = await driver.findOne('account', { - object: 'account', orderBy: [{ field: 'rank', order: 'asc' }], offset: 1, limit: 1, - } as any); - expect(row?.id).toBe('c'); // ranks asc → b, c, a - }); - - // ── and the half that must stay absent (#4363) ─────────────────────── - - it('imposes NO sort when the caller asked for none — the limit: 1 is not a page', async () => { - const collection = (driver as any).getCollection('account'); - const spy = vi.spyOn(collection, 'findOne'); - try { - await driver.findOne('account', { object: 'account', where: { id: 'a' }, limit: 1 } as any); - expect(spy).toHaveBeenCalledTimes(1); - expect(spy.mock.calls[0][1]).not.toHaveProperty('sort'); - } finally { - spy.mockRestore(); - } - }); - - it('a caller-supplied sort still gets the id tie-breaker appended', async () => { - const collection = (driver as any).getCollection('account'); - const spy = vi.spyOn(collection, 'findOne'); - try { - await driver.findOne('account', { - object: 'account', orderBy: [{ field: 'rank', order: 'desc' }], limit: 1, - } as any); - expect(spy.mock.calls[0][1]?.sort).toEqual({ rank: -1, id: -1 }); - } finally { - spy.mockRestore(); + for (const c of FINDONE_CASES) { + it(c.name, async () => { + const row = await driver.findOne('account', { object: 'account', ...c.query } as any); + expect(row === null ? null : String((row as any).id)).toBe(c.expectId); + if (c.expectKeys && row) expect(new Set(Object.keys(row))).toEqual(new Set(c.expectKeys)); + }); + } + + it('find() is unchanged — a paged walk is still a partition of the rows', async () => { + const seen: string[] = []; + for (let offset = 0; offset < FINDONE_ROWS.length; offset += 2) { + const page = await driver.find('account', { object: 'account', limit: 2, offset } as any); + seen.push(...page.map((r) => String(r.id))); } - }); - - it('find() is unchanged — a paged read still gets its deterministic order', async () => { - const collection = (driver as any).getCollection('account'); - const spy = vi.spyOn(collection, 'find'); - try { - await driver.find('account', { object: 'account', limit: 2, offset: 0 } as any); - expect(spy.mock.calls[0][1]?.sort).toEqual({ id: 1 }); - } finally { - spy.mockRestore(); - } - }); - - it('a miss is still null', async () => { - expect(await driver.findOne('account', { - object: 'account', where: { id: 'nope' }, limit: 1, - } as any)).toBeNull(); + expect(seen).toHaveLength(FINDONE_ROWS.length); + expect(new Set(seen).size).toBe(FINDONE_ROWS.length); // none served twice, none missed }); });