From c2f682717b6a316d000ac412224aed608033831d Mon Sep 17 00:00:00 2001 From: "A.Shakhmatov" Date: Mon, 10 Aug 2026 21:08:29 +0300 Subject: [PATCH] update: $mul, $min and $max Three operators, two shapes. `$mul` joins `$inc` in `op_arith` because they differ only in the operation; `$min` and `$max` are not numeric operators at all and get their own. Measured, and each row is a rule that would have been guessed wrong: - `{$mul: {gone: 5}}` writes **0**, not 5. An absent field starts from zero under both operators, which is the identity for one and the annihilator for the other, and mongod picks zero for both. - `$min`/`$max` compare in BSON canonical order, so `{$min: {s: 5}}` on `s: "b"` writes 5 -- a number ranks below a string -- and `{$max: {a: 1}}` on `a: null` writes 1. An absent field is always written: there is nothing to be smaller or larger than. - an int32 product that does not fit widens to int64, the same ladder `numeric_add` already climbed. `$inc` changes answer with them: a non-numeric field or operand was `InvalidUpdate` -> BadValue (2), and mongod answers TypeMismatch (14) with a different sentence for each side. So two errors rather than one, and `$inc` gets the codes it should always have had. numeric.json 0/21 -> 20/21. The one left is `$min` and `$max` on the same field, which is ConflictingUpdateOperators (40) -- a whole error class this server does not have yet, and its own commit. 228/228 unit tests. --- src/commands.zig | 19 +++++ src/update.zig | 205 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 218 insertions(+), 6 deletions(-) diff --git a/src/commands.zig b/src/commands.zig index 4b5d26f..3b7fa36 100644 --- a/src/commands.zig +++ b/src/commands.zig @@ -4253,6 +4253,25 @@ fn update_refusal(reply: *wire.Reply, err: anyerror, diag: update.Diagnostic) !v "name, found '{s}' and '{s}'", .{ diag.segment, diag.other }, )), + error.NotNumericField => return reply.put_error( + @intFromEnum(ErrorCode.type_mismatch), + "TypeMismatch", + try std.fmt.allocPrint( + arena, + "Cannot apply {s} to a value of non-numeric type. The field '{s}' is of " ++ + "non-numeric type {s}", + .{ diag.segment, diag.path, diag.other }, + ), + ), + error.NotNumericOperand => return reply.put_error( + @intFromEnum(ErrorCode.type_mismatch), + "TypeMismatch", + try std.fmt.allocPrint( + arena, + "Cannot {s} with non-numeric argument at field '{s}'", + .{ if (std.mem.eql(u8, diag.segment, "$inc")) "increment" else "multiply", diag.path }, + ), + ), error.PathNotViable => return reply.put_error( @intFromEnum(ErrorCode.path_not_viable), "PathNotViable", diff --git a/src/update.zig b/src/update.zig index e62cce0..dca3b54 100644 --- a/src/update.zig +++ b/src/update.zig @@ -16,6 +16,12 @@ const query = @import("query.zig"); pub const UpdateError = error{ ImmutableId, InvalidUpdate, + /// `$inc` or `$mul` against a stored field that is not a number, and by an + /// operand that is not one. Both are mongod's `TypeMismatch` (14) rather + /// than the `BadValue` most bad updates answer, and each has its own + /// sentence, so they are two errors. + NotNumericField, + NotNumericOperand, /// A path segment that is not a field to create: a non-numeric name /// applied to an array, or any name applied to a scalar element a /// positional segment selected. mongod's `PathNotViable`. @@ -615,7 +621,10 @@ fn apply_operator( const ops = doc_pairs(value) orelse return error.InvalidUpdate; if (std.mem.eql(u8, op, "$set")) return op_set(arena, pairs, ops, opts); if (std.mem.eql(u8, op, "$unset")) return op_unset(arena, pairs, ops, opts); - if (std.mem.eql(u8, op, "$inc")) return op_inc(arena, pairs, ops, opts); + if (std.mem.eql(u8, op, "$inc")) return op_arith(arena, pairs, ops, opts, .add); + if (std.mem.eql(u8, op, "$mul")) return op_arith(arena, pairs, ops, opts, .mul); + if (std.mem.eql(u8, op, "$min")) return op_extremum(arena, pairs, ops, opts, .lt); + if (std.mem.eql(u8, op, "$max")) return op_extremum(arena, pairs, ops, opts, .gt); if (std.mem.eql(u8, op, "$push")) return op_push(arena, pairs, ops, opts); if (std.mem.eql(u8, op, "$pull")) return op_pull(arena, pairs, ops, opts); if (std.mem.eql(u8, op, "$rename")) return op_rename(arena, pairs, ops, opts); @@ -649,18 +658,58 @@ fn op_unset( } } -fn op_inc( +/// `$inc` and `$mul`, which differ only in the operation and in what an absent +/// field starts from: `$inc` from 0 because adding leaves the operand, `$mul` +/// from 0 because multiplying does too -- so `{$mul: {gone: 5}}` writes 0, not +/// 5. Measured; the natural guess is the other one. +fn op_arith( arena: std.mem.Allocator, pairs: *std.ArrayListUnmanaged(bson.Pair), ops: []const bson.Pair, opts: Options, + comptime kind: enum { add, mul }, +) UpdateError!void { + for (ops) |p| { + // The operand is checked before the paths are resolved, so an operand + // that is not a number is one answer for the whole update rather than + // one per element a positional segment reached. + const op_name = if (kind == .add) "$inc" else "$mul"; + if (!p.value.is_number()) { + note(opts.diag, p.key, op_name); + return error.NotNumericOperand; + } + for (try resolve(arena, pairs.items, p.key, opts)) |segs| { + const current = get_value(pairs.items, segs) orelse bson.Value{ .int32 = 0 }; + if (!current.is_number()) { + if (opts.diag) |d| d.* = .{ .path = p.key, .segment = op_name, .other = current.type_name() }; + return error.NotNumericField; + } + const result = switch (kind) { + .add => try numeric_add(current, p.value), + .mul => try numeric_mul(current, p.value), + }; + try set_path(arena, pairs, segs, result, p.key, opts.diag); + } + } +} + +/// `$min` and `$max`, which are not numeric operators at all: they compare in +/// BSON canonical order, so `{$min: {s: 5}}` on `s: "b"` writes 5 because a +/// number ranks below a string. An absent field is always written -- there is +/// nothing to be smaller or larger than. +fn op_extremum( + arena: std.mem.Allocator, + pairs: *std.ArrayListUnmanaged(bson.Pair), + ops: []const bson.Pair, + opts: Options, + comptime want: std.math.Order, ) UpdateError!void { for (ops) |p| { for (try resolve(arena, pairs.items, p.key, opts)) |segs| { - const current = get_value(pairs.items, segs) orelse bson.Value{ .int32 = 0 }; - if (!current.is_number() or !p.value.is_number()) return error.InvalidUpdate; - const sum = try numeric_add(current, p.value); - try set_path(arena, pairs, segs, sum, p.key, opts.diag); + if (get_value(pairs.items, segs)) |current| { + if (bson.compare(p.value, current) != want) continue; + } + try set_path(arena, pairs, segs, try bson.copy_value(arena, p.value), p.key, opts.diag); } } } @@ -970,6 +1019,35 @@ fn numeric_add(a: bson.Value, b: bson.Value) UpdateError!bson.Value { return .{ .int64 = sum }; } +/// The same widening ladder as `numeric_add`: a double anywhere makes the +/// answer a double, two int32s stay int32 unless the product does not fit, +/// and an int64 overflowing is a refusal rather than a wrap. +fn numeric_mul(a: bson.Value, b: bson.Value) UpdateError!bson.Value { + if (a == .double or b == .double) { + const product: f64 = @floatCast(a.as_f128() * b.as_f128()); + return .{ .double = product }; + } + if (a == .int64 or b == .int64) { + const av: i64 = as_int64(a); + const bv: i64 = as_int64(b); + const product = std.math.mul(i64, av, bv) catch return error.InvalidUpdate; + return .{ .int64 = product }; + } + const product: i64 = @as(i64, a.int32) * b.int32; + if (product >= std.math.minInt(i32) and product <= std.math.maxInt(i32)) { + return .{ .int32 = @intCast(product) }; + } + return .{ .int64 = product }; +} + +fn as_int64(v: bson.Value) i64 { + return switch (v) { + .int32 => |i| i, + .int64 => |i| i, + else => unreachable, + }; +} + // --------------------------------------------------------------------------- // Tests // --------------------------------------------------------------------------- @@ -1671,3 +1749,118 @@ test "a replacement is not a path, so it is not refused" { try testing.expect(doc.get("y") == null); try testing.expectEqual(@as(i32, 1), doc.get("z").?.int32); } + +test "$mul multiplies, and starts a missing field from zero" { + // The measured surprise: `{$mul: {gone: 5}}` writes 0, not 5. Mutation + // check: start `op_arith` from `.{ .int32 = 1 }` for `.mul` and the second + // half goes red -- which is the reading anyone would reach for. + var doc = try doc_with(testing.allocator, &.{ + .{ .key = "a", .value = .{ .int32 = 5 } }, + .{ .key = "d", .value = .{ .double = 2.5 } }, + }); + defer doc.arena.deinit(); + try apply(&doc, &doc_of(&.{ + .{ .key = "$mul", .value = .{ .doc = &.{ + .{ .key = "a", .value = .{ .int32 = 2 } }, + .{ .key = "d", .value = .{ .int32 = 2 } }, + .{ .key = "gone", .value = .{ .int32 = 5 } }, + } } }, + }), .{}); + try testing.expectEqual(@as(i32, 10), doc.get("a").?.int32); + try testing.expectEqual(@as(f64, 5.0), doc.get("d").?.double); + try testing.expectEqual(@as(i32, 0), doc.get("gone").?.int32); +} + +test "$mul widens an int32 product that does not fit" { + var doc = try doc_with(testing.allocator, &.{ + .{ .key = "a", .value = .{ .int32 = 2000000000 } }, + }); + defer doc.arena.deinit(); + try apply(&doc, &doc_of(&.{ + .{ .key = "$mul", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 2 } }} } }, + }), .{}); + try testing.expectEqual(@as(i64, 4000000000), doc.get("a").?.int64); +} + +test "$inc and $mul refuse a non-number, on either side" { + // TypeMismatch, not the BadValue the rest of a bad update answers, and the + // field and the operand are different sentences on mongod -- so they are + // different errors here. + var doc = try doc_with(testing.allocator, &.{ + .{ .key = "a", .value = .{ .int32 = 5 } }, + .{ .key = "s", .value = .{ .string = "b" } }, + }); + defer doc.arena.deinit(); + var diag: Diagnostic = .{}; + for ([_][]const u8{ "$inc", "$mul" }) |op| { + try testing.expectError(error.NotNumericField, apply(&doc, &doc_of(&.{ + .{ .key = op, .value = .{ .doc = &.{.{ .key = "s", .value = .{ .int32 = 2 } }} } }, + }), .{ .diag = &diag })); + try testing.expectEqualStrings("string", diag.other); + try testing.expectError(error.NotNumericOperand, apply(&doc, &doc_of(&.{ + .{ .key = op, .value = .{ .doc = &.{.{ .key = "a", .value = .{ .string = "x" } }} } }, + }), .{ .diag = &diag })); + try testing.expectEqualStrings(op, diag.segment); + } + // Neither refusal wrote anything. + try testing.expectEqual(@as(i32, 5), doc.get("a").?.int32); + try testing.expectEqualStrings("b", doc.get("s").?.string); +} + +test "$min and $max compare in BSON order, not numerically" { + // The load-bearing case: `s` holds a string and the operand is a number, + // and there is still a defined answer because a number ranks below a + // string. Mutation check: make `op_extremum` require both to be numbers + // and the two `s` rows go red. + var doc = try doc_with(testing.allocator, &.{ + .{ .key = "a", .value = .{ .int32 = 5 } }, + .{ .key = "s", .value = .{ .string = "b" } }, + .{ .key = "n", .value = .null }, + }); + 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 + } } }, + }), .{}); + 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 + } } }, + }), .{}); + try testing.expectEqual(@as(i32, 5), doc.get("a").?.int32); + try testing.expectEqual(@as(i32, 1), doc.get("n").?.int32); +} + +test "$min treats an int and an equal double as the same value" { + // Equal is not less, so nothing is written and the stored type survives. + var doc = try doc_with(testing.allocator, &.{.{ .key = "a", .value = .{ .int32 = 5 } }}); + defer doc.arena.deinit(); + try apply(&doc, &doc_of(&.{ + .{ .key = "$min", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .double = 5.0 } }} } }, + }), .{}); + try testing.expect(doc.get("a").? == .int32); +} + +test "$mul reaches every element a positional segment names" { + var doc = try doc_with(testing.allocator, &.{ + .{ .key = "t", .value = .{ .array = &.{ + .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 2 } }} }, + .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 3 } }} }, + } } }, + }); + defer doc.arena.deinit(); + try apply(&doc, &doc_of(&.{ + .{ .key = "$mul", .value = .{ .doc = &.{.{ .key = "t.$[].a", .value = .{ .int32 = 10 } }} } }, + }), .{}); + const t = doc.get("t").?.array; + try testing.expectEqual(@as(i32, 20), t[0].doc[0].value.int32); + try testing.expectEqual(@as(i32, 30), t[1].doc[0].value.int32); +}