Reverse relation on collection/model #273

Closed
omriyariv opened this Issue Feb 20, 2013 · 8 comments

Comments

Projects
None yet
2 participants
@omriyariv

I have a Project model with a HasMany relation to a File model. defined as follows:

Models.Project = Backbone.RelationalModel.extend({

    relations: [
        {
            type: Backbone.HasMany,
            key: 'files',
            relatedModel: 'Models.File',
            collectionType: 'Collections.Files',
            reverseRelation: {
                key: 'project_id',
                includeInJSON: 'id'
            }
        }
    ]
}

Models.File = Backbone.RelationalModel.extend({});

What I need is that File model will be sent to the server with it's 'project_id' (my server defines a relational SQL DB).

The problem is that the reverse relation is created on the Files collection. The File model can only access the Project model through 'myFile.collection.project_id'. This of course causes the 'project_id' attribute not to be included when calling toJSON() on a File model.

It looks like there is no meaning to includeInJSON on reverseRelation for a HasMany relation.

Am I missing something?

@omriyariv omriyariv closed this Feb 20, 2013

@omriyariv omriyariv reopened this Feb 20, 2013

@PaulUithol

This comment has been minimized.

Show comment
Hide comment
@PaulUithol

PaulUithol Feb 20, 2013

Owner

I don't really get what you mean. When defined like this, a file can access it's project by doing file.get('project_id').

Since you've defined includeInJSON as id, you'll only get a file's id of course when calling file.toJSON(). If you want all fields, set it to true; if you want both id and project_id, set it to ['id, 'project_id'].

Owner

PaulUithol commented Feb 20, 2013

I don't really get what you mean. When defined like this, a file can access it's project by doing file.get('project_id').

Since you've defined includeInJSON as id, you'll only get a file's id of course when calling file.toJSON(). If you want all fields, set it to true; if you want both id and project_id, set it to ['id, 'project_id'].

@omriyariv

This comment has been minimized.

Show comment
Hide comment
@omriyariv

omriyariv Feb 20, 2013

What I forgot to mention is that File model defines 2 subclasses. If I remove the subclass definitions it works as expected. With subclasses however, I don't get the reverse relation on the File model.

What I forgot to mention is that File model defines 2 subclasses. If I remove the subclass definitions it works as expected. With subclasses however, I don't get the reverse relation on the File model.

@PaulUithol

This comment has been minimized.

Show comment
Hide comment
@PaulUithol

PaulUithol Feb 20, 2013

Owner

Could you post a runnable example of your code, preferably a jsFiddle or qunit test - that'll make it a lot easier to see what's going on exactly.

Owner

PaulUithol commented Feb 20, 2013

Could you post a runnable example of your code, preferably a jsFiddle or qunit test - that'll make it a lot easier to see what's going on exactly.

@omriyariv

This comment has been minimized.

Show comment
Hide comment
@omriyariv

omriyariv Feb 27, 2013

Here's the thing:
I managed to recreate the bug I'm getting with a small example. When running over JSFiddle however, the bug does not occur. If you kindly take the code and paste in a small HTML, you will probably be able to see it (I'm using the latest Chrome).

http://jsfiddle.net/omriyariv/h7vdx/4/

Things to note:

  • As you can see, the first File in the files collection which is a Video subclass instance does not contain a reference to Project.
  • If you switch the order of Project and Video model definitions the bug does not occur.
  • I think this is coming from initializeModelHierarchy() method. supermodelRelationsExist is true because Video already inherited File's relation to URL, but the reverse relation for Project is skipped. Maybe this can be resolved by applying a union operation on the supermodel/submodel relations at all times without trying to calculate supermodelRelationsExist value.

Here's the thing:
I managed to recreate the bug I'm getting with a small example. When running over JSFiddle however, the bug does not occur. If you kindly take the code and paste in a small HTML, you will probably be able to see it (I'm using the latest Chrome).

http://jsfiddle.net/omriyariv/h7vdx/4/

Things to note:

  • As you can see, the first File in the files collection which is a Video subclass instance does not contain a reference to Project.
  • If you switch the order of Project and Video model definitions the bug does not occur.
  • I think this is coming from initializeModelHierarchy() method. supermodelRelationsExist is true because Video already inherited File's relation to URL, but the reverse relation for Project is skipped. Maybe this can be resolved by applying a union operation on the supermodel/submodel relations at all times without trying to calculate supermodelRelationsExist value.
@PaulUithol

This comment has been minimized.

Show comment
Hide comment
@PaulUithol

PaulUithol Mar 5, 2013

Owner

@DouweM, you're the super/submodel expert, thoughts on this?

Owner

PaulUithol commented Mar 5, 2013

@DouweM, you're the super/submodel expert, thoughts on this?

@PaulUithol

This comment has been minimized.

Show comment
Hide comment
@PaulUithol

PaulUithol Mar 12, 2013

Owner

Did some digging, and it seems you're 100% correct. The following lines don't take into account that, in addition to the "regular" relations, reverseRelations may have been added before we get here as well:

var supermodelRelationsExist = _.any( this.prototype.relations || [], function( rel ) {
    return rel.model && rel.model !== this;
}, this );
Owner

PaulUithol commented Mar 12, 2013

Did some digging, and it seems you're 100% correct. The following lines don't take into account that, in addition to the "regular" relations, reverseRelations may have been added before we get here as well:

var supermodelRelationsExist = _.any( this.prototype.relations || [], function( rel ) {
    return rel.model && rel.model !== this;
}, this );
@PaulUithol

This comment has been minimized.

Show comment
Hide comment
@PaulUithol

PaulUithol Mar 12, 2013

Owner

How does the following look to you? We can't do a straight _.union (or _.difference) since there's no guarantee at all the objects refs will be the same, and it wouldn't recognize overridden relations either. Your example works nicely with this, the rest of the tests pass as well. Clearly, we need some more of those though.

if ( this._superModel && this._superModel.prototype.relations ) {
    // Find relations that exist on the `_superModel`, but not yet on this model.
    var inheritedRelations = _.select( this._superModel.prototype.relations || [], function( superRel ) {
        return !_.any( this.prototype.relations || [], function( rel ) {
            return superRel.model === rel.model && superRel.key === rel.key;
        }, this );
    }, this );

    this.prototype.relations = inheritedRelations.concat( this.prototype.relations );
}

This would also (I think) allow explicit overriding of specific relations on a submodel; it doesn't look like that would work currently.

Owner

PaulUithol commented Mar 12, 2013

How does the following look to you? We can't do a straight _.union (or _.difference) since there's no guarantee at all the objects refs will be the same, and it wouldn't recognize overridden relations either. Your example works nicely with this, the rest of the tests pass as well. Clearly, we need some more of those though.

if ( this._superModel && this._superModel.prototype.relations ) {
    // Find relations that exist on the `_superModel`, but not yet on this model.
    var inheritedRelations = _.select( this._superModel.prototype.relations || [], function( superRel ) {
        return !_.any( this.prototype.relations || [], function( rel ) {
            return superRel.model === rel.model && superRel.key === rel.key;
        }, this );
    }, this );

    this.prototype.relations = inheritedRelations.concat( this.prototype.relations );
}

This would also (I think) allow explicit overriding of specific relations on a submodel; it doesn't look like that would work currently.

@omriyariv

This comment has been minimized.

Show comment
Hide comment
@omriyariv

omriyariv Mar 12, 2013

Seems like this does the trick for me... Thanks man ;)

Seems like this does the trick for me... Thanks man ;)

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