fix(data): a filter the server cannot apply is rejected, not silently ignored (#4181)

.changeset/rest-list-malformed-filter-rejected.md

@@ -0,0 +1,48 @@
+---
+"@objectstack/metadata-protocol": patch
+"@objectstack/rest": patch
+---
+
+fix(data): a filter the server cannot apply is rejected, not silently ignored (#4181)
+
+`GET /api/v1/data/:object?filter={status:done` — one missing quote — answered
+`200` with the **unfiltered** page. The JSON-parse tolerance
+(`catch { /* keep as-is */ }`) left the raw string on `where`, a shape no
+driver consumes, so the filter was dropped whole and the response was
+byte-for-byte a successful unfiltered query. The worst failure direction in
+this family: #4134 returned nothing, #4164 dropped one predicate, this
+returned everything.
+
+The sibling `GET /data/:object/export` route had rejected the same input since
+it was written — the list path was the outlier. That guard now lives in the
+shared normalizer, so `GET /data/:object`, `POST /data/:object/query` and the
+runtime dispatcher all give one answer:
+
+- Unparseable JSON → `400 INVALID_FILTER`, naming the parameter and stating the
+  filter was not applied.
+- Parses but is not a filter (`?filter=5`, `?filter="done"`, `?filter=null`) →
+  same rejection; usable JSON is not a usable filter.
+- Blank `?filter=` → treated as absent, as before. No error.
+- `filter` / `filters` / `$filter` / `where` are four spellings of ONE slot.
+  Sending two with **different** values used to run one and discard the rest
+  silently; it is now `400 INVALID_REQUEST` (each value is a valid filter — the
+  *request* is ambiguous, so it does not share the malformed-filter code).
+  Redundant identical spellings pass.
+- `orderby` on the export route gets the same treatment — a sort that cannot be
+  parsed is refused rather than dropped (lower stakes than a filter: the row set
+  is unchanged, but a caller taking "latest N" got an arbitrary N).
+
+**One wire code for one condition.** #4121 landed `400 INVALID_FILTER` for
+malformed filter *arrays* on this same code path while this fix was in flight;
+the non-array rejections above use that code too, so a caller asking "did my
+filter run?" never has to know which branch caught it. The export route's
+filter guard moves from `INVALID_REQUEST` to `INVALID_FILTER` to match — a wire
+change on an existing route, and the reason it is worth making is that a client
+otherwise has to handle two codes for one condition depending on which URL it
+called. The route's `orderby` guard keeps `INVALID_REQUEST` (it is not a
+filter).
+
+**What changes for callers:** requests carrying a malformed filter now fail
+loudly instead of receiving every record. Every valid filter shape — JSON
+string, live object, `FilterCondition` AST array, and all four alias spellings
+used alone — is unaffected.

content/docs/api/data-api.mdx

@@ -19,7 +19,7 @@ Query records with filtering, sorting, selection, and pagination.
 |:----------|:---------|:------------|
 | `object` | path | Object name |
 | `select` | query | Comma-separated field names |
-| `filter` | query | Filter expression (JSON). `filters` also accepted for backward compatibility. |
+| `filter` | query | Filter expression (JSON). `filters` also accepted for backward compatibility. Malformed JSON is rejected with `400 INVALID_FILTER` — never ignored. |
 | `sort` | query | Sort expression (e.g. `name asc` or `-created_at`) |
 | `top` | query | Max records to return. No default — omitting it returns all matching records. |
 | `skip` | query | Offset |
@@ -60,6 +60,24 @@ Reserved names cannot double as implicit filters. An object with a field
 literally called `count`, `cursor`, `distinct`, `object`, `search` or `top`
 filters it through the explicit form (`?filter={"count":3}`).
 
+#### A filter either applies or fails — it is never ignored
+
+`filter`, `filters`, `$filter` and `where` are four spellings of one slot. A
+value the server cannot turn into a filter is rejected with
+`400 INVALID_FILTER` rather than dropped, because a dropped filter would
+return the **unfiltered** result set — a response indistinguishable from a
+successful query:
+
+| Request | Result |
+|:---|:---|
+| `?filter={"status":"done"}` | filter applies |
+| `?filter={status:done` (invalid JSON) | `400` — `filter must be valid JSON` |
+| `?filter=5`, `?filter="done"`, `?filter=null` | `400` — parses, but is not a filter |
+| `?filter=` (blank) | treated as absent — no filter, no error |
+| `where` and `filter` sent with **different** values | `400` — aliases for one slot; send exactly one |
+
+The same rule applies to `orderby` on `GET /data/:object/export`.
+
 **Response**:
 ```json
 {

content/docs/api/error-catalog.mdx

@@ -122,8 +122,15 @@ substitute. See the [Data API](/docs/api/data-api).
 **Retry:** `no_retry`
 
 ### `INVALID_FILTER`
-**Cause:** A filter operator or field in the `where` clause is invalid.  
-**Fix:** Use valid filter operators (`$eq`, `$ne`, `$gt`, `$lt`, `$in`, `$contains`, etc.).  
+**Cause:** The server could not turn the request's filter into something it can
+run. Covers an invalid operator or field in the `where` clause, a `$filter`
+array that is not a filter AST (a bad operator, a lone `["and"]`), a `filter`
+query parameter that is not valid JSON, and JSON that parses to something that
+is not a filter (`5`, `"done"`, `null`).  
+**Fix:** Use valid filter operators (`$eq`, `$ne`, `$gt`, `$lt`, `$in`, `$contains`, etc.) and a filter shape the [Data API](/docs/api/data-api) accepts — an object, or an array of `[field, operator, value]` conditions. The `error` message names the offending element.  
+**Why it is an error and not an empty result:** a filter the server cannot run
+is never silently skipped, because skipping it would return the **unfiltered**
+result set — a response indistinguishable from a successful query.  
 **Retry:** `no_retry`
 
 ### `INVALID_SORT`

packages/metadata-protocol/src/protocol.ts

@@ -301,17 +301,19 @@ function resolveOverlaySchema(type: string, _item: unknown): z.ZodTypeAny | null
 }
 
 /**
- * A 400 for a `$filter` that looks like a filter AST but is not one.
+ * A 400 for a `$filter` ARRAY 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.
+ * practice. #4121. Sibling of {@link unusableFilterError}, which covers the
+ * non-array ways a filter fails to become one (#4181); both emit
+ * `INVALID_FILTER` so the condition has one wire code however it was reached.
  */
-function malformedFilterError(filter: unknown[]): Error {
+function malformedFilterArrayError(filter: unknown[]): Error {
     const detail = describeMalformedFilter(filter);
     const err: any = new Error(
         `Malformed $filter: ${detail} A filter array is a comparison ` +
@@ -853,6 +855,33 @@ const ODATA_SPELLING: Readonly<Record<string, string>> = {
     filter: '$filter', select: '$select', expand: '$expand',
 };
 
+/**
+ * [#4181] A filter the normalizer cannot turn into a usable `FilterCondition`
+ * by any route other than the array shapes {@link malformedFilterArrayError}
+ * already diagnoses: unparseable JSON, or JSON that parses to something no
+ * driver can read (a number, a bare string, `null`).
+ *
+ * Carries `INVALID_FILTER` — the standard-catalog code (`errors.zod.ts`,
+ * "Invalid filter expression") that #4121 introduced on this same code path for
+ * the array case. One condition, one wire code, however the caller reached it:
+ * a `$filter` array with a bad operator and a `?filter=` that is not JSON are
+ * the same answer to the same question ("this filter cannot run").
+ *
+ * The message states the filter was NOT APPLIED, because that is the part a
+ * caller cannot infer: the pre-#4181 behavior was an ordinary-looking 200 over
+ * the unfiltered set.
+ */
+function unusableFilterError(param: string, detail: string): Error {
+    const err: any = new Error(
+        `Query parameter '${param}' ${detail}. It was not applied, and an unapplied `
+        + 'filter would have returned the unfiltered result set.',
+    );
+    err.status = 400;
+    err.code = 'INVALID_FILTER';
+    err.param = param;
+    return err;
+}
+
 /** Fold a parameter name to its near-miss lookup key. */
 function nearMissKey(name: string): string {
     return name.toLowerCase().replace(/[_-]/g, '');
@@ -3140,37 +3169,103 @@ export class ObjectStackProtocolImplementation implements
         }
         delete options.sort;
 
-        // Filter/filters/$filter → where: normalize all filter aliases
+        // Filter/filters/$filter → where: normalize all filter aliases.
+        //
+        // [#4181] These four names are FOUR SPELLINGS OF ONE SLOT (`filters` is
+        // documented as a deprecated alias of `filter`), so `??` picking the
+        // first non-null silently discarded the others: a body carrying both
+        // `where` and a different `filter` ran the `filter` and dropped the
+        // `where` with no signal. Two different values for one slot cannot be
+        // reconciled — merging them would invent an intent the caller never
+        // expressed, and picking one is the silent drop itself — so an
+        // ambiguous request is refused. Redundant identical spellings are
+        // harmless and pass.
+        const filterAliases = (['filter', 'filters', '$filter', 'where'] as const)
+            .filter((k) => options[k] !== undefined)
+            .map((k) => ({ key: k, value: options[k] }));
+        if (filterAliases.length > 1) {
+            const distinct = new Set(filterAliases.map((a) => JSON.stringify(a.value)));
+            if (distinct.size > 1) {
+                const err: any = new Error(
+                    `Conflicting filter parameters: ${filterAliases.map((a) => `'${a.key}'`).join(', ')} `
+                    + 'are aliases for the same filter and were given different values. Send exactly one.',
+                );
+                err.status = 400;
+                err.code = 'INVALID_REQUEST';
+                throw err;
+            }
+        }
+
         const filterValue = options.filter ?? options.filters ?? options.$filter ?? options.where;
+        const filterKey = filterAliases[0]?.key ?? 'filter';
         delete options.filter;
         delete options.filters;
         delete options.$filter;
 
         if (filterValue !== undefined) {
             let parsedFilter = filterValue;
-            // JSON string → object
-            if (typeof parsedFilter === 'string') {
-                try { parsedFilter = JSON.parse(parsedFilter); } catch { /* keep as-is */ }
-            }
-            // 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.
+            // A blank `?filter=` is ABSENT, not malformed — the same `length > 0`
+            // guard the export route applies before parsing. Deleting `where`
+            // here (rather than leaving `''` on it) is what lets every consumer
+            // below test presence with a plain falsy check.
+            if (typeof parsedFilter === 'string' && parsedFilter.trim() === '') {
+                delete options.where;
+            } else {
+                // [#4181] JSON string → object. Parse failure is a REJECTION, not
+                // a fallback. The `catch { /* keep as-is */ }` this replaces left
+                // the raw string on `where`, a shape no driver consumes — so the
+                // filter was dropped whole and `?filter={status:done` (one missing
+                // quote) answered 200 with the UNFILTERED page. Worst member of
+                // the #3948 family: #4134 zeroed, #4164 dropped one predicate,
+                // this returned everything.
                 //
-                // 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);
+                // The sibling `GET /data/:object/export` route has rejected this
+                // exact input since it was written (`400 INVALID_REQUEST`,
+                // "filter must be JSON"); the list path was the outlier. The
+                // guard lives HERE, in the shared normalizer, so `GET
+                // /data/:object`, `POST /data/:object/query` and the runtime
+                // dispatcher all inherit one answer instead of three.
+                if (typeof parsedFilter === 'string') {
+                    try {
+                        parsedFilter = JSON.parse(parsedFilter);
+                    } catch {
+                        throw unusableFilterError(filterKey, 'must be valid JSON');
+                    }
+                }
+                // Filter AST array → FilterCondition object
+                if (isFilterAST(parsedFilter)) {
+                    parsedFilter = parseFilterAST(parsedFilter);
+                } else if (Array.isArray(parsedFilter) && parsedFilter.length > 0) {
+                    // [#4121] `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 — so it falls through to the shape check below, which
+                    // passes it (an array IS an object).
+                    throw malformedFilterArrayError(parsedFilter);
+                }
+                // [#4181] Parsed-but-unusable is the same failure one step later:
+                // `?filter=5` / `?filter="open"` / `?filter=null` all yield a
+                // non-object `where` that no driver reads. #4121 above catches the
+                // array shapes; this catches the scalar ones, and together they are
+                // what lets the #4164 merge below trust `where` to be an object.
+                if (parsedFilter === null || typeof parsedFilter !== 'object') {
+                    throw unusableFilterError(
+                        filterKey,
+                        `must be a filter object or condition array, received ${parsedFilter === null ? 'null' : typeof parsedFilter}`,
+                    );
+                }
+                options.where = parsedFilter;
             }
-            options.where = parsedFilter;
         }
 
         // Populate/expand/$expand → expand (Record<string, QueryAST>)
@@ -3268,14 +3363,14 @@ export class ObjectStackProtocolImplementation implements
         // 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.
+        // #4164 shipped with a `typeof explicitWhere === 'object'` guard here,
+        // because the parse tolerance above could leave a raw unparseable string
+        // on `where` and folding real predicates into that garbage would neither
+        // apply them nor surface the bug. #4181 fixed that at the source — a
+        // filter now either parses to an object or 400s — so `where` is an
+        // object or absent by construction and the guard is gone with it.
         const explicitWhere = options.where;
-        const whereIsMergeable = !explicitWhere || typeof explicitWhere === 'object';
-        if (leftoverParams.length > 0 && whereIsMergeable) {
+        if (leftoverParams.length > 0) {
             const implicitFilters: Record<string, unknown> = {};
             for (const key of leftoverParams) {
                 implicitFilters[key] = options[key];