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

[BUGFIX release] relationship [x, y] should not break on x.unloadRecord() #5092

Merged
merged 3 commits into from
Jul 25, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 28 additions & 20 deletions addon/-private/system/model/internal-model.js
Original file line number Diff line number Diff line change
Expand Up @@ -430,14 +430,10 @@ export default class InternalModel {
*/
_directlyRelatedInternalModels() {
let array = [];
this.type.eachRelationship((key, relationship) => {
if (this._relationships.has(key)) {
let relationship = this._relationships.get(key);
let localRelationships = relationship.members.toArray();
let serverRelationships = relationship.canonicalMembers.toArray();

array = array.concat(localRelationships, serverRelationships);
}
this._relationships.forEach((name, rel) => {
let local = rel.members.toArray();
let server = rel.canonicalMembers.toArray();
array = array.concat(local, server);
});
return array;
}
Expand Down Expand Up @@ -491,6 +487,7 @@ export default class InternalModel {
unloadRecord() {
this.send('unloadRecord');
this.dematerializeRecord();

if (this._scheduledDestroy === null) {
this._scheduledDestroy = run.schedule('destroy', this, '_checkForOrphanedInternalModels');
}
Expand All @@ -510,6 +507,7 @@ export default class InternalModel {
if (this.isDestroyed) { return; }

this._cleanupOrphanedInternalModels();

}

_cleanupOrphanedInternalModels() {
Expand All @@ -532,6 +530,9 @@ export default class InternalModel {
assert("Cannot destroy an internalModel while its record is materialized", !this._record || this._record.get('isDestroyed') || this._record.get('isDestroying'));

this.store._internalModelDestroyed(this);

this._relationships.forEach((name, rel) => rel.destroy());

this._isDestroyed = true;
}

Expand Down Expand Up @@ -888,14 +889,10 @@ export default class InternalModel {
@private
*/
removeFromInverseRelationships(isNew = false) {
this.eachRelationship((name) => {
if (this._relationships.has(name)) {
let rel = this._relationships.get(name);

rel.removeCompletelyFromInverse();
if (isNew === true) {
rel.clear();
}
this._relationships.forEach((name, rel) => {
rel.removeCompletelyFromInverse();
if (isNew === true) {
rel.clear();
}
});

Expand All @@ -917,17 +914,28 @@ export default class InternalModel {
and destroys any ManyArrays.
*/
destroyRelationships() {
this.eachRelationship((name, relationship) => {
if (this._relationships.has(name)) {
let rel = this._relationships.get(name);
this._relationships.forEach((name, rel) => {
if (rel._inverseIsAsync()) {
rel.removeInternalModelFromInverse(this);
rel.removeInverseRelationships();
} else {
rel.removeCompletelyFromInverse();
}
});

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

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

rel.destroy();
});
}

Expand Down
7 changes: 7 additions & 0 deletions addon/-private/system/relationships/state/create.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,13 @@ export default class Relationships {
return !!this.initializedRelationships[key];
}

forEach(cb) {
let rels = this.initializedRelationships;
Object.keys(rels).forEach(name => {
cb(name, rels[name]);
});
}

get(key) {
let relationships = this.initializedRelationships;
let relationship = relationships[key];
Expand Down
14 changes: 14 additions & 0 deletions addon/-private/system/relationships/state/has-many.js
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,20 @@ export default class ManyRelationship extends Relationship {
this.updateInternalModelsFromAdapter(internalModels);
}
}

destroy() {
super.destroy();
let manyArray = this._manyArray;
if (manyArray) {
manyArray.destroy();
}

let proxy = this.__loadingPromise;

if (proxy) {
proxy.destroy();
}
}
}

function setForArray(array) {
Expand Down
14 changes: 12 additions & 2 deletions addon/-private/system/relationships/state/relationship.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,13 @@ export default class Relationship {
return this.internalModel.modelName;
}

_inverseIsAsync() {
if (!this.inverseKey || !this.inverseInternalModel) {
return false;
}
return this.inverseInternalModel._relationships.get(this.inverseKey).isAsync;
}

removeInverseRelationships() {
if (!this.inverseKey) { return; }

Expand Down Expand Up @@ -174,7 +181,7 @@ export default class Relationship {
let relationship = relationships[this.inverseKeyForImplicit];
if (!relationship) {
relationship = relationships[this.inverseKeyForImplicit] =
new Relationship(this.store, internalModel, this.key, { options: {} });
new Relationship(this.store, internalModel, this.key, { options: { async: this.isAsync } });
}
relationship.addCanonicalInternalModel(this.internalModel);
}
Expand Down Expand Up @@ -215,7 +222,7 @@ export default class Relationship {
internalModel._relationships.get(this.inverseKey).addInternalModel(this.internalModel);
} else {
if (!internalModel._implicitRelationships[this.inverseKeyForImplicit]) {
internalModel._implicitRelationships[this.inverseKeyForImplicit] = new Relationship(this.store, internalModel, this.key, { options: {} });
internalModel._implicitRelationships[this.inverseKeyForImplicit] = new Relationship(this.store, internalModel, this.key, { options: { async: this.isAsync } });
}
internalModel._implicitRelationships[this.inverseKeyForImplicit].addInternalModel(this.internalModel);
}
Expand Down Expand Up @@ -453,4 +460,7 @@ export default class Relationship {
}

updateData() {}

destroy() {
}
}
91 changes: 44 additions & 47 deletions tests/integration/records/rematerialize-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,22 +146,47 @@ test("an async has many relationship to an unloaded record can restore that reco
// disable background reloading so we do not re-create the relationship.
env.adapter.shouldBackgroundReloadRecord = () => false;

env.adapter.findRecord = function() {
assert.ok('adapter called');
return Ember.RSVP.Promise.resolve({
data: {
type: 'boat',
id: '1',
attributes: {
name: "Boaty McBoatface"
},
relationships: {
person: {
data: { type: 'person', id: '1' }
}
}
const BOAT_ONE = {
type: 'boat',
id: '1',
attributes: {
name: "Boaty McBoatface"
},
relationships: {
person: {
data: { type: 'person', id: '1' }
}
});
}
};

const BOAT_TWO = {
type: 'boat',
id: '2',
attributes: {
name: 'Some other boat'
},
relationships: {
person: {
data: { type: 'person', id: '1' }
}
}
};

env.adapter.findRecord = function(store, model, param) {
assert.ok('adapter called');

let data;
if (param === '1') {
data = BOAT_ONE;
} else if (param === '1') {
data = BOAT_TWO;
} else {
throw new Error(`404: no such boat with id=${param}`);
}

return {
data
};
}

run(function() {
Expand All @@ -186,29 +211,7 @@ test("an async has many relationship to an unloaded record can restore that reco

run(function() {
env.store.push({
data: [{
type: 'boat',
id: '2',
attributes: {
name: 'Some other boat'
},
relationships: {
person: {
data: { type: 'person', id: '1' }
}
}
}, {
type: 'boat',
id: '1',
attributes: {
name: 'Boaty McBoatface'
},
relationships: {
person: {
data: { type: 'person', id: '1' }
}
}
}]
data: [BOAT_ONE, BOAT_TWO]
});
});

Expand All @@ -220,22 +223,16 @@ test("an async has many relationship to an unloaded record can restore that reco
assert.equal(env.store.hasRecordForId('boat', 1), true, 'The boat is in the store');
assert.equal(env.store._internalModelsFor('boat').has(1), true, 'The boat internalModel is loaded');

let boats = run(() => {
return adam.get('boats');
});
let boats = run(() => adam.get('boats'));

assert.equal(boats.get('length'), 2, 'Before unloading boats.length is correct');

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

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');

let rematerializedBoaty = run(() => {
return rematerializedBoaty = adam.get('boats').objectAt(1);
});
let rematerializedBoaty = run(() => adam.get('boats')).objectAt(0);

assert.equal(adam.get('boats.length'), 2, 'boats.length correct after rematerialization');
assert.equal(rematerializedBoaty.get('id'), '1');
Expand Down
Loading