SERVER-59409 part 3, make the ElectionIdSetVersionPair comparisons tidy

5 files changed, 112 insertions, 81 deletions

diff --git a/src/mongo/client/sdam/election_id_set_version_pair.h b/src/mongo/client/sdam/election_id_set_version_pair.h
index 33e8667c935..296f94b6116 100644
--- a/src/mongo/client/sdam/election_id_set_version_pair.h
+++ b/src/mongo/client/sdam/election_id_set_version_pair.h
@@ -30,14 +30,14 @@
 
 #include <boost/optional.hpp>
 
+#include "mongo/bson/bsonobjbuilder.h"
 #include "mongo/bson/oid.h"
-
 namespace mongo::sdam {
 
 // Comparable pair or ElectionId (term) and SetVersion.
 struct ElectionIdSetVersionPair {
-    const boost::optional<mongo::OID> electionId;
-    const boost::optional<int> setVersion;
+    boost::optional<mongo::OID> electionId;
+    boost::optional<int> setVersion;
 
     bool allDefined() const {
         return electionId && setVersion;
@@ -46,6 +46,12 @@ struct ElectionIdSetVersionPair {
     bool allUndefined() const {
         return !electionId && !setVersion;
     }
+
+    bool anyUndefined() const {
+        return !electionId || !setVersion;
+    }
+
+    BSONObj toBSON() const;
 };
 
 inline bool electionIdEqual(const ElectionIdSetVersionPair& p1,
@@ -54,19 +60,12 @@ inline bool electionIdEqual(const ElectionIdSetVersionPair& p1,
 }
 
 inline bool operator<(const ElectionIdSetVersionPair& p1, const ElectionIdSetVersionPair& p2) {
-    if (electionIdEqual(p1, p2)) {
-        if (p1.setVersion && p2.setVersion) {
-            return *p1.setVersion < *p2.setVersion;
-        }
-        return false;  // Cannot compare Set versions.
-    }
-
-    if (p1.electionId && p2.electionId) {
-        return (*p1.electionId).compare(*p2.electionId) < 0;
+    if (p1.anyUndefined() && p2.allDefined()) {
+        return true;
     }
 
-    // We know that election Ids are not comparable.
-    return false;
+    return (p1.electionId < p2.electionId) ||
+        ((p1.electionId == p2.electionId) && (p1.setVersion < p2.setVersion));
 }
 
 inline bool operator>(const ElectionIdSetVersionPair& p1, const ElectionIdSetVersionPair& p2) {
@@ -75,15 +74,7 @@ inline bool operator>(const ElectionIdSetVersionPair& p1, const ElectionIdSetVer
 
 // Equality operator is not equivalent to "!< && !>" because of grey area of undefined values.
 inline bool operator==(const ElectionIdSetVersionPair& p1, const ElectionIdSetVersionPair& p2) {
-    if (!electionIdEqual(p1, p2)) {
-        return false;
-    }
-
-    if (!p1.setVersion && !p2.setVersion) {
-        return true;
-    }
-
-    return p1.setVersion && p2.setVersion && *p1.setVersion == *p2.setVersion;
+    return ((p1.electionId == p2.electionId) && (p1.setVersion == p2.setVersion));
 }
 
 inline bool setVersionWentBackwards(const ElectionIdSetVersionPair& current,
@@ -96,30 +87,26 @@ inline bool setVersionWentBackwards(const ElectionIdSetVersionPair& current,
  */
 inline bool isIncomingPrimaryConsistent(const ElectionIdSetVersionPair& current,
                                         const ElectionIdSetVersionPair& incoming) {
-    // If coming from primary both election Id and Set version should be always set.
-    // This requirement is true for at least MDB >= 4.0.
-    if (!incoming.electionId || !incoming.setVersion) {
-        return false;
-    }
-
-    if (current.allUndefined()) {
-        return true;  // Further check is not necessary.
-    }
-
-    // Identical term means Set version should be equal or advance because no failover
-    // happened and Set version at the same primary cannot go backwards.
-    if (electionIdEqual(current, incoming)) {
-        return !current.setVersion || *current.setVersion <= *incoming.setVersion;
-    }
-
     // If Set version goes backwards the term should advance, because it means
     // failover happened. This is possible if the previous primary failed to replicate
     // new Set version before failover. When it happens, we will rollback the Set version.
     if (setVersionWentBackwards(current, incoming)) {
-        return !current.electionId || (*current.electionId).compare(*incoming.electionId) < 0;
+        return !current.electionId || !incoming.electionId ||
+            (*current.electionId).compare(*incoming.electionId) < 0;
     }
 
     return true;
 }
 
+inline BSONObj ElectionIdSetVersionPair::toBSON() const {
+    BSONObjBuilder bob;
+    if (electionId) {
+        bob.append("electionId", *electionId);
+    }
+    if (setVersion) {
+        bob.append("setVersion", *setVersion);
+    }
+    return bob.obj();
+}
+
 }  // namespace mongo::sdam
diff --git a/src/mongo/client/sdam/election_id_set_version_pair_test.cpp b/src/mongo/client/sdam/election_id_set_version_pair_test.cpp
index b375a6760fc..93a95c10097 100644
--- a/src/mongo/client/sdam/election_id_set_version_pair_test.cpp
+++ b/src/mongo/client/sdam/election_id_set_version_pair_test.cpp
@@ -97,27 +97,27 @@ public:
 
 TEST_F(ElectionIdSetVersionPairTest, ExpectedOutcome) {
     std::vector<TestCase> tests = {
-        // At startup, both current fields are not set.
-        {kNullOid, kNullSet, kOidOne, 1, Compare::kNotComparable, Consistency::kConsistent},
+        // At startup, both current fields are not set. Field set > unset.
+        {kNullOid, kNullSet, kOidOne, 1, Compare::kLess, Consistency::kConsistent},
 
         {kOidOne, 1, kOidOne, 1, Compare::kEquals, Consistency::kConsistent},
 
         // One field is not set. This should never happen however added for better
         // coverage for malformed protocol.
-        {kNullOid, 1, kOidOne, 1, Compare::kNotComparable, Consistency::kConsistent},
-        {kOidOne, kNullSet, kOidOne, 1, Compare::kNotComparable, Consistency::kConsistent},
-        {kOidOne, 1, kNullOid, 1, Compare::kNotComparable, Consistency::kInconsistent},
-        {kOidOne, 1, kOidOne, kNullSet, Compare::kNotComparable, Consistency::kInconsistent},
+        {kNullOid, 1, kOidOne, 1, Compare::kLess, Consistency::kConsistent},
+        {kOidOne, kNullSet, kOidOne, 1, Compare::kLess, Consistency::kConsistent},
+        {kOidOne, 1, kNullOid, 1, Compare::kGreater, Consistency::kConsistent},
+        {kOidOne, 1, kOidOne, kNullSet, Compare::kGreater, Consistency::kConsistent},
 
         // Primary advanced one way or another. "Less" means current < incoming.
         {kOidOne, 1, kOidTwo, 1, Compare::kLess, Consistency::kConsistent},
         {kOidOne, 1, kOidOne, 2, Compare::kLess, Consistency::kConsistent},
 
         // Primary advanced but current state is incomplete.
-        {kNullOid, 1, kOidTwo, 1, Compare::kNotComparable, Consistency::kConsistent},
-        {kNullOid, 1, kOidOne, 2, Compare::kNotComparable, Consistency::kConsistent},
+        {kNullOid, 1, kOidTwo, 1, Compare::kLess, Consistency::kConsistent},
+        {kNullOid, 1, kOidOne, 2, Compare::kLess, Consistency::kConsistent},
         {kOidOne, kNullSet, kOidTwo, 1, Compare::kLess, Consistency::kConsistent},
-        {kOidOne, kNullSet, kOidOne, 2, Compare::kNotComparable, Consistency::kConsistent},
+        {kOidOne, kNullSet, kOidOne, 2, Compare::kLess, Consistency::kConsistent},
 
         // Primary went backwards one way or another.
         // Inconsistent because Set version went backwards without Term being changed (same
@@ -126,7 +126,7 @@ TEST_F(ElectionIdSetVersionPairTest, ExpectedOutcome) {
         {kOidTwo, 2, kOidOne, 2, Compare::kGreater, Consistency::kConsistent},
 
         // Primary went backwards with current state incomplete.
-        {kNullOid, 2, kOidTwo, 1, Compare::kNotComparable, Consistency::kConsistent},
+        {kNullOid, 2, kOidTwo, 1, Compare::kLess, Consistency::kConsistent},
         {kOidTwo, kNullSet, kOidOne, 1, Compare::kGreater, Consistency::kConsistent},
 
         // Stale primary case when Term went backwards but Set version advanced.
@@ -145,5 +145,49 @@ TEST_F(ElectionIdSetVersionPairTest, ExpectedOutcome) {
     }
 }
 
+TEST_F(ElectionIdSetVersionPairTest, TestLessThanRelation) {
+    ElectionIdSetVersionPair hasNone{boost::none, boost::none};
+    ElectionIdSetVersionPair hasBoth{kOidOne, 1};
+    ElectionIdSetVersionPair hasBoth2{kOidOne, 2};
+    ElectionIdSetVersionPair noEid{boost::none, 1};
+    ElectionIdSetVersionPair noEid2{boost::none, 2};
+    ElectionIdSetVersionPair noSet{kOidOne, boost::none};
+    ElectionIdSetVersionPair noSet2{kOidTwo, boost::none};
+
+    ASSERT_TRUE(hasNone < noEid);
+    ASSERT(hasNone == hasNone);
+
+    ASSERT_TRUE(hasNone < noEid2);
+    ASSERT_TRUE(noEid < noEid2);
+    ASSERT(noEid == noEid);
+
+    ASSERT_TRUE(hasNone < noSet);
+    ASSERT_TRUE(noEid < noSet);
+    ASSERT_TRUE(noEid2 < noSet);
+    ASSERT(noEid2 == noEid2);
+
+    ASSERT_TRUE(hasNone < noSet2);
+    ASSERT_TRUE(noEid < noSet2);
+    ASSERT_TRUE(noEid2 < noSet2);
+    ASSERT_TRUE(noSet < noSet2);
+    ASSERT(noSet == noSet);
+
+    ASSERT_TRUE(hasNone < hasBoth);
+    ASSERT_TRUE(noEid < hasBoth);
+    ASSERT_TRUE(noEid2 < hasBoth);
+    ASSERT_TRUE(noSet < hasBoth);
+    ASSERT_TRUE(noSet2 < hasBoth);
+    ASSERT(noSet2 == noSet2);
+
+    ASSERT_TRUE(hasNone < hasBoth2);
+    ASSERT_TRUE(noEid < hasBoth2);
+    ASSERT_TRUE(noEid2 < hasBoth2);
+    ASSERT_TRUE(noSet < hasBoth2);
+    ASSERT_TRUE(noSet2 < hasBoth2);
+    ASSERT_TRUE(hasBoth < hasBoth2);
+    ASSERT(hasBoth == hasBoth);
+    ASSERT(hasBoth2 == hasBoth2);
+}
+
 }  // namespace
 }  // namespace mongo::sdam
diff --git a/src/mongo/client/sdam/sdam_json_test_runner.cpp b/src/mongo/client/sdam/sdam_json_test_runner.cpp
index 0363d4ca599..749585cf01c 100644
--- a/src/mongo/client/sdam/sdam_json_test_runner.cpp
+++ b/src/mongo/client/sdam/sdam_json_test_runner.cpp
@@ -223,7 +223,7 @@ private:
                                       }
                                       return result;
                                   },
-                                  serverDescription->getSetVersion());
+                                  serverDescription->getElectionIdSetVersionPair().setVersion);
 
         } else if (fieldName == "electionId") {
             doValidateServerField(result,
@@ -236,7 +236,7 @@ private:
                                       }
                                       return result;
                                   },
-                                  serverDescription->getElectionId());
+                                  serverDescription->getElectionIdSetVersionPair().electionId);
 
         } else if (fieldName == "logicalSessionTimeoutMinutes") {
             doValidateServerField(result,
@@ -368,36 +368,36 @@ private:
         {
             constexpr auto fieldName = "maxSetVersion";
             if (bsonTopologyDescription.hasField(fieldName)) {
-                doValidateTopologyDescriptionField(result,
-                                                   fieldName,
-                                                   [&]() {
-                                                       boost::optional<int> ret;
-                                                       auto bsonField =
-                                                           bsonTopologyDescription[fieldName];
-                                                       if (!bsonField.isNull()) {
-                                                           ret = bsonField.numberInt();
-                                                       }
-                                                       return ret;
-                                                   },
-                                                   topologyDescription->getMaxSetVersion());
+                doValidateTopologyDescriptionField(
+                    result,
+                    fieldName,
+                    [&]() {
+                        boost::optional<int> ret;
+                        auto bsonField = bsonTopologyDescription[fieldName];
+                        if (!bsonField.isNull()) {
+                            ret = bsonField.numberInt();
+                        }
+                        return ret;
+                    },
+                    topologyDescription->getMaxElectionIdSetVersionPair().setVersion);
             }
         }
 
         {
             constexpr auto fieldName = "maxElectionId";
             if (bsonTopologyDescription.hasField(fieldName)) {
-                doValidateTopologyDescriptionField(result,
-                                                   fieldName,
-                                                   [&]() {
-                                                       boost::optional<OID> ret;
-                                                       auto bsonField =
-                                                           bsonTopologyDescription[fieldName];
-                                                       if (!bsonField.isNull()) {
-                                                           ret = bsonField.OID();
-                                                       }
-                                                       return ret;
-                                                   },
-                                                   topologyDescription->getMaxElectionId());
+                doValidateTopologyDescriptionField(
+                    result,
+                    fieldName,
+                    [&]() {
+                        boost::optional<OID> ret;
+                        auto bsonField = bsonTopologyDescription[fieldName];
+                        if (!bsonField.isNull()) {
+                            ret = bsonField.OID();
+                        }
+                        return ret;
+                    },
+                    topologyDescription->getMaxElectionIdSetVersionPair().electionId);
             }
         }
 
diff --git a/src/mongo/client/sdam/topology_description_test.cpp b/src/mongo/client/sdam/topology_description_test.cpp
index 9622e3dfdd7..8c741bce134 100644
--- a/src/mongo/client/sdam/topology_description_test.cpp
+++ b/src/mongo/client/sdam/topology_description_test.cpp
@@ -79,7 +79,7 @@ protected:
 void TopologyDescriptionTestFixture::assertDefaultConfig(
     const TopologyDescription& topologyDescription) {
     ASSERT_EQUALS(boost::none, topologyDescription.getSetName());
-    ASSERT_EQUALS(boost::none, topologyDescription.getMaxElectionId());
+    ASSERT_EQUALS(boost::none, topologyDescription.getMaxElectionIdSetVersionPair().electionId);
 
     auto expectedDefaultServer = ServerDescription(HostAndPort("localhost:27017"));
     ASSERT_EQUALS(expectedDefaultServer, *topologyDescription.getServers().front());
diff --git a/src/mongo/client/sdam/topology_state_machine_test.cpp b/src/mongo/client/sdam/topology_state_machine_test.cpp
index 6671d6eb9e2..5f47b8386f0 100644
--- a/src/mongo/client/sdam/topology_state_machine_test.cpp
+++ b/src/mongo/client/sdam/topology_state_machine_test.cpp
@@ -378,7 +378,7 @@ TEST_F(TopologyStateMachineTestFixture, ShouldSaveNewMaxSetVersion) {
                                  .instance();
 
     stateMachine.onServerDescription(*topologyDescription, serverDescription);
-    ASSERT_EQUALS(100, topologyDescription->getMaxSetVersion());
+    ASSERT_EQUALS(100, topologyDescription->getMaxElectionIdSetVersionPair().setVersion);
 
     auto serverDescriptionEvenBiggerSetVersion = ServerDescriptionBuilder()
                                                      .withType(ServerType::kRSPrimary)
@@ -391,7 +391,7 @@ TEST_F(TopologyStateMachineTestFixture, ShouldSaveNewMaxSetVersion) {
                                                      .instance();
 
     stateMachine.onServerDescription(*topologyDescription, serverDescriptionEvenBiggerSetVersion);
-    ASSERT_EQUALS(200, topologyDescription->getMaxSetVersion());
+    ASSERT_EQUALS(200, topologyDescription->getMaxElectionIdSetVersionPair().setVersion);
 }
 
 TEST_F(TopologyStateMachineTestFixture, ShouldSaveNewMaxElectionId) {
@@ -410,7 +410,7 @@ TEST_F(TopologyStateMachineTestFixture, ShouldSaveNewMaxElectionId) {
                                  .instance();
 
     stateMachine.onServerDescription(*topologyDescription, serverDescription);
-    ASSERT_EQUALS(kOidOne, topologyDescription->getMaxElectionId());
+    ASSERT_EQUALS(kOidOne, topologyDescription->getMaxElectionIdSetVersionPair().electionId);
 
     auto serverDescriptionEvenBiggerElectionId = ServerDescriptionBuilder()
                                                      .withType(ServerType::kRSPrimary)
@@ -423,7 +423,7 @@ TEST_F(TopologyStateMachineTestFixture, ShouldSaveNewMaxElectionId) {
                                                      .instance();
 
     stateMachine.onServerDescription(*topologyDescription, serverDescriptionEvenBiggerElectionId);
-    ASSERT_EQUALS(kOidTwo, topologyDescription->getMaxElectionId());
+    ASSERT_EQUALS(kOidTwo, topologyDescription->getMaxElectionIdSetVersionPair().electionId);
 }
 
 // The following two tests (ShouldNotUpdateToplogyType, ShouldUpdateToCorrectToplogyType) assert