From 7b45be3b1dc556af6d390de4f5d27a3cbc79dc69 Mon Sep 17 00:00:00 2001 From: Daria Pardue <81593090+dariakp@users.noreply.github.com> Date: Tue, 27 Jul 2021 17:31:49 -0400 Subject: [PATCH 1/5] feat(NODE-2843): implement sessions advanceClusterTime method --- src/sdam/common.ts | 14 +++++++------- src/sdam/topology.ts | 4 ++-- src/sessions.ts | 31 +++++++++++++++++++++++++++++-- 3 files changed, 38 insertions(+), 11 deletions(-) diff --git a/src/sdam/common.ts b/src/sdam/common.ts index 80ff92fa13..d3ae9e620d 100644 --- a/src/sdam/common.ts +++ b/src/sdam/common.ts @@ -66,16 +66,16 @@ export interface ClusterTime { }; } -/** Shared function to determine clusterTime for a given topology */ -export function resolveClusterTime( - topology: Topology | ClientSession, +/** Shared function to determine clusterTime for a given topology or session */ +export function _advanceClusterTime( + entity: Topology | ClientSession, $clusterTime: ClusterTime ): void { - if (topology.clusterTime == null) { - topology.clusterTime = $clusterTime; + if (entity.clusterTime == null) { + entity.clusterTime = $clusterTime; } else { - if ($clusterTime.clusterTime.greaterThan(topology.clusterTime.clusterTime)) { - topology.clusterTime = $clusterTime; + if ($clusterTime.clusterTime.greaterThan(entity.clusterTime.clusterTime)) { + entity.clusterTime = $clusterTime; } } } diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 756d57d21b..602e206495 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -28,7 +28,7 @@ import { ServerType, ClusterTime, TimerQueue, - resolveClusterTime, + _advanceClusterTime, drainTimerQueue, clearAndRemoveTimerFrom, STATE_CLOSED, @@ -681,7 +681,7 @@ export class Topology extends TypedEventEmitter { // value of the clusterTime embedded field." const clusterTime = serverDescription.$clusterTime; if (clusterTime) { - resolveClusterTime(this, clusterTime); + _advanceClusterTime(this, clusterTime); } // If we already know all the information contained in this updated description, then diff --git a/src/sessions.ts b/src/sessions.ts index 08eba0ec4d..831aeb6b8d 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -2,7 +2,7 @@ import { PromiseProvider } from './promise_provider'; import { Binary, Long, Timestamp, Document } from './bson'; import { ReadPreference } from './read_preference'; import { isTransactionCommand, TxnState, Transaction, TransactionOptions } from './transactions'; -import { resolveClusterTime, ClusterTime } from './sdam/common'; +import { _advanceClusterTime, ClusterTime } from './sdam/common'; import { isSharded } from './cmap/wire_protocol/shared'; import { MongoError, @@ -249,6 +249,33 @@ export class ClientSession extends TypedEventEmitter { } } + /** + * Advances the clusterTime for a ClientSession to the provided clusterTime of another ClientSession + * + * @param clusterTime - the $clusterTime returned by the server from another session in the form of a document containing the `BSON.Timestamp` clusterTime and signature + */ + advanceClusterTime(clusterTime: ClusterTime): void { + if (!clusterTime || typeof clusterTime !== 'object') { + throw new MongoInvalidArgumentError('input cluster time must be an object'); + } + if (!clusterTime.clusterTime || clusterTime.clusterTime._bsontype !== 'Timestamp') { + throw new MongoInvalidArgumentError( + 'input cluster time "clusterTime" property must be a valid BSON Timestamp' + ); + } + if ( + !clusterTime.signature || + clusterTime.signature.hash._bsontype !== 'Binary' || + clusterTime.signature.keyId._bsontype !== 'Long' + ) { + throw new MongoInvalidArgumentError( + 'input cluster time must have a valid "signature" property with BSON Binary hash and BSON Long keyId' + ); + } + + _advanceClusterTime(this, clusterTime); + } + /** * Used to determine if this session equals another * @@ -886,7 +913,7 @@ export function applySession( export function updateSessionFromResponse(session: ClientSession, document: Document): void { if (document.$clusterTime) { - resolveClusterTime(session, document.$clusterTime); + _advanceClusterTime(session, document.$clusterTime); } if (document.operationTime && session && session.supports.causalConsistency) { From 90b0efcd1496318bf6b9eed89015108b6371cc14 Mon Sep 17 00:00:00 2001 From: Daria Pardue <81593090+dariakp@users.noreply.github.com> Date: Wed, 28 Jul 2021 17:01:59 -0400 Subject: [PATCH 2/5] test: add unit/validation tests for advanceClusterTime --- src/sessions.ts | 4 +- test/unit/core/sessions.test.js | 126 ++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 2 deletions(-) diff --git a/src/sessions.ts b/src/sessions.ts index 831aeb6b8d..4b314fdcc2 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -265,8 +265,8 @@ export class ClientSession extends TypedEventEmitter { } if ( !clusterTime.signature || - clusterTime.signature.hash._bsontype !== 'Binary' || - clusterTime.signature.keyId._bsontype !== 'Long' + clusterTime.signature.hash?._bsontype !== 'Binary' || + clusterTime.signature.keyId?._bsontype !== 'Long' ) { throw new MongoInvalidArgumentError( 'input cluster time must have a valid "signature" property with BSON Binary hash and BSON Long keyId' diff --git a/test/unit/core/sessions.test.js b/test/unit/core/sessions.test.js index 2ecc8468fe..4c477533c4 100644 --- a/test/unit/core/sessions.test.js +++ b/test/unit/core/sessions.test.js @@ -1,6 +1,7 @@ 'use strict'; const mock = require('../../tools/mock'); +const BSON = require('bson'); const { expect } = require('chai'); const { genClusterTime, sessionCleanupHandler } = require('./common'); const { Topology } = require('../../../src/sdam/topology'); @@ -8,6 +9,14 @@ const { ServerSessionPool, ServerSession, ClientSession } = require('../../../sr const { now } = require('../../../src/utils'); let test = {}; + +const generateClusterTime = time => { + return { + clusterTime: new BSON.Timestamp(time), + signature: { hash: new BSON.Binary('test'), keyId: new BSON.Long(1) } + }; +}; + describe('Sessions - unit/core', function () { describe('ClientSession', function () { let session; @@ -84,6 +93,123 @@ describe('Sessions - unit/core', function () { } }); }); + + describe('advanceClusterTime()', () => { + // TODO: add functional tests to make sure: + // 1) using the method to update session clusterTime results in usable session + // 2) using the method to update session to invalid time results in unusable session but still usable client + beforeEach(() => { + const client = new Topology('localhost:27017', {}); + sessionPool = client.s.sessionPool; + session = new ClientSession(client, sessionPool, {}); + }); + it('should throw an error if the input cluster time is not an object', { + metadata: { requires: { topology: 'single' } }, + test: function () { + const invalidInputs = [undefined, null, 3, 'a']; + for (const input of invalidInputs) { + expect(() => session.advanceClusterTime(input)).to.throw( + 'input cluster time must be an object' + ); + } + } + }); + + it( + 'should throw an error if the input cluster time is missing a valid clusterTime property', + { + metadata: { requires: { topology: 'single' } }, + test: function () { + const invalidInputs = Array(5) + .fill(1) + .map(time => generateClusterTime(time)); + + delete invalidInputs[0].clusterTime; + invalidInputs[1].clusterTime = null; + invalidInputs[2].clusterTime = 5; + invalidInputs[3].clusterTime = 'not a timestamp'; + invalidInputs[4].clusterTime = new Date('1'); + + for (const input of invalidInputs) { + expect( + () => session.advanceClusterTime(input), + `expected to fail on input: ${JSON.stringify(input)}` + ).to.throw( + 'input cluster time "clusterTime" property must be a valid BSON Timestamp' + ); + } + } + } + ); + + it('should throw an error if the input cluster time is missing a valid signature property', { + metadata: { requires: { topology: 'single' } }, + test: function () { + const invalidInputs = Array(9) + .fill(1) + .map(time => generateClusterTime(time)); + + // null types + delete invalidInputs[0].signature; + delete invalidInputs[1].signature.hash; + delete invalidInputs[2].signature.keyId; + invalidInputs[3].signature.hash = null; + invalidInputs[4].signature.keyId = null; + // invalid non-null types + invalidInputs[5].signature.keyId = 1; + invalidInputs[6].signature.keyId = 'not BSON Long'; + invalidInputs[7].signature.hash = 123; + invalidInputs[8].signature.hash = 'not BSON Binary'; + + for (const input of invalidInputs) { + expect( + () => session.advanceClusterTime(input), + `expected to fail on input: ${JSON.stringify(input)}` + ).to.throw( + 'input cluster time must have a valid "signature" property with BSON Binary hash and BSON Long keyId' + ); + } + } + }); + + it('should set the session clusterTime to the one provided if the existing session clusterTime is null', () => { + expect(session).property('clusterTime').to.be.undefined; + const validTime = generateClusterTime(100); + session.advanceClusterTime(validTime); + expect(session).property('clusterTime').to.equal(validTime); + + session.clusterTime = null; + expect(session).property('clusterTime').to.be.null; + session.advanceClusterTime(validTime); + expect(session).property('clusterTime').to.equal(validTime); + }); + + it('should set the session clusterTime to the one provided if it is greater than the the existing session clusterTime', () => { + const validInitialTime = generateClusterTime(100); + const validGreaterTime = generateClusterTime(200); + + session.advanceClusterTime(validInitialTime); + expect(session).property('clusterTime').to.equal(validInitialTime); + + session.advanceClusterTime(validGreaterTime); + expect(session).property('clusterTime').to.equal(validGreaterTime); + }); + + it('should leave the session clusterTime unchanged if it is less than or equal to the the existing session clusterTime', () => { + const validInitialTime = generateClusterTime(100); + const validEqualTime = generateClusterTime(100); + const validLesserTime = generateClusterTime(50); + + session.advanceClusterTime(validInitialTime); + expect(session).property('clusterTime').to.equal(validInitialTime); + + session.advanceClusterTime(validEqualTime); + expect(session).property('clusterTime').to.equal(validInitialTime); // the reference check ensures no update happened + + session.advanceClusterTime(validLesserTime); + expect(session).property('clusterTime').to.equal(validInitialTime); + }); + }); }); describe('ServerSessionPool', function () { From 3f9f27b226d3ec9a5dfcfc2c6e4174a0ac5f2f3b Mon Sep 17 00:00:00 2001 From: Daria Pardue <81593090+dariakp@users.noreply.github.com> Date: Thu, 29 Jul 2021 16:21:42 -0400 Subject: [PATCH 3/5] test: require bson from src Co-authored-by: Neal Beeken --- test/unit/core/sessions.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/core/sessions.test.js b/test/unit/core/sessions.test.js index 4c477533c4..ff448facc9 100644 --- a/test/unit/core/sessions.test.js +++ b/test/unit/core/sessions.test.js @@ -1,7 +1,7 @@ 'use strict'; const mock = require('../../tools/mock'); -const BSON = require('bson'); +const BSON = require('../../../src/bson'); const { expect } = require('chai'); const { genClusterTime, sessionCleanupHandler } = require('./common'); const { Topology } = require('../../../src/sdam/topology'); From ff3a2b043d656294c9a02ac5385523d1b7995063 Mon Sep 17 00:00:00 2001 From: Daria Pardue <81593090+dariakp@users.noreply.github.com> Date: Fri, 30 Jul 2021 12:49:00 -0400 Subject: [PATCH 4/5] test: add functional tests for advanceClusterTime --- src/sessions.ts | 3 +- test/functional/sessions.test.js | 509 ++++++++++++++++++++----------- 2 files changed, 332 insertions(+), 180 deletions(-) diff --git a/src/sessions.ts b/src/sessions.ts index 4b314fdcc2..0d7d1002b1 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -266,7 +266,8 @@ export class ClientSession extends TypedEventEmitter { if ( !clusterTime.signature || clusterTime.signature.hash?._bsontype !== 'Binary' || - clusterTime.signature.keyId?._bsontype !== 'Long' + (typeof clusterTime.signature.keyId !== 'number' && + clusterTime.signature.keyId?._bsontype !== 'Long') // apparently we decode the key to number? ) { throw new MongoInvalidArgumentError( 'input cluster time must have a valid "signature" property with BSON Binary hash and BSON Long keyId' diff --git a/test/functional/sessions.test.js b/test/functional/sessions.test.js index 5be0843d3c..abb6d2a243 100644 --- a/test/functional/sessions.test.js +++ b/test/functional/sessions.test.js @@ -2,6 +2,8 @@ const path = require('path'); const expect = require('chai').expect; +const BSON = require('../../src/bson'); +const { MongoServerError } = require('../../src'); const { setupDatabase, withMonitoredClient } = require('./shared'); const { TestRunnerContext, generateTopologyTests } = require('./spec-runner'); const { loadSpecTests } = require('../spec'); @@ -30,93 +32,114 @@ const test = { } }; -describe('Sessions - functional', function () { - before(function () { - return setupDatabase(this.configuration); - }); - - describe('endSessions', function () { - beforeEach(function () { - return test.setup(this.configuration); - }); - - it('should send endSessions for multiple sessions', { - metadata: { - requires: { topology: ['single'], mongodb: '>=3.6.0' }, - // Skipping session leak tests b/c these are explicit sessions - sessions: { skipLeakTests: true } - }, - test: function (done) { - const client = test.client; - let sessions = [client.startSession(), client.startSession()].map(s => s.id); - - client.close(err => { - expect(err).to.not.exist; - expect(test.commands.started).to.have.length(1); - expect(test.commands.started[0].commandName).to.equal('endSessions'); - expect(test.commands.started[0].command.endSessions).to.include.deep.members(sessions); - expect(client.s.sessions.size).to.equal(0); - - done(); - }); - } +describe('Sessions', function () { + describe('Sessions - functional - old format', function () { + before(function () { + return setupDatabase(this.configuration); }); - }); - describe('withSession', { - metadata: { requires: { mongodb: '>=3.6.0' } }, - test: function () { + describe('endSessions', function () { beforeEach(function () { return test.setup(this.configuration); }); - [ - { - description: 'should support operations that return promises', - operation: client => session => { - return client.db('test').collection('foo').find({}, { session }).toArray(); - } - }, - // { - // nodeVersion: '>=8.x', - // description: 'should support async operations', - // operation: client => session => - // async function() { - // await client - // .db('test') - // .collection('foo') - // .find({}, { session }) - // .toArray(); - // } - // }, - { - description: 'should support operations that return rejected promises', - operation: (/* client */) => (/* session */) => { - return Promise.reject(new Error('something awful')); - } + it('should send endSessions for multiple sessions', { + metadata: { + requires: { topology: ['single'], mongodb: '>=3.6.0' }, + // Skipping session leak tests b/c these are explicit sessions + sessions: { skipLeakTests: true } }, - { - description: "should support operations that don't return promises", - operation: (/* client */) => (/* session */) => { - setTimeout(() => {}); - } - }, - { - description: 'should support operations that throw exceptions', - operation: (/* client */) => (/* session */) => { - throw new Error('something went wrong!'); - } + test: function (done) { + const client = test.client; + let sessions = [client.startSession(), client.startSession()].map(s => s.id); + + client.close(err => { + expect(err).to.not.exist; + expect(test.commands.started).to.have.length(1); + expect(test.commands.started[0].commandName).to.equal('endSessions'); + expect(test.commands.started[0].command.endSessions).to.include.deep.members(sessions); + expect(client.s.sessions.size).to.equal(0); + + done(); + }); } - ].forEach(testCase => { - it(testCase.description, function () { + }); + }); + + describe('withSession', { + metadata: { requires: { mongodb: '>=3.6.0' } }, + test: function () { + beforeEach(function () { + return test.setup(this.configuration); + }); + + [ + { + description: 'should support operations that return promises', + operation: client => session => { + return client.db('test').collection('foo').find({}, { session }).toArray(); + } + }, + // { + // nodeVersion: '>=8.x', + // description: 'should support async operations', + // operation: client => session => + // async function() { + // await client + // .db('test') + // .collection('foo') + // .find({}, { session }) + // .toArray(); + // } + // }, + { + description: 'should support operations that return rejected promises', + operation: (/* client */) => (/* session */) => { + return Promise.reject(new Error('something awful')); + } + }, + { + description: "should support operations that don't return promises", + operation: (/* client */) => (/* session */) => { + setTimeout(() => {}); + } + }, + { + description: 'should support operations that throw exceptions', + operation: (/* client */) => (/* session */) => { + throw new Error('something went wrong!'); + } + } + ].forEach(testCase => { + it(testCase.description, function () { + const client = test.client; + + return client + .withSession(testCase.operation(client)) + .then( + () => expect(client.topology.s.sessionPool.sessions).to.have.length(1), + () => expect(client.topology.s.sessionPool.sessions).to.have.length(1) + ) + .then(() => client.close()) + .then(() => { + // verify that the `endSessions` command was sent + const lastCommand = test.commands.started[test.commands.started.length - 1]; + expect(lastCommand.commandName).to.equal('endSessions'); + expect(client.topology).to.not.exist; + }); + }); + }); + + it('supports passing options to ClientSession', function () { const client = test.client; - return client - .withSession(testCase.operation(client)) - .then( - () => expect(client.topology.s.sessionPool.sessions).to.have.length(1), - () => expect(client.topology.s.sessionPool.sessions).to.have.length(1) - ) + const promise = client.withSession({ causalConsistency: false }, session => { + expect(session.supports.causalConsistency).to.be.false; + return client.db('test').collection('foo').find({}, { session }).toArray(); + }); + + return promise + .then(() => expect(client.topology.s.sessionPool.sessions).to.have.length(1)) .then(() => client.close()) .then(() => { // verify that the `endSessions` command was sent @@ -125,130 +148,258 @@ describe('Sessions - functional', function () { expect(client.topology).to.not.exist; }); }); + } + }); + + describe('legacy spec tests', function () { + class SessionSpecTestContext extends TestRunnerContext { + assertSessionNotDirty(options) { + const session = options.session; + expect(session.serverSession.isDirty).to.be.false; + } + + assertSessionDirty(options) { + const session = options.session; + expect(session.serverSession.isDirty).to.be.true; + } + + assertSameLsidOnLastTwoCommands() { + expect(this.commandEvents).to.have.length.of.at.least(2); + const lastTwoCommands = this.commandEvents.slice(-2).map(c => c.command); + lastTwoCommands.forEach(command => expect(command).to.have.property('lsid')); + expect(lastTwoCommands[0].lsid).to.eql(lastTwoCommands[1].lsid); + } + + assertDifferentLsidOnLastTwoCommands() { + expect(this.commandEvents).to.have.length.of.at.least(2); + const lastTwoCommands = this.commandEvents.slice(-2).map(c => c.command); + lastTwoCommands.forEach(command => expect(command).to.have.property('lsid')); + expect(lastTwoCommands[0].lsid).to.not.eql(lastTwoCommands[1].lsid); + } + } + + const testContext = new SessionSpecTestContext(); + const testSuites = loadSpecTests(path.join('sessions', 'legacy')); + + after(() => testContext.teardown()); + before(function () { + return testContext.setup(this.configuration); }); - it('supports passing options to ClientSession', function () { - const client = test.client; + function testFilter(spec) { + const SKIP_TESTS = [ + // These two tests need to run against multiple mongoses + 'Dirty explicit session is discarded', + 'Dirty implicit session is discarded (write)' + ]; + + return SKIP_TESTS.indexOf(spec.description) === -1; + } - const promise = client.withSession({ causalConsistency: false }, session => { - expect(session.supports.causalConsistency).to.be.false; - return client.db('test').collection('foo').find({}, { session }).toArray(); + generateTopologyTests(testSuites, testContext, testFilter); + }); + + describe('unified spec tests', function () { + for (const sessionTests of loadSpecTests(path.join('sessions', 'unified'))) { + expect(sessionTests).to.be.an('object'); + context(String(sessionTests.description), function () { + for (const test of sessionTests.tests) { + it(String(test.description), { + metadata: { sessions: { skipLeakTests: true } }, + test: async function () { + await runUnifiedTest(this, sessionTests, test); + } + }); + } }); + } + }); - return promise - .then(() => expect(client.topology.s.sessionPool.sessions).to.have.length(1)) - .then(() => client.close()) - .then(() => { - // verify that the `endSessions` command was sent - const lastCommand = test.commands.started[test.commands.started.length - 1]; - expect(lastCommand.commandName).to.equal('endSessions'); - expect(client.topology).to.not.exist; - }); + context('unacknowledged writes', () => { + it('should not include session for unacknowledged writes', { + metadata: { requires: { topology: 'single', mongodb: '>=3.6.0' } }, + test: withMonitoredClient( + 'insert', + { clientOptions: { writeConcern: { w: 0 } } }, + function (client, events, done) { + client + .db('test') + .collection('foo') + .insertOne({ foo: 'bar' }, err => { + expect(err).to.not.exist; + const event = events[0]; + expect(event).nested.property('command.writeConcern.w').to.equal(0); + expect(event).to.not.have.nested.property('command.lsid'); + done(); + }); + } + ) + }); + it('should throw error with explicit session', { + metadata: { requires: { topology: 'replicaset', mongodb: '>=3.6.0' } }, + test: withMonitoredClient( + 'insert', + { clientOptions: { writeConcern: { w: 0 } } }, + function (client, events, done) { + const session = client.startSession({ causalConsistency: true }); + client + .db('test') + .collection('foo') + .insertOne({ foo: 'bar' }, { session }, err => { + expect(err).to.exist; + expect(err.message).to.equal( + 'Cannot have explicit session with unacknowledged writes' + ); + client.close(done); + }); + } + ) }); - } + }); }); - describe('legacy spec tests', function () { - class SessionSpecTestContext extends TestRunnerContext { - assertSessionNotDirty(options) { - const session = options.session; - expect(session.serverSession.isDirty).to.be.false; - } + describe('Sessions - functional - new format', function () { + let client; + let collection; - assertSessionDirty(options) { - const session = options.session; - expect(session.serverSession.isDirty).to.be.true; - } + beforeEach(async function () { + const config = this.configuration; + client = config.newClient(); + await client.connect(); - assertSameLsidOnLastTwoCommands() { - expect(this.commandEvents).to.have.length.of.at.least(2); - const lastTwoCommands = this.commandEvents.slice(-2).map(c => c.command); - lastTwoCommands.forEach(command => expect(command).to.have.property('lsid')); - expect(lastTwoCommands[0].lsid).to.eql(lastTwoCommands[1].lsid); + try { + await client + .db('sessions_functional_test_db') + .collection('sessions_functional_test') + .drop(); + } catch (_) { + // do not care } - assertDifferentLsidOnLastTwoCommands() { - expect(this.commandEvents).to.have.length.of.at.least(2); - const lastTwoCommands = this.commandEvents.slice(-2).map(c => c.command); - lastTwoCommands.forEach(command => expect(command).to.have.property('lsid')); - expect(lastTwoCommands[0].lsid).to.not.eql(lastTwoCommands[1].lsid); - } - } + collection = await client + .db('sessions_functional_test_db') + .createCollection('sessions_functional_test'); - const testContext = new SessionSpecTestContext(); - const testSuites = loadSpecTests(path.join('sessions', 'legacy')); + await collection.deleteMany({}); + }); - after(() => testContext.teardown()); - before(function () { - return testContext.setup(this.configuration); + afterEach(async () => { + if (client) await client.close(); }); - function testFilter(spec) { - const SKIP_TESTS = [ - // These two tests need to run against multiple mongoses - 'Dirty explicit session is discarded', - 'Dirty implicit session is discarded (write)' - ]; + describe('advanceClusterTime()', () => { + let controlSession; + let testSession; + let otherSession; - return SKIP_TESTS.indexOf(spec.description) === -1; - } + beforeEach(async () => { + testSession = client.startSession(); + otherSession = client.startSession(); + controlSession = client.startSession(); - generateTopologyTests(testSuites, testContext, testFilter); - }); + // set up sessions with two sets of cluster times + expect(await collection.findOne({}, { session: controlSession })).to.be.undefined; + expect(await collection.findOne({}, { session: testSession })).to.be.undefined; + await collection.insertOne({ apple: 'green' }); + expect(await collection.findOne({}, { session: otherSession })) + .property('apple') + .to.equal('green'); + expect(testSession.clusterTime).not.deep.equal(otherSession.clusterTime); + expect(controlSession.clusterTime).not.deep.equal(otherSession.clusterTime); + // it's ok for the control session to have the same starting clusterTime as testSession + // since the testSession is the one that will be updated + }); - describe('unified spec tests', function () { - for (const sessionTests of loadSpecTests(path.join('sessions', 'unified'))) { - expect(sessionTests).to.be.an('object'); - context(String(sessionTests.description), function () { - for (const test of sessionTests.tests) { - it(String(test.description), { - metadata: { sessions: { skipLeakTests: true } }, - test: async function () { - await runUnifiedTest(this, sessionTests, test); - } - }); + afterEach(async () => { + await Promise.all([ + controlSession.endSession(), + testSession.endSession(), + otherSession.endSession() + ]); + }); + + it( + 'should result in a usable session when called with a valid cluster time and should not affect any other sessions', + { + metadata: { requires: { mongodb: '>= 3.6.0', topology: ['replicaset'] } }, + async test() { + // advance cluster time to a new valid value + testSession.advanceClusterTime(otherSession.clusterTime); + expect(testSession.clusterTime).to.deep.equal(otherSession.clusterTime); + + // check control session + expect(controlSession.clusterTime).to.not.deep.equal(testSession.clusterTime); + + // check that the session still works + expect(await collection.findOne({}, { session: testSession })) + .property('apple') + .to.equal('green'); + } + } + ); + + it('should not let an invalid cluster time impact existing sessions', { + metadata: { requires: { mongodb: '>= 3.6.0', topology: ['replicaset'] } }, + async test() { + // note, because of our validation, we can't use advanceClusterTime to set an invalid clusterTime + // so for testing, we have to set it directly + testSession.clusterTime = { clusterTime: { greaterThan: () => true } }; + + try { + await collection.findOne({}, { session: testSession }); + expect.fail('expected findOne to fail, but it passed'); + } catch (err) { + expect(err).to.be.instanceOf(MongoServerError); + } + + expect(await collection.findOne({}, { session: controlSession })) + .property('apple') + .to.equal('green'); } }); - } - }); - context('unacknowledged writes', () => { - it('should not include session for unacknowledged writes', { - metadata: { requires: { topology: 'single', mongodb: '>=3.6.0' } }, - test: withMonitoredClient('insert', { clientOptions: { writeConcern: { w: 0 } } }, function ( - client, - events, - done - ) { - client - .db('test') - .collection('foo') - .insertOne({ foo: 'bar' }, err => { - expect(err).to.not.exist; - const event = events[0]; - expect(event).nested.property('command.writeConcern.w').to.equal(0); - expect(event).to.not.have.nested.property('command.lsid'); - done(); - }); - }) - }); - it('should throw error with explicit session', { - metadata: { requires: { topology: 'replicaset', mongodb: '>=3.6.0' } }, - test: withMonitoredClient('insert', { clientOptions: { writeConcern: { w: 0 } } }, function ( - client, - events, - done - ) { - const session = client.startSession({ causalConsistency: true }); - client - .db('test') - .collection('foo') - .insertOne({ foo: 'bar' }, { session }, err => { - expect(err).to.exist; - expect(err.message).to.equal('Cannot have explicit session with unacknowledged writes'); - client.close(done); - }); - }) + it('should not let an invalid cluster time impact new sessions', { + metadata: { requires: { mongodb: '>= 3.6.0', topology: ['replicaset'] } }, + async test() { + // note, because of our validation, we can't use advanceClusterTime to set an invalid clusterTime + // so for testing, we have to set it directly + testSession.clusterTime = { clusterTime: { greaterThan: () => true } }; + + try { + await collection.findOne({}, { session: testSession }); + expect.fail('expected findOne to fail, but it passed'); + } catch (err) { + expect(err).to.be.instanceOf(MongoServerError); + } + + await otherSession.endSession(); + otherSession = client.startSession(); + + expect(await collection.findOne({}, { session: otherSession })) + .property('apple') + .to.equal('green'); + } + }); + + it('should not let an invalid cluster time impact other uses of the client', { + metadata: { requires: { mongodb: '>= 3.6.0', topology: ['replicaset'] } }, + async test() { + // note, because of our validation, we can't use advanceClusterTime to set an invalid clusterTime + // so for testing, we have to set it directly + testSession.clusterTime = { clusterTime: { greaterThan: () => true } }; + + try { + await collection.findOne({}, { session: testSession }); + expect.fail('expected findOne to fail, but it passed'); + } catch (err) { + expect(err).to.be.instanceOf(MongoServerError); + } + + expect(await collection.findOne({})) + .property('apple') + .to.equal('green'); + } + }); }); }); }); From 5cfcc9851f9fd837119a76e70f156b35e8824383 Mon Sep 17 00:00:00 2001 From: Daria Pardue <81593090+dariakp@users.noreply.github.com> Date: Fri, 30 Jul 2021 15:20:16 -0400 Subject: [PATCH 5/5] test: clean up session tests --- test/functional/sessions.test.js | 1 - test/unit/core/sessions.test.js | 16 ++++++++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/test/functional/sessions.test.js b/test/functional/sessions.test.js index abb6d2a243..1a16c018cf 100644 --- a/test/functional/sessions.test.js +++ b/test/functional/sessions.test.js @@ -2,7 +2,6 @@ const path = require('path'); const expect = require('chai').expect; -const BSON = require('../../src/bson'); const { MongoServerError } = require('../../src'); const { setupDatabase, withMonitoredClient } = require('./shared'); const { TestRunnerContext, generateTopologyTests } = require('./spec-runner'); diff --git a/test/unit/core/sessions.test.js b/test/unit/core/sessions.test.js index ff448facc9..1c45dd2674 100644 --- a/test/unit/core/sessions.test.js +++ b/test/unit/core/sessions.test.js @@ -95,14 +95,12 @@ describe('Sessions - unit/core', function () { }); describe('advanceClusterTime()', () => { - // TODO: add functional tests to make sure: - // 1) using the method to update session clusterTime results in usable session - // 2) using the method to update session to invalid time results in unusable session but still usable client beforeEach(() => { const client = new Topology('localhost:27017', {}); sessionPool = client.s.sessionPool; session = new ClientSession(client, sessionPool, {}); }); + it('should throw an error if the input cluster time is not an object', { metadata: { requires: { topology: 'single' } }, test: function () { @@ -156,7 +154,9 @@ describe('Sessions - unit/core', function () { invalidInputs[3].signature.hash = null; invalidInputs[4].signature.keyId = null; // invalid non-null types - invalidInputs[5].signature.keyId = 1; + // keyId must be number or BSON long + // hash must be BSON binary + invalidInputs[5].signature.keyId = {}; invalidInputs[6].signature.keyId = 'not BSON Long'; invalidInputs[7].signature.hash = 123; invalidInputs[8].signature.hash = 'not BSON Binary'; @@ -182,6 +182,14 @@ describe('Sessions - unit/core', function () { expect(session).property('clusterTime').to.be.null; session.advanceClusterTime(validTime); expect(session).property('clusterTime').to.equal(validTime); + + // extra test case for valid alternative keyId type in signature + const alsoValidTime = generateClusterTime(200); + alsoValidTime.signature.keyId = 10; + session.clusterTime = null; + expect(session).property('clusterTime').to.be.null; + session.advanceClusterTime(alsoValidTime); + expect(session).property('clusterTime').to.equal(alsoValidTime); }); it('should set the session clusterTime to the one provided if it is greater than the the existing session clusterTime', () => {