diff --git a/src/commands.zig b/src/commands.zig index b0c1c1c..5dc74cc 100644 --- a/src/commands.zig +++ b/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", diff --git a/src/update.zig b/src/update.zig index a6d804f..abdb2ba 100644 --- a/src/update.zig +++ b/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. - try testing.expectError(error.InvalidUpdate, apply(&doc, &doc_of(&.{ + // Starts with an operator, so operators are expected throughout -- and a + // 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 = "s", .value = .{ .int32 = 5 } }, // a number is below a string - .{ .key = "gone", .value = .{ .int32 = 7 } }, // absent: always written - } } }, + .{ + .key = "$min", + .value = .{ + .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 = "n", .value = .{ .int32 = 1 } }, // a number is above null - } } }, + .{ + .key = "$max", + .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 = "n", .value = .{ .double = 2.0 } }, - // The same fields in the other order are two. - .{ .key = "d", .value = .{ .doc = &.{ - .{ .key = "b", .value = .{ .int32 = 2 } }, - .{ .key = "a", .value = .{ .int32 = 1 } }, - } } }, - } } }, + .{ + .key = "$addToSet", + .value = .{ + .doc = &.{ + // An int32 2 and a double 2.0 are one value. + .{ .key = "n", .value = .{ .double = 2.0 } }, + // 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 = "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 - } } }, + .{ + .key = "$pop", + .value = .{ + .doc = &.{ + .{ .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 } }, - // Measured: `false` writes a date too. The boolean says "a date", - // 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" } }} } }, - } } }, + .{ + .key = "$currentDate", + .value = .{ + .doc = &.{ + .{ .key = "d", .value = .{ .bool = true } }, + // Measured: `false` writes a date too. The boolean says "a date", + // 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); +}