Skip to content

Commit

Permalink
fix(query): support not passing any arguments to orFail()
Browse files Browse the repository at this point in the history
Fix #7409
  • Loading branch information
vkarpov15 committed Jan 17, 2019
1 parent 0348984 commit 12be7a6
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 17 deletions.
10 changes: 6 additions & 4 deletions lib/error/notFound.js
Expand Up @@ -13,15 +13,15 @@ const util = require('util');
* @inherits MongooseError
*/

function DocumentNotFoundError(query) {
function DocumentNotFoundError(filter) {
let msg;
const messages = MongooseError.messages;
if (messages.DocumentNotFoundError != null) {
msg = typeof messages.DocumentNotFoundError === 'function' ?
messages.DocumentNotFoundError(query) :
messages.DocumentNotFoundError(filter) :
messages.DocumentNotFoundError;
} else {
msg = 'No document found for query "' + util.inspect(query) + '"';
msg = 'No document found for query "' + util.inspect(filter) + '"';

This comment has been minimized.

Copy link
@piranna

piranna Jan 17, 2019

Shouldn't the word query being changed to filter, too?

This comment has been minimized.

Copy link
@vkarpov15

vkarpov15 Jan 19, 2019

Author Collaborator

I wanted to avoid changing the error message in a point release. Just in case someone relies on the format of the error message. filter is the more correct term but we still use query in other places. Consider this change a small bit of internal refactor that will help us see how viable it is to change to using filter across the board.

This comment has been minimized.

Copy link
@piranna

piranna Jan 20, 2019

Make sense, let's left it for a major one.

}

MongooseError.call(this, msg);
Expand All @@ -33,7 +33,9 @@ function DocumentNotFoundError(query) {
this.stack = new Error().stack;
}

this.query = query;
this.filter = filter;
// Backwards compat
this.query = filter;
}

/*!
Expand Down
37 changes: 24 additions & 13 deletions lib/query.js
Expand Up @@ -5,6 +5,7 @@
*/

const CastError = require('./error/cast');
const DocumentNotFoundError = require('./error/notFound');
const Kareem = require('kareem');
const ObjectParameterError = require('./error/objectParameter');
const QueryCursor = require('./cursor/QueryCursor');
Expand Down Expand Up @@ -3903,7 +3904,7 @@ Query.prototype.map = function(fn) {
* @method orFail
* @memberOf Query
* @instance
* @param {Function|Error} [err] optional error to throw if no docs match `filter`
* @param {Function|Error} [err] optional error to throw if no docs match `filter`. If not specified, `orFail()` will throw a `DocumentNotFoundError`
* @return {Query} this
*/

Expand All @@ -3912,42 +3913,36 @@ Query.prototype.orFail = function(err) {
switch (this.op) {
case 'find':
if (res.length === 0) {
err = typeof err === 'function' ? err.call(this) : err;
throw err;
throw _orFailError(err, this);
}
break;
case 'findOne':
if (res == null) {
err = typeof err === 'function' ? err.call(this) : err;
throw err;
throw _orFailError(err, this);
}
break;
case 'update':
case 'updateMany':
case 'updateOne':
if (get(res, 'result.nModified') === 0) {
err = typeof err === 'function' ? err.call(this) : err;
throw err;
throw _orFailError(err, this);
}
break;
case 'findOneAndDelete':
if (get(res, 'lastErrorObject.n') === 0) {
err = typeof err === 'function' ? err.call(this) : err;
throw err;
throw _orFailError(err, this);
}
break;
case 'findOneAndUpdate':
if (get(res, 'lastErrorObject.updatedExisting') === false) {
err = typeof err === 'function' ? err.call(this) : err;
throw err;
throw _orFailError(err, this);
}
break;
case 'deleteMany':
case 'deleteOne':
case 'remove':
if (res.n === 0) {
err = typeof err === 'function' ? err.call(this) : err;
throw err;
throw _orFailError(err, this);
}
break;
default:
Expand All @@ -3959,6 +3954,22 @@ Query.prototype.orFail = function(err) {
return this;
};

/*!
* Get the error to throw for `orFail()`
*/

function _orFailError(err, query) {
if (typeof err === 'function') {
err = err.call(query);
}

if (err == null) {

This comment has been minimized.

Copy link
@piranna

piranna Jan 17, 2019

Allowing the usage of a function to fully get control and let it to return undefined as a way to let it to swallow errors would have been interesting, but maybe yes, it makes more sense to always throw an Error and let the swallowing to the application instead of left open a way to have hidding bugs...

This comment has been minimized.

Copy link
@vkarpov15

vkarpov15 Jan 19, 2019

Author Collaborator

It might be interesting for a future release, but the priority here was to make orFail() do something sensible if you don't pass any args. Supporting cancelling the orFail() error is something interesting to add in the future, but outside the scope of this issue IMO.

This comment has been minimized.

Copy link
@piranna

piranna Jan 20, 2019

Yeah, just only I wanted to left the comment, to see if it make sense to you. Maybe there would be a safer way than left it as a special case.

err = new DocumentNotFoundError(query.getQuery());
}

return err;
}

/**
* Executes the query
*
Expand Down

0 comments on commit 12be7a6

Please sign in to comment.