can.Model.findAll loads lists and mixes it with previously loaded #245

Merged
merged 4 commits into from Jan 28, 2013

Conversation

Projects
None yet
4 participants
@daffl
Contributor

daffl commented Jan 25, 2013

http://jsfiddle.net/qYdwR/643/

First, fiddle loads and renders 5 todos with set attribute index (from 0.. 5)
Then click "Load New Todos" - with another fixture (that doesn't provide index attributes) we load new 10 todos, and render new list, so I would expect that: ALL of new todos would have index attribute undefined - but they do not, todos 1 to 4 have index attribute from old list.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jan 22, 2013

Contributor

Looks like a bug in .replace. Will mark it as a 1.1.4 fix.

Contributor

daffl commented Jan 22, 2013

Looks like a bug in .replace. Will mark it as a 1.1.4 fix.

@ghost ghost assigned daffl Jan 22, 2013

@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Jan 22, 2013

Contributor

Do you mean Observe.List.protorype.replace? Why it is used while creating new model/list from new data arrived?

Contributor

whitecolor commented Jan 22, 2013

Do you mean Observe.List.protorype.replace? Why it is used while creating new model/list from new data arrived?

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jan 22, 2013

Contributor

Your Fiddle uses it in line 38. This is where I think the problem is.

Contributor

daffl commented Jan 22, 2013

Your Fiddle uses it in line 38. This is where I think the problem is.

@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Jan 22, 2013

Contributor

No, I've updated fiddle and removed replace, I just rerender view now with new list, it seems that the problem in findAll.
http://jsfiddle.net/qYdwR/645/

Contributor

whitecolor commented Jan 22, 2013

No, I've updated fiddle and removed replace, I just rerender view now with new list, it seems that the problem in findAll.
http://jsfiddle.net/qYdwR/645/

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jan 22, 2013

Contributor

Ok I see what you mean. Problem is, that this is how the model store works. If you explicitly set the attribute to null it does what you expect it to. I understand that this isn't always the expected behaviour and there is a flag in can.Observe.prototype.attr that allows to remove non-existent attributes. Overriding can.Model.model like this:

can.Model.model = function( attributes ) {
    if ( ! attributes ) {
        return;
    }
    if ( attributes instanceof this ) {
        attributes = attributes.serialize();
    }
    var id = attributes[ this.id ],
        model = id && this.store[id] ? this.store[id].attr(attributes, true) : new this( attributes );
    if(this._reqs){
        this.store[attributes[this.id]] = model;
    }
    return model;
}

Should do the trick. Please give it a quick try for your problem. If that works I'd propose adding a static merge property to can.Model that allows to change the merging strategy.

Contributor

daffl commented Jan 22, 2013

Ok I see what you mean. Problem is, that this is how the model store works. If you explicitly set the attribute to null it does what you expect it to. I understand that this isn't always the expected behaviour and there is a flag in can.Observe.prototype.attr that allows to remove non-existent attributes. Overriding can.Model.model like this:

can.Model.model = function( attributes ) {
    if ( ! attributes ) {
        return;
    }
    if ( attributes instanceof this ) {
        attributes = attributes.serialize();
    }
    var id = attributes[ this.id ],
        model = id && this.store[id] ? this.store[id].attr(attributes, true) : new this( attributes );
    if(this._reqs){
        this.store[attributes[this.id]] = model;
    }
    return model;
}

Should do the trick. Please give it a quick try for your problem. If that works I'd propose adding a static merge property to can.Model that allows to change the merging strategy.

@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Jan 22, 2013

Contributor

Yes, thank you, it seems that it does. I just doesn't understand what is the logic behind mixing behind model store that made it to look at previously loaded instance? Can you explain?

Contributor

whitecolor commented Jan 22, 2013

Yes, thank you, it seems that it does. I just doesn't understand what is the logic behind mixing behind model store that made it to look at previously loaded instance? Can you explain?

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jan 22, 2013

Contributor

The idea is: Models are uniquely identified by ID. So if you get model data with the same id Can assumes that it is the same model and will update the store. When your model data is live-bound (or listening to Observe changes any other way) somewhere (and models instances in the store always are, otherwise they will be removed from it) all your views and other listeners will be notified if the model is re-loaded from the server and attributes are updated. You also want all lists to remove a model if it is destroyed etc.

Contributor

daffl commented Jan 22, 2013

The idea is: Models are uniquely identified by ID. So if you get model data with the same id Can assumes that it is the same model and will update the store. When your model data is live-bound (or listening to Observe changes any other way) somewhere (and models instances in the store always are, otherwise they will be removed from it) all your views and other listeners will be notified if the model is re-loaded from the server and attributes are updated. You also want all lists to remove a model if it is destroyed etc.

@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Jan 23, 2013

Contributor

The idea is: Models are uniquely identified by ID. So if you get model data with the same id Can assumes that it is the same model and will update the store.

Ok that is very good point. I just don't undertand how nested lists where updated that caused the issue?

My particular case:

I load from server a model, model contains nested list of items. Then I decide to synchronize model state with server, I load again the same model (with the same id). Between this two syncs new item was added on server to DB to the list. So I get new model with updated list that contains one new item. Items in the list say may have or have not field called 'finished', new item doesn't have it, but after loading can.Model adds this field ("finished") to new item and takes the value from first item of the list that where loaded previously.

I hope I made my self clear. Can you explain it?

Contributor

whitecolor commented Jan 23, 2013

The idea is: Models are uniquely identified by ID. So if you get model data with the same id Can assumes that it is the same model and will update the store.

Ok that is very good point. I just don't undertand how nested lists where updated that caused the issue?

My particular case:

I load from server a model, model contains nested list of items. Then I decide to synchronize model state with server, I load again the same model (with the same id). Between this two syncs new item was added on server to DB to the list. So I get new model with updated list that contains one new item. Items in the list say may have or have not field called 'finished', new item doesn't have it, but after loading can.Model adds this field ("finished") to new item and takes the value from first item of the list that where loaded previously.

I hope I made my self clear. Can you explain it?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 23, 2013

Contributor

I'm not totally clear. A demo fiddle might help a lot.

Sent from my iPhone

On Jan 22, 2013, at 11:05 PM, Alex Osh notifications@github.com wrote:

The idea is: Models are uniquely identified by ID. So if you get model data with the same id Can assumes that it is the same model and will update the store.

Ok that is very good point. I just don't undertand how nested lists where updated that caused the issue?

My particular case:

I load from server a model, model contains nested list of items. Then I decide to synchronize model state with server, I load again the same model (with the same id). Between this two syncs new item was added on server to DB to the list. So I get new model with updated list that contains one new item. Items in the list say may have or have not field called 'finished', new item doesn't have it, but after loading can.Model adds this field ("finished") to new item and takes the value from first item of the list that where loaded previously.

I hope I made my self clear.


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jan 23, 2013

I'm not totally clear. A demo fiddle might help a lot.

Sent from my iPhone

On Jan 22, 2013, at 11:05 PM, Alex Osh notifications@github.com wrote:

The idea is: Models are uniquely identified by ID. So if you get model data with the same id Can assumes that it is the same model and will update the store.

Ok that is very good point. I just don't undertand how nested lists where updated that caused the issue?

My particular case:

I load from server a model, model contains nested list of items. Then I decide to synchronize model state with server, I load again the same model (with the same id). Between this two syncs new item was added on server to DB to the list. So I get new model with updated list that contains one new item. Items in the list say may have or have not field called 'finished', new item doesn't have it, but after loading can.Model adds this field ("finished") to new item and takes the value from first item of the list that where loaded previously.

I hope I made my self clear.


Reply to this email directly or view it on GitHub.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jan 23, 2013

Contributor

This are actually two issues (part of which I think the http://jsfiddle.net/qYdwR/645/ Fiddle demonstrates):

The first one @moschel just addressed in #251: Nested arrays should never be merged. Otherwise you have weird things like this happening:

var state = new can.Observe({
  arr: ["a", "b"]
});
state.attr({
  arr: ["c"]
});
// state => {arr: ["c", "b"]}

The second one is, that can.Model.model doesn't offer an option to update the attributes of a model in the store with the removeAttr flag set to true. If your backend allows removing attributes this will cause problems like this:

var MyModel = can.Model({}),
    model = MyModel.model({
        id: 0,
        name: 'Test',
        index: 1
    });

// After e.g. reloading
model = MyModel.model({
    id: 0,
    name: 'Test'
});

model.attr('index') // -> still 1, but should be undefined

I think it would make sense to add a removeAttr flag or something to the static Model attributes in case the store attributes need to be merged with this.store[id].attr(attributes, true). The issue got addressed in #248 today but not with a can.Model property, just an additional parameter like can.Model.model(data, removeAttr).

Contributor

daffl commented Jan 23, 2013

This are actually two issues (part of which I think the http://jsfiddle.net/qYdwR/645/ Fiddle demonstrates):

The first one @moschel just addressed in #251: Nested arrays should never be merged. Otherwise you have weird things like this happening:

var state = new can.Observe({
  arr: ["a", "b"]
});
state.attr({
  arr: ["c"]
});
// state => {arr: ["c", "b"]}

The second one is, that can.Model.model doesn't offer an option to update the attributes of a model in the store with the removeAttr flag set to true. If your backend allows removing attributes this will cause problems like this:

var MyModel = can.Model({}),
    model = MyModel.model({
        id: 0,
        name: 'Test',
        index: 1
    });

// After e.g. reloading
model = MyModel.model({
    id: 0,
    name: 'Test'
});

model.attr('index') // -> still 1, but should be undefined

I think it would make sense to add a removeAttr flag or something to the static Model attributes in case the store attributes need to be merged with this.store[id].attr(attributes, true). The issue got addressed in #248 today but not with a can.Model property, just an additional parameter like can.Model.model(data, removeAttr).

@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Jan 23, 2013

Contributor

Yes, that is the point, as I've said new list was "mixed" with previously loaded, wrong verb -) it was actually merged with it.

Contributor

whitecolor commented Jan 23, 2013

Yes, that is the point, as I've said new list was "mixed" with previously loaded, wrong verb -) it was actually merged with it.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jan 25, 2013

Contributor

Ok, as a non inversive fix for 1.1.4 I attached a pull request that adds a removeAttr flag which allows you to do something like:

  var Todo = can.Model({
    findAll : 'GET /todos',
    findOne : 'GET /todos/{id}',
    create  : 'POST /todos',
    update  : 'PUT /todos/{id}',
    destroy : 'DELETE /todos/{id}',
    removeAttr: true
  }, {});

Which will cause any request and .model call on this Todo model to remove attributes that don't exist in the response instead of merging them.

Contributor

daffl commented Jan 25, 2013

Ok, as a non inversive fix for 1.1.4 I attached a pull request that adds a removeAttr flag which allows you to do something like:

  var Todo = can.Model({
    findAll : 'GET /todos',
    findOne : 'GET /todos/{id}',
    create  : 'POST /todos',
    update  : 'PUT /todos/{id}',
    destroy : 'DELETE /todos/{id}',
    removeAttr: true
  }, {});

Which will cause any request and .model call on this Todo model to remove attributes that don't exist in the response instead of merging them.

model/model.js
+ * be merged. Setting `removeAttr` to `true` will result in model attributes like
+ *
+ * { id: 1, name: 'Really do dishes' }
+ */

This comment has been minimized.

@moschel

moschel Jan 27, 2013

Contributor

Good docs, but might also want to mention the effect this has on arrays (causes them to be replaced instead of "merged").

I can help with this.

@moschel

moschel Jan 27, 2013

Contributor

Good docs, but might also want to mention the effect this has on arrays (causes them to be replaced instead of "merged").

I can help with this.

daffl added a commit that referenced this pull request Jan 28, 2013

Merge pull request #245 from bitovi/model-removeAttr-245
added removeAttr property for can.Model to prevent attribute merging

@daffl daffl merged commit c25d5af into master Jan 28, 2013

@daffl daffl deleted the model-removeAttr-245 branch Jan 28, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment