Collection length does not match models length, when fetching and parsing data with duplicate ids

[eschwartz]

Duplication Steps

• Create a model, which parses it's id from an arbitrary response object attribute
• Call fetch on a collection for the model
  • API returns multiple models with the same id
• Call fetch a second time
  • API returns multiple models with the same id
• Backbone throws error at 719:46 TypeError: Cannot read property 'cid' of undefined

set: function(models, options) {
      // ....

      // Remove nonexistent models if appropriate.
      if (remove) {
        for (i = 0, l = this.length; i < l; ++i) {
          if (!modelMap[(model = this.models[i]).cid]) toRemove.push(model);
          // TypeError; this.models.length !== this.length
        }
        if (toRemove.length) this.remove(toRemove, options);
      }

      // ...
}

See Spec (failing) for mismatched collection length for a failing spec, duplicating this issue.

Expected Behavior

The root issue is clearly that the API is returning duplicate objects. However, I'm wondering if there might be a better way for Backbone to handle this situation, which would make the problem easier to trace.

What is the default behavior when adding a model to a collection with the same id as an existing model in the collection? I would expect an error to be thrown.

[akre54]

Your model ids should be unique per model, and if the server is returning multiple objects with the same id, that seems like a bug on the part of the server. Not much Backbone can (or should) do about it.

[jashkenas]

But that error seems incorrect...

[eschwartz]

Right -- I understand that there's a bug on the server. I was just wondering if there is a better way for Backbone handle the error.

I've noticed that if you attempt to add 2 models with the same id to a collection, that the adding behavior silently fails (the model is not added, no error is thrown). I would expect that adding models with the same id via a fetch call would act the same way.

If it were up to me, I would prefer that adding duplicate models would throw an error, though I imagine this would be a breaking change for many users.

[jashkenas]

If it were up to me, I would prefer that adding duplicate models would throw an error, though I imagine this would be a breaking change for many users.

That's how it used to be ... and this behavior was an "enhancement" ;)

I would expect that adding models with the same id via a fetch call would act the same way.

Quite so.

[akre54]

So moral of the story is that seting duplicate models breaks when passed remove: true but not with remove: false (which is why directly calling add works while set does not ... you can also pass in remove: false to fetch to see the same results).

If I had to hazard a guess at this point, I'd say it's because of this line and the new idAttribute parsing. The server response hasn't been parsed yet to expose the models' ids and thus the existing check fails a few lines later. Maybe @braddunbar can shed a little more light on a solution to this.

The algorithm is still incorrect with duplicates on non-nested attributes too (collection.length becomes different from collection.models.length), but it doesn't throw. This is due to the order array getting priority over the toAdd array when setting collection models. Probably best to detect dups early and throw or wrap a _.uniq around this line.

[eschwartz]

I updated my spec to use the set method -- you are correct that we're only running into problems when using the remove:true option.

Is the intended behavior to silently reject the duplicate model, or should set throw an error in this case? My opinion is that throwing an error is more developer friendly -- if my server or client-side code is giving me duplicate ids, I want to know about it upfront. The alternative is creating bugs downstream which are much harder to identify and track.

If you want to go this route, I wrote a couple of specs for the expected behavior.

[caseywebdev]

09e0cb3 dropped support for this case.

I think we need a generateId(attrs) function to solve issues like this. It would also make composite keys easy.

// Default
generateId: function (attrs) {
  return attrs[this.idAttribute];
}

// For @eschwartz's test case...
generateId: function (attrs) {
  return attrs.nested.id;
}

Then instead of using attrs[model.idAttribute], we use modelgenerateId(attrs). The change would be non-breaking. Thoughts?

[akre54]

Definitely. Relying on the data to be parsed before it's passed to parse is just cruel. Where else could composite keys be called from? Would this function be used in multiple places?

[caseywebdev]

@akre54 working up a quick PR, so we can get into details there.

[caseywebdev]

That still doesn't solve the problem of duplicate models being passed in the same set call though.

[chiplay]

One note here - I may be missing it, but it seems that if your model is defined as a function on your collection, you still wont have access to the idAttribute of that model until its called - correct? How can targetProto be defined before the model function is called and returned?

[caseywebdev]

@chiplay Check out https://github.com/jashkenas/backbone/pull/3042/files for an example of setting idAttribute (optionally generateId in master) for a polymorphic Collection#model.

[chiplay]

Thanks @caseywebdev I dont think I can set the idAttribute on the Collection, since the idAttribute is different for both Model types on the polymorphic model. I'm not sure by looking at it how I can use generateId to produce the correct id - can you explain or point me the right direction? Thanks!

[chiplay]

@caseywebdev ahhh - got it - little slow today. I just need to set the generateId method on my collection and then use the attrs to return the correct Id - brilliant!

[caseywebdev]

var TypeA = Backbone.Model.extend({idAttribute: '_a'});
var TypeB = Backbone.Model.extend({idAttribute: '_b'});
var Poly = Backbone.Collection.extend({
  model: Backbone.Model.extend({
    constructor: function (attrs, options) {
      var model = attrs.type === 'a' ? TypeA : TypeB;
      return new model(attrs, options);
    },

    generateId: function (attrs) {
      return (attrs.type === 'a' ? TypeA : TypeB)
        .prototype.generateId(attrs);
    }
  })
});

Let me know if that helps. Keep in mind generateId was recently added so it's not in the latest release yet.