commands: an upsert reports the _id it generated
`Engine.insert` generates an `_id` into the bytes it writes and leaves the caller's tree without one, so every handler that looks afterwards found nothing: `update`'s `upserted` array carried `_id: null`, which the driver surfaces as `upsertedId: null`, and `findAndModify` with `returnDocument: after` returned an upserted document with no `_id` at all. The client has never seen this document. The `_id` in the reply is the only way it can name it again, so both answers were wrong in the way that matters. Settled in `build_upsert_doc` rather than in the storage engine: the upsert path is what decides the identity of the document it builds, and it has to be after the operators run because `$setOnInsert` may supply the `_id` itself. At the front of the document, where MongoDB stores it and where the `_id_` index descends on it. Found by `tests/spec/operators/`, which is the first corpus here to upsert into an empty collection and then look at what came back. The pinned crud corpus does not move -- its upsert cases match `upsertedId` loosely. set-on-insert.json 7/9 -> 8/9. 245/245 unit tests.
This commit is contained in:
@@ -1992,7 +1992,7 @@ fn cmd_update(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void {
|
|||||||
|
|
||||||
if (matched.items.len == 0) {
|
if (matched.items.len == 0) {
|
||||||
if (upsert) {
|
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);
|
return update_refusal(reply, err, diag);
|
||||||
ctx.engine.insert(db_name, coll_name, new_doc, ctx.oid_gen) catch |err| switch (err) {
|
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),
|
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) {
|
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 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);
|
return update_refusal(reply, err, diag);
|
||||||
ctx.engine.insert(db_name, coll_name, new_doc, ctx.oid_gen) catch |err| switch (err) {
|
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),
|
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
|
/// Build the document for an upsert: equality fields from the filter, then
|
||||||
/// the update operators applied. Owned by the reply arena.
|
/// 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(
|
fn build_upsert_doc(
|
||||||
reply: *wire.Reply,
|
reply: *wire.Reply,
|
||||||
|
ctx: *Context,
|
||||||
q: []const bson.Pair,
|
q: []const bson.Pair,
|
||||||
u_doc: []const bson.Pair,
|
u_doc: []const bson.Pair,
|
||||||
opts: update.Options,
|
opts: update.Options,
|
||||||
@@ -4428,6 +4437,16 @@ fn build_upsert_doc(
|
|||||||
var insert_opts = opts;
|
var insert_opts = opts;
|
||||||
insert_opts.inserting = true;
|
insert_opts.inserting = true;
|
||||||
try update.apply(owned, &.{ .arena = undefined, .pairs = u_doc }, insert_opts);
|
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;
|
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(i64, 0), id);
|
||||||
try testing.expectEqual(@as(u32, 60), seen);
|
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);
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user