Skip to content

Commit

Permalink
refactor!: remove strict/callback mode from Db.collection helper (#2817)
Browse files Browse the repository at this point in the history 
Strict mode has been removed from the `Db.collection` helper,
as well as the callback argument which was required by strict mode.

`Db.createCollection` can be used when a collection needs to be
created explicitly.

NODE-2752
  • Loading branch information
@emadum
emadum committed May 28, 2021
1 parent 734b481 commit 53abfe7
Show file tree
Hide file tree
Showing 17 changed files with 635 additions and 952 deletions.
2 changes: 0 additions & 2 deletions src/collection.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,6 @@ export interface CollectionOptions
WriteConcernOptions,
LoggerOptions {
slaveOk?: boolean;
/** Returns an error if the collection does not exist */
strict?: boolean;
/** Specify a read concern for the collection. (only MongoDB 3.2 or higher supported) */
readConcern?: ReadConcernLike;
/** The preferred read preference (ReadPreference.PRIMARY, ReadPreference.PRIMARY_PREFERRED, ReadPreference.SECONDARY, ReadPreference.SECONDARY_PREFERRED, ReadPreference.NEAREST). */
Expand Down
74 changes: 10 additions & 64 deletions src/db.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -311,77 +311,23 @@ export class Db {
}

/**
* Fetch a specific collection (containing the actual collection information). If the application does not use strict mode you
* can use it without a callback in the following way: `const collection = db.collection('mycollection');`
* Returns a reference to a MongoDB Collection. If it does not exist it will be created implicitly.
*
* @param name - the collection name we wish to access.
* @returns return the new Collection instance if not in strict mode
* @returns return the new Collection instance
*/
collection<TSchema extends Document = Document>(name: string): Collection<TSchema>;
collection<TSchema extends Document = Document>(
name: string,
options: CollectionOptions
): Collection<TSchema>;
collection<TSchema extends Document = Document>(
name: string,
callback: Callback<Collection<TSchema>>
): void;
collection<TSchema extends Document = Document>(
name: string,
options: CollectionOptions,
callback: Callback<Collection<TSchema>>
): void;
collection<TSchema extends Document = Document>(
name: string,
options?: CollectionOptions | Callback<Collection<TSchema>>,
callback?: Callback<Collection<TSchema>>
): Collection<TSchema> | void {
if (typeof options === 'function') (callback = options), (options = {});
const finalOptions = resolveOptions(this, options);

// Execute
if (!finalOptions.strict) {
try {
const collection = new Collection<TSchema>(this, name, finalOptions);
if (callback) callback(undefined, collection);
return collection;
} catch (err) {
if (err instanceof MongoError && callback) return callback(err);
throw err;
}
}

// Strict mode
if (typeof callback !== 'function') {
throw new MongoError(
`A callback is required in strict mode. While getting collection ${name}`
);
options?: CollectionOptions
): Collection<TSchema> {
if (!options) {
options = {};
} else if (typeof options === 'function') {
throw new TypeError('The callback form of this helper has been removed.');
}

// Did the user destroy the topology
if (getTopology(this).isDestroyed()) {
return callback(new MongoError('topology was destroyed'));
}

const listCollectionOptions: ListCollectionsOptions = Object.assign({}, finalOptions, {
nameOnly: true
});

// Strict mode
this.listCollections({ name }, listCollectionOptions).toArray((err, collections) => {
if (callback == null) return;
if (err != null || !collections) return callback(err);
if (collections.length === 0)
return callback(
new MongoError(`Collection ${name} does not exist. Currently in strict mode.`)
);

try {
return callback(undefined, new Collection(this, name, finalOptions));
} catch (err) {
return callback(err);
}
});
const finalOptions = resolveOptions(this, options);
return new Collection<TSchema>(this, name, finalOptions);
}

/**
Expand Down
50 changes: 24 additions & 26 deletions test/functional/aggregation.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -806,37 +806,35 @@ describe('Aggregation', function () {
expect(err).to.not.exist;

var db = client.db(databaseName);
db.collection('te.st', function (err, col) {
const col = db.collection('te.st');
var count = 0;

col.insertMany([{ a: 1 }, { a: 1 }, { a: 1 }], function (err, r) {
expect(err).to.not.exist;
var count = 0;
expect(r).property('insertedCount').to.equal(3);

col.insertMany([{ a: 1 }, { a: 1 }, { a: 1 }], function (err, r) {
expect(err).to.not.exist;
expect(r).property('insertedCount').to.equal(3);
const cursor = col.aggregate([{ $project: { a: 1 } }]);

const cursor = col.aggregate([{ $project: { a: 1 } }]);
cursor.toArray(function (err, docs) {
expect(err).to.not.exist;
expect(docs.length).to.be.greaterThan(0);

cursor.toArray(function (err, docs) {
expect(err).to.not.exist;
expect(docs.length).to.be.greaterThan(0);

//Using cursor - KO
col
.aggregate([{ $project: { a: 1 } }], {
cursor: { batchSize: 10000 }
})
.forEach(
function () {
count = count + 1;
},
function (err) {
expect(err).to.not.exist;
expect(count).to.be.greaterThan(0);
//Using cursor - KO
col
.aggregate([{ $project: { a: 1 } }], {
cursor: { batchSize: 10000 }
})
.forEach(
function () {
count = count + 1;
},
function (err) {
expect(err).to.not.exist;
expect(count).to.be.greaterThan(0);

client.close(done);
}
);
});
client.close(done);
}
);
});
});
});
Expand Down
130 changes: 47 additions & 83 deletions test/functional/collection.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,35 +83,32 @@ describe('Collection', function () {
db.createCollection('test.spiderman', () => {
db.createCollection('test.mario', () => {
// Insert test documents (creates collections)
db.collection('test.spiderman', (err, spiderman_collection) => {
spiderman_collection.insertOne({ foo: 5 }, configuration.writeConcernMax(), err => {
const spiderman_collection = db.collection('test.spiderman');
spiderman_collection.insertOne({ foo: 5 }, configuration.writeConcernMax(), err => {
expect(err).to.not.exist;
const mario_collection = db.collection('test.mario');
mario_collection.insertOne({ bar: 0 }, configuration.writeConcernMax(), err => {
expect(err).to.not.exist;
db.collection('test.mario', (err, mario_collection) => {
mario_collection.insertOne({ bar: 0 }, configuration.writeConcernMax(), err => {
expect(err).to.not.exist;
// Assert collections
db.collections((err, collections) => {
expect(err).to.not.exist;
// Assert collections
db.collections((err, collections) => {
expect(err).to.not.exist;

let found_spiderman = false;
let found_mario = false;
let found_does_not_exist = false;

collections.forEach(collection => {
if (collection.collectionName === 'test.spiderman') {
found_spiderman = true;
}
if (collection.collectionName === 'test.mario') found_mario = true;
if (collection.collectionName === 'does_not_exist')
found_does_not_exist = true;
});
let found_spiderman = false;
let found_mario = false;
let found_does_not_exist = false;

expect(found_spiderman).to.be.true;
expect(found_mario).to.be.true;
expect(found_does_not_exist).to.be.false;
done();
});
collections.forEach(collection => {
if (collection.collectionName === 'test.spiderman') {
found_spiderman = true;
}
if (collection.collectionName === 'test.mario') found_mario = true;
if (collection.collectionName === 'does_not_exist') found_does_not_exist = true;
});

expect(found_spiderman).to.be.true;
expect(found_mario).to.be.true;
expect(found_does_not_exist).to.be.false;
done();
});
});
});
Expand Down Expand Up @@ -166,23 +163,6 @@ describe('Collection', function () {
});
});

it('should ensure strict access collection', function (done) {
db.collection('does-not-exist', { strict: true }, err => {
expect(err).to.be.an.instanceof(Error);
expect(err.message).to.equal(
'Collection does-not-exist does not exist. Currently in strict mode.'
);
db.createCollection('test_strict_access_collection', err => {
expect(err).to.not.exist;
db.collection('test_strict_access_collection', configuration.writeConcernMax(), err => {
expect(err).to.not.exist;
// Let's close the db
done();
});
});
});
});

it('should fail to insert due to illegal keys', function (done) {
db.createCollection('test_invalid_key_names', (err, collection) => {
// Legal inserts
Expand Down Expand Up @@ -280,39 +260,25 @@ describe('Collection', function () {
});

it('should fail due to illegal listCollections', function (done) {
db.collection(5, err => {
expect(err.message).to.equal('collection name must be a String');
});

db.collection('', err => {
expect(err.message).to.equal('collection names cannot be empty');
});

db.collection('te$t', err => {
expect(err.message).to.equal("collection names must not contain '$'");
});

db.collection('.test', err => {
expect(err.message).to.equal("collection names must not start or end with '.'");
});

db.collection('test.', err => {
expect(err.message).to.equal("collection names must not start or end with '.'");
});

db.collection('test..t', err => {
expect(err.message).to.equal('collection names cannot be empty');
done();
});
expect(() => db.collection(5)).to.throw('collection name must be a String');
expect(() => db.collection('')).to.throw('collection names cannot be empty');
expect(() => db.collection('te$t')).to.throw("collection names must not contain '$'");
expect(() => db.collection('.test')).to.throw(
"collection names must not start or end with '.'"
);
expect(() => db.collection('test.')).to.throw(
"collection names must not start or end with '.'"
);
expect(() => db.collection('test..t')).to.throw('collection names cannot be empty');
done();
});

it('should correctly count on non-existent collection', function (done) {
db.collection('test_multiple_insert_2', (err, collection) => {
collection.countDocuments((err, count) => {
expect(count).to.equal(0);
// Let's close the db
done();
});
const collection = db.collection('test_multiple_insert_2');
collection.countDocuments((err, count) => {
expect(count).to.equal(0);
// Let's close the db
done();
});
});

Expand Down Expand Up @@ -351,22 +317,20 @@ describe('Collection', function () {
});

it('should perform collection remove with no callback', function (done) {
db.collection('remove_with_no_callback_bug_test', (err, collection) => {
const collection = db.collection('remove_with_no_callback_bug_test');
collection.insertOne({ a: 1 }, configuration.writeConcernMax(), err => {
expect(err).to.not.exist;
collection.insertOne({ a: 1 }, configuration.writeConcernMax(), err => {
collection.insertOne({ b: 1 }, configuration.writeConcernMax(), err => {
expect(err).to.not.exist;
collection.insertOne({ b: 1 }, configuration.writeConcernMax(), err => {
collection.insertOne({ c: 1 }, configuration.writeConcernMax(), err => {
expect(err).to.not.exist;
collection.insertOne({ c: 1 }, configuration.writeConcernMax(), err => {
collection.remove({ a: 1 }, configuration.writeConcernMax(), err => {
expect(err).to.not.exist;
collection.remove({ a: 1 }, configuration.writeConcernMax(), err => {
// Let's perform a count
collection.countDocuments((err, count) => {
expect(err).to.not.exist;
// Let's perform a count
collection.countDocuments((err, count) => {
expect(err).to.not.exist;
expect(count).to.equal(2);
done();
});
expect(count).to.equal(2);
done();
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion test/functional/connection.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ describe('Connection - functional', function () {
expect(err).to.not.exist;
var db = client.db(configuration.db);

db.collection(testName, function (err, collection) {
db.createCollection(testName, function (err, collection) {
expect(err).to.not.exist;

collection.insert({ foo: 123 }, { writeConcern: { w: 1 } }, function (err) {
Expand Down
Loading

0 comments on commit 53abfe7

Please sign in to comment.