Skip to content

Commit ccccbc8

Browse files
mbroadstmbroadst
committed
fix: createCollection only uses listCollections in strict mode
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
1 parent 841d680 commit ccccbc8

File tree

10 files changed

+78
-89
lines changed

10 files changed

+78
-89
lines changed

‎lib/operations/create_collection.js

Lines changed: 38 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,11 @@
33
const CommandOperation = require('./command');
44
const ReadPreference = require('../read_preference');
55
const { Aspect, defineAspects } = require('./operation');
6-
const { applyWriteConcern, handleCallback } = require('../utils');
6+
const { applyWriteConcern } = require('../utils');
77
const { loadCollection } = require('../dynamic_loaders');
88
const { MongoError } = require('../error');
99

10-
// Filter out any write concern options
11-
const illegalCommandFields = [
10+
const ILLEGAL_COMMAND_FIELDS = new Set([
1211
'w',
1312
'wtimeout',
1413
'j',
@@ -22,27 +21,24 @@ const illegalCommandFields = [
2221
'session',
2322
'readConcern',
2423
'writeConcern'
25-
];
24+
]);
2625

2726
class CreateCollectionOperation extends CommandOperation {
2827
constructor(db, name, options) {
2928
super(db, options);
30-
3129
this.name = name;
3230
}
3331

3432
_buildCommand() {
3533
const name = this.name;
3634
const options = this.options;
3735

38-
// Create collection command
3936
const cmd = { create: name };
40-
// Add all optional parameters
4137
for (let n in options) {
4238
if (
4339
options[n] != null &&
4440
typeof options[n] !== 'function' &&
45-
illegalCommandFields.indexOf(n) === -1
41+
!ILLEGAL_COMMAND_FIELDS.has(n)
4642
) {
4743
cmd[n] = options[n];
4844
}
@@ -55,61 +51,51 @@ class CreateCollectionOperation extends CommandOperation {
5551
const db = this.db;
5652
const name = this.name;
5753
const options = this.options;
54+
const Collection = loadCollection();
5855

59-
let Collection = loadCollection();
56+
let listCollectionOptions = Object.assign({ nameOnly: true, strict: false }, options);
57+
listCollectionOptions = applyWriteConcern(listCollectionOptions, { db }, listCollectionOptions);
6058

61-
// Did the user destroy the topology
62-
if (db.serverConfig && db.serverConfig.isDestroyed()) {
63-
return callback(new MongoError('topology was destroyed'));
64-
}
59+
function done(err) {
60+
if (err) {
61+
return callback(err);
62+
}
6563

66-
let listCollectionOptions = Object.assign({}, options, { nameOnly: true });
67-
listCollectionOptions = applyWriteConcern(listCollectionOptions, { db }, listCollectionOptions);
64+
try {
65+
callback(
66+
null,
67+
new Collection(db, db.s.topology, db.databaseName, name, db.s.pkFactory, options)
68+
);
69+
} catch (err) {
70+
callback(err);
71+
}
72+
}
6873

69-
// Check if we have the name
70-
db.listCollections({ name }, listCollectionOptions)
71-
.setReadPreference(ReadPreference.PRIMARY)
72-
.toArray((err, collections) => {
73-
if (err != null) return handleCallback(callback, err, null);
74-
if (collections.length > 0 && listCollectionOptions.strict) {
75-
return handleCallback(
76-
callback,
77-
MongoError.create({
78-
message: `Collection ${name} already exists. Currently in strict mode.`,
79-
driver: true
80-
}),
81-
null
82-
);
83-
} else if (collections.length > 0) {
84-
try {
85-
return handleCallback(
86-
callback,
87-
null,
88-
new Collection(db, db.s.topology, db.databaseName, name, db.s.pkFactory, options)
89-
);
90-
} catch (err) {
91-
return handleCallback(callback, err);
74+
const strictMode = listCollectionOptions.strict;
75+
if (strictMode) {
76+
db.listCollections({ name }, listCollectionOptions)
77+
.setReadPreference(ReadPreference.PRIMARY)
78+
.toArray((err, collections) => {
79+
if (err) {
80+
return callback(err);
9281
}
93-
}
94-
95-
// Execute command
96-
super.execute(err => {
97-
if (err) return handleCallback(callback, err);
9882

99-
try {
100-
return handleCallback(
101-
callback,
102-
null,
103-
new Collection(db, db.s.topology, db.databaseName, name, db.s.pkFactory, options)
83+
if (collections.length > 0) {
84+
return callback(
85+
new MongoError(`Collection ${name} already exists. Currently in strict mode.`)
10486
);
105-
} catch (err) {
106-
return handleCallback(callback, err);
10787
}
88+
89+
super.execute(done);
10890
});
109-
});
91+
92+
return;
93+
}
94+
95+
// otherwise just execute the command
96+
super.execute(done);
11097
}
11198
}
11299

113100
defineAspects(CreateCollectionOperation, Aspect.WRITE_OPERATION);
114-
115101
module.exports = CreateCollectionOperation;

‎test/examples/change_streams.js

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,10 @@ describe('examples(change-stream):', function() {
1616
client = await this.configuration.newClient().connect();
1717
db = client.db(this.configuration.db);
1818

19-
await db.createCollection('inventory');
19+
// ensure database exists, we need this for 3.6
20+
await db.collection('inventory').insertOne({});
21+
22+
// now clear the collection
2023
await db.collection('inventory').deleteMany({});
2124
});
2225

‎test/functional/collection.test.js

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ describe('Collection', function() {
1111
let configuration;
1212
before(function() {
1313
configuration = this.configuration;
14-
return setupDatabase(configuration);
14+
return setupDatabase(configuration, ['listCollectionsDb', 'listCollectionsDb2', 'test_db']);
1515
});
1616

1717
describe('standard collection tests', function() {
@@ -194,12 +194,7 @@ describe('Collection', function() {
194194
'Collection test_strict_create_collection already exists. Currently in strict mode.'
195195
);
196196

197-
// Switch out of strict mode and try to re-create collection
198-
db.createCollection('test_strict_create_collection', { strict: false }, err => {
199-
expect(err).to.not.exist;
200-
// Let's close the db
201-
done();
202-
});
197+
done();
203198
});
204199
});
205200
});
@@ -691,7 +686,7 @@ describe('Collection', function() {
691686
expect(coll).to.exist;
692687

693688
db.createCollection('shouldFailDueToExistingCollection', { strict: true }, err => {
694-
expect(err).to.be.an.instanceof(Error);
689+
expect(err).to.exist;
695690
expect(err.message).to.equal(
696691
'Collection shouldFailDueToExistingCollection already exists. Currently in strict mode.'
697692
);

‎test/functional/cursor.test.js

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -680,7 +680,7 @@ describe('Cursor', function() {
680680
test.equal(null, err);
681681

682682
var db = client.db(configuration.db);
683-
db.createCollection('test_limit_exceptions', function(err, collection) {
683+
db.createCollection('test_limit_exceptions_2', function(err, collection) {
684684
test.equal(null, err);
685685

686686
collection.insert({ a: 1 }, configuration.writeConcernMax(), function(err) {
@@ -753,7 +753,7 @@ describe('Cursor', function() {
753753
test.equal(null, err);
754754

755755
var db = client.db(configuration.db);
756-
db.createCollection('test_limit_exceptions', function(err, collection) {
756+
db.createCollection('test_limit_exceptions_1', function(err, collection) {
757757
test.equal(null, err);
758758

759759
collection.insert({ a: 1 }, configuration.writeConcernMax(), function(err) {
@@ -2147,7 +2147,10 @@ describe('Cursor', function() {
21472147

21482148
var db = client.db(configuration.db);
21492149
var options = { capped: true, size: 8 };
2150-
db.createCollection('should_await_data', options, function(err, collection) {
2150+
db.createCollection('should_await_data_retry_tailable_cursor', options, function(
2151+
err,
2152+
collection
2153+
) {
21512154
test.equal(null, err);
21522155

21532156
collection.insert({ a: 1 }, configuration.writeConcernMax(), function(err) {
@@ -3603,10 +3606,7 @@ describe('Cursor', function() {
36033606
test.equal(null, err);
36043607

36053608
var db = client.db(configuration.db);
3606-
db.createCollection('Should_correctly_execute_count_on_cursor_1_', function(
3607-
err,
3608-
collection
3609-
) {
3609+
db.createCollection('negative_batch_size_and_limit_set', function(err, collection) {
36103610
test.equal(null, err);
36113611

36123612
// insert all docs

‎test/functional/find.test.js

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1144,7 +1144,8 @@ describe('Find', function() {
11441144
var client = configuration.newClient(configuration.writeConcernMax(), { poolSize: 1 });
11451145
client.connect(function(err, client) {
11461146
var db = client.db(configuration.db);
1147-
db.createCollection('timeoutFalse', function(err, collection) {
1147+
db.createCollection('cursor_timeout_false_0', function(err, collection) {
1148+
expect(err).to.not.exist;
11481149
const cursor = collection.find({}, { timeout: false });
11491150
test.equal(false, cursor.cmd.noCursorTimeout);
11501151
client.close(done);
@@ -1163,7 +1164,8 @@ describe('Find', function() {
11631164
var client = configuration.newClient(configuration.writeConcernMax(), { poolSize: 1 });
11641165
client.connect(function(err, client) {
11651166
var db = client.db(configuration.db);
1166-
db.createCollection('timeoutFalse', function(err, collection) {
1167+
db.createCollection('cursor_timeout_false_1', function(err, collection) {
1168+
expect(err).to.not.exist;
11671169
const cursor = collection.find({}, { timeout: true });
11681170
test.equal(true, cursor.cmd.noCursorTimeout);
11691171
client.close(done);
@@ -1319,7 +1321,7 @@ describe('Find', function() {
13191321
var client = configuration.newClient(configuration.writeConcernMax(), { poolSize: 1 });
13201322
client.connect(function(err, client) {
13211323
var db = client.db(configuration.db);
1322-
db.createCollection('shouldCorrectlyExecuteFindAndModify', function(err, collection) {
1324+
db.createCollection('execute_find_and_modify', function(err, collection) {
13231325
var self = { _id: new ObjectID() };
13241326
var _uuid = 'sddffdss';
13251327

@@ -1513,7 +1515,7 @@ describe('Find', function() {
15131515
transactions: transactions
15141516
};
15151517

1516-
db.createCollection('shouldCorrectlyExecuteFindAndModify', function(err, collection) {
1518+
db.createCollection('find_and_modify_generate_correct_bson', function(err, collection) {
15171519
test.equal(null, err);
15181520

15191521
collection.insert(wrapingObject, configuration.writeConcernMax(), function(err, r) {

‎test/functional/mapreduce.test.js

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,11 @@
22
var test = require('./shared').assert;
33
var setupDatabase = require('./shared').setupDatabase;
44
const { Code } = require('../..');
5+
const { expect } = require('chai');
56

67
describe('MapReduce', function() {
78
before(function() {
8-
return setupDatabase(this.configuration);
9+
return setupDatabase(this.configuration, ['outputCollectionDb']);
910
});
1011

1112
it('shouldCorrectlyExecuteGroupFunctionWithFinalizeFunction', {
@@ -125,8 +126,9 @@ describe('MapReduce', function() {
125126
var configuration = this.configuration;
126127
var client = configuration.newClient(configuration.writeConcernMax(), { poolSize: 1 });
127128
client.connect(function(err, client) {
129+
expect(err).to.not.exist;
128130
var db = client.db(configuration.db);
129-
db.createCollection('test_map_reduce', function(err, collection) {
131+
db.createCollection('should_force_map_reduce_error', function(err, collection) {
130132
collection.insert(
131133
[{ user_id: 1 }, { user_id: 2 }],
132134
configuration.writeConcernMax(),
@@ -358,7 +360,7 @@ describe('MapReduce', function() {
358360
collection.mapReduce(
359361
map,
360362
reduce,
361-
{ out: { replace: 'tempCollection', db: 'outputCollectionDb' } },
363+
{ out: { replace: 'test_map_reduce_functions_temp', db: 'outputCollectionDb' } },
362364
function(err, collection) {
363365
test.equal(null, err);
364366

@@ -481,7 +483,7 @@ describe('MapReduce', function() {
481483
collection.mapReduce(
482484
map,
483485
reduce,
484-
{ scope: { util: util }, out: { replace: 'tempCollection' } },
486+
{ scope: { util: util }, out: { replace: 'test_map_reduce_temp' } },
485487
function(err, collection) {
486488
// After MapReduce
487489
test.equal(200, util.times_one_hundred(2));

‎test/functional/multiple_db.test.js

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
'use strict';
22
var test = require('./shared').assert;
33
var setupDatabase = require('./shared').setupDatabase;
4+
const { expect } = require('chai');
45

56
describe('Multiple Databases', function() {
67
before(function() {
7-
return setupDatabase(this.configuration);
8+
return setupDatabase(this.configuration, ['integration_tests2']);
89
});
910

1011
/**
@@ -26,11 +27,8 @@ describe('Multiple Databases', function() {
2627
// Close second database
2728
second_test_database.close();
2829
// Let's grab a connection to the different db resusing our connection pools
29-
var secondDb = client.db(configuration.db_name + '_2');
30-
secondDb.createCollection('shouldCorrectlyUseSameConnectionsForTwoDifferentDbs', function(
31-
err,
32-
collection
33-
) {
30+
var secondDb = client.db('integration_tests2');
31+
secondDb.createCollection('same_connection_two_dbs', function(err, collection) {
3432
// Insert a dummy document
3533
collection.insert({ a: 20 }, { safe: true }, function(err) {
3634
test.equal(null, err);
@@ -40,10 +38,9 @@ describe('Multiple Databases', function() {
4038
test.equal(20, item.a);
4139

4240
// Use the other db
43-
db.createCollection('shouldCorrectlyUseSameConnectionsForTwoDifferentDbs', function(
44-
err,
45-
collection
46-
) {
41+
db.createCollection('same_connection_two_dbs', function(err, collection) {
42+
expect(err).to.not.exist;
43+
4744
// Insert a dummy document
4845
collection.insert({ b: 20 }, { safe: true }, function(err) {
4946
test.equal(null, err);
@@ -74,11 +71,13 @@ describe('Multiple Databases', function() {
7471
var configuration = this.configuration;
7572
var client = configuration.newClient(configuration.writeConcernMax(), { poolSize: 1 });
7673
client.connect(function(err, client) {
74+
expect(err).to.not.exist;
7775
var db_instance = client.db('site1');
7876
db_instance = client.db('site2');
7977
db_instance = client.db('rss');
8078

8179
db_instance.collection('counters', function(err, collection) {
80+
expect(err).to.not.exist;
8281
collection.findAndModify({}, {}, { $inc: { db: 1 } }, { new: true }, function(err) {
8382
test.equal(null, err);
8483
client.close(done);

‎test/functional/operation_promises_example.test.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ var delay = function(ms) {
1616

1717
describe('Operation (Promises)', function() {
1818
before(function() {
19-
return setupDatabase(this.configuration, ['integration_tests_2']);
19+
return setupDatabase(this.configuration, ['integration_tests_2', 'hr', 'reporting']);
2020
});
2121

2222
/**************************************************************************

‎test/functional/readconcern.test.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -474,7 +474,9 @@ describe('ReadConcern', function() {
474474
expect(db.readConcern).to.deep.equal({ level: 'local' });
475475

476476
// Get a collection using createCollection
477-
db.createCollection('readConcernCollection', (err, collection) => {
477+
db.createCollection('readConcernCollection_createCollection', (err, collection) => {
478+
expect(err).to.not.exist;
479+
478480
// Validate readConcern
479481
expect(collection.readConcern).to.deep.equal({ level: 'local' });
480482
done();

‎test/unit/db_list_collections.test.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ describe('db.listCollections', function() {
4343
},
4444
{
4545
description: 'should send nameOnly: true for db.createCollection',
46-
command: db => db.createCollection('foo', () => {}),
46+
command: db => db.createCollection('foo', { strict: true }, () => {}),
4747
listCollectionsValue: true
4848
},
4949
{

0 commit comments

Comments
 (0)