db: a write that changes nothing is not a write

`nModified` counted every write, so an update that altered nothing was reported
as a modification. MongoDB counts a document as modified only if applying the
update changed it, and writes no oplog entry when it did not: `$set: {x: 11}`
on a document already holding `x: 11` is matched and not modified. The spec
suite says it plainly -- `bulkWrite` with four updateOne operations expects
matchedCount 2 and modifiedCount 1.

Decided in the engine rather than the command, because that is where the
document is already serialized: the comparison is against the bytes that would
actually be stored, and it lands before the log append, so a no-op costs no log
record, no fsync, no slab bytes and no garbage. `Engine.replace` returns
`Written.modified` or `.unchanged` and `cmd_update` counts the first.

That exposed a second difference. A replacement keeps `_id` at the front, so
replacing a document with itself was a byte-level change whenever `_id` was not
stored first -- and it usually was not: the Node driver fills a missing `_id` by
assigning the property, which in JavaScript appends it, so `insertOne({name,
age})` reaches the server as `{name, age, _id}` and we stored it that way.
MongoDB moves `_id` to the front whatever order it arrives in. Now so does
`serialize_with_id`, for every document rather than only the ones whose `_id` it
generates. Visible to clients as `_id` coming back first, as it does from
MongoDB.

  spec scorecard   161 pass / 131 fail  ->  163 pass / 129 fail
  bulkWrite.json   8 pass / 2 fail      ->  10 pass / 0 fail
  e2e.js           45 checks -> 49

No spec file regressed. Mutation: delete the byte comparison in `upsert`'s
`.replace` arm -- red on the log growing, on the garbage counters moving, and on
`replace` claiming `.modified`.

src/commands.zig

@@ -920,7 +920,7 @@ fn cmd_update(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void {
                 error.ImmutableId, error.InvalidUpdate => return bad_value(reply, "bad update"),
                 else => return err,
             };
-            ctx.engine.replace(db_name, coll_name, copy, ctx.oid_gen) catch |err| switch (err) {
+            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);
                     e[0] = .{ .key = "index", .value = .{ .int32 = @intCast(si) } };
@@ -931,7 +931,10 @@ fn cmd_update(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void {
                 },
                 else => return err,
             };
-            n_modified += 1;
+            // `n` counts matches, `nModified` counts documents the update
+            // actually altered. A write that would store the same bytes is
+            // neither logged nor counted here.
+            if (written == .modified) n_modified += 1;
         }
     }
 
@@ -1040,7 +1043,9 @@ fn cmd_find_and_modify(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !v
             error.ImmutableId, error.InvalidUpdate => return bad_value(reply, "bad update"),
             else => return err,
         };
-        try ctx.engine.replace(db_name, coll_name, copy, ctx.oid_gen);
+        // 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);
         n = 1;
         updated_existing = true;
         value = if (ret_new) try project_doc(reply, copy, proj_pairs) else .{ .doc = before };

src/db.zig

@@ -685,17 +685,24 @@ pub const Engine = struct {
         doc: *const bson.Document,
         oid_gen: *bson.ObjectIdGen,
     ) !void {
-        return self.upsert(db_name, coll_name, doc, oid_gen, .insert);
+        _ = try self.upsert(db_name, coll_name, doc, oid_gen, .insert);
     }
 
+    /// Whether a write changed anything. A replace whose result is byte-identical
+    /// to what is stored is not an error and not a write: MongoDB reports it as
+    /// matched but not modified, and writes no oplog entry for it.
+    pub const Written = enum { modified, unchanged };
+
     /// Insert or replace a document by _id (upsert without existence check).
+    /// Returns `.unchanged` when the stored document already had these exact
+    /// bytes -- see `Written`.
     pub fn replace(
         self: *Engine,
         db_name: []const u8,
         coll_name: []const u8,
         doc: *const bson.Document,
         oid_gen: *bson.ObjectIdGen,
-    ) !void {
+    ) !Written {
         return self.upsert(db_name, coll_name, doc, oid_gen, .replace);
     }
 
@@ -716,7 +723,7 @@ pub const Engine = struct {
         doc: *const bson.Document,
         oid_gen: *bson.ObjectIdGen,
         mode: enum { insert, replace },
-    ) !void {
+    ) !Written {
         const coll = try self.get_or_create_collection(db_name, coll_name);
         const doc_bytes = try self.serialize_with_id(doc, oid_gen);
         defer self.gpa.free(doc_bytes);
@@ -759,6 +766,20 @@ pub const Engine = struct {
             };
         }
 
+        // A replace that would store the same bytes is not a write at all. It
+        // has to be decided here -- after the document is serialized, so the
+        // comparison is against what would actually be stored, and before the
+        // log append, so a no-op costs no log record, no fsync, no slab bytes
+        // and no garbage. `nModified` is the visible half of this: MongoDB
+        // counts a document as modified only if the update altered it, so
+        // `$set: {x: 11}` on a document already holding `x: 11` is matched and
+        // not modified.
+        if (mode == .replace) {
+            if (coll.id_index.lookup_exact(id_enc)) |old_off| {
+                if (std.mem.eql(u8, coll.doc_bytes(old_off), doc_bytes)) return .unchanged;
+            }
+        }
+
         // 2. Unique-index checks, _id_ included; a rejected write never
         //    reaches the log. `_id` uniqueness used to be a `docs.contains`
         //    probe here, which the docs map will not be around to answer
@@ -818,6 +839,7 @@ pub const Engine = struct {
         self.release_write_reservations(coll);
         self.note_compact();
         self.note_checkpoint();
+        return .modified;
     }
 
     /// Remove a document by its `_id` value. Returns true if it existed.
@@ -1117,12 +1139,33 @@ pub const Engine = struct {
         doc: *const bson.Document,
         oid_gen: *bson.ObjectIdGen,
     ) ![]u8 {
-        if (doc.get("_id") != null) return serialize_doc(self.gpa, doc);
+        // `_id` first, always -- generated here, or moved if the client put it
+        // later. MongoDB stores it first whatever order it arrives in, and the
+        // Node driver arrives in the other order: it fills a missing `_id` by
+        // assigning the property, which in JavaScript appends it, so an
+        // `insertOne({name, age})` reaches us as `{name, age, _id}`.
+        //
+        // Two things depend on this beyond field order in results. A replacement
+        // keeps `_id` at the front, so storing it elsewhere made replacing a
+        // document with itself a byte-level change and therefore a write. And
+        // the position is part of the stored bytes, so it has to be settled once,
+        // here, rather than by every reader.
+        if (doc.pairs.len > 0 and std.mem.eql(u8, doc.pairs[0].key, "_id")) {
+            return serialize_doc(self.gpa, doc);
+        }
         var pairs: std.ArrayListUnmanaged(bson.Pair) = .empty;
         defer pairs.deinit(self.gpa);
+        if (doc.get("_id")) |id| {
+            try pairs.append(self.gpa, .{ .key = "_id", .value = id });
+            for (doc.pairs) |p| {
+                if (std.mem.eql(u8, p.key, "_id")) continue;
+                try pairs.append(self.gpa, p);
+            }
+        } else {
             const oid = oid_gen.new(self.io);
             try pairs.append(self.gpa, .{ .key = "_id", .value = .{ .object_id = oid } });
             try pairs.appendSlice(self.gpa, doc.pairs);
+        }
 
         var out: std.ArrayListUnmanaged(u8) = .empty;
         defer out.deinit(self.gpa);
@@ -1974,7 +2017,7 @@ test "live/dead doc accounting drives compaction" {
     // A replace supersedes one record: live is unchanged, garbage grows.
     var d1b = try make_doc(gpa, 1, "alice2");
     defer d1b.deinit();
-    try engine.replace("app", "users", &d1b, &env.gen);
+    _ = try engine.replace("app", "users", &d1b, &env.gen);
     try testing.expectEqual(@as(u64, 2), engine.live_docs);
     try testing.expectEqual(@as(u64, 1), engine.dead_docs);
 
@@ -2030,7 +2073,7 @@ test "compaction reclaims garbage but leaves a garbage-free log alone" {
         for (0..200) |i| {
             var d = try make_doc(gpa, @intCast(i), if (round % 2 == 0) "yy" else "z");
             defer d.deinit();
-            try engine.replace("app", "c", &d, &env.gen);
+            _ = try engine.replace("app", "c", &d, &env.gen);
         }
         try engine.commit();
         if (engine.log.data_bytes >= after_insert * 2) break;
@@ -2044,6 +2087,63 @@ test "compaction reclaims garbage but leaves a garbage-free log alone" {
     try testing.expect(engine.log.data_bytes < after_insert * 2);
 }
 
+test "a replace that changes nothing is not a write" {
+    // Mutation check: delete the byte comparison in `upsert`'s `.replace` arm.
+    // Red on all three: the log grows, the document is superseded so the engine
+    // counts garbage that does not exist, and `replace` claims `.modified` --
+    // which is what `nModified` reports to the client.
+    //
+    // MongoDB counts a document as modified only if the update altered it, and
+    // writes no oplog entry when it did not. `$set: {x: 11}` on a document
+    // already holding `x: 11` is matched and not modified.
+    var threaded: std.Io.Threaded = .init_single_threaded;
+    defer threaded.deinit();
+    var env = test_env(&threaded);
+    const io = env.io;
+    const gpa = testing.allocator;
+
+    var tmp = try TmpLog.init(gpa);
+    defer tmp.deinit(gpa);
+    var engine = try Engine.open(gpa, io, tmp.path);
+    defer engine.deinit();
+    engine.compact_threshold = std.math.maxInt(u64);
+    try engine.lock();
+    defer engine.unlock();
+
+    var d = try make_doc(gpa, 1, "alice");
+    defer d.deinit();
+    try engine.insert("app", "users", &d, &env.gen);
+    try engine.commit();
+    const log_after_insert = engine.log.data_bytes;
+
+    // The same document again, byte for byte.
+    var same = try make_doc(gpa, 1, "alice");
+    defer same.deinit();
+    try testing.expectEqual(Engine.Written.unchanged, try engine.replace("app", "users", &same, &env.gen));
+    try engine.commit();
+    try testing.expectEqual(log_after_insert, engine.log.data_bytes);
+    try testing.expectEqual(@as(u64, 0), engine.dead_docs);
+    try testing.expectEqual(@as(u64, 0), engine.dead_bytes);
+    try testing.expectEqual(@as(u64, 1), engine.live_docs);
+
+    // A different one is a write, and is reported as one.
+    var changed = try make_doc(gpa, 1, "bob");
+    defer changed.deinit();
+    try testing.expectEqual(Engine.Written.modified, try engine.replace("app", "users", &changed, &env.gen));
+    try engine.commit();
+    try testing.expect(engine.log.data_bytes > log_after_insert);
+    try testing.expectEqual(@as(u64, 1), engine.dead_docs);
+    try testing.expectEqual(@as(u64, 1), engine.live_docs);
+
+    // And the skipped write left the document readable and correctly indexed.
+    const id_enc = try id_key_for(gpa, bson.Value{ .int32 = 1 });
+    defer gpa.free(id_enc);
+    const coll = engine.get_collection("app", "users").?;
+    const off = coll.id_index.lookup_exact(id_enc).?;
+    const stored = try bson.get_at(gpa, coll.doc_bytes(off), "name");
+    try testing.expectEqualStrings("bob", stored.?.string);
+}
+
 test "compaction still triggers after a checkpoint has truncated the log" {
     // Mutation: gate `note_compact` on `self.log.data_bytes` (what it read
     // before the checkpoint existed) instead of `self.dead_bytes`. Red, because
@@ -2090,7 +2190,7 @@ test "compaction still triggers after a checkpoint has truncated the log" {
     for (0..200) |i| {
         var d = try make_doc(gpa, @intCast(i), "yy");
         defer d.deinit();
-        try engine.replace("app", "c", &d, &env.gen);
+        _ = try engine.replace("app", "c", &d, &env.gen);
     }
     try engine.commit();
 
@@ -2138,7 +2238,7 @@ test "a rebuild leaves the space it reclaimed ready to reuse" {
     for (0..300) |i| {
         var d = try make_doc(gpa, @intCast(i), "yy");
         defer d.deinit();
-        try engine.replace("app", "c", &d, &env.gen);
+        _ = try engine.replace("app", "c", &d, &env.gen);
     }
     try engine.commit();
     try testing.expect(engine.take_compact());
@@ -2153,7 +2253,7 @@ test "a rebuild leaves the space it reclaimed ready to reuse" {
     for (0..300) |i| {
         var d = try make_doc(gpa, @intCast(i), "zzz");
         defer d.deinit();
-        try engine.replace("app", "c", &d, &env.gen);
+        _ = try engine.replace("app", "c", &d, &env.gen);
     }
     try engine.commit();
     try testing.expect(engine.pager.alloc_tail < tail_before + engine.pager.free_ready_pages() + 64);
@@ -2184,7 +2284,7 @@ test "reopen carries the garbage counter across a restart" {
         for (0..50) |i| {
             var d = try make_doc(gpa, @intCast(i), "yy");
             defer d.deinit();
-            try engine.replace("app", "c", &d, &env.gen);
+            _ = try engine.replace("app", "c", &d, &env.gen);
         }
         try engine.commit();
         try engine.checkpoint();
@@ -2431,7 +2531,7 @@ test "compact yields to a compaction already in flight" {
     var doc2 = try make_doc(gpa, 1, "alice-again");
     defer doc2.deinit();
     // A replace supersedes the first record, leaving it behind as garbage.
-    try engine.replace("app", "users", &doc2, &env.gen);
+    _ = try engine.replace("app", "users", &doc2, &env.gen);
     try engine.commit();
     engine.unlock();
     try testing.expect(engine.dead_docs > 0);
@@ -2629,7 +2729,7 @@ test "unique index enforced on insert, replace, and upsert-conflict" {
     // A replace that keeps its own email is fine (own entries excluded).
     var d1b = try make_user(gpa, 1, "a@x.io");
     defer d1b.deinit();
-    try engine.replace("app", "users", &d1b, &env.gen);
+    _ = try engine.replace("app", "users", &d1b, &env.gen);
     try testing.expectEqual(@as(usize, 1), try index_count(gpa, &engine, "app", "users", "email_1", .{ .string = "a@x.io" }));
 
     // An update that would collide is rejected.
@@ -2672,7 +2772,7 @@ test "index maintained across update and delete" {
     // Replace doc 1 with a new value: old entry gone, new entry present.
     var d1b = try doc_with_a(gpa, 1, 30);
     defer d1b.deinit();
-    try engine.replace("app", "items", &d1b, &env.gen);
+    _ = try engine.replace("app", "items", &d1b, &env.gen);
     try testing.expectEqual(@as(usize, 0), try index_count(gpa, &engine, "app", "items", "a_1", .{ .int32 = 10 }));
     try testing.expectEqual(@as(usize, 1), try index_count(gpa, &engine, "app", "items", "a_1", .{ .int32 = 30 }));
 
@@ -2946,7 +3046,7 @@ test "a rebuild reclaims dead document bytes and keeps every index valid" {
     while (i < 60) : (i += 1) {
         var d = try make_user(gpa, i, "b@x.io");
         defer d.deinit();
-        try engine.replace("app", "users", &d, &env.gen);
+        _ = try engine.replace("app", "users", &d, &env.gen);
     }
     engine.unlock();
 

tests/e2e/e2e.js

@@ -60,6 +60,30 @@ async function main() {
   const um = await users.updateMany({}, { $set: { seen: true } });
   check('updateMany', um.modifiedCount === 4, um);
 
+  // `_id` comes back first, whatever order the client sent it in. The driver
+  // fills a missing `_id` by assigning the property, which in JavaScript appends
+  // it, so this document reaches the server as {name, age, _id}.
+  const ordered = db.collection('ordering');
+  await ordered.drop().catch(() => {});
+  await ordered.insertOne({ zzz: 1, aaa: 2 });
+  const orderedDoc = await ordered.findOne({});
+  check('_id is stored first however the client ordered it', Object.keys(orderedDoc)[0] === '_id', Object.keys(orderedDoc).join(','));
+  await ordered.insertOne({ mid: 1, _id: 'explicit', tail: 2 });
+  const explicitDoc = await ordered.findOne({ _id: 'explicit' });
+  check(
+    '_id moves to the front and the other fields keep their order',
+    Object.keys(explicitDoc).join(',') === '_id,mid,tail',
+    Object.keys(explicitDoc).join(','),
+  );
+
+  // Matched but not modified: MongoDB counts a document as modified only if the
+  // update altered it, and writes nothing when it did not. Setting a field to
+  // the value it already holds is the plain case.
+  const noop = await users.updateMany({}, { $set: { seen: true } });
+  check('a no-op update matches without modifying', noop.matchedCount === 4 && noop.modifiedCount === 0, JSON.stringify(noop));
+  const noopRep = await users.replaceOne({ name: 'alice' }, await users.findOne({ name: 'alice' }));
+  check('replacing a document with itself modifies nothing', noopRep.matchedCount === 1 && noopRep.modifiedCount === 0, JSON.stringify(noopRep));
+
   const push = await users.updateOne({ name: 'dave' }, { $push: { tags: 'x' } });
   check('$push', push.modifiedCount === 1);
   check('$push visible', (await users.findOne({ name: 'dave' })).tags.length === 1);

tests/spec/scorecard.txt

@@ -13,7 +13,7 @@
 # semantics; ignoring them makes some cases pass that a full runner would
 # fail, so treat `pass` as an upper bound until M1 wires events up.
 
-total	161 pass	131 fail	195 skip	175 files	0 errored
+total	163 pass	129 fail	195 skip	175 files	0 errored
 
 # per-file: name	pass	fail	skip
 aggregate-allowdiskuse.json	3	0	0
@@ -57,7 +57,7 @@ bulkWrite-updateOne-let.json	0	1	1
 bulkWrite-updateOne-pipeline.json	0	1	0
 bulkWrite-updateOne-rawdata.json	0	1	1
 bulkWrite-updateOne-sort.json	1	0	1
-bulkWrite.json	8	2	0
+bulkWrite.json	10	0	0
 bypassDocumentValidation.json	8	1	0
 client-bulkWrite-delete-options.json	0	0	2
 client-bulkWrite-delete-rawdata.json	0	0	2
@@ -213,7 +213,7 @@ aggregate.json	SKIP	aggregate with a document comment - pre 4.4	needs server <=
 aggregate.json	SKIP	aggregate with comment does not set comment on getMore - pre 4.4	needs server <= 4.3.99
 bulkWrite-arrayFilters.json	FAIL	BulkWrite updateOne with arrayFilters	outcome crud-tests.test[0].y: expected an array, got {"$[i]":{"b":2}}
 bulkWrite-arrayFilters.json	FAIL	BulkWrite updateMany with arrayFilters	outcome crud-tests.test[0].y: expected an array, got {"$[i]":{"b":2}}
-bulkWrite-arrayFilters.json	FAIL	BulkWrite with arrayFilters	outcome crud-tests.test[0].y: expected an array, got {"$[i]":{"b":2}}
+bulkWrite-arrayFilters.json	FAIL	BulkWrite with arrayFilters	bulkWrite.modifiedCount: expected 3, got 2
 bulkWrite-collation.json	FAIL	BulkWrite with delete operations and collation	bulkWrite.deletedCount: expected 4, got 0
 bulkWrite-collation.json	FAIL	BulkWrite with update operations and collation	bulkWrite.matchedCount: expected 6, got 2
 bulkWrite-comment.json	SKIP	BulkWrite with comment - pre 4.4	needs server <= 4.2.99
@@ -266,8 +266,6 @@ bulkWrite-updateOne-pipeline.json	FAIL	UpdateOne in bulk write using pipelines	M
 bulkWrite-updateOne-rawdata.json	SKIP	BulkWrite updateOne with rawData option	needs server >= 8.2.0
 bulkWrite-updateOne-rawdata.json	FAIL	BulkWrite updateOne with rawData option on less than 8.2.0 - ignore argument	MongoBulkWriteError: update spec requires u
 bulkWrite-updateOne-sort.json	SKIP	BulkWrite updateOne with sort option	needs server >= 8.0
-bulkWrite.json	FAIL	BulkWrite with updateOne operations	bulkWrite.modifiedCount: expected 1, got 2
-bulkWrite.json	FAIL	BulkWrite with updateMany operations	bulkWrite.modifiedCount: expected 2, got 4
 bypassDocumentValidation.json	FAIL	Aggregate with $out passes bypassDocumentValidation: false	MongoServerError: Unrecognized pipeline stage name: '$out'
 client-bulkWrite-delete-options.json	SKIP	*	needs server >= 8.0
 client-bulkWrite-delete-rawdata.json	SKIP	client bulk write delete with rawData option	needs server >= 8.2.0