[6.x behavior change due to change in bson] Mongoose Types.ObjectId.isValid is now more lax, and now returns true for objects that are "new ObjectId"-able

[ronjouch]

Feature request or bug?

Bug / behavior change in mongoose 6.x, which brings mongodb 4.x, which brings bson 4.x

Behavior change

Mongoose exposes Mongo ObjectId as Types.ObjectId and its isValid method itself coming from bson, and its behavior changed in the latest release.

In OLD Mongoose 5.x / mongodb 3.x / bson 1.x,

Welcome to Node.js v14.19.0.

> let o = { _id: new require('.').Types.ObjectId('123456123456123456123456'), id: '123456123456123456123456', __v: 0 }

> require('bson/package.json').version
'1.1.6'

> require('bson').ObjectId.isValid(o)
false

And that makes sense, in the sense that this o object is not a valid ObjectId. It's an object that can definitely, with enough effort, be interpreted as an ObjectId, but it's not an ObjectId. Let's look at the bson v1.1.6 code:

https://github.com/mongodb/js-bson/blob/v1.1.6/lib/bson/objectid.js#L334-L368

Now, in NEW Mongoose 6.x / mongodb 4.x / bson 4.x,

Welcome to Node.js v14.19.0.

> let o = { _id: new require('.').Types.ObjectId('123456123456123456123456'), id: '123456123456123456123456', __v: 0 }

> require('bson/package.json').version
'4.6.1'

> require('bson').ObjectId.isValid(o)
true

And that kinda makes sense too, if we interpret isValid as "can be interpreted as an ObjectId with enough effort. But that's a behavior change. Let's look at the less strict bson v4.6.1 code:

https://github.com/mongodb/js-bson/blob/v4.6.1/src/objectid.ts#L288-L302

Last words

I agree that, strictly speaking, this isn't Mongoose we're talking about here, it's bson.

That being said, Mongoose does expose directly bson's isValid method as mongoose Types.ObjectId.isValid, so from my mongoose user's perspective it's a breaking change, one that breaks my code testing if a thing is an ID or a populated object. So, I'd loooove to be informed about the behavior change in the release notes.

→ Same request as in #11038 : can you document the behavior change in your Migrating to 6.x guide ?

Thanks for Mongoose!

[ronjouch]

Whoopsie, should have commented in dup issue #11301 . Sorry, forgot to search before posting. Well, I think my bug:

• Explains why it's a breaking change especially in a Mongoose context where as a user I sometimes need to differentiate between an ID and a populated object (see "Last words"). I want to know if I have the full object already, or if I have just an ID needing fetching.
• Suggests to document the change

As for a workaround, do you have anything to suggest to differentiate between a full object and an "ID-ish" (objectId or string with right shape)? For now, I just inlined bson 1.1.6's isValid 🤷

---

And that bug reminded of Hyrum's Law:

With a sufficient number of users of an API,
it does not matter what you promise in the contract:
all observable behaviors of your system
will be depended on by somebody.

[Maximusya]

@ronjouch PTAL #11227

[ronjouch]

@Maximusya thanks. Yeah, did the same as you:

Meanwhile in our business code we resorted to our own very narrow guard:

export const isValidObjectId = value => (
  value != null
  && (
    value instanceof mongoose.Types.ObjectId
    || (typeof value === 'string' && value.length === 24 && /^\[0-9A-Fa-f\]{24}$/.test(value))
  )
);

@ Mongoose maintainers: leaving this issue open, as it documents the reason to want the old behavior but not the new one (wanting to differentiate between "just an ID-ish ObjectID/string" vs. a "full document with data"), and it pushes forwards the extra requests to:

1. Document this breaking change in the “Migrating to 6.x guide”.
2. Provide a replacement for the current behavior, or suggest to inline (what @Maximusya & I did, copy-pasta-ing or taking inspiration on [email protected]'s behavior)

[vkarpov15]

We're adding isObjectIdOrHexString() for v6.2.5 as a workaround for the use case of "is this an ObjectId or 24 char hex string?" This issue has caused a lot of confusion, so we're just going to split out isValidObjectId() and isObjectIdOrHexString(). isValidObjectId() is consistent with ObjectIs.isValid(), isObjectIdOrHexString() checks for instanceof ObjectId || /^[0-9A-Fa-f]{24}$/.test()

[ronjouch]

We're adding isObjectIdOrHexString() for v6.2.5 as a workaround for the use case of "is this an ObjectId or 24 char hex string?" This issue has caused a lot of confusion, so we're just going to split out isValidObjectId() and isObjectIdOrHexString(). isValidObjectId() is consistent with ObjectIs.isValid(), isObjectIdOrHexString() checks for instanceof ObjectId || /^[0-9A-Fa-f]{24}$/.test()

@vkarpov15 perfect. As soon as 6.2.5 is released, will de-inline my isValidObjectId and switch to 6.2.5's isObjectIdOrHexString. As usual, thanks for listening 🙂.

[ronjouch]

@vkarpov15 one more thing.

> require('mongoose').isObjectIdOrHexString('123456123456123456123456')
true

> require('mongoose').isObjectIdOrHexString('12345612345612345612345z')
false

> require('mongoose').isObjectIdOrHexString(new require('mongoose').Types.ObjectId())
true

are all great. But shouldn't the below be true? It was with the old 5.x ObjectId.isValid implem:

> require('mongoose').isObjectIdOrHexString(new require('mongodb').ObjectID())
false

[vkarpov15]

@ronjouch that depends on how you've installed mongodb. If you're using an incompatible version of the MongoDB node driver, then you may have a different ObjectId class floating around. Can you show the output of npm list | grep "mongo"?

[ronjouch]

@ronjouch that depends on how you've installed mongodb. If you're using an incompatible version of the MongoDB node driver, then you may have a different ObjectId class floating around. Can you show the output of npm list | grep "mongo"?

@vkarpov15 sure. npm list | grep mongo isn't super helpful, as it trims too much and makes hierarchies difficult to read. So, here's an npm list that I manually trimmed, keeping only mongo* leafs:

npm list manually trimmed to keep only mongo leafs (click to expand)

[email protected] /home/ronanj/work/myapp
├─┬ [email protected]
│ ├── [email protected] deduped
│ ...
├─┬ [email protected]
│ ├─┬ [email protected]
│ │ └─┬ [email protected]
│ │   ├── [email protected] deduped
│ │   └── [email protected] deduped
│ ├── [email protected]
│ ├─┬ [email protected]
│ │ ├── [email protected] deduped
│ │ ├── [email protected]
│ │ ├─┬ [email protected]
│ │ │ ├─┬ @types/[email protected]
│ │ │ │ ├── @types/[email protected] deduped
│ │ │ │ └── @types/[email protected]
│ │ │ └─┬ [email protected]
│ │ │   ├─┬ [email protected]
│ │ │   │ └── [email protected]
│ │ │   └── [email protected]
│ │ ├─┬ [email protected]
│ │ │ └─┬ [email protected]
│ │ │   └── [email protected]
│ │ └─┬ [email protected]
│ │   ├── [email protected]
│ │   └── [email protected]
│ ...
...

Do you see anything wrong? I don't:

• Both myapp and myapp_internal_dependency depend on [email protected], and in myapp_internal_dependency it's deduped from myapp
• Neither myapp nor myapp_internal_dependency depend on mongodb; we let mongoose bring the mongodb it needs.

That being said, I tried to repro the issue in a fresh new folder where I just npm i mongoose, and I confirm that I get a correct

> [require('mongoose').isObjectIdOrHexString(new require('mongoose').Types.ObjectId()), require('mongoose').isObjectIdOrHexString(new require('mongodb').ObjectID())]
[ true, true ]

→ Any suggestion for further local troubleshooting?

[vkarpov15]

@ronjouch I set up an attempt at similar project, with an npm module that lists Mongoose as a dependency. isObjectIdOrHexString() gives the correct result as shown below:

$ npm list
/home/val/Workspace/MongoDB/troubleshoot-mongoose
├─┬ [email protected]
│ ├─┬ [email protected]
│ │ └─┬ [email protected]
│ │   ├── [email protected]
│ │   └── [email protected]
│ ├── [email protected]
│ ├─┬ [email protected]
│ │ ├── [email protected] deduped
│ │ ├── [email protected]
│ │ ├─┬ [email protected]
│ │ │ ├─┬ @types/[email protected]
│ │ │ │ ├── @types/[email protected]
│ │ │ │ └── @types/[email protected]
│ │ │ └─┬ [email protected]
│ │ │   ├─┬ [email protected]
│ │ │   │ └── [email protected]
│ │ │   └── [email protected]
│ │ ├─┬ [email protected]
│ │ │ └─┬ [email protected]
│ │ │   └── [email protected]
│ │ └─┬ [email protected]
│ │   ├── [email protected]
│ │   └── [email protected]
│ ├── [email protected]
│ ├─┬ [email protected]
│ │ └─┬ [email protected]
│ │   └── [email protected]
│ ├── [email protected]
│ └── [email protected]
├─┬ [email protected]
│ ├── [email protected]
│ ├── [email protected] deduped
│ └── [email protected]
└── [email protected] extraneous

npm ERR! extraneous: [email protected] /home/val/Workspace/MongoDB/troubleshoot-mongoose/node_modules/typescript
val@musashi:~/Workspace/MongoDB/troubleshoot-mongoose$ node
Welcome to Node.js v14.18.2.
Type ".help" for more information.
> const mongodb = require('mongodb')
undefined
> const mongoose = require('mongoose')
undefined
> mongoose.isValidObjectId(new mongodb.ObjectId())
true
> mongoose.isObjectIdOrHexString(new mongodb.ObjectId())
true
> mongoose.isObjectIdOrHexString(new require('mongodb').ObjectID())
true

Maybe try putting Mongoose in peerDependencies in your internal dep.

[ronjouch]

Thanks for trying, Valeri.

I monkeyed around again a bit this morning, without success. Closing for now, will re-open if I have something more reproducible.