Fix ObjectId instanceof checks

[noseworthy]

Summary

PR #11841 changed a whole bunch of checks for instanceof ObjectId to
use the isBsonType() function instead. Unfortunately some were missed
in the ./lib/schema/objectid.js file.

Convert the remaining instanceof checks in objectid.js to use the
isBsonType() check instead so that all ObjectIds created from any
bson modules should validate/cast properly.

This was found because my nested document with required ObjectIds
started failing validation when I loaded the documents from JSON files
in my tests using bson.EJSON after the release of 6.3.4 -> 6.3.5.

Examples

This test when run in a project that depends on mongoose and bson
will fail before this fix and work afterward.

const mongoose = require('mongoose');
const { EJSON } = require('bson');

describe('validation of required ObjectId type', () => {
  it('should be able to insert documents that correspond to the schema', async () => {
    const TestDocumentSchema = new mongoose.Schema({
      name: {
        type: mongoose.Schema.Types.String,
        required: true,
      },
      owner: {
        type: mongoose.Schema.Types.ObjectId,
        default: () => mongoose.Types.ObjectId(),
        required: true,
      },
    });

    const TestDocumentModel = mongoose.model(
      'TestDocument',
      TestDocumentSchema,
    );

    const result = await TestDocumentModel.insertMany([
      EJSON.parse(
        EJSON.stringify({
          name: 'Document 1',
          owner: new mongoose.Types.ObjectId(),
        }),
      ),
      EJSON.parse(EJSON.stringify({ name: 'document 2' })),
    ]);

    expect(result).not.toBeNull();
  });
});

[Uzlopak]

Can you add the unit test to avoid again an regression?

[noseworthy]

Can you add the unit test to avoid again an regression?

Sure thing, @Uzlopak. Where should I put it? I was going to add that example test to this MR, and started putting it in test/schema.validation.test.js, but that didn't feel like the right place

[Uzlopak]

Just create a schema.objectid.test.js. there is currently not a very strict Organisation. It is more important to have a test

[noseworthy]

Just create a schema.objectid.test.js. there is currently not a very strict Organisation. It is more important to have a test

Good thing you asked for a test. I can't recreate this issue in this repo, but I keep getting this error in my repository with the exact same test code:

const result = await TestModel.insertMany([
  EJSON.deserialize({ name: 'Document 1', owner: { $oid: '6005c0bb52e4d267b5d96009' } })
]);

TestDocument validation failed: owner: Path `owner` is required.
ValidationError: TestDocument validation failed: owner: Path `owner` is required.
    at model.Object.<anonymous>.Document.invalidate (/Users/noseworthy/Development/X/backend/node_modules/mongoose/lib/document.js:2970:32)
    at /Users/noseworthy/Development/X/backend/node_modules/mongoose/lib/document.js:2759:17
    at /Users/noseworthy/Development/X/backend/node_modules/mongoose/lib/schematype.js:1333:9
    at processTicksAndRejections (node:internal/process/task_queues:78:11)

So there's something weird going on in my repository where the v => v instanceof oid check is returning false for me, but not in the mongoose repo. I'll try and dig a bit deeper.

[noseworthy]

Sorry, @Uzlopak. I can't build a test in this repo that fails before my fix. As @coyotte508 said in the original MR #11841 , this issue is caused by having different instances of bson installed. instanceof is not validating that ObjectIds created by one instance of the dependency match objects created by mongooses instance of bson.

I tried to hack together a test where I tried to artificially break instanceof but wasn't successful. I don't think there's much point in adding a test that doesn't fail before the fix. Any ideas on how I can replicate this?

[AbdelrahmanHafez]

LGTM, thanks! 👍

[Uzlopak]

@AbdelrahmanHafez why did you merge this?

[AbdelrahmanHafez]

@Uzlopak
Because it's an extension of #11841 which didn't include any tests, coming up with test cases for these scenarios is quite difficult, and OP couldn't come up with test cases.