Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

cohuman commented Nov 5, 2012

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

Contributor

justinbmeyer commented Nov 5, 2012

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.

Contributor

cohuman commented Nov 5, 2012

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

@cohuman cohuman commented on an outdated diff Nov 6, 2012

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 Nov 6, 2012

Contributor

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.

@imjoshdean imjoshdean closed this in f9896d9 Nov 8, 2012

Contributor

makebbekus commented Nov 9, 2012

Awesome! Thanks, Josh.

Contributor

iamnoah commented Dec 13, 2012

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.

Contributor

iamnoah commented Dec 13, 2012

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