SERVER-63428 Robustify oplog applying code for update operation

3 files changed, 70 insertions, 42 deletions

diff --git a/src/mongo/db/ops/write_ops.cpp b/src/mongo/db/ops/write_ops.cpp
index b2b0758b284..c4b6d5bf54a 100644
--- a/src/mongo/db/ops/write_ops.cpp
+++ b/src/mongo/db/ops/write_ops.cpp
@@ -134,6 +134,11 @@ int32_t getStmtIdForWriteAt(const WriteCommandRequestBase& writeCommandBase, siz
     return kFirstStmtId + writePos;
 }
 
+bool isClassicalUpdateReplacement(const BSONObj& update) {
+    // An empty update object will be treated as replacement as firstElementFieldName() returns "".
+    return update.firstElementFieldName()[0] != '$';
+}
+
 }  // namespace write_ops
 
 write_ops::InsertCommandRequest InsertOp::parse(const OpMsgRequest& request) {
@@ -223,15 +228,20 @@ void DeleteOp::validate(const DeleteCommandRequest& deleteOp) {
 write_ops::UpdateModification write_ops::UpdateModification::parseFromOplogEntry(
     const BSONObj& oField, const DiffOptions& options) {
     BSONElement vField = oField[kUpdateOplogEntryVersionFieldName];
+    BSONElement idField = oField["_id"];
 
-    // If this field appears it should be an integer.
+    // If _id field is present, we're getting a replacement style update in which $v can be a user
+    // field. Otherwise, $v field has to be either missing or be one of the version flag $v:1 /
+    // $v:2.
     uassert(4772600,
-            str::stream() << "Expected $v field to be missing or an integer, but got type: "
-                          << vField.type(),
-            !vField.ok() ||
-                (vField.type() == BSONType::NumberInt || vField.type() == BSONType::NumberLong));
-
-    if (vField.ok() && vField.numberInt() == static_cast<int>(UpdateOplogEntryVersion::kDeltaV2)) {
+            str::stream() << "Expected _id field or $v field missing or $v:1/$v:2, but got: "
+                          << vField,
+            idField.ok() || !vField.ok() ||
+                vField.numberInt() == static_cast<int>(UpdateOplogEntryVersion::kUpdateNodeV1) ||
+                vField.numberInt() == static_cast<int>(UpdateOplogEntryVersion::kDeltaV2));
+
+    if (!idField.ok() && vField.ok() &&
+        vField.numberInt() == static_cast<int>(UpdateOplogEntryVersion::kDeltaV2)) {
         // Make sure there's a diff field.
         BSONElement diff = oField[update_oplog_entry::kDiffObjectFieldName];
         uassert(4772601,
@@ -240,18 +250,11 @@ write_ops::UpdateModification write_ops::UpdateModification::parseFromOplogEntry
                 diff.type() == BSONType::Object);
 
         return UpdateModification(doc_diff::Diff{diff.embeddedObject()}, options);
-    } else if (!vField.ok() ||
-               vField.numberInt() == static_cast<int>(UpdateOplogEntryVersion::kUpdateNodeV1)) {
+    } else {
         // Treat it as a "classic" update which can either be a full replacement or a
-        // modifier-style update. Which variant it is will be determined when the update driver is
-        // constructed.
-        return UpdateModification(oField, ClassicTag{});
+        // modifier-style update. Use "_id" field to determine whether which style it is.
+        return UpdateModification(oField, ClassicTag{}, idField.ok());
     }
-
-    // The $v field must be present, but have some unsupported value.
-    uasserted(4772604,
-              str::stream() << "Unrecognized value for '$v' (Version) field: "
-                            << vField.numberInt());
 }
 
 write_ops::UpdateModification::UpdateModification(doc_diff::Diff diff, DiffOptions options)
@@ -263,7 +266,7 @@ write_ops::UpdateModification::UpdateModification(TransformFunc transform)
 write_ops::UpdateModification::UpdateModification(BSONElement update) {
     const auto type = update.type();
     if (type == BSONType::Object) {
-        _update = ClassicUpdate{update.Obj()};
+        _update = UpdateModification(update.Obj(), ClassicTag{})._update;
         return;
     }
 
@@ -274,19 +277,24 @@ write_ops::UpdateModification::UpdateModification(BSONElement update) {
     _update = PipelineUpdate{parsePipelineFromBSON(update)};
 }
 
-write_ops::UpdateModification::UpdateModification(const BSONObj& update, ClassicTag) {
-    // Do a sanity check that the $v field is either not provided or has value of 1.
-    const auto versionElem = update["$v"];
-    uassert(4772602,
-            str::stream() << "Expected classic update either contain no '$v' field, or "
-                          << "'$v' field with value 1, but found: " << versionElem,
-            !versionElem.ok() ||
-                versionElem.numberInt() ==
-                    static_cast<int>(UpdateOplogEntryVersion::kUpdateNodeV1));
-
-    _update = ClassicUpdate{update};
+// If we know whether the update is a replacement, use that value. For example, when we're parsing
+// the oplog entry, we know if the update is a replacement by checking whether there's an _id field.
+write_ops::UpdateModification::UpdateModification(const BSONObj& update,
+                                                  ClassicTag,
+                                                  bool isReplacement) {
+    if (isReplacement) {
+        _update = ReplacementUpdate{update};
+    } else {
+        _update = ModifierUpdate{update};
+    }
 }
 
+// If we don't know whether the update is a replacement, for example while we are parsing a user
+// request, we infer this by checking whether the first element is a $-field to distinguish modifier
+// style updates.
+write_ops::UpdateModification::UpdateModification(const BSONObj& update, ClassicTag)
+    : UpdateModification(update, ClassicTag{}, isClassicalUpdateReplacement(update)) {}
+
 write_ops::UpdateModification::UpdateModification(std::vector<BSONObj> pipeline)
     : _update{PipelineUpdate{std::move(pipeline)}} {}
 
@@ -301,7 +309,8 @@ write_ops::UpdateModification write_ops::UpdateModification::parseFromBSON(BSONE
 int write_ops::UpdateModification::objsize() const {
     return stdx::visit(
         visit_helper::Overloaded{
-            [](const ClassicUpdate& classic) -> int { return classic.bson.objsize(); },
+            [](const ReplacementUpdate& replacement) -> int { return replacement.bson.objsize(); },
+            [](const ModifierUpdate& modifier) -> int { return modifier.bson.objsize(); },
             [](const PipelineUpdate& pipeline) -> int {
                 int size = 0;
                 std::for_each(pipeline.begin(), pipeline.end(), [&size](const BSONObj& obj) {
@@ -319,7 +328,8 @@ int write_ops::UpdateModification::objsize() const {
 write_ops::UpdateModification::Type write_ops::UpdateModification::type() const {
     return stdx::visit(
         visit_helper::Overloaded{
-            [](const ClassicUpdate& classic) { return Type::kClassic; },
+            [](const ReplacementUpdate& replacement) { return Type::kReplacement; },
+            [](const ModifierUpdate& modifier) { return Type::kModifier; },
             [](const PipelineUpdate& pipelineUpdate) { return Type::kPipeline; },
             [](const DeltaUpdate& delta) { return Type::kDelta; },
             [](const TransformUpdate& transform) { return Type::kTransform; }},
@@ -335,7 +345,12 @@ void write_ops::UpdateModification::serializeToBSON(StringData fieldName,
 
     stdx::visit(
         visit_helper::Overloaded{
-            [fieldName, bob](const ClassicUpdate& classic) { *bob << fieldName << classic.bson; },
+            [fieldName, bob](const ReplacementUpdate& replacement) {
+                *bob << fieldName << replacement.bson;
+            },
+            [fieldName, bob](const ModifierUpdate& modifier) {
+                *bob << fieldName << modifier.bson;
+            },
             [fieldName, bob](const PipelineUpdate& pipeline) {
                 BSONArrayBuilder arrayBuilder(bob->subarrayStart(fieldName));
                 for (auto&& stage : pipeline) {
diff --git a/src/mongo/db/ops/write_ops_parsers.h b/src/mongo/db/ops/write_ops_parsers.h
index 4a70bfdc0da..797af279df7 100644
--- a/src/mongo/db/ops/write_ops_parsers.h
+++ b/src/mongo/db/ops/write_ops_parsers.h
@@ -79,7 +79,7 @@ repl::OpTime opTimeParser(BSONElement elem);
 
 class UpdateModification {
 public:
-    enum class Type { kClassic, kPipeline, kDelta, kTransform };
+    enum class Type { kReplacement, kModifier, kPipeline, kDelta, kTransform };
     using TransformFunc = std::function<boost::optional<BSONObj>(const BSONObj&)>;
 
     /**
@@ -109,6 +109,7 @@ public:
     // Creates an transform-style update. The transform function MUST preserve the _id element.
     UpdateModification(TransformFunc transform);
     // This constructor exists only to provide a fast-path for constructing classic-style updates.
+    UpdateModification(const BSONObj& update, ClassicTag, bool isReplacement);
     UpdateModification(const BSONObj& update, ClassicTag);
 
     /**
@@ -129,9 +130,14 @@ public:
 
     Type type() const;
 
-    BSONObj getUpdateClassic() const {
-        invariant(type() == Type::kClassic);
-        return stdx::get<ClassicUpdate>(_update).bson;
+    BSONObj getUpdateReplacement() const {
+        invariant(type() == Type::kReplacement);
+        return stdx::get<ReplacementUpdate>(_update).bson;
+    }
+
+    BSONObj getUpdateModifier() const {
+        invariant(type() == Type::kModifier);
+        return stdx::get<ModifierUpdate>(_update).bson;
     }
 
     const std::vector<BSONObj>& getUpdatePipeline() const {
@@ -159,8 +165,11 @@ public:
 
         stdx::visit(
             visit_helper::Overloaded{
-                [&sb](const ClassicUpdate& classic) {
-                    sb << "{type: Classic, update: " << classic.bson << "}";
+                [&sb](const ReplacementUpdate& replacement) {
+                    sb << "{type: Replacement, update: " << replacement.bson << "}";
+                },
+                [&sb](const ModifierUpdate& modifier) {
+                    sb << "{type: Modifier, update: " << modifier.bson << "}";
                 },
                 [&sb](const PipelineUpdate& pipeline) {
                     sb << "{type: Pipeline, update: " << Value(pipeline).toString() << "}";
@@ -176,7 +185,10 @@ public:
 
 private:
     // Wrapper class used to avoid having a variant where multiple alternatives have the same type.
-    struct ClassicUpdate {
+    struct ReplacementUpdate {
+        BSONObj bson;
+    };
+    struct ModifierUpdate {
         BSONObj bson;
     };
     using PipelineUpdate = std::vector<BSONObj>;
@@ -187,7 +199,8 @@ private:
     struct TransformUpdate {
         TransformFunc transform;
     };
-    stdx::variant<ClassicUpdate, PipelineUpdate, DeltaUpdate, TransformUpdate> _update;
+    stdx::variant<ReplacementUpdate, ModifierUpdate, PipelineUpdate, DeltaUpdate, TransformUpdate>
+        _update;
 };
 
 }  // namespace write_ops
diff --git a/src/mongo/db/ops/write_ops_parsers_test.cpp b/src/mongo/db/ops/write_ops_parsers_test.cpp
index 7fd55f1ef32..dff6ab2cb0b 100644
--- a/src/mongo/db/ops/write_ops_parsers_test.cpp
+++ b/src/mongo/db/ops/write_ops_parsers_test.cpp
@@ -311,8 +311,8 @@ TEST(CommandWriteOpsParsers, UpdateCommandRequest) {
                 ASSERT_BSONOBJ_EQ(op.getUpdates()[0].getQ(), query);
 
                 const auto& updateMod = op.getUpdates()[0].getU();
-                ASSERT(updateMod.type() == write_ops::UpdateModification::Type::kClassic);
-                ASSERT_BSONOBJ_EQ(updateMod.getUpdateClassic(), update);
+                ASSERT(updateMod.type() == write_ops::UpdateModification::Type::kModifier);
+                ASSERT_BSONOBJ_EQ(updateMod.getUpdateModifier(), update);
 
                 ASSERT_BSONOBJ_EQ(write_ops::collationOf(op.getUpdates()[0]), collation);
                 ASSERT_EQ(write_ops::arrayFiltersOf(op.getUpdates()[0]).size(), 1u);