From 1f4f0bda94d1cb019222b3a69e77404a8cbcdf20 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 20 Jan 2022 00:58:57 +0100 Subject: [PATCH 1/6] improve performance of ObjectId.equals --- src/objectid.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/objectid.ts b/src/objectid.ts index fe2cfa179..0ffebe547 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.id[11] === otherId.id[11] && this.id.equals(otherId.id); } if ( From c910b9bb04afa6ffa6b79e2f011c23031cec1b48 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 24 Jan 2022 16:47:18 -0500 Subject: [PATCH 2/6] test: new equality check and lowercases hex strings --- src/objectid.ts | 4 +- test/node/object_id_tests.js | 91 ++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 1 deletion(-) diff --git a/src/objectid.ts b/src/objectid.ts index 0ffebe547..c10a73c7f 100644 --- a/src/objectid.ts +++ b/src/objectid.ts @@ -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 5a4b18ecd..9be0004ac 100644 --- a/test/node/object_id_tests.js +++ b/test/node/object_id_tests.js @@ -289,4 +289,95 @@ 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); + 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', () => { + const equalId = { toString: () => oidString + 'wrong', id: 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.id Buffer for equality when otherId is instanceof ObjectId', () => { + let equalId = { id: 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(['id', 'id']); + }); + }); }); From c63cabb775100df16ac009b785fa94e9d87b37f6 Mon Sep 17 00:00:00 2001 From: Uzlopak Date: Mon, 24 Jan 2022 23:29:16 +0100 Subject: [PATCH 3/6] Update src/objectid.ts Co-authored-by: Bailey Pearson --- src/objectid.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/objectid.ts b/src/objectid.ts index c10a73c7f..d430b62ac 100644 --- a/src/objectid.ts +++ b/src/objectid.ts @@ -212,7 +212,7 @@ export class ObjectId { } if (otherId instanceof ObjectId) { - return this.id[11] === otherId.id[11] && this.id.equals(otherId.id); + return this.id[11] === otherId.id[11] && this[kId].equals(otherId[kId]); } if ( From 233779e3bc7d899ea5a2831eadc12c3834666ff6 Mon Sep 17 00:00:00 2001 From: Uzlopak Date: Tue, 25 Jan 2022 16:20:40 +0100 Subject: [PATCH 4/6] Update src/objectid.ts --- src/objectid.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/objectid.ts b/src/objectid.ts index d430b62ac..6e88fa57c 100644 --- a/src/objectid.ts +++ b/src/objectid.ts @@ -212,7 +212,7 @@ export class ObjectId { } if (otherId instanceof ObjectId) { - return this.id[11] === otherId.id[11] && this[kId].equals(otherId[kId]); + return this[kId][11] === this[kId][11] && this[kId].equals(otherId[kId]); } if ( From 6f90b65509f3b7d72a9e1b3b54e9a429915c9fcc Mon Sep 17 00:00:00 2001 From: Uzlopak Date: Tue, 25 Jan 2022 17:11:31 +0100 Subject: [PATCH 5/6] Update src/objectid.ts Co-authored-by: Neal Beeken --- src/objectid.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/objectid.ts b/src/objectid.ts index 6e88fa57c..583609cc5 100644 --- a/src/objectid.ts +++ b/src/objectid.ts @@ -212,7 +212,7 @@ export class ObjectId { } if (otherId instanceof ObjectId) { - return this[kId][11] === this[kId][11] && this[kId].equals(otherId[kId]); + return this[kId][11] === otherId[kId][11] && this[kId].equals(otherId[kId]); } if ( From 018e73959cf140a80e925b4d4fccfbe5ea99a4e9 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 25 Jan 2022 13:51:38 -0500 Subject: [PATCH 6/6] test: update tests to correctly set symbol id property --- test/node/object_id_tests.js | 13 ++++++++----- test/node/tools/utils.js | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/test/node/object_id_tests.js b/test/node/object_id_tests.js index 9be0004ac..ff9743244 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) { @@ -298,6 +299,7 @@ describe('ObjectId', function () { 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; @@ -356,14 +358,15 @@ describe('ObjectId', function () { }); it('should not rely on toString for otherIds that are instanceof ObjectId', () => { - const equalId = { toString: () => oidString + 'wrong', id: oid.id }; + // 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.id Buffer for equality when otherId is instanceof ObjectId', () => { - let equalId = { id: oid.id }; + 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 = []; @@ -377,7 +380,7 @@ describe('ObjectId', function () { expect(oid.equals(equalId)).to.be.true; // once for the 11th byte shortcut // once for the total equality - expect(propAccessRecord).to.deep.equal(['id', 'id']); + expect(propAccessRecord).to.deep.equal([oidKId, oidKId]); }); }); }); diff --git a/test/node/tools/utils.js b/test/node/tools/utils.js index ae464833b..b628ec4f1 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;