diff --git a/packages/ember-data/lib/system/model/internal-model.js b/packages/ember-data/lib/system/model/internal-model.js index a304982b89b..843de1f75fa 100644 --- a/packages/ember-data/lib/system/model/internal-model.js +++ b/packages/ember-data/lib/system/model/internal-model.js @@ -190,7 +190,8 @@ InternalModel.prototype = { }, setupData: function(data) { - var changedKeys = mergeAndReturnChangedKeys(this._data, data); + var changedKeys = this._changedKeys(data); + merge(this._data, data); this.pushedData(); if (this.record) { this.record._notifyProperties(changedKeys); @@ -584,13 +585,12 @@ InternalModel.prototype = { @method adapterDidCommit */ adapterDidCommit: function(data) { - var changedKeys; this.didCleanError(); + var changedKeys = this._changedKeys(data); + merge(this._data, this._inFlightAttributes); if (data) { - changedKeys = mergeAndReturnChangedKeys(this._data, data); - } else { - merge(this._data, this._inFlightAttributes); + merge(this._data, data); } this._inFlightAttributes = Ember.create(null); @@ -663,6 +663,80 @@ InternalModel.prototype = { this._inFlightAttributes = Ember.create(null); }, + /** + @method _changedKeys + + Ember Data has 3 buckets for storing the value of an attribute on an internalModel. + + `_data` holds all of the attributes that have been acknowledged by + a backend via the adapter. When rollback is called on a model all + attributes will revert to the record's state in `_data`. + + `_attributes` holds any change the user has made to an attribute + that has not been acknowledged by the adapter. Any values in + `_attributes` are have priority over values in `_data`. + + `_inFlightAttributes`. When a record is being synced with the + backend the values in `_attributes` are copied to + `_inFlightAttributes`. This way if the backend acknowledges the + save but does not return the new state Ember Data can copy the + values from `_inFlightAttributes` to `_data`. Without having to + worry about changes made to `_attributes` while the save was + happenign. + + + Changed keys builds a list of all of the values that may have been + changed by the backend after a successful save. + + It does this by iterating over each key, value pair in the payload + returned from the server after a save. If the `key` is found in + `_attributes` then the user has a local changed to the attribute + that has not been synced with the server and the key is not + included in the list of changed keys. + + + + If the value, for a key differs from the value in what Ember Data + believes to be the truth about the backend state (A merger of the + `_data` and `_inFlightAttributes` objects where + `_inFlightAttributes` has priority) then that means the backend + has updated the value and the key is added to the list of changed + keys. + + @private + */ + _changedKeys: function(updates) { + var changedKeys = []; + + if (updates) { + var original, i, value, key; + var keys = Ember.keys(updates); + var length = keys.length; + + original = merge(Ember.create(null), this._data); + original = merge(original, this._inFlightAttributes); + + for (i = 0; i < length; i++) { + key = keys[i]; + value = updates[key]; + + // A value in _attributes means the user has a local change to + // this attributes. We never override this value when merging + // updates from the backend so we should not sent a change + // notification if the server value differs from the original. + if (this._attributes[key] !== undefined) { + continue; + } + + if (!Ember.isEqual(original[key], value)) { + changedKeys.push(key); + } + } + } + + return changedKeys; + }, + toString: function() { if (this.record) { return this.record.toString(); @@ -672,31 +746,5 @@ InternalModel.prototype = { } }; -// Like Ember.merge, but instead returns a list of keys -// for values that fail a strict equality check -// instead of the original object. -function mergeAndReturnChangedKeys(original, updates) { - var changedKeys = []; - - if (!updates || typeof updates !== 'object') { - return changedKeys; - } - - var keys = Ember.keys(updates); - var length = keys.length; - var i, val, key; - - for (i = 0; i < length; i++) { - key = keys[i]; - val = updates[key]; - - if (original[key] !== val) { - changedKeys.push(key); - } - - original[key] = val; - } - return changedKeys; -} export default InternalModel; diff --git a/packages/ember-data/tests/integration/records/property-changes-test.js b/packages/ember-data/tests/integration/records/property-changes-test.js new file mode 100644 index 00000000000..228f1ae3fdf --- /dev/null +++ b/packages/ember-data/tests/integration/records/property-changes-test.js @@ -0,0 +1,146 @@ +var env, store, Person; +var attr = DS.attr; +var run = Ember.run; + +module('integration/records/property-changes - Property changes', { + setup: function() { + Person = DS.Model.extend({ + firstName: attr('string'), + lastName: attr('string') + }); + Person.toString = function() { return 'Person'; }; + + env = setupStore({ + person: Person + }); + store = env.store; + }, + + teardown: function() { + Ember.run(function() { + env.container.destroy(); + }); + } +}); + +test('Calling push with partial records trigger observers for just those attributes that changed', function() { + expect(1); + var person; + + run(function() { + person = store.push('person', { + id: 'wat', + firstName: 'Yehuda', + lastName: 'Katz' + }); + }); + + person.addObserver('firstName', function() { + ok(false, 'firstName observer should not be triggered'); + }); + + person.addObserver('lastName', function() { + ok(true, 'lastName observer should be triggered'); + }); + + run(function() { + store.push('person', { + id: 'wat', + firstName: 'Yehuda', + lastName: 'Katz!' + }); + }); +}); + +test('Calling push does not trigger observers for locally changed attributes with the same value', function() { + expect(0); + var person; + + run(function() { + person = store.push('person', { + id: 'wat', + firstName: 'Yehuda', + lastName: 'Katz' + }); + + person.set('lastName', 'Katz!'); + }); + + person.addObserver('firstName', function() { + ok(false, 'firstName observer should not be triggered'); + }); + + person.addObserver('lastName', function() { + ok(false, 'lastName observer should not be triggered'); + }); + + run(function() { + store.push('person', { + id: 'wat', + firstName: 'Yehuda', + lastName: 'Katz!' + }); + }); +}); + +test('Saving a record trigger observers for locally changed attributes with the same canonical value', function() { + expect(1); + var person; + + env.adapter.updateRecord = function(store, type, snapshot) { + return Ember.RSVP.resolve({ id: 'wat', lastName: 'Katz' }); + }; + + run(function() { + person = store.push('person', { + id: 'wat', + firstName: 'Yehuda', + lastName: 'Katz' + }); + + person.set('lastName', 'Katz!'); + }); + + person.addObserver('firstName', function() { + ok(false, 'firstName observer should not be triggered'); + }); + + person.addObserver('lastName', function() { + ok(true, 'lastName observer should be triggered'); + }); + + run(function() { + person.save(); + }); +}); + +test('store.push should not override a modified attribute', function() { + expect(1); + var person; + + run(function() { + person = store.push('person', { + id: 'wat', + firstName: 'Yehuda', + lastName: 'Katz' + }); + + person.set('lastName', 'Katz!'); + }); + + person.addObserver('firstName', function() { + ok(true, 'firstName observer should be triggered'); + }); + + person.addObserver('lastName', function() { + ok(false, 'lastName observer should not be triggered'); + }); + + run(function() { + person = store.push('person', { + id: 'wat', + firstName: 'Tom', + lastName: 'Dale' + }); + }); +}); diff --git a/packages/ember-data/tests/unit/store/push-test.js b/packages/ember-data/tests/unit/store/push-test.js index fc2f436c384..f1a30aa2469 100644 --- a/packages/ember-data/tests/unit/store/push-test.js +++ b/packages/ember-data/tests/unit/store/push-test.js @@ -164,35 +164,6 @@ test("Calling push on normalize allows partial updates with raw JSON", function equal(person.get('lastName'), "Jackson", "existing fields are untouched"); }); -test("Calling push with partial records triggers observers for just those attributes that changed", function() { - expect(1); - var person; - - run(function() { - person = store.push('person', { - id: 'wat', - firstName: "Yehuda", - lastName: "Katz" - }); - }); - - person.addObserver('firstName', function() { - ok(false, 'firstName observer should not be triggered'); - }); - - person.addObserver('lastName', function() { - ok(true, 'lastName observer should be triggered'); - }); - - run(function() { - store.push('person', { - id: 'wat', - firstName: 'Yehuda', - lastName: 'Katz!' - }); - }); -}); - test("Calling push with a normalized hash containing related records returns a record", function() { var number1, number2, person; run(function() {