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/src/commands.zig b/src/commands.zig index 1b35d38..4b5d26f 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) 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) 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. @@ -4141,20 +4167,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", @@ -4169,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; @@ -4190,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); + try update.apply(owned, &.{ .arena = undefined, .pairs = u_doc }, opts); return owned; } @@ -5629,9 +5789,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 +5829,134 @@ 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 "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 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 = &.{ + .{ .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 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); } 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