Fix findByIdAndUpdate behavior with undefined id #11079
Conversation
lib/model.js
Outdated
| @@ -2676,6 +2680,9 @@ Model.findByIdAndUpdate = function(id, update, options, callback) { | |||
| throw new TypeError(msg); | |||
| } | |||
| return this.findOneAndUpdate({ _id: id }, undefined); | |||
| } else if (id === undefined) { | |||
| const msg = 'Model.findByIdAndUpdate(): id cannot be undefined if update is specified.\n'; | |||
There was a problem hiding this comment.
Oh, I was just following the format of the preceding error messages. I went ahead and removed it
lib/model.js
Outdated
| @@ -2676,6 +2680,9 @@ Model.findByIdAndUpdate = function(id, update, options, callback) { | |||
| throw new TypeError(msg); | |||
| } | |||
| return this.findOneAndUpdate({ _id: id }, undefined); | |||
| } else if (id === undefined) { | |||
There was a problem hiding this comment.
Our tests rely on Model.findByIdAndUpdate() with no arguments working, so can we make this else if (arguments.length > 0 && id === undefined)?
There was a problem hiding this comment.
Thank you for bringing this up. I forgot to consider that. I went ahead and added this to the condition
|
@vkarpov15 I pushed some changes that might resolve the failing tests. For the function itself, I added the check for undefined id only if the length of arguments was greater than 1. Moreover, I updated the test I wrote to properly check that the error is thrown. It turns out that my original test was wrong |
|
Unfortunately I have to revert this change - it is backwards breaking as indicated in #11149. We'll consider this for our next major release. |
Summary
findByIdAndUpdate should not work with an undefined id, if an update is specified. Resolves #10964
Examples
I added a test to demonstrate this change.