fix(data): gate unknown fields on the explicit filter axes (#7534)

.changeset/rest-list-explicit-filter-unknown-field.md

@@ -0,0 +1,95 @@
+---
+"@objectstack/metadata-protocol": patch
+---
+
+fix(data): an unknown field inside `where` / `$filter` / a filter AST is rejected, not answered with an empty list (#7534)
+
+`POST /api/v1/data/showcase_invoice/query` with `{"where":{"not_a_field":"x"}}`
+answered `200 {"records":[],"total":0}` — no `code`, no mention of the unknown
+name — and identically through the `$filter` door and the filter-AST door. The
+bare-key door on the same object with the same field name, in the same run,
+answered `400 INVALID_FIELD`.
+
+So one endpoint family gave **two verdicts for one mistake**, chosen by which
+door the caller used, and the losing verdict is indistinguishable from "no
+data". That is the exact failure #4134 was filed about: an unknown name is
+lowered into a field-equality predicate that can only match zero rows.
+
+This is **not** a regression of #4134 — that gate still holds on the door it
+covers (measured at the branch point alongside the three failures). It is the
+sibling door its fix never reached: `assertQueryParamsAreFields` gated only the
+**implicit** filters `findData` derives from leftover query parameters, while
+the **explicit** axes reached the driver ungated — even though
+`resolveQueryFields` was written as "ONE resolution shared by all four read
+axes".
+
+**The gate.** A new `assertFilterFieldsExist` calls that same existing
+resolution — additively; `resolveQueryFields` itself is unchanged — on the
+normalized `where`. One call covers all three doors because they are not three
+code paths: `where` / `filter` / `filters` / `$filter` resolve to one slot at
+the #3795 fold, and a filter AST is lowered by `parseFilterAST` — the single
+sink for that sugar — before the gate runs. The gate therefore reads the same
+`FilterCondition` the driver will read, which is what keeps "the field the gate
+saw" from drifting away from "the column that reached the driver".
+
+Rejections carry the envelope the write path and the bare-key door already
+produce — `400 INVALID_FIELD` + `field` + `fields` + `object` — plus `param`
+naming the caller's own wire spelling (`$filter`, not `where`), and a message
+that states the zero-row consequence, since that is the part a caller cannot
+infer from a `200`.
+
+**Deliberately unchanged.**
+
+- **Precedence.** The gate runs *after* the #4134 param gate, so a request that
+  gets both a bare key and its filter wrong answers exactly as it did before;
+  and *before* the #4164 implicit/explicit merge, which is what still lets it
+  name the axis the caller actually used.
+- **Reach.** Structure is discarded — `$and` / `$or` / `$not` are recursed
+  into — but a field key's VALUE is not descended into: it is either an operator
+  bag (`{$gte: 18}`) or a nested-relation condition (`{owner_id: {region:
+  'NA'}}`) whose keys belong to a *different* object. Judging those against this
+  object's field map would refuse legitimate relation filters. A dotted path is
+  judged on its head segment, the same reach the bare-key door has on
+  `owner_id.name`. An unrecognised `$`-combinator is skipped without descending —
+  a hole rather than a false rejection, the right failure direction for a gate
+  that exists to stop wrong answers.
+- **The honest zero.** A real field that genuinely matches nothing is still a
+  `200` with `total: 0`. A filter that cannot be *run* at all is still
+  `INVALID_FILTER` (#4121 / #4181), which answers first; this gate answers only
+  "does this field exist".
+
+## Upgrade note — data import: a `matchField` naming no field now fails the row
+
+The gate sits at the `findData` ingress, so it also reaches the record-matching
+lookup the CSV/JSON import runner performs for `update` and `upsert` writes.
+This is a **user-visible behaviour change on a second surface**, and it is
+deliberate — the ruling on #7534 was to keep it rather than exempt the import
+path.
+
+**Before.** A `matchFields` entry naming a field the target object does not have
+produced a filter that could only match zero rows. The lookup read that as
+`'none'` — "no existing record" — and an `upsert` therefore fell through to a
+**create**. The import reported success while writing duplicate rows the caller
+believed were being matched and updated, and nothing in the response
+distinguished that from a genuinely new record.
+
+**Now.** That row fails with `400 INVALID_FIELD` naming the field. The failure
+is contained by the row loop's own `try`/`catch`, so it is reported as one
+failed row in the import results and **the rest of the import proceeds** — it is
+not an aborted job.
+
+**Remedy.** Correct the `matchFields` name to a field that exists on the object.
+The rejection names the offending field and, when it reads like a typo, suggests
+the closest real field name.
+
+Exempting the import path would have meant *adding* code — catching
+`INVALID_FIELD` and restoring `'none'` — to preserve a silent data-correctness
+bug of exactly the family this change closes, so the invariant is restored
+instead.
+
+**Unaffected: reference resolution.** The import runner's `resolveRef` probes
+candidate display fields (`name`, `title`, `label`, `full_name`, `email`,
+`username`) that legitimately may not exist on the object being referenced, and
+it already wraps each probe in a deliberate `catch` that moves on to the next
+candidate. A `400` lands exactly where the empty result did, so reference
+resolution behaves as before.

packages/metadata-protocol/src/protocol.ts

@@ -2174,6 +2174,69 @@ function suggestFieldName(name: string, knownFields: readonly string[]): string
     return '';
 }
 
+/**
+ * [#7534] The logical combinators a `FilterCondition` may carry. These hold
+ * NESTED CONDITIONS rather than naming a field, so {@link collectFilterFieldKeys}
+ * descends through them instead of judging them.
+ *
+ * Exactly the three the contract declares (`FilterConditionSchema`,
+ * `@objectstack/spec`) — `$and` / `$or` / `$not`. `$nor` is deliberately absent:
+ * it is a driver-INTERNAL lowering (`driver-memory` rewrites an input `$not`
+ * into a one-operand `$nor`, MongoDB's document-level negation) and is REFUSED
+ * as input vocabulary by that same driver, so a `$nor` arriving on the wire is
+ * not a combinator this layer should silently descend into.
+ */
+const FILTER_LOGICAL_KEYS: ReadonlySet<string> = new Set(['$and', '$or', '$not']);
+
+/**
+ * [#7534] Every key of a `FilterCondition` that NAMES A FIELD, structure
+ * discarded — whether a predicate sits under an `$or` changes nothing about
+ * whether its column exists.
+ *
+ * Two rules, and both are deliberately conservative in the direction that
+ * cannot invent a rejection:
+ *
+ * - **A `$`-prefixed key is never a field.** `$and`/`$or`/`$not` are recursed
+ *   into; any OTHER `$` key is skipped WITHOUT descending. An unrecognised
+ *   combinator therefore leaves the fields beneath it ungated — a hole, not a
+ *   false 400 — which is the right failure direction for a gate whose whole
+ *   purpose is to stop wrong answers, not to invent new ones.
+ * - **A field key's VALUE is not descended into.** It is either an operator bag
+ *   (`{$gte: 18}`) or a nested-relation condition (`{owner: {region: 'NA'}}`),
+ *   and the latter's keys belong to a DIFFERENT object whose field map this
+ *   gate has not resolved. Judging them against THIS object's fields would
+ *   refuse legitimate relation filters. The head segment — `owner` — is a field
+ *   of this object and IS judged, which is the same reach
+ *   {@link ObjectStackProtocolImplementation.assertQueryParamsAreFields} has on
+ *   a dotted path (`owner_id.name`).
+ *
+ * `depth` is a cheap backstop against a self-referential `where`. JSON cannot
+ * produce one, but `POST /data/:object/query` is not the only door — the RPC
+ * dispatcher and in-process callers hand over live objects — and a gate that
+ * can hang the read path is worse than the defect it closes.
+ */
+function collectFilterFieldKeys(
+    where: unknown,
+    out: string[] = [],
+    depth = 0,
+): string[] {
+    if (depth > 32) return out;
+    if (!where || typeof where !== 'object' || Array.isArray(where)) return out;
+    for (const [key, value] of Object.entries(where as Record<string, unknown>)) {
+        if (key.startsWith('$')) {
+            if (!FILTER_LOGICAL_KEYS.has(key)) continue;
+            if (Array.isArray(value)) {
+                for (const arm of value) collectFilterFieldKeys(arm, out, depth + 1);
+            } else {
+                collectFilterFieldKeys(value, out, depth + 1);
+            }
+            continue;
+        }
+        out.push(key);
+    }
+    return out;
+}
+
 /**
  * Service Configuration for Discovery
  * Maps service names to their routes and plugin providers.
@@ -5183,6 +5246,84 @@ export class ObjectStackProtocolImplementation implements
         throw err;
     }
 
+    /**
+     * [#7534] The same read-path gate, on the EXPLICIT filter axes — the `where`
+     * object, the `$filter` string and the filter AST.
+     *
+     * #4134 closed this defect for the filters `findData` DERIVES from leftover
+     * query parameters, and {@link resolveQueryFields} was written for "ONE
+     * resolution shared by all four read axes". The explicit axes never called
+     * it, so one endpoint family answered ONE mistake two ways, chosen by which
+     * door the caller used:
+     *
+     * ```
+     * GET  /data/showcase_invoice?not_a_field=x            -> 400 INVALID_FIELD
+     * POST /data/showcase_invoice/query {where:{not_a_field:'x'}} -> 200 {records:[],total:0}
+     * ```
+     *
+     * The losing answer is the exact failure #4134 was filed about: an unknown
+     * name lowers into a field-equality predicate that can only match zero rows,
+     * so the response is indistinguishable from "no data" — and it cost a real
+     * investigation once already, where an empty list was read as an RLS /
+     * org-scope visibility bug rather than a typo.
+     *
+     * ONE call covers all three doors because they are not three code paths:
+     * `where` / `filter` / `filters` / `$filter` resolve to one slot at the
+     * #3795 fold, and a filter AST is lowered by `parseFilterAST` — the single
+     * sink for that sugar — before this runs. So this gate reads the same
+     * `FilterCondition` the driver will read, which is what keeps "the field the
+     * gate saw" and "the column that reached the driver" from drifting apart.
+     *
+     * # Ordering: after the #4134 param gate, before the #4164 merge
+     *
+     * Deliberately NOT reordered relative to its siblings. Running it AFTER
+     * {@link assertQueryParamsAreFields} keeps that gate's verdict first when a
+     * request gets both wrong, so no existing precedence moves; running it
+     * BEFORE the #4164 implicit/explicit merge is what lets it name the axis the
+     * caller actually used, since after the merge the two are one `$and` and the
+     * distinction is gone.
+     *
+     * # What it does NOT do
+     *
+     * The `param` in the message is the caller's own wire spelling (#4226's
+     * discipline — telling someone who sent `?$filter=…` that "'where' is
+     * invalid" names a parameter absent from their request). The message states
+     * the zero-row consequence rather than just the bad name, because that is
+     * the part a caller cannot infer from a `200`.
+     *
+     * Value shapes are NOT judged here: a wrong-typed or unrunnable filter is
+     * `INVALID_FILTER`'s job (#4121 / #4181), already answered upstream in this
+     * same block. This gate answers exactly one question — does this field
+     * exist — with exactly the envelope the write path and the bare-key door
+     * already give it.
+     */
+    private assertFilterFieldsExist(object: string, where: unknown, param: string): void {
+        if (!where || typeof where !== 'object') return;
+        const names = collectFilterFieldKeys(where);
+        if (names.length === 0) return;
+        const gate = this.resolveQueryFields(object);
+        if (!gate) return;
+        // Head segment only, exactly as the bare-key door judges `owner_id.name`.
+        const unknown = names.filter((f) => !gate.known.has(f.split('.')[0]));
+        if (unknown.length === 0) return;
+        const first = unknown[0];
+        const err: any = new Error(
+            `Query parameter '${param}' filters on '${first}', which is not a field on object `
+            + `'${object}'`
+            + (unknown.length > 1 ? ` (also: ${unknown.slice(1).join(', ')})` : '')
+            + '. A filter on a field that does not exist can only match zero records, so the '
+            + 'query was refused instead of answered with an empty list.'
+            + suggestFieldName(first, gate.declared),
+        );
+        err.code = 'INVALID_FIELD';
+        err.status = 400;
+        err.field = first;
+        err.fields = unknown;
+        err.object = object;
+        err.param = param;
+        throw err;
+    }
+
     /**
      * [#4226] SORT axis. A sort naming a field the object does not have is
      * refused (`400 INVALID_SORT`) instead of being dropped on the floor.
@@ -6293,6 +6434,15 @@ export class ObjectStackProtocolImplementation implements
             this.assertQueryParamsAreFields(request.object, leftoverParams);
         }
 
+        // [#7534] The same question, on the EXPLICIT filter the caller wrote —
+        // the sibling door #4134's fix never reached. `options.where` is a
+        // lowered `FilterCondition` by this point whichever of the three doors
+        // carried it (`where` object, `$filter` string, filter AST), so one call
+        // covers all three. Placed here, and not earlier, on purpose: see
+        // `assertFilterFieldsExist` for why it runs after the param gate above
+        // and before the #4164 merge below.
+        this.assertFilterFieldsExist(request.object, options.where, filterKey);
+
         // Flat field filters: REST-style query params like ?id=abc&status=open
         // are implicit field-level equality predicates. Every leftover key is a
         // verified field name by this point — the #4134 gate above runs FIRST,