fix(NODE-4496): counter values incorrecly compared when instance of Long

src/bulk/common.ts

@@ -441,6 +441,7 @@ export class WriteError {
 
 /** Converts the number to a Long or returns it. */
 function longOrConvert(value: number | Long | Timestamp): Long | Timestamp {
+  // TODO(NODE-2674): Preserve int64 sent from MongoDB
   return typeof value === 'number' ? Long.fromNumber(value) : value;
 }
 

src/cursor/abstract_cursor.ts

@@ -654,6 +654,7 @@ export abstract class AbstractCursor<
         this[kServer] = state.server;
 
         if (response.cursor) {
+          // TODO(NODE-2674): Preserve int64 sent from MongoDB
           this[kId] =
             typeof response.cursor.id === 'number'
               ? Long.fromNumber(response.cursor.id)

src/sdam/monitor.ts

@@ -374,6 +374,7 @@ function makeTopologyVersion(tv: TopologyVersion) {
   return {
     processId: tv.processId,
     // tests mock counter as just number, but in a real situation counter should always be a Long
+    // TODO(NODE-2674): Preserve int64 sent from MongoDB
     counter: Long.isLong(tv.counter) ? tv.counter : Long.fromNumber(tv.counter)
   };
 }

src/sdam/server_description.ts

@@ -1,6 +1,6 @@
 import { Document, Long, ObjectId } from '../bson';
 import type { MongoError } from '../error';
-import { arrayStrictEqual, errorStrictEqual, HostAddress, now } from '../utils';
+import { arrayStrictEqual, compareObjectId, errorStrictEqual, HostAddress, now } from '../utils';
 import type { ClusterTime } from './common';
 import { ServerType } from './common';
 
@@ -105,6 +105,7 @@ export class ServerDescription {
     if (options?.topologyVersion) {
       this.topologyVersion = options.topologyVersion;
     } else if (hello?.topologyVersion) {
+      // TODO(NODE-2674): Preserve int64 sent from MongoDB
       this.topologyVersion = hello.topologyVersion;
     }
 
@@ -179,15 +180,16 @@ export class ServerDescription {
    * Determines if another `ServerDescription` is equal to this one per the rules defined
    * in the {@link https://github.com/mongodb/specifications/blob/master/source/server-discovery-and-monitoring/server-discovery-and-monitoring.rst#serverdescription|SDAM spec}
    */
-  equals(other: ServerDescription): boolean {
+  equals(other?: ServerDescription | null): boolean {
+    // TODO(NODE-4510): Check ServerDescription equality logic for nullish topologyVersion meaning "greater than"
     const topologyVersionsEqual =
-      this.topologyVersion === other.topologyVersion ||
-      compareTopologyVersion(this.topologyVersion, other.topologyVersion) === 0;
+      this.topologyVersion === other?.topologyVersion ||
+      compareTopologyVersion(this.topologyVersion, other?.topologyVersion) === 0;
 
-    const electionIdsEqual: boolean =
-      this.electionId && other.electionId
-        ? other.electionId && this.electionId.equals(other.electionId)
-        : this.electionId === other.electionId;
+    const electionIdsEqual =
+      this.electionId != null && other?.electionId != null
+        ? compareObjectId(this.electionId, other.electionId) === 0
+        : this.electionId === other?.electionId;
 
     return (
       other != null &&
@@ -254,19 +256,37 @@ function tagsStrictEqual(tags: TagSet, tags2: TagSet): boolean {
 /**
  * Compares two topology versions.
  *
- * @returns A negative number if `lhs` is older than `rhs`; positive if `lhs` is newer than `rhs`; 0 if they are equivalent.
+ * 1. If the response topologyVersion is unset or the ServerDescription's
+ *    topologyVersion is null, the client MUST assume the response is more recent.
+ * 1. If the response's topologyVersion.processId is not equal to the
+ *    ServerDescription's, the client MUST assume the response is more recent.
+ * 1. If the response's topologyVersion.processId is equal to the
+ *    ServerDescription's, the client MUST use the counter field to determine
+ *    which topologyVersion is more recent.
+ *
+ * ```ts
+ * currentTv <   newTv === -1
+ * currentTv === newTv === 0
+ * currentTv >   newTv === 1
+ * ```
  */
-export function compareTopologyVersion(lhs?: TopologyVersion, rhs?: TopologyVersion): number {
-  if (lhs == null || rhs == null) {
+export function compareTopologyVersion(
+  currentTv?: TopologyVersion,
+  newTv?: TopologyVersion
+): 0 | -1 | 1 {
+  if (currentTv == null || newTv == null) {
     return -1;
   }
 
-  if (lhs.processId.equals(rhs.processId)) {
-    // tests mock counter as just number, but in a real situation counter should always be a Long
-    const lhsCounter = Long.isLong(lhs.counter) ? lhs.counter : Long.fromNumber(lhs.counter);
-    const rhsCounter = Long.isLong(rhs.counter) ? lhs.counter : Long.fromNumber(rhs.counter);
-    return lhsCounter.compare(rhsCounter);
+  if (!currentTv.processId.equals(newTv.processId)) {
+    return -1;
   }
 
-  return -1;
+  // TODO(NODE-2674): Preserve int64 sent from MongoDB
+  const currentCounter = Long.isLong(currentTv.counter)
+    ? currentTv.counter
+    : Long.fromNumber(currentTv.counter);
+  const newCounter = Long.isLong(newTv.counter) ? newTv.counter : Long.fromNumber(newTv.counter);
+
+  return currentCounter.compare(newCounter);
 }

src/sdam/topology_description.ts

@@ -1,7 +1,7 @@
-import type { Document, ObjectId } from '../bson';
+import type { ObjectId } from '../bson';
 import * as WIRE_CONSTANTS from '../cmap/wire_protocol/constants';
 import { MongoError, MongoRuntimeError } from '../error';
-import { shuffle } from '../utils';
+import { compareObjectId, shuffle } from '../utils';
 import { ServerType, TopologyType } from './common';
 import { ServerDescription } from './server_description';
 import type { SrvPollingEvent } from './srv_polling';
@@ -363,27 +363,6 @@ function topologyTypeForServerType(serverType: ServerType): TopologyType {
   }
 }
 
-// TODO: improve these docs when ObjectId is properly typed
-function compareObjectId(oid1: Document, oid2: Document): number {
-  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: Map<string, ServerDescription>,
   serverDescription: ServerDescription,

src/sessions.ts

@@ -1002,6 +1002,7 @@ export function applySession(
   if (isRetryableWrite || inTxnOrTxnCommand) {
     serverSession.txnNumber += session[kTxnNumberIncrement];
     session[kTxnNumberIncrement] = 0;
+    // TODO(NODE-2674): Preserve int64 sent from MongoDB
     command.txnNumber = Long.fromNumber(serverSession.txnNumber);
   }
 

src/utils.ts

@@ -1387,3 +1387,25 @@ export function getMongoDBClientEncryption(): {
 
   return mongodbClientEncryption;
 }
+
+/**
+ * Compare objectIds. `null` is always less
+ * - `+1 = oid1 is greater than oid2`
+ * - `-1 = oid1 is less than oid2`
+ * - `+0 = oid1 is equal oid2`
+ */
+export function compareObjectId(oid1?: ObjectId, oid2?: ObjectId): 0 | 1 | -1 {
+  if (oid1 == null && oid2 == null) {
+    return 0;
+  }
+
+  if (oid1 == null) {
+    return -1;
+  }
+
+  if (oid2 == null) {
+    return 1;
+  }
+
+  return oid1.id.compare(oid2.id);
+}

test/unit/sdam/server_description.test.ts

@@ -0,0 +1,156 @@
+import { expect } from 'chai';
+
+import { Long, ObjectId } from '../../../src/bson';
+import {
+  compareTopologyVersion,
+  ServerDescription,
+  TopologyVersion
+} from '../../../src/sdam/server_description';
+
+describe('ServerDescription', function () {
+  describe('error equality', function () {
+    const errorEqualityTests = [
+      {
+        description: 'equal error types and messages',
+        lhs: new ServerDescription('127.0.0.1:27017', undefined, { error: new Error('test') }),
+        rhs: new ServerDescription('127.0.0.1:27017', undefined, { error: new Error('test') }),
+        equal: true
+      },
+      {
+        description: 'equal error types and unequal messages',
+        lhs: new ServerDescription('127.0.0.1:27017', undefined, { error: new Error('test') }),
+        rhs: new ServerDescription('127.0.0.1:27017', undefined, { error: new Error('blah') }),
+        equal: false
+      },
+      {
+        description: 'unequal error types and equal messages',
+        lhs: new ServerDescription('127.0.0.1:27017', undefined, { error: new TypeError('test') }),
+        rhs: new ServerDescription('127.0.0.1:27017', undefined, { error: new Error('test') }),
+        equal: false
+      },
+      {
+        description: 'null lhs',
+        lhs: new ServerDescription('127.0.0.1:27017', undefined, { error: null }),
+        rhs: new ServerDescription('127.0.0.1:27017', undefined, { error: new Error('test') }),
+        equal: false
+      },
+      {
+        description: 'null rhs',
+        lhs: new ServerDescription('127.0.0.1:27017', undefined, { error: new TypeError('test') }),
+        rhs: new ServerDescription('127.0.0.1:27017', undefined, { error: undefined }),
+        equal: false
+      }
+    ];
+
+    for (const test of errorEqualityTests) {
+      it(test.description, function () {
+        expect(test.lhs.equals(test.rhs)).to.equal(test.equal);
+      });
+    }
+  });
+
+  it('should sensibly parse an ipv6 address', function () {
+    const description = new ServerDescription('[ABCD:f::abcd:abcd:abcd:abcd]:27017');
+    expect(description.host).to.equal('abcd:f::abcd:abcd:abcd:abcd');
+    expect(description.port).to.equal(27017);
+  });
+
+  describe('compareTopologyVersion()', () => {
+    const processIdZero = new ObjectId('00'.repeat(12));
+    const processIdOne = new ObjectId('00'.repeat(11) + '01');
+    type CompareTopologyVersionTest = {
+      title: string;
+      currentTv?: TopologyVersion;
+      newTv?: TopologyVersion;
+      out: 0 | -1 | 1;
+    };
+
+    const compareTopologyVersionEqualTests: CompareTopologyVersionTest[] = [
+      {
+        title: 'when process ids are equal and both counter values are Long.ZERO',
+        currentTv: { processId: processIdZero, counter: Long.ZERO },
+        newTv: { processId: processIdZero, counter: Long.ZERO },
+        out: 0
+      },
+      {
+        title: 'when process ids are equal and both counter values are non-zero numbers',
+        // @ts-expect-error: Testing that the function handles numbers
+        currentTv: { processId: processIdZero, counter: 2 },
+        // @ts-expect-error: Testing that the function handles numbers
+        newTv: { processId: processIdZero, counter: 2 },
+        out: 0
+      },
+      {
+        title: 'when process ids are equal and both counter values are zero numbers',
+        // @ts-expect-error: Testing that the function handles numbers
+        currentTv: { processId: processIdZero, counter: 0 },
+        // @ts-expect-error: Testing that the function handles numbers
+        newTv: { processId: processIdZero, counter: 0 },
+        out: 0
+      },
+      {
+        title:
+          'when process ids are equal and counter values are equal but current has a different type',
+        currentTv: { processId: processIdZero, counter: Long.fromNumber(2) },
+        // @ts-expect-error: Testing that the function handles numbers
+        newTv: { processId: processIdZero, counter: 2 },
+        out: 0
+      },
+      {
+        title:
+          'when process ids are equal and counter values are equal but new has a different type',
+        // @ts-expect-error: Testing that the function handles numbers
+        currentTv: { processId: processIdZero, counter: 2 },
+        newTv: { processId: processIdZero, counter: Long.fromNumber(2) },
+        out: 0
+      }
+    ];
+    const compareTopologyVersionLessThanTests: CompareTopologyVersionTest[] = [
+      {
+        title: 'when both versions are nullish',
+        currentTv: undefined,
+        newTv: undefined,
+        out: -1
+      },
+      {
+        title:
+          'when new processId is greater the topologyVersion is too regardless of counter being less',
+        // Even if processId of current is greater, it is not an ordered value
+        currentTv: { processId: processIdOne, counter: Long.fromNumber(2) },
+        newTv: { processId: processIdZero, counter: Long.fromNumber(1) },
+        out: -1
+      },
+      {
+        title: 'when processIds are equal but new counter is greater',
+        currentTv: { processId: processIdZero, counter: Long.fromNumber(2) },
+        newTv: { processId: processIdZero, counter: Long.fromNumber(3) },
+        out: -1
+      }
+    ];
+    const compareTopologyVersionGreaterThanTests: CompareTopologyVersionTest[] = [
+      {
+        title: 'when processIds are equal but new counter is less than current',
+        currentTv: { processId: processIdZero, counter: Long.fromNumber(3) },
+        newTv: { processId: processIdZero, counter: Long.fromNumber(2) },
+        out: 1
+      }
+    ];
+
+    const makeTopologyVersionComparisonTests = tests => {
+      for (const { title, currentTv, newTv, out } of tests) {
+        it(title, () => {
+          expect(compareTopologyVersion(currentTv, newTv)).to.equal(out);
+        });
+      }
+    };
+    context('should return that versions are equal', () => {
+      makeTopologyVersionComparisonTests(compareTopologyVersionEqualTests);
+    });
+    context('should return that current version is less than', () => {
+      makeTopologyVersionComparisonTests(compareTopologyVersionLessThanTests);
+    });
+    context('should return that current version is greater than', () => {
+      makeTopologyVersionComparisonTests(compareTopologyVersionGreaterThanTests);
+    });
+  });
+});