Rollback Relationships

[mmpestorich]

This commit:

1. Allows one to rollback belongsTo and hasMany relationships.
2. Reintroduces dirtyRecordFor*Change hooks on the adapter that allow one to customize when a record becomes dirty.
3. Added removeDeletedFromRelationshipsPriorToSave flag to Adapter that allows one to opt back into the old deleted record from many array behavior (pre Only remove deleted records form record arrays when saved #3539).

Known issues:

• Rolling back a hasMany relationship from the parent side of the relationship does not work (doing the same from the child side works fine). See test that is commented out below as well as the discussion at the end of #2881#issuecomment-204634262

This was previously #2881 and is related to #3698

[frunjik]

What is the status of this?

• Will it work in current ember-data version ?
• If not what is missing for this to be accepted ?
• If this is not a good solution, what is the (long term) solution for rollback (relations) ?

[Awem]

This is needed. How can I help to get this merged?

[tamebadger]

Hi guys, also keen on this to get merged. Willing to help out as well, so let me know :)

[fivetanley]

what's up with these commented tests? how can i help?

[mmpestorich]

The commented tests are for rolling back a hasMany relationship (via parent.rollback() which I don't think is proper behavior) instead of rolling back each modified child. Basically, I left it in here to get feedback from people on whether this behavior should be supported or not...

// for something like:
//  parent: {
//    children: [ child1, child2 ]
//  }

parent.get('children').removeObject(child1)  // children: [ child2 ]
parent.get('children').addObject(child3)     // children: [ child2, child3 ]

parent.rollback() 
// some people believe this should result in `children: [ child1, child2 ]`;
// however the parent is not what is actually changing here
// so rolling it back doesn't really make sense; 
// child1 and child3 are actually what became dirty

[child1, child3].invoke('rollback'); // children: [ child1, child2 ]

This was sort of previously discussed in #2881...

Some have suggested to me that one should be able to rollback a hasMany relationship via the parent as described above. After relationships were refactored it became rather simple to rollback each childs' belongsTo but it is not obvious how to do the same from the parent's hasMany...

In order to rollback from the parent side it's not just as simple as reverting back to the relationship's canonical state... For example, there may have been unsaved objects added to that relationship. What would one do with those? Blindly delete them? What if they previously belonged to another parent? Do we blindly add them back to that other parents relationship? What if that other parent had been changed in the meantime and been saved, now what happens to that child?

I'm not really sure that supporting rollback from the parent in this case makes much sense. Each modified child basically has to be handled on a case by case basis anyway to achieve an expected result. The child is what points to - belongs to - a parent, from a rollback perspective, a belongsTo relationship is similar an attribute and very easy to reason about. The same can't be said of the array of objects one has to work from the hasMany side of things...

[tamebadger]

So I'll add my 2c:

In order to rollback from the parent side it's not just as simple as reverting back to the relationship's canonical state... For example, there may have been unsaved objects added to that relationship. What would one do with those? Blindly delete them? What if they previously belonged to another parent? Do we blindly add them back to that other parents relationship? What if that other parent had been changed in the meantime and been saved, now what happens to that child?

So when I think of parent.rollback() in the above code you noted, I see it simply as, rollback changes that occurred on that "parent" record where changes have been only the relationship state between it and the child records. The unsaved objects that are children should be handled as a separate concern, where you reference them and roll them back should you want to. Or alternatively when a parent wants to rollback more than just relationship state, it should be a different method or an option passed to rollback ? parent.rollback({cascade:true}) or parent.rollbackChildren()

[tamebadger]

So after thinking about this more, it is probably more relevant to restart the discussion on emberjs/rfcs#21 about the above concerns. What could be better than specifying options in rollback() might be the suggested approach in the rfc along the lines of posts: hasMany('post', { dirties: 'relationship' }), etc and rollback just reading from that initial config instead.

[BryanCrotaz]

I did a lot of work with a Delphi and C# ORM which went down this road by defining object space transactions. You don't roll back a parent, you roll back a transaction. This ensures that inverse relationships stay in sync - nothing worse than rolling back half of an inverse pair!

[mmpestorich]

Updated for current master (2.15.2). All existing tests are passing. Probably needs a few more, as well as some cleanup.

One big change in the last couple years or at least since I last updated this was #3539. Basically since that pull request was merged, deleted records are no longer removed from related many arrays on delete. Removal only happens after a save.

When considering rollback scenarios of has many relationships, I'd imagine there's a lot of people out there that would expect a record to actually go away when its deleted (not just be marked as such). Our entire code base expects deleted records to be removed from the UI prior to save. Short of adding filters all over the place the same behavoir can't be replicated with a current version of ember-data. In any case, while bringing this PR back inline with master, I added a flag on the adapter that allows one to opt back into the old deleted record from many array behavior (removeDeletedFromRelationshipsPriorToSave).

Everything else is the same as discussed above.

[koemeet]

Any new things on this subject?

[mmpestorich]

Updated for 2.18.0 LTS Release and current master (3.0?). Branches specific to older versions of ember-data are still available here: https://github.com/swarmbox/data/branches/all

[runspired]

Closing this in favor of addons like ember-data-changetracker and use of a RecordData based approach. That said improved relationship primitives are in the works for RecordData authors to make use of, and would include tracked changes for easier rollback and diffing.

[mmpestorich]

I'm not sure how I missed the closing of this pull request, but for those interested, we have a branch up that works with the 3.12.x LTS Releases that you can find here: https://github.com/swarmbox/data/branches/all

I strongly disagree that rolling back relationship changes belongs in an addon. This is functionality that I believe any developer would expect to be included in a data object abstraction library like ember data.

@runspired This pull request has made every attempt to stay current and use a RecordData based approach, which ember-data-changetracker does not (even though it offers similar functionality and seems to be a well designed approach to address the same problem). Maybe when relationship primitives have been finalized you guys would be willing to revisit something like this???

[runspired]

@mmpestorich we were discussing this in our meeting today and we've got a few action items:

First, we'd love to have you join us in these meetings. These capabilities are capabilities we discuss and put effort into fairly regularly, and the complexity around solving them is high. You can find us in the dev-ember-data channel on discord and our weekly meeting info is here

Second, we want to clarify what I mean when we'd prefer an add-on approach. EmberData wants to provide the capability for an add-on to do this effectively (and easily), but since there is no general solution and the best (more general) API pattern unclear, we don't believe EmberData itself belongs providing the entirety of this feature. We'd love to have your help!