Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Make sure model hierarchy is determined when needed and actually nullify reverse relations when model is destroyed #456

Open
wants to merge 2 commits into from

2 participants

@DouweM
Collaborator

No description provided.

@PaulUithol
Owner

Hi Douwe! Could you describe for which scenarios this actually fixes the model hierarchy? I'm a bit worried about performance here. Calling initializeModelHierarchy from addReverseRelation shouldn't hurt too much, but from getCollection... that's called from all over the place.

@DouweM
Collaborator

Yo Paul!

Pfft, I should really have written down the exact circumstances when I ran into the issue, but I was able to retrace most of my my steps.

We have a Listing class defined like so:

class Listing extends Backbone.RelationalModel
  relations: [
    type:           Backbone.HasOne
    key:            "video_section"
    keyDestination: "video_section_id"
    relatedModel:   ContentSectionVideo
    includeInJSON:  ContentSectionVideo::idAttribute
    reverseRelation:
      type:           Backbone.HasOne
      key:            "listing"
      keySource: "listing_id"
      includeInJSON:  false
  ]

  @setup()

Listing has a HasOne relation to ContentSectionVideo:

class ContentSectionVideo extends ContentSectionBase
  @setup _super

ContentSectionVideo is a subclass (subtype) of ContentSectionBase:

class ContentSectionBase extends Backbone.RelationalModel
  subModelTypes: 
    text:  "ContentSectionText"
    video: "ContentSectionVideo"

  @setup()

When we instantiate a new Listing model the_listing with id: 123, its #set method is called and because it's a new listing, this calls #initializeRelations, which ends up calling new Backbone.HasOne( listing, the_video_section_relation ). The Backbone.Relational constructor needs to know when new ContentSectionVideo objects are born so it can connect them to the listing when the listing_id field matches the listing's ID. It does this by calling this.listenTo( Backbone.Relational.store.getCollection( ContentSectionVideo ), 'relational:add relational:change:id', this.tryAddRelated ).

Because Backbone.Relational.store.getCollection is setup to return the collection for a model's root supermodel if we're dealing with a hierarchy, everything looks fine and dandy, except for the fact that at this point no ContentSectionVideo has been instantiated yet, so its .initializeModelHierarchy was never called, ContentSectionVideo._superModel is null rather than ContentSectionBase, and #getCollection returns a new collection for ContentSectionVideo rather than ContentSectionBase!

When, later on, a ContentSectionVideo is instantiated with listing_id: 123, its #set method is called, which does two things:

  • it calls ContentSectionVideo.initializeModelHierarchy, which sets ContentSectionVideo._superModel to ContentSectionBase; and
  • it calls Backbone.Relational.store.register( the_video ), which adds the_video to Backbone.Relational.store.getCollection( ContentSectionVideo ), which now does return ContentSectionBase, which results in relational:add being triggered on that collection for ContentSectionBase.

the_listing.get( "video_section" ) should now equal the_video, but alas, the listing was never made aware of a new ContentSectionBase being born because it was listening to the wrong collection, and the_listing.get( "video_section" ) is still null.

The point being that getCollection is another possible entry point where _superModel needs to have been set.

When I had figured this out, I started looking for all code that uses _superModel and _subModels, and I found this was the case in addReverseRelation or more specifically _addRelation as well.

I came across the bug in #destroy at the same time, which has to do with the fact that this.getReverseRelations uses this.related, which had actually just been set to null, resulting in the reverse relations not being nullified.

And now my hands hurt from all the typing :) If you can think of a more performant implementation, go for it, but I hope you see the problem now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 8, 2014
  1. @DouweM
  2. @DouweM

    Make sure model hierarchy is determined when needed and actually null…

    DouweM authored
    …ify reverse relations when model is destroyed.
This page is out of date. Refresh to see the latest.
Showing with 30 additions and 10 deletions.
  1. +12 −5 backbone-relational.js
  2. +18 −5 index.html
View
17 backbone-relational.js
@@ -261,6 +261,10 @@
});
if ( !exists && relation.model && relation.type ) {
+ // we need the model hierarchy in _addRelation,
+ // but it's possible it hasn't been determined yet.
+ relation.model.initializeModelHierarchy();
+
this._reverseRelations.push( relation );
this._addRelation( relation.model, relation );
this.retroFitRelation( relation );
@@ -340,7 +344,10 @@
if ( type instanceof Backbone.RelationalModel ) {
type = type.constructor;
}
-
+
+ // it's possible no model hierarchy has been determined yet.
+ type.initializeModelHierarchy();
+
var rootModel = type;
while ( rootModel._superModel ) {
rootModel = rootModel._superModel;
@@ -760,16 +767,16 @@
destroy: function() {
this.stopListening();
+ _.each( this.getReverseRelations(), function( relation ) {
+ relation.removeRelated( this.instance );
+ }, this );
+
if ( this instanceof Backbone.HasOne ) {
this.setRelated( null );
}
else if ( this instanceof Backbone.HasMany ) {
this.setRelated( this._prepareCollection() );
}
-
- _.each( this.getReverseRelations(), function( relation ) {
- relation.removeRelated( this.instance );
- }, this );
}
});
View
23 index.html
@@ -883,11 +883,14 @@ <h4 class="code">
the method that Backbone-relational overrides to set up relations as you're defining your <q>Backbone.RelationalModel</q> subclass.
</p>
<p>
- In this case, you should call <q>setup</q> manually after defining your subclass CoffeeScript-style. For example:
+ In this case, you should call <q>setup</q> manually after defining your subclass CoffeeScript-style. If your class extends a model of your own instead of <q>Backbone.RelationalModel</q> directly, you need to pass the superclass as an argument. For example:
</p>
<p class="warning">
Note: this is a static method. It operates on the model type itself, not on an instance of it.
</p>
+ <p class="warning">
+ Inside the CoffeeScript class definition, <q>this</q> and <q>@</q> refer to the class being defined, so <q>@setup()</q> at the end of the <q>Mammal</q> class definition is equivalent to the static method call <q>Mammal.setup()</q>.
+ </p>
</section>
<pre class="language-javascript"><code class="language-javascript"><!--
@@ -896,20 +899,30 @@ <h4 class="code">
class Mammal extends Animal
subModelTypes:
- "primate": "Primate"
+ "primate": "Primate"
"carnivore": "Carnivore"
relations: [
- # More relations
+ # Relations
]
-Mammal.setup()
+ @setup()
class Primate extends Mammal
+ relations: [
+ # More relations
+ ]
+
+ @setup Mammal
class Carnivore extends Mammal
+ @setup Mammal
+
+chimp = Mammal.build
+ id: 3
+ species: "chimp"
+ type: "primate"
-chimp = Mammal.build( { id: 3, species: "chimp", type: "primate" } )
</code></pre>
<section id="RelationalModel-build">
Something went wrong with that request. Please try again.