jashkenas / backbone

Give your JS App some Backbone with Models, Views, Collections, and Events
http://backbonejs.org
MIT License
28.09k stars 5.39k forks source link

Models are skipped or merged when idAttribute values look like cid (c12, c78, etc) #4226

Closed jSanchoDev closed 1 year ago

jSanchoDev commented 4 years ago

Hi there!

We're having a strange issue when using a collection of models with custom idAttribute. Basically, if idAttribute attribute has a value that "looks like" cid (c12, c78, etc), the model is not added to collection or merged with some previously added model.

Please, check the following simplified example:

const MyModel = Backbone.Model.extend({
    idAttribute: 'myId'
});

const MyCollection = Backbone.Collection.extend({
    model: MyModel
});

const list = [
  { myId: 'c10', name: 'John' },
  { myId: 'c9', name: 'James' },
  { myId: 'c8', name: 'William' },
  { myId: 'c7', name: 'Henry' },
  { myId: 'c6', name: 'Michael' },
  { myId: 'c5', name: 'Fred' },
  { myId: 'c4', name: 'George' },
  { myId: 'c3', name: 'Larry' },
  { myId: 'c2', name: 'Peter' },
  { myId: 'c1', name: 'Neil' }
];

const testCollection = new MyCollection(list);

alert(testCollection.length); // expected: 10, actual 5

Or check this fiddle: https://jsfiddle.net/0goy4u9w/8/

I believe this happens because Backbone stores models both by cid and idAttribute, when available, here: https://github.com/jashkenas/backbone/blob/75e6d0ce6394bd2b809823c7f7dc014ddb6ae287/backbone.js#L1178-L1183 Then, during set() it finds the previously stored (by cid) model and assumes it's found a duplicate: https://github.com/jashkenas/backbone/blob/75e6d0ce6394bd2b809823c7f7dc014ddb6ae287/backbone.js#L870-L877

Now, I understand that this looks like someone's tried really hard to find a defect 😬and created ids that look like cids, but we actually use custom id attributes that are unique, but don't follow same pattern (they come from different sources), and sometimes they actually look like cid. 😱

Is there a reason why cid is still being used as key for _byId even when models have idAttribute declared?

jaapz commented 4 years ago

This does seem like it might cause collisions if your idAttribute format clashes with the default return values of _.uniqueId (which is used to generate the value of this.cid).

You could set the cidPrefix to something else than the default (c) to make sure it doesn't clash with your idAttribute to see if that fixes your problem.

jSanchoDev commented 4 years ago

Thanks a lot, @jaapz, this indeed fixes this and it's a very simple solution too, which is great. Perhaps it's a good idea to add some sort of warning in docs related to idAttribute section? It's a kind of bug that can be hard to reproduce and find the cause of. Thanks again!