Rollback Relationships

[mmpestorich]

This commit allows relationships to be rolledback and also reintroduces dirtyRecordFor*Change hooks on the adapter.

[fivetanley]

Thanks! I will bring this up at friday's meeting.

[jsedlacek]

Thanks @mmpestorich! This solves a very painful point of ember data. Are there any news on having this merged?

[topaxi]

What is the state on this? We're currently managing our relation rollback by hand and it gets very confusing sometimes.

[dallasread]

+1

[bmac]

I'm going to close this pr. @igorT is working on an RFC to describe what needs to happen to for Ember Data to support relationship rollback.

[mmpestorich]

@bmac This pull request implements rollback for both belongsTo and hasMany relationships. @igorT work in #3698 only addresses belongsTo relationships.

If it's a matter of more discussion, conforming to an RFC, cleaning up the code, etc... please point me in the right direction. I'll gladly help in anyway that I can. This PR has been sitting out here for over a year now with no feedback from the ember-data team... slightly frustrating. Prior to this, I had pushed up #1631 - which I closed after being told that rolling back relationships would be addressed by the single-source-of-truth branch - ulitmately it wasn't.

The bottom line is that this PR works and has a handful of passing tests and it's been over 2 years since ember-data was able to rollback relationships (when this behavoir was removed from ember-data, it was sold as a temporary thing). I'd gladly contribute more time and energy to this in order to put something together that you all would feel comfortable accepting... anything I can do please let me know.

There's up to date version of this commit here

[koemeet]

@mmpestorich It looks really nice, but I am wondering if it is also working correctly after you've saved a record. I've also implemented something like this myself, but canonicalState of a relationship isn't updated with the new saved records after a successful save. It might also be a good idea to test that, since I think it is broken in this PR too (still have to test it out, but wanted to leave this note here).

[mmpestorich]

@steffenbrem Hm... our UI doesn't allow a rollback after a record has been saved so I don't think I've ever encountered that. I'll create a test and look into what it would take to handle that properly.

[koemeet]

@mmpestorich I think it is a common scenario, lets say you have a parent record and you add a child record on it's children relationship and save parent. Now you have a saved state of parent that contains one child. Next, you edit the parent record and add another child, it now has two children. This time, we do not save it, but we rollback the parent record. Right there, the issue exposes itself, since you now have zero children after the rollback (because of the canonicalState I think).

A code example to illustratie this use-case:

parent.get(‘children’); // []

// we add our first child
parent.get(‘children’).pushObject(child1);

// we save parent and it's linked child record
parent.save().then(() => {

    // add another child
    parent.get(‘children’).pushObject(child2);
    parent.rollback();

    // this will be empty, but you would expect it to contain `child1`
    parent.get(‘children’); // []
});

[koemeet]

A way to solve it is to let the API return the ids of the children relationship of the parent record. It will than update the canonicalState appropriately. So having a related link for the relationship will break proper rollback of saved records. I do think that it might be good to make the currentState the canonicalState of all relationships if the save was successful, unregarded if the PATCH request includes the ids in the response payload. It seems a little un-efficient to include all relationships in the API response just to properly update the canonicalState.

[mmpestorich]

@steffenbrem Ok, couple things....

I added a test that passes the parent-child scenario you outlined above. Both currentState and canonicalState are maintained properly.

The dirtyRecordFor hooks that I added back to DS.Adapter, by default, won't dirty the parent when any of its hasMany relationships change. The parent doesn't actually change, but rather the child that points to the parent does. I'm making an assumption here (that I probably shouldn't be) about the type of backend/database in use. If you're using an RDMS, there's no reason to dirty the parent in this instance as there's nothing to actually persist to the parent. It's the child that actually maintains all the relational data in an RDMS type system. So, along this line of thinking, you would save or rollback the child and not the parent when the relationship between the two change. Using this child-up approach properly updates the canonicalState of the relationship.

With that said, I realize that there are a lot of people using some form or fashion of a NoSQL database in which it might make more sense to simply save or rollback the parent. To handle that, the dirtyRecordFor hooks could be easily modified to dirty the parent when a child is added or removed from a hasMany relationship. However, I believe that you might be right in that the canoncialState is not kept up to date when you take this parent-down approach. Though I haven't really tested this scenario...

[koemeet]

@mmpestorich Thanks for taking time to look into this 😃! You are indeed right that people manage their relationships differently in Ember Data, but I think there should be a mechanism build in to update the canonicalState of relationships when the parent is saved. I am using a RDMS btw (not that important), and do persist relationship changes through the parent, simply because it requires much less requests. I don't think saving every relationship independently of the parent is a good solution (at least not in my apps). Right now I use the include trick when saving, to let ember data reload the relationships and so the canonical state.

Do you have an idea on how you can update the canonicalState of all relationships of a parent record that has been saved (without including every relationship in the response)? I have tried to fix this, but I lack the knowledge for it.

[mmpestorich]

@steffenbrem

people manage their relationships differently in Ember Data

😜 Well it just wouldn't be fun if everyone did everything the same way!

[I] do persist relationship changes through the parent, simply because it requires much less requests. I don't think saving every relationship independently of the parent is a good solution

I actually agree. On our end we added a commitRecords method to DS.Store for just that reason. You can look at that code here if you're interested. Of course, you'd also have to implement something on the backend as well that can handle the request it sends off.

To handle commits and rollbacks for multiple records, we rolled our own TransactionMixin. In conjunction with the commitRecords code, commiting and rollingback multiple records at one time is seamless and transparent. Here's the gist of our transaction related code:

Basically, we extend DS.Model to notify the current Route when it becomes dirty or clean (our models all extend this instead of DS.Model):

var TransactionEnabledModel = DS.Model.extend({

  _isDirtyDidChange: function () {
    var router = Ember.getOwner(this).lookup('router:main');
    var activeTransition = router.get('router.activeTransition');
    var record = this;

    if (!router.router.currentHandlerInfos) { return; }

    handleDirtyChange(activeTransition || router);

    function handleDirtyChange(handler) {
      if (record.get('isDirty')) {
        handler.send('modelBecameDirty', record);
      } else {
        handler.send('modelBecameClean', record);
      }
    }
  }.observes('currentState.isDirty'),
});

Then we add our TransactionMixin to routes that manage persistent data. Its purpose is to respond to the modelBecameDiry and modelBecameClean notifications that originate from our models. In addition, it handles commit and rollback notifications triggered by a user action on the current page:

var TransactionMixin = Ember.Mixin.create({

  transaction: Transaction.create(),

  commitPromise: null,

  commitTransaction: function () {
    var self = this;

    this.get('commitPromise', this.get('transaction').commit(this.store))
                                                     .then(saveSuccess)
                                                     .catch(saveFail)
                                                     .finally(clearPromise);

    function clearPromise() {
      self.set('commitPromise', null);
    }
  },

  actions: {

    modelBecameDirty: function (model) {
      this.get('transaction').addDirty(model);
    },

    modelBecameClean: function (model) {
      this.get('transaction').removeClean(model);
    },

    rollback: function () {
      this.get('transaction').rollback();
    },

    commit: function () {
      this.commitTransaction();
    },

    willTransition: function (transition) {
      if (this.get('transaction.dirty').length) {
        transition.abort();
        return false;
      }
      return true;
    }
  }
});

var TransactionAwareRoute = Ember.Route.extend(TransactionMixin, { ... });

And of course the actual Transaction that manages the dirty objects:

var Transaction = Ember.Object.extend({

  deleted: [],
  created: [],
  updated: [],

  dirty: Ember.computed.union('deleted', 'created', 'updated'),

  isDirty: Ember.computed.notEmpty('dirty.[]'),

  addDirty: function (model) {
    var type = model.get('isNew') ? 'created' : model.get('isDeleted') ? 'deleted' : 'updated';

    if (type === 'deleted') {
      this.get('updated').removeObject(model);
    }
    this.get(type).addObject(model);
  },

  removeClean: function (model) {
    this.get('deleted').removeObject(model);
    this.get('created').removeObject(model);
    this.get('updated').removeObject(model);
  },

  commit: function (store) {
    return store.commitRecords(this.get('dirty'));
  },

  rollback: function () {
    return this.get('dirty').slice().reverse().invoke('rollback');
  }
});

Ember Data used to have a transaction api and IMHO it worked pretty well. I was disappointed when it was removed. I don't know if any of this helps, but along with the code in this PR it made working with Ember Data a lot more productive. Without it, I don't think that we could have continued using Ember Data in its current state. There's just too much overhead in manually managing dirty objects on data-heavy pages (again IMHO).

[mmpestorich]

@steffenbrem

Do you have an idea on how you can update the canonicalState of all relationships of a parent record that has been saved (without including every relationship in the response)? I have tried to fix this, but I lack the knowledge for it.

I am currently looking into this and have written a few tests for it, but haven't come up with a good solution yet. If I figure something out, i'll put it up here.

[bmac]

If it's a matter of more discussion, conforming to an RFC, cleaning up the code, etc... please point me in the right direction. I'll gladly help in anyway that I can. This PR has been sitting out here for over a year now with no feedback from the ember-data team... slightly frustrating. Prior to this, I had pushed up #1631 - which I closed after being told that rolling back relationships would be addressed by the single-source-of-truth branch - ulitmately it wasn't.

The bottom line is that this PR works and has a handful of passing tests and it's been over 2 years since ember-data was able to rollback relationships (when this behavoir was removed from ember-data, it was sold as a temporary thing). I'd gladly contribute more time and energy to this in order to put something together that you all would feel comfortable accepting... anything I can do please let me know.

Thanks for your comments @mmpestorich. I understand the frustration. I think I may have been wrong to close this pull request. Unfortunately, github wont let me reopen this pr. Do you mind submitting a new one and I will get it on the agenda for the next meeting?

[mmpestorich]

@bmac Sorry for the late response, I've been on vacation these past couple weeks. I'd be more than happy to submit this as a new pull request in order to gather more input from the community and your team. I should be able to get something up by tomorrow.