diff --git a/src/commands.zig b/src/commands.zig index dfa6e6d..b0c1c1c 100644 --- a/src/commands.zig +++ b/src/commands.zig @@ -1992,7 +1992,7 @@ fn cmd_update(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void { if (matched.items.len == 0) { if (upsert) { - const new_doc = build_upsert_doc(reply, q, u_doc, opts) catch |err| + const new_doc = build_upsert_doc(reply, ctx, 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), @@ -2134,7 +2134,7 @@ 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 = build_upsert_doc(reply, q, u_doc, opts) catch |err| + const new_doc = build_upsert_doc(reply, ctx, 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), @@ -4404,8 +4404,17 @@ fn wrong_type_dynamic( /// Build the document for an upsert: equality fields from the filter, then /// the update operators applied. Owned by the reply arena. +/// +/// The `_id` is settled *here* rather than in the storage engine. `insert` +/// generates one into the bytes it writes and leaves the caller's tree without +/// it, so `updateOne(..., {upsert: true}).upsertedId` came back null and +/// `findOneAndUpdate` with `returnDocument: after` returned a document missing +/// its `_id`. Both are what the client is told about a document it has never +/// seen, and both were wrong. Found by `tests/spec/operators/`, which is the +/// first corpus here to upsert into an empty collection and then look. fn build_upsert_doc( reply: *wire.Reply, + ctx: *Context, q: []const bson.Pair, u_doc: []const bson.Pair, opts: update.Options, @@ -4428,6 +4437,16 @@ fn build_upsert_doc( var insert_opts = opts; insert_opts.inserting = true; try update.apply(owned, &.{ .arena = undefined, .pairs = u_doc }, insert_opts); + // After the operators, because `$setOnInsert` may supply the `_id` itself + // and a generated one would then be the wrong answer. At the front, + // because that is where MongoDB stores it and where the `_id_` index + // descends on it. + if (owned.get("_id") == null) { + const with_id = try arena.alloc(bson.Pair, owned.pairs.len + 1); + with_id[0] = .{ .key = "_id", .value = .{ .object_id = ctx.oid_gen.new(ctx.io) } }; + @memcpy(with_id[1..], owned.pairs); + owned.pairs = with_id; + } return owned; } @@ -7058,3 +7077,57 @@ test "a rebuild kills an offsets cursor and spares a streaming one" { try testing.expectEqual(@as(i64, 0), id); try testing.expectEqual(@as(u32, 60), seen); } + +test "an upsert reports the _id it generated" { + // The client has never seen this document, so the `_id` in the reply is + // the only way it can name it again. `Engine.insert` generates one into + // the bytes it writes and leaves the caller's tree without it, so both + // `upserted` here and `findAndModify`'s returned document used to come + // back without an `_id` at all -- `upsertedId: null` on the driver. + // + // Mutation check: delete the `_id` block in `build_upsert_doc` and both + // halves go red. + 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 updates = [_]bson.Value{.{ .doc = &.{ + .{ .key = "q", .value = .{ .doc = &.{.{ .key = "k", .value = .{ .int32 = 1 } }} } }, + .{ .key = "u", .value = .{ .doc = &.{ + .{ .key = "$set", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 1 } }} } }, + } } }, + .{ .key = "upsert", .value = .{ .bool = true } }, + } }}; + var reply = wire.Reply.init(testing.allocator); + defer reply.deinit(); + var msg = try parse_fake_msg("update", .{ .string = "up" }, &.{ + .{ .key = "updates", .value = .{ .array = &updates } }, + }); + defer msg.deinit(); + try dispatch(&ctx, &msg, &reply); + const upserted = bson.get_pair(reply.pairs.items, "upserted").?.array; + try testing.expectEqual(@as(usize, 1), upserted.len); + try testing.expect(bson.get_pair(upserted[0].doc, "_id").? == .object_id); + + // And an `_id` the update supplied itself is the one that is used, rather + // than being generated over. + const with_id = [_]bson.Value{.{ .doc = &.{ + .{ .key = "q", .value = .{ .doc = &.{.{ .key = "k", .value = .{ .int32 = 2 } }} } }, + .{ .key = "u", .value = .{ .doc = &.{ + .{ .key = "$setOnInsert", .value = .{ .doc = &.{.{ .key = "_id", .value = .{ .int32 = 9 } }} } }, + } } }, + .{ .key = "upsert", .value = .{ .bool = true } }, + } }}; + var reply2 = wire.Reply.init(testing.allocator); + defer reply2.deinit(); + var msg2 = try parse_fake_msg("update", .{ .string = "up" }, &.{ + .{ .key = "updates", .value = .{ .array = &with_id } }, + }); + defer msg2.deinit(); + try dispatch(&ctx, &msg2, &reply2); + const upserted2 = bson.get_pair(reply2.pairs.items, "upserted").?.array; + try testing.expectEqual(@as(i32, 9), bson.get_pair(upserted2[0].doc, "_id").?.int32); +}