diff --git a/src/commands.zig b/src/commands.zig index 59d2c0b..f8b6c49 100644 --- a/src/commands.zig +++ b/src/commands.zig @@ -81,6 +81,10 @@ pub const ErrorCode = enum(i32) { query_plan_killed = 175, unauthorized = 13, type_mismatch = 14, + /// `PathNotViable`, measured on mongod 8.3.7: what an update answers when + /// a path segment names a field inside something that cannot hold one -- + /// in practice, a non-numeric segment applied to an array. + path_not_viable = 28, operation_failed = 96, // Session and transaction codes, measured against mongod 8.3.7 with a raw // OP_MSG probe -- the driver rewrites `lsid` with its own session, so a @@ -1951,7 +1955,9 @@ fn cmd_update(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void { if (matched.items.len == 0) { if (upsert) { - const new_doc = try build_upsert_doc(reply, q, u_doc); + var up_diag: update.Diagnostic = .{}; + const new_doc = build_upsert_doc(reply, q, u_doc, &up_diag) catch |err| + return update_refusal(reply, err, up_diag); ctx.engine.insert(db_name, coll_name, new_doc, ctx.oid_gen) catch |err| switch (err) { error.DuplicateKey, error.DuplicateKeyIndex => return duplicate_key_error(ctx, reply, db_name, coll_name, new_doc), else => return err, @@ -1973,10 +1979,9 @@ fn cmd_update(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void { // and a rejected update must not corrupt the stored document. const doc = try doc_tree(reply.arena_alloc(), coll, off); const copy = try clone_doc(reply, doc); - update.apply(copy, &.{ .arena = undefined, .pairs = u_doc }) catch |err| switch (err) { - error.ImmutableId, error.InvalidUpdate => return bad_value(reply, "bad update"), - else => return err, - }; + var diag: update.Diagnostic = .{}; + update.apply(copy, &.{ .arena = undefined, .pairs = u_doc }, &diag) catch |err| + return update_refusal(reply, err, diag); const written = ctx.engine.replace(db_name, coll_name, copy, ctx.oid_gen) catch |err| switch (err) { error.DuplicateKey, error.DuplicateKeyIndex => { const e = try reply.arena_alloc().alloc(bson.Pair, 3); @@ -2079,7 +2084,9 @@ fn cmd_find_and_modify(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !v if (target == null and do_update and upsert) { const u_doc = doc_arg(msg.body.get("update")) orelse return bad_value(reply, "update must be a document"); - const new_doc = try build_upsert_doc(reply, q, u_doc); + var up_diag: update.Diagnostic = .{}; + const new_doc = build_upsert_doc(reply, q, u_doc, &up_diag) catch |err| + return update_refusal(reply, err, up_diag); ctx.engine.insert(db_name, coll_name, new_doc, ctx.oid_gen) catch |err| switch (err) { error.DuplicateKey, error.DuplicateKeyIndex => return duplicate_key_error(ctx, reply, db_name, coll_name, new_doc), else => return err, @@ -2096,10 +2103,9 @@ fn cmd_find_and_modify(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !v const u_doc = doc_arg(msg.body.get("update")) orelse return bad_value(reply, "update must be a document"); const before = try bson.copy_pairs(arena, target.?.pairs); const copy = try clone_doc(reply, target.?); - update.apply(copy, &.{ .arena = undefined, .pairs = u_doc }) catch |err| switch (err) { - error.ImmutableId, error.InvalidUpdate => return bad_value(reply, "bad update"), - else => return err, - }; + var diag: update.Diagnostic = .{}; + update.apply(copy, &.{ .arena = undefined, .pairs = u_doc }, &diag) catch |err| + return update_refusal(reply, err, diag); // findAndModify reports `n` (matched) and `updatedExisting`, neither of // which distinguishes a no-op, so whether it wrote is not needed here. _ = try ctx.engine.replace(db_name, coll_name, copy, ctx.oid_gen); @@ -4108,12 +4114,48 @@ fn clone_doc(reply: *wire.Reply, doc: *const bson.Document) !*bson.Document { return owned; } +/// The reply an update refusal turns into. +/// +/// Shared by all four places that apply an update, because a refusal wired +/// into three of them would be a silent divergence between `update`, +/// `findAndModify` and the upsert path -- and the two new ones exist to stop +/// a silent divergence in the first place. +/// +/// The messages are this server's own words. mongod's `PathNotViable` text +/// embeds a shell-syntax rendering of the offending element (`Cannot create +/// field 'nope' in element {y: [ { b: 3 }, { b: 1 } ]}`), and there is no BSON +/// formatter here that produces it. The code is what the corpus asserts and +/// the code is exact; a half-copy of the text would be worse than a clear +/// sentence that does not pretend. +fn update_refusal(reply: *wire.Reply, err: anyerror, diag: update.Diagnostic) !void { + const arena = reply.arena_alloc(); + switch (err) { + error.PositionalUnsupported => return bad_value(reply, try std.fmt.allocPrint( + arena, + "the positional operator '{s}' in path '{s}' is not implemented by this server", + .{ diag.segment, diag.path }, + )), + error.PathNotViable => return reply.put_error( + @intFromEnum(ErrorCode.path_not_viable), + "PathNotViable", + try std.fmt.allocPrint( + arena, + "Cannot create field '{s}' in an array, at path '{s}'", + .{ diag.segment, diag.path }, + ), + ), + error.ImmutableId, error.InvalidUpdate => return bad_value(reply, "bad update"), + else => return err, + } +} + /// Build the document for an upsert: equality fields from the filter, then /// the update operators applied. Owned by the reply arena. fn build_upsert_doc( reply: *wire.Reply, q: []const bson.Pair, u_doc: []const bson.Pair, + diag: *update.Diagnostic, ) !*bson.Document { const arena = reply.arena_alloc(); var pairs: std.ArrayListUnmanaged(bson.Pair) = .empty; @@ -4128,10 +4170,7 @@ fn build_upsert_doc( const owned = try arena.create(bson.Document); owned.* = bson.Document{ .arena = std.heap.ArenaAllocator.init(arena), .pairs = try pairs.toOwnedSlice(arena) }; // Apply update operators to build the final doc; _id handled by insert. - update.apply(owned, &.{ .arena = undefined, .pairs = u_doc }) catch |err| switch (err) { - error.ImmutableId, error.InvalidUpdate => return error.InvalidUpdate, - else => return err, - }; + try update.apply(owned, &.{ .arena = undefined, .pairs = u_doc }, diag); return owned; } @@ -5494,6 +5533,61 @@ test "distinct applies its filter before collecting" { try testing.expectEqual(@as(i32, 33), values[1].int32); } +test "a positional update is refused on the wire and stores nothing" { + // The end of the chain the unit tests start: the refusal has to reach the + // client as a code, and the stored document -- not just the working copy + // -- has to be the one that was there before. + // + // Every case here previously answered ok: 1 with nModified: 1, having + // replaced `y` with a document keyed by the path segment's text. + var threaded: std.Io.Threaded = .init_single_threaded; + defer threaded.deinit(); + const io = threaded.io(); + var tdb = try TestDb.init(io); + defer tdb.deinit(); + var ctx = tdb.ctx(io); + + const cases = [_]struct { coll: []const u8, path: []const u8, code: i32 }{ + .{ .coll = "a1", .path = "y.$[i].b", .code = 2 }, // filtered positional + .{ .coll = "a2", .path = "y.$[].b", .code = 2 }, // all-positional + .{ .coll = "a3", .path = "y.$.b", .code = 2 }, // positional + .{ .coll = "a4", .path = "y.nope.b", .code = 28 }, // PathNotViable, same branch + }; + for (cases) |c| { + try dispatch_insert(&tdb, io, c.coll, &.{ + .{ .doc = &.{ + .{ .key = "_id", .value = .{ .int32 = 1 } }, + .{ .key = "y", .value = .{ .array = &.{ + .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 3 } }} }, + } } }, + } }, + }); + + const updates = [_]bson.Value{.{ .doc = &.{ + .{ .key = "q", .value = .{ .doc = &.{} } }, + .{ .key = "u", .value = .{ .doc = &.{ + .{ .key = "$set", .value = .{ .doc = &.{ + .{ .key = c.path, .value = .{ .int32 = 2 } }, + } } }, + } } }, + } }}; + try testing.expectEqual(@as(?i32, c.code), try run_for_code(&ctx, "update", .{ .string = c.coll }, &.{ + .{ .key = "updates", .value = .{ .array = &updates } }, + })); + + // Read it back through `distinct`: if the array survived it still has + // an element with `b: 3`, and if it was overwritten by a document + // there is nothing at `y.b` at all. + var reply = wire.Reply.init(testing.allocator); + defer reply.deinit(); + const values = try distinct_values(&tdb, io, &reply, c.coll, &.{ + .{ .key = "key", .value = .{ .string = "y.b" } }, + }); + try testing.expectEqual(@as(usize, 1), values.len); + try testing.expectEqual(@as(i32, 3), values[0].int32); + } +} + test "aggregate $sort without a preceding $group sorts and frees correctly" { // Regression test for a remote, client-triggerable invalid free: the // $sort stage materialized its document list from the reply arena and diff --git a/src/update.zig b/src/update.zig index d432b46..0aecf2e 100644 --- a/src/update.zig +++ b/src/update.zig @@ -6,7 +6,42 @@ const std = @import("std"); const bson = @import("bson.zig"); const query = @import("query.zig"); -pub const UpdateError = error{ ImmutableId, InvalidUpdate, OutOfMemory }; +pub const UpdateError = error{ + ImmutableId, + InvalidUpdate, + /// A `$`, `$[]` or `$[]` segment: MongoDB's three ways of + /// saying "descend into this array", none of them implemented here. + PositionalUnsupported, + /// A non-numeric segment applied to an array, which is never a field to + /// create. mongod's `PathNotViable`. + PathNotViable, + OutOfMemory, +}; + +/// Which path a refusal was about, so the reply can name it instead of saying +/// "bad update". Borrowed from the update document, which outlives the call. +pub const Diagnostic = struct { + path: []const u8 = "", + segment: []const u8 = "", +}; + +fn note(diag: ?*Diagnostic, path: []const u8, segment: []const u8) void { + if (diag) |d| d.* = .{ .path = path, .segment = segment }; +} + +/// `$`, `$[]` and `$[]` -- the three spellings of "descend into +/// this array". +/// +/// None is implemented, and until they are each has to be refused rather than +/// walked. `set_path` used to reach the array, fail to read the segment as an +/// index, and overwrite the array with a document keyed by the segment's +/// literal text: `{$set: {"y.$[i].b": 2}}` turned `y: [{b: 3}, {b: 1}]` into +/// `y: {"$[i]": {"b": 2}}` and answered ok: 1. Every element was discarded, +/// under every operator -- `$inc` stored its operand rather than incrementing. +fn is_positional(seg: []const u8) bool { + if (std.mem.eql(u8, seg, "$")) return true; + return seg.len >= 3 and std.mem.startsWith(u8, seg, "$[") and seg[seg.len - 1] == ']'; +} const max_path_segments = 16; @@ -28,8 +63,16 @@ fn is_operator_key(key: []const u8) bool { } /// Apply an update document to `doc`: a replacement, or a set of operators. -pub fn apply(doc: *bson.Document, update: *const bson.Document) UpdateError!void { +pub fn apply( + doc: *bson.Document, + update: *const bson.Document, + diag: ?*Diagnostic, +) UpdateError!void { + // A replacement carries data, not paths, so nothing below applies to it -- + // which is also why mongod ignores `arrayFilters` alongside one, the single + // case out of seventeen where this server already agreed with it. if (is_replacement(update.pairs)) return apply_replacement(doc, update.pairs); + try reject_positional(update.pairs, diag); const arena = doc.arena.allocator(); var pairs = try copy_to_list(bson.Pair, arena, doc.pairs); for (update.pairs) |op| { @@ -37,11 +80,40 @@ pub fn apply(doc: *bson.Document, update: *const bson.Document) UpdateError!void // is not one is a mixed document -- which MongoDB rejects rather than // guessing at. if (!is_operator_key(op.key)) return error.InvalidUpdate; - try apply_operator(arena, &pairs, op.key, op.value); + try apply_operator(arena, &pairs, op.key, op.value, diag); } doc.pairs = try pairs.toOwnedSlice(arena); } +/// Refuse every positional path in the update before any of it is applied. +/// +/// Up front rather than at the point of use, because one update names several +/// paths: checking as we walk would refuse the third path having already +/// rewritten what the first two named. The caller discards its copy on error +/// either way, so this is not what makes the refusal safe -- it is what makes +/// the refusal *about the update* rather than about however far a walk got. +fn reject_positional(update: []const bson.Pair, diag: ?*Diagnostic) UpdateError!void { + for (update) |op| { + const ops = doc_pairs(op.value) orelse continue; + for (ops) |p| { + try reject_positional_path(p.key, diag); + // `$rename`'s destination is a path too, and it is the *value*. + if (std.mem.eql(u8, op.key, "$rename") and p.value == .string) { + try reject_positional_path(p.value.string, diag); + } + } + } +} + +fn reject_positional_path(path: []const u8, diag: ?*Diagnostic) UpdateError!void { + var it = std.mem.splitScalar(u8, path, '.'); + while (it.next()) |seg| { + if (!is_positional(seg)) continue; + note(diag, path, seg); + return error.PositionalUnsupported; + } +} + /// Replace every field of `doc` with `replacement`'s, except `_id`. /// /// `_id` is immutable, so it survives and keeps its position at the front (which @@ -97,6 +169,7 @@ fn apply_operator( pairs: *std.ArrayListUnmanaged(bson.Pair), op: []const u8, value: bson.Value, + diag: ?*Diagnostic, ) UpdateError!void { if (std.mem.eql(u8, op, "$set")) { const ops = doc_pairs(value) orelse return error.InvalidUpdate; @@ -104,7 +177,7 @@ fn apply_operator( if (std.mem.eql(u8, p.key, "_id")) return error.ImmutableId; var segs: [max_path_segments][]const u8 = undefined; const n = split_path(p.key, &segs) orelse return error.InvalidUpdate; - try set_path(arena, pairs, segs[0..n], try bson.copy_value(arena, p.value)); + try set_path(arena, pairs, segs[0..n], try bson.copy_value(arena, p.value), p.key, diag); } return; } @@ -125,7 +198,7 @@ fn apply_operator( const current = get_value(pairs.items, segs[0..n]) orelse bson.Value{ .int32 = 0 }; if (!current.is_number() or !p.value.is_number()) return error.InvalidUpdate; const sum = try numeric_add(current, p.value); - try set_path(arena, pairs, segs[0..n], sum); + try set_path(arena, pairs, segs[0..n], sum, p.key, diag); } return; } @@ -151,12 +224,12 @@ fn apply_operator( else => return error.InvalidUpdate, }; for (arr) |item| try items.append(arena, try bson.copy_value(arena, item)); - try set_path(arena, pairs, segs[0..n], .{ .array = try items.toOwnedSlice(arena) }); + try set_path(arena, pairs, segs[0..n], .{ .array = try items.toOwnedSlice(arena) }, p.key, diag); continue; } } try items.append(arena, try bson.copy_value(arena, p.value)); - try set_path(arena, pairs, segs[0..n], .{ .array = try items.toOwnedSlice(arena) }); + try set_path(arena, pairs, segs[0..n], .{ .array = try items.toOwnedSlice(arena) }, p.key, diag); } return; } @@ -177,7 +250,7 @@ fn apply_operator( try items.append(arena, elem); } } - try set_path(arena, pairs, segs[0..n], .{ .array = try items.toOwnedSlice(arena) }); + try set_path(arena, pairs, segs[0..n], .{ .array = try items.toOwnedSlice(arena) }, p.key, diag); } return; } @@ -192,7 +265,7 @@ fn apply_operator( unset_path(arena, pairs, old_segs[0..old_n]); var new_segs: [max_path_segments][]const u8 = undefined; const new_n = split_path(p.value.string, &new_segs) orelse return error.InvalidUpdate; - try set_path(arena, pairs, new_segs[0..new_n], v); + try set_path(arena, pairs, new_segs[0..new_n], v, p.value.string, diag); } return; } @@ -258,6 +331,8 @@ fn set_path( pairs: *std.ArrayListUnmanaged(bson.Pair), segs: []const []const u8, value: bson.Value, + path: []const u8, + diag: ?*Diagnostic, ) UpdateError!void { if (segs.len == 1) { if (find_pair(pairs.items, segs[0])) |idx| { @@ -270,23 +345,31 @@ fn set_path( const idx = find_pair(pairs.items, segs[0]) orelse { const is_array = parse_index(segs[1]) != null; try pairs.append(arena, .{ .key = try arena.dupe(u8, segs[0]), .value = if (is_array) .{ .array = &.{} } else .{ .doc = &.{} } }); - return set_path(arena, pairs, segs, value); + return set_path(arena, pairs, segs, value, path, diag); }; switch (pairs.items[idx].value) { .doc => |sub| { var sub_pairs = try copy_to_list(bson.Pair, arena, sub); defer sub_pairs.deinit(arena); - try set_path(arena, &sub_pairs, segs[1..], value); + try set_path(arena, &sub_pairs, segs[1..], value, path, diag); pairs.items[idx].value = .{ .doc = try sub_pairs.toOwnedSlice(arena) }; }, .array => |arr| { const index = parse_index(segs[1]) orelse { - // treat as non-array: replace with a doc - var sub_pairs: std.ArrayListUnmanaged(bson.Pair) = .empty; - defer sub_pairs.deinit(arena); - try set_path(arena, &sub_pairs, segs[1..], value); - pairs.items[idx].value = .{ .doc = try sub_pairs.toOwnedSlice(arena) }; - return; + // A non-numeric segment under an array is never a field to + // create. This branch used to read "treat as non-array: + // replace with a doc" and did exactly that -- `y.nope.b` + // turned `y: [{b: 3}]` into `y: {nope: {b: 2}}`, discarding + // every element and answering ok: 1. mongod refuses with + // PathNotViable and leaves the document alone. + // + // The positional spellings took this same branch and are the + // reason it was found; they are refused earlier, by + // `reject_positional`. What reaches here is the rest of the + // class: a plain field name, and a `$`-prefixed one that is + // not positional. + note(diag, path, segs[1]); + return error.PathNotViable; }; var items = try copy_to_list(bson.Value, arena, arr); defer items.deinit(arena); @@ -300,13 +383,13 @@ fn set_path( .doc => |sub| { var sub_pairs = try copy_to_list(bson.Pair, arena, sub); defer sub_pairs.deinit(arena); - try set_path(arena, &sub_pairs, segs[2..], value); + try set_path(arena, &sub_pairs, segs[2..], value, path, diag); items.items[index] = .{ .doc = try sub_pairs.toOwnedSlice(arena) }; }, else => { var sub_pairs: std.ArrayListUnmanaged(bson.Pair) = .empty; defer sub_pairs.deinit(arena); - try set_path(arena, &sub_pairs, segs[2..], value); + try set_path(arena, &sub_pairs, segs[2..], value, path, diag); items.items[index] = .{ .doc = try sub_pairs.toOwnedSlice(arena) }; }, } @@ -316,7 +399,7 @@ fn set_path( else => { var sub_pairs: std.ArrayListUnmanaged(bson.Pair) = .empty; defer sub_pairs.deinit(arena); - try set_path(arena, &sub_pairs, segs[1..], value); + try set_path(arena, &sub_pairs, segs[1..], value, path, diag); pairs.items[idx].value = .{ .doc = try sub_pairs.toOwnedSlice(arena) }; }, } @@ -425,7 +508,7 @@ test "$set, $inc, $unset, $rename" { .{ .key = "$inc", .value = .{ .doc = &.{.{ .key = "user.age", .value = .{ .int32 = 2 } }} } }, .{ .key = "$unset", .value = .{ .doc = &.{.{ .key = "gone", .value = .{ .string = "" } }} } }, .{ .key = "$rename", .value = .{ .doc = &.{.{ .key = "new", .value = .{ .string = "renamed" } }} } }, - })); + }), null); const user = bson.get_pair(doc.pairs, "user").?; try testing.expectEqualStrings("alice", user.doc[0].value.string); @@ -445,13 +528,13 @@ test "$push and $pull" { try apply(&doc, &doc_of(&.{ .{ .key = "$push", .value = .{ .doc = &.{.{ .key = "tags", .value = .{ .string = "c" } }} } }, - })); + }), null); try testing.expectEqual(@as(usize, 3), bson.get_pair(doc.pairs, "tags").?.array.len); try testing.expectEqualStrings("c", bson.get_pair(doc.pairs, "tags").?.array[2].string); try apply(&doc, &doc_of(&.{ .{ .key = "$pull", .value = .{ .doc = &.{.{ .key = "tags", .value = .{ .string = "b" } }} } }, - })); + }), null); const tags = bson.get_pair(doc.pairs, "tags").?.array; try testing.expectEqual(@as(usize, 2), tags.len); try testing.expectEqualStrings("a", tags[0].string); @@ -459,7 +542,7 @@ test "$push and $pull" { try apply(&doc, &doc_of(&.{ .{ .key = "$push", .value = .{ .doc = &.{.{ .key = "tags", .value = .{ .doc = &.{.{ .key = "$each", .value = .{ .array = &.{ .{ .string = "x" }, .{ .string = "y" } } } }} } }} } }, - })); + }), null); try testing.expectEqual(@as(usize, 4), bson.get_pair(doc.pairs, "tags").?.array.len); } @@ -475,7 +558,7 @@ test "$set nested creation and _id protection" { .{ .key = "a.b.c", .value = .{ .int32 = 42 } }, .{ .key = "arr.1", .value = .{ .string = "x" } }, } } }, - })); + }), null); const a = bson.get_pair(doc.pairs, "a").?; const b = bson.get_pair(a.doc, "b").?; try testing.expectEqual(@as(i64, 42), bson.get_pair(b.doc, "c").?.int32); @@ -485,7 +568,7 @@ test "$set nested creation and _id protection" { try testing.expectError(error.ImmutableId, apply(&doc, &doc_of(&.{ .{ .key = "$set", .value = .{ .doc = &.{.{ .key = "_id", .value = .{ .int32 = 2 } }} } }, - }))); + }), null)); } test "a replacement keeps _id and drops every other field" { @@ -502,7 +585,7 @@ test "a replacement keeps _id and drops every other field" { try apply(&doc, &doc_of(&.{ .{ .key = "fresh", .value = .{ .int32 = 42 } }, - })); + }), null); try testing.expectEqual(@as(usize, 2), doc.pairs.len); // _id survives, and stays at the front where it is stored. @@ -523,7 +606,7 @@ test "an empty replacement leaves a document holding only its _id" { // Legal, and the reason `is_replacement` treats an empty document as one // rather than as a no-op set of operators. - try apply(&doc, &doc_of(&.{})); + try apply(&doc, &doc_of(&.{}), null); try testing.expectEqual(@as(usize, 1), doc.pairs.len); try testing.expectEqualStrings("_id", doc.pairs[0].key); } @@ -545,7 +628,7 @@ test "a replacement may repeat the _id it is replacing, but not change it" { try apply(&doc, &doc_of(&.{ .{ .key = "_id", .value = .{ .int32 = 7 } }, .{ .key = "b", .value = .{ .int32 = 2 } }, - })); + }), null); try testing.expectEqual(@as(i64, 7), doc.pairs[0].value.int32); try testing.expectEqual(@as(i64, 2), bson.get_pair(doc.pairs, "b").?.int32); // And only once, not twice. @@ -554,14 +637,14 @@ test "a replacement may repeat the _id it is replacing, but not change it" { // A different _id: refused. try testing.expectError(error.ImmutableId, apply(&doc, &doc_of(&.{ .{ .key = "_id", .value = .{ .int32 = 8 } }, - }))); + }), null)); // Equal across numeric types is the same _id, matching the canonical key // encoding the _id_ index descends on. try apply(&doc, &doc_of(&.{ .{ .key = "_id", .value = .{ .double = 7.0 } }, .{ .key = "c", .value = .{ .int32 = 3 } }, - })); + }), null); try testing.expectEqual(@as(i64, 3), bson.get_pair(doc.pairs, "c").?.int32); } @@ -578,7 +661,7 @@ test "a replacement supplies the _id when the document has none" { try apply(&doc, &doc_of(&.{ .{ .key = "a", .value = .{ .int32 = 1 } }, .{ .key = "_id", .value = .{ .int32 = 99 } }, - })); + }), null); try testing.expectEqual(@as(usize, 2), doc.pairs.len); try testing.expectEqualStrings("_id", doc.pairs[0].key); @@ -596,13 +679,13 @@ test "a mixed update document is refused from either side" { try testing.expectError(error.InvalidUpdate, apply(&doc, &doc_of(&.{ .{ .key = "$set", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 1 } }} } }, .{ .key = "plain", .value = .{ .int32 = 1 } }, - }))); + }), null)); // Starts with data, so it is a replacement and an operator has no meaning. try testing.expectError(error.InvalidUpdate, apply(&doc, &doc_of(&.{ .{ .key = "plain", .value = .{ .int32 = 1 } }, .{ .key = "$set", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 1 } }} } }, - }))); + }), null)); } test "is_replacement decides on the first field only" { @@ -613,3 +696,153 @@ test "is_replacement decides on the first field only" { // replacement path then stores it, which is what MongoDB does with it. try testing.expect(is_replacement(&.{.{ .key = "", .value = .{ .int32 = 1 } }})); } + +/// A document with one array field, rebuilt per case so a refusal can be +/// checked against untouched bytes. +fn array_doc(arena: std.mem.Allocator) !bson.Document { + var doc = bson.Document{ .arena = std.heap.ArenaAllocator.init(arena), .pairs = &.{} }; + doc.pairs = try doc.arena.allocator().dupe(bson.Pair, &.{ + .{ .key = "y", .value = .{ .array = &.{ + .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 3 } }} }, + .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 1 } }} }, + } } }, + }); + return doc; +} + +fn expect_y_untouched(doc: *const bson.Document) !void { + const y = doc.get("y") orelse return error.TestUnexpectedResult; + // The array is still an array. Before this refusal existed it was a + // document keyed by the path segment's literal text, and everything in it + // was gone. + try testing.expect(y == .array); + try testing.expectEqual(@as(usize, 2), y.array.len); + try testing.expectEqual(@as(i32, 3), y.array[0].doc[0].value.int32); + try testing.expectEqual(@as(i32, 1), y.array[1].doc[0].value.int32); +} + +test "a positional path is refused and the array survives" { + // The load-bearing test of the refusal. Each of these used to answer + // success having replaced `y` with `{"": ...}` -- the array and + // both its elements discarded, `ok: 1`, `modifiedCount: 1`. + // + // Mutation check: delete the `reject_positional` call in `apply` and every + // case here goes red on `expect_y_untouched`, not on the error. + const cases = [_][]const u8{ + "y.$[i].b", // filtered positional + "y.$[].b", // all-positional + "y.$.b", // positional + "y.$[i]", // as the leaf + "$", // in first position + "y.$[i].c.$[j].d", // nested, two identifiers + }; + for (cases) |path| { + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + var diag: Diagnostic = .{}; + try testing.expectError(error.PositionalUnsupported, apply(&doc, &doc_of(&.{ + .{ .key = "$set", .value = .{ .doc = &.{.{ .key = path, .value = .{ .int32 = 2 } }} } }, + }), &diag)); + try testing.expectEqualStrings(path, diag.path); + try expect_y_untouched(&doc); + } +} + +test "every operator refuses a positional path, not just $set" { + // The destruction was below the operator, in the shared path walk: `$inc` + // through `$[i]` stored its operand instead of incrementing. So the + // refusal has to be below the operator too. + const ops = [_]struct { op: []const u8, value: bson.Value }{ + .{ .op = "$set", .value = .{ .int32 = 2 } }, + .{ .op = "$inc", .value = .{ .int32 = 10 } }, + .{ .op = "$unset", .value = .{ .string = "" } }, + .{ .op = "$push", .value = .{ .int32 = 1 } }, + .{ .op = "$pull", .value = .{ .int32 = 1 } }, + }; + for (ops) |o| { + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + try testing.expectError(error.PositionalUnsupported, apply(&doc, &doc_of(&.{ + .{ .key = o.op, .value = .{ .doc = &.{.{ .key = "y.$[i].b", .value = o.value }} } }, + }), null)); + try expect_y_untouched(&doc); + } +} + +test "$rename checks its destination, which is the value not the key" { + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + var diag: Diagnostic = .{}; + try testing.expectError(error.PositionalUnsupported, apply(&doc, &doc_of(&.{ + .{ .key = "$rename", .value = .{ .doc = &.{ + .{ .key = "y", .value = .{ .string = "z.$[i]" } }, + } } }, + }), &diag)); + try testing.expectEqualStrings("z.$[i]", diag.path); + try expect_y_untouched(&doc); +} + +test "nothing in the update is applied when one of its paths is refused" { + // The refusal is taken before the first write, so the good path in this + // update does not land either. Mutation check: move `reject_positional` + // inside the operator loop and `ok` appears on the document. + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + try testing.expectError(error.PositionalUnsupported, apply(&doc, &doc_of(&.{ + .{ .key = "$set", .value = .{ .doc = &.{ + .{ .key = "ok", .value = .{ .int32 = 1 } }, + .{ .key = "y.$[i].b", .value = .{ .int32 = 2 } }, + } } }, + }), null)); + try testing.expect(doc.get("ok") == null); + try expect_y_untouched(&doc); +} + +test "a non-numeric segment under an array is PathNotViable, not a new field" { + // The rest of the class the positional forms belonged to. `y.nope.b` is + // not a positional operator and is refused for a different reason with a + // different code -- measured on mongod 8.3.7, which answers 28 here and 2 + // for the positional forms. + const cases = [_][]const u8{ "y.nope.b", "y.nope", "y.$x.b" }; + for (cases) |path| { + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + var diag: Diagnostic = .{}; + try testing.expectError(error.PathNotViable, apply(&doc, &doc_of(&.{ + .{ .key = "$set", .value = .{ .doc = &.{.{ .key = path, .value = .{ .int32 = 2 } }} } }, + }), &diag)); + try expect_y_untouched(&doc); + } +} + +test "a numeric segment still addresses an array element" { + // The regression guard for the refusal above: indexed paths are the one + // way into an array that does work, and they must keep working, including + // the null padding past the end. + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + try apply(&doc, &doc_of(&.{ + .{ .key = "$set", .value = .{ .doc = &.{ + .{ .key = "y.0.b", .value = .{ .int32 = 7 } }, + .{ .key = "y.3.b", .value = .{ .int32 = 8 } }, + } } }, + }), null); + const y = doc.get("y").?; + try testing.expectEqual(@as(usize, 4), y.array.len); + try testing.expectEqual(@as(i32, 7), y.array[0].doc[0].value.int32); + try testing.expect(y.array[2] == .null); + try testing.expectEqual(@as(i32, 8), y.array[3].doc[0].value.int32); +} + +test "a replacement is not a path, so it is not refused" { + // The one case out of seventeen where this server already agreed with + // mongod: `arrayFilters` alongside a replacement is ignored by both, and + // a replacement field named like a path is data, not a path. + var doc = try array_doc(testing.allocator); + defer doc.arena.deinit(); + try apply(&doc, &doc_of(&.{ + .{ .key = "z", .value = .{ .int32 = 1 } }, + }), null); + try testing.expect(doc.get("y") == null); + try testing.expectEqual(@as(i32, 1), doc.get("z").?.int32); +} diff --git a/tests/spec/scorecard.txt b/tests/spec/scorecard.txt index 5e08019..31134c4 100644 --- a/tests/spec/scorecard.txt +++ b/tests/spec/scorecard.txt @@ -209,9 +209,9 @@ aggregate-rawdata.json SKIP Aggregate with rawData option needs server >= 8.2.0 aggregate-write-readPreference.json SKIP * needs topology replicaset/sharded/load-balanced aggregate.json SKIP aggregate with a document comment - pre 4.4 needs server <= 4.2.99 aggregate.json SKIP aggregate with comment does not set comment on getMore - pre 4.4 needs server <= 4.3.99 -bulkWrite-arrayFilters.json FAIL BulkWrite updateOne with arrayFilters outcome crud-tests.test[0].y: expected an array, got {"$[i]":{"b":2}} -bulkWrite-arrayFilters.json FAIL BulkWrite updateMany with arrayFilters outcome crud-tests.test[0].y: expected an array, got {"$[i]":{"b":2}} -bulkWrite-arrayFilters.json FAIL BulkWrite with arrayFilters bulkWrite.modifiedCount: expected 3, got 2 +bulkWrite-arrayFilters.json FAIL BulkWrite updateOne with arrayFilters MongoBulkWriteError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +bulkWrite-arrayFilters.json FAIL BulkWrite updateMany with arrayFilters MongoBulkWriteError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +bulkWrite-arrayFilters.json FAIL BulkWrite with arrayFilters MongoBulkWriteError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server bulkWrite-collation.json FAIL BulkWrite with delete operations and collation bulkWrite.deletedCount: expected 4, got 0 bulkWrite-collation.json FAIL BulkWrite with update operations and collation bulkWrite.matchedCount: expected 6, got 2 bulkWrite-comment.json SKIP BulkWrite with comment - pre 4.4 needs server <= 4.2.99 @@ -361,9 +361,9 @@ findOneAndReplace-upsert.json FAIL FindOneAndReplace when no documents match wit findOneAndReplace-upsert.json FAIL FindOneAndReplace when no documents match with id specified with upsert returning the document after modification findOneAndReplace: expected a document, got null findOneAndReplace.json FAIL FindOneAndReplace when many documents match returning the document after modification findOneAndReplace.x: expected 32, got 22 findOneAndReplace.json FAIL FindOneAndReplace when one document matches returning the document after modification findOneAndReplace.x: expected 32, got 22 -findOneAndUpdate-arrayFilters.json FAIL FindOneAndUpdate when no document matches arrayFilters outcome crud-v1.coll[0].y: expected an array, got {"$[i]":{"b":2}} -findOneAndUpdate-arrayFilters.json FAIL FindOneAndUpdate when one document matches arrayFilters outcome crud-v1.coll[0].y: expected an array, got {"$[i]":{"b":2}} -findOneAndUpdate-arrayFilters.json FAIL FindOneAndUpdate when multiple documents match arrayFilters outcome crud-v1.coll[0].y: expected an array, got {"$[i]":{"b":2}} +findOneAndUpdate-arrayFilters.json FAIL FindOneAndUpdate when no document matches arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +findOneAndUpdate-arrayFilters.json FAIL FindOneAndUpdate when one document matches arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +findOneAndUpdate-arrayFilters.json FAIL FindOneAndUpdate when multiple documents match arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server findOneAndUpdate-collation.json FAIL FindOneAndUpdate when many documents match with collation returning the document before modification findOneAndUpdate: expected a document, got null findOneAndUpdate-comment.json FAIL findOneAndUpdate with string comment MongoServerError: update must be a document findOneAndUpdate-comment.json FAIL findOneAndUpdate with document comment MongoServerError: update must be a document @@ -403,9 +403,9 @@ replaceOne-let.json SKIP ReplaceOne with let option needs server >= 5.0 replaceOne-let.json FAIL ReplaceOne with let option unsupported (server-side error) replaceOne: expected an error, the operation succeeded replaceOne-rawdata.json SKIP ReplaceOne with rawData option needs server >= 8.2.0 replaceOne-sort.json SKIP ReplaceOne with sort option needs server >= 8.0 -updateMany-arrayFilters.json FAIL UpdateMany when no documents match arrayFilters updateMany.modifiedCount: expected 0, got 2 -updateMany-arrayFilters.json FAIL UpdateMany when one document matches arrayFilters updateMany.modifiedCount: expected 1, got 2 -updateMany-arrayFilters.json FAIL UpdateMany when multiple documents match arrayFilters outcome crud-v1.coll[0].y: expected an array, got {"$[i]":{"b":2}} +updateMany-arrayFilters.json FAIL UpdateMany when no documents match arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +updateMany-arrayFilters.json FAIL UpdateMany when one document matches arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +updateMany-arrayFilters.json FAIL UpdateMany when multiple documents match arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server updateMany-collation.json FAIL UpdateMany when many documents match with collation updateMany.matchedCount: expected 2, got 1 updateMany-comment.json SKIP UpdateMany with comment - pre 4.4 needs server <= 4.2.99 updateMany-dots_and_dollars.json SKIP Updating document to set top-level dollar-prefixed key on 5.0+ server needs server >= 5.0 @@ -416,11 +416,11 @@ updateMany-let.json SKIP updateMany with let option needs server >= 5.0 updateMany-let.json FAIL updateMany with let option unsupported (server-side error) updateMany: error message "update spec requires u" does not contain "'update.let' is an unknown field" updateMany-pipeline.json FAIL UpdateMany using pipelines MongoServerError: update spec requires u updateMany-rawdata.json SKIP updateMany with rawData option needs server >= 8.2.0 -updateOne-arrayFilters.json FAIL UpdateOne when no document matches arrayFilters updateOne.modifiedCount: expected 0, got 1 -updateOne-arrayFilters.json FAIL UpdateOne when one document matches arrayFilters outcome crud-v1.coll[0].y: expected an array, got {"$[i]":{"b":2}} -updateOne-arrayFilters.json FAIL UpdateOne when multiple documents match arrayFilters outcome crud-v1.coll[0].y: expected an array, got {"$[i]":{"b":2}} -updateOne-arrayFilters.json FAIL UpdateOne when no documents match multiple arrayFilters updateOne.modifiedCount: expected 0, got 1 -updateOne-arrayFilters.json FAIL UpdateOne when one document matches multiple arrayFilters outcome crud-v1.coll[2].y: expected an array, got {"$[i]":{"c":{"$[j]":{"d":0}}}} +updateOne-arrayFilters.json FAIL UpdateOne when no document matches arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +updateOne-arrayFilters.json FAIL UpdateOne when one document matches arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +updateOne-arrayFilters.json FAIL UpdateOne when multiple documents match arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].b' is not implemented by this server +updateOne-arrayFilters.json FAIL UpdateOne when no documents match multiple arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].c.$[j].d' is not implemented by this server +updateOne-arrayFilters.json FAIL UpdateOne when one document matches multiple arrayFilters MongoServerError: the positional operator '$[i]' in path 'y.$[i].c.$[j].d' is not implemented by this server updateOne-collation.json FAIL UpdateOne when one document matches with collation updateOne.matchedCount: expected 1, got 0 updateOne-comment.json SKIP UpdateOne with comment - pre 4.4 needs server <= 4.2.99 updateOne-dots_and_dollars.json SKIP Updating document to set top-level dollar-prefixed key on 5.0+ server needs server >= 5.0