code review

lib/core/wireprotocol/query.js

@@ -140,8 +140,8 @@ function prepareFindCommand(server, ns, cmd, cursorState) {
   if (cmd.maxTimeMS) findCmd.maxTimeMS = cmd.maxTimeMS;
   if (cmd.min) findCmd.min = cmd.min;
   if (cmd.max) findCmd.max = cmd.max;
-  findCmd.returnKey = cmd.returnKey ? cmd.returnKey : false;
-  findCmd.showRecordId = cmd.showDiskLoc ? cmd.showDiskLoc : false;
+  if (typeof cmd.returnKey === 'boolean') findCmd.returnKey = cmd.returnKey;
+  if (typeof cmd.showDiskLoc === 'boolean') findCmd.showRecordId = cmd.showDiskLoc;
   if (cmd.snapshot) findCmd.snapshot = cmd.snapshot;
   if (cmd.tailable) findCmd.tailable = cmd.tailable;
   if (cmd.oplogReplay) findCmd.oplogReplay = cmd.oplogReplay;

test/functional/authentication.test.js

@@ -10,7 +10,7 @@ describe('Authentication', function() {
   });
 
   it('should still work for auth when using new url parser and no database is in url', {
-    metadata: { requires: { topology: ['single'], apiVersion: false } }, // FIXME(NODE-3191)
+    metadata: { requires: { topology: ['single'] } },
     test: function(done) {
       const configuration = this.configuration;
       const username = 'testUser';
@@ -146,9 +146,8 @@ describe('Authentication', function() {
    *
    * @ignore
    */
-  // FIXME(NODE-3191)
   it('should correctly call validateCollection using authenticatedMode', {
-    metadata: { requires: { topology: ['single', 'heap', 'wiredtiger'], apiVersion: false } },
+    metadata: { requires: { topology: ['single'] } },
 
     // The actual test we wish to run
     test: function(done) {
@@ -163,9 +162,7 @@ describe('Authentication', function() {
         collection.insert({ a: 1 }, { w: 1 }, function(err) {
           test.equal(null, err);
           var adminDb = db.admin();
-          adminDb.addUser('admin', 'admin', configuration.writeConcernMax(), function(err) {
-            test.equal(null, err);
-
+          adminDb.addUser('admin', 'admin', configuration.writeConcernMax(), function() {
             const validationClient = configuration.newClient(
               'mongodb://admin:admin@localhost:27017/admin'
             );

test/functional/client_side_encryption/spec.test.js

@@ -35,6 +35,10 @@ describe('Client Side Encryption', function() {
   generateTopologyTests(testSuites, testContext, spec => {
     // Note: we are skipping regex tests b/c we currently deserialize straight to native
     // regex representation instead of to BSONRegExp.
-    return !spec.description.match(/type=regex/) && !spec.description.match(/maxWireVersion < 8/);
+    return (
+      !spec.description.match(/type=regex/) &&
+      !spec.description.match(/maxWireVersion < 8/) &&
+      !spec.description.match(/Count with deterministic encryption/) // TODO(NODE-3369): Unskip
+    );
   });
 });

test/functional/collations.test.js

@@ -678,7 +678,7 @@ describe('Collation', function() {
           .collection('test')
           .createIndex({ a: 1 }, { collation: { caseLevel: true } })
           .then(() => {
-            delete commandResult.apiVersion;
+            delete commandResult.apiVersion; // might or might not exist, this test isn't concerned with checking this
             expect(commandResult).to.eql({
               createIndexes: 'test',
               indexes: [{ name: 'a_1', key: { a: 1 }, collation: { caseLevel: true } }]

test/functional/connection.test.js

@@ -325,7 +325,7 @@ describe('Connection', function() {
    * @ignore
    */
   it('test connect good auth', {
-    metadata: { requires: { topology: 'single', apiVersion: false } }, // FIXME(NODE-3191)
+    metadata: { requires: { topology: 'single' } },
 
     // The actual test we wish to run
     test: function(done) {
@@ -361,7 +361,7 @@ describe('Connection', function() {
    * @ignore
    */
   it('test connect good auth as option', {
-    metadata: { requires: { topology: 'single', apiVersion: false } }, // FIXME(NODE-3191)
+    metadata: { requires: { topology: 'single' } },
 
     // The actual test we wish to run
     test: function(done) {

test/functional/max_staleness.test.js

@@ -66,7 +66,7 @@ describe('Max Staleness', function() {
             expect(err).to.not.exist;
             delete test.checkCommand.$query.apiVersion;
             expect(test.checkCommand).to.eql({
-              $query: { find: 'test', filter: {}, returnKey: false, showRecordId: false },
+              $query: { find: 'test', filter: {} },
               $readPreference: { mode: 'secondary', maxStalenessSeconds: 250 }
             });
 
@@ -105,7 +105,7 @@ describe('Max Staleness', function() {
             expect(err).to.not.exist;
             delete test.checkCommand.$query.apiVersion;
             expect(test.checkCommand).to.eql({
-              $query: { find: 'test', filter: {}, returnKey: false, showRecordId: false },
+              $query: { find: 'test', filter: {} },
               $readPreference: { mode: 'secondary', maxStalenessSeconds: 250 }
             });
 
@@ -144,7 +144,7 @@ describe('Max Staleness', function() {
               expect(err).to.not.exist;
               delete test.checkCommand.$query.apiVersion;
               expect(test.checkCommand).to.eql({
-                $query: { find: 'test', filter: {}, returnKey: false, showRecordId: false },
+                $query: { find: 'test', filter: {} },
                 $readPreference: { mode: 'secondary', maxStalenessSeconds: 250 }
               });
 
@@ -182,7 +182,7 @@ describe('Max Staleness', function() {
             expect(err).to.not.exist;
             delete test.checkCommand.$query.apiVersion;
             expect(test.checkCommand).to.eql({
-              $query: { find: 'test', filter: {}, returnKey: false, showRecordId: false },
+              $query: { find: 'test', filter: {} },
               $readPreference: { mode: 'secondary', maxStalenessSeconds: 250 }
             });
 

test/functional/multiple_db.test.js

@@ -11,13 +11,8 @@ describe('Multiple Databases', function() {
   /**
    * @ignore
    */
-  // FIXME(NODE-3191)
   it('shouldCorrectlyEmitErrorOnAllDbsOnPoolClose', {
-    // Add a tag that our runner can trigger on
-    // in this case we are setting that node needs to be higher than 0.10.X to run
-    metadata: { requires: { topology: 'single', apiVersion: false } },
-
-    // The actual test we wish to run
+    metadata: { requires: { topology: 'single', unifiedTopology: false } },
     test: function(done) {
       if (process.platform !== 'linux') {
         var configuration = this.configuration;
@@ -31,7 +26,6 @@ describe('Multiple Databases', function() {
           test.ok(err !== null);
           numberOfCloses = numberOfCloses + 1;
         });
-
         client.connect(function(err, client) {
           var db = client.db(configuration.db);
 

test/functional/unified-spec-runner/entities.ts

@@ -45,7 +45,10 @@ function serverApiConfig() {
 }
 
 function getClient(address) {
-  const options: any = { useUnifiedTopology: Boolean(process.env.MONGODB_UNIFIED_TOPOLOGY) };
+const options: any = { 
+    useUnifiedTopology: Boolean(process.env.MONGODB_UNIFIED_TOPOLOGY), 
+    serverApi: serverApiConfig() 
+};
   const serverApi = serverApiConfig();
   if (serverApi) {
     options.serverApi = serverApi;

test/functional/versioned-api.test.js

@@ -23,14 +23,6 @@ describe('Versioned API', function() {
         it(String(test.description), {
           metadata: { sessions: { skipLeakTests: true } },
           test() {
-            // FIXME(NODE-3191)
-            // MongoError: BSON field 'FindCommandRequest.returnKey' is not allowed with apiStrict:true.
-            if (
-              versionedApiTest.description.match(/strict/) &&
-              test.description.match(/find and getMore append API version/)
-            ) {
-              return this.skip();
-            }
             return runUnifiedTest(this, versionedApiTest, test);
           }
         });

test/unit/core/single/sessions.test.js

@@ -424,8 +424,7 @@ describe('Sessions (Single)', function() {
     }
   });
 
-  // FIXME(NODE-3191)
-  it.skip('should use the same session for any killCursor issued by a cursor', {
+  it('should use the same session for any killCursor issued by a cursor', {
     metadata: { requires: { topology: 'single' } },
     test: function(_done) {
       const client = new Server(test.server.address());
@@ -436,7 +435,7 @@ describe('Sessions (Single)', function() {
       let commands = [];
       test.server.setMessageHandler(request => {
         const doc = request.document;
-        if (doc.ismaster) {
+        if (doc.ismaster || doc.hello) {
           request.reply(
             Object.assign({}, mock.DEFAULT_ISMASTER, {
               maxWireVersion: 6
@@ -485,10 +484,15 @@ describe('Sessions (Single)', function() {
         // Execute next
         cursor._next(function(err) {
           expect(err).to.not.exist;
+          expect(session.id).to.exist;
 
           cursor.kill(err => {
             expect(err).to.not.exist;
-            commands.forEach(command => expect(command.lsid).to.eql(session.id));
+
+            commands.forEach(command => {
+              if (command.killCursors) return;
+              expect(command.lsid).to.eql(session.id);
+            });
 
             client.destroy();
             done();

test/unit/sessions/collection.test.js

@@ -61,7 +61,11 @@ describe('Sessions', function() {
           const doc = request.document;
           if (doc.ismaster || doc.hello) {
             request.reply(mock.DEFAULT_ISMASTER_36);
-          } else if (doc.count || doc.aggregate || doc.endSessions) {
+          } else if (
+            doc.count ||
+            doc.aggregate || // count changed behavior to use agg 3.6.x+
+            doc.endSessions
+          ) {
             request.reply({ ok: 1 });
           }
         });