Update modelId() to Allow Polymorphic Collections with Different, Unknown, Model.idAttributes

[amiller-gh]

For Issue: #3965

Changes

• modelId() now takes the idAttribute of the model in question as the second argument
• modelId() will use the passed idAttribute before falling back to Collection.prototype.model.idAttribute and then 'id'
• Retains backwards compatibility
• Update tests to show default modelId() fallbacks
• Update tests to show default polymorphic collection behavior

Rationale

Right now modelId(attrs) is only passed a plain-object representation of the model's attributes. This makes it difficult for application developers to create n-degree polymorphic Collection subclasses.

Contrived Example:

// A collection of mixed entity types that maintains sort order based 
// on their joinDate property

var Entities = Collection.extend({
  modelId(attrs){
    // What ID attribute to use? Not determined at runtime.
  },
  comparator: 'joinDate'
});


// These are only two entity types with different idAttributes, but assume 
// There may be up to (n) different entity types, each with different idAttributes, 
// determined by the database backing this example application.

var Company = Model.extend({
  idAttribute: 'companyId'
});

var Customer = Model.extend({
  idAttribute: 'customerId'
});

// Proceeding to add and remove Customers, Companies, etc, to the Entities
// Collection will mess up the `_byId` cache because the Entities collection
// is not aware of all the different possibilities for `idAttribute` at runtime and 
// a `modelId` method cannot be written to satisfy the needs of this app.

By making the modelId callback take the form modelId(attrs, model), where the second argument is the model in question, Backbone can retain backwards compatibility but enable the default implementation to be:

// Always give precedence to the provided model's idAttribute. 
// Fall back to the Collection's idAttribute, and then to the default value `id`.

Entities.modelId = function modelId(attrs, idAttribute){
  return attrs[(idAttribute || this.model.prototype.idAttribute || 'id'];
};

It still maintains backwards compatibility: For any implementation that is not already polymorphic, Collections will still function normally – models will have their idAttribute set when added to the Collection by the Collection's model constructor. Those that are already polymorphic, or have some type of non-standard idAttributes, will already have this modelId method overwritten.

[craigmichaelmartin]

Could this be shortened to just attrs[model.idAttribute || 'id'];?

[craigmichaelmartin]

Nevermind, just saw the note about maintaining backward compatibility, sorry :)

[jrdb]

Instead of passing the entire model, you could pass the idAttribute of the model.

Then you could more easily implement a get that would work with your example.

collection.get({companyId:1234}, 'companyId');
collection.get({customerId:4321}, 'customerId');

 get: function(obj, idAttribute) {
      if (obj == null) return void 0;
      var id = this.modelId(this._isModel(obj) ? obj.attributes : obj, idAttribute);
      return this._byId[obj] || this._byId[id] || this._byId[obj.cid];
    },

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

[amiller-gh]

I found a major hole in this PR because of this comment, thank you!

The _byId cache stores models in the collection by both cid and id, so by modifying modelId to return the id of polymorphic elements, we run the risk of cache conflicts. Ex: A mixed entities collection containing both companies and users where there is a company with companyId: 1 and a user with userId: 1. This could cause issues.

To fix this, the _byId cache would need to track models internally by the unique combination of idAttribute and id: _byId[model.idAttribute + this.modelId(model.attributes, model.idAttribute)] = model.

However, then Collection.get will no longer be able to find models by id at all any more.

We'd need to tweak Collection.get to do something like this:

    // Get a model from the set by cid, id, model object with id or cid
    // properties, or an attributes object that is transformed through modelId.
    // When fetching by id, use the optional idAttribute if provided, otherwise the 
    // provided model's idAttribute, or the collection's template model's idAttribute
     get: function(obj, idAttribute) {
      if (obj == null) return void 0;
      idAttribute = idAttribute || obj.idAttribute  || this.model.prototype.idAttribute;
      return this._byId[obj] ||
        this._byId[idAttribute + obj] ||
        this._byId[idAttribute + this.modelId(obj.attributes || obj, idAttribute)] ||
        obj.cid && this._byId[obj.cid];
    },

This just got a little more complicated, but what do you think? Messing with how models are stored in _byId is a major change...

[amiller-gh]

Although, that conflicting id concern is assuming polymorphic models can have conflicting ids, an assumption that Backbone already seems to ignore: http://backbonejs.org/#Collection-model

[amiller-gh]

Yeah, Backbone as it is assumes polymorphic collections won't contain models of different types with the same id: http://jsfiddle.net/53oo2bo9/

That doesn't feel intuitive to me, but as it is now, this PR will still work fine :)

I like the idea of just passing the idAttribute, will update the PR

[craigmichaelmartin]

👍

[amiller-gh]

Another benefit of this change: Allows collections using component generators to use the appropriate idAttribute of its possible models.

http://jsfiddle.net/53oo2bo9/4/

[amiller-gh]

@caseywebdev, pulling you in because of your original work on modelId: #3133

Also, wondering if it may be time to talk about this again in a separate thread: #2043 (see minor freak out on above line comment for context)