Allow Collections to determine uniqueness, not Models #3133

Merged
merged 1 commit into from Jun 27, 2014

Conversation

Projects
None yet
8 participants
Collaborator

caseywebdev commented Apr 23, 2014

Here's the approach from the other side. Tim made some good points regarding who should be determining uniqueness in #3080. With this setup, collections will never care about model.id and bookkeep purely by a customizable Collection#generateId. This means models can be in many collections and uniquely identified appropriately across all of them, all without having to alter the data in the model. After all, a model can't know how to distinguish itself from others in a collection as well as the collection can. This is a first draft, but I feel this may be the better way.

RFC @jashkenas @braddunbar @tgriesser

RE: #2976, #2985, #3080, #3132

Collaborator

tgriesser commented Apr 23, 2014

Well, I like the look of this already. I'm also kind of surprised this passes the test suite. Looking forward to diving in a bit more.

@akre54 akre54 commented on the diff Apr 23, 2014

backbone.js
@@ -698,19 +698,14 @@
// Turn bare objects into model references, and prevent invalid models
// from being added.
for (var i = 0, length = models.length; i < length; i++) {
- attrs = models[i] || {};
- if (this._isModel(attrs)) {
- id = model = attrs;
- } else {
- id = attrs[this.model.prototype.idAttribute || 'id'];
- }
+ attrs = models[i];
@akre54

akre54 Apr 23, 2014

Collaborator

Do we need a nullish guard here (for passing to get later)?

@caseywebdev

caseywebdev Apr 23, 2014

Collaborator

Collection#get disregards nullish values, so I don't think a guard is necessary.

@akre54

akre54 Apr 23, 2014

Collaborator

Oh sweet. Must've missed that change.

@caseywebdev caseywebdev commented on an outdated diff Apr 29, 2014

@@ -664,7 +664,8 @@
for (var i = 0, length = models.length; i < length; i++) {
var model = models[i] = this.get(models[i]);
if (!model) continue;
- delete this._byId[model.id];
+ var id = this.generateId(model.attributes);
+ if (id != null) delete this._byId[id];
@caseywebdev

caseywebdev Apr 29, 2014

Collaborator

Added a guard here for consistency with the deletes in Collection#_onModelEvent. If we want to remove this guard (I'm indifferent as using null or undefined as an id seems strange to me), we should do it in _onModelEvent as well.

I'm wondering if the following concept could work...

Instead of collection.generateId(attrs) something like collection.generateType(attrs).
There would still be a private collection._generateId(id,type) (responsible for generating the key in _byId map) and most importantly a private model._generateId(attrs) that could be called from the collection instance as model static method.
The collection.model would be either the model constructor (not an instance) or a map associating type and model constructors.
When passing raw attributes to collection.set (or collection.add for that matter) the type returned by generateType(attrs) would be used to get the appropriate model constructor.
Alternatively the type could explicitly be passed in collection.set method....maybe as an options attribute?
After knowing type and consequently the model constructor Model.prototype._generateId(attrs) would be called (internally by the collection) to get the model Id.
The model Id (and model.id) is always a string and the default implementation would simply join(",") on composite PK models.
collection._generateId would simply take model id and type and return the models map key, defaulting to modelId + "-" + type
The collection.get would then take optional "type" argument. Either collection.get(1,"typeA") or collection.get("1-typeA"). If course...if there's only one model in collection the normal get(1) still works
as well as passing the model instance.
For composite Pk models get([1,"a"]) or get({id:1,type:"a"}) also work.

Does this make any sense to you? Should I create a PR?

Collaborator

caseywebdev commented May 1, 2014

@ruifortes It's not clear to me what those additional methods would give you. I'd be curious to see your explanation ported to code in a PR.

There aren't really relevant additional methods, maybe the explanation was a little confusing.
"model._generateId" is just a method to avoid repeatedly generate the id from idAttribute because support for composite key models would add a little extra code to it and "collection._generateId" is just concatenation utility method.
The only real difference is using "collection.generateType" instead of "collection.generateId"
It is just an attempt find the minimal overridable function but maybe it losses some versatility.

I guess the real issue is how to retrieve the correct model constructor given just raw attributes.
The current implementation of collection.model acts like a constructor. It expects attributes and returns a new instance of the appropriate model.
When calling adding or updating models in collection by raw attributes there's the need generate the correct id to check if the model already exists in the collection.
To generate model id we must access the model.idAttribute (or the proposed _generateId) but it shouldn't be necessary to create a new model instance.
To access idAttribute as a static property collection.model could just return the appropriate model constructor without instantiating it.

The actual code is:

model: function (attrs) {
return attrs.type === 'a' ? new A(attrs) : new B(attrs);
},

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

Note that the above generateId method is only working because it defaults to 'id'
"this.model.prototype.idAttribute" is not really returning anything
At least it should be "this.model(attrs).prototype.idAttribute" but this always creates a new instance even when not necessary.

I guess something like the following should work:

model: function (attrs) {
return attrs.type === 'a' ? A : new B;
},

generateId: function (attrs) {
this.model(attrs).prototype._generateId(attrs) + "-" + attrs.type
}

What I'm considering here is that there's no real need to completely control the id string generation and that the following should suffice

model:{a:A, b:B},

generateType: function(attrs) {return attrs.type}

or instead of generateType name just typeAttribute that could be a function or just a string:

typeAttribute: "type"

I guess this is somewhat less versatile and involves some additional type checking.

What are the use cases that need more control of generated id ?

Not overriding the complete id generation would also allow collection.get(1,"a") instead of collection.get("1-a") but...that's not really a bid advantage.

I guess being able to access model constructor without instantiating seams a real issue with this PR though

PS: Is there any use case that justifies adding models to the collection that don't define a type attribute but being able to explicitly pass the type as argument in collection.add or set?

Collaborator

caseywebdev commented May 2, 2014

@ruifortes It sounds like you're suggesting an enhancement to the way Backbone deals with determining polymorphic models in collections. Changing that is out of the scope of this PR. I'd recommend creating your own PR after the fate of this one is decided so that discussion on those changes can go there.

The goal here is to make trivial the currently nontrivial task of keeping data that is unique by a composite key in a proper Backbone.Collection set, just like Backbone is great at with data that's unique by a single attribute. I think that by defining that uniqueness rule (generateId) on the collection, we have basically followed RDBMS style where records do not need to have an id column or even a single other column that identifies them, as any number of columns combined are allowed to do that.

Collaborator

caseywebdev commented May 6, 2014

After using this for a couple weeks I must say I'm happy with the implementation and the new range of possibilities. @tgriesser did you get a chance to look at it any further?

I'm looking at this feature in the context of handling model id values which are nested in the server response object. It seems like the generateId functionality will require duplicate parsing logic:

var MyModel = Backbone.Model.extend({
  parse: function(attrs) {
    attrs.id = attrs.nested.id;
    return attrs;
  }
});

var MyCollection = Backbone.Collection.extend({
  model: MyModel,
  generateId: function(attrs) {
    return attrs.nested.id;
  }
});

We need the Model#parse logic so each model maintains the correct id (and so my endpoint url is correct when syncing a single model). And we need the Collection#generateId method so that the collection can prevent duplicating models (see #3147, and this jsfiddle).

Am I understanding this correctly?

Collaborator

caseywebdev commented May 14, 2014

Your understanding is pretty much correct. Ideally we could have Model#generateId as well (see #3133 which was merged and reverted) to maintain a correct model.id property, or better yet completely remove the notion of model.id. This PR removes Collections' dependency on model.id in favor of a more flexible approach that can account for nested or composite identifiers.

My hope was to get this merged in, and then work on a branch that completely removes the redundant model.id in favor of a declarative Model#generateId that can get you an always-correct id given a set of attributes, regardless of how your data is structured.

Sounds like the way to go. Godspeed!

Hope it gets merged soon, totally support the idea behind generateId 👍

@braddunbar braddunbar commented on an outdated diff Jun 20, 2014

if (remove) modelMap[existing.cid] = true;
if (merge) {
- attrs = attrs === model ? model.attributes : attrs;
+ attrs = this._isModel(attrs) ? attrs.attributes : attrs;
@braddunbar

braddunbar Jun 20, 2014

Collaborator

We should probably also check attrs === existing here as we don't need to set a model with it's own attributes.

@braddunbar

braddunbar Jun 20, 2014

Collaborator

We should probably also check attrs === existing here as we don't need to set a model with it's own attributes.

Collaborator

braddunbar commented Jun 20, 2014

Putting this on the collection is much nicer than on the model. It allows multiple model types in a collection without changing model semantics in general...not to mention different behavior in different collections.

I don't love the name generateId as we're not really creating data, just teasing it out of the model. Perhaps findId, modelId, getId, idFor...?

Collaborator

braddunbar commented Jun 20, 2014

Maybe we should even call it key instead since it no longer maps directly to an id.

Collaborator

akre54 commented Jun 20, 2014

Thumbsup modelKey

Owner

jashkenas commented Jun 20, 2014

Let's keep the id terminology the same. I'd go with modelId, to mirror in part collection.model

Collaborator

caseywebdev commented Jun 21, 2014

Updated to modelId.

Owner

jashkenas commented Jun 27, 2014

Groovy!

jashkenas merged commit 8761f25 into jashkenas:master Jun 27, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

jashkenas added the fixed label Jun 27, 2014

caseywebdev deleted the caseywebdev:generate-id branch Jun 30, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment