Skip to content

Commit

Permalink
Merge pull request emberjs#5153 from emberjs/fix-destroy-relationship
Browse files Browse the repository at this point in the history
[BUGFIX release] ensure inverse async HasMany is correctly maintained
  • Loading branch information
stefanpenner authored Aug 25, 2017
2 parents 50592d8 + 5f5d5d1 commit 441e48c
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 41 deletions.
31 changes: 13 additions & 18 deletions addon/-private/system/model/internal-model.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,14 @@ function areAllModelsUnloaded(internalModels) {
return true;
}

function destroyRelationship(rel) {
if (rel._inverseIsAsync()) {
rel.removeInternalModelFromInverse(rel.inverseInternalModel);
rel.removeInverseRelationships();
} else {
rel.removeCompletelyFromInverse();
}
}
// this (and all heimdall instrumentation) will be stripped by a babel transform
// https://github.com/heimdalljs/babel5-plugin-strip-heimdall
const {
Expand Down Expand Up @@ -432,9 +440,7 @@ export default class InternalModel {
_directlyRelatedInternalModels() {
let array = [];
this._relationships.forEach((name, rel) => {
let local = rel.members.toArray();
let server = rel.canonicalMembers.toArray();
array = array.concat(local, server);
array = array.concat(rel.members.list, rel.canonicalMembers.list);
});
return array;
}
Expand Down Expand Up @@ -486,6 +492,7 @@ export default class InternalModel {
once all models that refer to it via some relationship are also unloaded.
*/
unloadRecord() {
if (this.isDestroyed) { return; }
this.send('unloadRecord');
this.dematerializeRecord();

Expand Down Expand Up @@ -539,7 +546,6 @@ export default class InternalModel {
if (this.isDestroyed) { return; }

this._cleanupOrphanedInternalModels();

}

_cleanupOrphanedInternalModels() {
Expand Down Expand Up @@ -946,26 +952,15 @@ export default class InternalModel {
and destroys any ManyArrays.
*/
destroyRelationships() {
this._relationships.forEach((name, rel) => {
if (rel._inverseIsAsync()) {
rel.removeInternalModelFromInverse(this);
rel.removeInverseRelationships();
} else {
rel.removeCompletelyFromInverse();
}
});
let relationships = this._relationships;
relationships.forEach((name, rel) => destroyRelationship(rel));

let implicitRelationships = this._implicitRelationships;
this.__implicitRelationships = null;
Object.keys(implicitRelationships).forEach((key) => {
let rel = implicitRelationships[key];

if (rel._inverseIsAsync()) {
rel.removeInternalModelFromInverse(this);
rel.removeInverseRelationships();
} else {
rel.removeCompletelyFromInverse();
}
destroyRelationship(rel);

rel.destroy();
});
Expand Down
7 changes: 4 additions & 3 deletions addon/-private/system/relationships/state/relationship.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,13 @@ export default class Relationship {
let allMembers =
// we actually want a union of members and canonicalMembers
// they should be disjoint but currently are not due to a bug
this.members.toArray().concat(this.canonicalMembers.toArray());
this.members.list.concat(this.canonicalMembers.list);

allMembers.forEach(inverseInternalModel => {
for (let i = 0; i < allMembers.length; i++) {
let inverseInternalModel = allMembers[i];
let relationship = inverseInternalModel._relationships.get(this.inverseKey);
relationship.inverseDidDematerialize();
});
}
}

inverseDidDematerialize() {}
Expand Down
23 changes: 11 additions & 12 deletions tests/integration/records/rematerialize-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,10 @@ module("integration/unload - Rematerializing Unloaded Records", {
});

test("a sync belongs to relationship to an unloaded record can restore that record", function(assert) {
let adam, bob;

// disable background reloading so we do not re-create the relationship.
env.adapter.shouldBackgroundReloadRecord = () => false;

run(function() {
let adam = run(() => {
env.store.push({
data: {
type: 'person',
Expand All @@ -79,10 +77,11 @@ test("a sync belongs to relationship to an unloaded record can restore that reco
}
}
});
adam = env.store.peekRecord('person', 1);

return env.store.peekRecord('person', 1);
});

run(function() {
let bob = run(() => {
env.store.push({
data: {
type: 'car',
Expand All @@ -98,7 +97,8 @@ test("a sync belongs to relationship to an unloaded record can restore that reco
}
}
});
bob = env.store.peekRecord('car', 1);

return env.store.peekRecord('car', 1);
});

let person = env.store.peekRecord('person', 1);
Expand All @@ -107,9 +107,7 @@ test("a sync belongs to relationship to an unloaded record can restore that reco
assert.equal(env.store.hasRecordForId('person', 1), true, 'The person is in the store');
assert.equal(env.store._internalModelsFor('person').has(1), true, 'The person internalModel is loaded');

run(function() {
person.unloadRecord();
});
run(() => person.unloadRecord());

assert.equal(env.store.hasRecordForId('person', 1), false, 'The person is unloaded');
assert.equal(env.store._internalModelsFor('person').has(1), true, 'The person internalModel is retained');
Expand Down Expand Up @@ -141,7 +139,7 @@ test("a sync belongs to relationship to an unloaded record can restore that reco
});

test("an async has many relationship to an unloaded record can restore that record", function(assert) {
assert.expect(14);
assert.expect(15);

// disable background reloading so we do not re-create the relationship.
env.adapter.shouldBackgroundReloadRecord = () => false;
Expand Down Expand Up @@ -189,7 +187,7 @@ test("an async has many relationship to an unloaded record can restore that reco
};
}

run(function() {
run(() => {
env.store.push({
data: {
type: 'person',
Expand All @@ -209,7 +207,7 @@ test("an async has many relationship to an unloaded record can restore that reco
});
});

run(function() {
run(() => {
env.store.push({
data: [BOAT_ONE, BOAT_TWO]
});
Expand All @@ -228,6 +226,7 @@ test("an async has many relationship to an unloaded record can restore that reco
assert.equal(boats.get('length'), 2, 'Before unloading boats.length is correct');

run(() => boaty.unloadRecord());
assert.equal(boats.get('length'), 1, 'after unloading boats.length is correct');

assert.equal(env.store.hasRecordForId('boat', 1), false, 'The boat is unloaded');
assert.equal(env.store._internalModelsFor('boat').has(1), true, 'The boat internalModel is retained');
Expand Down
16 changes: 8 additions & 8 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,12 @@ [email protected]:
dependencies:
ensure-posix-path "^1.0.1"

[email protected]:
version "0.0.7"
resolved "https://registry.yarnpkg.com/amd-name-resolver/-/amd-name-resolver-0.0.7.tgz#814301adfe8a2f109f6e84d5e935196efb669615"
dependencies:
ensure-posix-path "^1.0.1"

amdefine@>=0.0.4:
version "1.0.1"
resolved "https://registry.npmjs.org/amdefine/-/amdefine-1.0.1.tgz#4a5282ac164729e93619bcfd3ad151f817ce91f5"
Expand Down Expand Up @@ -1018,7 +1024,7 @@ [email protected]:
version "0.2.2"
resolved "https://registry.npmjs.org/bower-endpoint-parser/-/bower-endpoint-parser-0.2.2.tgz#00b565adbfab6f2d35addde977e97962acbcb3f6"

bower@^1.6.5, bower@^1.8.0:
bower@^1.8.0:
version "1.8.0"
resolved "https://registry.npmjs.org/bower/-/bower-1.8.0.tgz#55dbebef0ad9155382d9e9d3e497c1372345b44a"

Expand Down Expand Up @@ -1484,13 +1490,7 @@ [email protected]:
version "2.4.0"
resolved "https://registry.npmjs.org/bytes/-/bytes-2.4.0.tgz#7d97196f9d5baf7f6935e25985549edd2a6c2339"

calculate-cache-key-for-tree@^1.0.0:
version "1.1.0"
resolved "https://registry.npmjs.org/calculate-cache-key-for-tree/-/calculate-cache-key-for-tree-1.1.0.tgz#0c3e42c9c134f3c9de5358c0f16793627ea976d6"
dependencies:
json-stable-stringify "^1.0.1"

calculate-cache-key-for-tree@^1.1.0:
calculate-cache-key-for-tree@^1.0.0, calculate-cache-key-for-tree@^1.1.0:
version "1.1.0"
resolved "https://registry.yarnpkg.com/calculate-cache-key-for-tree/-/calculate-cache-key-for-tree-1.1.0.tgz#0c3e42c9c134f3c9de5358c0f16793627ea976d6"
dependencies:
Expand Down

0 comments on commit 441e48c

Please sign in to comment.