there is a way to add several models with "same" id to a collection

[taburetkin]

default normal behavior:

const coll = new Backbone.Collection();
coll.add({ id: 1 });
coll.add({ id: 1 });
coll.add({ id: 1 });
console.log(coll.lenght); //1

but, if we did that with model having id in defaults then all models will be added to a collection

const Model = Backbone.Model.extend({
  defaults: { id: 0 }
});
const coll = new Backbone.Collection([], { model: Model });
coll.add({  });
coll.add({  });
coll.add({  });
console.log(coll.length); // 3

working fiddle: https://jsfiddle.net/gkpLoaqx/

maybe id field from defaults should not be applied on model initialize?

[prma85]

I was looking at it. Logically, you shouldn't be able to add more than one item with the same id, since the ID is unique. When you are sending the id attribute, the Backbone verifies if the id already exists or not before trying to add it. If it exists, nothing is done. The problem is that if you don't send this attribute, the Backbone does not do this verification and then, create the models with the default values, creating the bug.

To fix it, need to do the verification when creating the model and not when it is doing the add function.

[taburetkin]

i suppose, that attribute defined in idAattribute for a model can not be part of defaults because of the id nature .
and i think that defaults should be transformed before applying to models attribute, something like this _.omit(defaults, this.idAttribute)
probably this solution is not good enough in performance terms.

i can suggest this small solution
link to model constructor:

backbone/backbone.js

Line 406 in 27f7d41

var defaults = _.result(this, 'defaults');

i think, that this part can be replaced with this few lines

    var defaults = this.getDefaults();
    attrs = _.defaults(_.extend({}, defaults, attrs), defaults);
    ...
},
getDefaults() {
    var defaults = _.result(this, 'defaults');
    delete defaults[this.idAttribute]; // or defaults = _.omit(defaults, this.idAttribute);
    return defaults;  
}

[jgonggrijp]

I've been investigating this issue, and I realized that the problem is not in Backbone, but in the hypothetical user violating the preconditions of the framework.

If you pass a raw hash of attributes to a collection, the collection must be able to establish, purely based on that hash, whether it represents a model that is already in the collection or not. It does that by passing the hash through modelId. If the idAttribute is not in the hash but in the Model.defaults, this approach does not work (unless Model.defaults is a function that is guaranteed to return a unique idAttribute on every invocation).

I could delete defaults[this.idAttribute], but if this.defaults is a plain object, that will modify the object in place. It would also be a breaking change for anyone who does not put their models in a collection and who (for whatever weird singleton pattern) actually wants all their model instances to have the same id.

I could do an _.omit (or even a _.extend({}, defaults) followed by a delete), which avoids modifying this.defaults in place but which is a disproportionally expensive safeguard against a situation that is arguably the responsibility of the user to prevent. It also has the same breaking effect for hypothetical users doing weird singleton patterns.

While the hypothetical singleton pattern thing is weird, that use case is more valid than one in which the idAttribute is defaulted to a fixed value and the user is also adding instances of the model to a collection.

I could do a hardcore refactor of Collection.set and Collection._prepareModel to prevent the aliasing models situation, but that would cause an entire cascade of breaking changes, all while the user shouldn't be doing these things in the first place.

Instead of all these things, I will simply submit a PR to document this gotcha in Model.defaults.

[prantlf]

I use a modified Backbone, which allows disabling the unique ID feature and enables adding multiple models with the same ID to the same collection:

idAattribute: null

A breaking change in Backbone 1.4 made this code change mode complicated. It introduced mandatory polymorphism - using idAttribute from single model instances. Previous versions used idAttribute from this.model.prototype and supported mandatory homogenous collections. I added polymorphic boolean to support polymorphism, otherwise remain homogenous.

modelId: function (attrs, idAttribute) {
  if (idAttribute === undefined || !this.polymorphic) {
    idAttribute = this.model.prototype.idAttribute;
  }
  return idAttribute !== null ? attrs[idAttribute || "id"] : null;
},

[jgonggrijp]

@prantlf That is interesting. But why mention it here? What was the breaking change in Backbone 1.4?

[prantlf]

Because this topic is: "there is a way to add several models with "same" id to a collection". The breaking change was the now mandatory polymorphism as I wrote above.

The code allowing not unique IDs like the code above, but for Backbone older than 1.4:

modelId: function(attrs) {
  var idAttribute = this.model.prototype.idAttribute;
  return idAttribute !== null ? attrs[idAttribute || 'id'] : null;
},

[jgonggrijp]

@prantlf I do think that your reply is about a different issue. The current issue is about modelId being broken when a non-unique idAttribute is sourced from the model's defaults. This would be problematic regardless of whether modelId prioritizes model.idAttribute or this.model.prototype.idAttribute, which seems to be what your issue revolves around.

Are you just pointing out that a breaking change was made in 1.4, or do you also mean to request another change? In the first case, I will acknowledge that it was a breaking change. The author intended it to be a backwards-compatible enhancement, but changed existing tests, which is generally a clear indication of breaking changes. See #3966.

In the second case, please check out #3966 and the other issues linked from there. You might find the comments in there illuminating, and you might find a more appropriate place to respond as well. If you still want to suggest changes to Backbone after that, I encourage you to open a new ticket or pull request.

[prantlf]

It appears that I misunderstood this issue. I thought and it still kind of looks to me that it's about using the id attribute with values that are not unique, which isn't possible with the vanilla Backbone.Model. That's why I thought that a code change would be helpful and put it here. I'm sorry for the trouble. Thanks for your patience and looking up the original pull request.

I'm not sure if disabling the ID with idAttribute:null should be included in Backbone and that's why I haven't opened a pull request for it. I need it in my codebase, which hasn't always evolved in the right direction, and I'm not so happy about it. But it works.

[jgonggrijp]

Please feel welcome to open a new ticket for your usecase. Maybe we can come up with a better solution!