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/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/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 2752438bba..faf4f79916 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -3244,6 +3244,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 @@ -3271,35 +3362,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; @@ -3391,6 +3454,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 })` @@ -3412,6 +3502,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-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 new file mode 100644 index 0000000000..73f84aa112 --- /dev/null +++ b/packages/plugins/driver-mongodb/src/mongodb-findone-options.test.ts @@ -0,0 +1,171 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * 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 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 `FindOptions` object is where the bug is legible. + * + * 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. (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(rows: readonly FindOneRow[] = FINDONE_ROWS) { + const seen: { findOne: Seen[]; find: Seen[] } = { findOne: [], find: [] }; + 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 run(filter, { ...options, limit: 1 })[0] ?? null; + }, + find: (filter: any, options: any) => { + seen.find.push({ filter, options }); + 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 }; +} + +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)!; + + // ── 1. what the driver emitted ────────────────────────────────────── + + 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(); + + // `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); + + 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('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('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); + }); + + // ── 2. what those options select ──────────────────────────────────── + + 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)); + }); + } + + // ── 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 }); + }); + + 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(); + }); +}); 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..10af39255e --- /dev/null +++ b/packages/plugins/driver-mongodb/src/mongodb-findone-query.test.ts @@ -0,0 +1,78 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * 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` + * 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 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. + * + * 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 } 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 { + 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)}`, + ); +} + +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 FINDONE_ROWS) await driver.create('account', { ...row }); + }, 90_000); + + afterAll(async () => { + if (driver) await driver.disconnect(); + if (sharedMongod) await sharedMongod.stop(); + }); + + 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))); + } + expect(seen).toHaveLength(FINDONE_ROWS.length); + expect(new Set(seen).size).toBe(FINDONE_ROWS.length); // none served twice, none missed + }); +}); 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' })`.