update: a positional path resolves to the elements it names
`$[]`, `$[<identifier>]` and `$` stop being refused and start being walked.
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` -- so
`resolve` expands it against the document at hand and every operator then
walks paths it already knew how to walk. Nothing below `resolve` knows a
positional segment exists, which is why `$set`, `$inc`, `$unset`, `$push` and
`$pull` all get it at once.
The static half -- which identifier binds to which array filter, whether a
segment may sit in first position, whether a filter went unused -- depends
only on the update and the filters, so it runs once in `validate` before any
document is touched. The document-dependent half is resolution itself: an
absent or non-array path is a refusal there, because array updates address
what is there rather than creating it, unlike `$set` on a plain path.
Codes and messages measured on mongod 8.3.7, not recalled. Sixteen shapes
were run; the ones that changed what this commit does:
- `$[]` in first position answers the *array filter identifier* message,
not the `$` one -- mongod treats the two spellings as one check.
- an array filter may have several top-level fields as long as they all
name the same identifier: `{i.b: 3, i.c: 1}` is legal, `{i.b: 3, j.b: 1}`
is not. So the check is on the name, not on the count.
- an identifier is `[a-z][a-zA-Z0-9]*`: `aB2` yes, `Ab`, `a_b`, `1x` no.
- `$rename` refuses a positional path on *either* end, with its own message
for each, so it keeps the plain split rather than resolving.
- `$unset` of an element leaves a null in its place rather than shortening
the array -- the same answer `$unset: {"y.0": ""}` already gave.
- a scalar element with more path below it is PathNotViable (28), not a
field to create. Without that check the walk would hand `y.1.b` to
`set_path` and it would replace the `7` at `y.1` with `{b: 9}` -- a
smaller copy of the destruction this whole walk replaced.
One divergence, measured and deliberate: `{"y.b": 3, "y.c": 2}` matches
`[{b: 3, c: 1}, {b: 1, c: 2}]` without either element satisfying both, and
`$` then picks element 1 on mongod and element 0 here. mongod's answer is an
artefact of which predicate last wrote its match position; guessing at it
would be worse than recording it. PLAN §6.
The command handlers still pass neither the query nor any array filters, so
over the wire this is `$[]` working and the other two refusing -- for the
right reason and with the right code, which they did not before. The plumbing
is the next commit.
221/221 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz.
Positional corpus 15 -> 23 of 51.
This commit is contained in:
142
src/commands.zig
142
src/commands.zig
@@ -2000,7 +2000,7 @@ fn cmd_update(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void {
|
||||
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 }, .{ .diag = &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 => {
|
||||
@@ -2124,7 +2124,7 @@ fn cmd_find_and_modify(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !v
|
||||
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 }, .{ .diag = &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.
|
||||
@@ -4141,20 +4141,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. '$[<id>]') 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",
|
||||
@@ -4190,7 +4262,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 }, .{ .diag = diag });
|
||||
return owned;
|
||||
}
|
||||
|
||||
@@ -5629,9 +5701,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 +5741,48 @@ 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 "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
|
||||
|
||||
1123
src/update.zig
1123
src/update.zig
File diff suppressed because it is too large
Load Diff
Reference in New Issue
Block a user