[BUGFIX] ensure removals from AdapterPopulatedRecordArrays also remov…

…e the corresponding internalModel -> recordArray reference.

When working with AdapterPopulatedRecordArrays:

* make the very private method more private \w underscore
* rename loadRecords to _setInternalModels

addon/-private/system/record-arrays/adapter-populated-record-array.js

@@ -69,13 +69,26 @@ export default RecordArray.extend({
   },
 
   /**
-    @method loadRecords
+    @method _setInternalModels
     @param {Array} internalModels
     @param {Object} payload normalized payload
     @private
   */
-  loadRecords(internalModels, payload) {
-    let token = heimdall.start('AdapterPopulatedRecordArray.loadRecords');
+  _setInternalModels(internalModels, payload) {
+    let token = heimdall.start('AdapterPopulatedRecordArray._setInternalModels');
+    let operations = {};
+
+    this.get('content').forEach(internalModel => {
+      operations[internalModel.id] = { type: 'remove', internalModel };
+    });
+
+    internalModels.forEach(internalModel => {
+      if (operations[internalModel.id]) {
+        operations[internalModel.id].type = 'remain';
+      } else {
+        operations[internalModel.id] = { type: 'add', internalModel };
+      }
+    });
 
       // TODO: initial load should not cause change events at all, only
     // subsequent. This requires changing the public api of adapter.query, but
@@ -89,8 +102,14 @@ export default RecordArray.extend({
       links: cloneNull(payload.links)
     });
 
-    internalModels.forEach(record => {
-      this.manager.recordArraysForRecord(record).add(this);
+    Object.keys(operations).forEach(id => {
+      let operation = operations[id];
+      let recordArraysSet = this.manager.recordArraysForRecord(operation.internalModel);
+
+      switch (operation.type) {
+        case 'remove': { recordArraysSet.delete(this); break; }
+        case 'add':    { recordArraysSet.add(this);    break; }
+      }
     });
 
     // TODO: should triggering didLoad event be the last action of the runLoop?

addon/-private/system/store/finders.js

@@ -153,27 +153,27 @@ export function _findAll(adapter, store, typeClass, sinceToken, options) {
 }
 
 export function _query(adapter, store, typeClass, query, recordArray) {
-  var modelName = typeClass.modelName;
-  var promise = adapter.query(store, typeClass, query, recordArray);
+  let modelName = typeClass.modelName;
+  let promise = adapter.query(store, typeClass, query, recordArray);
 
-  var serializer = serializerForAdapter(store, adapter, modelName);
-  var label = "DS: Handle Adapter#query of " + typeClass;
+  let serializer = serializerForAdapter(store, adapter, modelName);
+  let label = 'DS: Handle Adapter#query of ' + typeClass;
 
   promise = Promise.resolve(promise, label);
   promise = _guard(promise, _bind(_objectIsAlive, store));
 
-  return promise.then(function(adapterPayload) {
-    var internalModels, payload;
-    store._adapterRun(function() {
+  return promise.then(adapterPayload => {
+    let internalModels, payload;
+    store._adapterRun(() => {
       payload = normalizeResponseHelper(serializer, store, typeClass, adapterPayload, null, 'query');
       internalModels = store._push(payload);
     });
 
     assert('The response to store.query is expected to be an array but it was a single record. Please wrap your response in an array or use `store.queryRecord` to query for a single record.', Array.isArray(internalModels));
-    recordArray.loadRecords(internalModels, payload);
+    recordArray._setInternalModels(internalModels, payload);
 
     return recordArray;
-  }, null, "DS: Extract payload of query " + typeClass);
+  }, null, 'DS: Extract payload of query ' + typeClass);
 }
 
 export function _queryRecord(adapter, store, typeClass, query) {

tests/integration/record-arrays/adapter-populated-record-array-test.js

@@ -9,9 +9,13 @@ let store;
 const { run, RSVP: { Promise } } = Ember;
 
 const Person = DS.Model.extend({
-  name: DS.attr('string')
+  name: DS.attr('string'),
+  toString() {
+    return `<Person#${this.get('id')}>`;
+  }
 });
 
+
 const adapter = DS.Adapter.extend({
   deleteRecord() {
     return Promise.resolve();
@@ -58,8 +62,7 @@ test('when a record is deleted in an adapter populated record array, it should b
   };
 
   run(() => {
-    let records = store.push(payload);
-    recordArray.loadRecords(records, payload);
+    recordArray._setInternalModels(store._push(payload), payload);
   });
 
   assert.equal(recordArray.get('length'), 3, "expected recordArray to contain exactly 3 records");
@@ -103,8 +106,7 @@ test('stores the metadata off the payload', function(assert) {
   };
 
   run(() => {
-    let records = store.push(payload);
-    recordArray.loadRecords(records, payload);
+    recordArray._setInternalModels(store._push(payload), payload);
   });
 
   assert.equal(recordArray.get('meta.foo'), 'bar', 'expected meta.foo to be bar from payload');
@@ -144,8 +146,7 @@ test('stores the links off the payload', function(assert) {
   };
 
   run(() => {
-    let records = store.push(payload);
-    recordArray.loadRecords(records, payload);
+    recordArray._setInternalModels(store._push(payload), payload);
   });
 
   assert.equal(recordArray.get('links.first'), '/foo?page=1', 'expected links.first to be "/foo?page=1" from payload');
@@ -160,6 +161,64 @@ test('recordArray.replace() throws error', function(assert) {
   }, Error('The result of a server query (on (subclass of DS.Model)) is immutable.'), 'throws error');
 });
 
+function assertRecordHasRecordArray(assert, record, recordArray) {
+  if (!assert) { throw TypeError('assertRecordHasRecordArray first argument must be QUnit\'s assert'); }
+  if (!record) { throw TypeError('assertRecordHasRecordArray second argument must be a record'); }
+  if (!assert) { throw TypeError('assertRecordHasRecordArray third argument must be a recordArray'); }
+
+  assert.ok(recordArray.manager.recordArraysForRecord(record._internalModel).has(recordArray), `record '${record}' should have reference to its record array`);
+}
+
+function assertRecordDoesNOTRecordArray(assert, record, recordArray) {
+  if (!assert) { throw TypeError('assertRecordHasRecordArray first argument must be QUnit\'s assert'); }
+  if (!record) { throw TypeError('assertRecordHasRecordArray second argument must be a record'); }
+  if (!assert) { throw TypeError('assertRecordHasRecordArray third argument must be a recordArray'); }
+
+  assert.ok(!recordArray.manager.recordArraysForRecord(record._internalModel).has(recordArray), `record '${record}' should NOT have reference to their record array`);
+}
+
+test('loadRecord re-syncs internalModels recordArrays', function(assert) {
+  let env = setupStore({ person: Person });
+  let store = env.store;
+
+  let payload = [
+    { id: '1', name: 'Scumbag Dale' },
+    { id: '2', name: 'Scumbag Katz' }
+  ];
+
+  env.adapter.query = function(store, type, query, recordArray) {
+    return payload;
+  };
+
+  let katz, dale, penner;
+  return store.query('person', { }).then(recordArray => {
+    return recordArray.update().then(recordArray => {
+      assert.deepEqual(recordArray.getEach('name'), ['Scumbag Dale', 'Scumbag Katz'], 'expected query to contain specific records');
+
+      dale = recordArray.findBy('id', '1');
+      katz = recordArray.findBy('id', '2');
+
+      assertRecordHasRecordArray(assert, dale, recordArray);
+      assertRecordHasRecordArray(assert, katz, recordArray);
+
+      payload = [
+        { id: '1', name: 'Scumbag Dale' },
+        { id: '3', name: 'Scumbag Penner' }
+      ];
+
+      return recordArray.update();
+    }).then(recordArray => {
+      penner = recordArray.findBy('id', '3');
+
+      assertRecordHasRecordArray(assert, dale, recordArray);
+      assertRecordDoesNOTRecordArray(assert, katz, recordArray);
+      assertRecordHasRecordArray(assert, penner, recordArray);
+
+      assert.deepEqual(recordArray.getEach('name'), ['Scumbag Dale', 'Scumbag Penner']);
+    });
+  });
+});
+
 test('when an adapter populated record gets updated the array contents are also updated', function(assert) {
   assert.expect(8);
 

tests/unit/record-arrays/adapter-populated-record-array-test.js

@@ -8,18 +8,18 @@ const { AdapterPopulatedRecordArray } = DS;
 
 module('unit/record-arrays/adapter-populated-record-array - DS.AdapterPopulatedRecordArray');
 
-function recordFor(record) {
+function internalModelFor(record) {
   let _internalModel = {
+    get id() {
+      return record.id;
+    },
     getRecord() {
-      record._internalModel = _internalModel
       return record;
     }
-  }
-
-  return {
-    id: record.id,
-    _internalModel
   };
+
+  record._internalModel = _internalModel
+  return _internalModel;
 }
 
 test('default initial state', function(assert) {
@@ -102,7 +102,7 @@ test('#update uses _update enabling query specific behavior', function(assert) {
 });
 
 // TODO: is this method required, i suspect store._query should be refactor so this is not needed
-test('#loadRecords', function(assert) {
+test('#_setInternalModels', function(assert) {
   let didAddRecord = 0;
   const manager = {
     recordArraysForRecord(record) {
@@ -120,8 +120,8 @@ test('#loadRecords', function(assert) {
     manager
   });
 
-  let model1 = recordFor({ id: 1 });
-  let model2 = recordFor({ id: 2 });
+  let model1 = internalModelFor({ id: 1 });
+  let model2 = internalModelFor({ id: 2 });
 
   assert.equal(didAddRecord, 0, 'no records should have been added yet');
 
@@ -130,21 +130,21 @@ test('#loadRecords', function(assert) {
     didLoad++;
   });
 
-  let links = { foo:1 };
-  let meta = { bar:2 };
+  let links = { foo: 1 };
+  let meta = { bar: 2 };
 
   run(() => {
-    assert.equal(recordArray.loadRecords([model1._internalModel, model2._internalModel], {
+    assert.equal(recordArray._setInternalModels([model1, model2], {
       links,
       meta
-    }), undefined, 'loadRecords should have no return value');
+    }), undefined, '_setInternalModels should have no return value');
 
-    assert.equal(didAddRecord, 2, 'two records should have been adde');
+    assert.equal(didAddRecord, 2, 'two records should have been added');
 
     assert.deepEqual(recordArray.toArray(), [
       model1,
       model2
-    ], 'should now contain the loaded records');
+    ].map(x => x.getRecord()), 'should now contain the loaded records');
 
     assert.equal(didLoad, 0, 'didLoad event should not have fired');
     assert.equal(recordArray.get('links').foo, 1);
@@ -154,20 +154,25 @@ test('#loadRecords', function(assert) {
 });
 
 test('change events when receiving a new query payload', function(assert) {
-  assert.expect(37);
+  assert.expect(41);
 
   let arrayDidChange = 0;
   let contentDidChange = 0;
   let didAddRecord = 0;
+  let didRemoveRecord = 0;
 
   const manager = {
     recordArraysForRecord(record) {
       return {
         add(array) {
           didAddRecord++;
           assert.equal(array, recordArray);
+        },
+        delete(array) {
+          didRemoveRecord++;
+          assert.equal(array, recordArray);
         }
-      }
+      };
     }
   };
 
@@ -177,9 +182,9 @@ test('change events when receiving a new query payload', function(assert) {
   });
 
   run(() => {
-    recordArray.loadRecords([
-      recordFor({ id: '1', name: 'Scumbag Dale' }),
-      recordFor({ id: '2', name: 'Scumbag Katz' })
+    recordArray._setInternalModels([
+      internalModelFor({ id: '1', name: 'Scumbag Dale' }),
+      internalModelFor({ id: '2', name: 'Scumbag Katz' })
     ], {});
   });
 
@@ -212,15 +217,17 @@ test('change events when receiving a new query payload', function(assert) {
   arrayDidChange = 0;
   contentDidChange = 0;
   didAddRecord = 0;
+  didRemoveRecord = 0;
 
   run(() => {
     // re-query
-    recordArray.loadRecords([
-      recordFor({ id: '3', name: 'Scumbag Penner' }),
-      recordFor({ id: '4', name: 'Scumbag Hamilton' })
+    recordArray._setInternalModels([
+      internalModelFor({ id: '3', name: 'Scumbag Penner' }),
+      internalModelFor({ id: '4', name: 'Scumbag Hamilton' })
     ], {});
   });
 
+  assert.equal(didRemoveRecord, 2, 'expected 2 didAddRecords');
   assert.equal(didAddRecord, 2, 'expected 2 didAddRecords');
   assert.equal(recordArray.get('isLoaded'), true, 'should be considered loaded');
   assert.equal(recordArray.get('isUpdating'), false, 'should no longer be updating');
@@ -233,6 +240,7 @@ test('change events when receiving a new query payload', function(assert) {
   arrayDidChange = 0; // reset change event counter
   contentDidChange = 0; // reset change event counter
   didAddRecord = 0;
+  didRemoveRecord= 0;
 
   recordArray.one('@array:change', function(array, startIdx, removeAmt, addAmt) {
     arrayDidChange++;
@@ -252,12 +260,13 @@ test('change events when receiving a new query payload', function(assert) {
   assert.equal(contentDidChange, 0, 'recordArray.content should not have changed');
 
   run(() => {
-    recordArray.loadRecords([
-      recordFor({ id: '3', name: 'Scumbag Penner' })
+    recordArray._setInternalModels([
+      internalModelFor({ id: '3', name: 'Scumbag Penner' })
     ], {});
   });
 
-  assert.equal(didAddRecord, 1, 'expected 1 didAddRecord');
+  assert.equal(didAddRecord, 0, 'expected 0 didAddRecord');
+  assert.equal(didRemoveRecord, 1, 'expected 1 didRemoveRecord');
 
   assert.equal(recordArray.get('isLoaded'), true, 'should be considered loaded');
   assert.equal(recordArray.get('isUpdating'), false, 'should not longer be updating');