Skip to content
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.

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

BREAKING CHANGE: make model.prototype.deleteOne() return query, not promise #13660

Merged
merged 4 commits into from
Aug 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/helpers/model/applyHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ function applyHooks(model, schema, options) {
continue;
}

applyHooks(childModel, type.schema, options);
applyHooks(childModel, type.schema, { ...options, isChildSchema: true });
if (childModel.discriminators != null) {
const keys = Object.keys(childModel.discriminators);
for (const key of keys) {
Expand Down Expand Up @@ -104,7 +104,8 @@ function applyHooks(model, schema, options) {

objToDecorate.$__originalValidate = objToDecorate.$__originalValidate || objToDecorate.$__validate;

for (const method of ['save', 'validate', 'remove', 'deleteOne']) {
const internalMethodsToWrap = options && options.isChildSchema ? ['save', 'validate', 'deleteOne'] : ['save', 'validate'];
for (const method of internalMethodsToWrap) {
const toWrap = method === 'validate' ? '$__originalValidate' : `$__${method}`;
const wrapped = middleware.
createWrapper(method, objToDecorate[toWrap], null, kareemOptions);
Expand Down
78 changes: 32 additions & 46 deletions lib/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const Document = require('./document');
const DocumentNotFoundError = require('./error/notFound');
const DivergentArrayError = require('./error/divergentArray');
const EventEmitter = require('events').EventEmitter;
const Kareem = require('kareem');
const MongooseBuffer = require('./types/buffer');
const MongooseError = require('./error/index');
const OverwriteModelError = require('./error/overwriteModel');
Expand Down Expand Up @@ -987,18 +988,18 @@ Model.prototype.$__where = function _where(where) {
};

/**
* Removes this document from the db. Equivalent to `.remove()`.
* Delete this document from the db.
*
* #### Example:
*
* product = await product.deleteOne();
* await product.deleteOne();
* await Product.findById(product._id); // null
hasezoey marked this conversation as resolved.
Show resolved Hide resolved
*
* @return {Promise} Promise
* @return {Query} Query
* @api public
*/

Model.prototype.deleteOne = async function deleteOne(options) {
Model.prototype.deleteOne = function deleteOne(options) {
if (typeof options === 'function' ||
typeof arguments[1] === 'function') {
throw new MongooseError('Model.prototype.deleteOne() no longer accepts a callback');
Expand All @@ -1012,51 +1013,36 @@ Model.prototype.deleteOne = async function deleteOne(options) {
this.$session(options.session);
}

const res = await new Promise((resolve, reject) => {
this.$__deleteOne(options, (err, res) => {
if (err != null) {
return reject(err);
}
resolve(res);
});
});

return res;
};

/*!
* ignore
*/

Model.prototype.$__deleteOne = function $__deleteOne(options, cb) {
if (this.$__.isDeleted) {
return immediate(() => cb(null, this));
}

const self = this;
const where = this.$__where();
if (where instanceof MongooseError) {
return cb(where);
if (where instanceof Error) {
throw where;
}

_applyCustomWhere(this, where);

const session = this.$session();
if (!options.hasOwnProperty('session')) {
options.session = session;
}

this[modelCollectionSymbol].deleteOne(where, options).then(
() => {
this.$__.isDeleted = true;
this.$emit('deleteOne', this);
this.constructor.emit('deleteOne', this);
return cb(null, this);
},
err => {
this.$__.isDeleted = false;
cb(err);
const query = self.constructor.deleteOne(where, options);
query.pre(function queryPreDeleteOne(cb) {
self.constructor._middleware.execPre('deleteOne', self, [self], cb);
});
query.pre(function callSubdocPreHooks(cb) {
each(self.$getAllSubdocs(), (subdoc, cb) => {
subdoc.constructor._middleware.execPre('deleteOne', subdoc, [subdoc], cb);
}, cb);
});
query.pre(function skipIfAlreadyDeleted(cb) {
if (self.$__.isDeleted) {
return cb(Kareem.skipWrappedFunction());
}
);
return cb();
});
query.post(function callSubdocPostHooks(cb) {
each(self.$getAllSubdocs(), (subdoc, cb) => {
subdoc.constructor._middleware.execPost('deleteOne', subdoc, [subdoc], {}, cb);
}, cb);
});
query.post(function queryPostDeleteOne(cb) {
self.constructor._middleware.execPost('deleteOne', self, [self], {}, cb);
});

return query;
};

/**
Expand Down
1 change: 0 additions & 1 deletion lib/plugins/index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
'use strict';

exports.removeSubdocs = require('./removeSubdocs');
exports.saveSubdocs = require('./saveSubdocs');
exports.sharding = require('./sharding');
exports.trackTransaction = require('./trackTransaction');
Expand Down
35 changes: 0 additions & 35 deletions lib/plugins/removeSubdocs.js

This file was deleted.

13 changes: 11 additions & 2 deletions lib/query.js
Original file line number Diff line number Diff line change
Expand Up @@ -4336,14 +4336,23 @@ Query.prototype.exec = async function exec(op) {
this._executionStack = new Error();
}

await _executePreExecHooks(this);
let skipWrappedFunction = null;
try {
await _executePreExecHooks(this);
} catch (err) {
if (err instanceof Kareem.skipWrappedFunction) {
skipWrappedFunction = err;
} else {
throw err;
}
}

let res;

let error = null;
try {
await _executePreHooks(this);
res = await this[thunk]();
res = skipWrappedFunction ? skipWrappedFunction.args[0] : await this[thunk]();

for (const fn of this._transforms) {
res = fn(res);
Expand Down
7 changes: 4 additions & 3 deletions test/document.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,10 +153,11 @@ describe('document', function() {

const test = new Test({ x: 'test' });
const doc = await test.save();
await doc.deleteOne();
const q = doc.deleteOne();
assert.ok(q instanceof mongoose.Query, `Expected query, got ${q.constructor.name}`);
await q;
const found = await Test.findOne({ _id: doc._id });
assert.strictEqual(found, null);

});
});

Expand Down Expand Up @@ -9819,7 +9820,7 @@ describe('document', function() {
assert.ok(doc);
});

it('Makes sure pre remove hook is executed gh-9885', async function() {
it('Makes sure pre deleteOne hook is executed (gh-9885)', async function() {
const SubSchema = new Schema({
myValue: {
type: String
Expand Down
4 changes: 2 additions & 2 deletions test/model.middleware.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -447,13 +447,13 @@ describe('model middleware', function() {

await doc.deleteOne();

assert.equal(queryPreCalled, 0);
assert.equal(queryPreCalled, 1);
assert.equal(preCalled, 1);
assert.equal(postCalled, 1);

await Model.deleteOne();

assert.equal(queryPreCalled, 1);
assert.equal(queryPreCalled, 2);
assert.equal(preCalled, 1);
assert.equal(postCalled, 1);
});
Expand Down
9 changes: 6 additions & 3 deletions test/model.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1177,8 +1177,12 @@ describe('Model', function() {
it('errors when id deselected (gh-3118)', async function() {
await BlogPost.create({ title: 1 }, { title: 2 });
const doc = await BlogPost.findOne({ title: 1 }, { _id: 0 });
const err = await doc.deleteOne().then(() => null, err => err);
assert.equal(err.message, 'No _id found on document!');
try {
await doc.deleteOne();
assert.ok(false);
} catch (err) {
assert.equal(err.message, 'No _id found on document!');
}
});

it('should not remove any records when deleting by id undefined', async function() {
Expand Down Expand Up @@ -5458,7 +5462,6 @@ describe('Model', function() {
await doc.deleteOne({ session });
assert.equal(sessions.length, 1);
assert.strictEqual(sessions[0], session);

});

it('set $session() before pre validate hooks run on bulkWrite and insertMany (gh-7769)', async function() {
Expand Down