diff --git a/.changeset/rest-list-implicit-filter-and-merge.md b/.changeset/rest-list-implicit-filter-and-merge.md new file mode 100644 index 0000000000..c37cfc9ecf --- /dev/null +++ b/.changeset/rest-list-implicit-filter-and-merge.md @@ -0,0 +1,26 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(data): implicit field filters compose with an explicit `filter` by AND instead of being silently dropped (#4164) + +`GET /api/v1/data/:object?filter={"status":"open"}&owner_id=usr_1` used to +apply only the explicit filter: the bare `owner_id` predicate was neither +merged nor reported — it rode to the engine as a stray AST key no driver +reads, and the response over-returned. The mirror of #4134's silent zero, +same disease, opposite direction. + +The two now compose the way the request reads: `{ $and: [explicit, implicit] }` +— the same combinator the engine already uses to fold the `search` predicate +into an existing `where`, and one the cross-backend filter-logic conformance +suite pins. Contradictory sides (`?filter={"status":"open"}&status=closed`) +apply both predicates and intersect to an honest empty set. Pagination totals +(`total` / `hasMore`) are computed over the merged predicate, so they cannot +disagree with `records`. + +**What changes for callers:** requests that sent both an explicit `filter` and +bare field parameters now get the narrower, as-written result set instead of +the explicit filter alone. Requests sending only one of the two mechanisms are +unaffected. Thanks to #4134 (shipped previously), every bare parameter that +reaches the merge is a verified field name, so the merge can never introduce a +zero-matching predicate. diff --git a/content/docs/api/data-api.mdx b/content/docs/api/data-api.mdx index 9162b0abb5..b0eb2e0057 100644 --- a/content/docs/api/data-api.mdx +++ b/content/docs/api/data-api.mdx @@ -32,9 +32,12 @@ Query records with filtering, sorting, selection, and pagination. A query parameter the endpoint does not reserve is read as a field-level equality filter, so `?status=done` is shorthand for -`?filter={"status":"done"}`. Because such a parameter *is* a predicate, one -naming a field the object does not have could only ever match zero records — -so the endpoint rejects it instead of returning an empty page: +`?filter={"status":"done"}`. When an explicit `filter` is also present, the +two compose by AND — `?filter={"amount":{"$gte":100}}&status=done` applies +both predicates, the same way the `search` parameter composes with `filter`. +Because such a parameter *is* a predicate, one naming a field the object does +not have could only ever match zero records — so the endpoint rejects it +instead of returning an empty page: ```http GET /api/v1/data/showcase_task?pageSize=5 diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 229a6c3ac4..60d5aad8d0 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -3182,27 +3182,44 @@ export class ObjectStackProtocolImplementation implements } // Flat field filters: REST-style query params like ?id=abc&status=open - // After extracting all known query parameters, any remaining keys are - // treated as implicit field-level equality filters merged into `where`. - // Every one of them is a verified field name by this point. + // are implicit field-level equality predicates. Every leftover key is a + // verified field name by this point — the #4134 gate above runs FIRST, + // which is exactly what makes the merge below safe: an unknown name + // already 400'd, so nothing merged here can be a predicate that matches + // nothing. // - // The `!options.where` guard is #4134's deliberate scope edge: when an - // explicit filter won, a leftover REAL field name is still dropped - // silently (it rides on to `engine.find` as a stray AST key no driver - // reads). Same disease, opposite direction — over-returning rather than - // zeroing — and fixing it means choosing a semantics (AND-merge vs. - // reject the conflict), so it is filed as #4164 rather than smuggled in - // here. The UNKNOWN half is already loud: the gate above runs before - // this branch, so a bad name 400s either way. - if (!options.where) { + // [#4164] Implicit predicates now COMPOSE with an explicit `where` + // instead of being silently dropped. `?filter={...}&status=open` means + // what it says — both apply, `{ $and: [explicit, implicit] }` — the + // same composition the engine itself uses to fold the `$search` + // predicate or an expand's declared filter into an existing `where`, + // and a combinator the #3774 conformance suite pins across every + // FilterCondition backend. Contradictory sides need no special case: + // both predicates apply and the intersection is an HONEST zero (the + // filters ran), unlike the pre-#4134 zero (a filter that never ran). + // Consuming the keys here also stops them riding to `engine.find` as + // stray top-level AST junk, and count() below reads the same + // `options.where`, so pagination totals see the merged predicate too. + // + // Deliberately NOT merged: a non-object truthy `where` — the parse + // tolerance above keeps an unparseable `filter` JSON string as-is + // (#4181), and folding real predicates into that garbage would neither + // apply them nor surface the actual bug. That path keeps its pre-#4164 + // shape until the tolerance itself is fixed at the source. + const explicitWhere = options.where; + const whereIsMergeable = !explicitWhere || typeof explicitWhere === 'object'; + if (leftoverParams.length > 0 && whereIsMergeable) { const implicitFilters: Record = {}; for (const key of leftoverParams) { implicitFilters[key] = options[key]; delete options[key]; } - if (Object.keys(implicitFilters).length > 0) { - options.where = implicitFilters; - } + // An absent or empty explicit filter (`?filter={}`, `?filter=`) is + // vacuous — the implicit predicates stand alone rather than being + // wrapped in a one-armed `$and`. + options.where = !explicitWhere || Object.keys(explicitWhere).length === 0 + ? implicitFilters + : { $and: [explicitWhere, implicitFilters] }; } // Route to engine.aggregate() when the query has GROUP BY / aggregations. diff --git a/packages/objectql/src/protocol-data.test.ts b/packages/objectql/src/protocol-data.test.ts index d9d86e6442..7a9c956811 100644 --- a/packages/objectql/src/protocol-data.test.ts +++ b/packages/objectql/src/protocol-data.test.ts @@ -760,16 +760,99 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { ).rejects.toMatchObject({ field: 'zzzz', fields: ['zzzz', 'yyyy'] }); }); - it('leaves the known-field-plus-explicit-filter case alone (#4164)', async () => { - // The scope edge, pinned so a later change to it is deliberate: a - // REAL field name alongside an explicit filter is still dropped - // silently. #4134 made only the UNKNOWN half loud. + it('AND-merges a known field with an explicit filter instead of dropping it (#4164)', async () => { + // The #4134 pin test used to freeze the drop behavior here; #4164 + // is the deliberate change it was waiting for. Both predicates now + // apply, and the consumed key no longer rides to the engine as a + // stray top-level AST key. const { protocol, engine } = makeProtocol(); await protocol.findData({ object: 'showcase_task', query: { filter: { status: 'open' }, owner_id: 'usr_1' }, }); - expect(engine.find.mock.calls[0][1].where).toEqual({ status: 'open' }); + const opts = engine.find.mock.calls[0][1]; + expect(opts.where).toEqual({ $and: [{ status: 'open' }, { owner_id: 'usr_1' }] }); + expect(opts.owner_id).toBeUndefined(); + }); + + it('merges every implicit field as ONE $and arm', async () => { + const { protocol, engine } = makeProtocol(); + await protocol.findData({ + object: 'showcase_task', + query: { filter: { status: 'open' }, owner_id: 'usr_1', due_date: '2026-08-01' }, + }); + expect(engine.find.mock.calls[0][1].where).toEqual({ + $and: [{ status: 'open' }, { owner_id: 'usr_1', due_date: '2026-08-01' }], + }); + }); + + it('a contradictory pair applies both sides — an honest empty set, not a silent wide one', async () => { + // `?filter={"status":"open"}&status=closed` — both predicates run; + // the intersection being empty is the caller's contradiction, not a + // dropped filter. No special-casing of same-field collisions. + const { protocol, engine } = makeProtocol(); + await protocol.findData({ + object: 'showcase_task', + query: { filter: { status: 'open' }, status: 'closed' }, + }); + expect(engine.find.mock.calls[0][1].where).toEqual({ + $and: [{ status: 'open' }, { status: 'closed' }], + }); + }); + + it('a vacuous explicit filter ({} or empty string) lets implicit predicates stand alone', async () => { + const { protocol, engine } = makeProtocol(); + await protocol.findData({ + object: 'showcase_task', + query: { filter: {}, owner_id: 'usr_1' }, + }); + expect(engine.find.mock.calls[0][1].where).toEqual({ owner_id: 'usr_1' }); + // `?filter=` — the parse tolerance keeps '' as-is; the old guard + // treated it as no-filter, and the merge must keep doing so. + await protocol.findData({ + object: 'showcase_task', + query: { filter: '', owner_id: 'usr_1' }, + }); + expect(engine.find.mock.calls[1][1].where).toEqual({ owner_id: 'usr_1' }); + }); + + it('count() sees the merged where — pagination totals cannot disagree with records (#4164)', async () => { + const { protocol, engine } = makeProtocol(); + await protocol.findData({ + object: 'showcase_task', + query: { filter: { status: 'open' }, owner_id: 'usr_1', top: 5 }, + }); + expect(engine.count).toHaveBeenCalledTimes(1); + expect(engine.count.mock.calls[0][1].where).toEqual({ + $and: [{ status: 'open' }, { owner_id: 'usr_1' }], + }); + }); + + it('merges identically through the $filter JSON-string alias', async () => { + const { protocol, engine } = makeProtocol(); + await protocol.findData({ + object: 'showcase_task', + query: { $filter: JSON.stringify({ status: 'open' }), owner_id: 'usr_1' }, + }); + expect(engine.find.mock.calls[0][1].where).toEqual({ + $and: [{ status: 'open' }, { owner_id: 'usr_1' }], + }); + }); + + it('leaves a non-object where (unparseable filter JSON) untouched — pinned until #4181', async () => { + // `?filter={oops` — the parse tolerance keeps the raw string and it + // becomes `where`. Folding real predicates into that garbage would + // neither apply them nor surface the tolerance bug itself (#4181), + // so this path keeps its pre-#4164 shape: string where forwarded, + // implicit key left as a stray option. + const { protocol, engine } = makeProtocol(); + await protocol.findData({ + object: 'showcase_task', + query: { filter: '{oops', owner_id: 'usr_1' }, + }); + const opts = engine.find.mock.calls[0][1]; + expect(opts.where).toBe('{oops'); + expect(opts.owner_id).toBe('usr_1'); }); it('rejects even when an explicit filter rode along — the 400 must not depend on it', async () => { diff --git a/packages/objectql/src/protocol-unknown-query-param.test.ts b/packages/objectql/src/protocol-unknown-query-param.test.ts index 3ef1b273a3..38d5631574 100644 --- a/packages/objectql/src/protocol-unknown-query-param.test.ts +++ b/packages/objectql/src/protocol-unknown-query-param.test.ts @@ -46,6 +46,13 @@ function makeMemoryDriver() { const matchesWhere = (row: Record, where: any): boolean => { if (!where || typeof where !== 'object') return true; for (const [k, v] of Object.entries(where)) { + // `$and` must be honored, not skipped — #4164 composes the explicit + // filter and the implicit field predicates through it, so a matcher + // that ignores `$and` would green-light a merge that does nothing. + if (k === '$and' && Array.isArray(v)) { + if (!v.every((arm) => matchesWhere(row, arm))) return false; + continue; + } if (k.startsWith('$')) continue; const expected = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; const a = row[k] === undefined ? null : row[k]; @@ -206,4 +213,45 @@ describe('#4134 — unknown list query params (real ObjectQL engine)', () => { protocol.findData({ object: 'no_such_object', query: { pageSize: '5' } }), ).rejects.toMatchObject({ status: 404, code: 'OBJECT_NOT_FOUND' }); }); + + // ───────────────────────────────────────────────────────────── + // #4164 — implicit predicates compose with an explicit filter + // ───────────────────────────────────────────────────────────── + + it('a bare field param NARROWS an explicit filter instead of vanishing (#4164)', async () => { + // Pre-#4164 the `title` predicate was dropped and this returned all 8 + // open tasks — over-returning, the mirror of #4134's zeroing. + const r: any = await protocol.findData({ + object: 'showcase_task', + query: { filter: JSON.stringify({ status: 'open' }), title: 'Task 3' }, + }); + expect(r.total).toBe(1); + expect(r.records.map((x: any) => x.id)).toEqual(['t3']); + }); + + it('contradictory sides intersect to an honest zero — both filters ran', async () => { + // 'Task 3' is open, so requiring status=done alongside it matches + // nothing. Unlike the #4134 zero, every written predicate was applied. + const r: any = await protocol.findData({ + object: 'showcase_task', + query: { filter: JSON.stringify({ status: 'done' }), title: 'Task 3' }, + }); + expect(r.total).toBe(0); + }); + + it('pagination totals are computed over the MERGED predicate', async () => { + // 8 open rows share created_at; page of 3 → total must be 8 (the + // merged match count), not 10 (unfiltered) nor 3 (page-local). + const r: any = await protocol.findData({ + object: 'showcase_task', + query: { + filter: JSON.stringify({ status: 'open' }), + created_at: '2026-07-30T00:00:00.000Z', + top: 3, + }, + }); + expect(r.records).toHaveLength(3); + expect(r.total).toBe(8); + expect(r.hasMore).toBe(true); + }); });