fix: createCollection only uses listCollections in strict mode

mbroadst · mbroadst · commit d368f1226ea7 · 2020-04-03T12:04:12.000-04:00
Our `createCollection` helper attempts to be too helpful. It will run a `listCollections` before attempting to send the `create` command to the server, and if a collection with the same name is present it will skip creating the collection and return a local reference to the collection. This is dangerous because there is no way to strictly verify that the remote collection has all the same options a user is passing into the helper NODE-2537
diff --git a/lib/operations/create_collection.js b/lib/operations/create_collection.js
@@ -4,13 +4,11 @@ const Aspect = require('./operation').Aspect;
 const defineAspects = require('./operation').defineAspects;
 const CommandOperation = require('./command');
 const applyWriteConcern = require('../utils').applyWriteConcern;
-const handleCallback = require('../utils').handleCallback;
 const loadCollection = require('../dynamic_loaders').loadCollection;
 const MongoError = require('../core').MongoError;
 const ReadPreference = require('../core').ReadPreference;
 
-// Filter out any write concern options
-const illegalCommandFields = [
+const ILLEGAL_COMMAND_FIELDS = new Set([
   'w',
   'wtimeout',
   'j',
@@ -24,27 +22,24 @@ const illegalCommandFields = [
   'session',
   'readConcern',
   'writeConcern'
-];
+]);
 
 class CreateCollectionOperation extends CommandOperation {
   constructor(db, name, options) {
     super(db, options);
-
     this.name = name;
   }
 
   _buildCommand() {
     const name = this.name;
     const options = this.options;
 
-    // Create collection command
     const cmd = { create: name };
-    // Add all optional parameters
     for (let n in options) {
       if (
         options[n] != null &&
         typeof options[n] !== 'function' &&
-        illegalCommandFields.indexOf(n) === -1
+        !ILLEGAL_COMMAND_FIELDS.has(n)
       ) {
         cmd[n] = options[n];
       }
@@ -57,61 +52,51 @@ class CreateCollectionOperation extends CommandOperation {
     const db = this.db;
     const name = this.name;
     const options = this.options;
+    const Collection = loadCollection();
 
-    let Collection = loadCollection();
+    let listCollectionOptions = Object.assign({ nameOnly: true, strict: false }, options);
+    listCollectionOptions = applyWriteConcern(listCollectionOptions, { db }, listCollectionOptions);
 
-    // Did the user destroy the topology
-    if (db.serverConfig && db.serverConfig.isDestroyed()) {
-      return callback(new MongoError('topology was destroyed'));
-    }
+    function done(err) {
+      if (err) {
+        return callback(err);
+      }
 
-    let listCollectionOptions = Object.assign({}, options, { nameOnly: true });
-    listCollectionOptions = applyWriteConcern(listCollectionOptions, { db }, listCollectionOptions);
+      try {
+        callback(
+          null,
+          new Collection(db, db.s.topology, db.databaseName, name, db.s.pkFactory, options)
+        );
+      } catch (err) {
+        callback(err);
+      }
+    }
 
-    // Check if we have the name
-    db.listCollections({ name }, listCollectionOptions)
-      .setReadPreference(ReadPreference.PRIMARY)
-      .toArray((err, collections) => {
-        if (err != null) return handleCallback(callback, err, null);
-        if (collections.length > 0 && listCollectionOptions.strict) {
-          return handleCallback(
-            callback,
-            MongoError.create({
-              message: `Collection ${name} already exists. Currently in strict mode.`,
-              driver: true
-            }),
-            null
-          );
-        } else if (collections.length > 0) {
-          try {
-            return handleCallback(
-              callback,
-              null,
-              new Collection(db, db.s.topology, db.databaseName, name, db.s.pkFactory, options)
-            );
-          } catch (err) {
-            return handleCallback(callback, err);
+    const strictMode = listCollectionOptions.strict;
+    if (strictMode) {
+      db.listCollections({ name }, listCollectionOptions)
+        .setReadPreference(ReadPreference.PRIMARY)
+        .toArray((err, collections) => {
+          if (err) {
+            return callback(err);
           }
-        }
-
-        // Execute command
-        super.execute(err => {
-          if (err) return handleCallback(callback, err);
 
-          try {
-            return handleCallback(
-              callback,
-              null,
-              new Collection(db, db.s.topology, db.databaseName, name, db.s.pkFactory, options)
+          if (collections.length > 0) {
+            return callback(
+              new MongoError(`Collection ${name} already exists. Currently in strict mode.`)
             );
-          } catch (err) {
-            return handleCallback(callback, err);
           }
+
+          super.execute(done);
         });
-      });
+
+      return;
+    }
+
+    // otherwise just execute the command
+    super.execute(done);
   }
 }
 
 defineAspects(CreateCollectionOperation, Aspect.WRITE_OPERATION);
-
 module.exports = CreateCollectionOperation;
diff --git a/test/examples/change_streams.js b/test/examples/change_streams.js
@@ -16,8 +16,11 @@ describe('examples(change-stream):', function() {
     client = await this.configuration.newClient().connect();
     db = client.db(this.configuration.db);
 
-    await db.createCollection('inventory');
-    await db.collection('inventory').deleteMany({});
+    // ensure database exists, we need this for 3.6
+    await db.collection('inventory').insertOne({});
+
+    // now clear the collection
+    await db.collection('inventory').deleteMany();
   });
 
   afterEach(async function() {
diff --git a/test/functional/collection.test.js b/test/functional/collection.test.js
@@ -10,7 +10,7 @@ describe('Collection', function() {
   let configuration;
   before(function() {
     configuration = this.configuration;
-    return setupDatabase(configuration);
+    return setupDatabase(configuration, ['listCollectionsDb', 'listCollectionsDb2', 'test_db']);
   });
 
   describe('standard collection tests', function() {
@@ -208,12 +208,7 @@ describe('Collection', function() {
             'Collection test_strict_create_collection already exists. Currently in strict mode.'
           );
 
-          // Switch out of strict mode and try to re-create collection
-          db.createCollection('test_strict_create_collection', { strict: false }, err => {
-            expect(err).to.not.exist;
-            // Let's close the db
-            done();
-          });
+          done();
         });
       });
     });
@@ -749,7 +744,7 @@ describe('Collection', function() {
         expect(coll).to.exist;
 
         db.createCollection('shouldFailDueToExistingCollection', { strict: true }, err => {
-          expect(err).to.be.an.instanceof(Error);
+          expect(err).to.exist;
           expect(err.message).to.equal(
             'Collection shouldFailDueToExistingCollection already exists. Currently in strict mode.'
           );
diff --git a/test/functional/cursor.test.js b/test/functional/cursor.test.js
@@ -2549,7 +2549,10 @@ describe('Cursor', function() {
 
         var db = client.db(configuration.db);
         var options = { capped: true, size: 8 };
-        db.createCollection('should_await_data', options, function(err, collection) {
+        db.createCollection('should_await_data_retry_tailable_cursor', options, function(
+          err,
+          collection
+        ) {
           test.equal(null, err);
 
           collection.insert({ a: 1 }, configuration.writeConcernMax(), function(err) {
@@ -4042,10 +4045,7 @@ describe('Cursor', function() {
         test.equal(null, err);
 
         var db = client.db(configuration.db);
-        db.createCollection('Should_correctly_execute_count_on_cursor_1_', function(
-          err,
-          collection
-        ) {
+        db.createCollection('negative_batch_size_and_limit_set', function(err, collection) {
           test.equal(null, err);
 
           // insert all docs
diff --git a/test/functional/find.test.js b/test/functional/find.test.js
@@ -1379,7 +1379,7 @@ describe('Find', function() {
       var client = configuration.newClient(configuration.writeConcernMax(), { poolSize: 1 });
       client.connect(function(err, client) {
         var db = client.db(configuration.db);
-        db.createCollection('shouldCorrectlyExecuteFindAndModify', function(err, collection) {
+        db.createCollection('execute_find_and_modify', function(err, collection) {
           var self = { _id: new ObjectID() };
           var _uuid = 'sddffdss';
 
@@ -1601,7 +1601,7 @@ describe('Find', function() {
           transactions: transactions
         };
 
-        db.createCollection('shouldCorrectlyExecuteFindAndModify', function(err, collection) {
+        db.createCollection('find_and_modify_generate_correct_bson', function(err, collection) {
           test.equal(null, err);
 
           collection.insert(wrapingObject, configuration.writeConcernMax(), function(err, r) {
diff --git a/test/functional/mapreduce.test.js b/test/functional/mapreduce.test.js
@@ -1,10 +1,11 @@
 'use strict';
 var test = require('./shared').assert;
 var setupDatabase = require('./shared').setupDatabase;
+const expect = require('chai').expect;
 
 describe('MapReduce', function() {
   before(function() {
-    return setupDatabase(this.configuration);
+    return setupDatabase(this.configuration, ['outputCollectionDb']);
   });
 
   /**
@@ -133,7 +134,9 @@ describe('MapReduce', function() {
       var client = configuration.newClient(configuration.writeConcernMax(), { poolSize: 1 });
       client.connect(function(err, client) {
         var db = client.db(configuration.db);
-        db.createCollection('test_map_reduce', function(err, collection) {
+        db.createCollection('should_force_map_reduce_error', function(err, collection) {
+          expect(err).to.not.exist;
+
           collection.insert(
             [{ user_id: 1 }, { user_id: 2 }],
             configuration.writeConcernMax(),
@@ -369,7 +372,7 @@ describe('MapReduce', function() {
         // Create a test collection
         db.createCollection('test_map_reduce_functions', function(err, collection) {
           // create the output collection
-          outDb.createCollection('tempCollection', err => {
+          outDb.createCollection('test_map_reduce_functions_temp', err => {
             test.equal(null, err);
 
             // Insert some documents to perform map reduce over
@@ -391,7 +394,7 @@ describe('MapReduce', function() {
                 collection.mapReduce(
                   map,
                   reduce,
-                  { out: { replace: 'tempCollection', db: 'outputCollectionDb' } },
+                  { out: { replace: 'test_map_reduce_functions_temp', db: 'outputCollectionDb' } },
                   function(err, collection) {
                     test.equal(null, err);
 
@@ -520,7 +523,7 @@ describe('MapReduce', function() {
               collection.mapReduce(
                 map,
                 reduce,
-                { scope: { util: util }, out: { replace: 'tempCollection' } },
+                { scope: { util: util }, out: { replace: 'test_map_reduce_temp' } },
                 function(err, collection) {
                   // After MapReduce
                   test.equal(200, util.times_one_hundred(2));
diff --git a/test/functional/multiple_db.test.js b/test/functional/multiple_db.test.js
@@ -1,10 +1,11 @@
 'use strict';
 var test = require('./shared').assert;
 var setupDatabase = require('./shared').setupDatabase;
+const expect = require('chai').expect;
 
 describe('Multiple Databases', function() {
   before(function() {
-    return setupDatabase(this.configuration);
+    return setupDatabase(this.configuration, ['integration_tests2']);
   });
 
   /**
@@ -84,16 +85,18 @@ describe('Multiple Databases', function() {
       var second_test_database_client = configuration.newClient({ w: 1 }, { poolSize: 1 });
       // Just create second database
       client.connect(function(err, client) {
+        expect(err).to.not.exist;
+
         second_test_database_client.connect(function(err, second_test_database) {
+          expect(err).to.not.exist;
           var db = client.db(configuration.db);
           // Close second database
           second_test_database.close();
           // Let's grab a connection to the different db resusing our connection pools
-          var secondDb = client.db(configuration.db_name + '_2');
-          secondDb.createCollection('shouldCorrectlyUseSameConnectionsForTwoDifferentDbs', function(
-            err,
-            collection
-          ) {
+          var secondDb = client.db('integration_tests2');
+          secondDb.createCollection('same_connection_two_dbs', function(err, collection) {
+            expect(err).to.not.exist;
+
             // Insert a dummy document
             collection.insert({ a: 20 }, { safe: true }, function(err) {
               test.equal(null, err);
@@ -103,10 +106,9 @@ describe('Multiple Databases', function() {
                 test.equal(20, item.a);
 
                 // Use the other db
-                db.createCollection('shouldCorrectlyUseSameConnectionsForTwoDifferentDbs', function(
-                  err,
-                  collection
-                ) {
+                db.createCollection('same_connection_two_dbs', function(err, collection) {
+                  expect(err).to.not.exist;
+
                   // Insert a dummy document
                   collection.insert({ b: 20 }, { safe: true }, function(err) {
                     test.equal(null, err);
diff --git a/test/functional/operation_promises_example.test.js b/test/functional/operation_promises_example.test.js
@@ -15,7 +15,7 @@ var delay = function(ms) {
 
 describe('Operation (Promises)', function() {
   before(function() {
-    return setupDatabase(this.configuration, ['integration_tests_2']);
+    return setupDatabase(this.configuration, ['integration_tests_2', 'hr', 'reporting']);
   });
 
   /**************************************************************************
diff --git a/test/functional/readconcern.test.js b/test/functional/readconcern.test.js
@@ -474,7 +474,9 @@ describe('ReadConcern', function() {
         expect(db.readConcern).to.deep.equal({ level: 'local' });
 
         // Get a collection using createCollection
-        db.createCollection('readConcernCollection', (err, collection) => {
+        db.createCollection('readConcernCollection_createCollection', (err, collection) => {
+          expect(err).to.not.exist;
+
           // Validate readConcern
           expect(collection.readConcern).to.deep.equal({ level: 'local' });
           done();
diff --git a/test/unit/db_list_collections.test.js b/test/unit/db_list_collections.test.js
@@ -43,7 +43,7 @@ describe('db.listCollections', function() {
     },
     {
       description: 'should send nameOnly: true for db.createCollection',
-      command: db => db.createCollection('foo', () => {}),
+      command: db => db.createCollection('foo', { strict: true }, () => {}),
       listCollectionsValue: true
     },
     {

Original file line number	Diff line number	Diff line change
`@@ -43,7 +43,7 @@ describe('db.listCollections', function() {`
`43`	`43`	`},`
`44`	`44`	`{`
`45`	`45`	`description: 'should send nameOnly: true for db.createCollection',`
`46`		`- command: db => db.createCollection('foo', () => {}),`
	`46`	`+ command: db => db.createCollection('foo', { strict: true }, () => {}),`
`47`	`47`	`listCollectionsValue: true`
`48`	`48`	`},`
`49`	`49`	`{`