dev/MultiforaDB
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

update: refuse a positional path instead of destroying the array

Browse Source 
`{$set: {"y.$[i].b": 2}}` did not fail to update the array. It replaced
`y: [{b: 3}, {b: 1}]` with `y: {"$[i]": {"b": 2}}` -- every element
discarded -- and answered ok: 1, modifiedCount: 1. Remotely reachable by any
client issuing an ordinary MongoDB update.

`arrayFilters` was not implicated: the string appears nowhere in src/, the
option is accepted off the wire and dropped. The destruction was in the
path, at `set_path`'s "treat as non-array: replace with a doc" branch, so it
fired for all three spellings of "descend into this array" -- `$`, `$[]` and
`$[<ident>]` -- under every operator. `$inc` through `$[i]` stored its
operand rather than incrementing.

Two refusals, because the branch held two different mistakes:

  - a positional segment is refused up front, before anything is applied, so
    an update naming a good path and a positional one lands neither. Its
    code is BadValue (2), which is what mongod answers for every positional
    path failure.
  - a plain non-numeric segment under an array -- `y.nope.b`, `y.$x.b` -- is
    PathNotViable (28), measured. It is never a field to create, which is
    the opposite of what `set_path` does for a missing *document* field and
    the reason this branch existed at all.

Numeric segments are untouched, including the null padding past the end,
which a test now pins.

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 no BSON formatter here 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.

Re-running the 17-case probe against both servers: 16 of 17 diverged before,
0 are destructive now, and 5 agree with mongod's code exactly -- every case
where mongod also refuses. The rest refuse where mongod succeeds, which is
the honest not-implemented state and is what the design review chose.

Scorecard unchanged at 204 pass / 87 fail: the 14 arrayFilters cases still
fail, now reporting the refusal rather than a corrupted document. That was
the gate this review picked -- `docs/M3_ARRAYFILTERS_DESIGN_REVIEW.md` §5,
option D -- because the corpus has no `$[]` case and no bare `$` case at
all, so passing it would have certified two live ways to destroy an array.

206/206 unit (8 new) in ReleaseFast and ReleaseSafe, 83/83 fuzz, aggregation
corpus 70/0, full e2e matrix and crash-fuzz green. Both refusals are
mutation-checked: dropping the up-front scan reddens four tests on the
array's contents rather than on the error, and restoring the destructive
branch reddens the PathNotViable test.
This commit is contained in:
A.Shakhmatov
2026-08-10 18:21:14 +03:00
parent 3c5eee2171
commit f04e7125c9
3 changed files with 389 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files

122
src/commands.zig
View File

@@ -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