bug in mongoose.Types.ObjectId.isValid()

[maximilianschmid]

bug in mongoosejs 6.1.6 version:

ObjectId.isValid validates an Object with prop id as a valid ObjectId:
mongoose.Types.ObjectId.isValid({ id: '580e0797bb495f0a200e91ad' }) // true

But in 6.1.5 this is an invalid ObjectId
mongoose.Types.ObjectId.isValid({ id: '580e0797bb495f0a200e91ad' }) // false

[Maximusya]

For me it results in true both for 6.1.5 and 6.1.6:

const mongoose = require('mongoose');

const whatIsThis = { id: '580e0797bb495f0a200e91ad' };
console.log(whatIsThis, 'mongoose.Types.ObjectId.isValid', mongoose.Types.ObjectId.isValid(whatIsThis));

6.1.5:

$ node test.js
{ id: '580e0797bb495f0a200e91ad' } mongoose.Types.ObjectId.isValid true
$ npm ls bson
[email protected] /home/max/dev/mongoose615
└─┬ [email protected]
  ├── [email protected]
  └─┬ [email protected]
    └── [email protected] deduped

6.1.6:

$ node test.js
{ id: '580e0797bb495f0a200e91ad' } mongoose.Types.ObjectId.isValid true
$ npm ls bson
[email protected] /home/max/dev/mongoose616
└─┬ [email protected]
  ├── [email protected]
  └─┬ [email protected]
    └── [email protected] deduped

Note: npm ls bson is displayed because mongoose.Types.ObjectId.isValid is forwarded to bson lib

[maximilianschmid]

@Maximusya thx for the help.

re-checked and found that the isValid changed behaviour from [email protected] to [email protected]

❯ npm ls bson
[email protected] /Users/milian/extendedbrain/planfredapp2020/server
├── [email protected] 
└─┬ UNMET PEER DEPENDENCY [email protected]
  ├── [email protected]  deduped
  └─┬ [email protected]
    └── [email protected]  deduped

npm ERR! peer dep missing: mongoose@^5.6.4, required by [email protected]
❯ node test.js
{ id: '580e0797bb495f0a200e91ad' } mongoose.Types.ObjectId.isValid false

So the issue is caused by bson npm module.

This PR causes the change in the isValid behaviour:
mongodb/js-bson#475

isValid now also accepts ObjectIdLike type, which matches an object like { id: '580e0797bb495f0a200e91ad'}
580e0797bb495f0a200e91ad

Defined in file on line 13:
https://github.com/mongodb/js-bson/blob/master/src/objectid.ts

[maximilianschmid]

This behaviour change seems to be intended by bson module maintainers. So is

{ id: '580e0797bb495f0a200e91ad' }

now to be considered to be a valid ObjectId?

To introduce such a breaking change in a bson patch version update is odd.

[Maximusya]

Right now I am migrating from Mongoose 5.x to 6.x and got about half of integration tests fail just because of undocumented changes in ObjectId validation.
These two mongoose validation methods differ in behavior both between the two in one version of mongoose and between mongoose versions (+dependencies):

• mongoose.Types.ObjectId.isValid
• mongoose.isValidObjectId

I was just about to start a discussion about the issue when I saw your bug report.
MongoDB and mongoose seem to be mature products. And having a long history of changes/bugs with such base entity as ObjectId (regarding what is a valid ObjectId) - is appalling to say the least.

[maximilianschmid]

@Maximusya I think I will add a test to our test suite which validates the isValid() behaviour to guard against such changes.

[maximilianschmid]

@Maximusya I've written a cypress test to check isValid() behaviour:

const mongoose = require('../../../server/node_modules/mongoose/')

describe('ObjectId.isValid() test …', () => {
  it('valid ObjectIds', () => {
    const validObjectIds = [
      '580e0797bb495f0a200e91ad',
      { id: '580e0797bb495f0a200e91ad' },
      { id: Buffer.from([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]) },
      Buffer.from([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
    ]

    validObjectIds.forEach(id => {
      expect(mongoose.Types.ObjectId.isValid(id), JSON.stringify(id)).to.eq(true)
    })
  })

  it('invalid ObjectIds', () => {
    const invalidObjectIds = [
      ' ',
      '123',
      '580e0797bb495f0a200e91a',
      ['580e0797bb495f0a200e91a'],
      { _id: '580e0797bb495f0a200e91ad' },
      { $oid: '580e0797bb495f0a200e91ad' }
    ]

    invalidObjectIds.forEach(id => {
      expect(mongoose.Types.ObjectId.isValid(id), JSON.stringify(id)).to.eq(false)
    })
  })
})

[Maximusya]

I have actually gathered some thoughts here: #11221

[Uzlopak]

Yeah i agree. But the issue is that the bson package is just some dumb patched code. Also it was always(?) "clear" that a jump from bson 1 to bson 4 will break a lot.

Anyway... I agree with you that isValidObjectId and ObjectId.isValid should have the same result.

I think the issue arises from

mongoose/lib/index.js

Line 956 in a8a615c

Mongoose.prototype.isValidObjectId = function(v) {

What should happen is actually using the validation method from bson and not reinventing the wheel again. But to fix that maybe makes sense to use your test cases and integrate them into mongoose and check if the isValidObjectId is behaving the same as ObjectId.isValid.

[vkarpov15]

@maximilianschmid can you please elaborate why this is a breaking change? The intent of isValid() and isValidObjectId() is to check whether a value is something that can be converted to an ObjectId, and growing the set of objects that can be converted to ObjectIds is at worst a semver minor. We can consider explicitly avoiding casting { id: <string> } as an ObjectId if that causes problems.

In the meantime, as per @Uzlopak 's suggestion, we'll make isValidObjectId() consistent with isValid(). There's no reason for us to mix/match rules as to what is a valid ObjectId anymore.