Skip to content

Commit d6f3001

Browse files
mbroadstmbroadst
committed
Revert "fix: createCollection only uses listCollections in strict mode"
This reverts commit d368f12.
1 parent a50611b commit d6f3001

File tree

10 files changed

+87
-77
lines changed

10 files changed

+87
-77
lines changed

‎lib/operations/create_collection.js‎

Lines changed: 52 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,13 @@ 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;
78
const loadCollection = require('../dynamic_loaders').loadCollection;
89
const MongoError = require('../core').MongoError;
910
const ReadPreference = require('../core').ReadPreference;
1011

11-
const ILLEGAL_COMMAND_FIELDS = new Set([
12+
// Filter out any write concern options
13+
const illegalCommandFields = [
1214
'w',
1315
'wtimeout',
1416
'j',
@@ -22,24 +24,27 @@ const ILLEGAL_COMMAND_FIELDS = new Set([
2224
'session',
2325
'readConcern',
2426
'writeConcern'
25-
]);
27+
];
2628

2729
class CreateCollectionOperation extends CommandOperation {
2830
constructor(db, name, options) {
2931
super(db, options);
32+
3033
this.name = name;
3134
}
3235

3336
_buildCommand() {
3437
const name = this.name;
3538
const options = this.options;
3639

40+
// Create collection command
3741
const cmd = { create: name };
42+
// Add all optional parameters
3843
for (let n in options) {
3944
if (
4045
options[n] != null &&
4146
typeof options[n] !== 'function' &&
42-
!ILLEGAL_COMMAND_FIELDS.has(n)
47+
illegalCommandFields.indexOf(n) === -1
4348
) {
4449
cmd[n] = options[n];
4550
}
@@ -52,51 +57,61 @@ class CreateCollectionOperation extends CommandOperation {
5257
const db = this.db;
5358
const name = this.name;
5459
const options = this.options;
55-
const Collection = loadCollection();
56-
57-
let listCollectionOptions = Object.assign({ nameOnly: true, strict: false }, options);
58-
listCollectionOptions = applyWriteConcern(listCollectionOptions, { db }, listCollectionOptions);
5960

60-
function done(err) {
61-
if (err) {
62-
return callback(err);
63-
}
61+
let Collection = loadCollection();
6462

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-
}
63+
// Did the user destroy the topology
64+
if (db.serverConfig && db.serverConfig.isDestroyed()) {
65+
return callback(new MongoError('topology was destroyed'));
7366
}
7467

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);
82-
}
68+
let listCollectionOptions = Object.assign({}, options, { nameOnly: true });
69+
listCollectionOptions = applyWriteConcern(listCollectionOptions, { db }, listCollectionOptions);
8370

84-
if (collections.length > 0) {
85-
return callback(
86-
new MongoError(`Collection ${name} already exists. Currently in strict mode.`)
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)
8791
);
92+
} catch (err) {
93+
return handleCallback(callback, err);
8894
}
95+
}
8996

90-
super.execute(done);
91-
});
92-
93-
return;
94-
}
97+
// Execute command
98+
super.execute(err => {
99+
if (err) return handleCallback(callback, err);
95100

96-
// otherwise just execute the command
97-
super.execute(done);
101+
try {
102+
return handleCallback(
103+
callback,
104+
null,
105+
new Collection(db, db.s.topology, db.databaseName, name, db.s.pkFactory, options)
106+
);
107+
} catch (err) {
108+
return handleCallback(callback, err);
109+
}
110+
});
111+
});
98112
}
99113
}
100114

101115
defineAspects(CreateCollectionOperation, Aspect.WRITE_OPERATION);
116+
102117
module.exports = CreateCollectionOperation;

‎test/examples/change_streams.js‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,8 @@ describe('examples(change-stream):', function() {
1616
client = await this.configuration.newClient().connect();
1717
db = client.db(this.configuration.db);
1818

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();
19+
await db.createCollection('inventory');
20+
await db.collection('inventory').deleteMany({});
2421
});
2522

2623
afterEach(async function() {

‎test/functional/collection.test.js‎

Lines changed: 8 additions & 3 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, ['listCollectionsDb', 'listCollectionsDb2', 'test_db']);
13+
return setupDatabase(configuration);
1414
});
1515

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

211-
done();
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+
});
212217
});
213218
});
214219
});
@@ -744,7 +749,7 @@ describe('Collection', function() {
744749
expect(coll).to.exist;
745750

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

‎test/functional/cursor.test.js‎

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

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

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

40474044
var db = client.db(configuration.db);
4048-
db.createCollection('negative_batch_size_and_limit_set', function(err, collection) {
4045+
db.createCollection('Should_correctly_execute_count_on_cursor_1_', function(
4046+
err,
4047+
collection
4048+
) {
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('execute_find_and_modify', function(err, collection) {
1382+
db.createCollection('shouldCorrectlyExecuteFindAndModify', 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('find_and_modify_generate_correct_bson', function(err, collection) {
1604+
db.createCollection('shouldCorrectlyExecuteFindAndModify', 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: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,10 @@
11
'use strict';
22
var test = require('./shared').assert;
33
var setupDatabase = require('./shared').setupDatabase;
4-
const expect = require('chai').expect;
54

65
describe('MapReduce', function() {
76
before(function() {
8-
return setupDatabase(this.configuration, ['outputCollectionDb']);
7+
return setupDatabase(this.configuration);
98
});
109

1110
/**
@@ -134,9 +133,7 @@ describe('MapReduce', function() {
134133
var client = configuration.newClient(configuration.writeConcernMax(), { poolSize: 1 });
135134
client.connect(function(err, client) {
136135
var db = client.db(configuration.db);
137-
db.createCollection('should_force_map_reduce_error', function(err, collection) {
138-
expect(err).to.not.exist;
139-
136+
db.createCollection('test_map_reduce', function(err, collection) {
140137
collection.insert(
141138
[{ user_id: 1 }, { user_id: 2 }],
142139
configuration.writeConcernMax(),
@@ -372,7 +369,7 @@ describe('MapReduce', function() {
372369
// Create a test collection
373370
db.createCollection('test_map_reduce_functions', function(err, collection) {
374371
// create the output collection
375-
outDb.createCollection('test_map_reduce_functions_temp', err => {
372+
outDb.createCollection('tempCollection', err => {
376373
test.equal(null, err);
377374

378375
// Insert some documents to perform map reduce over
@@ -394,7 +391,7 @@ describe('MapReduce', function() {
394391
collection.mapReduce(
395392
map,
396393
reduce,
397-
{ out: { replace: 'test_map_reduce_functions_temp', db: 'outputCollectionDb' } },
394+
{ out: { replace: 'tempCollection', db: 'outputCollectionDb' } },
398395
function(err, collection) {
399396
test.equal(null, err);
400397

@@ -523,7 +520,7 @@ describe('MapReduce', function() {
523520
collection.mapReduce(
524521
map,
525522
reduce,
526-
{ scope: { util: util }, out: { replace: 'test_map_reduce_temp' } },
523+
{ scope: { util: util }, out: { replace: 'tempCollection' } },
527524
function(err, collection) {
528525
// After MapReduce
529526
test.equal(200, util.times_one_hundred(2));

‎test/functional/multiple_db.test.js‎

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

65
describe('Multiple Databases', function() {
76
before(function() {
8-
return setupDatabase(this.configuration, ['integration_tests2']);
7+
return setupDatabase(this.configuration);
98
});
109

1110
/**
@@ -85,18 +84,16 @@ describe('Multiple Databases', function() {
8584
var second_test_database_client = configuration.newClient({ w: 1 }, { poolSize: 1 });
8685
// Just create second database
8786
client.connect(function(err, client) {
88-
expect(err).to.not.exist;
89-
9087
second_test_database_client.connect(function(err, second_test_database) {
91-
expect(err).to.not.exist;
9288
var db = client.db(configuration.db);
9389
// Close second database
9490
second_test_database.close();
9591
// Let's grab a connection to the different db resusing our connection pools
96-
var secondDb = client.db('integration_tests2');
97-
secondDb.createCollection('same_connection_two_dbs', function(err, collection) {
98-
expect(err).to.not.exist;
99-
92+
var secondDb = client.db(configuration.db_name + '_2');
93+
secondDb.createCollection('shouldCorrectlyUseSameConnectionsForTwoDifferentDbs', function(
94+
err,
95+
collection
96+
) {
10097
// Insert a dummy document
10198
collection.insert({ a: 20 }, { safe: true }, function(err) {
10299
test.equal(null, err);
@@ -106,9 +103,10 @@ describe('Multiple Databases', function() {
106103
test.equal(20, item.a);
107104

108105
// Use the other db
109-
db.createCollection('same_connection_two_dbs', function(err, collection) {
110-
expect(err).to.not.exist;
111-
106+
db.createCollection('shouldCorrectlyUseSameConnectionsForTwoDifferentDbs', function(
107+
err,
108+
collection
109+
) {
112110
// Insert a dummy document
113111
collection.insert({ b: 20 }, { safe: true }, function(err) {
114112
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', 'hr', 'reporting']);
18+
return setupDatabase(this.configuration, ['integration_tests_2']);
1919
});
2020

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

‎test/functional/readconcern.test.js‎

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

476476
// Get a collection using createCollection
477-
db.createCollection('readConcernCollection_createCollection', (err, collection) => {
478-
expect(err).to.not.exist;
479-
477+
db.createCollection('readConcernCollection', (err, collection) => {
480478
// Validate readConcern
481479
expect(collection.readConcern).to.deep.equal({ level: 'local' });
482480
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', { strict: true }, () => {}),
46+
command: db => db.createCollection('foo', () => {}),
4747
listCollectionsValue: true
4848
},
4949
{

0 commit comments

Comments
 (0)