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] keep local state after a deletion #5058

Merged
merged 16 commits into from
Jul 14, 2017
46 changes: 35 additions & 11 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._resetAllRelationshipsToEmpty();
}

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

/*
@method clearRelationships
This method should only be called by records in the `isNew()` state.
It will completely reset all relationships to an empty state and remove
the record from any associated inverses as well.

@method _resetAllRelationshipsToEmpty
@private
*/
clearRelationships() {
_resetAllRelationshipsToEmpty() {
Copy link
Member

Choose a reason for hiding this comment

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

What is the difference in behavior between the isNew and the deletion case?

this.eachRelationship((name, relationship) => {
if (this._relationships.has(name)) {
let rel = this._relationships.get(name);

rel.clear();
rel.removeInverseRelationships();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hjdivad commenting these out didn't make any tests fail o.O

Copy link
Member

Choose a reason for hiding this comment

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

hmm yeah it looks like the unload tests assert against the id map and the relationship payloads, but not against relationships (either our side or inverse).

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

rel.clear();
rel.removeInverseRelationships();
});
}

/*
This method should only be called once the record has been deleted
and that deletion has been persisted. It will remove this record from
any associated relationships.

@method unloadDeletedRecordFromRelationships
@private
*/
unloadDeletedRecordFromRelationships() {
Copy link
Member

Choose a reason for hiding this comment

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

per our chat out of band, let's rename this to not conflate this case with unloadRecord

Copy link
Member

Choose a reason for hiding this comment

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

Also let's add some comments here as breadcrumbs to our future selves who will clean this up.

This does fix an important bug, but currently there are two codepaths whose different semantics are not obvious within the same level of abstraction, ie that rel.clear() causes a flushCanonical but unloadDeletedInverseInternalModel does not is not clear, and that this is the entire point is also not clear.

Copy link
Member

Choose a reason for hiding this comment

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

We use unload as the term for unloading records, maybe we should call this method something else, like remove?

Copy link
Member

Choose a reason for hiding this comment

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

Absolutely.

@runspired I guess we missed an instance of the renaming

Copy link
Contributor Author

Choose a reason for hiding this comment

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

confirm, will rename

Copy link
Member

Choose a reason for hiding this comment

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

How about: removeFromInverseRelationships, example:

internalModel.removeFromInverseRelationships();

The method would now describes concisely what is does not when to use it. The when and why should be provided via docs, or derived contextually from where it is used.

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.eachRelationship((name) => {
if (this._relationships.has(name)) {
let rel = this._relationships.get(name);

rel.removeDeletedInternalModelFromInverse(this);
}
});
Object.keys(this._implicitRelationships).forEach((key) => {
let rel = this._implicitRelationships[key];

rel.removeDeletedInternalModelFromInverse(this);
});
}

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.unloadDeletedRecordFromRelationships();
Copy link
Member

Choose a reason for hiding this comment

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

internalModel.removeFromInverseRelationships()

},

invokeLifecycleCallbacks(internalModel) {
Expand Down
19 changes: 19 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,25 @@ export default class BelongsToRelationship extends Relationship {
this.notifyBelongsToChanged();
}

removeDeletedInternalModelFromOwn(internalModel) {
super.removeDeletedInternalModelFromOwn(internalModel);
Copy link
Member

Choose a reason for hiding this comment

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

I thought we moved away from calling super in relationships?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We never landed that

Copy link
Member

Choose a reason for hiding this comment

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

:(

let didChange = false;

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

if (this.inverseInternalModel === internalModel) {
this.inverseInternalModel = this.canonicalState;
didChange = true;
}

if (didChange === true) {
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);
}

removeDeletedInternalModelFromOwn(internalModel) {
Copy link
Member

Choose a reason for hiding this comment

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

seems related to removeInternalModelFromOwn, why diverge?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removeInternalModelFromOwn removes only current state while removeDeletedInternalModelFromOwn removes from current and canonical state.

super.removeDeletedInternalModelFromOwn(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
38 changes: 38 additions & 0 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 @@ -266,6 +269,41 @@ 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 removeDeletedInternalModelFromInverse
@private
*/
removeDeletedInternalModelFromInverse(internalModel) {
Copy link
Member

Choose a reason for hiding this comment

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

what's the case where internalModel != this.internalModel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there isn't one afaik, just seemed better to pass this in with the original implementation. using this.internalModel is probably a bit safer

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 unload = inverseInternalModel => {
const id = guidFor(inverseInternalModel);

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

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

removeDeletedInternalModelFromOwn(internalModel) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe Deleted should be removed from this record name. In general a method name should describe what it does, not why or when it should be invoked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will follow guidance on your other comment for renaming

Copy link
Member

Choose a reason for hiding this comment

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

@runspired this function is quite similar to removeInternalModelFromOwn what is the divergence?

Copy link
Member

@stefanpenner stefanpenner Jul 14, 2017

Choose a reason for hiding this comment

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

The divergence (but feel free to confirm):

  • removeInternalModelFromOwn: is removing from current state (awaiting future canonical flush)
  • removeDeletedInternalModelFromOwn: is removing from both current state (members) and canonical state (canonicalMembers)

The naming of these methods sorta blows, but naming is hard. So at the very least we may need to document them somewhat.

Some ideas:

Removes a given internal model from the relationships currentState, but not from that relationships canonicalState. This results in an optimistic delete, and subsequent confirmation (typically server side) will update the canonicalState.

@method removeInternalModelFromOwn
@param internalModel
Removes a given internal model from both a relationships currentState and canonical state. This typically occurs when an internalRecord's deletion is persisted.

@method removeDeletedInternalModelFromOwn
@param internalModel

this.canonicalMembers.delete(internalModel);
this.members.delete(internalModel);
this.notifyRecordRelationshipRemoved(internalModel);
Copy link
Member

Choose a reason for hiding this comment

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

does this method do anything? I only see two implementations and they appear to be noops.

https://github.com/emberjs/data/search?utf8=%E2%9C%93&q=notifyRecordRelationshipRemoved&

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this was put here for something with dematerialization that was never completed maybe?

}

flushCanonical() {
heimdall.increment(flushCanonical);
let list = this.members.list;
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