Skip to content

Commit

Permalink
Disallow multiple instantiation of RelationalModels with the same typ…
Browse files Browse the repository at this point in the history 
…e and `id`.

This ensures (at least) deterministic behavior. Fixes #180.

This commit also includes a bit of cleanup in the override for
`Backbone.Collection.prototype.add`.
  • Loading branch information
@PaulUithol
PaulUithol committed Aug 9, 2012
1 parent 4e6da75 commit 351053c
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 30 deletions.
55 changes: 28 additions & 27 deletions backbone-relational.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -337,11 +337,18 @@
* @param {Backbone.RelationalModel} model
*/
register: function( model ) {
var modelColl = model.collection;
var coll = this.getCollection( model );
coll && coll.add( model );
model.bind( 'destroy', this.unregister, this );
model.collection = modelColl;

if ( coll ) {
if ( coll.get( model ) ) {
throw new Error( "Cannot instantiate more than one Backbone.RelationalModel with the same id per type!" );
}

var modelColl = model.collection;
coll.add( model );
model.bind( 'destroy', this.unregister, this );
model.collection = modelColl;
}
},

/**
Expand Down Expand Up @@ -1149,8 +1156,8 @@
/**
* Retrieve related objects.
* @param key {string} The relation key to fetch models for.
* @param options {Object} Options for 'Backbone.Model.fetch' and 'Backbone.sync'.
* @param update {boolean} Whether to force a fetch from the server (updating existing models).
* @param [options] {Object} Options for 'Backbone.Model.fetch' and 'Backbone.sync'.
* @param [update=false] {boolean} Whether to force a fetch from the server (updating existing models).
* @return {jQuery.when[]} An array of request objects
*/
fetchRelated: function( key, options, update ) {
Expand Down Expand Up @@ -1512,7 +1519,7 @@
// (unless 'options.create' is false).
if ( _.isObject( attributes ) ) {
if ( model ) {
model.set( attributes, options );
model.set( model.parse ? model.parse( attributes ) : attributes, options );
}
else if ( !options || ( options && options.create !== false ) ) {
model = this.build( attributes, options );
Expand All @@ -1534,9 +1541,9 @@
if ( !( model instanceof Backbone.Model ) ) {
var attrs = model;
options.collection = this;
if ( typeof this.model.build !== 'undefined' ) {
model = this.model.build( attrs, options );

if ( typeof this.model.findOrCreate !== 'undefined' ) {
model = this.model.findOrCreate( attrs, options );
}
else {
model = new this.model( attrs, options );
Expand Down Expand Up @@ -1568,31 +1575,25 @@

//console.debug( 'calling add on coll=%o; model=%o, options=%o', this, models, options );
_.each( models, function( model ) {
if ( !( model instanceof Backbone.Model ) ) {
// Try to find 'model' in Backbone.store. If it already exists, set the new properties on it.
var existingModel = Backbone.Relational.store.find( this.model, model[ this.model.prototype.idAttribute ] );
if ( existingModel ) {
existingModel.set( existingModel.parse ? existingModel.parse( model ) : model, options );
model = existingModel;
}
else {
model = Backbone.Collection.prototype._prepareModel.call( this, model, options );
}
}
if ( !( model instanceof Backbone.Model ) ) {
// `_prepareModel` attempts to find `model` in Backbone.store through `findOrCreate`,
// and sets the new properties on it if is found. Otherwise, a new model is instantiated.
model = Backbone.Collection.prototype._prepareModel.call( this, model, options );
}

if ( model instanceof Backbone.Model && !this.get( model ) && !this.getByCid( model ) ) {
modelsToAdd.push( model );
}
}, this );
if ( model instanceof Backbone.Model && !this.get( model ) && !this.getByCid( model ) ) {
modelsToAdd.push( model );
}
}, this );


// Add 'models' in a single batch, so the original add will only be called once (and thus 'sort', etc).
if ( modelsToAdd.length ) {
add.call( this, modelsToAdd, options );

_.each( modelsToAdd, function( model ) {
this.trigger( 'relational:add', model, this, options );
}, this );
this.trigger( 'relational:add', model, this, options );
}, this );
}

return this;
Expand Down
49 changes: 46 additions & 3 deletions test/tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,9 @@ $(document).ready(function() {
]
});

window.CompanyCollection = Backbone.Collection.extend({
model: Company
});


window.Node = Backbone.RelationalModel.extend({
Expand Down Expand Up @@ -578,8 +581,8 @@ $(document).ready(function() {
equal( errorCount, 1, "The error callback executed successfully" );

var person2 = new Person({
id: 'person-10',
resource_uri: 'person-10'
id: 'person-11',
resource_uri: 'person-11'
});

requests = person2.fetchRelated( 'user' );
Expand Down Expand Up @@ -1269,7 +1272,7 @@ $(document).ready(function() {
});


module( "Backbone.Relation general", { setup: initObjects } );
module( "Backbone.Relation general" );


test( "Only valid models (no validation failure) should be added to a relation", function() {
Expand Down Expand Up @@ -1348,6 +1351,36 @@ $(document).ready(function() {
equal( child.get( 'parent' ), parent );
});

test("Repeated model initialization and a collection should not break existing models", function () {
var dataCompanyA = {
id: 'company-a',
name: 'Big Corp.',
employees: [ { id: 'job-a' }, { id: 'job-b' } ]
};
var dataCompanyB = {
id: 'company-b',
name: 'Small Corp.',
employees: []
};

var companyA = new Company( dataCompanyA );

// Attempting to instantiate another model with the same data will throw an error
raises( function() { new Company( dataCompanyA ); }, "Can only instantiate one model for a given `id` (per model type)" );

// init-ed a lead and its nested contacts are a collection
ok( companyA.get('employees') instanceof Backbone.Collection, "Company's employees should be a collection" );
equal(companyA.get('employees').length, 2, 'with elements');

var companyCollection = new CompanyCollection( [ dataCompanyA, dataCompanyB ] );

// After loading a collection with models of the same type
// the existing company should still have correct collections
ok( companyCollection.get( dataCompanyA.id ) === companyA );
ok( companyA.get('employees') instanceof Backbone.Collection, "Company's employees should still be a collection" );
equal( companyA.get('employees').length, 2, 'with elements' );
});


module( "Backbone.HasOne", { setup: initObjects } );

Expand Down Expand Up @@ -1818,6 +1851,16 @@ $(document).ready(function() {
ok( child2.get( 'parent' ) === parent );
equal( child2.get( 'children' ).length, 0 );
});

test( "Models referencing each other in the same relation", function() {
var parent = new Node({ id: 1 });
var child = new Node({ id: 2 });

child.set( 'parent', parent );
parent.save( { 'parent': child } );

console.log( parent, child );
});

test( "HasMany relations to self (tree structure)", function() {
var child1 = new Node({ id: '2', name: 'First child' });
Expand Down

2 comments on commit 351053c

@driverdan
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about cases when you'd intentionally want another instance of a model with the same data as one that's already registered? There needs to be an option to disable registering it. There are a number of reasons to want to have copies of models.

Use case: A model has data we sometimes don't want to save to the server but still want available to toJSON(). The only graceful way to handle this is to create a new model with the same data (and ID), remove the unwanted data, then save.

@gfarrell
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And, moreover, cases where Collection.reset are used seem to trigger this error.

Please sign in to comment.