fix(sdam): use ObjectId comparison to track maxElectionId

Code for tracking the `maxElectionId` currently assumes that the id is represented in extended JSON. This fix modifies the test runner to parse the extended JSON into BSON, and modified the comparison logic to assume ObjectId NODE-2464
mongodb · Feb 21, 2020 · db991d6 · db991d6
1 parent acdb648
commit db991d6
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 8 deletions.
diff --git a/lib/core/sdam/topology_description.js b/lib/core/sdam/topology_description.js
@@ -279,6 +279,26 @@ function topologyTypeForServerType(serverType) {
   return TopologyType.ReplicaSetNoPrimary;
 }
 
+function compareObjectId(oid1, oid2) {
+  if (oid1 == null) {
+    return -1;
+  }
+
+  if (oid2 == null) {
+    return 1;
+  }
+
+  if (oid1.id instanceof Buffer && oid2.id instanceof Buffer) {
+    const oid1Buffer = oid1.id;
+    const oid2Buffer = oid2.id;
+    return oid1Buffer.compare(oid2Buffer);
+  }
+
+  const oid1String = oid1.toString();
+  const oid2String = oid2.toString();
+  return oid1String.localeCompare(oid2String);
+}
+
 function updateRsFromPrimary(
   serverDescriptions,
   setName,
@@ -292,11 +312,13 @@ function updateRsFromPrimary(
     return [checkHasPrimary(serverDescriptions), setName, maxSetVersion, maxElectionId];
   }
 
-  const electionIdOID = serverDescription.electionId ? serverDescription.electionId.$oid : null;
-  const maxElectionIdOID = maxElectionId ? maxElectionId.$oid : null;
-  if (serverDescription.setVersion != null && electionIdOID != null) {
-    if (maxSetVersion != null && maxElectionIdOID != null) {
-      if (maxSetVersion > serverDescription.setVersion || maxElectionIdOID > electionIdOID) {
+  const electionId = serverDescription.electionId ? serverDescription.electionId : null;
+  if (serverDescription.setVersion && electionId) {
+    if (maxSetVersion && maxElectionId) {
+      if (
+        maxSetVersion > serverDescription.setVersion ||
+        compareObjectId(maxElectionId, electionId) > 0
+      ) {
         // this primary is stale, we must remove it
         serverDescriptions.set(
           serverDescription.address,

diff --git a/test/unit/core/sdam_spec.test.js b/test/unit/core/sdam_spec.test.js
@@ -7,6 +7,7 @@ const ServerDescription = require('../../../lib/core/sdam/server_description').S
 const sdamEvents = require('../../../lib/core/sdam/events');
 const parse = require('../../../lib/core/uri_parser');
 const sinon = require('sinon');
+const EJSON = require('mongodb-extjson');
 
 const chai = require('chai');
 chai.use(require('chai-subset'));
@@ -25,7 +26,10 @@ function collectTests() {
       .readdirSync(path.join(specDir, testType))
       .filter(f => path.extname(f) === '.json')
       .map(f => {
-        const result = JSON.parse(fs.readFileSync(path.join(specDir, testType, f)));
+        const result = EJSON.parse(fs.readFileSync(path.join(specDir, testType, f)), {
+          relaxed: true
+        });
+
         result.type = testType;
         return result;
       });
@@ -226,7 +230,7 @@ function executeSDAMTest(testData, testDone) {
               const omittedFields = findOmittedFields(expectedServer);
 
               const actualServer = actualServers.get(serverName);
-              expect(actualServer).to.deep.include(expectedServer);
+              expect(actualServer).to.matchMongoSpec(expectedServer);
 
               if (omittedFields.length) {
                 expect(actualServer).to.not.have.all.keys(omittedFields);
@@ -254,7 +258,7 @@ function executeSDAMTest(testData, testDone) {
           }
 
           expect(description).to.include.keys(translatedKey);
-          expect(description[translatedKey]).to.eql(outcomeValue);
+          expect(description[translatedKey]).to.eql(outcomeValue, `(key="${translatedKey}")`);
         });
 
         // remove error handler