findByIdAndUpdate should not work with undefined id

[fromi]

Do you want to request a feature or report a bug?
Neither a feature or a bug, but an improvement for the API

What is the current behavior?
Actually, findByIdAndUpdate(id, ...) is equivalent to findOneAndUpdate({ _id: id }, ...), as stated by the documentation.
That's working accordingly, however this is not the logically expected behavior when the id is undefined:
findOneAndUpdate({ _id: undefined }, ...) => this form, understandably, should update the first entry.
findByIdAndUpdate(undefined, ...) => this form however should fail.

What is the expected behavior?
I expect an argument error when using findByIdAndUpdate with an undefined id.

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.
Mongoose v5.13.8

[IslandRhythms]

When you call findByIdAndUpdate() its expecting an _id, so you can just pass that in without having to do the {_id: obj._id}. As a result, when you pass in undefined, it turns undefined into that structure like so: {_id: undefined}

[fromi]

Hi @IslandRhythms, I understand that, I am just saying that it is an unexpected behavior, which in my case made a bug much harder to track and to understand than if the id parameter was mandatory (with control checking). I am not saying that there is a bug, I am suggesting an improvement for the API.

[IslandRhythms]

So something that should be mentioned in the docs?

[fromi]

Yes, and also a minor change in the code:

• Inside the code, add something like that in the function:

if (id === undefined) throw new Error('Illegal argument: id cannot be undefined)

• Inside the doc, the error throwing can be documented too.

[vkarpov15]

We took a closer look, and realistically I don't think we should make this particular change. We would have to throw an error in findByIdAndUpdate(), findById(), etc. But then that would break chaining, like Model.findByIdAndUpdate(undefined, { bar: 'baz' }).findByIdAndUpdate('foo');. Admittedly, this is a strange pattern, but it illustrates the fundamental issue with eagerly validating arguments in some cases, when Mongoose relies on lazy validation for queries in all other cases (we don't run query validation until the query actually runs).

If you need Mongoose to throw an error if filtering by an undefined id, you can use middleware as follows:

schema.pre(/.*/, { query: true }, function() {
  if (this.getFilter() && this.getFilter()._id === undefined) {
    throw new Error('Cannot filter by _id === undefined');
  }
});

Below is a working example:

'use strict';

const mongoose = require('mongoose');

const schema = mongoose.Schema({ name: String });

schema.pre(/.*/, { query: true }, function() {
  if (this.getFilter() && this.getFilter()._id === undefined) {
    throw new Error('Cannot filter by _id === undefined');
  }
});

run().catch(err => console.log(err));

async function run() {
  await mongoose.connect('mongodb://127.0.0.1/gh10964');
  const Test = mongoose.model('Test', schema);

  // Throws "Error: Cannot filter by _id === undefined"
  await Test.findByIdAndUpdate(undefined, { name: 'foo' });
  console.log('Done');
}