Fix incorrect results in collection when models have duplicate ids

lib/base/collection.js

@@ -180,13 +180,20 @@ CollectionBase.prototype.toJSON = function(options) {
  * @method
  * @description
  *
- * The set method performs a "smart" update of the collection with the passed
- * model or list of models. If a model in the list isn't yet in the
- * collection it will be added; if the model is already in the collection
- * its attributes will be merged; and if the collection contains any models
- * that aren't present in the list, they'll be removed. If you'd like to
- * customize the behavior, you can disable it with options: `{add: false}`,
- * `{remove: false}`, or `{merge: false}`.
+ * The set method performs a smart update of the collection with the passed
+ * model or list of models by following the following rules:
+ * - If a model in the list isn't yet in the collection it will be added
+ * - if the model is already in the collection its attributes will be merged
+ * - if the collection contains any models that aren't present in the list,
+ * they'll be removed.
+ *
+ * If you'd like to customize the behavior, you can do so with the `add`,
+ * `merge` and `remove` options.
+ *
+ * Since version 0.14.0 if both `remove` and `merge` options are set to
+ * `false`, then any duplicate models present will be added to the collection,
+ * otherwise they will either be removed or merged, according to the chosen
+ * option.
  *
  * @example
  *
@@ -195,7 +202,18 @@ CollectionBase.prototype.toJSON = function(options) {
  *
  * @param {Object[]|Model[]|Object|Model} models One or more models or raw
  * attribute objects.
- * @param {Object=} options See description.
+ * @param {Object=} options
+ *   Options for controlling how models are added or removed.
+ * @param {Boolean=} options.add=true
+ *   If set to `true` it will add any new models to the collection, otherwise
+ *   any new models will be ignored.
+ * @param {Boolean=} options.merge=true
+ *   If set to `true` it will merge the attributes of duplicate models with the
+ *   attributes of existing models in the collection, otherwise duplicate
+ *   models in the list will be ignored.
+ * @param {Boolean=} options.remove=true
+ *   If set to `true` any models in the collection that are not in the list
+ *   will be removed from the collection, otherwise they will be kept.
  * @returns {Collection} Self, this method is chainable.
  */
 CollectionBase.prototype.set = function(models, options) {
@@ -223,7 +241,7 @@ CollectionBase.prototype.set = function(models, options) {
     // If a duplicate is found, prevent it from being added and
     // optionally merge it into the existing model.
     const existing = this.get(id)
-    if (existing) {
+    if (existing && (options.merge || options.remove)) {
       if (options.remove) {
         modelMap[existing.cid] = true;
       }
@@ -240,7 +258,8 @@ CollectionBase.prototype.set = function(models, options) {
       this._byId[model.cid] = model;
       if (model.id != null) this._byId[model.id] = model;
     }
-    if (order) order.push(existing || model);
+
+    if (order && !(existing && order.indexOf(existing) > -1)) order.push(existing || model);
   }
 
   // Remove nonexistent models if appropriate.
@@ -252,7 +271,7 @@ CollectionBase.prototype.set = function(models, options) {
   }
 
   // See if sorting is needed, update `length` and splice in new models.
-  if (toAdd.length || (order && order.length)) {
+  if (toAdd.length || order && order.length) {
     this.length += toAdd.length;
     if (at != null) {
       Array.prototype.splice.apply(this.models, [at, 0].concat(toAdd));
@@ -358,16 +377,17 @@ CollectionBase.prototype.fetch = function() {
  * @method
  * @description
  *
- * Add a {@link Model model} (or an array of models) to the collection, You may
- * also pass raw attributes objects, and have them be vivified as instances of
- * the model. Pass `{at: index}` to splice the model into the collection at the
- * specified `index`. If you're adding models to the collection that are already
- * in the collection, they'll be ignored, unless you pass `{merge: true}`, in
- * which case their {@link Model#attributes attributes} will be merged into the
- * corresponding models.
+ * Add a {@link Model model}, or an array of models, to the collection. You may
+ * also pass raw attribute objects, which will be converted to proper models
+ * when being added to the collection.
  *
- * *Note that adding the same model (a model with the same id) to a collection
- * more than once is a no-op.*
+ * You can pass the `{at: index}` option to splice the model into the
+ * collection at the specified `index`.
+ *
+ * By default if you're adding models to the collection that are already
+ * present, they'll be ignored, unless you pass `{merge: true}`, in
+ * which case their {@link Model#attributes attributes} will be merged with the
+ * corresponding models.
  *
  * @example
  *
@@ -378,13 +398,19 @@ CollectionBase.prototype.fetch = function() {
  *   {name: "Black Pearl"}
  * ]);
  *
- * @param {Object[]|Model[]|Object|Model} models One or more models or raw
- * attribute objects.
- * @param {Object=} options See description.
+ * @param {Object[]|Model[]|Object|Model} models
+ *   One or more models or raw attribute objects.
+ * @param {Object=} options Options for controlling how models are added.
+ * @param {Boolean=} options.merge=false
+ *   If set to `true` it will merge the attributes of duplicate models with the
+ *   attributes of existing models in the collection.
+ * @param {Number=} options.at
+ *   If set to a number equal to or greater than 0 it will splice the model
+ *   into the collection at the specified index number.
  * @returns {Collection} Self, this method is chainable.
  */
 CollectionBase.prototype.add = function(models, options) {
-  return this.set(models, _.extend({merge: false}, options, addOptions));
+  return this.set(models, Object.assign({merge: false}, options, addOptions));
 };
 
 /**
@@ -438,7 +464,7 @@ CollectionBase.prototype.reset = function(models, options) {
   options = options || {};
   options.previousModels = this.models;
   this._reset();
-  models = this.add(models, _.extend({silent: true}, options));
+  models = this.set(models, Object.assign({silent: true}, options));
   if (!options.silent) this.trigger('reset', this, options);
   return models;
 };

lib/base/eager.js

@@ -94,12 +94,12 @@ _.extend(EagerBase.prototype, {
 
   // Pushes each of the incoming models onto a new `related` array,
   // which is used to correcly pair additional nested relations.
-  pushModels: function(relationName, handled, response) {
+  pushModels: function pushModels(relationName, handled, response, options) {
     const models = this.parent;
     const relatedData = handled.relatedData;
     const related = _.map(response, row => relatedData.createModel(row));
 
-    return relatedData.eagerPair(relationName, related, models);
+    return relatedData.eagerPair(relationName, related, models, options);
   }
 
 });

lib/collection.js

@@ -117,7 +117,9 @@ const BookshelfCollection = module.exports = CollectionBase.extend({
       })
 
       // Now, load all of the data onto the collection as necessary.
-      .tap(this._handleResponse)
+      .tap(function(response) {
+        return this._handleResponse(response, options)
+      })
 
       // If the "withRelated" is specified, we also need to eager load all of the
       // data on the collection, as a side-effect, before we ultimately jump into the
@@ -424,10 +426,15 @@ const BookshelfCollection = module.exports = CollectionBase.extend({
    * Handles the response data for the collection, returning from the
    * collection's `fetch` call.
    */
-  _handleResponse: function(response) {
+  _handleResponse: function(response, options) {
     const relatedData = this.relatedData;
 
-    this.set(response, {silent: true, parse: true}).invokeMap('formatTimestamps');
+    this.set(response, {
+      merge: options.merge,
+      remove: options.remove,
+      silent: true,
+      parse: true
+    }).invokeMap('formatTimestamps');
     this.invokeMap('_reset');
 
     if (relatedData && relatedData.isJoined()) {

lib/eager.js

@@ -74,7 +74,7 @@ class EagerRelation extends EagerBase {
   _eagerLoadHelper(response, relationName, handled, options) {
     const relatedData = handled.relatedData;
     const isEmptyHasOne = response.length === 0 && relatedData.type === 'hasOne';
-    const relatedModels = isEmptyHasOne ? [] : this.pushModels(relationName, handled, response);
+    const relatedModels = isEmptyHasOne ? [] : this.pushModels(relationName, handled, response, options);
 
     return Promise.try(() => {
       // If there is a response, fetch additional nested eager relations, if any.

lib/relation.js

@@ -344,8 +344,9 @@ const Relation = RelationBase.extend({
 
   // Creates a new model or collection instance, depending on
   // the `relatedData` settings and the models passed in.
-  relatedInstance(models) {
+  relatedInstance(models, options) {
     models = models || [];
+    options = options || {};
 
     const Target = this.target;
 
@@ -361,14 +362,14 @@ const Relation = RelationBase.extend({
     // Allows us to just use a model, but create a temporary
     // collection for a "*-many" relation.
     if (Target.prototype instanceof ModelBase) {
-      return Target.collection(models, {parse: true});
+      return Target.collection(models, {parse: true, merge: options.merge, remove: options.remove});
     }
     return new Target(models, {parse: true});
   },
 
   // Groups the related response according to the type of relationship
   // we're handling, for easy attachment to the parent models.
-  eagerPair(relationName, related, parentModels) {
+  eagerPair(relationName, related, parentModels, options) {
     // If this is a morphTo, we only want to pair on the morphValue for the current relation.
     if (this.type === 'morphTo') {
       parentModels = _.filter(parentModels, (m) => {
@@ -411,7 +412,7 @@ const Relation = RelationBase.extend({
         groupedKey = formatted[keyColumn];
       }
       if (groupedKey != null) {
-        const relation = model.relations[relationName] = this.relatedInstance(grouped[groupedKey]);
+        const relation = model.relations[relationName] = this.relatedInstance(grouped[groupedKey], options);
         if (this.type === 'belongsToMany') {
           // If type is of "belongsToMany" then the relatedData need to be recreated through the parent model
           relation.relatedData = model[relationName]().relatedData;

test/base/tests/collection.js

@@ -22,10 +22,10 @@ module.exports = function() {
     });
 
     beforeEach(function() {
-      collection = new Collection([{some_id: 1, name: 'Test'}, {id: 2, name: 'No Id'}]);
+      collection = new Collection([{some_id: 1, name: 'Test'}, {name: 'No Id'}]);
     });
 
-    it('should have a tableName method, returning the tableName of the model', function () {
+    it('should have a tableName method that returns the tableName of the model', function() {
       equal(collection.tableName(), 'test_table');
     });
 
@@ -48,38 +48,85 @@ module.exports = function() {
       equal(collection.at(1).id, undefined);
     });
 
+    describe('#add()', function() {
+      it('adds new models to the collection', function() {
+        var originalLength = collection.length;
+        var newLength = collection.add({some_id: 3, name: 'Alice'}).length;
+        expect(newLength).to.be.above(originalLength);
+      })
+
+      it('ignores duplicate models by default', function() {
+        collection.add({some_id: 1, name: 'Not Test'});
+        expect(collection.at(0).get('name')).to.equal('Test');
+      })
+
+      it('merges duplicate models when the merge option is set', function() {
+        collection.add({some_id: 1, name: 'Not Test'}, {merge: true});
+        expect(collection.at(0).get('name')).to.equal('Not Test');
+      })
+
+      it('Ignores the remove option when it\'s set to true', function() {
+        var originalLength = collection.length;
+        var newLength = collection.add(null, {remove: true}).length;
+
+        expect(collection.at(0).get('name')).to.equal('Test');
+        expect(newLength).to.equal(originalLength);
+      })
+
+      it('Ignores the add option when it\'s set to false and still adds new models', function() {
+        var originalLength = collection.length;
+        var newLength = collection.add({some_id: 3, name: 'Alice'}, {add: false}).length;
+        expect(newLength).to.be.above(originalLength);
+      })
+    });
 
     describe('#set()', function() {
+      it('should accept a single object as argument', function() {
+        collection.set({some_id: 3, name: 'New Model'});
+        expect(collection.at(0).get('name')).to.equal('New Model');
+      });
+
+      it('should accept Models as argument', function() {
+        var model = new collection.model({some_id: 3, name: 'New Model'});
+        collection.set([model]);
+        expect(collection.at(0).get('name')).to.equal('New Model');
+      });
+
       it('should delete old models and add new ones by default', function() {
-        collection.set([{some_id: 1, name: 'Test'}, {some_id: 2, name: 'Item'}]);
+        collection.set([{some_id: 1, name: 'Item 1'}, {some_id: 2, name: 'Item 2'}]);
         equal(collection.length, 2);
-        equal(collection.models.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');
+        expect(collection.length).to.equal(1);
+      })
+
+      it('should merge duplicate models in the new set', function() {
+        collection.set([{some_id: 1, name: 'Not Test'}, {some_id: 1, name: 'Not Test As Well'}]);
+        expect(collection.at(0).get('name')).to.equal('Not Test As Well');
+        expect(collection.toJSON().length).to.equal(collection.length);
+        expect(collection.length).to.equal(1);
+      })
+
       it('should not remove models with {remove: false} option set', function() {
         collection.set([{some_id: 2, name: 'Item2'}], {remove: false});
         equal(collection.length, 3);
       });
 
       it('should not merge new attribute values with {merge: false} option set', function() {
-        collection.set([{some_id: 1, name: 'WontChange'}], {merge: false, parse: true});
+        collection.set([{some_id: 1, name: 'WontChange'}], {merge: false});
         equal(collection.get(1).get('name'), 'Test');
       });
 
-      it('should accept a single model, not an array', function() {
-        collection.set({some_id: 1, name: 'Changed'});
-        equal(collection.get(1).get('name'), 'Changed');
-      });
-
-      it('should accept Models', function() {
-        var model = new collection.model({
-          some_id: 3,
-          name: 'Changed'
-        });
-        collection.set([model]);
-
-        equal(collection.get(3).get('name'), 'Changed');
-      });
+      it('should add duplicate models if both the remove and merge options are false', function() {
+        var originalLength = collection.length;
+        var newLength = collection.set({some_id: 1, name: 'Not Test'}, {merge: false, remove: false}).length;
+        expect(newLength).to.be.above(originalLength);
+      })
 
       it('should not add models with {add: false} option set', function() {
         collection.set([{some_id: 3, name: 'WontAdd'}], {add: false});
@@ -114,7 +161,7 @@ module.exports = function() {
       assert.ok((newModel instanceof collection.model));
     });
 
-    it('contains a mapThen method, which calls map on the models, and returns a when.all promise', function() {
+    it('contains a mapThen method which calls map on the models and returns a when.all promise', function() {
       var spyIterator = sinon.spy(function(model) {
         return model.id;
       });
@@ -125,7 +172,7 @@ module.exports = function() {
       });
     });
 
-    it('contains an invokeThen method, which does an invoke on the models, and returns a when.all promise', function() {
+    it('contains an invokeThen method which does an invoke on the models and returns a when.all promise', function() {
       return collection.invokeThen('invokedMethod').then(function(resp) {
         expect(_.compact(resp)).to.eql([1]);
       })

test/integration/helpers/inserts.js

@@ -265,9 +265,13 @@ module.exports = function(bookshelf) {
       {id: 1, name: 'enhanced'} // We need to explicitly set the id to 1 otherwise MySQL will get confused
     ]),
 
-    knex('organization').insert({organization_name: 'Test Organization'}),
+    knex('organization').insert([{organization_name: 'Test Organization'}, {organization_name: 'Duplicates'}]),
 
-    knex('members').insert({organization_id: 1}, {organization_id: 2})
+    knex('members').insert([
+      {id: 1, organization_id: 1, name: 'Shuri'},
+      {id: 2, organization_id: 2, name: 'Bob'},
+      {id: 2, organization_id: 2, name: 'Alice'}
+    ])
   ]).then(function() {
     if (knex.client.dialect !== 'postgresql') return;
 

test/integration/helpers/migration.js

@@ -173,8 +173,9 @@ module.exports = function(Bookshelf) {
       table.boolean('organization_is_active').defaultTo(false);
     })
     .createTable('members', function(table) {
-      table.increments();
+      table.integer('id').notNullable();
       table.integer('organization_id').notNullable();
+      table.string('name');
     })
     .createTable('locales', function(table) {
       table.string('isoCode');