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] Preserve local relationship changes after persisting a delet… #5067

Merged
merged 1 commit into from
Jul 16, 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
46 changes: 31 additions & 15 deletions addon/-private/system/model/internal-model.js
Original file line number Diff line number Diff line change
Expand Up @@ -728,12 +728,6 @@ export default class InternalModel {
}
}

notifyHasManyRemoved(key, record, idx) {
if (this.hasRecord) {
this._record.notifyHasManyRemoved(key, record, idx);
}
}

notifyBelongsToChanged(key, record) {
if (this.hasRecord) {
this._record.notifyBelongsToChanged(key, record);
Expand Down Expand Up @@ -768,7 +762,7 @@ export default class InternalModel {
}

if (this.isNew()) {
this.clearRelationships();
this.removeFromInverseRelationships(true);
}

if (this.isValid()) {
Expand Down Expand Up @@ -882,23 +876,43 @@ export default class InternalModel {
}

/*
@method clearRelationships
This method should only be called by records in the `isNew()` state OR once the record
has been deleted and that deletion has been persisted.

It will remove this record from any associated relationships.

If `isNew` is true (default false), it will also completely reset all
relationships to an empty state as well.

@method removeFromInverseRelationships
@param {Boolean} isNew whether to unload from the `isNew` perspective
@private
*/
clearRelationships() {
this.eachRelationship((name, relationship) => {
*/
removeFromInverseRelationships(isNew = false) {
this.eachRelationship((name) => {
if (this._relationships.has(name)) {
let rel = this._relationships.get(name);
rel.clear();
rel.removeInverseRelationships();

rel.removeCompletelyFromInverse();
if (isNew === true) {
rel.clear();
}
}
});
Object.keys(this._implicitRelationships).forEach((key) => {
this._implicitRelationships[key].clear();
this._implicitRelationships[key].removeInverseRelationships();
let rel = this._implicitRelationships[key];

rel.removeCompletelyFromInverse();
if (isNew === true) {
rel.clear();
}
});
}

/*
Notify all inverses that this internalModel has been dematerialized
and destroys any ManyArrays.
*/
destroyRelationships() {
this.eachRelationship((name, relationship) => {
if (this._relationships.has(name)) {
Expand Down Expand Up @@ -982,6 +996,8 @@ export default class InternalModel {
}

/*
Used to notify the store to update FilteredRecordArray membership.

@method updateRecordArrays
@private
*/
Expand Down
2 changes: 1 addition & 1 deletion addon/-private/system/model/states.js
Original file line number Diff line number Diff line change
Expand Up @@ -671,7 +671,7 @@ const RootState = {
isDirty: false,

setup(internalModel) {
internalModel.clearRelationships();
internalModel.removeFromInverseRelationships();
},

invokeLifecycleCallbacks(internalModel) {
Expand Down
2 changes: 1 addition & 1 deletion addon/-private/system/record-array-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ export default class RecordArrayManager {
return;
}

internalModel._pendingRecordArrayManagerFlush = true
internalModel._pendingRecordArrayManagerFlush = true;

let pending = this._pending;
let models = pending[modelName] = pending[modelName] || [];
Expand Down
13 changes: 13 additions & 0 deletions addon/-private/system/relationships/state/belongs-to.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,19 @@ export default class BelongsToRelationship extends Relationship {
this.notifyBelongsToChanged();
}

removeCompletelyFromOwn(internalModel) {
super.removeCompletelyFromOwn(internalModel);

if (this.canonicalState === internalModel) {
this.canonicalState = null;
}

if (this.inverseInternalModel === internalModel) {
this.inverseInternalModel = null;
this.notifyBelongsToChanged();
}
}

flushCanonical() {
//temporary fix to not remove newly created records if server returned null.
//TODO remove once we have proper diffing
Expand Down
20 changes: 20 additions & 0 deletions addon/-private/system/relationships/state/has-many.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,26 @@ export default class ManyRelationship extends Relationship {
super.removeCanonicalInternalModelFromOwn(internalModel, idx);
}

removeCompletelyFromOwn(internalModel) {
super.removeCompletelyFromOwn(internalModel);

const canonicalIndex = this.canonicalState.indexOf(internalModel);

if (canonicalIndex !== -1) {
this.canonicalState.splice(canonicalIndex, 1);
}

const manyArray = this._manyArray;

if (manyArray) {
const idx = manyArray.currentState.indexOf(internalModel);

if (idx !== -1) {
manyArray.internalReplace(idx, 1);
}
}
}

flushCanonical() {
if (this._manyArray) {
this._manyArray.flushCanonical();
Expand Down
47 changes: 45 additions & 2 deletions addon/-private/system/relationships/state/relationship.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
import { assert, warn } from '@ember/debug';
import OrderedSet from '../../ordered-set';
import _normalizeLink from '../../normalize-link';
import Ember from 'ember';

const { guidFor } = Ember;

const {
addCanonicalInternalModel,
Expand Down Expand Up @@ -247,7 +250,6 @@ export default class Relationship {
removeInternalModelFromOwn(internalModel) {
heimdall.increment(removeInternalModelFromOwn);
this.members.delete(internalModel);
this.notifyRecordRelationshipRemoved(internalModel);
this.internalModel.updateRecordArrays();
}

Expand All @@ -266,6 +268,48 @@ export default class Relationship {
this.flushCanonicalLater();
}

/*
Call this method once a record deletion has been persisted
to purge it from BOTH current and canonical state of all
relationships.

@method removeCompletelyFromInverse
@private
*/
removeCompletelyFromInverse() {
if (!this.inverseKey) { return; }

// we actually want a union of members and canonicalMembers
// they should be disjoint but currently are not due to a bug
let seen = Object.create(null);
const internalModel = this.internalModel;

const unload = inverseInternalModel => {
const id = guidFor(inverseInternalModel);

if (seen[id] === undefined) {
const relationship = inverseInternalModel._relationships.get(this.inverseKey);
relationship.removeCompletelyFromOwn(internalModel);
seen[id] = true;
}
};

this.members.forEach(unload);
this.canonicalMembers.forEach(unload);
}

/*
Removes the given internalModel from BOTH canonical AND current state.

This method is useful when either a deletion or a rollback on a new record
needs to entirely purge itself from an inverse relationship.
*/
removeCompletelyFromOwn(internalModel) {
this.canonicalMembers.delete(internalModel);
this.members.delete(internalModel);
this.internalModel.updateRecordArrays();
}

flushCanonical() {
heimdall.increment(flushCanonical);
let list = this.members.list;
Expand Down Expand Up @@ -327,7 +371,6 @@ export default class Relationship {
}

notifyRecordRelationshipAdded() { }
notifyRecordRelationshipRemoved() { }

/*
`hasData` for a relationship is a flag to indicate if we consider the
Expand Down
4 changes: 3 additions & 1 deletion tests/integration/relationships/has-many-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2520,7 +2520,9 @@ test("adding and removing records from hasMany relationship #2666", function(ass
return comments.get('lastObject').destroyRecord();
}).then(() => {
let comments = post.get('comments');
assert.equal(comments.get('length'), 3, "Comments count after destroy");
let length = comments.get('length');

assert.equal(length, 3, "Comments count after destroy");

// Add another comment #4
let comment = env.store.createRecord('comment');
Expand Down
Loading