Skip to content

Commit

Permalink
Fixes #836, Fixes #708 -- going back to previous stance: two models w…
Browse files Browse the repository at this point in the history
…ith the same id can't be added to the same collection.
  • Loading branch information
jashkenas committed Jan 11, 2012
1 parent cb7090d commit 8cfb243
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 3 deletions.
6 changes: 4 additions & 2 deletions backbone.js
Expand Up @@ -422,10 +422,12 @@
models = slice.call(models);
for (i = 0, l = models.length; i < l; i++) {
var model = models[i] = this._prepareModel(models[i], options);
if (this._byCid[model.cid]) {
var hasId = model.id != null;
if (this._byCid[model.cid] || (hasId && this._byId[model.id])) {
throw new Error("Can't add the same model to a set twice");
}
this._byId[model.id] = this._byCid[model.cid] = model;
this._byCid[model.cid] = model;
if (hasId) this._byId[model.id] = model;
model.bind('all', this._onModelEvent, this);
this.length++;
}
Expand Down
13 changes: 12 additions & 1 deletion test/collection.js
Expand Up @@ -136,7 +136,7 @@ $(document).ready(function() {
}
});

test("Collection: add model to collection twice", function() {
test("Collection: can't add model to collection twice", function() {
try {
// no id, same cid
var a2 = new Backbone.Model({label: a.label});
Expand All @@ -148,6 +148,17 @@ $(document).ready(function() {
}
});

test("Collection: can't add different model with same id to collection twice", function() {
var col = new Backbone.Collection;
try {
col.add({id: 101});
col.add({id: 101});
ok(false, "duplicate; expected add to fail");
} catch (e) {
equals(e.message, "Can't add the same model to a set twice");
}
});

test("Collection: add model to multiple collections", function() {
var counter = 0;
var e = new Backbone.Model({id: 10, label : 'e'});
Expand Down

3 comments on commit 8cfb243

@braddunbar
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand not adding the model twice but why throw an error here? Couldn't the duplicate model just be skipped? Has there been discussion about this before elsewhere?

@jashkenas
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be, but this usually indicates an error in your application logic, as the recently-closed tickets that raised this issue demonstrated. I think it's best to scream bloody murder here.

@braddunbar
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Ignoring a separate model instance with the same id would be inconsistent to say the least. Thanks for the clarification!

Please sign in to comment.