Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

can.Model with attributes that are models gets corrupted when you call attr() #141

Closed
wants to merge 8 commits into from

5 participants

@cohuman

Hey guys,

The test illustrates pretty accurately the problem we're having in our app. The behavior that both the task.owner and project.creator are pointing at the same object in the model store is a good thing because we want to have changes on the model update the view everywhere.

However, when you call attr(), it previously didn't do any check to see if the model objects were the same which meant the next call to attr() would clobber the model object and change its id and other data essentially corrupting the model store. The last three assertions in the test show this where you'd end up with User.store[19].id returning 27!

Our fix basically says, if the objects have the same id call attr() which updates it; otherwise, the next elseif leads you down the _set path which correctly handles this case.

We weren't quite sure where to put the test as it seems to be depend on both can.Model and the attributes plugin being present. Feel free to move it to where you see fit.

Let us know if you have any questions about the use case.

Thanks,
Michael Kebbekus & Daniel Salet

@justinbmeyer
Owner

I think we will have to find another way to make this work. Observe shouldn't really know much about ids.

If I'm reading this right (haven't pulled to test it yet) it seems the problem is you want to call:

task.attr({ owner : { id : 29, name : 'Amy'}});

And have that set a new Owner on the task. Even worse:

task.attr({ owner : new User({id: 29, name: 'Amy'})});

won't work.

My thought is that if there's a convert function specified by the attributes property, we should always run through the convert function. Then we should check if the newVal is not an Observe in:

canMakeObserve(curVal) && canMakeObserve(newVal) && curVal.attr

Let me know if that makes sense.

@cohuman

Yeah, that makes sense to me.

I was originally thinking the change would need to go in can.Model as it's really the model object that has an id; but then I saw that the constructor's id property is actually set in can.Observe so figured it would be fine to attack the problem there.

As long as you can make our test pass, we're happy!

Thanks,
Michael

observe/attributes/attributes_test.js
@@ -167,6 +167,61 @@ test("defaults", function(){
equals(link.attr('rupees'), 255);
});
+test("nested model attr", function(){
+ can.Model('NestedAttrTest.User', {
+ model : function( data ) {
+ debugger;
@cohuman
cohuman added a note

oops. didn't mean to leave debugger in there :). In fact the whole model method definition can go away for this one. We were just making sure it was getting called.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@imjoshdean imjoshdean closed this in f9896d9
@makebbekus

Awesome! Thanks, Josh.

@iamnoah

The fix makes it impossible to do updates on nested objects that have a converter to an Observe without clobbering the reference. You'll always get a new observe, which breaks all kinds of references in our app. e.g, we do this:

var MyObserve = can.Observe({
   attributes: {
      nested: "MyModel.model"
   }
});

var myObseve = new MyObserve({
   nested: {
     id: 123,
     name: "foo",
     count: 1
   }
});

myObserve.attr({nested:{count:2}});

If nested isn't bound yet or is not a Model, then we get a new instance (breaks references) and lose any old keys (e.g., id and name). This is a reversal of the behavior we had before.

@iamnoah

The worst part is that it is inconsistent. If I don't add a converter, the nested Observe is updated. But if I do, even if it's still an Observe and not a Model, the nested Observe is replaced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
5 model/list/list.js
@@ -1,4 +1,4 @@
-steal('can/util', 'can/model/elements', function(can) {
+steal('can/util', 'can/observe/elements', function(can) {
var getArgs = function( args ) {
if ( args[0] && (can.isArray(args[0])) ) {
@@ -540,6 +540,9 @@ steal('can/util', 'can/model/elements', function(can) {
if (itemsNotInList.length > 0){
//splice everything onto end of list so as not to trigger change events for each push
+ if (this.constructor.namespace){
+ itemsNotInList = can.makeArray(this.constructor.namespace.models(itemsNotInList));
+ }
this.splice.apply(this, [this.length, 0].concat(itemsNotInList));
}
View
92 observe/attributes/attributes_test.js
@@ -1,4 +1,4 @@
-steal('can/util', 'can/observe/attributes', function(can) {
+steal('can/util', 'can/model', 'can/model/list', 'can/observe/attributes', function(can) {
module("can/observe/attributes");
@@ -167,6 +167,96 @@ test("defaults", function(){
equals(link.attr('rupees'), 255);
});
+test("nested model attr", function(){
+ can.Model('NestedAttrTest.User', {}, {});
+ can.Model('NestedAttrTest.Task', {
+ attributes : {
+ owner : "NestedAttrTest.User.model"
+ }
+ }, {});
+
+
+ can.Model('NestedAttrTest.Project',{
+ attributes : {
+ creator : "NestedAttrTest.User.model"
+ }
+ }, {});
+
+ var michael = NestedAttrTest.User.model({ id : 17, name : 'Michael'});
+ var amy = NestedAttrTest.User.model({ id : 29, name : 'Amy'});
+
+ // add bindings so the objects get stored in the User.store
+ michael.bind('foo', function(){});
+ amy.bind('foo', function(){});
+
+ var task = NestedAttrTest.Task.model({
+ id : 1,
+ name : "Do it!",
+ owner : {id : 17}
+ });
+
+ var project = NestedAttrTest.Project.model({
+ id : 1,
+ name : "Get Things Done",
+ creator : {id : 17}
+ });
+
+ task.bind('foo', function(){});
+ project.bind('foo', function(){});
+
+ equal(task.attr('owner.name'), 'Michael', 'owner hash correctly modeled');
+ equal(project.attr('creator.name'), 'Michael', 'creator hash correctly modeled');
+
+ task.attr({ owner : { id : 29, name : 'Amy'}});
+ equal(task.attr('owner.name'), 'Amy', 'Task correctly updated to Amy user model');
+ equal(task.attr('owner.id'), 29, 'Task correctly updated to Amy user model');
+
+ equal(project.attr('creator.name'), 'Michael', 'Project creator should still be Michael');
+ equal(project.attr('creator.id'), 17, 'Project creator should still be Michael');
+ equal(NestedAttrTest.User.store[17].id, 17, "The model store should still have Michael associated by his id");
+});
+
+test("attr() should respect convert functions for lists when updating", function(){
+ can.Model('ListTest.User', {}, {});
+ can.Model.List('ListTest.User.List', {}, {});
+
+ can.Model('ListTest.Task', {
+ attributes : {
+ project : "ListTest.Project.model"
+ }
+ }, {});
+
+ can.Model('ListTest.Project',{
+ attributes : {
+ members : "ListTest.User.models"
+ }
+ }, {});
+
+ var task = ListTest.Task.model({
+ id : 1,
+ name : "Do it!",
+ project : {
+ id : 789,
+ name : "Get stuff done",
+ members : []
+ }
+ });
+
+ equal(task.project.members instanceof ListTest.User.List, true, "the list is a User List");
+
+ task.attr({
+ id : 1,
+ project : {
+ id : 789,
+ members: [{ id : 72, name : "Andy"}, { id : 74, name : "Michael"}]
+ }
+ });
+
+ equal(task.project.members instanceof ListTest.User.List, true, "the list is a User List");
+ equal(task.project.members.length, 2, "The members were added");
+ equal(task.project.members[0] instanceof ListTest.User, true, "The members was converted to a model object");
+ equal(task.project.members[1] instanceof ListTest.User, true, "The user was converted to a model object");
+});
})();
View
2  observe/observe.js
@@ -724,7 +724,7 @@ steal('can/util','can/construct', function(can, Construct) {
remove && self.removeAttr(prop);
return;
}
- if ( canMakeObserve(curVal) && canMakeObserve(newVal) && curVal.attr ) {
+ if ( canMakeObserve(curVal) && canMakeObserve(newVal) && curVal.attr && (curVal[this.constructor.id] == newVal[this.constructor.id]) ) {
curVal.attr(newVal, remove)
} else if ( curVal != newVal ) {
self._set(prop, newVal)
Something went wrong with that request. Please try again.