Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from
Apr 6, 2018
6 changes: 4 additions & 2 deletions addon/-private/system/model/internal-model.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
26 changes: 24 additions & 2 deletions addon/-private/system/promise-proxies.js
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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
);
}),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sidenote: this is actually true for hasMany as well. It does proxy meta, however it rarely picks up the change notification (if ever). We should address this soon.


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.
Expand Down Expand Up @@ -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;
},

Expand Down
4 changes: 2 additions & 2 deletions addon/-private/system/references/has-many.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably be noted for a follow up PR to be changed. No tests failed but now that we've clarified what the flags are it is rather obvious that this is not correct.

return false;
}

Expand Down
96 changes: 78 additions & 18 deletions addon/-private/system/relationships/relationship-payloads.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,30 @@
import { assert } from '@ember/debug';

/**
* Merge data,meta,links information forward to the next payload
* if required.
*
* @param oldPayload
* @param newPayload
*/
function mergeForwardPayload(oldPayload, newPayload) {
if (oldPayload && oldPayload.data !== undefined && newPayload.data === undefined) {
newPayload.data = oldPayload.data;
}

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 {
Expand Down Expand Up @@ -204,7 +229,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;
Expand Down Expand Up @@ -257,14 +281,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`.

Expand Down Expand Up @@ -306,30 +350,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);
Expand Down Expand Up @@ -357,7 +412,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 }
Expand All @@ -367,10 +425,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 {
Expand All @@ -388,15 +446,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;
}
}
}
72 changes: 50 additions & 22 deletions addon/-private/system/relationships/state/belongs-to.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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) {
Expand Down
Loading