From e430edee24591afa562167c6f5775b61d1031b75 Mon Sep 17 00:00:00 2001
From: Stefan Penner <stefan.penner@gmail.com>
Date: Sun, 23 Jul 2017 21:54:18 -0700
Subject: [PATCH] [BUGFIX release] handle dupe relationship entries

If a relationship was setup with duplicate entries, it would enter an
invalid state. Specifically, this.canonicalMembers and
this.canonicalState would be out of sync. Resulting in some sad things.

This was most likely introduced by https://github.com/emberjs/data/commit/f8304b23c792a1e74cefb63140e6ee1917cbdfa1#commitcomment-23256408
---
 .../system/relationships/state/has-many.js    | 18 +++--
 .../unit/model/relationships/has-many-test.js | 79 ++++++++++++++++---
 2 files changed, 80 insertions(+), 17 deletions(-)

diff --git a/addon/-private/system/relationships/state/has-many.js b/addon/-private/system/relationships/state/has-many.js
index e700112ff7f..b7a318a5543 100644
--- a/addon/-private/system/relationships/state/has-many.js
+++ b/addon/-private/system/relationships/state/has-many.js
@@ -200,17 +200,25 @@ export default class ManyRelationship extends Relationship {
   }
 
   setInitialInternalModels(internalModels) {
-    if (!internalModels) {
+    if (Array.isArray(internalModels) === false || internalModels.length === 0) {
       return;
     }
 
-    let args = [0, this.canonicalState.length].concat(internalModels);
-    this.canonicalState.splice.apply(this.canonicalState, args);
-    internalModels.forEach(internalModel => {
+    let forCanonical = [];
+
+    for (let i = 0; i< internalModels.length; i++) {
+      let internalModel = internalModels[i];
+      if (this.canonicalMembers.has(internalModel)) {
+        continue;
+      }
+
+      forCanonical.push(internalModel);
       this.canonicalMembers.add(internalModel);
       this.members.add(internalModel);
       this.setupInverseRelationship(internalModel);
-    });
+    }
+
+    this.canonicalState.splice(0, this.canonicalState.length, ...forCanonical);
   }
 
   fetchLink() {
diff --git a/tests/unit/model/relationships/has-many-test.js b/tests/unit/model/relationships/has-many-test.js
index 56792ac8fd9..4a96b4bece5 100644
--- a/tests/unit/model/relationships/has-many-test.js
+++ b/tests/unit/model/relationships/has-many-test.js
@@ -399,6 +399,72 @@ test('hasMany with explicit initial null works even when the inverse was set to
   });
 });
 
+test('hasMany with duplicates from payload', function(assert) {
+  assert.expect(1);
+
+  const Tag = DS.Model.extend({
+    name: DS.attr('string'),
+    people: DS.hasMany('person', { async: false })
+  });
+
+  const Person = DS.Model.extend({
+    name: DS.attr('string'),
+    tag: DS.belongsTo('tag', { async: false })
+  });
+
+  let env = setupStore({ tag: Tag, person: Person });
+  let { store } = env;
+
+  run(() => {
+    // first we push in data with the relationship
+    store.push({
+      data: {
+        type: 'person',
+        id: 1,
+        attributes: {
+          name: 'David J. Hamilton'
+        },
+        relationships: {
+          tag: {
+            data: {
+              type: 'tag',
+              id: 1
+            }
+          }
+        }
+      },
+      included: [
+        {
+          type: 'tag',
+          id: 1,
+          attributes: {
+            name: 'whatever'
+          },
+          relationships: {
+            people: {
+              data: [
+                {
+                  type: 'person',
+                  id: 1
+                },
+                {
+                  type: 'person',
+                  id: 1
+                }
+              ]
+            }
+          }
+        }
+      ]
+    });
+  });
+
+  run(() => {
+    let tag = store.peekRecord('tag', 1);
+    assert.equal(tag.get('people.length'), 1, 'relationship does not contain duplicates');
+  });
+});
+
 test('hasMany with explicit null works even when the inverse was set to not null', function(assert) {
   assert.expect(3);
 
@@ -485,18 +551,7 @@ test('hasMany with explicit null works even when the inverse was set to not null
     let tag = store.peekRecord('tag', 1);
 
     assert.equal(person.get('tag'), null,'relationship is now empty');
-
-    /*
-      TODO this should be asserting `0` however
-      before pushing null, length is actually secretly out-of-sync with
-      the canonicalState array, which has duplicated the addCanonicalRecord
-      leading to length `2`, so when we splice out the record we are left
-      with length 1.
-
-      This is fixed by the relationship cleanup PR which noticed this churn
-      and removed it: https://github.com/emberjs/data/pull/4882
-     */
-    assert.equal(tag.get('people.length'), 1, 'relationship is correct');
+    assert.equal(tag.get('people.length'), 0, 'relationship is correct');
   });
 });