fix(mongoose): make isValidObjectId() consistent with isValid(), make ObjectId() casting more flexible

vkarpov15 · vkarpov15 · commit 61d01fa5241a · 2022-01-29T21:16:49.000-05:00
Re: Automattic#11209
diff --git a/lib/cast/objectid.js b/lib/cast/objectid.js
@@ -1,7 +1,6 @@
 'use strict';
 
 const ObjectId = require('../driver').get().ObjectId;
-const assert = require('assert');
 
 module.exports = function castObjectId(value) {
   if (value == null) {
@@ -25,5 +24,5 @@ module.exports = function castObjectId(value) {
     return new ObjectId(value.toString());
   }
 
-  assert.ok(false);
+  return new ObjectId(value);
 };
diff --git a/lib/index.js b/lib/index.js
@@ -947,50 +947,21 @@ Mongoose.prototype.ObjectId = SchemaTypes.ObjectId;
  *
  *     mongoose.isValidObjectId(new mongoose.Types.ObjectId()); // true
  *     mongoose.isValidObjectId('0123456789ab'); // true
- *     mongoose.isValidObjectId(6); // false
+ *     mongoose.isValidObjectId(6); // true
+ *     mongoose.isValidObjectId({ test: 42 }); // false
  *
  * @method isValidObjectId
  * @api public
  */
 
 Mongoose.prototype.isValidObjectId = function(v) {
-  if (v == null) {
-    return true;
-  }
-  const base = this || mongoose;
-  const ObjectId = base.driver.get().ObjectId;
-  if (v instanceof ObjectId) {
-    return true;
-  }
-
-  if (v._id != null) {
-    if (v._id instanceof ObjectId) {
-      return true;
-    }
-    if (v._id.toString instanceof Function) {
-      v = v._id.toString();
-      if (typeof v === 'string' && v.length === 12) {
-        return true;
-      }
-      if (typeof v === 'string' && /^[0-9A-Fa-f]{24}$/.test(v)) {
-        return true;
-      }
-      return false;
-    }
-  }
-
-  if (v.toString instanceof Function) {
-    v = v.toString();
-  }
-
-  if (typeof v === 'string' && v.length === 12) {
-    return true;
-  }
-  if (typeof v === 'string' && /^[0-9A-Fa-f]{24}$/.test(v)) {
-    return true;
+  try {
+    new this.Types.ObjectId(v);
+  } catch (err) {
+    return false;
   }
 
-  return false;
+  return true;
 };
 
 Mongoose.prototype.syncIndexes = function() {
diff --git a/test/index.test.js b/test/index.test.js
@@ -699,7 +699,8 @@ describe('mongoose module:', function() {
     assert.ok(mongoose.isValidObjectId('5f5c2d56f6e911019ec2acdc'));
     assert.ok(mongoose.isValidObjectId('608DE01F32B6A93BBA314159'));
     assert.ok(mongoose.isValidObjectId(new mongoose.Types.ObjectId()));
-    assert.ok(!mongoose.isValidObjectId(6));
+    assert.ok(mongoose.isValidObjectId(6));
+    assert.ok(!mongoose.isValidObjectId({ test: 42 }));
   });
   it('global `strictPopulate` works when false (gh-10694)', async function() {
     const mongoose = new Mongoose();
