Breaking change in findOne(ObjectId) method from mongoose 5 to mongoose 6

[abarriel]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the bug has not already been reported

Mongoose version

6.5.2

Node.js version

14

MongoDB server version

4.8.1

Description

Hello 👋

The query findOne(ObjectId(id) is not working as expected anymore. Even more it return not the document we are looking for.

I looked in Changelog, and Documation about migrating from 5 to 6, I don't see this documented.

The native driver of mongo handles this correctly and I think mongoose should handle this too.
When I looked a #8268, I understood that was a bug and was fixed on >5.7.

We had a critical bug because a completely different document were returned, please let me knows.
Thank you.

PS:
I understand this is not the best solution to find a document with an ObjectId given, we have findById for that.
But this should throw an error at least, or return a null value but never another document.

Steps to Reproduce

const mongoose = require("mongoose");
const connection = mongoose.createConnection("mongodb://localhost:27017/test");

const userSchema = new mongoose.Schema(
  {
    name: String,
  },
  {
    collection: "usersFid",
    versionKey: false,
  }
);

const testModel = connection.model("User", userSchema);

let testId = null;

async function populateCollection() {
  await Promise.all(
    [...Array(100).keys()].map((idx) =>
      new testModel({ name: `name${idx}` }).save()
    )
  );
}

async function cleanInsertTest() {
  await testModel.deleteMany({});

  await populateCollection();

  testId = (await new testModel({ name: "User To Get" }).save())._id;

  await populateCollection();
}

async function findTest() {
  const result = await testModel.findOne(testId).lean().exec();
  console.log("Search Id", testId.toString()); // NO THE SAME USER !
  console.log("Doc Found:", result); // NO THE SAME USER !

  //in 5.X.X it prints the inserted test user
  //in 6.X.X it prints random user
}

async function run() {
  await cleanInsertTest();
  await findTest();
  connection.close();
}

run();

Expected Behavior

No response

[IslandRhythms]

You're only passing in the object id, you need to pass in the form of {_id: testId} for it to work. When using find and findOne it needs a field name. You can do what you're doing with findById because it is expecting an _id. If I remember correctly, I believe findById will create the _id key if you do not add it yourself. So findById will take testId and do {_id: testId} while find and findOne will not. With that being said, the behavior you expect is on 5.x.x so we will figure out if this was an intentional change or not.

[IslandRhythms]

It is recommended that you use findById()

[abarriel]

I don't quite follow your answer @IslandRhythms . I made it clear that I know this may be not the right way to use findOne, and will recommend to use findById().
I open an issue because for me this is a critical bug, It's not documented that you change the behavior of findOne. And findOne(ObjectId) is working well in mongo drive and mongoose <6.
There is a regression here, and can lead to huge bug.
I can't see why there is such a huge regression when mongo drive natively handle this perfectly, or I least this should be a breaking change.

[abarriel]

Do you understand that we are talking about a find method which return an different document @IslandRhythms ?
Where the same implementation works fine in the native drive and mongoose prior 6.

[IslandRhythms]

The most likely explanation as to why it returns a totally random document is because how find and findOne work. When you pass a field, it will search for documents containing that field. If the field does not exist, mongoose will return a random document. below is an example that further demonstrates this conflict:

const testSchema = new mongoose.Schema({
name: String
})
const Test = mongoose.model('Test', testSchema);

// mongoose interprets this as findOne({name: 'Test'})
await Test.findOne({ name: 'Test'})

// mongoose interprets this as findOne();
await Test.findOne({ nickName: 'test' });

So when you pass in the ObjectId with no key, it interprets it as findOne().
As to your other points, I will have to direct to you @vkarpov15

[abarriel]

@IslandRhythms this is quiet different. There is an option to forbid filtering with an unknown field, you can set strictQuery to throw. In your example, allowing user to find by nickName can be forbid. This is exactly what we do in my company.

[abarriel]

Hello @vkarpov15 , do you think this shouldn't be fix ? It looks like a regression same as #8268.

[abarriel]

Hello, any news pls 👍 @vkarpov15

[jbsulli]

This burned us badly as well. Shouldn't just return a random document if anything was passed.