From 2988803a2fc5897047b5f7e75f1749631353ce3f Mon Sep 17 00:00:00 2001 From: Daria Pardue <81593090+dariakp@users.noreply.github.com> Date: Wed, 14 Jul 2021 17:20:27 -0400 Subject: [PATCH 1/5] test(NODE-3393): update runner to support sessions on most operations --- test/functional/sessions.test.js | 34 +++------ .../unified-spec-runner/operations.ts | 74 ++++++------------- 2 files changed, 33 insertions(+), 75 deletions(-) diff --git a/test/functional/sessions.test.js b/test/functional/sessions.test.js index f0e0fb5cb7..e5d02d9e20 100644 --- a/test/functional/sessions.test.js +++ b/test/functional/sessions.test.js @@ -201,34 +201,20 @@ describe('Sessions - functional', function () { expect(sessionTests).to.be.an('object'); context(String(sessionTests.description), function () { // TODO: NODE-3393 fix test runner to apply session to all operations - const skipTestMap = { - 'snapshot-sessions': [ - 'countDocuments operation with snapshot', - 'Distinct operation with snapshot', - 'Mixed operation with snapshot' - ], - 'snapshot-sessions-not-supported-client-error': [ - 'Client error on distinct with snapshot' - ], - 'snapshot-sessions-not-supported-server-error': [ - 'Server returns an error on distinct with snapshot' - ], - 'snapshot-sessions-unsupported-ops': [ - 'Server returns an error on listCollections with snapshot', - 'Server returns an error on listDatabases with snapshot', - 'Server returns an error on listIndexes with snapshot', - 'Server returns an error on runCommand with snapshot', - 'Server returns an error on findOneAndUpdate with snapshot', - 'Server returns an error on deleteOne with snapshot', - 'Server returns an error on updateOne with snapshot' - ] - }; - const testsToSkip = skipTestMap[sessionTests.description] || []; + // const skipTestMap = { + // 'snapshot-sessions': [ + // 'countDocuments operation with snapshot', + // 'Distinct operation with snapshot', + // 'Mixed operation with snapshot' + // ], + // 'snapshot-sessions-not-supported-client-error': ['Client error on distinct with snapshot'] + // }; + // const testsToSkip = skipTestMap[sessionTests.description] || []; for (const test of sessionTests.tests) { it(String(test.description), { metadata: { sessions: { skipLeakTests: true } }, test: async function () { - await runUnifiedTest(this, sessionTests, test, testsToSkip); + await runUnifiedTest(this, sessionTests, test, []); } }); } diff --git a/test/functional/unified-spec-runner/operations.ts b/test/functional/unified-spec-runner/operations.ts index 40aa0f864a..e502a50337 100644 --- a/test/functional/unified-spec-runner/operations.ts +++ b/test/functional/unified-spec-runner/operations.ts @@ -22,18 +22,6 @@ interface OperationFunctionParams { type RunOperationFn = (p: OperationFunctionParams) => Promise; export const operations = new Map(); -function executeWithPotentialSession( - entities: EntitiesMap, - operation: OperationDescription, - cursor: AbstractCursor -) { - const session = entities.getEntity('session', operation.arguments.session, false); - if (session) { - cursor.session = session; - } - return cursor.toArray(); -} - operations.set('abortTransaction', async ({ entities, operation }) => { const session = entities.getEntity('session', operation.object); return session.abortTransaction(); @@ -44,18 +32,9 @@ operations.set('aggregate', async ({ entities, operation }) => { if (!(dbOrCollection instanceof Db || dbOrCollection instanceof Collection)) { throw new Error(`Operation object '${operation.object}' must be a db or collection`); } - const cursor = dbOrCollection.aggregate(operation.arguments.pipeline, { - allowDiskUse: operation.arguments.allowDiskUse, - batchSize: operation.arguments.batchSize, - bypassDocumentValidation: operation.arguments.bypassDocumentValidation, - maxTimeMS: operation.arguments.maxTimeMS, - maxAwaitTimeMS: operation.arguments.maxAwaitTimeMS, - collation: operation.arguments.collation, - hint: operation.arguments.hint, - let: operation.arguments.let, - out: operation.arguments.out - }); - return executeWithPotentialSession(entities, operation, cursor); + const { pipeline, ...opts } = operation.arguments; + const cursor = dbOrCollection.aggregate(pipeline, opts); + return cursor.toArray(); }); operations.set('assertCollectionExists', async ({ operation, client }) => { @@ -280,9 +259,8 @@ operations.set('endSession', async ({ entities, operation }) => { operations.set('find', async ({ entities, operation }) => { const collection = entities.getEntity('collection', operation.object); - const { filter, sort, batchSize, limit, let: vars } = operation.arguments; - const cursor = collection.find(filter, { sort, batchSize, limit, let: vars }); - return executeWithPotentialSession(entities, operation, cursor); + const { filter, ...opts } = operation.arguments; + return collection.find(filter, opts).toArray(); }); operations.set('findOneAndReplace', async ({ entities, operation }) => { @@ -304,27 +282,16 @@ operations.set('failPoint', async ({ entities, operation }) => { operations.set('insertOne', async ({ entities, operation }) => { const collection = entities.getEntity('collection', operation.object); - - const session = entities.getEntity('session', operation.arguments.session, false); - - const options = { - session - } as InsertOneOptions; - - return collection.insertOne(operation.arguments.document, options); + const { document, ...opts } = operation.arguments; + return collection.insertOne(document, opts); }); operations.set('insertMany', async ({ entities, operation }) => { const collection = entities.getEntity('collection', operation.object); - - const session = entities.getEntity('session', operation.arguments.session, false); - - const options = { - session, - ordered: operation.arguments.ordered ?? true - }; - - return collection.insertMany(operation.arguments.documents, options); + // TODO: remove comment if all tests pass + // PREVIOUSLY: ordered: operation.arguments.ordered ?? true + const { documents, ...opts } = operation.arguments; + return collection.insertMany(documents, opts); }); operations.set('iterateUntilDocumentOrError', async ({ entities, operation }) => { @@ -349,7 +316,7 @@ operations.set('listCollections', async ({ entities, operation }) => { operations.set('listDatabases', async ({ entities, operation }) => { const client = entities.getEntity('client', operation.object); - return client.db().admin().listDatabases(); + return client.db().admin().listDatabases(operation.arguments); }); operations.set('listIndexes', async ({ entities, operation }) => { @@ -432,7 +399,8 @@ operations.set('withTransaction', async ({ entities, operation, client }) => { operations.set('countDocuments', async ({ entities, operation }) => { const collection = entities.getEntity('collection', operation.object); - return collection.countDocuments(operation.arguments.filter as Document); + const { filter, ...opts } = operation.arguments; + return collection.countDocuments(filter, opts); }); operations.set('deleteMany', async ({ entities, operation }) => { @@ -442,10 +410,8 @@ operations.set('deleteMany', async ({ entities, operation }) => { operations.set('distinct', async ({ entities, operation }) => { const collection = entities.getEntity('collection', operation.object); - return collection.distinct( - operation.arguments.fieldName as string, - operation.arguments.filter as Document - ); + const { fieldName, filter, ...opts } = operation.arguments; + return collection.distinct(fieldName, filter, opts); }); operations.set('estimatedDocumentCount', async ({ entities, operation }) => { @@ -460,7 +426,8 @@ operations.set('findOneAndDelete', async ({ entities, operation }) => { operations.set('runCommand', async ({ entities, operation }: OperationFunctionParams) => { const db = entities.getEntity('db', operation.object); - return db.command(operation.arguments.command); + const { command, ...opts } = operation.arguments; + return db.command(command, opts); }); operations.set('updateMany', async ({ entities, operation }) => { @@ -483,6 +450,11 @@ export async function executeOperationAndCheck( const opFunc = operations.get(operation.name); expect(opFunc, `Unknown operation: ${operation.name}`).to.exist; + if (operation.arguments?.session) { + const session = entities.getEntity('session', operation.arguments.session, false); + operation.arguments.session = session; + } + let result; try { From 5c4b9d00da05f841e69b3439de2b73671640c646 Mon Sep 17 00:00:00 2001 From: Daria Pardue <81593090+dariakp@users.noreply.github.com> Date: Wed, 14 Jul 2021 17:48:41 -0400 Subject: [PATCH 2/5] fix: remove explicit references to sessions in runner operation definitions --- .../unified-spec-runner/operations.ts | 29 +++++++------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/test/functional/unified-spec-runner/operations.ts b/test/functional/unified-spec-runner/operations.ts index e502a50337..0073fbfe2a 100644 --- a/test/functional/unified-spec-runner/operations.ts +++ b/test/functional/unified-spec-runner/operations.ts @@ -4,14 +4,13 @@ import { Collection, Db, GridFSFile, MongoClient, ObjectId, AbstractCursor } fro import { ReadConcern } from '../../../src/read_concern'; import { ReadPreference } from '../../../src/read_preference'; import { WriteConcern } from '../../../src/write_concern'; -import { Document, InsertOneOptions } from '../../../src'; +import { Document } from '../../../src'; import { EventCollector } from '../../tools/utils'; import { EntitiesMap } from './entities'; import { expectErrorCheck, resultCheck } from './match'; import type { OperationDescription } from './schema'; import { CommandStartedEvent } from '../../../src/cmap/command_monitoring_events'; import { translateOptions } from './unified-utils'; -import { getSymbolFrom } from '../../tools/utils'; interface OperationFunctionParams { client: MongoClient; @@ -118,27 +117,27 @@ operations.set('assertSameLsidOnLastTwoCommands', async ({ entities, operation } }); operations.set('assertSessionDirty', async ({ entities, operation }) => { - const session = entities.getEntity('session', operation.arguments.session); + const session = operation.arguments.session; expect(session.serverSession.isDirty).to.be.true; }); operations.set('assertSessionNotDirty', async ({ entities, operation }) => { - const session = entities.getEntity('session', operation.arguments.session); + const session = operation.arguments.session; expect(session.serverSession.isDirty).to.be.false; }); operations.set('assertSessionPinned', async ({ entities, operation }) => { - const session = entities.getEntity('session', operation.arguments.session); + const session = operation.arguments.session; expect(session.transaction.isPinned).to.be.true; }); operations.set('assertSessionUnpinned', async ({ entities, operation }) => { - const session = entities.getEntity('session', operation.arguments.session); + const session = operation.arguments.session; expect(session.transaction.isPinned).to.be.false; }); operations.set('assertSessionTransactionState', async ({ entities, operation }) => { - const session = entities.getEntity('session', operation.arguments.session); + const session = operation.arguments.session; const transactionStateTranslation = { none: 'NO_TRANSACTION', @@ -214,11 +213,8 @@ operations.set('createChangeStream', async ({ entities, operation }) => { operations.set('createCollection', async ({ entities, operation }) => { const db = entities.getEntity('db', operation.object); - const { session, collection, ...opts } = operation.arguments; - await db.createCollection(collection, { - session: entities.getEntity('session', session, false), - ...opts - }); + const { collection, ...opts } = operation.arguments; + await db.createCollection(collection, opts); }); operations.set('createFindCursor', async ({ entities, operation }) => { @@ -234,11 +230,8 @@ operations.set('createFindCursor', async ({ entities, operation }) => { operations.set('createIndex', async ({ entities, operation }) => { const collection = entities.getEntity('collection', operation.object); - const session = entities.getEntity('session', operation.arguments.session, false); - await collection.createIndex(operation.arguments.keys, { - session, - name: operation.arguments.name - }); + const { keys, ...opts } = operation.arguments; + await collection.createIndex(keys, opts); }); operations.set('deleteOne', async ({ entities, operation }) => { @@ -340,7 +333,7 @@ operations.set('startTransaction', async ({ entities, operation }) => { }); operations.set('targetedFailPoint', async ({ entities, operation }) => { - const session = entities.getEntity('session', operation.arguments.session); + const session = operation.arguments.session; expect(session.transaction.isPinned, 'Session must be pinned for a targetedFailPoint').to.be.true; await entities.failPoints.enableFailPoint( session.transaction._pinnedServer.s.description.hostAddress, From fb23b37d3f1ab442cbcbfd7562271015110c5cdf Mon Sep 17 00:00:00 2001 From: Daria Pardue <81593090+dariakp@users.noreply.github.com> Date: Wed, 14 Jul 2021 18:05:20 -0400 Subject: [PATCH 3/5] test: skip the remaining failing snapshot test --- test/functional/sessions.test.js | 15 +++++---------- test/functional/unified-spec-runner/operations.ts | 2 -- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/test/functional/sessions.test.js b/test/functional/sessions.test.js index e5d02d9e20..a823261c57 100644 --- a/test/functional/sessions.test.js +++ b/test/functional/sessions.test.js @@ -201,20 +201,15 @@ describe('Sessions - functional', function () { expect(sessionTests).to.be.an('object'); context(String(sessionTests.description), function () { // TODO: NODE-3393 fix test runner to apply session to all operations - // const skipTestMap = { - // 'snapshot-sessions': [ - // 'countDocuments operation with snapshot', - // 'Distinct operation with snapshot', - // 'Mixed operation with snapshot' - // ], - // 'snapshot-sessions-not-supported-client-error': ['Client error on distinct with snapshot'] - // }; - // const testsToSkip = skipTestMap[sessionTests.description] || []; + const skipTestMap = { + 'snapshot-sessions': ['Distinct operation with snapshot'] + }; + const testsToSkip = skipTestMap[sessionTests.description] || []; for (const test of sessionTests.tests) { it(String(test.description), { metadata: { sessions: { skipLeakTests: true } }, test: async function () { - await runUnifiedTest(this, sessionTests, test, []); + await runUnifiedTest(this, sessionTests, test, testsToSkip); } }); } diff --git a/test/functional/unified-spec-runner/operations.ts b/test/functional/unified-spec-runner/operations.ts index 0073fbfe2a..6b5584403b 100644 --- a/test/functional/unified-spec-runner/operations.ts +++ b/test/functional/unified-spec-runner/operations.ts @@ -281,8 +281,6 @@ operations.set('insertOne', async ({ entities, operation }) => { operations.set('insertMany', async ({ entities, operation }) => { const collection = entities.getEntity('collection', operation.object); - // TODO: remove comment if all tests pass - // PREVIOUSLY: ordered: operation.arguments.ordered ?? true const { documents, ...opts } = operation.arguments; return collection.insertMany(documents, opts); }); From 5774bfe4f46c3d4caade4d2abf5fba171246e72b Mon Sep 17 00:00:00 2001 From: Daria Pardue <81593090+dariakp@users.noreply.github.com> Date: Thu, 15 Jul 2021 13:16:54 -0400 Subject: [PATCH 4/5] fix: snapshot time not applied if distinct executed first --- src/sessions.ts | 13 +++++++------ test/functional/sessions.test.js | 7 +------ 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/sessions.ts b/src/sessions.ts index 2b5324061f..fb59f75952 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -891,11 +891,12 @@ export function updateSessionFromResponse(session: ClientSession, document: Docu session.transaction._recoveryToken = document.recoveryToken; } - if ( - document.cursor?.atClusterTime && - session?.[kSnapshotEnabled] && - session[kSnapshotTime] === undefined - ) { - session[kSnapshotTime] = document.cursor.atClusterTime; + if (session?.[kSnapshotEnabled] && session[kSnapshotTime] === undefined) { + // find and aggregate commands return atClusterTime on the cursor + // distinct includes it in the response body + const atClusterTime = document.cursor?.atClusterTime || document.atClusterTime; + if (atClusterTime) { + session[kSnapshotTime] = atClusterTime; + } } } diff --git a/test/functional/sessions.test.js b/test/functional/sessions.test.js index a823261c57..5be0843d3c 100644 --- a/test/functional/sessions.test.js +++ b/test/functional/sessions.test.js @@ -200,16 +200,11 @@ describe('Sessions - functional', function () { for (const sessionTests of loadSpecTests(path.join('sessions', 'unified'))) { expect(sessionTests).to.be.an('object'); context(String(sessionTests.description), function () { - // TODO: NODE-3393 fix test runner to apply session to all operations - const skipTestMap = { - 'snapshot-sessions': ['Distinct operation with snapshot'] - }; - const testsToSkip = skipTestMap[sessionTests.description] || []; for (const test of sessionTests.tests) { it(String(test.description), { metadata: { sessions: { skipLeakTests: true } }, test: async function () { - await runUnifiedTest(this, sessionTests, test, testsToSkip); + await runUnifiedTest(this, sessionTests, test); } }); } From 0841d1ff213d4e2f05745e61f86753beb8db8b3e Mon Sep 17 00:00:00 2001 From: Daria Pardue <81593090+dariakp@users.noreply.github.com> Date: Thu, 15 Jul 2021 13:33:04 -0400 Subject: [PATCH 5/5] test: update unified collection operations to pass in all options by default --- .../unified-spec-runner/operations.ts | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/test/functional/unified-spec-runner/operations.ts b/test/functional/unified-spec-runner/operations.ts index 6b5584403b..b83bc6366e 100644 --- a/test/functional/unified-spec-runner/operations.ts +++ b/test/functional/unified-spec-runner/operations.ts @@ -219,8 +219,8 @@ operations.set('createCollection', async ({ entities, operation }) => { operations.set('createFindCursor', async ({ entities, operation }) => { const collection = entities.getEntity('collection', operation.object); - const { filter, sort, batchSize, limit, let: vars } = operation.arguments; - const cursor = collection.find(filter, { sort, batchSize, limit, let: vars }); + const { filter, ...opts } = operation.arguments; + const cursor = collection.find(filter, opts); // The spec dictates that we create the cursor and force the find command // to execute, but don't move the cursor forward. hasNext() accomplishes // this. @@ -242,7 +242,8 @@ operations.set('deleteOne', async ({ entities, operation }) => { operations.set('dropCollection', async ({ entities, operation }) => { const db = entities.getEntity('db', operation.object); - return await db.dropCollection(operation.arguments.collection); + const { collection, ...opts } = operation.arguments; + return await db.dropCollection(collection, opts); }); operations.set('endSession', async ({ entities, operation }) => { @@ -317,12 +318,8 @@ operations.set('listIndexes', async ({ entities, operation }) => { operations.set('replaceOne', async ({ entities, operation }) => { const collection = entities.getEntity('collection', operation.object); - return collection.replaceOne(operation.arguments.filter, operation.arguments.replacement, { - bypassDocumentValidation: operation.arguments.bypassDocumentValidation, - collation: operation.arguments.collation, - hint: operation.arguments.hint, - upsert: operation.arguments.upsert - }); + const { filter, replacement, ...opts } = operation.arguments; + return collection.replaceOne(filter, replacement, opts); }); operations.set('startTransaction', async ({ entities, operation }) => { @@ -396,7 +393,8 @@ operations.set('countDocuments', async ({ entities, operation }) => { operations.set('deleteMany', async ({ entities, operation }) => { const collection = entities.getEntity('collection', operation.object); - return collection.deleteMany(operation.arguments.filter); + const { filter, ...opts } = operation.arguments; + return collection.deleteMany(filter, opts); }); operations.set('distinct', async ({ entities, operation }) => { @@ -412,7 +410,8 @@ operations.set('estimatedDocumentCount', async ({ entities, operation }) => { operations.set('findOneAndDelete', async ({ entities, operation }) => { const collection = entities.getEntity('collection', operation.object); - return collection.findOneAndDelete(operation.arguments.filter); + const { filter, ...opts } = operation.arguments; + return collection.findOneAndDelete(filter, opts); }); operations.set('runCommand', async ({ entities, operation }: OperationFunctionParams) => {