jashkenas / jashkenas/backbone

Rethink parsing (Collection#set doesn't intelligently merge unparsed models)

Open
#3,760 7 comments 0 reactions 0 assignees View on GitHub
break change
Dominant language
JavaScript
Stars
28.1k
Forks
5.3k
PR merge metrics
No merged PRs in 30d

Description

`Collection#set` takes an option `parse` that, if set to true, will [call the collection's `parse` method on the input array](https://github.com/jashkenas/backbone/blob/master/backbone.js#L812), as well as the [model's `parse` method on individual objects in the input array](https://github.com/jashkenas/backbone/blob/master/backbone.js#L837). This implies that it's ok for non-parsed data to be passed into `Collection#set`.

However, if non-parsed data is passed to `Collection#set`, [line 833](https://github.com/jashkenas/backbone/blob/master/backbone.js#L833) may fail to correctly merge the new data with existing data in the collection (in particular, if the non-parsed data is wrapped in a way that obscures the idAttribute).

Below is a failing test case that illustrates this issue:

```
test("collection should merge in duplicate raw objects with {merge: true}", 1, function() {
var Model = Backbone.Model.extend({
parse: function(data) { return data.wrapper; }
});

var Col = Backbone.Collection.extend({model: Model});
var col = new Col;

col.set([{wrapper: {id: 1, name: 'Foo'}}], {parse: true, merge: true});
var firstModel = col.first();

col.set([{wrapper: {id: 1, name: 'Bar'}}], {parse: true, merge: true});
var secondModel = col.first();

strictEqual(firstModel, secondModel);
});
```

The `{merge: true}` option indicates that the updated model in the collection should be the same object reference, but instead it is a newly instantiated model.

A potential fix might involve changing [line 833](https://github.com/jashkenas/backbone/blob/master/backbone.js#L833) from this:

```
if (existing = this.get(attrs)) {
```

to this:

```
existing = this.get(attrs);
if (!existing && !this._isModel(attrs) && options.parse) {
var tempModel = new this.model(attrs, options);
existing = this.get(tempModel.attributes);
}
if (existing) {
```

although there may be a more elegant solution. If people agree that this is a bug, I can open a pull request.

Contributor guide

Open the contributing guide

Research direction

Start in backbone.js around lines 812, 833, and 837, then run the supplied Collection#set test case with parse and merge enabled. Trace how the wrapped id is resolved and verify that repeated raw input updates the existing model reference rather than creating a second model.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
frontend, web-dev
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.