- Fixed collections not being created properly with binary IDs due to…

… collection keying again

- Added a collection test with binary IDs

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)];
 };
 
 /**

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));

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');

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);