diff --git a/docs/M3_ARRAYFILTERS_DESIGN_REVIEW.md b/docs/M3_ARRAYFILTERS_DESIGN_REVIEW.md new file mode 100644 index 0000000..3ce9277 --- /dev/null +++ b/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 `$[]` (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 | +| `$[]` | 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 `$[]` 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 `$[]` 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 `$[]` 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 `$[]` 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.