From a05874d597d43e29883f79d04e3e5cba9e34928a Mon Sep 17 00:00:00 2001 From: "A.Shakhmatov" Date: Mon, 10 Aug 2026 20:31:54 +0300 Subject: [PATCH 1/4] update: a positional path resolves to the elements it names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `$[]`, `$[]` and `$` stop being refused and start being walked. A path with a positional segment names a *set* of concrete paths rather than one -- `y.$[].b` on a two-element array names `y.0.b` and `y.1.b` -- so `resolve` expands it against the document at hand and every operator then walks paths it already knew how to walk. Nothing below `resolve` knows a positional segment exists, which is why `$set`, `$inc`, `$unset`, `$push` and `$pull` all get it at once. The static half -- which identifier binds to which array filter, whether a segment may sit in first position, whether a filter went unused -- depends only on the update and the filters, so it runs once in `validate` before any document is touched. The document-dependent half is resolution itself: an absent or non-array path is a refusal there, because array updates address what is there rather than creating it, unlike `$set` on a plain path. Codes and messages measured on mongod 8.3.7, not recalled. Sixteen shapes were run; the ones that changed what this commit does: - `$[]` in first position answers the *array filter identifier* message, not the `$` one -- mongod treats the two spellings as one check. - an array filter may have several top-level fields as long as they all name the same identifier: `{i.b: 3, i.c: 1}` is legal, `{i.b: 3, j.b: 1}` is not. So the check is on the name, not on the count. - an identifier is `[a-z][a-zA-Z0-9]*`: `aB2` yes, `Ab`, `a_b`, `1x` no. - `$rename` refuses a positional path on *either* end, with its own message for each, so it keeps the plain split rather than resolving. - `$unset` of an element leaves a null in its place rather than shortening the array -- the same answer `$unset: {"y.0": ""}` already gave. - a scalar element with more path below it is PathNotViable (28), not a field to create. Without that check the walk would hand `y.1.b` to `set_path` and it would replace the `7` at `y.1` with `{b: 9}` -- a smaller copy of the destruction this whole walk replaced. One divergence, measured and deliberate: `{"y.b": 3, "y.c": 2}` matches `[{b: 3, c: 1}, {b: 1, c: 2}]` without either element satisfying both, and `$` then picks element 1 on mongod and element 0 here. mongod's answer is an artefact of which predicate last wrote its match position; guessing at it would be worse than recording it. PLAN §6. The command handlers still pass neither the query nor any array filters, so over the wire this is `$[]` working and the other two refusing -- for the right reason and with the right code, which they did not before. The plumbing is the next commit. 221/221 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz. Positional corpus 15 -> 23 of 51. --- src/commands.zig | 142 +++++- src/update.zig | 1123 +++++++++++++++++++++++++++++++++++++++------- 2 files changed, 1086 insertions(+), 179 deletions(-) diff --git a/src/commands.zig b/src/commands.zig index 1b35d38..e38f1ab 100644 --- a/src/commands.zig +++ b/src/commands.zig @@ -2000,7 +2000,7 @@ fn cmd_update(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void { const doc = try doc_tree(reply.arena_alloc(), coll, off); const copy = try clone_doc(reply, doc); var diag: update.Diagnostic = .{}; - update.apply(copy, &.{ .arena = undefined, .pairs = u_doc }, &diag) catch |err| + update.apply(copy, &.{ .arena = undefined, .pairs = u_doc }, .{ .diag = &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 => { @@ -2124,7 +2124,7 @@ fn cmd_find_and_modify(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !v const before = try bson.copy_pairs(arena, target.?.pairs); const copy = try clone_doc(reply, target.?); var diag: update.Diagnostic = .{}; - update.apply(copy, &.{ .arena = undefined, .pairs = u_doc }, &diag) catch |err| + update.apply(copy, &.{ .arena = undefined, .pairs = u_doc }, .{ .diag = &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. @@ -4141,20 +4141,92 @@ fn clone_doc(reply: *wire.Reply, doc: *const bson.Document) !*bson.Document { /// `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. +/// Every message here was measured on mongod 8.3.7 and is reproduced verbatim, +/// except where mongod's text embeds a shell-syntax rendering of the offending +/// BSON (`Cannot create field 'nope' in element {y: [ { b: 3 } ]}`): there is +/// no formatter here that produces it, and a half-copy would be worse than a +/// clear sentence that does not pretend. Codes are exact throughout, and codes +/// are what the corpus asserts. +/// +/// mongod wraps the refusals it only reaches with a document in hand in +/// `Plan executor error during update :: caused by :: `. That prefix is +/// dropped here: it names a mongod component this server does not have. 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( + error.PositionalFirst => return bad_value(reply, if (std.mem.eql(u8, diag.segment, "$")) + try std.fmt.allocPrint( + arena, + "Cannot have positional (i.e. '$') element in the first position in path '{s}'", + .{diag.path}, + ) + else + try std.fmt.allocPrint( + arena, + "Cannot have array filter identifier (i.e. '$[]') element in the " ++ + "first position in path '{s}'", + .{diag.path}, + )), + error.TooManyPositional => return bad_value(reply, try std.fmt.allocPrint( arena, - "the positional operator '{s}' in path '{s}' is not implemented by this server", + "Too many positional (i.e. '$') elements found in path '{s}'", + .{diag.path}, + )), + error.NoArrayFilter => return bad_value(reply, try std.fmt.allocPrint( + arena, + "No array filter found for identifier '{s}' in path '{s}'", .{ diag.segment, diag.path }, )), + error.BadArrayFilterIdentifier => return bad_value(reply, try std.fmt.allocPrint( + arena, + "Error parsing array filter :: caused by :: The top-level field name must be an " ++ + "alphanumeric string beginning with a lowercase letter, found '{s}'", + .{diag.segment}, + )), + error.NoPositionalMatch => return bad_value( + reply, + "The positional operator did not find the match needed from the query.", + ), + error.ArrayPathRequired => return bad_value(reply, try std.fmt.allocPrint( + arena, + "The path '{s}' must exist in the document in order to apply array updates.", + .{diag.path}, + )), + error.NotAnArrayPath => return bad_value(reply, try std.fmt.allocPrint( + arena, + "Cannot apply array updates to non-array element at path '{s}'", + .{diag.path}, + )), + error.RenameDynamicSource => return bad_value(reply, try std.fmt.allocPrint( + arena, + "The source field for $rename may not be dynamic: {s}", + .{diag.path}, + )), + error.RenameDynamicDestination => return bad_value(reply, try std.fmt.allocPrint( + arena, + "The destination field for $rename may not be dynamic: {s}", + .{diag.path}, + )), + error.UnusedArrayFilter => return failed_to_parse(reply, try std.fmt.allocPrint( + arena, + "The array filter for identifier '{s}' was not used in the update", + .{diag.segment}, + )), + error.DuplicateArrayFilter => return failed_to_parse(reply, try std.fmt.allocPrint( + arena, + "Found multiple array filters with the same top-level field name {s}", + .{diag.segment}, + )), + error.EmptyArrayFilter => return failed_to_parse( + reply, + "Cannot use an expression without a top-level field name in arrayFilters", + ), + error.MultipleArrayFilterIdentifiers => return failed_to_parse(reply, try std.fmt.allocPrint( + arena, + "Error parsing array filter :: caused by :: Expected a single top-level field " ++ + "name, found '{s}' and '{s}'", + .{ diag.segment, diag.other }, + )), error.PathNotViable => return reply.put_error( @intFromEnum(ErrorCode.path_not_viable), "PathNotViable", @@ -4190,7 +4262,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. - try update.apply(owned, &.{ .arena = undefined, .pairs = u_doc }, diag); + try update.apply(owned, &.{ .arena = undefined, .pairs = u_doc }, .{ .diag = diag }); return owned; } @@ -5629,9 +5701,9 @@ test "a positional update is refused on the wire and stores nothing" { 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 = "a1", .path = "y.$[i].b", .code = 2 }, // no array filter binds `i` + .{ .coll = "a2", .path = "$[]", .code = 2 }, // positional in first position + .{ .coll = "a3", .path = "y.$.b", .code = 2 }, // no predicate for `$` to use .{ .coll = "a4", .path = "y.nope.b", .code = 28 }, // PathNotViable, same branch }; for (cases) |c| { @@ -5669,6 +5741,48 @@ test "a positional update is refused on the wire and stores nothing" { } } +test "an all-positional update writes every element on the wire" { + // The other end of the same chain: `$[]` reaches the stored document, and + // reaches *all* of it. `distinct` on `y.b` is the check that says so in one + // number -- two values means only one element moved. + 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, "all", &.{ + .{ .doc = &.{ + .{ .key = "_id", .value = .{ .int32 = 1 } }, + .{ .key = "y", .value = .{ .array = &.{ + .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 3 } }} }, + .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 1 } }} }, + } } }, + } }, + }); + + const updates = [_]bson.Value{.{ .doc = &.{ + .{ .key = "q", .value = .{ .doc = &.{} } }, + .{ .key = "u", .value = .{ .doc = &.{ + .{ .key = "$set", .value = .{ .doc = &.{ + .{ .key = "y.$[].b", .value = .{ .int32 = 9 } }, + } } }, + } } }, + } }}; + try testing.expectEqual(@as(?i32, null), try run_for_code(&ctx, "update", .{ .string = "all" }, &.{ + .{ .key = "updates", .value = .{ .array = &updates } }, + })); + + var reply = wire.Reply.init(testing.allocator); + defer reply.deinit(); + const values = try distinct_values(&tdb, io, &reply, "all", &.{ + .{ .key = "key", .value = .{ .string = "y.b" } }, + }); + try testing.expectEqual(@as(usize, 1), values.len); + try testing.expectEqual(@as(i32, 9), 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 0aecf2e..44035cc 100644 --- a/src/update.zig +++ b/src/update.zig @@ -1,6 +1,13 @@ //! Update operators: $set, $unset, $inc, $push, $pull, $rename with -//! dot-path navigation. Mutates the document's pairs in place, allocating -//! from the document's own arena. +//! dot-path navigation, including MongoDB's three positional forms. +//! Mutates the document's pairs in place, allocating from the document's +//! own arena. +//! +//! A path with a positional segment names a *set* of concrete paths rather +//! than one: `y.$[].b` on a two-element array names `y.0.b` and `y.1.b`. +//! `resolve` turns the former into the latter against the document at hand, +//! and every operator then walks the concrete paths it already knew how to +//! walk. Nothing below `resolve` knows a positional segment exists. const std = @import("std"); const bson = @import("bson.zig"); @@ -9,41 +16,246 @@ const query = @import("query.zig"); 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`. + /// A path segment that is not a field to create: a non-numeric name + /// applied to an array, or any name applied to a scalar element a + /// positional segment selected. mongod's `PathNotViable`. PathNotViable, + /// `$`, `$[]` or `$[]` as a path's first segment, where there + /// is no array above it to descend into. + PositionalFirst, + /// More than one `$` in one path. Only the first can be resolved: the + /// query records one matched element per document, not one per level. + TooManyPositional, + /// `$[]` naming an identifier that no array filter binds. + NoArrayFilter, + /// An array filter whose top-level field name is not `[a-z][a-zA-Z0-9]*`. + BadArrayFilterIdentifier, + /// An array filter whose identifier no path in the update mentions. A + /// filter that selects nothing is a typo, so mongod refuses rather than + /// quietly ignoring it -- and so a `$[i]` misspelt on one side or the + /// other is caught by whichever half it broke. + UnusedArrayFilter, + /// Two array filters binding the same identifier. + DuplicateArrayFilter, + /// An array filter with no top-level field name to bind. + EmptyArrayFilter, + /// An array filter whose top-level fields name two different identifiers. + MultipleArrayFilterIdentifiers, + /// A positional segment whose array is absent from the document. Array + /// updates address what is there; unlike `$set` on a plain path, they do + /// not bring it into being. + ArrayPathRequired, + /// A positional segment whose path names something that is not an array. + NotAnArrayPath, + /// `$` where the query holds no predicate on the array, or none that any + /// element satisfies. `$` writes *the element the query matched*, so with + /// no such predicate there is nothing for it to mean. + NoPositionalMatch, + /// `$rename` with a positional source or destination. Both ends of a + /// rename are refused by mongod: a rename moves a field, and a positional + /// path names an element rather than a field. + RenameDynamicSource, + RenameDynamicDestination, 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. +/// What a refusal was about, so the reply can name it instead of saying "bad +/// update". Mostly borrowed from the update document, which outlives the call; +/// `ArrayPathRequired` and `NotAnArrayPath` name a *resolved* prefix +/// (`y.1.c`), which is allocated from the same arena as the document copy. pub const Diagnostic = struct { path: []const u8 = "", segment: []const u8 = "", + /// A second name, for the one message that mentions two. + other: []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". +/// One `arrayFilters` entry: the identifier `$[]` spells, and the +/// predicate that decides which elements it selects. /// -/// 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 { +/// `used` is written by `validate` as it walks the update's paths, and read +/// once afterwards -- which is the whole of `UnusedArrayFilter`. +pub const ArrayFilter = struct { + /// Filled in by `validate` from `pairs`, not by the caller. + ident: []const u8 = "", + /// The predicate, with `ident` still on the front of every key. + pairs: []const bson.Pair, + used: bool = false, +}; + +pub const Options = struct { + /// Bound to the `$[]` segments of the update's paths. + array_filters: []ArrayFilter = &.{}, + /// The command's query. `$` resolves against it, so an update carrying one + /// without this is refused rather than guessing at an element. + query: ?[]const bson.Pair = null, + diag: ?*Diagnostic = null, +}; + +const max_path_segments = 16; + +/// The wrapper field an element is matched under. A predicate written against +/// an element (`{"i.b": 3}`, `{"y.b": 3}`) becomes one written against +/// `{elem_key: }`, which is a document the query engine can answer +/// about without knowing anything about arrays or identifiers. +const elem_key = "e"; + +/// One segment of a parsed path. +const Segment = union(enum) { + literal: []const u8, + /// `$[]` -- every element. + all, + /// `$[]` -- the elements its array filter selects, by index + /// into `Options.array_filters`. + filtered: usize, + /// `$` -- the one element the query matched. + first, +}; + +const Path = struct { + text: []const u8, + segs: [max_path_segments]Segment, + n: usize, +}; + +fn is_positional_segment(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; +fn has_positional(path: []const u8) bool { + var it = std.mem.splitScalar(u8, path, '.'); + while (it.next()) |seg| if (is_positional_segment(seg)) return true; + return false; +} + +/// mongod's rule for an array filter identifier, measured on 8.3.7: `aB2` is +/// accepted, `Ab`, `a_b` and `1x` are not. +fn valid_identifier(ident: []const u8) bool { + if (ident.len == 0) return false; + if (ident[0] < 'a' or ident[0] > 'z') return false; + for (ident[1..]) |c| if (!std.ascii.isAlphanumeric(c)) return false; + return true; +} + +fn find_filter(filters: []const ArrayFilter, ident: []const u8) ?usize { + for (filters, 0..) |f, i| if (std.mem.eql(u8, f.ident, ident)) return i; + return null; +} + +/// Split a path into segments, classifying the positional ones and binding +/// each identifier to its array filter. +/// +/// This is the static half of a positional update: it depends on the update +/// and the filters, never on a document, so it answers the same way for every +/// document a multi-update touches. +fn parse_path(text: []const u8, opts: Options) UpdateError!Path { + var p: Path = .{ .text = text, .segs = undefined, .n = 0 }; + var dollars: usize = 0; + var it = std.mem.splitScalar(u8, text, '.'); + while (it.next()) |seg| { + if (p.n >= max_path_segments) return error.InvalidUpdate; + p.segs[p.n] = seg: { + if (!is_positional_segment(seg)) break :seg .{ .literal = seg }; + if (p.n == 0) { + note(opts.diag, text, seg); + return error.PositionalFirst; + } + if (std.mem.eql(u8, seg, "$")) { + dollars += 1; + break :seg .first; + } + const ident = seg[2 .. seg.len - 1]; + if (ident.len == 0) break :seg .all; + const found = find_filter(opts.array_filters, ident) orelse { + note(opts.diag, text, ident); + return error.NoArrayFilter; + }; + opts.array_filters[found].used = true; + break :seg .{ .filtered = found }; + }; + p.n += 1; + } + if (dollars > 1) { + note(opts.diag, text, "$"); + return error.TooManyPositional; + } + return p; +} + +/// Everything about an update that can be refused without reading a document: +/// the array filters, then every path they bind into. +/// +/// Called by `apply`, and separately by the command handlers *before* they +/// scan for matches -- an update naming an identifier nothing binds is +/// refused whether or not it would have matched anything, which is also what +/// makes `UnusedArrayFilter` observable on a filter that matches no document. +pub fn validate(update: []const bson.Pair, opts: Options) UpdateError!void { + // A replacement carries data, not paths, so nothing here 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)) return; + try bind_array_filters(opts); + for (update) |op| { + const ops = doc_pairs(op.value) orelse continue; + const rename = std.mem.eql(u8, op.key, "$rename"); + for (ops) |p| { + if (rename) { + if (has_positional(p.key)) { + note(opts.diag, p.key, ""); + return error.RenameDynamicSource; + } + if (p.value == .string and has_positional(p.value.string)) { + note(opts.diag, p.value.string, ""); + return error.RenameDynamicDestination; + } + continue; + } + _ = try parse_path(p.key, opts); + } + } + for (opts.array_filters) |f| { + if (f.used) continue; + note(opts.diag, "", f.ident); + return error.UnusedArrayFilter; + } +} + +/// Read each array filter's identifier off its top-level field names. +/// +/// `{"i.b": 3}` binds `i`; `{"i.b": 3, "i.c": 1}` also binds `i` and is legal; +/// `{"i.b": 3, "j.b": 1}` names two and is not. Measured, all four. +fn bind_array_filters(opts: Options) UpdateError!void { + for (opts.array_filters) |*f| { + var ident: ?[]const u8 = null; + for (f.pairs) |p| { + const name = p.key[0 .. std.mem.indexOfScalar(u8, p.key, '.') orelse p.key.len]; + if (ident) |first| { + if (std.mem.eql(u8, first, name)) continue; + if (opts.diag) |d| d.* = .{ .segment = first, .other = name }; + return error.MultipleArrayFilterIdentifiers; + } + ident = name; + } + const name = ident orelse return error.EmptyArrayFilter; + if (!valid_identifier(name)) { + note(opts.diag, "", name); + return error.BadArrayFilterIdentifier; + } + f.ident = name; + } + for (opts.array_filters, 0..) |f, i| { + for (opts.array_filters[i + 1 ..]) |g| { + if (!std.mem.eql(u8, f.ident, g.ident)) continue; + note(opts.diag, "", f.ident); + return error.DuplicateArrayFilter; + } + } +} /// Whether an update document is a *replacement* rather than a set of /// operators. MongoDB decides on the first field and nothing else: a @@ -66,13 +278,13 @@ fn is_operator_key(key: []const u8) bool { pub fn apply( doc: *bson.Document, update: *const bson.Document, - diag: ?*Diagnostic, + opts: Options, ) 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); + // Up front rather than at the point of use, because one update names + // several paths: validating as we walk would refuse the third path having + // already rewritten what the first two named. + try validate(update.pairs, opts); const arena = doc.arena.allocator(); var pairs = try copy_to_list(bson.Pair, arena, doc.pairs); for (update.pairs) |op| { @@ -80,37 +292,264 @@ pub fn apply( // 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, diag); + try apply_operator(arena, &pairs, op.key, op.value, opts); } doc.pairs = try pairs.toOwnedSlice(arena); } -/// Refuse every positional path in the update before any of it is applied. +/// The concrete paths one update path names in `root`. /// -/// 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); - } +/// A path with no positional segment names itself, and is returned unresolved +/// -- including when nothing along it exists, because `$set` on a plain path +/// creates what it needs. A positional segment is different: it addresses +/// elements that are already there, so an absent or non-array path is a +/// refusal rather than a creation. +fn resolve( + arena: std.mem.Allocator, + root: []const bson.Pair, + text: []const u8, + opts: Options, +) UpdateError![]const []const []const u8 { + const path = try parse_path(text, opts); + var out: std.ArrayListUnmanaged([]const []const u8) = .empty; + var walk = Walk{ .arena = arena, .opts = opts, .path = &path, .out = &out }; + try walk.descend(.{ .doc = root }, 0); + return out.items; +} + +/// Expands one parsed path into the concrete paths it names, depth first, so +/// the results come out in document order. +const Walk = struct { + arena: std.mem.Allocator, + opts: Options, + path: *const Path, + out: *std.ArrayListUnmanaged([]const []const u8), + /// The concrete segments chosen so far. Positional ones hold the index + /// they resolved to, which is what makes `y.1.c` the prefix a refusal + /// names. + buf: [max_path_segments][]const u8 = undefined, + + fn descend(self: *Walk, container: ?bson.Value, i: usize) UpdateError!void { + if (i == self.path.n) { + try self.out.append(self.arena, try self.arena.dupe([]const u8, self.buf[0..i])); + return; } + switch (self.path.segs[i]) { + .literal => |lit| { + self.buf[i] = lit; + try self.descend(child_value(container, lit), i + 1); + }, + else => try self.spread(container, i), + } + } + + /// A positional segment: one step down, several ways. + fn spread(self: *Walk, container: ?bson.Value, i: usize) UpdateError!void { + const value = container orelse { + note(self.opts.diag, try self.prefix_text(i), ""); + return error.ArrayPathRequired; + }; + const arr = switch (value) { + .array => |a| a, + else => { + note(self.opts.diag, try self.prefix_text(i), ""); + return error.NotAnArrayPath; + }, + }; + switch (self.path.segs[i]) { + .all => for (arr, 0..) |elem, k| try self.element(elem, k, i), + .filtered => |fi| { + const f = self.opts.array_filters[fi]; + const pred = try rewrite_keys(self.arena, f.pairs, f.ident.len); + for (arr, 0..) |elem, k| { + if (matches_element(self.arena, .{ .wrapped = pred }, elem)) { + try self.element(elem, k, i); + } + } + }, + .first => { + const k = try self.first_match(arr, i); + try self.element(arr[k], k, i); + }, + .literal => unreachable, + } + } + + fn element(self: *Walk, elem: bson.Value, k: usize, i: usize) UpdateError!void { + // A scalar element with more path below it is not a field to create. + // Without this the walk would hand `y.1.b` to `set_path`, which would + // replace the `7` at `y.1` with `{b: 9}` -- the same class of silent + // destruction the positional forms themselves used to cause. + if (i + 1 < self.path.n) switch (self.path.segs[i + 1]) { + .literal => |lit| switch (elem) { + .doc, .array => {}, + else => { + note(self.opts.diag, self.path.text, lit); + return error.PathNotViable; + }, + }, + else => {}, + }; + self.buf[i] = try std.fmt.allocPrint(self.arena, "{d}", .{k}); + try self.descend(elem, i + 1); + } + + /// The resolved path above segment `i`: `y`, or `y.1.c` under a positional + /// segment that already chose an element. This is the path a refusal + /// names, and mongod names the same one. + fn prefix_text(self: *Walk, i: usize) UpdateError![]const u8 { + return std.mem.join(self.arena, ".", self.buf[0..i]); + } + + /// Which element the query matched, for `$`. + /// + /// The query is written against the *unresolved* path -- `{"y.b": 3}` + /// selects an element of `y` however deep a positional segment above it + /// went -- so the predicates are gathered by the path with its positional + /// segments elided, and then run against the concrete array here. + fn first_match(self: *Walk, arr: []const bson.Value, i: usize) UpdateError!usize { + const q = self.opts.query orelse return self.no_match(); + var preds: std.ArrayListUnmanaged(ElemPred) = .empty; + try collect_preds(self.arena, q, try self.query_prefix(i), &preds); + if (preds.items.len == 0) return self.no_match(); + for (arr, 0..) |elem, k| { + var all = true; + for (preds.items) |p| { + if (!matches_element(self.arena, p, elem)) { + all = false; + break; + } + } + if (all) return k; + } + // Two predicates on one array need not agree on an element, and the + // document still matched: `{"y.b": 3, "y.c": 2}` matches + // `[{b: 3, c: 1}, {b: 1, c: 2}]` without either element satisfying + // both. mongod answers 1 here; this answers 0. Recorded in PLAN §6 + // rather than guessed at -- the rule mongod uses is an artefact of + // which predicate last wrote its match position. + for (arr, 0..) |elem, k| { + for (preds.items) |p| if (matches_element(self.arena, p, elem)) return k; + } + return self.no_match(); + } + + fn no_match(self: *Walk) UpdateError { + note(self.opts.diag, self.path.text, "$"); + return error.NoPositionalMatch; + } + + fn query_prefix(self: *Walk, i: usize) UpdateError![]const u8 { + var parts: [max_path_segments][]const u8 = undefined; + var n: usize = 0; + for (self.path.segs[0..i]) |seg| switch (seg) { + .literal => |lit| { + parts[n] = lit; + n += 1; + }, + else => {}, + }; + return std.mem.join(self.arena, ".", parts[0..n]); + } +}; + +fn child_value(container: ?bson.Value, key: []const u8) ?bson.Value { + return switch (container orelse return null) { + .doc => |sub| bson.get_pair(sub, key), + .array => |arr| blk: { + const index = parse_index(key) orelse break :blk null; + break :blk if (index < arr.len) arr[index] else null; + }, + else => null, + }; +} + +/// A predicate about one array element. +const ElemPred = union(enum) { + /// Keys rewritten onto `elem_key`, matched against `{e: }`. + wrapped: []const bson.Pair, + /// `$elemMatch`'s own document, matched against the element directly. + direct: []const bson.Pair, +}; + +fn matches_element(arena: std.mem.Allocator, pred: ElemPred, elem: bson.Value) bool { + switch (pred) { + .wrapped => |pairs| { + const wrapper = [_]bson.Pair{.{ .key = elem_key, .value = elem }}; + return query.matches( + arena, + &.{ .arena = undefined, .pairs = pairs }, + &.{ .arena = undefined, .pairs = &wrapper }, + ) catch false; + }, + .direct => |pairs| { + const sub = switch (elem) { + .doc => |d| d, + else => return false, + }; + return query.matches( + arena, + &.{ .arena = undefined, .pairs = pairs }, + &.{ .arena = undefined, .pairs = sub }, + ) catch false; + }, } } -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; +/// Re-point a predicate's keys from whatever they were written against onto +/// `elem_key`: `i.b` with `prefix_len` 1 becomes `e.b`, and `i` becomes `e`. +fn rewrite_keys( + arena: std.mem.Allocator, + pairs: []const bson.Pair, + prefix_len: usize, +) UpdateError![]const bson.Pair { + const out = try arena.alloc(bson.Pair, pairs.len); + for (pairs, 0..) |p, i| { + out[i] = .{ + .key = try std.mem.concat(arena, u8, &.{ elem_key, p.key[prefix_len..] }), + .value = p.value, + }; + } + return out; +} + +/// The query's predicates about elements of the array at `prefix`. +/// +/// `$and` is descended into because a driver writes one whenever two +/// predicates share a field; `$or` is not, because an element satisfying one +/// branch says nothing about the document having matched through it. +fn collect_preds( + arena: std.mem.Allocator, + q: []const bson.Pair, + prefix: []const u8, + out: *std.ArrayListUnmanaged(ElemPred), +) UpdateError!void { + for (q) |p| { + if (std.mem.eql(u8, p.key, "$and")) { + const branches = switch (p.value) { + .array => |a| a, + else => continue, + }; + for (branches) |b| switch (b) { + .doc => |sub| try collect_preds(arena, sub, prefix, out), + else => {}, + }; + continue; + } + if (!std.mem.startsWith(u8, p.key, prefix)) continue; + const rest = p.key[prefix.len..]; + if (rest.len != 0 and rest[0] != '.') continue; + if (rest.len == 0) { + if (p.value == .doc) { + if (bson.get_pair(p.value.doc, "$elemMatch")) |em| { + if (em == .doc) { + try out.append(arena, .{ .direct = em.doc }); + continue; + } + } + } + } + try out.append(arena, .{ .wrapped = try rewrite_keys(arena, &.{p}, prefix.len) }); } } @@ -169,91 +608,94 @@ fn apply_operator( pairs: *std.ArrayListUnmanaged(bson.Pair), op: []const u8, value: bson.Value, - diag: ?*Diagnostic, + opts: Options, ) UpdateError!void { + const diag = opts.diag; if (std.mem.eql(u8, op, "$set")) { const ops = doc_pairs(value) orelse return error.InvalidUpdate; for (ops) |p| { 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), p.key, diag); + for (try resolve(arena, pairs.items, p.key, opts)) |segs| { + try set_path(arena, pairs, segs, try bson.copy_value(arena, p.value), p.key, diag); + } } return; } if (std.mem.eql(u8, op, "$unset")) { const ops = doc_pairs(value) orelse return error.InvalidUpdate; for (ops) |p| { - var segs: [max_path_segments][]const u8 = undefined; - const n = split_path(p.key, &segs) orelse continue; - unset_path(arena, pairs, segs[0..n]); + for (try resolve(arena, pairs.items, p.key, opts)) |segs| { + unset_path(arena, pairs, segs); + } } return; } if (std.mem.eql(u8, op, "$inc")) { const ops = doc_pairs(value) orelse return error.InvalidUpdate; for (ops) |p| { - var segs: [max_path_segments][]const u8 = undefined; - const n = split_path(p.key, &segs) orelse return error.InvalidUpdate; - 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, p.key, diag); + for (try resolve(arena, pairs.items, p.key, opts)) |segs| { + const current = get_value(pairs.items, segs) 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, sum, p.key, diag); + } } return; } if (std.mem.eql(u8, op, "$push")) { const ops = doc_pairs(value) orelse return error.InvalidUpdate; for (ops) |p| { - var segs: [max_path_segments][]const u8 = undefined; - const n = split_path(p.key, &segs) orelse return error.InvalidUpdate; - const current_opt = get_value(pairs.items, segs[0..n]); - var items: std.ArrayListUnmanaged(bson.Value) = .empty; - defer items.deinit(arena); - if (current_opt) |current| { - switch (current) { - .array => |arr| try items.appendSlice(arena, arr), - .null => {}, - else => return error.InvalidUpdate, // non-array field + for (try resolve(arena, pairs.items, p.key, opts)) |segs| { + const current_opt = get_value(pairs.items, segs); + var items: std.ArrayListUnmanaged(bson.Value) = .empty; + defer items.deinit(arena); + if (current_opt) |current| { + switch (current) { + .array => |arr| try items.appendSlice(arena, arr), + .null => {}, + else => return error.InvalidUpdate, // non-array field + } } - } - if (p.value == .doc) { - if (bson.get_pair(p.value.doc, "$each")) |each| { - const arr = switch (each) { - .array => |a| a, - 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) }, p.key, diag); - continue; + if (p.value == .doc) { + if (bson.get_pair(p.value.doc, "$each")) |each| { + const arr = switch (each) { + .array => |a| a, + else => return error.InvalidUpdate, + }; + for (arr) |item| try items.append(arena, try bson.copy_value(arena, item)); + try set_path(arena, pairs, segs, .{ .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, .{ .array = try items.toOwnedSlice(arena) }, p.key, diag); } - try items.append(arena, try bson.copy_value(arena, p.value)); - try set_path(arena, pairs, segs[0..n], .{ .array = try items.toOwnedSlice(arena) }, p.key, diag); } return; } if (std.mem.eql(u8, op, "$pull")) { const ops = doc_pairs(value) orelse return error.InvalidUpdate; for (ops) |p| { - var segs: [max_path_segments][]const u8 = undefined; - const n = split_path(p.key, &segs) orelse return error.InvalidUpdate; - const current = get_value(pairs.items, segs[0..n]) orelse continue; - const arr = switch (current) { - .array => |a| a, - else => return error.InvalidUpdate, - }; - var items: std.ArrayListUnmanaged(bson.Value) = .empty; - defer items.deinit(arena); - for (arr) |elem| { - if (!pull_matches(arena, p.value, elem)) { - try items.append(arena, elem); + for (try resolve(arena, pairs.items, p.key, opts)) |segs| { + const current = get_value(pairs.items, segs) orelse continue; + const arr = switch (current) { + .array => |a| a, + else => return error.InvalidUpdate, + }; + var items: std.ArrayListUnmanaged(bson.Value) = .empty; + defer items.deinit(arena); + for (arr) |elem| { + if (!pull_matches(arena, p.value, elem)) { + try items.append(arena, elem); + } } + try set_path(arena, pairs, segs, .{ .array = try items.toOwnedSlice(arena) }, p.key, diag); } - try set_path(arena, pairs, segs[0..n], .{ .array = try items.toOwnedSlice(arena) }, p.key, diag); } return; } + // `$rename` alone keeps the plain split: `validate` has already refused a + // positional path on either end of it, which is what mongod does too. if (std.mem.eql(u8, op, "$rename")) { const ops = doc_pairs(value) orelse return error.InvalidUpdate; for (ops) |p| { @@ -423,6 +865,25 @@ fn unset_path( unset_path(arena, &sub_pairs, segs[1..]); pairs.items[idx].value = .{ .doc = sub_pairs.items }; }, + // Unsetting an element leaves a null in its place rather than + // shortening the array: measured on both `$unset: {"y.0": ""}` and + // `$unset: {"y.$[]": ""}`, which is the same rule reached two ways. + .array => |arr| { + const index = parse_index(segs[1]) orelse return; + if (index >= arr.len) return; + var items = copy_to_list(bson.Value, arena, arr) catch return; + if (segs.len == 2) { + items.items[index] = .null; + } else switch (items.items[index]) { + .doc => |sub| { + var sub_pairs = copy_to_list(bson.Pair, arena, sub) catch return; + unset_path(arena, &sub_pairs, segs[2..]); + items.items[index] = .{ .doc = sub_pairs.items }; + }, + else => return, + } + pairs.items[idx].value = .{ .array = items.items }; + }, else => {}, } } @@ -508,7 +969,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); @@ -528,13 +989,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); @@ -542,7 +1003,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); } @@ -558,7 +1019,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); @@ -568,7 +1029,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" { @@ -585,7 +1046,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. @@ -606,7 +1067,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(&.{}), null); + try apply(&doc, &doc_of(&.{}), .{}); try testing.expectEqual(@as(usize, 1), doc.pairs.len); try testing.expectEqualStrings("_id", doc.pairs[0].key); } @@ -628,7 +1089,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. @@ -637,14 +1098,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); } @@ -661,7 +1122,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); @@ -679,13 +1140,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" { @@ -697,17 +1158,57 @@ test "is_replacement decides on the first field only" { try testing.expect(is_replacement(&.{.{ .key = "", .value = .{ .int32 = 1 } }})); } +fn doc_with(gpa: std.mem.Allocator, pairs: []const bson.Pair) !bson.Document { + var doc = bson.Document{ .arena = std.heap.ArenaAllocator.init(gpa), .pairs = &.{} }; + doc.pairs = try doc.arena.allocator().dupe(bson.Pair, pairs); + return doc; +} + /// 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, &.{ + return doc_with(arena, &.{ .{ .key = "y", .value = .{ .array = &.{ .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 3 } }} }, .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 1 } }} }, } } }, }); - return doc; +} + +/// `$set` of one path, the shape most of the cases below need. Owns its two +/// pairs: a helper *returning* the document would hand back a slice of its own +/// dead frame. +const SetOne = struct { + inner: [1]bson.Pair, + outer: [1]bson.Pair = undefined, + + fn init(path: []const u8, value: bson.Value) SetOne { + return .{ .inner = .{.{ .key = path, .value = value }} }; + } + + fn doc(self: *SetOne) bson.Document { + self.outer = .{.{ .key = "$set", .value = .{ .doc = &self.inner } }}; + return doc_of(&self.outer); + } +}; + +fn apply_set( + doc: *bson.Document, + path: []const u8, + value: bson.Value, + opts: Options, +) UpdateError!void { + var u = SetOne.init(path, value); + return apply(doc, &u.doc(), opts); +} + +/// The `b` of `y[k]`, which is what most of these updates move. +fn y_b(doc: *const bson.Document, k: usize) !i32 { + const y = doc.get("y") orelse return error.TestUnexpectedResult; + if (y != .array or k >= y.array.len) return error.TestUnexpectedResult; + const elem = y.array[k]; + if (elem != .doc) return error.TestUnexpectedResult; + return (bson.get_pair(elem.doc, "b") orelse return error.TestUnexpectedResult).int32; } fn expect_y_untouched(doc: *const bson.Document) !void { @@ -721,79 +1222,371 @@ fn expect_y_untouched(doc: *const bson.Document) !void { 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 "$[] writes every element of the array" { + // The all-positional operator, which the pinned crud corpus does not + // contain a single case of. Mutation check: make `spread`'s `.all` arm + // stop after the first element and the second half goes red. + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + try apply_set(&doc, "y.$[].b", .{ .int32 = 9 }, .{}); + try testing.expectEqual(@as(i32, 9), try y_b(&doc, 0)); + try testing.expectEqual(@as(i32, 9), try y_b(&doc, 1)); +} + +test "every operator goes through a positional segment, not just $set" { + // The destruction this replaced was below the operator, in the shared path + // walk: `$inc` through `$[]` stored its operand instead of incrementing. + // So the walk has to be below the operator too. + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + try apply(&doc, &doc_of(&.{ + .{ .key = "$inc", .value = .{ .doc = &.{.{ .key = "y.$[].b", .value = .{ .int32 = 10 } }} } }, + }), .{}); + try testing.expectEqual(@as(i32, 13), try y_b(&doc, 0)); + try testing.expectEqual(@as(i32, 11), try y_b(&doc, 1)); + + // `$unset` through an element removes the field; `$unset` *of* an element + // leaves a null in its place rather than shortening the array. + try apply(&doc, &doc_of(&.{ + .{ .key = "$unset", .value = .{ .doc = &.{.{ .key = "y.$[].b", .value = .{ .string = "" } }} } }, + }), .{}); + try testing.expectEqual(@as(usize, 0), doc.get("y").?.array[0].doc.len); + try apply(&doc, &doc_of(&.{ + .{ .key = "$unset", .value = .{ .doc = &.{.{ .key = "y.$[]", .value = .{ .string = "" } }} } }, + }), .{}); + try testing.expect(doc.get("y").?.array[0] == .null); + try testing.expect(doc.get("y").?.array[1] == .null); +} + +test "$[] over an empty array writes nothing at all" { + // Zero concrete paths, so the operator never runs -- which is how the + // document comes back byte-identical and the reply says modified 0. + var doc = try doc_with(testing.allocator, &.{ + .{ .key = "y", .value = .{ .array = &.{} } }, + }); + defer doc.arena.deinit(); + try apply_set(&doc, "y.$[].b", .{ .int32 = 9 }, .{}); + try testing.expectEqual(@as(usize, 0), doc.get("y").?.array.len); +} + +test "nested $[] is a cross product over both levels" { + var doc = try doc_with(testing.allocator, &.{ + .{ .key = "y", .value = .{ .array = &.{ + .{ .doc = &.{.{ .key = "c", .value = .{ .array = &.{ + .{ .doc = &.{.{ .key = "d", .value = .{ .int32 = 1 } }} }, + .{ .doc = &.{.{ .key = "d", .value = .{ .int32 = 2 } }} }, + } } }} }, + .{ .doc = &.{.{ .key = "c", .value = .{ .array = &.{ + .{ .doc = &.{.{ .key = "d", .value = .{ .int32 = 3 } }} }, + } } }} }, + } } }, + }); + defer doc.arena.deinit(); + try apply_set(&doc, "y.$[].c.$[].d", .{ .int32 = 0 }, .{}); + const y = doc.get("y").?.array; + for (y) |outer| { + for (outer.doc[0].value.array) |inner| { + try testing.expectEqual(@as(i32, 0), inner.doc[0].value.int32); + } } } -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 "an array update addresses an array that is there, and refuses one that is not" { + // The difference between a positional segment and a plain one: `$set` on + // `a.b.c` creates what it needs, and this does not. Both refusals are code + // 2 on mongod, with different text, so they are different errors here. + var missing = try doc_with(testing.allocator, &.{.{ .key = "z", .value = .{ .int32 = 1 } }}); + defer missing.arena.deinit(); + var diag: Diagnostic = .{}; + try testing.expectError( + error.ArrayPathRequired, + apply_set(&missing, "y.$[].b", .{ .int32 = 9 }, .{ .diag = &diag }), + ); + try testing.expectEqualStrings("y", diag.path); + + var scalar = try doc_with(testing.allocator, &.{.{ .key = "y", .value = .{ .int32 = 5 } }}); + defer scalar.arena.deinit(); + try testing.expectError( + error.NotAnArrayPath, + apply_set(&scalar, "y.$[].b", .{ .int32 = 9 }, .{ .diag = &diag }), + ); + try testing.expectEqualStrings("y", diag.path); + + // Under a positional segment the refusal names the *resolved* prefix, so + // it says which element was missing the array. mongod names the same one. + var nested = try doc_with(testing.allocator, &.{ + .{ .key = "y", .value = .{ .array = &.{ + .{ .doc = &.{.{ .key = "c", .value = .{ .array = &.{} } }} }, + .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 2 } }} }, + } } }, + }); + defer nested.arena.deinit(); + try testing.expectError( + error.ArrayPathRequired, + apply_set(&nested, "y.$[].c.$[].d", .{ .int32 = 0 }, .{ .diag = &diag }), + ); + try testing.expectEqualStrings("y.1.c", diag.path); } -test "$rename checks its destination, which is the value not the key" { +test "a scalar element with more path below it is PathNotViable" { + // Mutation check: delete the `.literal` arm of `Walk.element` and this + // update answers ok having replaced the `7` with `{b: 9}` -- a smaller + // version of the destruction the whole walk exists to have stopped. + var doc = try doc_with(testing.allocator, &.{ + .{ .key = "y", .value = .{ .array = &.{ + .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 1 } }} }, + .{ .int32 = 7 }, + } } }, + }); + defer doc.arena.deinit(); + try testing.expectError( + error.PathNotViable, + apply_set(&doc, "y.$[].b", .{ .int32 = 9 }, .{}), + ); + // And the first element, which the walk had already resolved, is untouched + // too: the refusal is taken during resolution, before any write. + try testing.expectEqual(@as(i32, 1), try y_b(&doc, 0)); + try testing.expect(doc.get("y").?.array[1] == .int32); +} + +test "$[] writes only the elements its array filter selects" { + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + var filters = [_]ArrayFilter{ + .{ .pairs = &.{.{ .key = "i.b", .value = .{ .int32 = 3 } }} }, + }; + try apply_set(&doc, "y.$[i].b", .{ .int32 = 2 }, .{ .array_filters = &filters }); + try testing.expectEqual(@as(i32, 2), try y_b(&doc, 0)); + try testing.expectEqual(@as(i32, 1), try y_b(&doc, 1)); + try testing.expect(filters[0].used); + + // The identifier as the leaf replaces the whole element, and a filter on + // the identifier itself is how an array of scalars is addressed. + var scalars = try doc_with(testing.allocator, &.{ + .{ .key = "y", .value = .{ .array = &.{ .{ .int32 = 1 }, .{ .int32 = 2 }, .{ .int32 = 3 }, .{ .int32 = 2 } } } }, + }); + defer scalars.arena.deinit(); + var by_value = [_]ArrayFilter{ + .{ .pairs = &.{.{ .key = "i", .value = .{ .int32 = 2 } }} }, + }; + try apply_set(&scalars, "y.$[i]", .{ .int32 = 9 }, .{ .array_filters = &by_value }); + const y = scalars.get("y").?.array; + try testing.expectEqual(@as(i32, 1), y[0].int32); + try testing.expectEqual(@as(i32, 9), y[1].int32); + try testing.expectEqual(@as(i32, 3), y[2].int32); + try testing.expectEqual(@as(i32, 9), y[3].int32); +} + +test "an array filter that selects nothing leaves the document alone" { + // Not an error: matched, modified nothing. The reply's `modifiedCount` is + // the only place this shows, which is why the corpus records outcomes. + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + var filters = [_]ArrayFilter{ + .{ .pairs = &.{.{ .key = "i.b", .value = .{ .int32 = 4 } }} }, + }; + try apply_set(&doc, "y.$[i].b", .{ .int32 = 2 }, .{ .array_filters = &filters }); + try expect_y_untouched(&doc); +} + +test "an identifier and its array filter each refuse the other's absence" { var doc = try array_doc(testing.allocator); defer doc.arena.deinit(); var diag: Diagnostic = .{}; - try testing.expectError(error.PositionalUnsupported, apply(&doc, &doc_of(&.{ + + // A path naming an identifier nothing binds. + var one = [_]ArrayFilter{.{ .pairs = &.{.{ .key = "i.b", .value = .{ .int32 = 3 } }} }}; + try testing.expectError( + error.NoArrayFilter, + apply_set(&doc, "y.$[k].b", .{ .int32 = 2 }, .{ .array_filters = &one, .diag = &diag }), + ); + try testing.expectEqualStrings("k", diag.segment); + try testing.expectEqualStrings("y.$[k].b", diag.path); + + // A filter no path names. Mutation check: drop the `used` loop at the end + // of `validate` and this goes green -- and a misspelt identifier on the + // filter side becomes a silent no-op update. + var unused = [_]ArrayFilter{.{ .pairs = &.{.{ .key = "i.b", .value = .{ .int32 = 3 } }} }}; + try testing.expectError( + error.UnusedArrayFilter, + apply_set(&doc, "y.b", .{ .int32 = 2 }, .{ .array_filters = &unused, .diag = &diag }), + ); + try testing.expectEqualStrings("i", diag.segment); + try expect_y_untouched(&doc); +} + +test "an array filter names exactly one identifier, spelled the one legal way" { + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + var diag: Diagnostic = .{}; + + var empty = [_]ArrayFilter{.{ .pairs = &.{} }}; + try testing.expectError( + error.EmptyArrayFilter, + apply_set(&doc, "y.$[i].b", .{ .int32 = 2 }, .{ .array_filters = &empty }), + ); + + var two = [_]ArrayFilter{.{ .pairs = &.{ + .{ .key = "i.b", .value = .{ .int32 = 3 } }, + .{ .key = "j.b", .value = .{ .int32 = 1 } }, + } }}; + try testing.expectError( + error.MultipleArrayFilterIdentifiers, + apply_set(&doc, "y.$[i].b", .{ .int32 = 2 }, .{ .array_filters = &two, .diag = &diag }), + ); + try testing.expectEqualStrings("i", diag.segment); + try testing.expectEqualStrings("j", diag.other); + + var dup = [_]ArrayFilter{ + .{ .pairs = &.{.{ .key = "i.b", .value = .{ .int32 = 3 } }} }, + .{ .pairs = &.{.{ .key = "i.b", .value = .{ .int32 = 1 } }} }, + }; + try testing.expectError( + error.DuplicateArrayFilter, + apply_set(&doc, "y.$[i].b", .{ .int32 = 2 }, .{ .array_filters = &dup }), + ); + + // Two fields naming the *same* identifier are one predicate, not two + // identifiers -- measured, and the reason the check is on the name rather + // than on the count of fields. + var same = [_]ArrayFilter{.{ .pairs = &.{ + .{ .key = "i.b", .value = .{ .int32 = 3 } }, + .{ .key = "i.c", .value = .{ .int32 = 1 } }, + } }}; + try apply_set(&doc, "y.$[i].b", .{ .int32 = 2 }, .{ .array_filters = &same }); + try expect_y_untouched(&doc); // no element has both, so nothing moved + + for ([_][]const u8{ "1x", "Ab", "a_b" }) |bad| { + var f = [_]ArrayFilter{.{ .pairs = &.{.{ .key = bad, .value = .{ .int32 = 3 } }} }}; + try testing.expectError( + error.BadArrayFilterIdentifier, + apply_set(&doc, "y.$[i].b", .{ .int32 = 2 }, .{ .array_filters = &f, .diag = &diag }), + ); + try testing.expectEqualStrings(bad, diag.segment); + } +} + +test "$ writes the one element the query matched" { + // The operator that needs something the document alone does not hold. + var doc = try doc_with(testing.allocator, &.{ + .{ .key = "y", .value = .{ .array = &.{ + .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 1 } }} }, + .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 1 } }} }, + .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 2 } }} }, + } } }, + }); + defer doc.arena.deinit(); + // Two elements qualify and only the first is written. Mutation check: make + // `first_match` return the last index instead and this goes red both ways. + try apply_set(&doc, "y.$.b", .{ .int32 = 7 }, .{ + .query = &.{.{ .key = "y.b", .value = .{ .int32 = 1 } }}, + }); + try testing.expectEqual(@as(i32, 7), try y_b(&doc, 0)); + try testing.expectEqual(@as(i32, 1), try y_b(&doc, 1)); + try testing.expectEqual(@as(i32, 2), try y_b(&doc, 2)); +} + +test "$ takes the element from a predicate on any field of it, including an operator" { + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + // `{y.b: {$lt: 2}}` matches the second element, not the first. + try apply_set(&doc, "y.$.b", .{ .int32 = 7 }, .{ + .query = &.{.{ .key = "y.b", .value = .{ .doc = &.{ + .{ .key = "$lt", .value = .{ .int32 = 2 } }, + } } }}, + }); + try testing.expectEqual(@as(i32, 3), try y_b(&doc, 0)); + try testing.expectEqual(@as(i32, 7), try y_b(&doc, 1)); + + // `$elemMatch` names the element directly, and is the one predicate shape + // that is matched against the element rather than through a wrapper. + var em = try array_doc(testing.allocator); + defer em.arena.deinit(); + try apply_set(&em, "y.$.b", .{ .int32 = 7 }, .{ + .query = &.{.{ .key = "y", .value = .{ .doc = &.{ + .{ .key = "$elemMatch", .value = .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 1 } }} } }, + } } }}, + }); + try testing.expectEqual(@as(i32, 3), try y_b(&em, 0)); + try testing.expectEqual(@as(i32, 7), try y_b(&em, 1)); +} + +test "$ with no predicate on the array is refused, and writes nothing" { + // The query matched the document by `_id`, so nothing recorded which + // element `$` meant. mongod refuses this, and the document is what makes + // that the only safe answer: any element would be a guess. + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + try testing.expectError(error.NoPositionalMatch, apply_set(&doc, "y.$.b", .{ .int32 = 7 }, .{ + .query = &.{.{ .key = "_id", .value = .{ .int32 = 1 } }}, + })); + try expect_y_untouched(&doc); + + // And with no query at all -- the upsert path, where the document being + // built never matched anything. + try testing.expectError( + error.NoPositionalMatch, + apply_set(&doc, "y.$.b", .{ .int32 = 7 }, .{}), + ); + try expect_y_untouched(&doc); +} + +test "a positional segment is refused in first position, and $ twice in one path" { + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + var diag: Diagnostic = .{}; + for ([_][]const u8{ "$", "$[]", "$[i]" }) |path| { + try testing.expectError( + error.PositionalFirst, + apply_set(&doc, path, .{ .int32 = 2 }, .{ .diag = &diag }), + ); + try testing.expectEqualStrings(path, diag.segment); + } + // Two `$` cannot both be resolved: the query records one matched element + // per document, not one per level. + try testing.expectError( + error.TooManyPositional, + apply_set(&doc, "y.$.c.$.d", .{ .int32 = 0 }, .{ .diag = &diag }), + ); + try testing.expectEqualStrings("y.$.c.$.d", diag.path); + try expect_y_untouched(&doc); +} + +test "$rename refuses a positional path on either end" { + // A rename moves a field; a positional path names an element. mongod + // refuses both ends with its own message for each, and the destination is + // the *value* of the pair, which is the end easiest to forget. + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + var diag: Diagnostic = .{}; + try testing.expectError(error.RenameDynamicSource, apply(&doc, &doc_of(&.{ + .{ .key = "$rename", .value = .{ .doc = &.{ + .{ .key = "y.$[].b", .value = .{ .string = "z" } }, + } } }, + }), .{ .diag = &diag })); + try testing.expectEqualStrings("y.$[].b", diag.path); + + try testing.expectError(error.RenameDynamicDestination, apply(&doc, &doc_of(&.{ .{ .key = "$rename", .value = .{ .doc = &.{ .{ .key = "y", .value = .{ .string = "z.$[i]" } }, } } }, - }), &diag)); + }), .{ .diag = &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. + // The static half of the refusal is taken before the first write, so the + // good path in this update does not land either. Mutation check: move the + // `validate` call inside the operator loop and `ok` appears. var doc = try array_doc(testing.allocator); defer doc.arena.deinit(); - try testing.expectError(error.PositionalUnsupported, apply(&doc, &doc_of(&.{ + try testing.expectError(error.NoArrayFilter, 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); } @@ -810,7 +1603,7 @@ test "a non-numeric segment under an array is PathNotViable, not a new field" { var diag: Diagnostic = .{}; try testing.expectError(error.PathNotViable, apply(&doc, &doc_of(&.{ .{ .key = "$set", .value = .{ .doc = &.{.{ .key = path, .value = .{ .int32 = 2 } }} } }, - }), &diag)); + }), .{ .diag = &diag })); try expect_y_untouched(&doc); } } @@ -826,7 +1619,7 @@ test "a numeric segment still addresses an array element" { .{ .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); @@ -842,7 +1635,7 @@ test "a replacement is not a path, so it is not refused" { 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); } -- 2.39.5 From 200228b0cb5135270e900220c442ec32a197145c Mon Sep 17 00:00:00 2001 From: "A.Shakhmatov" Date: Mon, 10 Aug 2026 20:36:55 +0300 Subject: [PATCH 2/4] commands: arrayFilters and the query reach the update The engine landed last commit with nothing feeding it: `$[]` had no filters to bind and `$` had no query to resolve against, so both refused correctly and uselessly. This is the plumbing. `arrayFilters` is read per update statement on `update` and once on `findAndModify`, which is where each command carries it. Only the *shape* is checked here -- an array, of documents, TypeMismatch (14) with mongod's own field names for either -- because which identifier a filter names, whether it is spelled legally and whether the update ever uses it all need the update's paths, and those belong to `update.validate`. `validate` is called before `scan_matching`, not inside the per-document loop, and that placement is load-bearing in both directions: an array filter the update never uses is refused (9) even when the query matches nothing at all, and an identifier nothing binds is refused (2) before a single document is read. Both measured; a test pins each, with the mutation that reddens it named in the comment. Positional corpus 23 -> 51 of 51, the gate green. Pinned crud scorecard 204 -> 218 pass, 87 -> 73 fail: the whole arrayFilters cluster, which until two commits ago was answering ok: 1 having replaced the array with a document keyed by the path segment's literal text. 222/222 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz. --- src/commands.zig | 197 ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 185 insertions(+), 12 deletions(-) diff --git a/src/commands.zig b/src/commands.zig index e38f1ab..0828e8d 100644 --- a/src/commands.zig +++ b/src/commands.zig @@ -1969,15 +1969,30 @@ fn cmd_update(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void { return failed_to_parse(reply, "multi update is not supported for replacement-style update"); } + var diag: update.Diagnostic = .{}; + const opts = update.Options{ + .array_filters = try parse_array_filters( + reply, + spec.get("arrayFilters"), + "update.updates.arrayFilters", + ) orelse return, + .query = q, + .diag = &diag, + }; + // Before the scan, not after: an update naming an identifier nothing + // binds is refused whether or not it would have matched anything, and + // an array filter the update never uses is refused even when the whole + // command was a no-op. Both measured. + update.validate(u_doc, opts) catch |err| return update_refusal(reply, err, diag); + var matched: std.ArrayListUnmanaged(u64) = .empty; defer matched.deinit(ctx.gpa); _ = try scan_matching(ctx, db_name, coll_name, q, if (multi) 0 else 1, &matched); if (matched.items.len == 0) { if (upsert) { - 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); + const new_doc = build_upsert_doc(reply, q, u_doc, opts) catch |err| + return update_refusal(reply, err, 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, @@ -1999,8 +2014,7 @@ 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); - var diag: update.Diagnostic = .{}; - update.apply(copy, &.{ .arena = undefined, .pairs = u_doc }, .{ .diag = &diag }) catch |err| + update.apply(copy, &.{ .arena = undefined, .pairs = u_doc }, opts) 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 => { @@ -2080,6 +2094,20 @@ fn cmd_find_and_modify(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !v if (remove and do_update) return bad_value(reply, "remove and update are mutually exclusive"); if (!remove and !do_update) return bad_value(reply, "must specify update or remove"); + var diag: update.Diagnostic = .{}; + const opts = update.Options{ + .array_filters = try parse_array_filters( + reply, + msg.body.get("arrayFilters"), + "findAndModify.arrayFilters", + ) orelse return, + .query = q, + .diag = &diag, + }; + if (doc_arg(msg.body.get("update"))) |u| { + update.validate(u, opts) catch |err| return update_refusal(reply, err, diag); + } + var matched: std.ArrayListUnmanaged(u64) = .empty; defer matched.deinit(ctx.gpa); // Without a sort, only the first match is ever used. @@ -2104,9 +2132,8 @@ 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"); - 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); + const new_doc = build_upsert_doc(reply, q, u_doc, opts) catch |err| + return update_refusal(reply, err, 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, @@ -2123,8 +2150,7 @@ 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.?); - var diag: update.Diagnostic = .{}; - update.apply(copy, &.{ .arena = undefined, .pairs = u_doc }, .{ .diag = &diag }) catch |err| + update.apply(copy, &.{ .arena = undefined, .pairs = u_doc }, opts) 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. @@ -4241,13 +4267,75 @@ fn update_refusal(reply: *wire.Reply, err: anyerror, diag: update.Diagnostic) !v } } +/// Read an `arrayFilters` argument into the bindings `$[]` +/// resolves against. +/// +/// Only the shape is the command's business, and only the shape is checked +/// here: which identifier a filter names, whether it is spelled legally and +/// whether the update ever uses it all belong to `update.validate`, which is +/// the half that can see the update's paths. `field` is the dotted name +/// mongod puts in the message, and it differs between the two callers. +/// +/// Returns null having written the error reply, like the other `*_arg` +/// helpers. +fn parse_array_filters( + reply: *wire.Reply, + value: ?bson.Value, + comptime field: []const u8, +) !?[]update.ArrayFilter { + const v = value orelse return &.{}; + const arr = switch (v) { + .array => |a| a, + else => { + try wrong_type(reply, field, v, "array"); + return null; + }, + }; + const out = try reply.arena_alloc().alloc(update.ArrayFilter, arr.len); + for (arr, 0..) |elem, i| { + out[i] = .{ .pairs = switch (elem) { + .doc => |d| d, + else => { + const arena = reply.arena_alloc(); + const name = try std.fmt.allocPrint(arena, field ++ ".{d}", .{i}); + try wrong_type_dynamic(reply, name, elem, "object"); + return null; + }, + } }; + } + return out; +} + +fn wrong_type( + reply: *wire.Reply, + comptime field: []const u8, + got: bson.Value, + comptime want: []const u8, +) !void { + return wrong_type_dynamic(reply, field, got, want); +} + +fn wrong_type_dynamic( + reply: *wire.Reply, + field: []const u8, + got: bson.Value, + want: []const u8, +) !void { + const text = try std.fmt.allocPrint( + reply.arena_alloc(), + "BSON field '{s}' is the wrong type '{s}', expected type '{s}'", + .{ field, got.type_name(), want }, + ); + return reply.put_error(@intFromEnum(ErrorCode.type_mismatch), "TypeMismatch", text); +} + /// 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, + opts: update.Options, ) !*bson.Document { const arena = reply.arena_alloc(); var pairs: std.ArrayListUnmanaged(bson.Pair) = .empty; @@ -4262,7 +4350,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. - try update.apply(owned, &.{ .arena = undefined, .pairs = u_doc }, .{ .diag = diag }); + try update.apply(owned, &.{ .arena = undefined, .pairs = u_doc }, opts); return owned; } @@ -5783,6 +5871,91 @@ test "an all-positional update writes every element on the wire" { try testing.expectEqual(@as(i32, 9), values[0].int32); } +test "arrayFilters reach the update, and are refused before the scan" { + // The plumbing, end to end: an identifier in the path only means anything + // if the filter beside it arrives with it. + 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, "af", &.{ + .{ .doc = &.{ + .{ .key = "_id", .value = .{ .int32 = 1 } }, + .{ .key = "y", .value = .{ .array = &.{ + .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 3 } }} }, + .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 1 } }} }, + } } }, + } }, + }); + + const filters = [_]bson.Value{.{ .doc = &.{.{ .key = "i.b", .value = .{ .int32 = 3 } }} }}; + const updates = [_]bson.Value{.{ .doc = &.{ + .{ .key = "q", .value = .{ .doc = &.{} } }, + .{ .key = "u", .value = .{ .doc = &.{ + .{ .key = "$set", .value = .{ .doc = &.{ + .{ .key = "y.$[i].b", .value = .{ .int32 = 9 } }, + } } }, + } } }, + .{ .key = "arrayFilters", .value = .{ .array = &filters } }, + } }}; + try testing.expectEqual(@as(?i32, null), try run_for_code(&ctx, "update", .{ .string = "af" }, &.{ + .{ .key = "updates", .value = .{ .array = &updates } }, + })); + + // Only the element the filter selected moved. Mutation check: drop the + // `arrayFilters` read in `cmd_update` and this is a `NoArrayFilter` reply + // instead, which is the honest failure -- but silently ignoring the field + // would write both elements. + var reply = wire.Reply.init(testing.allocator); + defer reply.deinit(); + const values = try distinct_values(&tdb, io, &reply, "af", &.{ + .{ .key = "key", .value = .{ .string = "y.b" } }, + }); + try testing.expectEqual(@as(usize, 2), values.len); + try testing.expectEqual(@as(i32, 1), values[0].int32); + try testing.expectEqual(@as(i32, 9), values[1].int32); + + // A filter no path uses is refused even when the query matches nothing at + // all, which is what makes the check belong before the scan rather than + // inside the per-document loop. Mutation check: move `update.validate` + // below `scan_matching` and this answers ok. + const unused = [_]bson.Value{.{ .doc = &.{ + .{ .key = "q", .value = .{ .doc = &.{.{ .key = "nomatch", .value = .{ .int32 = 1 } }} } }, + .{ .key = "u", .value = .{ .doc = &.{ + .{ .key = "$set", .value = .{ .doc = &.{.{ .key = "y.b", .value = .{ .int32 = 2 } }} } }, + } } }, + .{ .key = "arrayFilters", .value = .{ .array = &filters } }, + } }}; + try testing.expectEqual(@as(?i32, 9), try run_for_code(&ctx, "update", .{ .string = "af" }, &.{ + .{ .key = "updates", .value = .{ .array = &unused } }, + })); + + // Shape is the command's business, and it answers TypeMismatch for it. + const bad = [_]bson.Value{.{ .doc = &.{ + .{ .key = "q", .value = .{ .doc = &.{} } }, + .{ .key = "u", .value = .{ .doc = &.{ + .{ .key = "$set", .value = .{ .doc = &.{.{ .key = "y.$[i].b", .value = .{ .int32 = 9 } }} } }, + } } }, + .{ .key = "arrayFilters", .value = .{ .array = &.{.{ .int32 = 3 }} } }, + } }}; + try testing.expectEqual(@as(?i32, 14), try run_for_code(&ctx, "update", .{ .string = "af" }, &.{ + .{ .key = "updates", .value = .{ .array = &bad } }, + })); + const not_an_array = [_]bson.Value{.{ .doc = &.{ + .{ .key = "q", .value = .{ .doc = &.{} } }, + .{ .key = "u", .value = .{ .doc = &.{ + .{ .key = "$set", .value = .{ .doc = &.{.{ .key = "y.$[i].b", .value = .{ .int32 = 9 } }} } }, + } } }, + .{ .key = "arrayFilters", .value = .{ .int32 = 1 } }, + } }}; + try testing.expectEqual(@as(?i32, 14), try run_for_code(&ctx, "update", .{ .string = "af" }, &.{ + .{ .key = "updates", .value = .{ .array = ¬_an_array } }, + })); +} + 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 -- 2.39.5 From 1892cb796951d91c483dec7116bf0e81dccce355 Mon Sep 17 00:00:00 2001 From: "A.Shakhmatov" Date: Mon, 10 Aug 2026 20:40:41 +0300 Subject: [PATCH 3/4] plan/spec: the positional gate goes green MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scorecard 204 -> 218 pass, 87 -> 73 fail: the fourteen `arrayFilters` cases across five files, which is the whole of what the two implementation commits were expected to move and nothing else. Positional corpus 51/51. The three divergences measured on the way are written into PLAN §6 rather than left in commit messages: `$` with two predicates on one array that no element satisfies together, an array filter with a top-level `$and`/`$or`, and a literal index into a scalar element -- the last being the one place the positional walk and the plain indexed path now answer differently, which is worth a commit of its own and needs its own measurements first. The design review gets an outcome note, since two of its guesses were wrong and the corpus is where that was settled. --- PLAN.md | 33 +++++++++++++++++++++++---- docs/M3_ARRAYFILTERS_DESIGN_REVIEW.md | 8 +++++++ tests/spec/positional/README.md | 28 ++++++++++++----------- tests/spec/scorecard.txt | 24 ++++--------------- 4 files changed, 56 insertions(+), 37 deletions(-) diff --git a/PLAN.md b/PLAN.md index 502bc41..6c0d1e6 100644 --- a/PLAN.md +++ b/PLAN.md @@ -467,7 +467,7 @@ answers, and the trade is only acceptable because the lie is removed first. | M1 | **Cursors + wire polish** | getMore / killCursors / batchSize; server-side cursor state with idle timeout; sessions plumbing (lsid accepted) as drivers send it; hello advertisement updates; **`moreToCome` on requests** (see the bug below); command-monitoring assertions in the spec runner | crud spec suite green; e2e green | | M2 | **The `aggregate` command surface** | `$out` and `$merge` (7 of the 13 failures), and refusing every pipeline construct the engine does not implement instead of answering `0` (amendment A6). The other 6 failures are blocked on M2.5, M4 and M8 — see `docs/M2_DESIGN_REVIEW.md` §7 | `aggregate-*.json`: 0 fail among the 7 reachable cases | | M2.5 | **The aggregation engine** | expression evaluator, per-stage document iterator, the accumulators, `$unwind`; `$lookup`/`$facet` explicitly out of the first cut (amendment A6) | a purpose-built stage corpus, every expectation measured against mongod | -| M3 | **Update operators + index types** | `distinct` (**done**); positional paths refused rather than destructive (**done**); then `$`/`$[]`/`$[]` implemented, $setOnInsert, $addToSet, $mul, $min/$max, $pop, $pullAll, $currentDate, pipeline updates; partial + hashed indexes | `tests/spec/positional/` 0 fail (51 cases, recorded from mongod — the named gate could not see this work, see below); remaining crud coverage; e2e3/e2e4 green | +| M3 | **Update operators + index types** | `distinct` (**done**); positional paths refused rather than destructive (**done**); `$`/`$[]`/`$[]` + `arrayFilters` implemented (**done**); then $setOnInsert, $addToSet, $mul, $min/$max, $pop, $pullAll, $currentDate, pipeline updates; partial + hashed indexes | `tests/spec/positional/` 0 fail (51 cases, recorded from mongod — **green**; the named gate could not see this work, see below); remaining crud coverage; e2e3/e2e4 green | | M4 | **Sessions + transactions** | logical sessions, snapshot isolation on the mmap engine, write concern at commit | sessions + transactions spec suites green | | M5 | **Change streams** | change feed + resume tokens (likely log-seq based), getMore integration | change-streams spec suite green | | M6 | **Admin/ops commands** | dbStats, collStats, serverStatus, ping, buildInfo, listDatabases filters, dropDatabase durability (log it) | mongosh UX smoke; e2e green | @@ -586,9 +586,10 @@ gaps (`bad update`, `update must be a document` — M3), unimplemented commands `bulkWrite`/`insertMany`. That list, not the total, is the milestone backlog. All three named commands have since landed: `$out`/`$merge` in M2, `distinct` -as M3's first commit. The backlog the same grouping gives today is -`arrayFilters` (14 cases), the `findOneAndUpdate`/`findOneAndReplace` shapes -(~10), pipeline-form updates (~10), and `create-null-ids` (6). +as M3's first commit, and the `arrayFilters` cluster (14 cases) with M3's +positional operators. The backlog the same grouping gives today is the +`findOneAndUpdate`/`findOneAndReplace` shapes (~10), pipeline-form updates +(~10), and `create-null-ids` (6). --- @@ -1091,7 +1092,7 @@ has to be its own commit with its own re-recorded scorecard. `sources/`, every expectation recorded from mongod 8.3.7, run through the shared runner with `--suite-dir`. - It stands at 15 pass / 36 fail against the refusal, which is the intended + It stood at 15 pass / 36 fail against the refusal, which was the intended shape — `expressions.json` was 1/26 before the evaluator and is green now. Recording it settled a dozen things, and the first contradicts what the design review assumed: `y.$[i].c.$[i].d` reusing one identifier at two @@ -1100,6 +1101,28 @@ has to be its own commit with its own re-recorded scorecard. is error 28 where every other path failure is 2; a positional segment never creates, so a missing or non-array path is an error where `$set: {'a.b': 1}` would construct; and an upsert gets no special case. + + **Green as of the two implementation commits**, 51/51, and the pinned crud + scorecard moved 204 → 218 with it. Three divergences from mongod were + measured on the way and left in place rather than guessed at: + - **`$` with two predicates on one array.** `{"y.b": 3, "y.c": 2}` matches + `[{b: 3, c: 1}, {b: 1, c: 2}]` without either element satisfying both, and + the document still matched. mongod writes element 1; this writes element 0. + mongod's answer is an artefact of which predicate last wrote its match + position — the same query with the two predicates *reversed* still gives 1 + — so there is no rule here to copy, only a behaviour to record. + - **An array filter with a top-level `$and`/`$or`.** mongod accepts + `[{$or: [{"i.b": 3}]}]` and finds the identifier inside it; this refuses + with 9, the same answer it gives `[{}]`. The identifier is read off + top-level field names, and an operator is not one. + - **A literal index into a scalar element.** `$set: {"y.0.b": 1}` on + `y: [null]` is PathNotViable (28) on mongod; here it replaces the null + with `{b: 1}`. The positional walk refuses this — `Walk.element` is where + the corpus measures it — but the plain indexed path still takes + `set_path`'s creating branch. One rule, reached two ways, and only one way + agrees. Worth one commit, and it needs its own measurements first: the + padding case (`y.3.b` past the end) is a *legal* creation on mongod, so + the fix is not "refuse a non-document element". - **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 index 3ce9277..d5a1fcc 100644 --- a/docs/M3_ARRAYFILTERS_DESIGN_REVIEW.md +++ b/docs/M3_ARRAYFILTERS_DESIGN_REVIEW.md @@ -10,6 +10,14 @@ 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. +**Outcome.** All of it landed, in the order §6 recommended: the refusal +(`f04e712`), the corpus (`e5a84c0`), then the implementation in two commits. +The corpus is 51/51 and the pinned crud scorecard moved 204 → 218. Two of the +review's own guesses were wrong and the corpus caught both — a reused +identifier is accepted, and `$[]` in first position answers the array-filter +message rather than the `$` one. The divergences that remain are listed in +PLAN §6 under the M3 gate; this document is not the place to track them. + --- ## 1. The plan names a feature; the measurement found data loss diff --git a/tests/spec/positional/README.md b/tests/spec/positional/README.md index 2059254..1a9cbdc 100644 --- a/tests/spec/positional/README.md +++ b/tests/spec/positional/README.md @@ -72,24 +72,26 @@ measures the version gap, not the engine. ## Where it stands -Recorded against mongod 8.3.7, run against the server at the positional -refusal: +Recorded against mongod 8.3.7. Green: ``` -filtered.json 8 pass 19 fail 0 skip -all-positional.json 3 pass 9 fail 0 skip -first-positional.json 4 pass 8 fail 0 skip +filtered.json 27 pass 0 fail 0 skip +all-positional.json 12 pass 0 fail 0 skip +first-positional.json 12 pass 0 fail 0 skip ``` -Red by construction and by design. The 15 that pass are the refusals where -this server's code already matches mongod's; of the 36 that fail, 31 are the -constructs themselves answering "not implemented by this server", and 5 are -refusals whose code differs — four of them the `arrayFilters` validation this -server cannot do yet because it never parses the option. +It was recorded red — 15 pass / 36 fail against the positional refusal — and +driven green by the two implementation commits, the same shape +`tests/spec/aggregate/expressions.json` had at 1 pass / 26 fail before the +expression evaluator existed. The 15 that passed then were the refusals where +this server already agreed with mongod, which is the only part of a red gate +that is worth anything: it says the corpus is measuring the server and not +the harness. -That is the intended shape. `tests/spec/aggregate/expressions.json` was -recorded at 1 pass / 26 fail before the evaluator existed and is green now; -this is the same gate at the same stage. +Three answers here still differ from mongod and are green only because no +case covers them; they are written down in PLAN §6 rather than papered over — +`$` with two disagreeing predicates on one array, an array filter with a +top-level `$and`/`$or`, and a literal index into a scalar element. ## What recording it settled diff --git a/tests/spec/scorecard.txt b/tests/spec/scorecard.txt index 31134c4..2f6eef4 100644 --- a/tests/spec/scorecard.txt +++ b/tests/spec/scorecard.txt @@ -18,7 +18,7 @@ # hasServerConnectionId, and maxTimeMS in an expected command (CSOT rewrites # it -- the only assertion this runner declines to make). -total 204 pass 87 fail 196 skip 175 files 0 errored +total 218 pass 73 fail 196 skip 175 files 0 errored # per-file: name pass fail skip aggregate-allowdiskuse.json 3 0 0 @@ -31,7 +31,7 @@ aggregate-out.json 2 0 0 aggregate-rawdata.json 1 0 1 aggregate-write-readPreference.json 0 0 4 aggregate.json 5 0 2 -bulkWrite-arrayFilters.json 0 3 0 +bulkWrite-arrayFilters.json 3 0 0 bulkWrite-collation.json 0 2 0 bulkWrite-comment.json 2 0 1 bulkWrite-delete-hint-serverError.json 0 0 2 @@ -141,7 +141,7 @@ findOneAndReplace-let.json 0 1 1 findOneAndReplace-rawdata.json 1 0 1 findOneAndReplace-upsert.json 2 2 0 findOneAndReplace.json 4 2 0 -findOneAndUpdate-arrayFilters.json 0 3 0 +findOneAndUpdate-arrayFilters.json 3 0 0 findOneAndUpdate-collation.json 0 1 0 findOneAndUpdate-comment.json 0 2 1 findOneAndUpdate-dots_and_dollars.json 0 0 4 @@ -172,7 +172,7 @@ replaceOne-rawdata.json 1 0 1 replaceOne-sort.json 1 0 1 replaceOne-validation.json 1 0 0 replaceOne.json 5 0 0 -updateMany-arrayFilters.json 0 3 0 +updateMany-arrayFilters.json 3 0 0 updateMany-collation.json 0 1 0 updateMany-comment.json 2 0 1 updateMany-dots_and_dollars.json 0 0 4 @@ -183,7 +183,7 @@ updateMany-pipeline.json 0 1 0 updateMany-rawdata.json 1 0 1 updateMany-validation.json 1 0 0 updateMany.json 4 0 0 -updateOne-arrayFilters.json 0 5 0 +updateOne-arrayFilters.json 5 0 0 updateOne-collation.json 0 1 0 updateOne-comment.json 2 0 1 updateOne-dots_and_dollars.json 0 0 4 @@ -209,9 +209,6 @@ 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 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 +358,6 @@ 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 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 +397,6 @@ 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 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 +407,6 @@ 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 MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server -updateOne-arrayFilters.json FAIL UpdateOne when one document matches arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server -updateOne-arrayFilters.json FAIL UpdateOne when multiple documents match arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server -updateOne-arrayFilters.json FAIL UpdateOne when no documents match multiple arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].c.$[j].d' is not implemented by this server -updateOne-arrayFilters.json FAIL UpdateOne when one document matches multiple arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].c.$[j].d' is not implemented by this server updateOne-collation.json FAIL UpdateOne when one document matches with collation updateOne.matchedCount: expected 1, got 0 updateOne-comment.json SKIP UpdateOne with comment - pre 4.4 needs server <= 4.2.99 updateOne-dots_and_dollars.json SKIP Updating document to set top-level dollar-prefixed key on 5.0+ server needs server >= 5.0 -- 2.39.5 From 1482df891b9dbdfe9820d7433c24d6e5f4cd1afc Mon Sep 17 00:00:00 2001 From: "A.Shakhmatov" Date: Mon, 10 Aug 2026 20:45:03 +0300 Subject: [PATCH 4/4] commands/tests: name the mutation that actually reddens the arrayFilters test The comment claimed moving `update.validate` below `scan_matching` would redden it. Ran the mutation: it does not -- the call still sits above the zero-match branch, so it still runs. What reddens it is deleting the standalone call and leaving the check to `apply`, which runs once per matched document and so never at all when nothing matched. A mutation note that has not been run is worth less than no note. --- src/commands.zig | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/commands.zig b/src/commands.zig index 0828e8d..4b5d26f 100644 --- a/src/commands.zig +++ b/src/commands.zig @@ -5919,9 +5919,10 @@ test "arrayFilters reach the update, and are refused before the scan" { try testing.expectEqual(@as(i32, 9), values[1].int32); // A filter no path uses is refused even when the query matches nothing at - // all, which is what makes the check belong before the scan rather than - // inside the per-document loop. Mutation check: move `update.validate` - // below `scan_matching` and this answers ok. + // all, which is what makes the check belong beside the scan rather than + // inside `apply`. Mutation check: delete the standalone `update.validate` + // call and this answers ok -- `apply` runs once per matched document, and + // there are none. const unused = [_]bson.Value{.{ .doc = &.{ .{ .key = "q", .value = .{ .doc = &.{.{ .key = "nomatch", .value = .{ .int32 = 1 } }} } }, .{ .key = "u", .value = .{ .doc = &.{ -- 2.39.5