From 248d57da4ed61e26bd05a42ab0ebc7c02fa26ac5 Mon Sep 17 00:00:00 2001 From: Matthew Gabeler-Lee Date: Wed, 3 Oct 2018 18:17:08 -0400 Subject: [PATCH 1/2] fix: accelerate unique id checking --- lib/relation-definition.js | 11 +++------ lib/utils.js | 46 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index 398019b2..288a5564 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -17,6 +17,7 @@ var defineScope = require('./scope.js').defineScope; var g = require('strong-globalize')(); var mergeQuery = utils.mergeQuery; var idEquals = utils.idEquals; +var idsHaveDuplicates = utils.idsHaveDuplicates; var ModelBaseClass = require('./model.js'); var applyFilter = require('./connectors/memory').applyFilter; var ValidationError = require('./validations.js').ValidationError; @@ -2502,10 +2503,7 @@ RelationDefinition.embedsMany = function embedsMany(modelFrom, modelToRef, param modelFrom.validate(propertyName, function(err) { var embeddedList = this[propertyName] || []; var ids = embeddedList.map(function(m) { return m[idName] && m[idName].toString(); }); // mongodb - var uniqueIds = ids.filter(function(id, pos) { - return utils.findIndexOf(ids, id, idEquals) === pos; - }); - if (ids.length !== uniqueIds.length) { + if (idsHaveDuplicates(ids)) { this.errors.add(propertyName, 'contains duplicate `' + idName + '`', 'uniqueness'); err(false); } @@ -3155,10 +3153,7 @@ RelationDefinition.referencesMany = function referencesMany(modelFrom, modelToRe modelFrom.validate(relationName, function(err) { var ids = this[fk] || []; - var uniqueIds = ids.filter(function(id, pos) { - return utils.findIndexOf(ids, id, idEquals) === pos; - }); - if (ids.length !== uniqueIds.length) { + if (idsHaveDuplicates(ids)) { var msg = 'contains duplicate `' + modelTo.modelName + '` instance'; this.errors.add(relationName, msg, 'uniqueness'); err(false); diff --git a/lib/utils.js b/lib/utils.js index dcd9760b..bb6f638e 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -26,6 +26,7 @@ exports.findIndexOf = findIndexOf; exports.collectTargetIds = collectTargetIds; exports.idName = idName; exports.rankArrayElements = rankArrayElements; +exports.idsHaveDuplicates = idsHaveDuplicates; var g = require('strong-globalize')(); var traverse = require('traverse'); @@ -685,3 +686,48 @@ function collectTargetIds(targetData, idPropertyName) { function idName(m) { return m.definition.idName() || 'id'; } + +/** + * Check a list of IDs to see if there are any duplicates. + * + * @param {Array} The array of IDs to check + * @returns {boolean} If any duplicates were found + */ +function idsHaveDuplicates(ids) { + // use Set if available and all ids are of string or number type + var hasDuplicates = undefined; + var i, j; + if (typeof Set === 'function') { + var uniqueIds = new Set(); + for (i = 0; i < ids.length; ++i) { + var idType = typeof ids[i]; + if (idType === 'string' || idType === 'number') { + if (uniqueIds.has(ids[i])) { + hasDuplicates = true; + break; + } else { + uniqueIds.add(ids[i]); + } + } else { + // ids are not all string/number that can be checked via Set, stop and do the slow test + break; + } + } + if (hasDuplicates === undefined && uniqueIds.length === ids.length) { + hasDuplicates = false; + } + } + if (hasDuplicates === undefined) { + // fast check was inconclusive or unavailable, do the slow check + // can still optimize this by doing 1/2 N^2 instead of the full N^2 + for (i = 0; i < ids.length && hasDuplicates === undefined; ++i) { + for (j = 0; j < i; ++j) { + if (idEquals(ids[i], ids[j])) { + hasDuplicates = true; + break; + } + } + } + } + return hasDuplicates === true; +} From 1ddac5bddb3ba9326911e5703bd7df7fcd785a90 Mon Sep 17 00:00:00 2001 From: Matthew Gabeler-Lee Date: Wed, 3 Oct 2018 18:56:57 -0400 Subject: [PATCH 2/2] fix: add test coverage, correct typo that exposed --- lib/utils.js | 2 +- test/util.test.js | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/lib/utils.js b/lib/utils.js index bb6f638e..8cd3ada8 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -713,7 +713,7 @@ function idsHaveDuplicates(ids) { break; } } - if (hasDuplicates === undefined && uniqueIds.length === ids.length) { + if (hasDuplicates === undefined && uniqueIds.size === ids.length) { hasDuplicates = false; } } diff --git a/test/util.test.js b/test/util.test.js index 9da2d512..132bdf64 100644 --- a/test/util.test.js +++ b/test/util.test.js @@ -559,3 +559,35 @@ describe('util.hasRegExpFlags', function() { }); }); }); + +describe('util.idsHaveDuplicates', function() { + context('with string IDs', function() { + it('should be true with a duplicate present', function() { + utils.idsHaveDuplicates(['a', 'b', 'a']).should.be.ok; + }); + + it('should be false when no duplicates are present', function() { + utils.idsHaveDuplicates(['a', 'b', 'c']).should.not.be.ok; + }); + }); + + context('with numeric IDs', function() { + it('should be true with a duplicate present', function() { + utils.idsHaveDuplicates([1, 2, 1]).should.be.ok; + }); + + it('should be false when no duplicates are present', function() { + utils.idsHaveDuplicates([1, 2, 3]).should.not.be.ok; + }); + }); + + context('with complex IDs', function() { + it('should be true with a duplicate present', function() { + utils.idsHaveDuplicates(['a', 'b', 'a'].map(id => ({id}))).should.be.ok; + }); + + it('should be false when no duplicates are present', function() { + utils.idsHaveDuplicates(['a', 'b', 'c'].map(id => ({id}))).should.not.be.ok; + }); + }); +});