From 3c5eee21716f9d48fae98ca77f8854fabc8a9328 Mon Sep 17 00:00:00 2001 From: "A.Shakhmatov" Date: Mon, 10 Aug 2026 17:59:08 +0300 Subject: [PATCH 1/4] 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. -- 2.39.5 From f04e7125c91a4d0d9d80ebbc64ccede40b99d5da Mon Sep 17 00:00:00 2001 From: "A.Shakhmatov" Date: Mon, 10 Aug 2026 18:21:14 +0300 Subject: [PATCH 2/4] update: refuse a positional path instead of destroying the array MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `{$set: {"y.$[i].b": 2}}` did not fail to update the array. It replaced `y: [{b: 3}, {b: 1}]` with `y: {"$[i]": {"b": 2}}` -- every element discarded -- and answered ok: 1, modifiedCount: 1. Remotely reachable by any client issuing an ordinary MongoDB update. `arrayFilters` was not implicated: the string appears nowhere in src/, the option is accepted off the wire and dropped. The destruction was in the path, at `set_path`'s "treat as non-array: replace with a doc" branch, so it fired for all three spellings of "descend into this array" -- `$`, `$[]` and `$[]` -- under every operator. `$inc` through `$[i]` stored its operand rather than incrementing. Two refusals, because the branch held two different mistakes: - a positional segment is refused up front, before anything is applied, so an update naming a good path and a positional one lands neither. Its code is BadValue (2), which is what mongod answers for every positional path failure. - a plain non-numeric segment under an array -- `y.nope.b`, `y.$x.b` -- is PathNotViable (28), measured. It is never a field to create, which is the opposite of what `set_path` does for a missing *document* field and the reason this branch existed at all. Numeric segments are untouched, including the null padding past the end, which a test now pins. Messages are this server's own words. mongod's PathNotViable text embeds a shell-syntax rendering of the offending element (`Cannot create field 'nope' in element {y: [ { b: 3 }, { b: 1 } ]}`) and no BSON formatter here produces it. The code is what the corpus asserts and the code is exact; a half-copy of the text would be worse than a clear sentence that does not pretend. Re-running the 17-case probe against both servers: 16 of 17 diverged before, 0 are destructive now, and 5 agree with mongod's code exactly -- every case where mongod also refuses. The rest refuse where mongod succeeds, which is the honest not-implemented state and is what the design review chose. Scorecard unchanged at 204 pass / 87 fail: the 14 arrayFilters cases still fail, now reporting the refusal rather than a corrupted document. That was the gate this review picked -- `docs/M3_ARRAYFILTERS_DESIGN_REVIEW.md` §5, option D -- because the corpus has no `$[]` case and no bare `$` case at all, so passing it would have certified two live ways to destroy an array. 206/206 unit (8 new) in ReleaseFast and ReleaseSafe, 83/83 fuzz, aggregation corpus 70/0, full e2e matrix and crash-fuzz green. Both refusals are mutation-checked: dropping the up-front scan reddens four tests on the array's contents rather than on the error, and restoring the destructive branch reddens the PathNotViable test. --- src/commands.zig | 122 ++++++++++++++-- src/update.zig | 301 ++++++++++++++++++++++++++++++++++----- tests/spec/scorecard.txt | 28 ++-- 3 files changed, 389 insertions(+), 62 deletions(-) diff --git a/src/commands.zig b/src/commands.zig index 59d2c0b..f8b6c49 100644 --- a/src/commands.zig +++ b/src/commands.zig @@ -81,6 +81,10 @@ pub const ErrorCode = enum(i32) { query_plan_killed = 175, unauthorized = 13, type_mismatch = 14, + /// `PathNotViable`, measured on mongod 8.3.7: what an update answers when + /// a path segment names a field inside something that cannot hold one -- + /// in practice, a non-numeric segment applied to an array. + path_not_viable = 28, operation_failed = 96, // Session and transaction codes, measured against mongod 8.3.7 with a raw // OP_MSG probe -- the driver rewrites `lsid` with its own session, so a @@ -1951,7 +1955,9 @@ fn cmd_update(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void { if (matched.items.len == 0) { if (upsert) { - const new_doc = try build_upsert_doc(reply, q, u_doc); + var up_diag: update.Diagnostic = .{}; + const new_doc = build_upsert_doc(reply, q, u_doc, &up_diag) catch |err| + return update_refusal(reply, err, up_diag); ctx.engine.insert(db_name, coll_name, new_doc, ctx.oid_gen) catch |err| switch (err) { error.DuplicateKey, error.DuplicateKeyIndex => return duplicate_key_error(ctx, reply, db_name, coll_name, new_doc), else => return err, @@ -1973,10 +1979,9 @@ fn cmd_update(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void { // and a rejected update must not corrupt the stored document. const doc = try doc_tree(reply.arena_alloc(), coll, off); const copy = try clone_doc(reply, doc); - update.apply(copy, &.{ .arena = undefined, .pairs = u_doc }) catch |err| switch (err) { - error.ImmutableId, error.InvalidUpdate => return bad_value(reply, "bad update"), - else => return err, - }; + var diag: update.Diagnostic = .{}; + update.apply(copy, &.{ .arena = undefined, .pairs = u_doc }, &diag) catch |err| + return update_refusal(reply, err, diag); const written = ctx.engine.replace(db_name, coll_name, copy, ctx.oid_gen) catch |err| switch (err) { error.DuplicateKey, error.DuplicateKeyIndex => { const e = try reply.arena_alloc().alloc(bson.Pair, 3); @@ -2079,7 +2084,9 @@ fn cmd_find_and_modify(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !v if (target == null and do_update and upsert) { const u_doc = doc_arg(msg.body.get("update")) orelse return bad_value(reply, "update must be a document"); - const new_doc = try build_upsert_doc(reply, q, u_doc); + var up_diag: update.Diagnostic = .{}; + const new_doc = build_upsert_doc(reply, q, u_doc, &up_diag) catch |err| + return update_refusal(reply, err, up_diag); ctx.engine.insert(db_name, coll_name, new_doc, ctx.oid_gen) catch |err| switch (err) { error.DuplicateKey, error.DuplicateKeyIndex => return duplicate_key_error(ctx, reply, db_name, coll_name, new_doc), else => return err, @@ -2096,10 +2103,9 @@ fn cmd_find_and_modify(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !v const u_doc = doc_arg(msg.body.get("update")) orelse return bad_value(reply, "update must be a document"); const before = try bson.copy_pairs(arena, target.?.pairs); const copy = try clone_doc(reply, target.?); - update.apply(copy, &.{ .arena = undefined, .pairs = u_doc }) catch |err| switch (err) { - error.ImmutableId, error.InvalidUpdate => return bad_value(reply, "bad update"), - else => return err, - }; + var diag: update.Diagnostic = .{}; + update.apply(copy, &.{ .arena = undefined, .pairs = u_doc }, &diag) catch |err| + return update_refusal(reply, err, diag); // findAndModify reports `n` (matched) and `updatedExisting`, neither of // which distinguishes a no-op, so whether it wrote is not needed here. _ = try ctx.engine.replace(db_name, coll_name, copy, ctx.oid_gen); @@ -4108,12 +4114,48 @@ fn clone_doc(reply: *wire.Reply, doc: *const bson.Document) !*bson.Document { return owned; } +/// The reply an update refusal turns into. +/// +/// Shared by all four places that apply an update, because a refusal wired +/// into three of them would be a silent divergence between `update`, +/// `findAndModify` and the upsert path -- and the two new ones exist to stop +/// a silent divergence in the first place. +/// +/// The messages are this server's own words. mongod's `PathNotViable` text +/// embeds a shell-syntax rendering of the offending element (`Cannot create +/// field 'nope' in element {y: [ { b: 3 }, { b: 1 } ]}`), and there is no BSON +/// formatter here that produces it. The code is what the corpus asserts and +/// the code is exact; a half-copy of the text would be worse than a clear +/// sentence that does not pretend. +fn update_refusal(reply: *wire.Reply, err: anyerror, diag: update.Diagnostic) !void { + const arena = reply.arena_alloc(); + switch (err) { + error.PositionalUnsupported => return bad_value(reply, try std.fmt.allocPrint( + arena, + "the positional operator '{s}' in path '{s}' is not implemented by this server", + .{ diag.segment, diag.path }, + )), + error.PathNotViable => return reply.put_error( + @intFromEnum(ErrorCode.path_not_viable), + "PathNotViable", + try std.fmt.allocPrint( + arena, + "Cannot create field '{s}' in an array, at path '{s}'", + .{ diag.segment, diag.path }, + ), + ), + error.ImmutableId, error.InvalidUpdate => return bad_value(reply, "bad update"), + else => return err, + } +} + /// Build the document for an upsert: equality fields from the filter, then /// the update operators applied. Owned by the reply arena. fn build_upsert_doc( reply: *wire.Reply, q: []const bson.Pair, u_doc: []const bson.Pair, + diag: *update.Diagnostic, ) !*bson.Document { const arena = reply.arena_alloc(); var pairs: std.ArrayListUnmanaged(bson.Pair) = .empty; @@ -4128,10 +4170,7 @@ fn build_upsert_doc( const owned = try arena.create(bson.Document); owned.* = bson.Document{ .arena = std.heap.ArenaAllocator.init(arena), .pairs = try pairs.toOwnedSlice(arena) }; // Apply update operators to build the final doc; _id handled by insert. - update.apply(owned, &.{ .arena = undefined, .pairs = u_doc }) catch |err| switch (err) { - error.ImmutableId, error.InvalidUpdate => return error.InvalidUpdate, - else => return err, - }; + try update.apply(owned, &.{ .arena = undefined, .pairs = u_doc }, diag); return owned; } @@ -5494,6 +5533,61 @@ test "distinct applies its filter before collecting" { try testing.expectEqual(@as(i32, 33), values[1].int32); } +test "a positional update is refused on the wire and stores nothing" { + // The end of the chain the unit tests start: the refusal has to reach the + // client as a code, and the stored document -- not just the working copy + // -- has to be the one that was there before. + // + // Every case here previously answered ok: 1 with nModified: 1, having + // replaced `y` with a document keyed by the path segment's text. + var threaded: std.Io.Threaded = .init_single_threaded; + defer threaded.deinit(); + const io = threaded.io(); + var tdb = try TestDb.init(io); + defer tdb.deinit(); + var ctx = tdb.ctx(io); + + const cases = [_]struct { coll: []const u8, path: []const u8, code: i32 }{ + .{ .coll = "a1", .path = "y.$[i].b", .code = 2 }, // filtered positional + .{ .coll = "a2", .path = "y.$[].b", .code = 2 }, // all-positional + .{ .coll = "a3", .path = "y.$.b", .code = 2 }, // positional + .{ .coll = "a4", .path = "y.nope.b", .code = 28 }, // PathNotViable, same branch + }; + for (cases) |c| { + try dispatch_insert(&tdb, io, c.coll, &.{ + .{ .doc = &.{ + .{ .key = "_id", .value = .{ .int32 = 1 } }, + .{ .key = "y", .value = .{ .array = &.{ + .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 3 } }} }, + } } }, + } }, + }); + + const updates = [_]bson.Value{.{ .doc = &.{ + .{ .key = "q", .value = .{ .doc = &.{} } }, + .{ .key = "u", .value = .{ .doc = &.{ + .{ .key = "$set", .value = .{ .doc = &.{ + .{ .key = c.path, .value = .{ .int32 = 2 } }, + } } }, + } } }, + } }}; + try testing.expectEqual(@as(?i32, c.code), try run_for_code(&ctx, "update", .{ .string = c.coll }, &.{ + .{ .key = "updates", .value = .{ .array = &updates } }, + })); + + // Read it back through `distinct`: if the array survived it still has + // an element with `b: 3`, and if it was overwritten by a document + // there is nothing at `y.b` at all. + var reply = wire.Reply.init(testing.allocator); + defer reply.deinit(); + const values = try distinct_values(&tdb, io, &reply, c.coll, &.{ + .{ .key = "key", .value = .{ .string = "y.b" } }, + }); + try testing.expectEqual(@as(usize, 1), values.len); + try testing.expectEqual(@as(i32, 3), values[0].int32); + } +} + test "aggregate $sort without a preceding $group sorts and frees correctly" { // Regression test for a remote, client-triggerable invalid free: the // $sort stage materialized its document list from the reply arena and diff --git a/src/update.zig b/src/update.zig index d432b46..0aecf2e 100644 --- a/src/update.zig +++ b/src/update.zig @@ -6,7 +6,42 @@ const std = @import("std"); const bson = @import("bson.zig"); const query = @import("query.zig"); -pub const UpdateError = error{ ImmutableId, InvalidUpdate, OutOfMemory }; +pub const UpdateError = error{ + ImmutableId, + InvalidUpdate, + /// A `$`, `$[]` or `$[]` segment: MongoDB's three ways of + /// saying "descend into this array", none of them implemented here. + PositionalUnsupported, + /// A non-numeric segment applied to an array, which is never a field to + /// create. mongod's `PathNotViable`. + PathNotViable, + OutOfMemory, +}; + +/// Which path a refusal was about, so the reply can name it instead of saying +/// "bad update". Borrowed from the update document, which outlives the call. +pub const Diagnostic = struct { + path: []const u8 = "", + segment: []const u8 = "", +}; + +fn note(diag: ?*Diagnostic, path: []const u8, segment: []const u8) void { + if (diag) |d| d.* = .{ .path = path, .segment = segment }; +} + +/// `$`, `$[]` and `$[]` -- the three spellings of "descend into +/// this array". +/// +/// None is implemented, and until they are each has to be refused rather than +/// walked. `set_path` used to reach the array, fail to read the segment as an +/// index, and overwrite the array with a document keyed by the segment's +/// literal text: `{$set: {"y.$[i].b": 2}}` turned `y: [{b: 3}, {b: 1}]` into +/// `y: {"$[i]": {"b": 2}}` and answered ok: 1. Every element was discarded, +/// under every operator -- `$inc` stored its operand rather than incrementing. +fn is_positional(seg: []const u8) bool { + if (std.mem.eql(u8, seg, "$")) return true; + return seg.len >= 3 and std.mem.startsWith(u8, seg, "$[") and seg[seg.len - 1] == ']'; +} const max_path_segments = 16; @@ -28,8 +63,16 @@ fn is_operator_key(key: []const u8) bool { } /// Apply an update document to `doc`: a replacement, or a set of operators. -pub fn apply(doc: *bson.Document, update: *const bson.Document) UpdateError!void { +pub fn apply( + doc: *bson.Document, + update: *const bson.Document, + diag: ?*Diagnostic, +) UpdateError!void { + // A replacement carries data, not paths, so nothing below applies to it -- + // which is also why mongod ignores `arrayFilters` alongside one, the single + // case out of seventeen where this server already agreed with it. if (is_replacement(update.pairs)) return apply_replacement(doc, update.pairs); + try reject_positional(update.pairs, diag); const arena = doc.arena.allocator(); var pairs = try copy_to_list(bson.Pair, arena, doc.pairs); for (update.pairs) |op| { @@ -37,11 +80,40 @@ pub fn apply(doc: *bson.Document, update: *const bson.Document) UpdateError!void // is not one is a mixed document -- which MongoDB rejects rather than // guessing at. if (!is_operator_key(op.key)) return error.InvalidUpdate; - try apply_operator(arena, &pairs, op.key, op.value); + try apply_operator(arena, &pairs, op.key, op.value, diag); } doc.pairs = try pairs.toOwnedSlice(arena); } +/// Refuse every positional path in the update before any of it is applied. +/// +/// Up front rather than at the point of use, because one update names several +/// paths: checking as we walk would refuse the third path having already +/// rewritten what the first two named. The caller discards its copy on error +/// either way, so this is not what makes the refusal safe -- it is what makes +/// the refusal *about the update* rather than about however far a walk got. +fn reject_positional(update: []const bson.Pair, diag: ?*Diagnostic) UpdateError!void { + for (update) |op| { + const ops = doc_pairs(op.value) orelse continue; + for (ops) |p| { + try reject_positional_path(p.key, diag); + // `$rename`'s destination is a path too, and it is the *value*. + if (std.mem.eql(u8, op.key, "$rename") and p.value == .string) { + try reject_positional_path(p.value.string, diag); + } + } + } +} + +fn reject_positional_path(path: []const u8, diag: ?*Diagnostic) UpdateError!void { + var it = std.mem.splitScalar(u8, path, '.'); + while (it.next()) |seg| { + if (!is_positional(seg)) continue; + note(diag, path, seg); + return error.PositionalUnsupported; + } +} + /// Replace every field of `doc` with `replacement`'s, except `_id`. /// /// `_id` is immutable, so it survives and keeps its position at the front (which @@ -97,6 +169,7 @@ fn apply_operator( pairs: *std.ArrayListUnmanaged(bson.Pair), op: []const u8, value: bson.Value, + diag: ?*Diagnostic, ) UpdateError!void { if (std.mem.eql(u8, op, "$set")) { const ops = doc_pairs(value) orelse return error.InvalidUpdate; @@ -104,7 +177,7 @@ fn apply_operator( if (std.mem.eql(u8, p.key, "_id")) return error.ImmutableId; var segs: [max_path_segments][]const u8 = undefined; const n = split_path(p.key, &segs) orelse return error.InvalidUpdate; - try set_path(arena, pairs, segs[0..n], try bson.copy_value(arena, p.value)); + try set_path(arena, pairs, segs[0..n], try bson.copy_value(arena, p.value), p.key, diag); } return; } @@ -125,7 +198,7 @@ fn apply_operator( const current = get_value(pairs.items, segs[0..n]) orelse bson.Value{ .int32 = 0 }; if (!current.is_number() or !p.value.is_number()) return error.InvalidUpdate; const sum = try numeric_add(current, p.value); - try set_path(arena, pairs, segs[0..n], sum); + try set_path(arena, pairs, segs[0..n], sum, p.key, diag); } return; } @@ -151,12 +224,12 @@ fn apply_operator( else => return error.InvalidUpdate, }; for (arr) |item| try items.append(arena, try bson.copy_value(arena, item)); - try set_path(arena, pairs, segs[0..n], .{ .array = try items.toOwnedSlice(arena) }); + try set_path(arena, pairs, segs[0..n], .{ .array = try items.toOwnedSlice(arena) }, p.key, diag); continue; } } try items.append(arena, try bson.copy_value(arena, p.value)); - try set_path(arena, pairs, segs[0..n], .{ .array = try items.toOwnedSlice(arena) }); + try set_path(arena, pairs, segs[0..n], .{ .array = try items.toOwnedSlice(arena) }, p.key, diag); } return; } @@ -177,7 +250,7 @@ fn apply_operator( try items.append(arena, elem); } } - try set_path(arena, pairs, segs[0..n], .{ .array = try items.toOwnedSlice(arena) }); + try set_path(arena, pairs, segs[0..n], .{ .array = try items.toOwnedSlice(arena) }, p.key, diag); } return; } @@ -192,7 +265,7 @@ fn apply_operator( unset_path(arena, pairs, old_segs[0..old_n]); var new_segs: [max_path_segments][]const u8 = undefined; const new_n = split_path(p.value.string, &new_segs) orelse return error.InvalidUpdate; - try set_path(arena, pairs, new_segs[0..new_n], v); + try set_path(arena, pairs, new_segs[0..new_n], v, p.value.string, diag); } return; } @@ -258,6 +331,8 @@ fn set_path( pairs: *std.ArrayListUnmanaged(bson.Pair), segs: []const []const u8, value: bson.Value, + path: []const u8, + diag: ?*Diagnostic, ) UpdateError!void { if (segs.len == 1) { if (find_pair(pairs.items, segs[0])) |idx| { @@ -270,23 +345,31 @@ fn set_path( const idx = find_pair(pairs.items, segs[0]) orelse { const is_array = parse_index(segs[1]) != null; try pairs.append(arena, .{ .key = try arena.dupe(u8, segs[0]), .value = if (is_array) .{ .array = &.{} } else .{ .doc = &.{} } }); - return set_path(arena, pairs, segs, value); + return set_path(arena, pairs, segs, value, path, diag); }; switch (pairs.items[idx].value) { .doc => |sub| { var sub_pairs = try copy_to_list(bson.Pair, arena, sub); defer sub_pairs.deinit(arena); - try set_path(arena, &sub_pairs, segs[1..], value); + try set_path(arena, &sub_pairs, segs[1..], value, path, diag); pairs.items[idx].value = .{ .doc = try sub_pairs.toOwnedSlice(arena) }; }, .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; - defer sub_pairs.deinit(arena); - try set_path(arena, &sub_pairs, segs[1..], value); - pairs.items[idx].value = .{ .doc = try sub_pairs.toOwnedSlice(arena) }; - return; + // A non-numeric segment under an array is never a field to + // create. This branch used to read "treat as non-array: + // replace with a doc" and did exactly that -- `y.nope.b` + // turned `y: [{b: 3}]` into `y: {nope: {b: 2}}`, discarding + // every element and answering ok: 1. mongod refuses with + // PathNotViable and leaves the document alone. + // + // The positional spellings took this same branch and are the + // reason it was found; they are refused earlier, by + // `reject_positional`. What reaches here is the rest of the + // class: a plain field name, and a `$`-prefixed one that is + // not positional. + note(diag, path, segs[1]); + return error.PathNotViable; }; var items = try copy_to_list(bson.Value, arena, arr); defer items.deinit(arena); @@ -300,13 +383,13 @@ fn set_path( .doc => |sub| { var sub_pairs = try copy_to_list(bson.Pair, arena, sub); defer sub_pairs.deinit(arena); - try set_path(arena, &sub_pairs, segs[2..], value); + try set_path(arena, &sub_pairs, segs[2..], value, path, diag); items.items[index] = .{ .doc = try sub_pairs.toOwnedSlice(arena) }; }, else => { var sub_pairs: std.ArrayListUnmanaged(bson.Pair) = .empty; defer sub_pairs.deinit(arena); - try set_path(arena, &sub_pairs, segs[2..], value); + try set_path(arena, &sub_pairs, segs[2..], value, path, diag); items.items[index] = .{ .doc = try sub_pairs.toOwnedSlice(arena) }; }, } @@ -316,7 +399,7 @@ fn set_path( else => { var sub_pairs: std.ArrayListUnmanaged(bson.Pair) = .empty; defer sub_pairs.deinit(arena); - try set_path(arena, &sub_pairs, segs[1..], value); + try set_path(arena, &sub_pairs, segs[1..], value, path, diag); pairs.items[idx].value = .{ .doc = try sub_pairs.toOwnedSlice(arena) }; }, } @@ -425,7 +508,7 @@ test "$set, $inc, $unset, $rename" { .{ .key = "$inc", .value = .{ .doc = &.{.{ .key = "user.age", .value = .{ .int32 = 2 } }} } }, .{ .key = "$unset", .value = .{ .doc = &.{.{ .key = "gone", .value = .{ .string = "" } }} } }, .{ .key = "$rename", .value = .{ .doc = &.{.{ .key = "new", .value = .{ .string = "renamed" } }} } }, - })); + }), null); const user = bson.get_pair(doc.pairs, "user").?; try testing.expectEqualStrings("alice", user.doc[0].value.string); @@ -445,13 +528,13 @@ test "$push and $pull" { try apply(&doc, &doc_of(&.{ .{ .key = "$push", .value = .{ .doc = &.{.{ .key = "tags", .value = .{ .string = "c" } }} } }, - })); + }), null); try testing.expectEqual(@as(usize, 3), bson.get_pair(doc.pairs, "tags").?.array.len); try testing.expectEqualStrings("c", bson.get_pair(doc.pairs, "tags").?.array[2].string); try apply(&doc, &doc_of(&.{ .{ .key = "$pull", .value = .{ .doc = &.{.{ .key = "tags", .value = .{ .string = "b" } }} } }, - })); + }), null); const tags = bson.get_pair(doc.pairs, "tags").?.array; try testing.expectEqual(@as(usize, 2), tags.len); try testing.expectEqualStrings("a", tags[0].string); @@ -459,7 +542,7 @@ test "$push and $pull" { try apply(&doc, &doc_of(&.{ .{ .key = "$push", .value = .{ .doc = &.{.{ .key = "tags", .value = .{ .doc = &.{.{ .key = "$each", .value = .{ .array = &.{ .{ .string = "x" }, .{ .string = "y" } } } }} } }} } }, - })); + }), null); try testing.expectEqual(@as(usize, 4), bson.get_pair(doc.pairs, "tags").?.array.len); } @@ -475,7 +558,7 @@ test "$set nested creation and _id protection" { .{ .key = "a.b.c", .value = .{ .int32 = 42 } }, .{ .key = "arr.1", .value = .{ .string = "x" } }, } } }, - })); + }), null); const a = bson.get_pair(doc.pairs, "a").?; const b = bson.get_pair(a.doc, "b").?; try testing.expectEqual(@as(i64, 42), bson.get_pair(b.doc, "c").?.int32); @@ -485,7 +568,7 @@ test "$set nested creation and _id protection" { try testing.expectError(error.ImmutableId, apply(&doc, &doc_of(&.{ .{ .key = "$set", .value = .{ .doc = &.{.{ .key = "_id", .value = .{ .int32 = 2 } }} } }, - }))); + }), null)); } test "a replacement keeps _id and drops every other field" { @@ -502,7 +585,7 @@ test "a replacement keeps _id and drops every other field" { try apply(&doc, &doc_of(&.{ .{ .key = "fresh", .value = .{ .int32 = 42 } }, - })); + }), null); try testing.expectEqual(@as(usize, 2), doc.pairs.len); // _id survives, and stays at the front where it is stored. @@ -523,7 +606,7 @@ test "an empty replacement leaves a document holding only its _id" { // Legal, and the reason `is_replacement` treats an empty document as one // rather than as a no-op set of operators. - try apply(&doc, &doc_of(&.{})); + try apply(&doc, &doc_of(&.{}), null); try testing.expectEqual(@as(usize, 1), doc.pairs.len); try testing.expectEqualStrings("_id", doc.pairs[0].key); } @@ -545,7 +628,7 @@ test "a replacement may repeat the _id it is replacing, but not change it" { try apply(&doc, &doc_of(&.{ .{ .key = "_id", .value = .{ .int32 = 7 } }, .{ .key = "b", .value = .{ .int32 = 2 } }, - })); + }), null); try testing.expectEqual(@as(i64, 7), doc.pairs[0].value.int32); try testing.expectEqual(@as(i64, 2), bson.get_pair(doc.pairs, "b").?.int32); // And only once, not twice. @@ -554,14 +637,14 @@ test "a replacement may repeat the _id it is replacing, but not change it" { // A different _id: refused. try testing.expectError(error.ImmutableId, apply(&doc, &doc_of(&.{ .{ .key = "_id", .value = .{ .int32 = 8 } }, - }))); + }), null)); // Equal across numeric types is the same _id, matching the canonical key // encoding the _id_ index descends on. try apply(&doc, &doc_of(&.{ .{ .key = "_id", .value = .{ .double = 7.0 } }, .{ .key = "c", .value = .{ .int32 = 3 } }, - })); + }), null); try testing.expectEqual(@as(i64, 3), bson.get_pair(doc.pairs, "c").?.int32); } @@ -578,7 +661,7 @@ test "a replacement supplies the _id when the document has none" { try apply(&doc, &doc_of(&.{ .{ .key = "a", .value = .{ .int32 = 1 } }, .{ .key = "_id", .value = .{ .int32 = 99 } }, - })); + }), null); try testing.expectEqual(@as(usize, 2), doc.pairs.len); try testing.expectEqualStrings("_id", doc.pairs[0].key); @@ -596,13 +679,13 @@ test "a mixed update document is refused from either side" { try testing.expectError(error.InvalidUpdate, apply(&doc, &doc_of(&.{ .{ .key = "$set", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 1 } }} } }, .{ .key = "plain", .value = .{ .int32 = 1 } }, - }))); + }), null)); // Starts with data, so it is a replacement and an operator has no meaning. try testing.expectError(error.InvalidUpdate, apply(&doc, &doc_of(&.{ .{ .key = "plain", .value = .{ .int32 = 1 } }, .{ .key = "$set", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 1 } }} } }, - }))); + }), null)); } test "is_replacement decides on the first field only" { @@ -613,3 +696,153 @@ test "is_replacement decides on the first field only" { // replacement path then stores it, which is what MongoDB does with it. try testing.expect(is_replacement(&.{.{ .key = "", .value = .{ .int32 = 1 } }})); } + +/// A document with one array field, rebuilt per case so a refusal can be +/// checked against untouched bytes. +fn array_doc(arena: std.mem.Allocator) !bson.Document { + var doc = bson.Document{ .arena = std.heap.ArenaAllocator.init(arena), .pairs = &.{} }; + doc.pairs = try doc.arena.allocator().dupe(bson.Pair, &.{ + .{ .key = "y", .value = .{ .array = &.{ + .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 3 } }} }, + .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 1 } }} }, + } } }, + }); + return doc; +} + +fn expect_y_untouched(doc: *const bson.Document) !void { + const y = doc.get("y") orelse return error.TestUnexpectedResult; + // The array is still an array. Before this refusal existed it was a + // document keyed by the path segment's literal text, and everything in it + // was gone. + try testing.expect(y == .array); + try testing.expectEqual(@as(usize, 2), y.array.len); + try testing.expectEqual(@as(i32, 3), y.array[0].doc[0].value.int32); + try testing.expectEqual(@as(i32, 1), y.array[1].doc[0].value.int32); +} + +test "a positional path is refused and the array survives" { + // The load-bearing test of the refusal. Each of these used to answer + // success having replaced `y` with `{"": ...}` -- the array and + // both its elements discarded, `ok: 1`, `modifiedCount: 1`. + // + // Mutation check: delete the `reject_positional` call in `apply` and every + // case here goes red on `expect_y_untouched`, not on the error. + const cases = [_][]const u8{ + "y.$[i].b", // filtered positional + "y.$[].b", // all-positional + "y.$.b", // positional + "y.$[i]", // as the leaf + "$", // in first position + "y.$[i].c.$[j].d", // nested, two identifiers + }; + for (cases) |path| { + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + var diag: Diagnostic = .{}; + try testing.expectError(error.PositionalUnsupported, apply(&doc, &doc_of(&.{ + .{ .key = "$set", .value = .{ .doc = &.{.{ .key = path, .value = .{ .int32 = 2 } }} } }, + }), &diag)); + try testing.expectEqualStrings(path, diag.path); + try expect_y_untouched(&doc); + } +} + +test "every operator refuses a positional path, not just $set" { + // The destruction was below the operator, in the shared path walk: `$inc` + // through `$[i]` stored its operand instead of incrementing. So the + // refusal has to be below the operator too. + const ops = [_]struct { op: []const u8, value: bson.Value }{ + .{ .op = "$set", .value = .{ .int32 = 2 } }, + .{ .op = "$inc", .value = .{ .int32 = 10 } }, + .{ .op = "$unset", .value = .{ .string = "" } }, + .{ .op = "$push", .value = .{ .int32 = 1 } }, + .{ .op = "$pull", .value = .{ .int32 = 1 } }, + }; + for (ops) |o| { + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + try testing.expectError(error.PositionalUnsupported, apply(&doc, &doc_of(&.{ + .{ .key = o.op, .value = .{ .doc = &.{.{ .key = "y.$[i].b", .value = o.value }} } }, + }), null)); + try expect_y_untouched(&doc); + } +} + +test "$rename checks its destination, which is the value not the key" { + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + var diag: Diagnostic = .{}; + try testing.expectError(error.PositionalUnsupported, apply(&doc, &doc_of(&.{ + .{ .key = "$rename", .value = .{ .doc = &.{ + .{ .key = "y", .value = .{ .string = "z.$[i]" } }, + } } }, + }), &diag)); + try testing.expectEqualStrings("z.$[i]", diag.path); + try expect_y_untouched(&doc); +} + +test "nothing in the update is applied when one of its paths is refused" { + // The refusal is taken before the first write, so the good path in this + // update does not land either. Mutation check: move `reject_positional` + // inside the operator loop and `ok` appears on the document. + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + try testing.expectError(error.PositionalUnsupported, apply(&doc, &doc_of(&.{ + .{ .key = "$set", .value = .{ .doc = &.{ + .{ .key = "ok", .value = .{ .int32 = 1 } }, + .{ .key = "y.$[i].b", .value = .{ .int32 = 2 } }, + } } }, + }), null)); + try testing.expect(doc.get("ok") == null); + try expect_y_untouched(&doc); +} + +test "a non-numeric segment under an array is PathNotViable, not a new field" { + // The rest of the class the positional forms belonged to. `y.nope.b` is + // not a positional operator and is refused for a different reason with a + // different code -- measured on mongod 8.3.7, which answers 28 here and 2 + // for the positional forms. + const cases = [_][]const u8{ "y.nope.b", "y.nope", "y.$x.b" }; + for (cases) |path| { + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + var diag: Diagnostic = .{}; + try testing.expectError(error.PathNotViable, apply(&doc, &doc_of(&.{ + .{ .key = "$set", .value = .{ .doc = &.{.{ .key = path, .value = .{ .int32 = 2 } }} } }, + }), &diag)); + try expect_y_untouched(&doc); + } +} + +test "a numeric segment still addresses an array element" { + // The regression guard for the refusal above: indexed paths are the one + // way into an array that does work, and they must keep working, including + // the null padding past the end. + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + try apply(&doc, &doc_of(&.{ + .{ .key = "$set", .value = .{ .doc = &.{ + .{ .key = "y.0.b", .value = .{ .int32 = 7 } }, + .{ .key = "y.3.b", .value = .{ .int32 = 8 } }, + } } }, + }), null); + const y = doc.get("y").?; + try testing.expectEqual(@as(usize, 4), y.array.len); + try testing.expectEqual(@as(i32, 7), y.array[0].doc[0].value.int32); + try testing.expect(y.array[2] == .null); + try testing.expectEqual(@as(i32, 8), y.array[3].doc[0].value.int32); +} + +test "a replacement is not a path, so it is not refused" { + // The one case out of seventeen where this server already agreed with + // mongod: `arrayFilters` alongside a replacement is ignored by both, and + // a replacement field named like a path is data, not a path. + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + try apply(&doc, &doc_of(&.{ + .{ .key = "z", .value = .{ .int32 = 1 } }, + }), null); + try testing.expect(doc.get("y") == null); + try testing.expectEqual(@as(i32, 1), doc.get("z").?.int32); +} diff --git a/tests/spec/scorecard.txt b/tests/spec/scorecard.txt index 5e08019..31134c4 100644 --- a/tests/spec/scorecard.txt +++ b/tests/spec/scorecard.txt @@ -209,9 +209,9 @@ aggregate-rawdata.json SKIP Aggregate with rawData option needs server >= 8.2.0 aggregate-write-readPreference.json SKIP * needs topology replicaset/sharded/load-balanced aggregate.json SKIP aggregate with a document comment - pre 4.4 needs server <= 4.2.99 aggregate.json SKIP aggregate with comment does not set comment on getMore - pre 4.4 needs server <= 4.3.99 -bulkWrite-arrayFilters.json FAIL BulkWrite updateOne with arrayFilters outcome crud-tests.test[0].y: expected an array, got {"$[i]":{"b":2}} -bulkWrite-arrayFilters.json FAIL BulkWrite updateMany with arrayFilters outcome crud-tests.test[0].y: expected an array, got {"$[i]":{"b":2}} -bulkWrite-arrayFilters.json FAIL BulkWrite with arrayFilters bulkWrite.modifiedCount: expected 3, got 2 +bulkWrite-arrayFilters.json FAIL BulkWrite updateOne with arrayFilters MongoBulkWriteError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +bulkWrite-arrayFilters.json FAIL BulkWrite updateMany with arrayFilters MongoBulkWriteError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +bulkWrite-arrayFilters.json FAIL BulkWrite with arrayFilters MongoBulkWriteError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server bulkWrite-collation.json FAIL BulkWrite with delete operations and collation bulkWrite.deletedCount: expected 4, got 0 bulkWrite-collation.json FAIL BulkWrite with update operations and collation bulkWrite.matchedCount: expected 6, got 2 bulkWrite-comment.json SKIP BulkWrite with comment - pre 4.4 needs server <= 4.2.99 @@ -361,9 +361,9 @@ findOneAndReplace-upsert.json FAIL FindOneAndReplace when no documents match wit findOneAndReplace-upsert.json FAIL FindOneAndReplace when no documents match with id specified with upsert returning the document after modification findOneAndReplace: expected a document, got null findOneAndReplace.json FAIL FindOneAndReplace when many documents match returning the document after modification findOneAndReplace.x: expected 32, got 22 findOneAndReplace.json FAIL FindOneAndReplace when one document matches returning the document after modification findOneAndReplace.x: expected 32, got 22 -findOneAndUpdate-arrayFilters.json FAIL FindOneAndUpdate when no document matches arrayFilters outcome crud-v1.coll[0].y: expected an array, got {"$[i]":{"b":2}} -findOneAndUpdate-arrayFilters.json FAIL FindOneAndUpdate when one document matches arrayFilters outcome crud-v1.coll[0].y: expected an array, got {"$[i]":{"b":2}} -findOneAndUpdate-arrayFilters.json FAIL FindOneAndUpdate when multiple documents match arrayFilters outcome crud-v1.coll[0].y: expected an array, got {"$[i]":{"b":2}} +findOneAndUpdate-arrayFilters.json FAIL FindOneAndUpdate when no document matches arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +findOneAndUpdate-arrayFilters.json FAIL FindOneAndUpdate when one document matches arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +findOneAndUpdate-arrayFilters.json FAIL FindOneAndUpdate when multiple documents match arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server findOneAndUpdate-collation.json FAIL FindOneAndUpdate when many documents match with collation returning the document before modification findOneAndUpdate: expected a document, got null findOneAndUpdate-comment.json FAIL findOneAndUpdate with string comment MongoServerError: update must be a document findOneAndUpdate-comment.json FAIL findOneAndUpdate with document comment MongoServerError: update must be a document @@ -403,9 +403,9 @@ replaceOne-let.json SKIP ReplaceOne with let option needs server >= 5.0 replaceOne-let.json FAIL ReplaceOne with let option unsupported (server-side error) replaceOne: expected an error, the operation succeeded replaceOne-rawdata.json SKIP ReplaceOne with rawData option needs server >= 8.2.0 replaceOne-sort.json SKIP ReplaceOne with sort option needs server >= 8.0 -updateMany-arrayFilters.json FAIL UpdateMany when no documents match arrayFilters updateMany.modifiedCount: expected 0, got 2 -updateMany-arrayFilters.json FAIL UpdateMany when one document matches arrayFilters updateMany.modifiedCount: expected 1, got 2 -updateMany-arrayFilters.json FAIL UpdateMany when multiple documents match arrayFilters outcome crud-v1.coll[0].y: expected an array, got {"$[i]":{"b":2}} +updateMany-arrayFilters.json FAIL UpdateMany when no documents match arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +updateMany-arrayFilters.json FAIL UpdateMany when one document matches arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +updateMany-arrayFilters.json FAIL UpdateMany when multiple documents match arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server updateMany-collation.json FAIL UpdateMany when many documents match with collation updateMany.matchedCount: expected 2, got 1 updateMany-comment.json SKIP UpdateMany with comment - pre 4.4 needs server <= 4.2.99 updateMany-dots_and_dollars.json SKIP Updating document to set top-level dollar-prefixed key on 5.0+ server needs server >= 5.0 @@ -416,11 +416,11 @@ updateMany-let.json SKIP updateMany with let option needs server >= 5.0 updateMany-let.json FAIL updateMany with let option unsupported (server-side error) updateMany: error message "update spec requires u" does not contain "'update.let' is an unknown field" updateMany-pipeline.json FAIL UpdateMany using pipelines MongoServerError: update spec requires u updateMany-rawdata.json SKIP updateMany with rawData option needs server >= 8.2.0 -updateOne-arrayFilters.json FAIL UpdateOne when no document matches arrayFilters updateOne.modifiedCount: expected 0, got 1 -updateOne-arrayFilters.json FAIL UpdateOne when one document matches arrayFilters outcome crud-v1.coll[0].y: expected an array, got {"$[i]":{"b":2}} -updateOne-arrayFilters.json FAIL UpdateOne when multiple documents match arrayFilters outcome crud-v1.coll[0].y: expected an array, got {"$[i]":{"b":2}} -updateOne-arrayFilters.json FAIL UpdateOne when no documents match multiple arrayFilters updateOne.modifiedCount: expected 0, got 1 -updateOne-arrayFilters.json FAIL UpdateOne when one document matches multiple arrayFilters outcome crud-v1.coll[2].y: expected an array, got {"$[i]":{"c":{"$[j]":{"d":0}}}} +updateOne-arrayFilters.json FAIL UpdateOne when no document matches arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +updateOne-arrayFilters.json FAIL UpdateOne when one document matches arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +updateOne-arrayFilters.json FAIL UpdateOne when multiple documents match arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +updateOne-arrayFilters.json FAIL UpdateOne when no documents match multiple arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].c.$[j].d' is not implemented by this server +updateOne-arrayFilters.json FAIL UpdateOne when one document matches multiple arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].c.$[j].d' is not implemented by this server updateOne-collation.json FAIL UpdateOne when one document matches with collation updateOne.matchedCount: expected 1, got 0 updateOne-comment.json SKIP UpdateOne with comment - pre 4.4 needs server <= 4.2.99 updateOne-dots_and_dollars.json SKIP Updating document to set top-level dollar-prefixed key on 5.0+ server needs server >= 5.0 -- 2.39.5 From 8136ffe8d4cf500df2dbd06a9d3bf847b43cf311 Mon Sep 17 00:00:00 2001 From: "A.Shakhmatov" Date: Mon, 10 Aug 2026 18:21:14 +0300 Subject: [PATCH 3/4] plan: drop segfaults when dispatched in-process Found while writing the positional-refusal tests and not caused by them -- it reproduces at 3c5eee2 with that work stashed. One insert followed by one drop against a TestDb engine terminates with SIGSEGV and no stack trace, while 25 insert/update/distinct/drop cycles over the wire are clean, so what differs is the in-process caller rather than the command. Worth more than it looks: D1's architecture is "in-process server now, library + C API later", and this is that path. `drop` has no unit test in the tree at all, which is why it went unseen. --- PLAN.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/PLAN.md b/PLAN.md index 081d6c2..684928c 100644 --- a/PLAN.md +++ b/PLAN.md @@ -1045,6 +1045,19 @@ has to be its own commit with its own re-recorded scorecard. `$out`/`$merge` durability semantics, whether the expression evaluator is shared with M3's pipeline updates, and whether `allowDiskUse` has to stop being a lie. +- **`drop` segfaults when dispatched in-process** — found while writing the + positional-refusal tests, unrelated to them, and **reproduces at `3c5eee2` + with the change stashed**, so it is not caused by that work. A single + `insert` followed by a single `drop` against a `TestDb` engine terminates + with SIGSEGV and no stack trace; dropping a collection that never existed is + fine. The wire path is clean — 25 insert / refused-update / distinct / drop + cycles against a live server on `:27020` all passed — so what differs is the + in-process caller, not the command. That matters more than it looks: D1's + whole architecture is "in-process server now, library + C API later", and + this is the embedding path. **`drop` has no unit test at all** — the only + `parse_fake_msg("drop", ...)` calls in the tree were the scratch ones written + to find this — which is why it went unseen. Needs its own commit: reproduce + it in a committed test first, then fix. - **M3 update operators** — open. `distinct` landed first because it was a whole missing command with no dependencies, and measuring it turned up three things worth keeping, none of which are `distinct`'s to fix: -- 2.39.5 From 3c2ac38fd9b46b2cd3c2a7ac29d892fcf94d9208 Mon Sep 17 00:00:00 2001 From: "A.Shakhmatov" Date: Mon, 10 Aug 2026 18:41:55 +0300 Subject: [PATCH 4/4] commands: drop does not unlock the collection it just freed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A use-after-free. Dispatch held `drop`'s collection lock across the handler, `drop_collection` freed the Collection the lock lives in, and dispatch then ran `unlock_collection` on freed memory -- an atomic read-modify-write inside `Io.RwLock.unlock`. One insert and one drop was enough. Two things kept it hidden for this long. `drop` had no unit test at all: before this commit every `parse_fake_msg("drop", ...)` in the tree was in a test written to hunt it. And over the wire it does not fault -- 25 insert/drop cycles against a live server pass -- because the general allocator leaves the freed page mapped and the atomic write lands somewhere harmless. That was luck, not safety: the same undefined behaviour either way, and testing.allocator is what makes it visible, which is why the regression test is a unit test rather than an e2e script. Fixed by giving `drop` no collection lock at all. The catalog lock is what actually excludes here: every collection lock in this engine -- dispatch, the TTL sweep, `compact`'s rebuild, `write_catalog`, `slab_stats`, reclamation -- is taken while holding the catalog at least shared, so holding it exclusively already keeps every one of them out. The collection lock was buying exclusion that was already there and paying for it by locking an object about to cease existing. That exposed a second bug rather than creating one. The dispatch epilogue -- commit, then maybe checkpoint, then maybe compact -- fired on `locks.coll == .exclusive` as a stand-in for "this was a write". It is now keyed on `kind == .write`, because the two agreed only by accident: `dropDatabase` is the one write that never held a collection lock, so it has never reached that epilogue, and a dropped database waited for some later write to trigger the checkpoint that records it. Mutation-checked: restoring `.coll = .exclusive` on the drop row reproduces the original SIGSEGV in the new test. 208/208 unit (2 new) in ReleaseFast and ReleaseSafe, 83/83 fuzz, crud scorecard unchanged at 204/87, aggregation corpus 70/0, full e2e matrix, crash-fuzz and the 25-cycle wire drop probe green. Left open and recorded in PLAN §6: `drop_collection` still writes no log record, so a dropped collection resurrects on reopen unless a checkpoint ran (pre-existing, with its own test at db.zig:5542); and `apply_pending_write` drops under `engine.rwlock` rather than the catalog lock, so the two drop paths disagree about which lock protects a namespace. --- PLAN.md | 50 ++++++++++++++++++++-------- src/commands.zig | 85 ++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 120 insertions(+), 15 deletions(-) diff --git a/PLAN.md b/PLAN.md index 684928c..7eadef4 100644 --- a/PLAN.md +++ b/PLAN.md @@ -1045,19 +1045,43 @@ has to be its own commit with its own re-recorded scorecard. `$out`/`$merge` durability semantics, whether the expression evaluator is shared with M3's pipeline updates, and whether `allowDiskUse` has to stop being a lie. -- **`drop` segfaults when dispatched in-process** — found while writing the - positional-refusal tests, unrelated to them, and **reproduces at `3c5eee2` - with the change stashed**, so it is not caused by that work. A single - `insert` followed by a single `drop` against a `TestDb` engine terminates - with SIGSEGV and no stack trace; dropping a collection that never existed is - fine. The wire path is clean — 25 insert / refused-update / distinct / drop - cycles against a live server on `:27020` all passed — so what differs is the - in-process caller, not the command. That matters more than it looks: D1's - whole architecture is "in-process server now, library + C API later", and - this is the embedding path. **`drop` has no unit test at all** — the only - `parse_fake_msg("drop", ...)` calls in the tree were the scratch ones written - to find this — which is why it went unseen. Needs its own commit: reproduce - it in a committed test first, then fix. +- **`drop` unlocked the collection it had just freed** — *fixed.* Found while + writing the positional-refusal tests and not caused by them; it reproduced + at `3c5eee2` with that work stashed. + + A use-after-free, not an allocator quirk. Dispatch held `drop`'s collection + lock across the handler, the handler freed the `Collection` the lock lives + in, and dispatch then ran `unlock_collection` on freed memory — an atomic + read-modify-write inside `Io.RwLock.unlock`. One insert and one drop was + enough. + + The wire path not faulting was luck, not safety: 25 insert/drop cycles + against a live server pass because the general allocator leaves the freed + page mapped, so the atomic write lands somewhere harmless. It was the same + undefined behaviour either way, and `testing.allocator` is what made it + visible. **`drop` had no unit test at all**, which is why it went unseen. + + Fixed by giving `drop` no collection lock. The catalog lock is what actually + excludes: every collection lock in the engine — dispatch, the TTL sweep, + `compact`'s rebuild, `write_catalog`, `slab_stats`, reclamation — is taken + while holding the catalog at least shared, so `drop` holding it exclusively + already keeps all of them out. The collection lock bought exclusion that was + already there and paid for it by locking an object about to cease existing. + + That change also made the dispatch epilogue's predicate wrong, and it turned + out to have been wrong already: it fired on `locks.coll == .exclusive` as a + stand-in for "this was a write", and **`dropDatabase` is the one write that + never held a collection lock**, so it had never reached the commit and + checkpoint epilogue at all. Now keyed on `kind == .write`. + + Still open, and deliberately not fixed here: `drop_collection` writes no log + record, so a dropped collection resurrects on reopen unless a checkpoint + happened to run — a pre-existing limitation with its own test at + `db.zig:5542`. Separately, `apply_pending_write` (`$out`/`$merge`) calls + `drop_collection` under `engine.rwlock` rather than the catalog lock, so it + is not excluded by the reasoning above; it holds no collection lock, so it + is not this crash, but the two drops disagree about which lock protects the + namespace and that wants one answer. - **M3 update operators** — open. `distinct` landed first because it was a whole missing command with no dependencies, and measuring it turned up three things worth keeping, none of which are `distinct`'s to fix: diff --git a/src/commands.zig b/src/commands.zig index f8b6c49..1b35d38 100644 --- a/src/commands.zig +++ b/src/commands.zig @@ -198,7 +198,19 @@ const command_table = [_]Command{ // Writes: the target collection exclusively; create/drop take the // catalog exclusively (they mutate the maps). .{ .name = "create", .kind = .write, .locks = .{ .catalog = .exclusive, .coll = .exclusive }, .handler = cmd_create }, - .{ .name = "drop", .kind = .write, .locks = .{ .catalog = .exclusive, .coll = .exclusive }, .handler = cmd_drop }, + // `drop` takes the catalog exclusively and **no collection lock**: it frees + // the very Collection a lock would live in, and dispatch then unlocked the + // freed memory. That was a use-after-free on an `Io.RwLock`, and under + // testing.allocator it is a hard SIGSEGV on the first insert-then-drop. + // + // Nothing is lost by dropping the lock, because the catalog lock is what + // actually excludes here: every collection lock in this engine -- dispatch, + // the TTL sweep, `compact`'s rebuild, `write_catalog`, `slab_stats`, + // reclamation -- is taken while holding the catalog at least shared, so + // holding it exclusively already keeps every one of them out. The + // collection lock was buying exclusion that was already there, and paying + // for it by locking an object about to cease existing. + .{ .name = "drop", .kind = .write, .locks = .{ .catalog = .exclusive }, .handler = cmd_drop }, .{ .name = "dropDatabase", .kind = .write, .locks = .{ .catalog = .exclusive }, .handler = cmd_drop_database }, .{ .name = "createIndexes", .kind = .write, .locks = .{ .catalog = .shared, .coll = .exclusive }, .handler = cmd_create_indexes }, .{ .name = "dropIndexes", .kind = .write, .locks = .{ .catalog = .shared, .coll = .exclusive }, .handler = cmd_drop_indexes }, @@ -313,7 +325,15 @@ pub fn dispatch(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void { }; try ctx.engine.commit(); } - if (cmd.locks.coll == .exclusive) { + // Keyed on what the command *is*, not on which lock it happened to take. + // These two agreed for every command until `drop` gave up its collection + // lock, at which point the old `locks.coll == .exclusive` would have + // silently stopped committing and checkpointing after a drop. It was + // already wrong for `dropDatabase`, the one write that never held a + // collection lock: it has been skipping this epilogue all along, so a + // dropped database waited for some later write to trigger a checkpoint + // before the catalog recording it was written. + if (cmd.kind == .write) { // Durability (seal + fsync) coalesces across concurrent writers. A // commit error deliberately wins over the handler's captured `result`: // whether the write reached disk matters more to the client than why @@ -5533,6 +5553,67 @@ test "distinct applies its filter before collecting" { try testing.expectEqual(@as(i32, 33), values[1].int32); } +test "drop does not unlock the collection it just freed" { + // Regression test for a use-after-free: dispatch held `drop`'s collection + // lock across the handler, the handler freed the Collection the lock lives + // in, and dispatch then ran `unlock_collection` on freed memory. One + // insert and one drop was enough -- SIGSEGV inside `Io.RwLock.unlock`. + // + // Two things kept it hidden. `drop` had no unit test at all: before this + // one, every `parse_fake_msg("drop", ...)` in the tree was inside a test + // written to hunt it. And over the wire it did not fault -- 25 + // insert/drop cycles against a live server pass -- because the general + // allocator leaves the freed page mapped, so the atomic write lands + // somewhere harmless. testing.allocator is what makes it visible, which + // is exactly why this test belongs here rather than in an e2e script. + var threaded: std.Io.Threaded = .init_single_threaded; + defer threaded.deinit(); + const io = threaded.io(); + var tdb = try TestDb.init(io); + defer tdb.deinit(); + var ctx = tdb.ctx(io); + + try dispatch_insert(&tdb, io, "arr", &.{ + .{ .doc = &.{.{ .key = "_id", .value = .{ .int32 = 1 } }} }, + }); + try testing.expectEqual(@as(?i32, null), try run_for_code(&ctx, "drop", .{ .string = "arr" }, &.{})); + try testing.expect(ctx.engine.get_collection("test", "arr") == null); + + // The namespace is reusable afterwards, and dropping it again is + // NamespaceNotFound rather than a second free. + try dispatch_insert(&tdb, io, "arr", &.{ + .{ .doc = &.{.{ .key = "_id", .value = .{ .int32 = 2 } }} }, + }); + try testing.expectEqual(@as(i32, 1), try doc_count(&ctx, "arr")); + try testing.expectEqual(@as(?i32, null), try run_for_code(&ctx, "drop", .{ .string = "arr" }, &.{})); + try testing.expectEqual( + @as(?i32, @intFromEnum(ErrorCode.namespace_not_found)), + try run_for_code(&ctx, "drop", .{ .string = "arr" }, &.{}), + ); +} + +test "dropDatabase frees its collections without unlocking them" { + // The same shape one level up, and the reason the epilogue is now keyed on + // `kind == .write`: dropDatabase is the one write that never held a + // collection lock, so it never reached the commit/checkpoint epilogue. + var threaded: std.Io.Threaded = .init_single_threaded; + defer threaded.deinit(); + const io = threaded.io(); + var tdb = try TestDb.init(io); + defer tdb.deinit(); + var ctx = tdb.ctx(io); + + try dispatch_insert(&tdb, io, "one", &.{ + .{ .doc = &.{.{ .key = "_id", .value = .{ .int32 = 1 } }} }, + }); + try dispatch_insert(&tdb, io, "two", &.{ + .{ .doc = &.{.{ .key = "_id", .value = .{ .int32 = 2 } }} }, + }); + try testing.expectEqual(@as(?i32, null), try run_for_code(&ctx, "dropDatabase", .{ .int32 = 1 }, &.{})); + try testing.expect(ctx.engine.get_collection("test", "one") == null); + try testing.expect(ctx.engine.get_collection("test", "two") == null); +} + test "a positional update is refused on the wire and stores nothing" { // The end of the chain the unit tests start: the refusal has to reach the // client as a code, and the stored document -- not just the working copy -- 2.39.5