From 3c5eee21716f9d48fae98ca77f8854fabc8a9328 Mon Sep 17 00:00:00 2001 From: "A.Shakhmatov" Date: Mon, 10 Aug 2026 17:59:08 +0300 Subject: [PATCH] docs: M3 design review -- arrayFilters, and the positional operators The plan names a missing feature sized at 14 corpus cases. The measurement found a data-loss bug. `{$set: {'y.$[i].b': 2}}` does not fail to update the array -- it replaces the array with `{"$[i]": {"b": 2}}` and answers ok: 1, modifiedCount: 1. Every element is discarded. `src/update.zig:283` reaches a non-numeric segment under an array and takes the "treat as non-array: replace with a doc" branch, so `$`, `$[]` and `$[]` all destroy what they were meant to descend into, under every operator -- `$inc` through `$[i]` stores the operand rather than incrementing. 16 of 17 probe cases diverge from mongod. `arrayFilters` is not implicated: the string appears nowhere in `src/`. The option is accepted off the wire and dropped. The gate finding, which is the reason to write this before any code: M3's gate is "remaining crud coverage; e2e3/e2e4 green". e2e3 and e2e4 contain zero positional paths and would stay green through every version of this bug. The pinned corpus covers `$[]` only -- it has no `$[]` case and no bare `$` case anywhere. A fix scoped to what the gate measures would go green on 14 new passes with two of the three ways to destroy an array still live. The gate would certify the bug as fixed. Also measured, none of it guessable and two of them inverting how set_path behaves today: one path can name many targets (`y.$[].c.$[].d` is a genuine cross-product); a positional segment never creates anything, so a missing or non-array path is an error where `$set: {'a.b': 1}` would construct, and upsert gets no special case; ten distinct refusals across codes 2, 9 and 14, recorded with their messages, including one row that breaks the otherwise tidy 2/9 split and is left untidy on purpose. Recommends refusing first as its own commit -- the M2 doctrine, and it stops the data loss without waiting on the redesign -- then a recorded positional corpus built like tests/spec/aggregate/, with `$[]` and `$[]` together and `$` after, since only `$` needs the matched index carried out of query evaluation. --- docs/M3_ARRAYFILTERS_DESIGN_REVIEW.md | 334 ++++++++++++++++++++++++++ 1 file changed, 334 insertions(+) create mode 100644 docs/M3_ARRAYFILTERS_DESIGN_REVIEW.md 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.