Skip to content

Commit d368f12

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 153646c commit d368f12

File tree

10 files changed

+77
-87
lines changed

10 files changed

+77
-87
lines changed

‎lib/operations/create_collection.js

Lines changed: 37 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,11 @@ const Aspect = require('./operation').Aspect;
44
const defineAspects = require('./operation').defineAspects;
55
const CommandOperation = require('./command');
66
const applyWriteConcern = require('../utils').applyWriteConcern;
7-
const handleCallback = require('../utils').handleCallback;
87
const loadCollection = require('../dynamic_loaders').loadCollection;
98
const MongoError = require('../core').MongoError;
109
const ReadPreference = require('../core').ReadPreference;
1110

12-
// Filter out any write concern options
13-
const illegalCommandFields = [
11+
const ILLEGAL_COMMAND_FIELDS = new Set([
1412
'w',
1513
'wtimeout',
1614
'j',
@@ -24,27 +22,24 @@ const illegalCommandFields = [
2422
'session',
2523
'readConcern',
2624
'writeConcern'
27-
];
25+
]);
2826

2927
class CreateCollectionOperation extends CommandOperation {
3028
constructor(db, name, options) {
3129
super(db, options);
32-
3330
this.name = name;
3431
}
3532

3633
_buildCommand() {
3734
const name = this.name;
3835
const options = this.options;
3936

40-
// Create collection command
4137
const cmd = { create: name };
42-
// Add all optional parameters
4338
for (let n in options) {
4439
if (
4540
options[n] != null &&
4641
typeof options[n] !== 'function' &&
47-
illegalCommandFields.indexOf(n) === -1
42+
!ILLEGAL_COMMAND_FIELDS.has(n)
4843
) {
4944
cmd[n] = options[n];
5045
}
@@ -57,61 +52,51 @@ class CreateCollectionOperation extends CommandOperation {
5752
const db = this.db;
5853
const name = this.name;
5954
const options = this.options;
55+
const Collection = loadCollection();
6056

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

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

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

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

101-
try {
102-
return handleCallback(
103-
callback,
104-
null,
105-
new Collection(db, db.s.topology, db.databaseName, name, db.s.pkFactory, options)
84+
if (collections.length > 0) {
85+
return callback(
86+
new MongoError(`Collection ${name} already exists. Currently in strict mode.`)
10687
);
107-
} catch (err) {
108-
return handleCallback(callback, err);
10988
}
89+
90+
super.execute(done);
11091
});
111-
});
92+
93+
return;
94+
}
95+
96+
// otherwise just execute the command
97+
super.execute(done);
11298
}
11399
}
114100

115101
defineAspects(CreateCollectionOperation, Aspect.WRITE_OPERATION);
116-
117102
module.exports = CreateCollectionOperation;

‎test/examples/change_streams.js

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,11 @@ 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');
20-
await db.collection('inventory').deleteMany({});
19+
// ensure database exists, we need this for 3.6
20+
await db.collection('inventory').insertOne({});
21+
22+
// now clear the collection
23+
await db.collection('inventory').deleteMany();
2124
});
2225

2326
afterEach(async function() {

‎test/functional/collection.test.js

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

1616
describe('standard collection tests', function() {
@@ -208,12 +208,7 @@ describe('Collection', function() {
208208
'Collection test_strict_create_collection already exists. Currently in strict mode.'
209209
);
210210

211-
// Switch out of strict mode and try to re-create collection
212-
db.createCollection('test_strict_create_collection', { strict: false }, err => {
213-
expect(err).to.not.exist;
214-
// Let's close the db
215-
done();
216-
});
211+
done();
217212
});
218213
});
219214
});
@@ -749,7 +744,7 @@ describe('Collection', function() {
749744
expect(coll).to.exist;
750745

751746
db.createCollection('shouldFailDueToExistingCollection', { strict: true }, err => {
752-
expect(err).to.be.an.instanceof(Error);
747+
expect(err).to.exist;
753748
expect(err.message).to.equal(
754749
'Collection shouldFailDueToExistingCollection already exists. Currently in strict mode.'
755750
);

‎test/functional/cursor.test.js

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2549,7 +2549,10 @@ describe('Cursor', function() {
25492549

25502550
var db = client.db(configuration.db);
25512551
var options = { capped: true, size: 8 };
2552-
db.createCollection('should_await_data', options, function(err, collection) {
2552+
db.createCollection('should_await_data_retry_tailable_cursor', options, function(
2553+
err,
2554+
collection
2555+
) {
25532556
test.equal(null, err);
25542557

25552558
collection.insert({ a: 1 }, configuration.writeConcernMax(), function(err) {
@@ -4042,10 +4045,7 @@ describe('Cursor', function() {
40424045
test.equal(null, err);
40434046

40444047
var db = client.db(configuration.db);
4045-
db.createCollection('Should_correctly_execute_count_on_cursor_1_', function(
4046-
err,
4047-
collection
4048-
) {
4048+
db.createCollection('negative_batch_size_and_limit_set', function(err, collection) {
40494049
test.equal(null, err);
40504050

40514051
// insert all docs

‎test/functional/find.test.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1379,7 +1379,7 @@ describe('Find', function() {
13791379
var client = configuration.newClient(configuration.writeConcernMax(), { poolSize: 1 });
13801380
client.connect(function(err, client) {
13811381
var db = client.db(configuration.db);
1382-
db.createCollection('shouldCorrectlyExecuteFindAndModify', function(err, collection) {
1382+
db.createCollection('execute_find_and_modify', function(err, collection) {
13831383
var self = { _id: new ObjectID() };
13841384
var _uuid = 'sddffdss';
13851385

@@ -1601,7 +1601,7 @@ describe('Find', function() {
16011601
transactions: transactions
16021602
};
16031603

1604-
db.createCollection('shouldCorrectlyExecuteFindAndModify', function(err, collection) {
1604+
db.createCollection('find_and_modify_generate_correct_bson', function(err, collection) {
16051605
test.equal(null, err);
16061606

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

‎test/functional/mapreduce.test.js

Lines changed: 8 additions & 5 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').expect;
45

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

1011
/**
@@ -133,7 +134,9 @@ describe('MapReduce', function() {
133134
var client = configuration.newClient(configuration.writeConcernMax(), { poolSize: 1 });
134135
client.connect(function(err, client) {
135136
var db = client.db(configuration.db);
136-
db.createCollection('test_map_reduce', function(err, collection) {
137+
db.createCollection('should_force_map_reduce_error', function(err, collection) {
138+
expect(err).to.not.exist;
139+
137140
collection.insert(
138141
[{ user_id: 1 }, { user_id: 2 }],
139142
configuration.writeConcernMax(),
@@ -369,7 +372,7 @@ describe('MapReduce', function() {
369372
// Create a test collection
370373
db.createCollection('test_map_reduce_functions', function(err, collection) {
371374
// create the output collection
372-
outDb.createCollection('tempCollection', err => {
375+
outDb.createCollection('test_map_reduce_functions_temp', err => {
373376
test.equal(null, err);
374377

375378
// Insert some documents to perform map reduce over
@@ -391,7 +394,7 @@ describe('MapReduce', function() {
391394
collection.mapReduce(
392395
map,
393396
reduce,
394-
{ out: { replace: 'tempCollection', db: 'outputCollectionDb' } },
397+
{ out: { replace: 'test_map_reduce_functions_temp', db: 'outputCollectionDb' } },
395398
function(err, collection) {
396399
test.equal(null, err);
397400

@@ -520,7 +523,7 @@ describe('MapReduce', function() {
520523
collection.mapReduce(
521524
map,
522525
reduce,
523-
{ scope: { util: util }, out: { replace: 'tempCollection' } },
526+
{ scope: { util: util }, out: { replace: 'test_map_reduce_temp' } },
524527
function(err, collection) {
525528
// After MapReduce
526529
test.equal(200, util.times_one_hundred(2));

‎test/functional/multiple_db.test.js

Lines changed: 12 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').expect;
45

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

1011
/**
@@ -84,16 +85,18 @@ describe('Multiple Databases', function() {
8485
var second_test_database_client = configuration.newClient({ w: 1 }, { poolSize: 1 });
8586
// Just create second database
8687
client.connect(function(err, client) {
88+
expect(err).to.not.exist;
89+
8790
second_test_database_client.connect(function(err, second_test_database) {
91+
expect(err).to.not.exist;
8892
var db = client.db(configuration.db);
8993
// Close second database
9094
second_test_database.close();
9195
// Let's grab a connection to the different db resusing our connection pools
92-
var secondDb = client.db(configuration.db_name + '_2');
93-
secondDb.createCollection('shouldCorrectlyUseSameConnectionsForTwoDifferentDbs', function(
94-
err,
95-
collection
96-
) {
96+
var secondDb = client.db('integration_tests2');
97+
secondDb.createCollection('same_connection_two_dbs', function(err, collection) {
98+
expect(err).to.not.exist;
99+
97100
// Insert a dummy document
98101
collection.insert({ a: 20 }, { safe: true }, function(err) {
99102
test.equal(null, err);
@@ -103,10 +106,9 @@ describe('Multiple Databases', function() {
103106
test.equal(20, item.a);
104107

105108
// Use the other db
106-
db.createCollection('shouldCorrectlyUseSameConnectionsForTwoDifferentDbs', function(
107-
err,
108-
collection
109-
) {
109+
db.createCollection('same_connection_two_dbs', function(err, collection) {
110+
expect(err).to.not.exist;
111+
110112
// Insert a dummy document
111113
collection.insert({ b: 20 }, { safe: true }, function(err) {
112114
test.equal(null, err);

‎test/functional/operation_promises_example.test.js

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

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

2121
/**************************************************************************

‎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)