diff --git a/lib/base/collection.js b/lib/base/collection.js index bcd05f55..629a71a7 100644 --- a/lib/base/collection.js +++ b/lib/base/collection.js @@ -142,6 +142,18 @@ CollectionBase.prototype.idAttribute = function() { return this.model.prototype.idAttribute; }; +/** + * @method + * @private + * @description + * When keying a collection by ID, ensure that it is safe to use as a key + * @param {any} id + * @return {string|number} The id safe for using as a key in a collection + */ +CollectionBase.prototype.idKey = function(id) { + return _.isBuffer(id) ? id.toString('hex') : id; +}; + CollectionBase.prototype.toString = function() { return '[Object Collection]'; }; @@ -263,8 +275,8 @@ CollectionBase.prototype.set = function(models, options) { } else if (options.add) { if (!(model = this._prepareModel(attrs, options))) continue; toAdd.push(model); - this._byId[model.cid] = model; - if (model.id != null) this._byId[model.id] = model; + this._byId[this.idKey(model.cid)] = model; + if (model.id != null) this._byId[this.idKey(model.id)] = model; } if (order && !(existing && order.indexOf(existing) > -1)) order.push(existing || model); @@ -445,7 +457,7 @@ CollectionBase.prototype.remove = function(models, options) { for (let i = 0; i < models.length; i++) { const model = (models[i] = this.get(models[i])); if (!model) continue; - delete this._byId[model.id]; + delete this._byId[this.idKey(model.id)]; delete this._byId[model.cid]; const index = this.indexOf(model); this.models.splice(index, 1); @@ -549,7 +561,7 @@ CollectionBase.prototype.slice = function() { */ CollectionBase.prototype.get = function(obj) { if (obj == null) return void 0; - return this._byId[obj.id] || this._byId[obj.cid] || this._byId[obj]; + return this._byId[this.idKey(obj.id)] || this._byId[obj.cid] || this._byId[this.idKey(obj)]; }; /** diff --git a/lib/relation.js b/lib/relation.js index 06955b9a..64a653b5 100644 --- a/lib/relation.js +++ b/lib/relation.js @@ -374,7 +374,7 @@ const Relation = RelationBase.extend({ if (this.isJoined()) related = this.parsePivot(related); // Group all of the related models for easier association with their parent models. - const groupingKey = (key) => (_.isBuffer(key) ? Buffer.toString() : key); + const idKey = (key) => (_.isBuffer(key) ? key.toString('hex') : key); const grouped = _.groupBy(related, (m) => { let key; if (m.pivot) { @@ -388,7 +388,7 @@ const Relation = RelationBase.extend({ } else { key = m.get(this.key('foreignKey')); } - return groupingKey(key); + return idKey(key); }); // Loop over the `parentModels` and attach the grouped sub-models, @@ -397,11 +397,11 @@ const Relation = RelationBase.extend({ let groupedKey; if (!this.isInverse()) { const parsedKey = Object.keys(model.parse({[this.parentIdAttribute]: null}))[0]; - groupedKey = groupingKey(model.get(parsedKey)); + groupedKey = idKey(model.get(parsedKey)); } else { const keyColumn = this.key(this.isThrough() ? 'throughForeignKey' : 'foreignKey'); const formatted = model.format(_.clone(model.attributes)); - groupedKey = groupingKey(formatted[keyColumn]); + groupedKey = idKey(formatted[keyColumn]); } if (groupedKey != null) { const relation = (model.relations[relationName] = this.relatedInstance(grouped[groupedKey], options)); diff --git a/test/base/tests/collection.js b/test/base/tests/collection.js index e90c2173..97ee37b4 100644 --- a/test/base/tests/collection.js +++ b/test/base/tests/collection.js @@ -99,6 +99,17 @@ module.exports = function() { equal(collection.at(1).get('name'), 'Item 2'); }); + it('should delete old models and add new ones with similar binary IDs', function() { + collection = new Collection([{some_id: new Buffer('90', 'hex'), name: 'Test'}, {name: 'No Id'}]); + collection.set([ + {some_id: new Buffer('90', 'hex'), name: 'Item 1'}, + {some_id: new Buffer('93', 'hex'), name: 'Item 2'} + ]); + equal(collection.length, 2); + equal(collection.at(0).get('name'), 'Item 1'); + equal(collection.at(1).get('name'), 'Item 2'); + }); + it('should merge duplicate models by default', function() { collection.set({some_id: 1, name: 'Not Test'}); expect(collection.at(0).get('name')).to.equal('Not Test'); diff --git a/test/integration/relations.js b/test/integration/relations.js index 82546a14..afdd36be 100644 --- a/test/integration/relations.js +++ b/test/integration/relations.js @@ -1391,7 +1391,7 @@ module.exports = function(Bookshelf) { }); }); - describe.only('Issue #xx - Binary ID relations', function() { + describe('Issue #xx - Binary ID relations', function() { it('should group relations properly with binary ID columns', function() { const critic1Id = new Buffer('93', 'hex'); const critic2Id = new Buffer('90', 'hex'); @@ -1414,7 +1414,6 @@ module.exports = function(Bookshelf) { }) .then(function(critics) { critics = critics.serialize(); - console.log(critics); expect(critics).to.have.lengthOf(2); expect(critics[0].comments).to.have.lengthOf(2); expect(critics[1].comments).to.have.lengthOf(1);