fix(metadata)!: a $filter array that is not a filter AST is rejected, not passed through (#4121)

.changeset/reject-malformed-filter-array.md

@@ -0,0 +1,47 @@
+---
+"@objectstack/metadata-protocol": minor
+---
+
+fix(metadata)!: a `$filter` array that is not a filter AST is rejected, not passed through (#4121)
+
+`isFilterAST` was being read as a *conversion* gate: an array it refused was
+assigned to `options.where` unconverted, leaving each backend to make sense of a
+value the protocol had already decided it could not parse.
+
+Item 2 of #3948, filed as error-locality work. The investigation found it is
+more than that.
+
+**It closes the last silently-unfiltered shape.** #3948 made the drivers throw
+on a bare triple with an unknown operator and on any element that is neither a
+join keyword nor a condition array. What it could not reach is a lone `['and']`
+or `['or']`: the driver sets its join mode, matches no element, emits **no
+predicate**, and returns every row. `isFilterAST` refuses it (a logical node
+needs `length >= 2`), so it arrived as an opaque `where` and no driver-side
+check applied. That is now a 400.
+
+**For every other shape this is not a narrowing.** driver-sql throws on all of
+them, driver-memory throws, driver-mongodb reaches its own parser and fails at
+the server. Rejecting at the protocol changes *which* error the caller sees, not
+*whether* there is one — and the message is in the request's own vocabulary
+(`unrecognised operator "not in"`, `element 1 is number`, plus the recognised
+operator list) rather than a driver's internal builder state.
+
+Scoped narrowly, because the regression to fear is rejecting something valid:
+
+- only `Array.isArray(filter)` values are in scope — a `where` **object** is
+  untouched, including `$and`/`$or`/`$gte` shapes;
+- an empty `[]` is left alone: it means "no filter", and every path already
+  treats it that way;
+- `isFilterAST` accepts nested arrays, so `[[a,'=',1],[b,'=',2]]` and
+  `['and', […], ['or', …]]` keep converting. A naive "arrays are suspect" rule
+  would have broken exactly those, which is why the accepted shapes are pinned
+  by more tests than the rejected ones.
+
+Errors carry `status: 400` and `code: 'INVALID_FILTER'`, matching the
+`UNSUPPORTED_QUERY_PARAM` convention alongside.
+
+Verified: 12 new tests driving the real `findData` normalisation, not a
+re-implementation of its rule — six for shapes that must keep converting, six
+for shapes that must be rejected, including the exact message text. Reverting the
+change fails six of them. Full `@objectstack/metadata-protocol` suite: **122
+tests across 19 files**, green.

packages/metadata-protocol/src/protocol.malformed-filter.test.ts

@@ -0,0 +1,149 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * A `$filter` that looks like a filter AST but is not one is rejected at the
+ * protocol, not handed to the driver as an opaque `where` (#4121).
+ *
+ * `isFilterAST` was being read as a *conversion* gate: an array it refused was
+ * assigned to `options.where` unconverted, leaving each backend to make sense of
+ * it. #3948 made the drivers throw rather than skip, so rejecting here buys
+ * (a) one error instead of a different one per backend, (b) a message in the
+ * *request's* vocabulary rather than a builder's internal state, and (c) the one
+ * shape the driver-side fix could not reach.
+ *
+ * That shape is a lone `['and']`: the driver sets its join mode, matches no
+ * element, emits no predicate, and returns **every row** — silently. It is the
+ * last survivor of the class #3948 closed, which makes this a correctness fix
+ * and not only an ergonomic one.
+ *
+ * The regression this change could plausibly cause is rejecting a filter that
+ * IS valid, so the accepted shapes are pinned at least as hard as the rejected
+ * ones. `isFilterAST` accepts nested arrays, and a naive "arrays are suspect"
+ * rule would have broken `[[a,'=',1],[b,'=',2]]`.
+ */
+import { describe, it, expect, vi } from 'vitest';
+import { ObjectStackProtocolImplementation } from './protocol.js';
+
+const SCHEMA = {
+    name: 'invoice',
+    fields: {
+        status: { name: 'status', type: 'text' },
+        close_date: { name: 'close_date', type: 'date' },
+        amount: { name: 'amount', type: 'number' },
+        stage: { name: 'stage', type: 'text' },
+        a: { name: 'a', type: 'number' },
+        b: { name: 'b', type: 'number' },
+        c: { name: 'c', type: 'number' },
+    },
+};
+
+function makeProtocol() {
+    const find = vi.fn(async () => []);
+    const engine = {
+        registry: { getObject: (n: string) => (n === 'invoice' ? SCHEMA : undefined) },
+        find,
+        count: vi.fn(async () => 0),
+    };
+    return { p: new ObjectStackProtocolImplementation(engine as any), find };
+}
+
+/** Run a `$filter` through the real `findData` normalisation. */
+async function whereFor(filter: unknown): Promise<unknown> {
+    const { p, find } = makeProtocol();
+    await p.findData({ object: 'invoice', query: { $filter: filter } } as never);
+    return (find.mock.calls[0] as unknown[])[1] as unknown;
+}
+
+async function errorFor(filter: unknown): Promise<{ message: string; status?: number; code?: string }> {
+    const { p } = makeProtocol();
+    try {
+        await p.findData({ object: 'invoice', query: { $filter: filter } } as never);
+    } catch (e) {
+        const err = e as Error & { status?: number; code?: string };
+        return { message: err.message, status: err.status, code: err.code };
+    }
+    throw new Error(`expected ${JSON.stringify(filter)} to be rejected`);
+}
+
+describe('filters that must keep working', () => {
+    it('converts a bare comparison triple', async () => {
+        expect(await whereFor(['status', '=', 'active'])).toMatchObject({ where: { status: 'active' } });
+    });
+
+    it('converts a flat list of conditions — the shape a naive rule would break', async () => {
+        const opts = await whereFor([['a', '=', 1], ['b', '=', 2]]) as { where: unknown };
+        expect(opts.where).toBeDefined();
+        expect(Array.isArray(opts.where)).toBe(false);
+    });
+
+    it('converts logical nodes, including nested ones', async () => {
+        for (const f of [
+            ['and', ['a', '=', 1], ['b', '=', 2]],
+            ['or', ['a', '=', 1], ['b', '=', 2]],
+            ['and', ['a', '=', 1], ['or', ['b', '=', 2], ['c', '=', 3]]],
+        ]) {
+            const opts = await whereFor(f) as { where: unknown };
+            expect(Array.isArray(opts.where), JSON.stringify(f)).toBe(false);
+        }
+    });
+
+    it('converts the date operators #3948 added to the AST vocabulary', async () => {
+        for (const op of ['before', 'after']) {
+            const opts = await whereFor(['close_date', op, '2024-01-01']) as { where: unknown };
+            expect(Array.isArray(opts.where), op).toBe(false);
+        }
+    });
+
+    it('leaves an empty filter alone — it means "no filter"', async () => {
+        const opts = await whereFor([]) as { where: unknown };
+        expect(opts.where).toEqual([]);
+    });
+
+    it('never touches a `where` object; only arrays are in scope', async () => {
+        for (const f of [{ status: 'active' }, { $and: [{ a: 1 }, { b: 2 }] }, { amount: { $gte: 100 } }]) {
+            const opts = await whereFor(f) as { where: unknown };
+            expect(opts.where).toEqual(f);
+        }
+    });
+});
+
+describe('filters that must be rejected', () => {
+    it('rejects a lone logical keyword — the remaining silent-unfiltered shape', async () => {
+        // The driver sets its join mode, matches nothing, emits no predicate,
+        // and returns every row. Nothing downstream can catch this.
+        for (const f of [['and'], ['or']]) {
+            const err = await errorFor(f);
+            expect(err.message).toContain('has no conditions to join');
+            expect(err.status).toBe(400);
+            expect(err.code).toBe('INVALID_FILTER');
+        }
+    });
+
+    it('rejects a triple whose operator is outside the AST vocabulary, and names it', async () => {
+        const err = await errorFor(['status', 'bogusop', 'x']);
+        expect(err.message).toContain('unrecognised operator "bogusop"');
+        expect(err.status).toBe(400);
+    });
+
+    it('rejects the space-spelled `not in`, which no vocabulary defines', async () => {
+        const err = await errorFor(['stage', 'not in', ['won', 'lost']]);
+        expect(err.message).toContain('unrecognised operator "not in"');
+    });
+
+    it('rejects an array of condition OBJECTS — a different filter dialect', async () => {
+        const err = await errorFor([{ field: 'a', operator: '=', value: 1 }]);
+        expect(err.status).toBe(400);
+    });
+
+    it('rejects a mixed array with a non-condition element, naming its position', async () => {
+        expect((await errorFor([['a', '=', 1], 42])).message).toContain('element 1 is number');
+        expect((await errorFor([['a', '=', 1], null])).message).toContain('element 1 is null');
+    });
+
+    it('quotes the recognised vocabulary, so the fix is in the error', async () => {
+        const err = await errorFor(['status', 'bogusop', 'x']);
+        expect(err.message).toContain('Recognised operators:');
+        expect(err.message).toContain('not_in');
+        expect(err.message).toContain('starts_with');
+    });
+});

packages/metadata-protocol/src/protocol.ts

@@ -18,7 +18,7 @@ import type {
 } from '@objectstack/spec/api';
 import type { MetadataCacheRequest, MetadataCacheResponse, ServiceInfo, ApiRoutes, WellKnownCapabilities } from '@objectstack/spec/api';
 import { readServiceSelfInfo } from '@objectstack/spec/api';
-import { parseFilterAST, isFilterAST, type DroppedFieldsEvent, type QueryAST } from '@objectstack/spec/data';
+import { parseFilterAST, isFilterAST, VALID_AST_OPERATORS, type DroppedFieldsEvent, type QueryAST } from '@objectstack/spec/data';
 import { PLURAL_TO_SINGULAR, SINGULAR_TO_PLURAL } from '@objectstack/spec/shared';
 import { type FormView, isAggregatedViewContainer } from '@objectstack/spec/ui';
 import { METADATA_FORM_REGISTRY } from '@objectstack/spec/system';
@@ -300,6 +300,59 @@ function resolveOverlaySchema(type: string, _item: unknown): z.ZodTypeAny | null
     return getMetadataTypeSchema(singular) ?? null;
 }
 
+/**
+ * A 400 for a `$filter` that looks like a filter AST but is not one.
+ *
+ * The message has to be *actionable from the request*, which is the whole point
+ * of rejecting here rather than letting a driver fail later: the caller sent a
+ * query parameter, so the error names the offending element and the vocabulary
+ * it was checked against — not a driver-internal builder state.
+ *
+ * Diagnoses the three shapes `isFilterAST` refuses, in the order they occur in
+ * practice. #4121.
+ */
+function malformedFilterError(filter: unknown[]): Error {
+    const detail = describeMalformedFilter(filter);
+    const err: any = new Error(
+        `Malformed $filter: ${detail} A filter array is a comparison ` +
+        `[field, operator, value], a logical node ["and"|"or", ...conditions], or a ` +
+        `list of those. Recognised operators: ${[...VALID_AST_OPERATORS].sort().join(', ')}.`,
+    );
+    err.status = 400;
+    err.code = 'INVALID_FILTER';
+    return err;
+}
+
+/** The specific reason a filter array failed `isFilterAST`, for the message above. */
+function describeMalformedFilter(filter: unknown[]): string {
+    const [first, second] = filter;
+    const isKeyword = typeof first === 'string' && ['and', 'or'].includes(first.toLowerCase());
+
+    // `["and"]` / `["or"]` with nothing to join. The one shape that still
+    // returned every row silently after #3948: the driver sets its join mode,
+    // matches no element, and emits no predicate.
+    if (isKeyword && filter.length < 2) {
+        return `logical node ["${String(first)}"] has no conditions to join.`;
+    }
+    // A bare triple whose operator is outside the AST vocabulary — the original
+    // `before` / `after` / `'not in'` case.
+    if (typeof first === 'string' && !isKeyword && typeof second === 'string'
+        && !VALID_AST_OPERATORS.has(second.toLowerCase())) {
+        return `unrecognised operator "${second}" in [${JSON.stringify(first)}, ...].`;
+    }
+    // An element that is neither a join keyword nor a nested condition.
+    const badIndex = filter.findIndex(
+        (item) => !Array.isArray(item)
+            && !(typeof item === 'string' && ['and', 'or'].includes(item.toLowerCase())),
+    );
+    if (badIndex >= 0 && filter.some((item) => Array.isArray(item))) {
+        const bad = filter[badIndex];
+        return `element ${badIndex} is ${bad === null ? 'null' : typeof bad}, ` +
+            `expected a condition array or a logical keyword.`;
+    }
+    return `${JSON.stringify(filter)} is not a recognised filter shape.`;
+}
+
 /**
  * Guarantee a `view` body carries a top-level `name`.
  *
@@ -3102,6 +3155,20 @@ export class ObjectStackProtocolImplementation implements
             // Filter AST array → FilterCondition object
             if (isFilterAST(parsedFilter)) {
                 parsedFilter = parseFilterAST(parsedFilter);
+            } else if (Array.isArray(parsedFilter) && parsedFilter.length > 0) {
+                // `isFilterAST` was being read as a *conversion* gate, so an array
+                // it refused was assigned to `where` unconverted — an opaque value
+                // the driver then had to make sense of. Every driver now fails on
+                // it, so this is not a narrowing; it moves the failure to where the
+                // malformed filter actually arrived, with the request's own
+                // vocabulary in the message instead of a driver-internal one.
+                //
+                // It also closes what the driver-side fix could not: a lone
+                // `['and']` / `['or']` sets the join mode, matches no element, and
+                // emits NO predicate — the last shape that still returned every row
+                // silently after #3948. An empty `[]` is left alone: it means "no
+                // filter", and every path already treats it that way. #4121.
+                throw malformedFilterError(parsedFilter);
             }
             options.where = parsedFilter;
         }