From dd4eb9a7d1ccbfcc46fef58c856d7cf5af2e75af Mon Sep 17 00:00:00 2001
From: Matt Broadstone <mbroadst@gmail.com>
Date: Thu, 19 Jul 2018 16:00:41 -0400
Subject: [PATCH] fix(read-preference): correct server sort for `nearest`
 selection

The server sorting during `nearest` server selection was incorrectly
using a greater-than sign, instead of following the rules of array
sort comparitors. This fixes the issue for `pickNearest` as well
as `pickNearestMaxStalenessSeconds`, and contributes tests for
both paths.

NODE-1577
---
 lib/topologies/replset_state.js               | 10 +---
 .../unit/replset/read_preference_tests.js     | 47 ++++++++++++++++++-
 2 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/lib/topologies/replset_state.js b/lib/topologies/replset_state.js
index 6ec3243b7..7ca5cb30a 100644
--- a/lib/topologies/replset_state.js
+++ b/lib/topologies/replset_state.js
@@ -878,10 +878,6 @@ function pickNearestMaxStalenessSeconds(self, readPreference) {
   // Filter by tags
   servers = filterByTags(readPreference, servers);
 
-  //
-  // Locate lowest time (picked servers are lowest time + acceptable Latency margin)
-  // var lowest = servers.length > 0 ? servers[0].lastIsMasterMS : 0;
-
   // Filter by latency
   servers = servers.filter(function(s) {
     return s.staleness <= maxStalenessMS;
@@ -889,8 +885,7 @@ function pickNearestMaxStalenessSeconds(self, readPreference) {
 
   // Sort by time
   servers.sort(function(a, b) {
-    // return a.time > b.time;
-    return a.lastIsMasterMS > b.lastIsMasterMS;
+    return a.lastIsMasterMS - b.lastIsMasterMS;
   });
 
   // No servers, default to primary
@@ -937,8 +932,7 @@ function pickNearest(self, readPreference) {
 
   // Sort by time
   servers.sort(function(a, b) {
-    // return a.time > b.time;
-    return a.lastIsMasterMS > b.lastIsMasterMS;
+    return a.lastIsMasterMS - b.lastIsMasterMS;
   });
 
   // Locate lowest time (picked servers are lowest time + acceptable Latency margin)
diff --git a/test/tests/unit/replset/read_preference_tests.js b/test/tests/unit/replset/read_preference_tests.js
index 00329eb87..d0b46be36 100644
--- a/test/tests/unit/replset/read_preference_tests.js
+++ b/test/tests/unit/replset/read_preference_tests.js
@@ -5,8 +5,10 @@ const ReplSet = require('../../../../lib/topologies/replset');
 const ReadPreference = require('../../../../lib/topologies/read_preference');
 const mock = require('mongodb-mock-server');
 const ReplSetFixture = require('../common').ReplSetFixture;
+const ReplSetState = require('../../../../lib/topologies/replset_state');
+const MongoError = require('../../../..').MongoError;
 
-describe('Secondaries (ReplSet)', function() {
+describe('ReadPreference (ReplSet)', function() {
   let test;
   before(() => (test = new ReplSetFixture()));
   afterEach(() => mock.cleanup());
@@ -48,4 +50,47 @@ describe('Secondaries (ReplSet)', function() {
       replSet.connect({ readPreference: new ReadPreference('secondary') });
     }
   });
+
+  it('should correctly sort servers by `lastIsMasterMS` during nearest selection', function() {
+    const state = new ReplSetState();
+    const sampleData = [
+      { type: 'RSPrimary', lastIsMasterMS: 5 },
+      { type: 'RSSecondary', lastIsMasterMS: 4 },
+      { type: 'RSSecondary', lastIsMasterMS: 4 },
+      { type: 'RSSecondary', lastIsMasterMS: 109 },
+      { type: 'RSSecondary', lastIsMasterMS: 110 },
+      { type: 'RSSecondary', lastIsMasterMS: 110 },
+      { type: 'RSSecondary', lastIsMasterMS: 245 },
+      { type: 'RSSecondary', lastIsMasterMS: 221 },
+      { type: 'RSSecondary', lastIsMasterMS: 199 },
+      { type: 'RSSecondary', lastIsMasterMS: 129 },
+      { type: 'RSSecondary', lastIsMasterMS: 131 },
+      { type: 'RSSecondary', lastIsMasterMS: 284 },
+      { type: 'RSSecondary', lastIsMasterMS: 298 },
+      { type: 'RSSecondary', lastIsMasterMS: 289 },
+      { type: 'RSSecondary', lastIsMasterMS: 312 }
+    ];
+
+    // load mock data into replset state
+    sampleData.forEach(desc => {
+      desc.ismaster = { maxWireVersion: 6 }; // for maxStalenessSeconds test
+      desc.staleness = Math.floor(Math.random() * 100);
+
+      if (desc.type === 'RSPrimary') {
+        state.primary = desc;
+      } else {
+        state.secondaries.push(desc);
+      }
+    });
+
+    // select the nearest server without max staleness seconds
+    let server = state.pickServer(ReadPreference.nearest);
+    expect(server).to.not.be.an.instanceOf(MongoError);
+    expect(server.lastIsMasterMS).to.equal(4);
+
+    // select the nearest server with max staleness seconds
+    server = state.pickServer(new ReadPreference('nearest', { maxStalenessSeconds: 100 }));
+    expect(server).to.not.be.an.instanceOf(MongoError);
+    expect(server.lastIsMasterMS).to.equal(4);
+  });
 });