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

[d-reinhold]

Collection#set takes an option parse that, if set to true, will call the collection's parse method on the input array, as well as the model's parse method on individual objects in the input array. 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 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 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.

[jashkenas]

Perhaps. I'm not sure if this is something that is intended to be handled by modelId these days, or by parse? Anyone?

[jridgewell]

Dupe of https://github.com/jashkenas/backbone/pull/3529. We closed that one, but I don't feel strongly one way or the other.

[jashkenas]

Oh, right, so ids need to be parsed out at the collection level then...

[d-reinhold]

I don't think we'd necessarily have to parse the model twice to fix this bug, just make sure to instantiate the model earlier if {parse: true} is passed in. I wouldn't mind working on a PR with a cleaner fix. Unless it's not worth the time; I know @jridgewell has been working on a larger refactoring of Collection#parse for V2 that might also address this.

[jridgewell]

just make sure to instantiate the model earlier if {parse: true} is passed in

The issue is we don't want to instantiate at all if it's not needed.

I know @jridgewell has been working on a larger refactoring of Collection#parse for V2 that might also address this.

I think the only other approach here would be completely removing { parse: true } from #set. When fetching, parse at the collection level, then map the result into models (passing the { parse: true }). Then pass those models into #set.

Yes, it'll mean instantiating redundant models, but it'll be moving the parsing issues entirely out of #set.

[jashkenas]

For a v2, we should rethink parsing entirely. The way it's currently implemented is quite half-assed, as a result of it never having been used in DocumentCloud originally (IMHO).

I'd put that towards the top of my list for a v2, actually.

[d-reinhold]

To summarize (let me know if I'm misunderstanding anything):

If {parse: true} is passed to Collection#set, it's technically impossible to be sure if any of the passed in raw objects are already in the collection without parsing them (since the idAttribute may not be in the root of the raw object). And since Model#parse can have side effects that mutate this, we can't call Model#parse without having a model instance. Therefore, we must instantiate and parse every returned model after e.g. fetching data from the server before attempting to intelligently merge those new models' attributes into the collection. I think that'd be the result of @jridgewell's suggestion of moving parsing concerns out of Collection#set. It would be the most correct solution to this problem, but could have significant performance implications. And also probably wouldn't happen until v2.

Alternatively for the short term, have a look at the pull request I created. It fixes the original bug for consumers of APIs with wrapped responses, and shouldn't degrade performance for anyone else (consumers of APIs where the idAttribute is in the root of the response shouldn't experience extra parsing). It passes the unit test suite, and I'd be interested in running it through a performance test suite if anyone has one.

Thanks!