diff --git a/src/objectid.ts b/src/objectid.ts index fe2cfa17..583609cc 100644 --- a/src/objectid.ts +++ b/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; diff --git a/test/node/object_id_tests.js b/test/node/object_id_tests.js index 5a4b18ec..ff974324 100644 --- a/test/node/object_id_tests.js +++ b/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]); + }); + }); }); diff --git a/test/node/tools/utils.js b/test/node/tools/utils.js index ae464833..b628ec4f 100644 --- a/test/node/tools/utils.js +++ b/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;