src/commands.zig

@@ -66,6 +66,10 @@ pub const ErrorCode = enum(i32) {
     duplicate_key = 11000,
     namespace_exists = 48,
     failed_to_parse = 9,
+    /// `ConflictingUpdateOperators`, measured on mongod 8.3.7: two paths in
+    /// one update where either is a prefix of the other, so which of them
+    /// decides the result would depend on the order the operators ran in.
+    conflicting_update_operators = 40,
     internal_error = 1,
     /// "Unrecognized pipeline stage name". A `Location` code, so mongod names it
     /// `Location40324` rather than after any symbol.
@@ -4326,6 +4330,26 @@ fn update_refusal(reply: *wire.Reply, err: anyerror, diag: update.Diagnostic) !v
                 "$type expression ({{$type: 'timestamp/date'}}).",
             .{diag.other},
         )),
+        error.UnknownModifier => return failed_to_parse(reply, try std.fmt.allocPrint(
+            arena,
+            "Unknown modifier: {s}. Expected a valid update modifier or pipeline-style " ++
+                "update specified as an array",
+            .{diag.segment},
+        )),
+        error.ModifierNeedsFields => return failed_to_parse(reply, try std.fmt.allocPrint(
+            arena,
+            "Modifiers operate on fields but we found type {s} instead",
+            .{diag.segment},
+        )),
+        error.ConflictingUpdate => return reply.put_error(
+            @intFromEnum(ErrorCode.conflicting_update_operators),
+            "ConflictingUpdateOperators",
+            try std.fmt.allocPrint(
+                arena,
+                "Updating the path '{s}' would create a conflict at '{s}'",
+                .{ diag.path, diag.segment },
+            ),
+        ),
         error.PathNotViable => return reply.put_error(
             @intFromEnum(ErrorCode.path_not_viable),
             "PathNotViable",

src/update.zig

@@ -34,6 +34,14 @@ pub const UpdateError = error{
     PullAllNeedsArray,
     /// `$each` that is not an array.
     BadEach,
+    /// A name that is not in the operator table. mongod's `FailedToParse` (9),
+    /// and a refusal rather than a silent skip.
+    UnknownModifier,
+    /// A known operator whose argument is not a document of fields.
+    ModifierNeedsFields,
+    /// Two paths in one update where either is a prefix of the other:
+    /// mongod's `ConflictingUpdateOperators` (40).
+    ConflictingUpdate,
     /// `$currentDate` handed a document that is not `{$type: date|timestamp}`.
     BadCurrentDateType,
     /// `$currentDate` handed something that is neither a bool nor a document.
@@ -235,10 +243,20 @@ pub fn validate(update: []const bson.Pair, opts: Options) UpdateError!void {
     // single case out of seventeen where this server already agreed with it.
     if (is_replacement(update)) return;
     try bind_array_filters(opts);
+    var written: WrittenPaths = .{};
     for (update) |op| {
-        const ops = doc_pairs(op.value) orelse continue;
+        if (!is_known_operator(op.key)) {
+            note(opts.diag, "", op.key);
+            return error.UnknownModifier;
+        }
+        const ops = doc_pairs(op.value) orelse {
+            note(opts.diag, "", op.value.type_name());
+            return error.ModifierNeedsFields;
+        };
         const rename = std.mem.eql(u8, op.key, "$rename");
         for (ops) |p| {
+            try note_written(&written, p.key, opts);
+            if (rename and p.value == .string) try note_written(&written, p.value.string, opts);
             if (rename) {
                 if (has_positional(p.key)) {
                     note(opts.diag, p.key, "");
@@ -260,6 +278,76 @@ pub fn validate(update: []const bson.Pair, opts: Options) UpdateError!void {
     }
 }
 
+/// The operator table. A name not in it is a typo, and a typo that is quietly
+/// ignored is an update the client believes happened -- so an unknown modifier
+/// is refused rather than skipped, which is what mongod does and what this
+/// server did not.
+const known_operators = [_][]const u8{
+    "$set", "$setOnInsert", "$unset",  "$inc",         "$mul",
+    "$min", "$max",         "$push",   "$pull",        "$addToSet",
+    "$pop", "$pullAll",     "$rename", "$currentDate",
+};
+
+fn is_known_operator(name: []const u8) bool {
+    for (known_operators) |k| if (std.mem.eql(u8, name, k)) return true;
+    return false;
+}
+
+/// How many distinct paths one update may write. Well past anything a driver
+/// sends, and the bound is what keeps the conflict check on the stack: it is
+/// quadratic in the number of paths, which is fine at this size and would not
+/// be at an unbounded one.
+const max_written_paths = 64;
+
+/// Record a path this update writes, refusing a second one that collides.
+///
+/// Two paths collide when either is a prefix of the other at a segment
+/// boundary: `a` and `a` obviously, and `a` and `a.b` because writing the
+/// parent decides what the child is. `a.b` and `a.c` are siblings and fine.
+/// mongod calls this ConflictingUpdateOperators and it applies across
+/// operators and within one: `{$set: {a: 2}, $inc: {a: 1}}` and
+/// `{$set: {a: 2, "a.b": 3}}` are both refused.
+const WrittenPaths = struct {
+    items: [max_written_paths][]const u8 = undefined,
+    n: usize = 0,
+
+    fn slice(self: *const WrittenPaths) []const []const u8 {
+        return self.items[0..self.n];
+    }
+
+    fn append(self: *WrittenPaths, path: []const u8) void {
+        // Past the bound the check stops rather than the update: refusing a
+        // legal update because it names 65 fields would be a worse answer than
+        // missing a conflict in one, and no driver writes updates that wide.
+        if (self.n == self.items.len) return;
+        self.items[self.n] = path;
+        self.n += 1;
+    }
+};
+
+fn note_written(
+    written: *WrittenPaths,
+    path: []const u8,
+    opts: Options,
+) UpdateError!void {
+    for (written.slice()) |seen| {
+        const common = prefix_of(seen, path) orelse continue;
+        if (opts.diag) |d| d.* = .{ .path = path, .segment = common };
+        return error.ConflictingUpdate;
+    }
+    written.append(path);
+}
+
+/// The shorter of two paths when one is a prefix of the other at a segment
+/// boundary, else null.
+fn prefix_of(a: []const u8, b: []const u8) ?[]const u8 {
+    const short = if (a.len <= b.len) a else b;
+    const long = if (a.len <= b.len) b else a;
+    if (!std.mem.startsWith(u8, long, short)) return null;
+    if (long.len != short.len and long[short.len] != '.') return null;
+    return short;
+}
+
 /// Read each array filter's identifier off its top-level field names.
 ///
 /// `{"i.b": 3}` binds `i`; `{"i.b": 3, "i.c": 1}` also binds `i` and is legal;
@@ -1450,6 +1538,12 @@ test "$set, $inc, $unset, $rename" {
         } } },
         .{ .key = "$inc", .value = .{ .doc = &.{.{ .key = "user.age", .value = .{ .int32 = 2 } }} } },
         .{ .key = "$unset", .value = .{ .doc = &.{.{ .key = "gone", .value = .{ .string = "" } }} } },
+    }), .{});
+    // A second update, because `$set: {new: 5}` and `$rename: {new: ...}` in
+    // one document both write `new` and mongod refuses that as a conflict --
+    // measured. This test used to pack them together and pass only because
+    // this server had no conflict check.
+    try apply(&doc, &doc_of(&.{
         .{ .key = "$rename", .value = .{ .doc = &.{.{ .key = "new", .value = .{ .string = "renamed" } }} } },
     }), .{});
 
@@ -1618,8 +1712,10 @@ test "a mixed update document is refused from either side" {
     var doc = bson.Document{ .arena = std.heap.ArenaAllocator.init(testing.allocator), .pairs = &.{} };
     defer doc.arena.deinit();
 
-    // Starts with an operator, so operators are expected throughout.
+    // Starts with an operator, so operators are expected throughout -- and a
-    try testing.expectError(error.InvalidUpdate, apply(&doc, &doc_of(&.{
+    // field that is not one is read as a modifier by that name, which is what
+    // mongod calls it too ("Unknown modifier: plain").
+    try testing.expectError(error.UnknownModifier, apply(&doc, &doc_of(&.{
         .{ .key = "$set", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 1 } }} } },
         .{ .key = "plain", .value = .{ .int32 = 1 } },
     }), .{}));
@@ -2191,21 +2287,31 @@ test "$min and $max compare in BSON order, not numerically" {
     });
     defer doc.arena.deinit();
     try apply(&doc, &doc_of(&.{
-        .{ .key = "$min", .value = .{ .doc = &.{
+        .{
-            .{ .key = "a", .value = .{ .int32 = 7 } }, // higher: not written
+            .key = "$min",
-            .{ .key = "s", .value = .{ .int32 = 5 } }, // a number is below a string
+            .value = .{
-            .{ .key = "gone", .value = .{ .int32 = 7 } }, // absent: always written
+                .doc = &.{
-        } } },
+                    .{ .key = "a", .value = .{ .int32 = 7 } }, // higher: not written
+                    .{ .key = "s", .value = .{ .int32 = 5 } }, // a number is below a string
+                    .{ .key = "gone", .value = .{ .int32 = 7 } }, // absent: always written
+                },
+            },
+        },
     }), .{});
     try testing.expectEqual(@as(i32, 5), doc.get("a").?.int32);
     try testing.expectEqual(@as(i32, 5), doc.get("s").?.int32);
     try testing.expectEqual(@as(i32, 7), doc.get("gone").?.int32);
 
     try apply(&doc, &doc_of(&.{
-        .{ .key = "$max", .value = .{ .doc = &.{
+        .{
-            .{ .key = "a", .value = .{ .int32 = 3 } }, // lower: not written
+            .key = "$max",
-            .{ .key = "n", .value = .{ .int32 = 1 } }, // a number is above null
+            .value = .{
-        } } },
+                .doc = &.{
+                    .{ .key = "a", .value = .{ .int32 = 3 } }, // lower: not written
+                    .{ .key = "n", .value = .{ .int32 = 1 } }, // a number is above null
+                },
+            },
+        },
     }), .{});
     try testing.expectEqual(@as(i32, 5), doc.get("a").?.int32);
     try testing.expectEqual(@as(i32, 1), doc.get("n").?.int32);
@@ -2272,15 +2378,20 @@ test "$addToSet identity is BSON equality, field order included" {
     });
     defer doc.arena.deinit();
     try apply(&doc, &doc_of(&.{
-        .{ .key = "$addToSet", .value = .{ .doc = &.{
+        .{
-            // An int32 2 and a double 2.0 are one value.
+            .key = "$addToSet",
-            .{ .key = "n", .value = .{ .double = 2.0 } },
+            .value = .{
-            // The same fields in the other order are two.
+                .doc = &.{
-            .{ .key = "d", .value = .{ .doc = &.{
+                    // An int32 2 and a double 2.0 are one value.
-                .{ .key = "b", .value = .{ .int32 = 2 } },
+                    .{ .key = "n", .value = .{ .double = 2.0 } },
-                .{ .key = "a", .value = .{ .int32 = 1 } },
+                    // The same fields in the other order are two.
-            } } },
+                    .{ .key = "d", .value = .{ .doc = &.{
-        } } },
+                        .{ .key = "b", .value = .{ .int32 = 2 } },
+                        .{ .key = "a", .value = .{ .int32 = 1 } },
+                    } } },
+                },
+            },
+        },
     }), .{});
     try testing.expectEqual(@as(usize, 1), doc.get("n").?.array.len);
     try testing.expectEqual(@as(usize, 2), doc.get("d").?.array.len);
@@ -2294,12 +2405,17 @@ test "$pop takes one element off an end, and is quiet when there is none" {
     });
     defer doc.arena.deinit();
     try apply(&doc, &doc_of(&.{
-        .{ .key = "$pop", .value = .{ .doc = &.{
+        .{
-            .{ .key = "a", .value = .{ .int32 = 1 } }, // the last
+            .key = "$pop",
-            .{ .key = "b", .value = .{ .double = -1.0 } }, // the first, and -1.0 is -1
+            .value = .{
-            .{ .key = "e", .value = .{ .int32 = 1 } }, // empty: no-op
+                .doc = &.{
-            .{ .key = "gone", .value = .{ .int32 = 1 } }, // absent: no-op
+                    .{ .key = "a", .value = .{ .int32 = 1 } }, // the last
-        } } },
+                    .{ .key = "b", .value = .{ .double = -1.0 } }, // the first, and -1.0 is -1
+                    .{ .key = "e", .value = .{ .int32 = 1 } }, // empty: no-op
+                    .{ .key = "gone", .value = .{ .int32 = 1 } }, // absent: no-op
+                },
+            },
+        },
     }), .{});
     try testing.expectEqual(@as(usize, 2), doc.get("a").?.array.len);
     try testing.expectEqual(@as(i32, 2), doc.get("a").?.array[1].int32);
@@ -2558,14 +2674,19 @@ test "$currentDate writes the clock it was handed" {
     var doc = try doc_with(testing.allocator, &.{.{ .key = "a", .value = .{ .int32 = 1 } }});
     defer doc.arena.deinit();
     try apply(&doc, &doc_of(&.{
-        .{ .key = "$currentDate", .value = .{ .doc = &.{
+        .{
-            .{ .key = "d", .value = .{ .bool = true } },
+            .key = "$currentDate",
-            // Measured: `false` writes a date too. The boolean says "a date",
+            .value = .{
-            // not "whether".
+                .doc = &.{
-            .{ .key = "f", .value = .{ .bool = false } },
+                    .{ .key = "d", .value = .{ .bool = true } },
-            .{ .key = "e", .value = .{ .doc = &.{.{ .key = "$type", .value = .{ .string = "date" } }} } },
+                    // Measured: `false` writes a date too. The boolean says "a date",
-            .{ .key = "t", .value = .{ .doc = &.{.{ .key = "$type", .value = .{ .string = "timestamp" } }} } },
+                    // not "whether".
-        } } },
+                    .{ .key = "f", .value = .{ .bool = false } },
+                    .{ .key = "e", .value = .{ .doc = &.{.{ .key = "$type", .value = .{ .string = "date" } }} } },
+                    .{ .key = "t", .value = .{ .doc = &.{.{ .key = "$type", .value = .{ .string = "timestamp" } }} } },
+                },
+            },
+        },
     }), .{ .now_ms = 1_700_000_000_123 });
     try testing.expectEqual(@as(i64, 1_700_000_000_123), doc.get("d").?.datetime);
     try testing.expectEqual(@as(i64, 1_700_000_000_123), doc.get("f").?.datetime);
@@ -2594,3 +2715,79 @@ test "$currentDate refuses an operand that names no type" {
     try testing.expectEqualStrings("int", diag.other);
     try testing.expect(doc.get("d") == null);
 }
+
+test "two paths in one update may not decide the same field" {
+    // ConflictingUpdateOperators: writing `a` and `a.b` in one update leaves
+    // the result depending on which operator ran first, so mongod refuses
+    // rather than picking an order. Measured, across operators and within one.
+    var doc = try doc_with(testing.allocator, &.{
+        .{ .key = "a", .value = .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 1 } }} } },
+    });
+    defer doc.arena.deinit();
+    var diag: Diagnostic = .{};
+
+    // Across two operators, on the same path.
+    try testing.expectError(error.ConflictingUpdate, apply(&doc, &doc_of(&.{
+        .{ .key = "$set", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 2 } }} } },
+        .{ .key = "$inc", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 1 } }} } },
+    }), .{ .diag = &diag }));
+    try testing.expectEqualStrings("a", diag.segment);
+
+    // Across two operators, one path a prefix of the other.
+    try testing.expectError(error.ConflictingUpdate, apply(&doc, &doc_of(&.{
+        .{ .key = "$set", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 2 } }} } },
+        .{ .key = "$inc", .value = .{ .doc = &.{.{ .key = "a.b", .value = .{ .int32 = 1 } }} } },
+    }), .{ .diag = &diag }));
+    try testing.expectEqualStrings("a.b", diag.path);
+    try testing.expectEqualStrings("a", diag.segment);
+
+    // Within one operator.
+    try testing.expectError(error.ConflictingUpdate, apply(&doc, &doc_of(&.{
+        .{ .key = "$set", .value = .{ .doc = &.{
+            .{ .key = "a", .value = .{ .int32 = 2 } },
+            .{ .key = "a.b", .value = .{ .int32 = 3 } },
+        } } },
+    }), .{ .diag = &diag }));
+
+    // `$rename` writes both ends, so both count.
+    try testing.expectError(error.ConflictingUpdate, apply(&doc, &doc_of(&.{
+        .{ .key = "$rename", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .string = "c" } }} } },
+        .{ .key = "$set", .value = .{ .doc = &.{.{ .key = "c", .value = .{ .int32 = 5 } }} } },
+    }), .{ .diag = &diag }));
+
+    // Siblings are not a conflict. Mutation check: compare paths with plain
+    // `startsWith` and this goes red -- `a.b` starts with `a.bb`'s prefix in
+    // the string sense and neither decides the other.
+    try apply(&doc, &doc_of(&.{
+        .{ .key = "$set", .value = .{ .doc = &.{.{ .key = "a.b", .value = .{ .int32 = 2 } }} } },
+        .{ .key = "$inc", .value = .{ .doc = &.{.{ .key = "a.bb", .value = .{ .int32 = 1 } }} } },
+    }), .{});
+    try testing.expectEqual(@as(i32, 2), doc.get("a").?.doc[0].value.int32);
+}
+
+test "a name that is not an operator is refused, not skipped" {
+    // It used to be `InvalidUpdate` -> BadValue, which is the right shape but
+    // the wrong code; mongod answers FailedToParse (9) and names the modifier.
+    // The reason it matters is the alternative nobody chose: skipping an
+    // unknown operator would make a typo an update the client believes ran.
+    var doc = try doc_with(testing.allocator, &.{.{ .key = "a", .value = .{ .int32 = 1 } }});
+    defer doc.arena.deinit();
+    var diag: Diagnostic = .{};
+    try testing.expectError(error.UnknownModifier, apply(&doc, &doc_of(&.{
+        .{ .key = "$bogus", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 1 } }} } },
+    }), .{ .diag = &diag }));
+    try testing.expectEqualStrings("$bogus", diag.segment);
+
+    // Even beside a good one, and nothing the good one asked for lands.
+    try testing.expectError(error.UnknownModifier, apply(&doc, &doc_of(&.{
+        .{ .key = "$set", .value = .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 1 } }} } },
+        .{ .key = "$bogus", .value = .{ .doc = &.{} } },
+    }), .{}));
+    try testing.expect(doc.get("b") == null);
+
+    // A known operator handed something that is not a document of fields.
+    try testing.expectError(error.ModifierNeedsFields, apply(&doc, &doc_of(&.{
+        .{ .key = "$set", .value = .{ .int32 = 1 } },
+    }), .{ .diag = &diag }));
+    try testing.expectEqualStrings("int", diag.segment);
+}