fix(NODE-3015): ObjectId.equals should use Buffer.equals for better performance (#478)

Author: Uzlopak

src/objectid.ts

@@ -212,7 +212,7 @@ export class ObjectId {
     }
 
     if (otherId instanceof ObjectId) {
-      return this.toString() === otherId.toString();
+      return this[kId][11] === otherId[kId][11] && this[kId].equals(otherId[kId]);
     }
 
     if (
@@ -237,7 +237,9 @@ export class ObjectId {
       'toHexString' in otherId &&
       typeof otherId.toHexString === 'function'
     ) {
-      return otherId.toHexString() === this.toHexString();
+      const otherIdString = otherId.toHexString();
+      const thisIdString = this.toHexString().toLowerCase();
+      return typeof otherIdString === 'string' && otherIdString.toLowerCase() === thisIdString;
     }
 
     return false;

test/node/object_id_tests.js

@@ -3,8 +3,9 @@
 const Buffer = require('buffer').Buffer;
 const BSON = require('../register-bson');
 const BSONTypeError = BSON.BSONTypeError;
-const util = require('util');
 const ObjectId = BSON.ObjectId;
+const util = require('util');
+const getSymbolFrom = require('./tools/utils').getSymbolFrom;
 
 describe('ObjectId', function () {
   it('should correctly handle objectId timestamps', function (done) {
@@ -289,4 +290,97 @@ describe('ObjectId', function () {
       new BSON.ObjectId(BSON.ObjectId.generate(farFuture / 1000)).getTimestamp().getTime()
     ).to.equal(farFuture);
   });
+
+  describe('.equals(otherId)', () => {
+    /*
+     * ObjectId.equals() covers many varieties of cases passed into it In an attempt to handle ObjectId-like objects
+     * Each test covers a corresponding if stmt in the equals method.
+     */
+    const oidBytesInAString = 'kaffeeklatch';
+    const oidString = '6b61666665656b6c61746368';
+    const oid = new ObjectId(oidString);
+    const oidKId = getSymbolFrom(oid, 'id');
+    it('should return false for an undefined otherId', () => {
+      // otherId === undefined || otherId === null
+      expect(oid.equals(null)).to.be.false;
+      expect(oid.equals(undefined)).to.be.false;
+      expect(oid.equals()).to.be.false;
+    });
+
+    it('should return true for another ObjectId with the same bytes', () => {
+      // otherId instanceof ObjectId
+      const equalOid = new ObjectId(oid.id);
+      expect(oid.equals(equalOid)).to.be.true;
+    });
+
+    it('should return true if otherId is a valid 24 char hex string', () => {
+      // typeof otherId === 'string' && ObjectId.isValid(otherId) && otherId.length === 24
+      const equalOid = oidString;
+      expect(oid.equals(equalOid)).to.be.true;
+    });
+
+    it('should return true if otherId is a valid 12 byte string', () => {
+      /*
+      typeof otherId === 'string' &&
+      ObjectId.isValid(otherId) &&
+      otherId.length === 12 &&
+      isUint8Array(this.id)
+      */
+      const equalOid = oidBytesInAString;
+      expect(oid.equals(equalOid)).to.be.true;
+    });
+
+    it.skip('should return true if otherId is a valid 12 byte string and oid.id is not Uint8Array', () => {
+      // typeof otherId === 'string' && ObjectId.isValid(otherId) && otherId.length === 12
+      // Skipped because the check inside of this if statement is incorrect, it will never return true
+      // But it is also unreachable because of the other 12 len case checked before it
+      const equalOid = oidBytesInAString;
+      Object.defineProperty(oid, 'id', { value: oid.toHexString() });
+      expect(oid.equals(equalOid)).to.be.true;
+    });
+
+    it('should return true if otherId is an object with a toHexString function, regardless of casing', () => {
+      /*
+      typeof otherId === 'object' &&
+      'toHexString' in otherId &&
+      typeof otherId.toHexString === 'function'
+      */
+      expect(oid.equals({ toHexString: () => oidString.toLowerCase() })).to.be.true;
+      expect(oid.equals({ toHexString: () => oidString.toUpperCase() })).to.be.true;
+    });
+
+    it('should return false if toHexString does not return a string', () => {
+      // typeof otherIdString === 'string'
+
+      // Now that we call toLowerCase() make sure we guard the call with a type check
+      expect(() => oid.equals({ toHexString: () => 100 })).to.not.throw(TypeError);
+      expect(oid.equals({ toHexString: () => 100 })).to.be.false;
+    });
+
+    it('should not rely on toString for otherIds that are instanceof ObjectId', () => {
+      // Note: the method access the symbol prop directly instead of the getter
+      const equalId = { toString: () => oidString + 'wrong', [oidKId]: oid.id };
+      Object.setPrototypeOf(equalId, ObjectId.prototype);
+      expect(oid.toString()).to.not.equal(equalId.toString());
+      expect(oid.equals(equalId)).to.be.true;
+    });
+
+    it('should use otherId[kId] Buffer for equality when otherId is instanceof ObjectId', () => {
+      let equalId = { [oidKId]: oid.id };
+      Object.setPrototypeOf(equalId, ObjectId.prototype);
+
+      const propAccessRecord = [];
+      equalId = new Proxy(equalId, {
+        get(target, prop, recv) {
+          propAccessRecord.push(prop);
+          return Reflect.get(target, prop, recv);
+        }
+      });
+
+      expect(oid.equals(equalId)).to.be.true;
+      // once for the 11th byte shortcut
+      // once for the total equality
+      expect(propAccessRecord).to.deep.equal([oidKId, oidKId]);
+    });
+  });
 });

test/node/tools/utils.js

@@ -156,3 +156,19 @@ exports.isNode6 = function () {
   // eslint-disable-next-line no-undef
   return process.version.split('.')[0] === 'v6';
 };
+
+const getSymbolFrom = function (target, symbolName, assertExists) {
+  if (assertExists == null) assertExists = true;
+
+  const symbol = Object.getOwnPropertySymbols(target).filter(
+    s => s.toString() === `Symbol(${symbolName})`
+  )[0];
+
+  if (assertExists && !symbol) {
+    throw new Error(`Did not find Symbol(${symbolName}) on ${target}`);
+  }
+
+  return symbol;
+};
+
+exports.getSymbolFrom = getSymbolFrom;