Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

array attribute #458

Closed
wants to merge 6 commits into from

4 participants

@tborg

Issue #53

I hacked in a special case for array attributes that addresses the isDirty problem by creating an array proxy, observing it, and calling setProperty with a copy when it changes. It seems to work, although it is not the cleanest thing in the whole world.

The array proxy is used automatically for attributes with '[]' in their name. You still have to define a custom transform.

Please review. This is an important issue.

@tborg

Wait! New failing tests. I will re-commit when this is ready.

@tborg tborg closed this
@tborg

The original approach I took tried to use a closure, with the unintended result that changes bled from one model to the next. This commit fixes that problem, by simply checking if the value is an array in the getter. If it is an array, and an observer has not already been added to the array's [] property, then one is added. Observers on [] fire whenever the array is changed through observer-aware functions such as pushObject (as you would expect). The observer triggers a call to setProperty with a copy of the array.

The original point about using '[]' in the attribute name is obviated by this solution. Any value that is an array will trigger the appropriate observer.

The observer has to call setProperty with a copy of the array in order to fool the dirty checker into recognizing the saved value as a different one. Otherwise the value to set would be identical with the array that's already in data.attributes, because a reference to that is what's returned by the getter.

Is there a problem with just setting 'isDirty' to true directly in the observer? It seems to me that indirection through setProperty is safer and more future-proof, but it also involves some sacrifice in efficiency.

This fix will prevent the subtle bugs Yehuda has mentioned previously as an argument against adding a default array transformation. With the caveat that the end user must be aware of the need to use observer-aware methods.

A similar fix would probably work for nested objects, but the argument against using relationships in that case is also weaker, IMO.

I am aware that this problem is already under consideration elsewhere. I hope that this fix at least provides some food for thought.

@tborg tborg reopened this
@tborg

Sorry about the sloppy commit history. I got a little excited.

@yipdw

I'd like to report a small success.

amvorg-underground/catalog is using @tborg's patch for some models that have array attributes; see e.g. https://github.com/amvorg-underground/catalog/blob/master/app/assets/javascripts/models/video.coffee. (They're arrays because it's the easiest way to record lists; entries in arrays are used at a later time to build "real" associations.)

List assignment, mutation, and serialization to JSON seem to work fine.

One nitpick with the current patch: the defaultValue option for array attributes has a gotcha. One must be careful when writing

Foo = DS.Model.extend({
  bars: DS.attr("string", {defaultValue: []})
})

because that [] will be reused across Foo instances. This isn't a huge problem; one can work around it by overriding init or createRecord.

@wagenet
Owner

Hearing that things were "hacked in" concerns me. I'll leave this open for now since it may be useful to people until we get a more solid solution.

@tborg

@wagenet,

Point taken about my less-than-reassuring diction in the original PR.

That said, I don't think that there's anything particularly hacky about the final version of the fix, if you want to talk about actual code, outside of the fact that I found it necessary to modify the DS.attr constructor directly. It is effective, encapsulated and concise.

Is there anything that you find particularly not-solid about my implementation? Do you have some ideas about what a more solid version would look like? We could work together to shore up whatever shortcomings that you see here. Allowing defaultValue to take a function that returns the default value at run time would go a long way toward resolving the issue mentioned by @yipdw.

A more "solid" solution, from my perspective, would entail redefining models as ArrayControllers over Attribute objects. I would argue this from the simple fact that ComputedProperties aren't extensible, and therefore far less flexible (particularly from an end-user perspective). Given that this isn't the case, and isn't likely to be, end-users dissatisfied with the current functionality must "hack in" a fix to the library source.

@wagenet
Owner

@tborg I haven't yet had a chance to review the code in detail yet. However, the amount of code added to DS.attr is a bit worrisome.

@wagenet
Owner

@tborg This doesn't apply given the numerous changes to current master. If this is something you still want to pursue, please submit as a new PR when it's ready.

@wagenet wagenet closed this
@ZenCocoon

@tborg @wagenet Is this approach still considered to solve this issue ( and #53 )? Is it only a matter of refactoring or an other path is to be used?

@wagenet
Owner

@ZenCocoon, @tborg Before pursuing this any further, I'd recommend trying to talk with @tomdale or @igorT or one of the other regular Data contributors. Unfortunately, I'm not in the trenches enough in this area to give the best advice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 2, 2012
  1. basic array attribute

    borg authored
Commits on Nov 3, 2012
  1. cleanliness, godliness

    borg authored
  2. edit for Ember.get usage consistency

    borg authored
  3. cosmetic edits

    borg authored
This page is out of date. Refresh to see the latest.
View
51 packages/ember-data/lib/system/model/attributes.js
@@ -1,4 +1,5 @@
var get = Ember.get;
+var set = Ember.set;
require("ember-data/system/model/model");
@@ -48,13 +49,61 @@ DS.attr = function(type, options) {
};
return Ember.computed(function(key, value) {
- var data;
+ function observeArray(arr, path, target) {
+ arr.addObserver('[]', this, function() {
+ target.setProperty(path, Ember.copy(arr));
+ });
+ }
+
+ function observeObject(proxy, path, child) {
+ function triggerChanges() {
+ this.triggerChanges();
+ }
+
+ if (child === undefined) child = get(proxy, 'content');
+
+ for (var key in child) {
+ var pathToVal = path.fmt(key);
+ var val = get(proxy, pathToVal);
+ if (typeof val === 'object') {
+ if (Ember.isArray(val)) {
+ observeArray(val, pathToVal, proxy.get('record'));
+ } else {
+ var childPath = pathToVal + '.%@';
+ observeObject(proxy, childPath, val);
+ }
+ } else {
+ proxy.addObserver(pathToVal, proxy, triggerChanges);
+ }
+ }
+ }
if (arguments.length === 2) {
Ember.assert("You may not set `id` as an attribute on your model. Please remove any lines that look like: `id: DS.attr('<type>')` from " + this.toString(), key !== 'id');
this.setProperty(key, value);
} else {
value = getAttr(this, options, key);
+ if (Ember.isArray(value)) {
+ observeArray(value, key, this);
+ } else if (typeof value === 'object') {
+ var proxyKey = '%@Proxy'.fmt(key);
+ if (get(this, proxyKey) === undefined) {
+ var record = this;
+ var proxy = Ember.ObjectProxy.create({
+ content: Ember.Object.create(value),
+ record: record,
+ triggerChanges: function() {
+ this.get('record').setProperty(key, this.get('content'));
+ }
+ });
+
+ observeObject(proxy, 'content.%@');
+
+ set(this, proxyKey, proxy);
+ }
+
+ value = get(this, proxyKey);
+ }
}
return value;
View
141 packages/ember-data/tests/unit/array_attribute_test.js
@@ -0,0 +1,141 @@
+var get = Ember.get, set = Ember.set;
+var Person;
+
+var array, store;
+
+var testSerializer = DS.Serializer.create({
+ primaryKey: function() {
+ return 'id';
+ }
+});
+
+var TestAdapter = DS.Adapter.extend({
+ serializer: testSerializer
+});
+
+module("DS.arrayAttr", {
+ setup: function() {
+ array = [
+ { id: '1', name: "Scumbag Dale", problems: ['booze'] },
+ { id: '2', name: "Scumbag Katz", problems: ['skag'] },
+ { id: '3', name: "Scumbag Bryn",}
+ ];
+
+ store = DS.Store.create({
+ adapter: TestAdapter.create()
+ });
+
+ store.adapter.registerTransform('string[]', {
+ fromJSON: function(serialized) {
+ return serialized;
+ },
+
+ toJSON: function(deserialized) {
+ return JSON.stringify(deserialized);
+ }
+ });
+
+ Person = DS.Model.extend({
+ name: DS.attr('string'),
+ problems: DS.attr('string[]', {defaultValue: []})
+ });
+
+ store.loadMany(Person, [1,2,3], array);
+ },
+
+ teardown: function() {
+ Person = null;
+ set(DS, 'defaultStore', null);
+ array = null;
+ }
+});
+
+test('the record should become dirty when array properties change', function() {
+ var dale = store.find(Person, 1);
+ equal(get(dale, 'problems')[0], 'booze', 'the array has some values');
+
+ equal(get(dale, 'isDirty'), false, 'the array is not dirty yet');
+
+ get(dale, 'problems').pushObject('cash flow');
+ equal(get(dale, 'problems.length'), 2, 'less money mo problems');
+ equal(get(dale, 'isDirty'), true, 'the model should be dirty now.');
+
+ var bryn = store.find(Person, 3);
+ equal(get(bryn, 'isDirty'), false, 'Bryn should still be clean');
+ get(bryn, 'problems').pushObject('laydeez');
+ equal(get(bryn, 'problems')[0], 'laydeez', 'Default array should be present');
+ ok(get(bryn, 'isDirty'), 'Pushing an object to the default should make it dirty');
+});
+
+module('DS.objectAttr', {
+ setup: function() {
+ array = [
+ { id: '1', name: "Scumbag Dale", bio: {
+ age: 80
+ }
+ },
+ { id: '2', name: "Scumbag Katz", bio: {
+ age: 75,
+ nested: {
+ tested: false
+ }
+ }
+ },
+ { id: '3', name: "Scumbag Bryn", bio: {
+ age: 99,
+ nested: {
+ array: ['one']
+ }
+ }}
+ ];
+
+ store = DS.Store.create({
+ adapter: TestAdapter.create()
+ });
+
+ store.adapter.registerTransform('object', {
+ fromJSON: function(serialized) {
+ return serialized;
+ },
+
+ toJSON: function(deserialized) {
+ return JSON.stringify(deserialized);
+ }
+ });
+
+ Person = DS.Model.extend({
+ name: DS.attr('string'),
+ bio: DS.attr('object', {defaultValue: {}})
+ });
+
+ store.loadMany(Person, [1,2,3], array);
+ },
+
+ teardown: function() {
+ Person = null;
+ set(DS, 'defaultStore', null);
+ array = null;
+ }
+});
+
+test('Changes to nested objects cause the model to become dirty', function() {
+
+ var dale = store.find(Person, 1);
+
+ set(dale, 'bio.age', 17);
+ equal(get(dale, 'bio.age'), 17, 'you can set a nested object value');
+ ok(dale.get('isDirty'), 'modifying a nested object makes dale dirty');
+
+ var katz = store.find(Person, 2);
+
+ set(katz, 'bio.nested.tested', true);
+ ok(get(katz, 'bio.nested.tested'), 'the deeper level has been set');
+ ok(get(katz, 'isDirty'), 'dirty at deeper levels');
+
+ var bryn = store.find(Person, 3);
+
+ get(bryn, 'bio.nested.array').pushObject('two');
+ ok(get(bryn, 'isDirty'), 'modifying an array in a nested object throws dirt');
+ equal(get(bryn, 'bio.nested.array')[1], 'two', 'the changes appear in the array');
+
+});
View
14 packages/ember-data/tests/unit/model_test.js
@@ -18,9 +18,19 @@ module("DS.Model", {
adapter: TestAdapter.create()
});
+ store.adapter.registerTransform('[]', {
+ fromJSON: function(serialized) {
+ return serialized;
+ },
+
+ toJSON: function(deserialized) {
+ return JSON.stringify(deserialized);
+ }
+ });
Person = DS.Model.extend({
name: DS.attr('string'),
- isDrugAddict: DS.attr('boolean')
+ isDrugAddict: DS.attr('boolean'),
+ convictions: DS.attr('[]')
});
},
@@ -105,6 +115,8 @@ test("a DS.Model can update its attributes", function() {
set(person, 'name', "Brohuda Katz");
equal(get(person, 'name'), "Brohuda Katz", "setting took hold");
+ set(person, 'convictions', ['petty theft']);
+ equal('petty theft', get(person, 'convictions')[0], 'array attr is set');
});
test("a DS.Model can have a defaultValue", function() {
Something went wrong with that request. Please try again.