Skip to content

refactor!: remove strict/callback mode from Db.collection helper #2817

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 28, 2021
Merged

refactor!: remove strict/callback mode from Db.collection helper #2817

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c7971d8
refactor!: remove strict mode and callback from db.collection helper
emadum May 25, 2021
263c0e1
test clean up, first pass
emadum May 25, 2021
036fcd1
test clean up, second pass
emadum May 25, 2021
b09b588
fix lint
emadum May 25, 2021
146100e
fix test
emadum May 25, 2021
ec79e4f
Merge remote-tracking branch 'origin/4.0' into NODE-2752/remove-stric…
emadum May 25, 2021
a3b0ca5
review feedback
emadum May 25, 2021
c6b3892
add test for error
emadum May 26, 2021
7ea428f
remove deprecated examples
emadum May 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/collection.ts
View file
Open in desktop
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
Open in desktop
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.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dariakp Just making sure we catch this, depending on who merges first this is a new error to convert.
Maybe this is best as a MongoError?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will likely go in first so I'll make a note to rebase my branch and check for any added TypeError/MongoError

}

// 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
Open in desktop
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
Open in desktop
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
Open in desktop
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