Unchanged hasMany causing isDirty

[BryanHunt]

I'm working on some code to autosave my model. It looks like:

export default Route.extend({
  autosave: task(function * () {
    yield timeout(3000);
    this.get('model').save();
  }).restartable(),

  model(params) {
    return this.store.findRecord('task', params.taskId);
  },

  afterModel(model) {
    this.set('model', model);
  },

  modelChanged: observer('model.isDirty', function() {
    if(this.get('model.isDirty')) {
      window.console.log(this.get('model').changed());
      this.get('autosave').perform();
    }
  })
});

I have the model configured with: changeTracker: { trackHasMany: true, auto: true, enableIsDirty: true }

When I change a string "deliverables" attribute in the model, I get on the console:

{deliverables: Array(2), subscribers: true}
{subscribers: true}
{}

Why does it think subscribers changed? subscribers is modeled as: subscribers: hasMany('user', {inverse: null})

[BryanHunt]

Just discovered that if I make a second change to the "deliverables" field, I don't get a change in the "subscribers"

{deliverables: Array(2), subscribers: true}
{subscribers: true}
{}
{deliverables: Array(2)}

[danielspaniel]

I can't tell you why this is happening because I don't know what you are doing. I would need to see the whole picture of what is going on, from start to finish.

[DavidPhilip]

I also experience a similar issue. Assume a model person with a hasMany relation to roles. When adding one role to the model person.changed() yields { roles: true } but hasDirtyRelations equals false. When adding two roles hasDirtyRelations equals true?!

[danielspaniel]

oooo .. this sounds like a bug.
can you do me a huge favour and make a test to show this ( broken behaviour ) and slap it in a PR so I can fix it / see the breaking thing / check it out that way?

[BryanHunt]

I have an example project that demonstrates this issue.

git clone https://github.com/BryanHunt/auto-save-test.git
cd auto-save-test
npm install
ember server

Open your browser http://localhost:4200 Click on one of the two task links that are displayed.

The first problem you will notice is that isDirty is true when the model hasn't been modified.

To replicate the problem described in this issue:

1. Click Save to clear isDirty
2. Click Toggle Autosave to enable autosave
3. Click Add Comment and notice that isDirty is false
4. Click Add Comment and notice that isDirty is true

[danielspaniel]

Thanks Bryan for writeup and this repo with bug example. It would be nice to have a failing test in change tracker repo, and hopefully I can make one on my own with your setup in this repo you made.

[danielspaniel]

what you are doing is:

loading all tasks ( in application route ) the tests are loaded with no comments then you load the task again asking for comments ( in the task route )

the task already exists so ember data just takes the comments and slaps them on the existing task, meaning now the comments are added and the relationship is dirty, since you just added comments to a task.

this would be no big deal IF ember data called the didUpdate hook for this kind of update ( new payload info ). But it does NOT and I think it is a bug.

Change tracker will call saveChanges for you when/if the didUpdate hook is called. ( Problem solved )

So, ember-data does NOT fire the didUpdate hook when it updates a model with pushPayload ( which is happening when you loading task again with comments included ) since all it is really doing is pushing more data to one of the existing tasks that you loaded from the application route.

to solve your problem you simply have to do this:

   // routes/task.js

  // make a custom setupController method. once the task model is ready and loaded.
  // it would be nice if ember-data fired the didUpdate method, but since it does not, 
  // you have to saveChanges() on the model manually and here is the perfect place to do it. 
   setupController(controller, model) {
    model.saveChanges();
    controller.setProperties({model});
  }

make sense?

[mk-conn]

@danielspaniel Thx for clarifying this! But one question remains to me: This works for a 'standard' model - how would this work for an RSVP.hash? E.g.

model(params) {
    return RSVP.hash({
      modelA: this.store.findRecord('model-a', params.id, {include: 'model-bs.model-c'}),
      modelG: this.store.query('model-c', {sort: 'whatever'}
    });
  },

modelA is the one with the dirty relationship. I did try

setupController(controller, model) {
    model.modelA.saveChanges();
    controller.setProperties({model});
  },

but this did not bring the expected result...

[danielspaniel]

You have to be more specific about what result your talking about .. I can guess, but I have to be super sure. If you can put this example in your repo and update it and tell me what you expect ( exactly ) then I can tell you what I think can be done

[richardfrosztega]

I'm using ember-data-change-tracker inside a custom adapter linking ember data with Spring data REST. It's solved my issue of observing changes to relationships and automatically making the correct API calls on model.save().

However, I've seen this issue in my code. When an async belongsTo or hasMany is loaded and auto tracking is true, that relationship is considered to be 'changed', i.e. model.modelChanges() returns { myBelongsTo: true }. This appears to be the same problem described in this Issue.

I've written some code to reset the relationship tracking on load

Here is a unit test snippet that illustrates the problem

import Application from '@ember/application';

import { initialize } from 'dummy/initializers/relationship-tracking';
import { module, test } from 'qunit';
import { setupTest } from 'ember-qunit';
import { run } from '@ember/runloop';
import setupMirage from 'ember-cli-mirage/test-support/setup-mirage';

module('Unit | Initializer | relationship-tracking', function(hooks) {
  setupTest(hooks);
  setupMirage(hooks);

  hooks.beforeEach(function() {
    this.TestApplication = Application.extend();
    this.TestApplication.initializer({
      name: 'initializer under test',
      initialize
    });

    this.application = this.TestApplication.create({ autoboot: false });
  });

  hooks.afterEach(function() {
    run(this.application, 'destroy');
  });

  test('loading a belongsTo relationship should not mark this relationship as \'changed\'', async function (assert) {
    await this.application.boot();

    let office = server.create('office', {
      name: 'Leeds'
    });
    let person = server.create('person', {
      name: 'Fred',
      office
    });
    const store = this.owner.lookup('service:store');

    let model = await store.findRecord('person', person.id);

    assert.deepEqual(model.modelChanges(), {}, 'model should have no changes');

    model.set('name', 'Bob');
    await model.get('office');

    assert.deepEqual(model.modelChanges(), {
      'name': [
        "Fred",
        "Bob"
      ]
    }, 'model should not think belongsTo relationship has changed after loading relationship');
  });

  test('loading a hasMany relationship should not mark this relationship as \'changed\'', async function (assert) {
    await this.application.boot();

    let desk = server.create('desk', {
      colour: 'blue'
    });
    let office = server.create('office', {
      name: 'Leeds',
      desks: [desk]
    });
    const store = this.owner.lookup('service:store');

    let model = await store.findRecord('office', office.id);

    assert.deepEqual(model.modelChanges(), {}, 'model should have no changes');

    model.set('name', 'London');
    await model.get('desks');

    assert.deepEqual(model.modelChanges(), {
      'name': [
        "Leeds",
        "London"
      ]
    }, 'model should not think hasMany relationship has changed after loading relationship');
  });
});

Here is an initializer that reopens the Store and makes the fix

import Store from 'ember-data/store';
import Tracker from 'ember-data-change-tracker/tracker';

export function initialize(/* application */) {
  Store.reopen({

    findBelongsTo(internalModel, link, relationship /*, options */) {
      return this._super(...arguments)
        .then(data => {
          this._resetChangeTrackingOfRelationship(internalModel.getRecord(), relationship.name);
          return data;
        });
    },

    findHasMany(internalModel, link, relationship /*, options */) {
      return this._super(...arguments)
        .then(data => {
          this._resetChangeTrackingOfRelationship(internalModel.getRecord(), relationship.name);
          return data;
        });
    },

    _resetChangeTrackingOfRelationship(model, relationshipName) {
      Tracker.saveKey(model, relationshipName);
    }
  });
}

export default {
  initialize
};

It seems to work, but I'm not entirely sure I'm happy making modifications to behaviour in 2 libraries simultaneously (i.e. both ember-data and ember-data-change-tracker). It would be great if this feature was offered by default inside the addon.

[danielspaniel]

wow .. this is blast from the past. but i would suggest you make a PR and write a test or make the fix in the change tracker repo, that way I can check it out and see if it makes sense. I still don't follow the test and for sure you can't do a fix where you open the ember-data store. that not going to make anyone happy. but if you can do a better fix or explain the issue in a test ( in change tracker ) then I happy to check it out

[richardfrosztega]

I've done some digging and found the route cause. Details and tests in #66

The behaviour of ember-data-change-tracker is closely linked to the chosen serialization format between the UI and the server. When resource linkage for associations is present in the response the association is not dirty on load, but when resource linkage is missing then the association becomes dirty on load