Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Associations: belongsTo does not work when combined with through #1031

Open
chrisbroome opened this issue Nov 17, 2015 · 10 comments
Open

Associations: belongsTo does not work when combined with through #1031

chrisbroome opened this issue Nov 17, 2015 · 10 comments

Comments

@chrisbroome
Copy link

I've found something similar to #1025, specifically when using belongsTo with through.

Let's say I have Author, Book, and AuthorBook models. Author and Book are standalone entities, and AuthorBook is a join table with the usual author_id and book_id foreign keys.

In my Book model, if i have a relation defined like this:

// plural
authors: function() {
  return this.belongsToMany('Author', 'author_book')
}

Then everything works as expected. The join table is used and the correct results come back.

However, let's say we decide that a book can have only zero or one author (I know that doesn't make much sense but bear with me). Also, we've still decided to use a join table to model the relationship. We can model that with exactly the same table structure, with the addition of a UNIQUE CONSTRAINT on book_id. We can even use the exact same authors association defined earlier. However, because of belongsToMany the result will be a Collection instead of a Model.

From what I've ready, the solution is to use belongsTo with through. For example:

// singular
author: function() {
  return this.belongsTo('Author').through('AuthorBook');
}

However, this never produces the correct join statement for me. Even using the 2nd and 3rd params to through, I'm never able to get it to generate the correct sql. Consequently, the above actually gives a SQL error.

I've gotta run right now, but I'll come back and post the generated SQL later tonight.

@chrisbroome
Copy link
Author

As promised, here's the association definition with the SQL output:

belongsToMany('Author', 'author_book')

    select `author`.*
         , `author_book`.`book_id` as `_pivot_book_id`
         , `author_book`.`author_id` as `_pivot_author_id`
      from `author`
inner join `author_book` on `author_book`.`author_id` = `author`.`id`
     where `author_book`.`book_id` in (?)

.belongsTo('Author').through('AuthorBook')

     select `author`.*
          , `author_book`.`id` as `_pivot_id`
          , `author_book`.`author_id` as `_pivot_author_id`
       from `author`
 inner join `author_book` on `author_book`.`author_id` = `author`.`id`
 inner join `book` on `author_book`.`id` = `book`.`author_book_id`
      where `book`.`id` in (?) 

Notice that the _pivot_id is incorrect

belongsTo('Author').through('AuthorBook', 'author_id', 'book_id')

     select `author`.*
          , `author_book`.`id` as `_pivot_id`
          , `author_book`.`author_id` as `_pivot_author_id`
       from `author`
 inner join `author_book` on `author_book`.`author_id` = `author`.`id`
 inner join `book` on `author_book`.`id` = `book`.`author_id`
      where `book`.`id` in (?) 

Notice that book_id is never used.

To make the last example more clear, I'll give one last example with column names that don't exist just to emphasize the point:

belongsTo('Author').through('AuthorBook', 'through_foreign_key', 'other_key')

    select `author`.*
         , `author_book`.`id` as `_pivot_id`
         , `author_book`.`author_id` as `_pivot_author_id`
      from `author`
inner join `author_book` on `author_book`.`author_id` = `author`.`id`
inner join `book` on `author_book`.`id` = `book`.`through_foreign_key`
     where `book`.`id` in (?)

The fix

The SQL produced by the first case, belongsToMany('Author', 'author_book') is the correct SQL. In order to get belongsTo to output this SQL, we could let it accept a second parameter that specifies the join table to use – exactly like what belongsToMany does now. For example:

this.belongsTo('Author', 'author_book');

would produce the same SQL as belongsToMany. The only thing that would change is that a Model would be returned with the first result, instead of Collection.

@ricardograca
Copy link
Member

Can you explain why those queries are incorrect and what you expected them to be? Also, can you check if .hasOne().through() is producing the query that you're after or not?

Sorry, I meant .hasOne, not hasMany, so I've updated my question.

@chrisbroome
Copy link
Author

Yep. Here's hasMany('Author').through('AuthorBook')

    select `author`.*
         , `author_book`.`id` as `_pivot_id`
         , `author_book`.`book_id` as `_pivot_book_id`
      from `author`
inner join `author_book` on `author_book`.`id` = `author`.`author_book_id`
     where `author_book`.`book_id` in (?)

This is incorrect because there is no author`.`author_book_id column

The primary problem with the belongsTo('Author').through('AuthorBook') SQL is that the join table's id is being used as the _pivot_id. I'll try to break this down as best I can. I'm not sure how to get line numbers to show up in language blocks, so I just added them in comments myself.

     select `author`.* -- line 1
          , `author_book`.`id` as `_pivot_id` -- line 2
          , `author_book`.`author_id` as `_pivot_author_id` -- line 3
       from `author` -- line 4
 inner join `author_book` on `author_book`.`author_id` = `author`.`id` -- line 5
 inner join `book` on `author_book`.`id` = `book`.`author_book_id` -- line 6
      where `book`.`id` in (?) -- line 7

Line 2 should be , author_book.book_idas_pivot_book_id``
Line 6 should be inner join book` on `author_book`.`book_id` = `book`.`id` -- line 5`

@chrisbroome
Copy link
Author

I'm in the process of fixing this issue in my fork. Looking through the tests, I see that the use case for belongsTo().through() is to get a model's grandparent.

Blog has many Post.
Post has many Comment.
Comment belongs to Blog through Post.

I think this is the source of my confusion because this is not the use case that I was describing above. Therefore I think no changes should be made to the belongsTo.through relationship type.

The relationship I'm describing is having an optional foreign key live on a join table (rather than as a nullable foreign key in the table itself).

Here's an example schema for such a relationship where a user has either zero or one shopping carts. In other words, a shopping cart belongs to a user.

CREATE TABLE user (
  id integer NOT NULL PRIMARY KEY,
  name varchar(255) NOT NULL
);

CREATE TABLE cart (
  id integer NOT NULL PRIMARY KEY
);

CREATE TABLE user_cart (
  id integer NOT NULL PRIMARY KEY,
  user_id integer NOT NULL,
  cart_id integer NOT NULL
);
ALTER TABLE user_cart ADD CONSTRAINT user_cart_user_id_fk FOREIGN KEY (user_id) REFERENCES user (id) ON DELETE CASCADE ON UPDATE CASCADE;
ALTER TABLE user_cart ADD CONSTRAINT user_cart_cart_id_fk FOREIGN KEY (cart_id) REFERENCES cart (id) ON DELETE CASCADE ON UPDATE CASCADE;
ALTER TABLE user_cart ADD CONSTRAINT user_cart_user_id_unique UNIQUE (user_id);

Note that the UNIQUE constraint confines this to be a zero or one type of relationship.

Maybe the solution is to support explicit hasZeroOrOne and hasZeroOrOne.through relationships as described in #753. Also, I actually like the name hasOptional better.

@ricardograca
Copy link
Member

Right, that seems to be correct. I thought that getting the "grandparent"
was the part that wasn't working. I think the problem with all these
related issues is a misunderstanding of how it works combined with
incomplete and ambiguous documentation.

What about hasOne().through()? Doesn't it achieve what you want?
Em 18/11/2015 19:36, "Chris Broome" notifications@github.com escreveu:

I'm in the process of fixing this issue in my fork. Looking through the
tests, I see that the use case for belongsTo().through() is to get a
model's grandparent.

Blog has many Post.
Post has many Comment.
Comment belongs to Blog through Post.

I think this is the source of my confusion because this is not the use
case that I was describing above. Therefore I think no changes should be
made to the belongsTo.through relationship type.

The relationship I'm describing is having an optional foreign key live on
a join table (rather than as a nullable foreign key in the table itself).

Here's an example schema for such a relationship where a user has either
zero or one shopping carts. In other words, a shopping cart belongs to a
user.

CREATE TABLE user (
id integer NOT NULL PRIMARY KEY,
name varchar(255) NOT NULL
);
CREATE TABLE cart (
id integer NOT NULL PRIMARY KEY
);
CREATE TABLE user_cart (
id integer NOT NULL PRIMARY KEY,
user_id integer NOT NULL,
cart_id integer NOT NULL
);ALTER TABLE user_cart ADD CONSTRAINT user_cart_user_id_fk FOREIGN KEY (user_id) REFERENCES user (id) ON DELETE CASCADE ON UPDATE CASCADE;ALTER TABLE user_cart ADD CONSTRAINT user_cart_cart_id_fk FOREIGN KEY (cart_id) REFERENCES cart (id) ON DELETE CASCADE ON UPDATE CASCADE;ALTER TABLE user_cart ADD CONSTRAINT user_cart_user_id_unique UNIQUE (user_id);

Note that the UNIQUE constraint confines this to be a zero or one type of
relationship.

Maybe the solution is to support explicit hasZeroOrOne and
hasZeroOrOne.through relationships as described in #753
#753. Also, I actually
like the name hasOptional better.


Reply to this email directly or view it on GitHub
#1031 (comment)
.

@vellotis
Copy link
Contributor

vellotis commented Apr 21, 2016

I share the same problem.

I am using bookshelf-soft-delete package. So I am dealing with records that are soft deleted. So I have three tables.

people homes houses
id id id
deleted_at person_id deleted_at
house_id
deleted_at

And the example models

var Person, House, Home;
Person = Model.extend({
  tableName: 'people',
  house: function() { return /* THE ACTUAL PLACE FOR RELATION DECLARATION */ }
});
Home = Model.extend({
  tableName: 'homes',
  house:
});
House = Model.extend({
  tableName: 'houses'
});

Person.forge({id: 1}).house().fetch()
.then(function(model) {
  /* Handle single model */
});

I would like to have query that is for fetching single record. Something like this

    SELECT `houses`.* FROM `houses`
INNER JOIN `homes` ON `homes`.`house_id` = `houses`.id
     WHERE `homes`.`person_id` = ?
       AND `homes`.`deleted_at` IS NULL
       AND `houses`.`deleted_at` IS NULL
     LIMIT 1

Now some relation declarations and their generated SQL:
this.belongsTo(House);

select `houses`.* from `houses`
 where (`houses`.`deleted_at` is null)
   and `houses`.`id` = ? limit ?

this.belongsTo(House).through(Homes, 'id');

    select `houses`.*,
           `homes`.`id` as `_pivot_id`,
           `homes`.`house_id` as `_pivot_house_id`
      from `houses`
inner join `homes`
        on `homes`.`house_id` = `houses`.`id`
inner join `people`
        on `homes`.`id` = `people`.`id`
        -- `homes`.`id` should be `homes`.`person_id`
        -- `people`.`id` key is the foreign key declared by "through"
     where (`houses`.`deleted_at` is null)
       and `people`.`id` = ? limit ?

this.hasOne(House);

select `houses`.*
  from `houses`
 where (`houses`.`deleted_at` is null)
   and `houses`.`person_id` = ? limit ?

this.hasOne(House).through(Home, 'xxx')

    select `houses`.*,
           `homes`.`id` as `_pivot_id`,
           `homes`.`person_id` as `_pivot_person_id`
      from `houses`
inner join `homes`
        on `homes`.`id` = `houses`.`id`
        -- Should join `homes`.`house_id` instead of `homes`.`id`
        -- `houses`.`id` key is the foreign key declared by "through"
     where (`houses`.`deleted_at` is null)
       and `homes`.`person_id` = ? limit ?

this.belongsToMany(House).through(Home, 'house_id', 'person_id');

    select `houses`.*,
           `homes`.`id` as `_pivot_id`,
           `homes`.`person_id` as `_pivot_person_id`,
           `homes`.`house_id` as `_pivot_house_id`
      from `houses`
inner join `homes`
        on `homes`.`house_id` = `houses`.`id`
     where (`houses`.`deleted_at` is null)
       and `homes`.`person_id` = ?

this.belongsToMany(House).through(Home, 'house_id', 'person_id').query(function(q){q.limit(1);});

    select `houses`.*,
           `homes`.`id` as `_pivot_id`,
           `homes`.`person_id` as `_pivot_person_id`,
           `homes`.`house_id` as `_pivot_house_id`
      from `houses`
inner join `homes`
        on `homes`.`house_id` = `houses`.`id`
     where (`houses`.`deleted_at` is null)
       and `homes`.`person_id` = ? limit ?

The last one is the closest I could get. But fetch() for this relation is still returning collection, not single model.

So @ricardograca. As you see I tried also hasOne through variant.

Is there a nice way for solving this? As stated in #1025 comment, through() method initally served other purpose. And as @chrisbroome stated in the end of this comment I would suggest the same fix for it.

@vellotis
Copy link
Contributor

vellotis commented Apr 21, 2016

Dirty solution by monkey patching Bookshelf.

Person = Model.extend({
  tableName: 'people',
  house: function() {
    return this._relation('belongsTo', House, {
      foreignKey: 'house_id',
      joinClauses: function(knex) {
        knex.join('homes', 'homes' + '.' + this.key('foreignKey'), '=', 'houses' + '.' + this.targetIdAttribute);
        knex.join('people', 'people' + '.' + this.throughIdAttribute, '=', 'homes' + '.' + this.key('throughForeignKey'));
      },
      joinColumns: function(knex) {
        var columns;
        columns = [];
        columns.push(this.throughIdAttribute);
        columns.push(this.key('foreignKey'));
        Array.prototype.push.apply(columns, this.pivotColumns);
        knex.columns(_.map(columns, function(col) {
          return 'homes' + '.' + col + ' as _pivot_' + col;
        }));
      },
      whereClauses: function(knex, response) {
        var key;
        key = 'people' + '.' + this.targetIdAttribute;
        knex.where(key, this.parentFk);
      }
    }).init(this).through(Home, 'person_id');
  }
});

This could be optimized.

This generates following SQL:

    select `brands`.*,
           `homes`.`id` as `_pivot_id`,
           `homes`.`house_id` as `_pivot_house_id`
      from `houses`
inner join `homes` on `homes`.`house_id` = `houses`.`id`
inner join `people` on `people`.`id` = `homes`.`person_id`
     where (`houses`.`deleted_at` is null)
       and `people`.`id` = ? limit ?

@vellotis
Copy link
Contributor

There is a slight problem with this solution. Nested eager loading doesn't work.

@vellotis
Copy link
Contributor

Managed to get it working following hacky way:

Person = Model.extend({
  tableName: 'people',
  house: function() {
    return this._relation('belongsTo', House, {
      foreignKey: 'person_id',
      joinClauses: function(knex) {
        knex.join('homes', 'homes' + '.' + this.otherKey, '=', 'houses' + '.' + this.targetIdAttribute);
      },
      joinColumns: function(knex) {
        var columns;
        columns = [];
        columns.push(this.throughIdAttribute);
        columns.push(this.key('foreignKey'));
        Array.prototype.push.apply(columns, this.pivotColumns);
        knex.columns(_.map(columns, function(col) {
          return 'homes' + '.' + col + ' as _pivot_' + col;
        }));
      },
      whereClauses: function(knex, response) {
        var key;
        key = 'people' + '.' + this.key('foreignKey');
        knex.where(key, this.parentId);
      }
    }).init(this).through(Home, 'person_id', 'house_id');
  }
});
    select `brands`.*,
           `homes`.`id` as `_pivot_id`,
           `homes`.`person_id` as `_pivot_person_id`
      from `houses`
inner join `homes` on `homes`.`house_id` = `houses`.`id`
     where (`houses`.`deleted_at` is null)
       and `homes`.`person_id` = ? limit ?

vellotis added a commit to vellotis/bookshelf that referenced this issue Jun 14, 2016
Current specification of belongsToMany has a bug. Its `otherKey` and `foreignKey` parameter description are mistakenly partially swapped.

Many issues have come up because of this problem. eg. bookshelf#397, bookshelf#1031, bookshelf#1276
@crunchtime-ali
Copy link
Member

The project leadership of Bookshelf recently changed. In an effort to advance the project we close all issues older than one year.

If you think this issue needs to be re-evaluated please post a comment on why this is still important and we will re-open it.

We also started an open discussion about the future of Bookshelf.js here #1600. Feel free to drop by and give us your opinion.
Let's make Bookshelf great again

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

No branches or pull requests

5 participants