Skip to content

Commit

Permalink
fix(topology): correct logic for checking for sessions support
Browse files Browse the repository at this point in the history
The logic for checking for sessions support was flawed, in that if
it was connect to a single server it would check the conditions for
single server session support and, failing that, would back off to
the generalized support check. These cases have been split up, and
unit tests were added.

Additionally, a typeo was fixed when determining if a topology had
known servers.

NODE-2342
  • Loading branch information
mbroadst committed Nov 21, 2019
1 parent 6801d21 commit 8d157c8
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 5 deletions.
9 changes: 5 additions & 4 deletions lib/core/sdam/topology.js
Original file line number Diff line number Diff line change
Expand Up @@ -448,10 +448,11 @@ class Topology extends EventEmitter {
* @return Whether the topology should initiate selection to determine session support
*/
shouldCheckForSessionSupport() {
return (
(this.description.type === TopologyType.Single && !this.description.hasKnownServers) ||
!this.description.hasDataBearingServers
);
if (this.description.type === TopologyType.Single) {
return !this.description.hasKnownServers;
}

return !this.description.hasDataBearingServers;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion lib/core/sdam/topology_description.js
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ class TopologyDescription {
* Determines if the topology description has any known servers
*/
get hasKnownServers() {
return Array.from(this.servers.values()).some(sd => sd.type !== ServerDescription.Unknown);
return Array.from(this.servers.values()).some(sd => sd.type !== ServerType.Unknown);
}

/**
Expand Down
73 changes: 73 additions & 0 deletions test/unit/sdam/topology_tests.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
'use strict';
const Topology = require('../../../lib/core/sdam/topology').Topology;
const Server = require('../../../lib/core/sdam/server').Server;
const ServerDescription = require('../../../lib/core/sdam/server_description').ServerDescription;
const expect = require('chai').expect;
const sinon = require('sinon');

describe('Topology (unit)', function() {
describe('shouldCheckForSessionSupport', function() {
beforeEach(function() {
this.sinon = sinon.sandbox.create();

// these are mocks we want across all tests
this.sinon.stub(Server.prototype, 'monitor');
this.sinon
.stub(Topology.prototype, 'selectServer')
.callsFake(function(selector, options, callback) {
const server = Array.from(this.s.servers.values())[0];
callback(null, server);
});
});

afterEach(function() {
this.sinon.restore();
});

it('should check for sessions if connected to a single server and has no known servers', function(done) {
const topology = new Topology('someserver:27019');
this.sinon.stub(Server.prototype, 'connect').callsFake(function() {
this.emit('connect');
});

topology.connect(() => {
expect(topology.shouldCheckForSessionSupport()).to.be.true;
topology.close(done);
});
});

it('should not check for sessions if connected to a single server', function(done) {
const topology = new Topology('someserver:27019');
this.sinon.stub(Server.prototype, 'connect').callsFake(function() {
this.emit(
'descriptionReceived',
new ServerDescription('someserver:27019', { ok: 1, maxWireVersion: 5 })
);

this.emit('connect');
});

topology.connect(() => {
expect(topology.shouldCheckForSessionSupport()).to.be.false;
topology.close(done);
});
});

it('should check for sessions if there are no data-bearing nodes', function(done) {
const topology = new Topology('mongos:27019,mongos:27018,mongos:27017');
this.sinon.stub(Server.prototype, 'connect').callsFake(function() {
this.emit(
'descriptionReceived',
new ServerDescription(this.name, { ok: 1, msg: 'isdbgrid', maxWireVersion: 5 })
);

this.emit('connect');
});

topology.connect(() => {
expect(topology.shouldCheckForSessionSupport()).to.be.false;
topology.close(done);
});
});
});
});

0 comments on commit 8d157c8

Please sign in to comment.