dev/MultiforaDB
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

M3: refuse positional paths instead of destroying arrays; fix the drop use-after-free #5

Merged
dev merged 4 commits from m3-positional-refusal into main 2026-08-10 15:48:40 +00:00
Conversation 0 Commits 4 Files Changed 5 +843 -64
5 changed files with 843 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files

37
PLAN.md
View File

@@ -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:

334
docs/M3_ARRAYFILTERS_DESIGN_REVIEW.md Normal file
View File

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

207
src/commands.zig
View File

@@ -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

301
src/update.zig
View File

@@ -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 `$[<identifier>]` 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 `$[<identifier>]` -- 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 `{"<segment>": ...}` -- 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);
}

28
tests/spec/scorecard.txt
View File

@@ -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