Merge pull request #1918 from betastreet/fix/related-binary-ids

Fix withRelated not always grouping properly for 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,20 +374,21 @@ 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 idKey = (key) => (_.isBuffer(key) ? key.toString('hex') : key);
     const grouped = _.groupBy(related, (m) => {
+      let key;
       if (m.pivot) {
         if (this.isInverse() && this.isThrough()) {
-          return this.isThroughForeignKeyTargeted() ? m.pivot.get(this.throughForeignKeyTarget) : m.pivot.id;
+          key = this.isThroughForeignKeyTargeted() ? m.pivot.get(this.throughForeignKeyTarget) : m.pivot.id;
+        } else {
+          key = m.pivot.get(this.key('foreignKey'));
         }
-
-        return m.pivot.get(this.key('foreignKey'));
-      }
-
-      if (this.isInverse()) {
-        return this.isForeignKeyTargeted() ? m.get(this.foreignKeyTarget) : m.id;
+      } else if (this.isInverse()) {
+        key = this.isForeignKeyTargeted() ? m.get(this.foreignKeyTarget) : m.id;
+      } else {
+        key = m.get(this.key('foreignKey'));
       }
-
-      return m.get(this.key('foreignKey'));
+      return idKey(key);
     });
 
     // Loop over the `parentModels` and attach the grouped sub-models,
@@ -396,11 +397,11 @@ const Relation = RelationBase.extend({
       let groupedKey;
       if (!this.isInverse()) {
         const parsedKey = Object.keys(model.parse({[this.parentIdAttribute]: null}))[0];
-        groupedKey = 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 = 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/helpers/migration.js

@@ -7,6 +7,8 @@ var drops = [
   'admins_sites',
   'authors',
   'authors_posts',
+  'critics',
+  'critics_comments',
   'blogs',
   'posts',
   'tags',
@@ -99,6 +101,15 @@ module.exports = function(Bookshelf) {
             table.string('first_name');
             table.string('last_name');
           })
+          .createTable('critics', function(table) {
+            table.binary('id', 16).primary();
+            table.string('name');
+          })
+          .createTable('critics_comments', function(table) {
+            table.increments();
+            table.binary('critic_id', 16).notNullable();
+            table.string('comment');
+          })
           .createTable('posts', function(table) {
             table.increments('id');
             table.integer('owner_id').notNullable();
@@ -147,9 +158,9 @@ module.exports = function(Bookshelf) {
             table.string('imageable_type');
           })
           /* The following table is for testing non-standard morphTo column name
-     * specification. The breaking of naming convention is intentional.
-     * Changing it back to snake_case will break the tests!
-     */
+           * specification. The breaking of naming convention is intentional.
+           * Changing it back to snake_case will break the tests!
+           */
           .createTable('thumbnails', function(table) {
             table.increments('id');
             table.string('url');

test/integration/helpers/objects.js

@@ -121,6 +121,21 @@ module.exports = function(Bookshelf) {
     }
   });
 
+  // Critic uses binary ID
+  var Critic = Bookshelf.Model.extend({
+    tableName: 'critics',
+    comments: function() {
+      return this.hasMany(CriticComment);
+    }
+  });
+
+  var CriticComment = Bookshelf.Model.extend({
+    tableName: 'critics_comments',
+    critic: function() {
+      return this.belongsTo(Critic);
+    }
+  });
+
   // A blog for a site.
   var Blog = Bookshelf.Model.extend({
     tableName: 'blogs',
@@ -414,6 +429,8 @@ module.exports = function(Bookshelf) {
       TestAuthor: TestAuthor,
       Author: Author,
       AuthorParsed: AuthorParsed,
+      Critic: Critic,
+      CriticComment: CriticComment,
       Backup: Backup,
       BackupType: BackupType,
       Blog: Blog,

test/integration/relations.js

@@ -29,6 +29,8 @@ module.exports = function(Bookshelf) {
     var Site = Models.Site;
     var Admin = Models.Admin;
     var Author = Models.Author;
+    var Critic = Models.Critic;
+    var CriticComment = Models.CriticComment;
     var Blog = Models.Blog;
     var Post = Models.Post;
     var Comment = Models.Comment;
@@ -1393,5 +1395,35 @@ module.exports = function(Bookshelf) {
         return new Locale({isoCode: 'en'}).fetch({withRelated: 'customersThrough'}).then(checkTest(this));
       });
     });
+
+    describe('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');
+        const critic1 = new Critic({id: critic1Id, name: '1'});
+        const critic2 = new Critic({id: critic2Id, name: '2'});
+        const comment1 = new CriticComment({critic_id: critic1Id, comment: 'c1-1'});
+        const comment2 = new CriticComment({critic_id: critic1Id, comment: 'c1-2'});
+        const comment3 = new CriticComment({critic_id: critic2Id, comment: 'c2-1'});
+        return Promise.all([
+          critic1.save(null, {method: 'insert'}),
+          critic2.save(null, {method: 'insert'}),
+          comment1.save(),
+          comment2.save(),
+          comment3.save()
+        ])
+          .then(function() {
+            return Critic.where('name', 'IN', ['1', '2'])
+              .orderBy('name', 'ASC')
+              .fetchAll({withRelated: 'comments'});
+          })
+          .then(function(critics) {
+            critics = critics.serialize();
+            expect(critics).to.have.lengthOf(2);
+            expect(critics[0].comments).to.have.lengthOf(2);
+            expect(critics[1].comments).to.have.lengthOf(1);
+          });
+      });
+    });
   });
 };