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

Author: nbbeeken

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);
+}