diff --git a/PLAN.md b/PLAN.md index 081d6c2..7eadef4 100644 --- a/PLAN.md +++ b/PLAN.md @@ -1045,6 +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` 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/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. diff --git a/src/commands.zig b/src/commands.zig index 59d2c0b..1b35d38 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 @@ -194,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 }, @@ -309,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 @@ -1951,7 +1975,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 +1999,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 +2104,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 +2123,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 +4134,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 +4190,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 +5553,122 @@ 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 + // -- 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