Unable to maintain isDirty state for a parent and its embedded records

[simonexmachina]

We use embedded records in our payloads API, for example we have a product and all of its variants, options, pricing etc in a single payload on the way up and down. We do this for a variety of reasons, but primarily to provide the ability to update a thing and all of it's related sub-objects in a database transaction, so that we get the benefit of our database's constraints, triggers etc.

One of the things we need to do is to manage the isDirty state for the product such that:

• When one of its sub-objects (its "children") becomes dirty the product (the "parent") also becomes dirty
• When the parent is saved, all of its children also become clean
• When the parent is deleted, all of its children also become deleted
• When we call rollback() on the parent, rollback() is called on all of its children

We've developed a HasEmbeddedChildren mixin that tries to handle this, but have ultimately run into issues with ember-data that prevent us from being able to do so.

IMHO, if ember-data is going to provide an EmbeddedRecordsMixin then it must also provide a solution for these issues, or else it's providing users with a leaky abstraction. That's what we're trying to do with this mixin, but we haven't been able to get this to work.

/**
 * Packages up some common helper stuff for embedded relations on a DS.Model
 *
 * Does _not_ implement serialization for them; that is handled
 * separately by manually making serializers that mixin EmbeddedRecordsMixin.
 *
 * Assumes the existence of an `embeddedRelations` property on the Model
 * that defines the set of relationships that are embedded.
 */

// So. The problems below can be reproduced only in certain specific cases,
// it looks like. They're captured in the test 'menu edit should
// display extant group management details' in file
// dashboard/tests/integration/menus-test.js, (uncomment the isDirty
// checks) but doing that sequence of actions manually works just as
// well.

// In short: Edit a menulist set to type 'group'. In the default data:
// 1. /org/1/menu/3/edit
// 2. click "Group Menu List"
// 3. click something to edit the menu list on the right hand side.

// Problem 1: The two .get() lines marked with the "// SIDE EFFECTS"
// comment seem to be _necessary_ for the observers to trigger at
// all. We suspect Ember caching logic is the culprit, here, but we
// really have no idea.

// Problem 2: the sequence of actions reveals a disconnect between
// model.isDirty and model.currentState.isDirty. After you edit the
// menulist, both it and its parent are isDirty and
// currentState.isDirty, as you'd expect. But then you save it, and
// both it and its parent have opposite isDirty and
// currentState.isDirty values.

export default Ember.Mixin.create({
  didLoad: function() {
    this._super();

    this.get('isDirty'); // SIDE EFFECTS

    var observer = function(ctx, key) {
      var rel = key.split('.')[0];
      if (ctx.get('isDeleted')) return;
      var childrenDirty = ctx.get(rel).any(obj => obj.get('isDirty'));
      if (childrenDirty) {
        ctx.send('becomeDirty');
        // should not be necessary - see problem 2
        ctx.notifyPropertyChange('currentState');
      }
    };

    var embedded = this.get('embeddedByType');
    embedded.hasMany.forEach(rel => {
      this.get(rel).mapBy('isDirty'); // SIDE EFFECTS

      var key = rel + '.@each.isDirty';
      // it would be nice to avoid having to removeObserver here
      this.removeObserver(key, observer);
      this.addObserver(key, observer);
    });

    embedded.belongsTo.forEach(rel => {
      var dirtyProp = rel + '.isDirty';
      this.addObserver(dirtyProp, function() {
        if (this.get('isDeleted')) return;
        if (this.get(dirtyProp)) {
          this.send('becomeDirty');
          // should not be necessary - see problem 2
          this.notifyPropertyChange('currentState');
        }
      });
    });

    var clean = function(record) {
      if (record.get('isDirty')) {
        // from http://stackoverflow.com/a/23344215, which is for beta-7
        // but seems to still work

        // changing to loaded.updated.inFlight, which has "didCommit"
        record.send('willCommit');
        // clear array of changed (dirty) model attributes
        record.set('_attributes', {});
        // changing to loaded.saved (hooks didCommit event in "inFlight" state)
        record.send('didCommit');
        record.notifyPropertyChange('currentState');
      }
    };

    this.addObserver('isDirty', function() {
      if (!this.get('isDirty')) {
        embedded.hasMany.forEach(rel => this.get(rel).map(clean));
        embedded.belongsTo.forEach(rel => clean(this.get(rel)));
      }
    });
  },

  deleteRecord: function(...args) {
    var embedded = this.get('embeddedByType');
    embedded.hasMany.forEach(rel => this.get(rel).toArray().forEach(obj => obj.deleteRecord()));
    embedded.belongsTo.forEach(rel => this.get(rel).deleteRecord());
    return this._super.apply(this, args);
  },

  embeddedByType: function() {
    var embedded = this.get('embeddedRelations');
    var relationships = Ember.get(this.constructor, 'relationshipsByName');

    // we don't support anything other than hasMany or belongsTo embedded relations yet
    var ret = embedded.reduce((acc, rel) => {
      var kind = relationships.get(rel).kind;
      acc[kind].push(rel);
      return acc;
    }, { hasMany: [], belongsTo: [] });

    return ret;
  }.property('embeddedRelations')

});

[jasolko]

Yes, but, simply because it is embedded doesn't mean all the operations cascade. If I delete an invoice, I expect it to delete its line items but not the vendor it belongs to. Even though the vendor is also embedded in the payload.

I'm upgrading an app from 0.14 to 1.0 beta 11. I was able to do this in 0.14 using an embeddedType of 'always' or 'load'.

[jasolko]

Nevermind me. I just found wycats comment regarding this being an application-level concern. Although that only satisfies your first concern of dirtying the parent.

[timrwood]

I've opened an RFC for this issue. Please weigh in.

emberjs/rfcs#21

[fivetanley]

Closing in favor of the RFC. @timrwood please comment there and bug us until we accept or reject the RFC. Sorry for the late response.