[FEAT BUGFIX] resolves issues with links and data in relationships (#…

…5410)

* [FEAT BUGFIX] resolves issues with links and data in relationships

* remove useless code

* fix belongsTo reload for async relationships

* fix merge forward of meta/links on inverses

* fix test

* fix much of merge-forward

* not done yet, but getting there

* aye! there's the rub

* apparently testem thinks successful TODOs are test failures

* misc cleanup

* add code comments

* add more comments and remove dead code

* document _partialData

addon/-private/system/model/internal-model.js

@@ -373,11 +373,13 @@ export default class InternalModel {
               break;
             case 'belongsTo':
               this.setDirtyBelongsTo(name, propertyValue);
-              relationships.get(name).setHasData(true);
+              relationships.get(name).setHasAnyRelationshipData(true);
+              relationships.get(name).setRelationshipIsEmpty(false);
               break;
             case 'hasMany':
               this.setDirtyHasMany(name, propertyValue);
-              relationships.get(name).setHasData(true);
+              relationships.get(name).setHasAnyRelationshipData(true);
+              relationships.get(name).setRelationshipIsEmpty(false);
               break;
             default:
               createOptions[name] = propertyValue;

addon/-private/system/promise-proxies.js

@@ -1,7 +1,7 @@
 import ObjectProxy from '@ember/object/proxy';
 import PromiseProxyMixin from '@ember/object/promise-proxy-mixin';
 import ArrayProxy from '@ember/array/proxy';
-import { get } from '@ember/object';
+import { get, computed } from '@ember/object';
 import { reads } from '@ember/object/computed';
 import { Promise } from 'rsvp';
 import { assert } from '@ember/debug';
@@ -82,6 +82,28 @@ export function promiseArray(promise, label) {
   });
 }
 
+export const PromiseBelongsTo = PromiseObject.extend({
+
+  // we don't proxy meta because we would need to proxy it to the relationship state container
+  //  however, meta on relationships does not trigger change notifications.
+  //  if you need relationship meta, you should do `record.belongsTo(relationshipName).meta()`
+  meta: computed(function() {
+    assert(
+      'You attempted to access meta on the promise for the async belongsTo relationship ' +
+      `${this.get('_belongsToState').internalModel.modelName}:${this.get('_belongsToState').key}'.` +
+       '\nUse `record.belongsTo(relationshipName).meta()` instead.',
+      false
+    );
+  }),
+
+  reload() {
+    assert('You are trying to reload an async belongsTo before it has been created', this.get('content') !== undefined);
+    this.get('_belongsToState').reload();
+
+    return this;
+  }
+});
+
 /**
   A PromiseManyArray is a PromiseArray that also proxies certain method calls
   to the underlying manyArray.
@@ -109,7 +131,7 @@ export function proxyToContent(method) {
 export const PromiseManyArray = PromiseArray.extend({
   reload() {
     assert('You are trying to reload an async manyArray before it has been created', get(this, 'content'));
-    this.set('promise', this.get('content').reload())
+    this.set('promise', this.get('content').reload());
     return this;
   },
 

addon/-private/system/references/has-many.js

@@ -258,8 +258,8 @@ export default class HasManyReference extends Reference {
   }
 
   _isLoaded() {
-    let hasData = get(this.hasManyRelationship, 'hasData');
-    if (!hasData) {
+    let hasRelationshipDataProperty = get(this.hasManyRelationship, 'hasAnyRelationshipData');
+    if (!hasRelationshipDataProperty) {
       return false;
     }
 

addon/-private/system/relationships/relationship-payloads.js

@@ -1,5 +1,41 @@
 import { assert } from '@ember/debug';
 
+/**
+ * Merge data,meta,links information forward to the next payload
+ * if required. Latest data will always win.
+ *
+ * @param oldPayload
+ * @param newPayload
+ */
+function mergeForwardPayload(oldPayload, newPayload) {
+  if (oldPayload && oldPayload.data !== undefined && newPayload.data === undefined) {
+    newPayload.data = oldPayload.data;
+  }
+
+  /*
+    _partialData is has-many relationship data that has been discovered via
+     inverses in the absence of canonical `data` availability from the primary
+     payload.
+
+    We can't merge this data into `data` as that would trick has-many relationships
+     into believing they know their complete membership. Anytime we find canonical
+     data from the primary record, this partial data is discarded. If no canonical
+     data is ever discovered, the partial data will be loaded by the relationship
+     in a way that correctly preserves the `stale` relationship state.
+   */
+  if (newPayload.data === undefined && oldPayload && oldPayload._partialData !== undefined) {
+    newPayload._partialData = oldPayload._partialData;
+  }
+
+  if (oldPayload && oldPayload.meta !== undefined && newPayload.meta === undefined) {
+    newPayload.meta = oldPayload.meta;
+  }
+
+  if (oldPayload && oldPayload.links !== undefined && newPayload.links === undefined) {
+    newPayload.links = oldPayload.links;
+  }
+}
+
 // TODO this is now VERY similar to the identity/internal-model map
 //  so we should probably generalize
 export class TypeCache {
@@ -204,7 +240,6 @@ export default class RelationshipPayloads {
       let payloadMap;
       let inversePayloadMap;
       let inverseIsMany;
-
       if (this._isLHS(modelName, relationshipName)) {
         previousPayload = this.lhs_payloads.get(modelName, id);
         payloadMap = this.lhs_payloads;
@@ -257,14 +292,34 @@ export default class RelationshipPayloads {
       // * null is considered new information "empty", and it should win
       // * undefined is NOT considered new information, we should keep original state
       // * anything else is considered new information, and it should win
+      let isMatchingIdentifier = this._isMatchingIdentifier(
+        relationshipData && relationshipData.data,
+        previousPayload && previousPayload.data
+      );
+
       if (relationshipData.data !== undefined) {
-        this._removeInverse(id, previousPayload, inversePayloadMap);
+        if (!isMatchingIdentifier) {
+          this._removeInverse(id, previousPayload, inversePayloadMap);
+        }
       }
+
+      mergeForwardPayload(previousPayload, relationshipData);
       payloadMap.set(modelName, id, relationshipData);
-      this._populateInverse(relationshipData, inverseRelationshipData, inversePayloadMap, inverseIsMany);
+
+      if (!isMatchingIdentifier) {
+        this._populateInverse(relationshipData, inverseRelationshipData, inversePayloadMap, inverseIsMany);
+      }
     }
   }
 
+  _isMatchingIdentifier(a, b) {
+    return a && b &&
+      a.type === b.type &&
+      a.id === b.id &&
+      !Array.isArray(a) &&
+      !Array.isArray(b);
+  }
+
   /**
    Populate the inverse relationship for `relationshipData`.
 
@@ -306,30 +361,41 @@ export default class RelationshipPayloads {
    */
   _addToInverse(inversePayload, resourceIdentifier, inversePayloadMap, inverseIsMany) {
     let relInfo = this._relInfo;
+    let inverseData = inversePayload.data;
 
-    if (relInfo.isReflexive && inversePayload.data.id === resourceIdentifier.id) {
+    if (relInfo.isReflexive && inverseData && inverseData.id === resourceIdentifier.id) {
       // eg <user:1>.friends = [{ id: 1, type: 'user' }]
       return;
     }
 
     let existingPayload = inversePayloadMap.get(resourceIdentifier.type, resourceIdentifier.id);
-    let existingData = existingPayload && existingPayload.data;
 
-    if (existingData) {
-      // There already is an inverse, either add or overwrite depehnding on
+    if (existingPayload) {
+      // There already is an inverse, either add or overwrite depending on
       // whether the inverse is a many relationship or not
       //
-      if (Array.isArray(existingData)) {
-        existingData.push(inversePayload.data);
+      if (inverseIsMany) {
+        let existingData = existingPayload.data;
+
+        // in the case of a hasMany
+        // we do not want create a `data` array where there was none before
+        // if we also have links, which this would indicate
+        if (existingData) {
+          existingData.push(inversePayload.data);
+        } else {
+          existingPayload._partialData = existingPayload._partialData || [];
+          existingPayload._partialData.push(inversePayload.data);
+        }
       } else {
+        mergeForwardPayload(existingPayload, inversePayload);
         inversePayloadMap.set(resourceIdentifier.type, resourceIdentifier.id, inversePayload);
       }
     } else {
       // first time we're populating the inverse side
       //
       if (inverseIsMany) {
         inversePayloadMap.set(resourceIdentifier.type, resourceIdentifier.id, {
-          data: [inversePayload.data]
+          _partialData: [inversePayload.data]
         });
       } else {
         inversePayloadMap.set(resourceIdentifier.type, resourceIdentifier.id, inversePayload);
@@ -357,7 +423,10 @@ export default class RelationshipPayloads {
    */
   _removeInverse(id, previousPayload, inversePayloadMap) {
     let data = previousPayload && previousPayload.data;
-    if (!data) {
+    let partialData = previousPayload && previousPayload._partialData;
+    let maybeData = data || partialData;
+
+    if (!maybeData) {
       // either this is the first time we've seen a payload for this id, or its
       // previous payload indicated that it had no inverse, eg a belongsTo
       // relationship with payload { data: null }
@@ -367,10 +436,10 @@ export default class RelationshipPayloads {
       return;
     }
 
-    if (Array.isArray(data)) {
+    if (Array.isArray(maybeData)) {
       // TODO: diff rather than removeall addall?
-      for (let i=0; i<data.length; ++i) {
-        const resourceIdentifier = data[i];
+      for (let i=0; i<maybeData.length; ++i) {
+        const resourceIdentifier = maybeData[i];
         this._removeFromInverse(id, resourceIdentifier, inversePayloadMap);
       }
     } else {
@@ -388,15 +457,17 @@ export default class RelationshipPayloads {
   _removeFromInverse(id, resourceIdentifier, inversePayloads) {
     let inversePayload = inversePayloads.get(resourceIdentifier.type, resourceIdentifier.id);
     let data = inversePayload && inversePayload.data;
+    let partialData = inversePayload && inversePayload._partialData;
 
-    if (!data) { return; }
+    if (!data && !partialData) { return; }
 
     if (Array.isArray(data)) {
       inversePayload.data = data.filter((x) => x.id !== id);
+    } else if (Array.isArray(partialData)) {
+      inversePayload._partialData = partialData.filter((x) => x.id !== id);
     } else {
-      inversePayloads.set(resourceIdentifier.type, resourceIdentifier.id, {
-        data: null
-      });
+      // this merges forward links and meta
+      inversePayload.data = null;
     }
   }
 }

addon/-private/system/relationships/state/belongs-to.js

@@ -2,17 +2,16 @@ import { Promise as EmberPromise } from 'rsvp';
 import { assert, inspect } from '@ember/debug';
 import { assertPolymorphicType } from 'ember-data/-debug';
 import {
-  PromiseObject
+  PromiseBelongsTo
 } from "../../promise-proxies";
 import Relationship from "./relationship";
 
 export default class BelongsToRelationship extends Relationship {
   constructor(store, internalModel, inverseKey, relationshipMeta) {
     super(store, internalModel, inverseKey, relationshipMeta);
-    this.internalModel = internalModel;
-    this.key = relationshipMeta.key;
     this.inverseInternalModel = null;
     this.canonicalState = null;
+    this._loadingPromise = null;
   }
 
   setInternalModel(internalModel) {
@@ -21,8 +20,10 @@ export default class BelongsToRelationship extends Relationship {
     } else if (this.inverseInternalModel) {
       this.removeInternalModel(this.inverseInternalModel);
     }
-    this.setHasData(true);
-    this.setHasLoaded(true);
+    this.setHasAnyRelationshipData(true);
+    this.setRelationshipIsStale(false);
+    this.setRelationshipIsEmpty(false);
+    this.setHasRelatedResources(!this.localStateIsEmpty());
   }
 
   setCanonicalInternalModel(internalModel) {
@@ -165,20 +166,16 @@ export default class BelongsToRelationship extends Relationship {
     //TODO(Igor) flushCanonical here once our syncing is not stupid
     if (this.isAsync) {
       let promise;
-      if (this.link) {
-        if (this.hasLoaded) {
-          promise = this.findRecord();
-        } else {
-          promise = this.findLink().then(() => this.findRecord());
-        }
+
+      if (this._shouldFindViaLink()) {
+        promise = this.findLink().then(() => this.findRecord());
       } else {
         promise = this.findRecord();
       }
 
-      return PromiseObject.create({
-        promise: promise,
-        content: this.inverseInternalModel ? this.inverseInternalModel.getRecord() : null
-      });
+      let record = this.inverseInternalModel ? this.inverseInternalModel.getRecord() : null
+
+      return this._updateLoadingPromise(promise, record);
     } else {
       if (this.inverseInternalModel === null) {
         return null;
@@ -189,19 +186,50 @@ export default class BelongsToRelationship extends Relationship {
     }
   }
 
+  _updateLoadingPromise(promise, content) {
+    if (this._loadingPromise) {
+      if (content) {
+        this._loadingPromise.set('content', content)
+      }
+      this._loadingPromise.set('promise', promise)
+    } else {
+      this._loadingPromise = PromiseBelongsTo.create({
+        _belongsToState: this,
+        promise,
+        content
+      });
+    }
+
+    return this._loadingPromise;
+  }
+
   reload() {
-    // TODO handle case when reload() is triggered multiple times
+    // we've already fired off a request
+    if (this._loadingPromise) {
+      if (this._loadingPromise.get('isPending')) {
+        return this._loadingPromise;
+      }
+    }
+
+    let promise;
+    this.setRelationshipIsStale(true);
 
     if (this.link) {
-      return this.fetchLink();
+      promise = this.fetchLink();
+    } else if (this.inverseInternalModel && this.inverseInternalModel.hasRecord) {
+      // reload record, if it is already loaded
+      promise = this.inverseInternalModel.getRecord().reload();
+    } else {
+      promise = this.findRecord();
     }
 
-    // reload record, if it is already loaded
-    if (this.inverseInternalModel && this.inverseInternalModel.hasRecord) {
-      return this.inverseInternalModel.getRecord().reload();
-    }
+    return this._updateLoadingPromise(promise);
+  }
+
+  localStateIsEmpty() {
+    let internalModel = this.inverseInternalModel;
 
-    return this.findRecord();
+    return !internalModel  || internalModel.isEmpty();
   }
 
   updateData(data, initial) {