SERVER-38324: add validation to TransactionCoordinatorDocument decision

9 files changed, 128 insertions, 40 deletions

diff --git a/buildscripts/idl/idl/cpp_types.py b/buildscripts/idl/idl/cpp_types.py
index 940cbe14649..98e5cc83dc0 100644
--- a/buildscripts/idl/idl/cpp_types.py
+++ b/buildscripts/idl/idl/cpp_types.py
@@ -520,7 +520,9 @@ class _CppTypeOptional(_CppTypeDelegating):
     def get_setter_body(self, member_name, validator_method_name):
         # type: (unicode, unicode) -> unicode
         convert = self._base.get_transform_to_storage_type("value.get()")
-        if convert:
+        if convert or validator_method_name:
+            if not convert:
+                convert = "value.get()"
             return common.template_args(
                 textwrap.dedent("""\
                             if (value.is_initialized()) {
diff --git a/jstests/sharding/txn_basic_two_phase_commit.js b/jstests/sharding/txn_basic_two_phase_commit.js
index 3fb043e0410..530399df016 100644
--- a/jstests/sharding/txn_basic_two_phase_commit.js
+++ b/jstests/sharding/txn_basic_two_phase_commit.js
@@ -40,11 +40,11 @@
                            .getCollection("transaction_coordinators")
                            .findOne({"_id.lsid.id": lsid.id, "_id.txnNumber": txnNumber});
         assert.neq(null, coordDoc);
-        assert.eq(expectedDecision, coordDoc.decision);
+        assert.eq(expectedDecision, coordDoc.decision.decision);
         if (expectedDecision === "commit") {
-            assert.neq(null, coordDoc.commitTimestamp);
+            assert.neq(null, coordDoc.decision.commitTimestamp);
         } else {
-            assert.eq(null, coordDoc.commitTimestamp);
+            assert.eq(null, coordDoc.decision.commitTimestamp);
         }
     };
 
diff --git a/src/mongo/db/transaction_coordinator.cpp b/src/mongo/db/transaction_coordinator.cpp
index 1040735154e..ea297b10375 100644
--- a/src/mongo/db/transaction_coordinator.cpp
+++ b/src/mongo/db/transaction_coordinator.cpp
@@ -35,6 +35,7 @@
 #include "mongo/db/transaction_coordinator.h"
 
 #include "mongo/db/logical_clock.h"
+#include "mongo/db/transaction_coordinator_document_gen.h"
 #include "mongo/db/transaction_coordinator_futures_util.h"
 #include "mongo/util/log.h"
 
@@ -146,11 +147,12 @@ void TransactionCoordinator::continueCommit(const TransactionCoordinatorDocument
     // Helper lambda to get the decision either from the document passed in or from the participants
     // (by performing 'phase one' of two-phase commit).
     auto getDecision = [&]() -> Future<CoordinatorCommitDecision> {
-        if (!doc.getDecision()) {
+        auto decision = doc.getDecision();
+        if (!decision) {
             return _runPhaseOne(participantShards);
         } else {
-            return (*doc.getDecision() == "commit")
-                ? CoordinatorCommitDecision{txn::CommitDecision::kCommit, *doc.getCommitTimestamp()}
+            return (decision->decision == txn::CommitDecision::kCommit)
+                ? CoordinatorCommitDecision{txn::CommitDecision::kCommit, decision->commitTimestamp}
                 : CoordinatorCommitDecision{txn::CommitDecision::kAbort, boost::none};
         }
     };
@@ -236,4 +238,55 @@ void TransactionCoordinator::_transitionToDone(stdx::unique_lock<stdx::mutex> lk
     }
 }
 
+StatusWith<CoordinatorCommitDecision> CoordinatorCommitDecision::fromBSON(const BSONObj& doc) {
+    CoordinatorCommitDecision decision;
+
+    for (const auto& e : doc) {
+        const auto fieldName = e.fieldNameStringData();
+
+        if (fieldName == "decision") {
+            if (e.type() != String) {
+                return Status(ErrorCodes::TypeMismatch, "decision must be a string");
+            }
+
+            if (e.str() == "commit") {
+                decision.decision = txn::CommitDecision::kCommit;
+            } else if (e.str() == "abort") {
+                decision.decision = txn::CommitDecision::kAbort;
+            } else {
+                return Status(ErrorCodes::BadValue, "decision must be either 'abort' or 'commit'");
+            }
+        } else if (fieldName == "commitTimestamp") {
+            if (e.type() != bsonTimestamp && e.type() != Date) {
+                return Status(ErrorCodes::TypeMismatch, "commit timestamp must be a timestamp");
+            }
+            decision.commitTimestamp = {e.timestamp()};
+        }
+    }
+
+    if (decision.decision == txn::CommitDecision::kAbort && decision.commitTimestamp) {
+        return Status(ErrorCodes::BadValue, "abort decision cannot have a timestamp");
+    }
+    if (decision.decision == txn::CommitDecision::kCommit && !decision.commitTimestamp) {
+        return Status(ErrorCodes::BadValue, "commit decision must have a timestamp");
+    }
+
+    return decision;
+}
+
+BSONObj CoordinatorCommitDecision::toBSON() const {
+    BSONObjBuilder builder;
+
+    if (decision == txn::CommitDecision::kCommit) {
+        builder.append("decision", "commit");
+    } else {
+        builder.append("decision", "abort");
+    }
+    if (commitTimestamp) {
+        builder.append("commitTimestamp", *commitTimestamp);
+    }
+
+    return builder.obj();
+}
+
 }  // namespace mongo
diff --git a/src/mongo/db/transaction_coordinator.h b/src/mongo/db/transaction_coordinator.h
index 2fb6bfa0cef..72b9810425d 100644
--- a/src/mongo/db/transaction_coordinator.h
+++ b/src/mongo/db/transaction_coordinator.h
@@ -56,6 +56,25 @@ public:
     struct CoordinatorCommitDecision {
         txn::CommitDecision decision;
         boost::optional<Timestamp> commitTimestamp;
+
+        /**
+         * Parses a CoordinatorCommitDecision from the object.
+         */
+        static StatusWith<CoordinatorCommitDecision> fromBSON(const BSONObj& obj);
+
+        /**
+         * Returns an instance of CoordinatorCommitDecision from an object.
+         *
+         * Throws if the object cannot be deserialized.
+         */
+        static CoordinatorCommitDecision fromBSONThrowing(const BSONObj& obj) {
+            return uassertStatusOK(fromBSON(obj));
+        };
+
+        /**
+         * Returns the BSON representation of this object.
+         */
+        BSONObj toBSON() const;
     };
 
     /**
@@ -233,5 +252,4 @@ inline logger::LogstreamBuilder& operator<<(logger::LogstreamBuilder& stream,
     // clang-format on
     return stream;
 }
-
 }  // namespace mongo
diff --git a/src/mongo/db/transaction_coordinator_document.idl b/src/mongo/db/transaction_coordinator_document.idl
index af6aaf36d7c..fdf4aed5cc6 100644
--- a/src/mongo/db/transaction_coordinator_document.idl
+++ b/src/mongo/db/transaction_coordinator_document.idl
@@ -30,12 +30,21 @@ global:
     cpp_namespace: "mongo"
     cpp_includes:
         - "mongo/db/logical_session_id.h"
+        - "mongo/db/transaction_coordinator.h"
 
 imports:
     - "mongo/idl/basic_types.idl"
     - "mongo/db/logical_session_id.idl"
     - "mongo/s/sharding_types.idl"
 
+types:
+    CoordinatorCommitDecision:
+        bson_serialization_type: object
+        description: "An object representing the coordinator's commit decision."
+        cpp_type: "TransactionCoordinator::CoordinatorCommitDecision"
+        serializer: "TransactionCoordinator::CoordinatorCommitDecision::toBSON"
+        deserializer: "TransactionCoordinator::CoordinatorCommitDecision::fromBSONThrowing"
+
 structs:
     TransactionCoordinatorDocument:
         description: "A document used for majority confirming the coordinator's state changes"
@@ -49,11 +58,8 @@ structs:
                 type: array<shard_id>
                 description: "The list of transaction participants."
             decision:
-                type: string
-                description: "The coordinator's decision for the transaction, that is, 'commit' or 'abort'. Only set if the coordinator has made a decision."
-                optional: true
-                # TODO (SERVER-38324): Add validator that the string is 'abort' or 'commit'
-            commitTimestamp:
-                type: timestamp
-                description: "The clusterTime at which the participants should make the transaction's writes visible. Only set if the coordinator decided to commit."
                 optional: true
+                type: CoordinatorCommitDecision
+                description: "The coordinator's decision for the transaction including the decision
+                              ('commit' or 'abort') and a commit timestamp (if the decision is
+                              'commit'). Only set if the coordinator has made a decision."
diff --git a/src/mongo/db/transaction_coordinator_driver.cpp b/src/mongo/db/transaction_coordinator_driver.cpp
index 1853053139c..937627aff3f 100644
--- a/src/mongo/db/transaction_coordinator_driver.cpp
+++ b/src/mongo/db/transaction_coordinator_driver.cpp
@@ -40,12 +40,16 @@
 #include "mongo/db/dbdirectclient.h"
 #include "mongo/db/ops/write_ops.h"
 #include "mongo/db/repl/repl_client_info.h"
+#include "mongo/db/transaction_coordinator.h"
+#include "mongo/db/transaction_coordinator_document_gen.h"
 #include "mongo/db/transaction_coordinator_futures_util.h"
 #include "mongo/db/write_concern.h"
 #include "mongo/rpc/get_status_from_command_result.h"
 #include "mongo/util/fail_point_service.h"
 #include "mongo/util/log.h"
 
+#include "mongo/bson/bsontypes.h"
+
 namespace mongo {
 namespace {
 
@@ -321,20 +325,18 @@ void persistDecisionBlocking(OperationContext* opCtx,
             // either has no decision or the same decision. The document may have the same decision
             // if an earlier attempt to write the decision failed waiting for writeConcern.
             BSONObj noDecision = BSON(TransactionCoordinatorDocument::kDecisionFieldName
-                                      << BSON("$exists" << false)
-                                      << "commitTimestamp"
                                       << BSON("$exists" << false));
             BSONObj sameDecision;
             if (commitTimestamp) {
                 sameDecision = BSON(TransactionCoordinatorDocument::kDecisionFieldName
-                                    << "commit"
-                                    << TransactionCoordinatorDocument::kCommitTimestampFieldName
-                                    << *commitTimestamp);
+                                    << BSON(TransactionCoordinatorDocument::kDecisionFieldName
+                                            << "commit"
+                                            << "commitTimestamp"
+                                            << *commitTimestamp));
             } else {
-                sameDecision = BSON(TransactionCoordinatorDocument::kDecisionFieldName
-                                    << "abort"
-                                    << TransactionCoordinatorDocument::kCommitTimestampFieldName
-                                    << BSON("$exists" << false));
+                sameDecision =
+                    BSON(TransactionCoordinatorDocument::kDecisionFieldName
+                         << BSON(TransactionCoordinatorDocument::kDecisionFieldName << "abort"));
             }
             entry.setQ(BSON(TransactionCoordinatorDocument::kIdFieldName
                             << sessionInfo.toBSON()
@@ -347,12 +349,14 @@ void persistDecisionBlocking(OperationContext* opCtx,
             TransactionCoordinatorDocument doc;
             doc.setId(sessionInfo);
             doc.setParticipants(std::move(participantList));
+            TransactionCoordinator::CoordinatorCommitDecision decision;
             if (commitTimestamp) {
-                doc.setDecision("commit"_sd);
-                doc.setCommitTimestamp(commitTimestamp);
+                decision.decision = txn::CommitDecision::kCommit;
+                decision.commitTimestamp = commitTimestamp;
             } else {
-                doc.setDecision("abort"_sd);
+                decision.decision = txn::CommitDecision::kAbort;
             }
+            doc.setDecision(decision);
             entry.setU(doc.toBSON());
 
             return entry;
diff --git a/src/mongo/db/transaction_coordinator_driver.h b/src/mongo/db/transaction_coordinator_driver.h
index 62354189320..436fe633c56 100644
--- a/src/mongo/db/transaction_coordinator_driver.h
+++ b/src/mongo/db/transaction_coordinator_driver.h
@@ -34,10 +34,15 @@
 
 #include "mongo/db/logical_session_id.h"
 #include "mongo/db/operation_context.h"
-#include "mongo/db/transaction_coordinator_document_gen.h"
 #include "mongo/db/transaction_coordinator_futures_util.h"
+#include "mongo/executor/task_executor.h"
+#include "mongo/s/shard_id.h"
+#include "mongo/util/concurrency/mutex.h"
+#include "mongo/util/concurrency/thread_pool.h"
+#include "mongo/util/future.h"
 
 namespace mongo {
+class TransactionCoordinatorDocument;
 namespace txn {
 
 /**
diff --git a/src/mongo/db/transaction_coordinator_service.cpp b/src/mongo/db/transaction_coordinator_service.cpp
index 3731a1853f5..5a1becbe5a3 100644
--- a/src/mongo/db/transaction_coordinator_service.cpp
+++ b/src/mongo/db/transaction_coordinator_service.cpp
@@ -37,6 +37,7 @@
 #include "mongo/db/operation_context.h"
 #include "mongo/db/repl/repl_client_info.h"
 #include "mongo/db/service_context.h"
+#include "mongo/db/transaction_coordinator_document_gen.h"
 #include "mongo/db/write_concern.h"
 #include "mongo/executor/task_executor.h"
 #include "mongo/executor/task_executor_pool.h"
diff --git a/src/mongo/db/transaction_coordinator_test.cpp b/src/mongo/db/transaction_coordinator_test.cpp
index 97bbf6624b0..9d97184e7bf 100644
--- a/src/mongo/db/transaction_coordinator_test.cpp
+++ b/src/mongo/db/transaction_coordinator_test.cpp
@@ -35,6 +35,7 @@
 #include "mongo/client/remote_command_targeter_mock.h"
 #include "mongo/db/commands/txn_cmds_gen.h"
 #include "mongo/db/commands/txn_two_phase_commit_cmds_gen.h"
+#include "mongo/db/transaction_coordinator_document_gen.h"
 #include "mongo/db/transaction_coordinator_test_fixture.h"
 #include "mongo/util/log.h"
 
@@ -48,9 +49,6 @@ const StatusWith<BSONObj> kNoSuchTransaction =
 const StatusWith<BSONObj> kOk = BSON("ok" << 1);
 const Timestamp kDummyPrepareTimestamp = Timestamp(1, 1);
 
-const std::string kAbortDecision{"abort"};
-const std::string kCommitDecision{"commit"};
-
 StatusWith<BSONObj> makePrepareOkResponse(const Timestamp& timestamp) {
     return BSON("ok" << 1 << "prepareTimestamp" << timestamp);
 }
@@ -385,7 +383,7 @@ protected:
         LogicalSessionId expectedLsid,
         TxnNumber expectedTxnNum,
         std::vector<ShardId> expectedParticipants,
-        boost::optional<std::string> expectedDecision = boost::none,
+        boost::optional<txn::CommitDecision> expectedDecision = boost::none,
         boost::optional<Timestamp> expectedCommitTimestamp = boost::none) {
         ASSERT(doc.getId().getSessionId());
         ASSERT_EQUALS(*doc.getId().getSessionId(), expectedLsid);
@@ -394,17 +392,18 @@ protected:
 
         ASSERT(doc.getParticipants() == expectedParticipants);
 
+        auto decision = doc.getDecision();
         if (expectedDecision) {
-            ASSERT_EQUALS(*expectedDecision, doc.getDecision()->toString());
+            ASSERT(*expectedDecision == decision->decision);
         } else {
-            ASSERT(!doc.getDecision());
+            ASSERT(!decision);
         }
 
         if (expectedCommitTimestamp) {
-            ASSERT(doc.getCommitTimestamp());
-            ASSERT_EQUALS(*expectedCommitTimestamp, *doc.getCommitTimestamp());
-        } else {
-            ASSERT(!doc.getCommitTimestamp());
+            ASSERT(decision->commitTimestamp);
+            ASSERT_EQUALS(*expectedCommitTimestamp, *decision->commitTimestamp);
+        } else if (decision) {
+            ASSERT(!decision->commitTimestamp);
         }
     }
 
@@ -433,11 +432,11 @@ protected:
                                   lsid,
                                   txnNumber,
                                   participants,
-                                  kCommitDecision,
+                                  txn::CommitDecision::kCommit,
                                   *commitTimestamp);
         } else {
             assertDocumentMatches(
-                allCoordinatorDocs[0], lsid, txnNumber, participants, kAbortDecision);
+                allCoordinatorDocs[0], lsid, txnNumber, participants, txn::CommitDecision::kAbort);
         }
     }