docs/M3_ARRAYFILTERS_DESIGN_REVIEW.md

@@ -0,0 +1,334 @@
+# M3 design review — `arrayFilters`, and the positional operators
+
+Written before any code, for the same reason the M2 review was: the previous
+two milestones both began with a gate that pointed somewhere other than the
+work, and both times the measurement said so before the implementation did.
+This one is no exception, but it fails in a new direction — the plan names a
+*missing feature*, and the measurement found a *data-loss bug*.
+
+Everything below was measured on 2026-08-10 against mongod 8.3.7 on
+`:27099` and this server at `5942f5e` on `:27020`, running the identical
+probe against both.
+
+---
+
+## 1. The plan names a feature; the measurement found data loss
+
+PLAN §3 lists `arrayFilters` in M3's scope, and PLAN §6 sizes it at "14 cases".
+That framing says: a feature is absent, and implementing it converts 14
+failures into passes.
+
+What is actually happening is worse and cheaper to describe. Take the corpus's
+own document and its own update:
+
+```js
+// { _id: 1, y: [ {b: 3}, {b: 1} ] }
+updateOne({}, {$set: {'y.$[i].b': 2}}, {arrayFilters: [{'i.b': 3}]})
+```
+
+| | result |
+|---|---|
+| mongod 8.3.7 | `{_id: 1, y: [{b: 2}, {b: 1}]}` |
+| this server | `{_id: 1, y: {"$[i]": {"b": 2}}}` |
+
+The array is gone. Not "not updated" — **replaced by a document whose single
+key is the literal text `$[i]`**, with every element and everything in them
+discarded. The server answers `ok: 1`, `matchedCount: 1`, `modifiedCount: 1`.
+No error, no warning, nothing in the log.
+
+This is the class M2 spent a milestone eliminating — a construct the engine
+does not implement answering confidently instead of refusing — except that
+here the wrong answer is not a `0` in a report, it is destruction of the
+client's stored data, and it is reachable by any unauthenticated client
+issuing a completely ordinary MongoDB update.
+
+### It is not an `arrayFilters` bug
+
+`arrayFilters` is not consulted at all. The string `arrayFilters` appears
+**nowhere in `src/`** — the option is accepted off the wire and dropped. The
+destruction comes from the *path*, and every path form that MongoDB gives a
+non-numeric array segment triggers it:
+
+| update | mongod | this server |
+|---|---|---|
+| `{$set: {'y.$[i].b': 2}}` + filters | `y: [{b:2},{b:1}]` | `y: {"$[i]": {"b": 2}}` |
+| `{$set: {'y.$[].b': 9}}` | `y: [{b:9},{b:9}]` | `y: {"$[]": {"b": 9}}` |
+| `{$set: {'y.$.b': 7}}` after `{'y.b': 3}` | `y: [{b:7},{b:1}]` | `y: {"$": {"b": 7}}` |
+| `{$inc: {'y.$[i].b': 10}}` + filters | `y: [{b:13},{b:1}]` | `y: {"$[i]": 10}` |
+
+The last row is worth a second look: `$inc` did not increment anything. It
+created a field and gave it the *operand*. So this is not `$set`-specific
+either — it is below the operator, in the path walk that every operator uses.
+
+**Three operators, one root cause.** `$` (positional, MongoDB 2.x),
+`$[]` (all-positional, 3.6) and `$[<ident>]` (filtered-positional, 3.6) all
+land in the same branch.
+
+### The branch
+
+`src/update.zig:283`, inside `set_path`, reached when a path segment under an
+array is not a number:
+
+```zig
+.array => |arr| {
+    const index = parse_index(segs[1]) orelse {
+        // treat as non-array: replace with a doc
+        var sub_pairs: std.ArrayListUnmanaged(bson.Pair) = .empty;
+        ...
+        pairs.items[idx].value = .{ .doc = try sub_pairs.toOwnedSlice(arena) };
+        return;
+    };
+```
+
+The comment states the behaviour plainly and is not wrong about what the code
+does. `parse_index("$[i]")` fails, so the array is treated as a thing that
+should have been a document, and is overwritten with one. For a genuinely
+unknown field name under an array — `y.nope.b` — that is arguably a defensible
+reading of MongoDB's create-intermediate-paths rule. For `$`, `$[]` and
+`$[i]`, which are the three ways MongoDB spells "descend into this array", it
+is the exact inversion of the intent.
+
+Out of 17 probe cases covering the operators, their edge cases and their
+refusals, **16 diverge from mongod and 1 agrees**. The one that agrees is
+`arrayFilters` supplied alongside a *replacement* document, where both servers
+ignore it.
+
+---
+
+## 2. Where the implementation actually is
+
+| | state |
+|---|---|
+| `arrayFilters` parsing | absent from `src/` entirely |
+| `$[<ident>]` | destroys the array |
+| `$[]` | destroys the array |
+| `$` positional | destroys the array |
+| `update.apply` signature | `(doc, update)` — no filter, no arrayFilters |
+| path model | `split_path` → `[]const []const u8`, max 16 segments |
+| target model | exactly one target per path, resolved while writing |
+| `modifiedCount` | byte comparison in `engine.replace` |
+
+Two of these rows are load-bearing for the design, and both are good news.
+
+**`modifiedCount` comes free.** `n_modified` is incremented only when
+`engine.replace` returns `.modified`, and that is decided by whether the
+serialized bytes changed — "a write that would store the same bytes is neither
+logged nor counted". So the moment the corruption stops, an update whose
+filters match nothing leaves the document byte-identical and is correctly
+reported as `modifiedCount: 0`. **5 of the 14 corpus failures are
+`modifiedCount` assertions and need no counting work at all.** (The runner
+reports the first failing assertion, so some of those 5 have a wrong outcome
+behind them too; the point is that none of them needs a *counter*.)
+
+**`update.apply` has no access to the filter.** This is the one real
+architectural fork, and it separates the three operators:
+
+- `$[]` and `$[<ident>]` need the document and the filter *list*. Both can be
+  passed down; nothing else is required.
+- `$` needs **which element the query matched** — information that exists only
+  during query evaluation and is thrown away before `apply` is called. Three
+  call sites (`cmd_update` at `commands.zig:1976`, `cmd_find_and_modify` at
+  `:2099`, and `:4131`) would each have to carry a matched-index out of the
+  match and into the update.
+
+So `$` is not "one more case of the same fix". It is the same *refusal*, but a
+different *implementation*, and it can be sequenced separately.
+
+---
+
+## 3. The gate named in the plan cannot see the worst of this
+
+M3's gate in PLAN §3 reads: *"remaining crud coverage; e2e3/e2e4 green"*.
+
+- **`e2e3.js` and `e2e4.js` contain zero occurrences** of `arrayFilters`, `$[`,
+  or a positional path. They are green today, they would stay green through
+  every version of this bug, and they will stay green after it is fixed. As a
+  gate for this work they measure nothing.
+- **The crud corpus covers `$[<ident>]` only.** Five files use it; four
+  contribute the 14 failures, and `client-bulkWrite-update-options.json`
+  is skipped entirely (needs server ≥ 8.0).
+- **The corpus contains no `$[]` case and no bare `$` case at all.** Grepped
+  the whole pinned unified corpus: zero.
+
+That last line is the finding. A fix scoped to what the gate measures would
+leave `$` and `$[]` still overwriting arrays with `{"$": {...}}`, and the
+scorecard would go green on 14 new passes while two of the three ways to
+destroy a client's data remained. The gate is not merely pointing away from
+the work — **it would certify the bug as fixed.**
+
+The 14, for the record:
+
+| file | cases | first failing assertion |
+|---|---|---|
+| `updateOne-arrayFilters.json` | 5 | 3 outcome, 2 modifiedCount |
+| `bulkWrite-arrayFilters.json` | 3 | 2 outcome, 1 modifiedCount |
+| `findOneAndUpdate-arrayFilters.json` | 3 | 3 outcome |
+| `updateMany-arrayFilters.json` | 3 | 1 outcome, 2 modifiedCount |
+
+---
+
+## 4. What the measurement says the fix has to look like
+
+Recorded because none of it is guessable, and because two of these directly
+contradict how `set_path` behaves today.
+
+### One path can name many targets
+
+```
+y.$[].c.$[].d   on   y: [ {c: [{d:1},{d:2}]}, {c: [{d:3}]} ]
+  -> y: [ {c: [{d:0},{d:0}]}, {c: [{d:0}]} ]
+```
+
+A genuine cross-product: three leaves, all written. `set_path`'s contract —
+walk to one place, write there — cannot express this. The path model has to
+become *enumerate the matching targets, then apply the operator to each*, which
+also means the resolution must sit **below** the operator dispatch, since
+`$unset` through `$[i]` works the same way (measured).
+
+### A positional segment never creates anything
+
+This is the sharp reversal. `$set: {'a.b': 1}` on a document without `a`
+creates the intermediate document — that is MongoDB's rule and `set_path`
+implements it. But:
+
+| situation | mongod |
+|---|---|
+| `y.$[i].b` where `y` is missing | error 2, "The path 'y' must exist in the document in order to apply array update" |
+| `y.$[i].b` where `y` is `5` | error 2, "Cannot apply array updates to non-array element y: 5" |
+| upsert with `$[i]`, nothing matched | error 2, same "path must exist" — the upsert does *not* get a special case |
+
+So a positional segment is a *filter over what is there*, never a constructor.
+The upsert row means no separate upsert path is needed: it falls out.
+
+### Identifiers are matched as a query against each element
+
+`arrayFilters: [{'i.b.q': 2}]` selects elements whose nested `b.q` is 2, and
+operators work (`{'i.b': {$gte: 0}}` matched every element). So each filter is
+an ordinary query document, rooted at the identifier, evaluated against each
+array element — `query.matches` should apply directly with the identifier
+stripped from the front of each key.
+
+### The refusals, measured
+
+| condition | code | message |
+|---|---|---|
+| identifier in path with no matching filter (including no `arrayFilters` at all) | 2 | `No array filter found for identifier 'k' in path 'y.$[k].b'` |
+| identifier is not lowercase-alphanumeric | 2 | `Error parsing array filter :: caused by :: The top-level field name must be an alphanumeric string beginning with a lowercase letter, found '1x'` |
+| the path's array does not exist | 2 | `The path 'y' must exist in the document in order to apply array update` |
+| the path's element is not an array | 2 | `Cannot apply array updates to non-array element y: 5` |
+| `$` when the query did not match the array | 2 | `Plan executor error during update :: caused by :: The positional operator did not find the match needed from the query.` |
+| a filter entry the update never uses | 9 | `The array filter for identifier 'i' was not used in the update { $set: { y.b: 2 } }` |
+| two filters with the same identifier | 9 | `Found multiple array filters with the same top-level field name i` |
+| a filter with no top-level field | 9 | `Cannot use an expression without a top-level field name in arrayFilters` |
+| a filter with two top-level fields | 9 | `Error parsing array filter :: caused by :: Expected a single top-level field name, found 'i' and 'j'` |
+| `arrayFilters` not an array | 14 | `BSON field 'update.updates.arrayFilters' is the wrong type 'string', expected type 'array'` |
+
+Mostly this splits as *9 = the `arrayFilters` array judged on its own*, *2 =
+anything needing the update document or the stored document*. **It is not a
+clean rule**, and it should not be tidied into one: "identifier is not
+lowercase-alphanumeric" carries the same `Error parsing array filter` prefix as
+the 9s and is nevertheless a 2. Recorded as measured. Inventing the rule and
+deriving the codes from it would get that row wrong, which is precisely the
+failure mode this repo has hit three times.
+
+`UpdateError` is a closed set consumed at three call sites that currently
+collapse everything to `bad_value(reply, "bad update")`. Ten distinct
+refusals with two codes and specific messages will not fit that shape; the
+error → reply mapping has to move or widen.
+
+---
+
+## 5. Gate options
+
+### Option A — the 14 corpus cases
+
+*"`updateOne`/`updateMany`/`findOneAndUpdate`/`bulkWrite`-arrayFilters: 0 fail."*
+
+Cheap, external, already written. And, per §3, it certifies the bug fixed
+while `$` and `$[]` still destroy arrays. It is the plan's implied gate and it
+is the one option the measurement rules out on its own.
+
+### Option B — the 14, plus a purpose-built positional corpus
+
+Option A plus a recorded corpus for the two operators the pinned suite forgot,
+built exactly like `tests/spec/aggregate/`: inputs authored in `sources/`,
+every expectation recorded from mongod 8.3.7 by a `record.js`, run through the
+shared runner with `--suite-dir`.
+
+That machinery exists and was built for this situation. The M2.5 corpus caught
+`$avg` over a no-numeric group returning `0` instead of `null` — a bug no
+hand-written expectation would have contained, because the author would have
+written down what they believed. The same argument applies here with more
+force: the beliefs about `$[]`, missing paths and upserts in §4 were wrong
+before I measured them.
+
+Cost: one `sources/positional.json`, one recorder run. The recorder is ~120
+lines and already written for aggregation; the update shape needs its own but
+the pattern transfers.
+
+### Option C — differential fuzzing against mongod
+
+Generate random documents and random positional updates, apply to both servers,
+compare. Strongest possible coverage of the target-enumeration logic, which is
+where the cross-product and the create-nothing rule will actually break.
+
+Rejected as *the gate* for the same reason M2 rejected it: it is not
+reproducible as a committed artifact, a red run names a random seed rather than
+a behaviour, and it needs a live mongod in the loop. Worth having as a
+one-off during development; not worth being the thing a milestone is judged on.
+
+### Option D — refuse, do not implement
+
+Detect `$`, `$[]` and `$[<ident>]` and answer BadValue instead of corrupting.
+Gate: the 14 cases fail *with the right code* rather than passing.
+
+This is not a joke option and it should not be dismissed quickly. It stops the
+data loss in roughly a day, it is the M2 doctrine applied exactly ("refuse what
+the engine does not implement rather than answering confidently"), and it can
+ship before the real implementation is designed. It leaves 14 cases red.
+
+---
+
+## 6. Recommendation
+
+**Option D first, as its own commit, then Option B.**
+
+The reasoning is the split between the two halves of this problem. They have
+very different urgency and very different sizes:
+
+- The data loss is a bug, it is remotely reachable, it needs no feature work to
+  fix, and every day it stays in `main` is a day the server can silently
+  destroy an array. Refusing costs a path scan and a `BadValue`.
+- The feature is a genuine redesign of the path model — one path to many
+  targets, resolution below operator dispatch, a create-nothing rule that
+  inverts the current one. It deserves to be designed against a corpus rather
+  than rushed to make a bug stop.
+
+Shipping D first also makes B's corpus honest: recorded against mongod while
+this server refuses cleanly, so every expectation in it is measured before a
+line of the implementation exists — the discipline that made M2.5's corpus
+worth having, where each tier was recorded red and then driven to green.
+
+Within B, sequence `$[]` and `$[<ident>]` together (same plumbing, both need
+only document + filters), and `$` after (needs the matched index carried out of
+query evaluation, three call sites).
+
+---
+
+## 7. What this review does not cover
+
+- **The other M3 operators.** `$setOnInsert`, `$addToSet`, `$mul`, `$min`/`$max`,
+  `$pop`, `$pullAll`, `$currentDate`, pipeline-form updates. Independent of this
+  work, and the ~10 pipeline-update failures are a separate design question.
+- **Index correctness after a positional update.** A multikey index over `y.b`
+  must be regenerated when an element changes. The normal `engine.replace` path
+  presumably handles it since it re-derives entries from the new document, but
+  that is an assumption stated, not a measurement — it needs a test in whichever
+  option is chosen.
+- **`max_path_segments = 16`.** Whether a positional segment costs one or two,
+  and what mongod's own limit is. Not measured.
+- **`$` in a projection** (`{'y.$': 1}`), which is the same sigil in a different
+  position and is not in scope here.
+- **The unknown-operator gap** recorded in PLAN §6 during the `distinct` work
+  (`{x: {$bogus: 1}}` matches nothing instead of erroring). Same silent-wrong-
+  answer class, different code path, still open.