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

Already on GitHub? Sign in to your account

Duplicate joins created when using has_many :through and explicit joins #4016

Closed
dvandersluis opened this Issue Dec 18, 2011 · 5 comments

Comments

Projects
None yet
5 participants

I'm not sure if this is related to #2556, but I'm using Rails 3.1.3 and experiencing this problem, so if it is, then it's not resolved still.

class Client < ActiveRecord::Base
  has_many :jobs
  has_many :job_applications, :through => :jobs
end

class JobApplications
  belongs_to :job
end

irb(main):001:0> Client.find(10).job_applications.joins(:job)
ActiveRecord::StatementInvalid: Mysql2::Error: Not unique table/alias: 'jobs': SELECT `job_applications`.* FROM `job_applications` INNER JOIN `jobs` ON `jobs`.`id` = `job_applications`.`job_id` INNER JOIN `jobs` ON `job_applications`.`job_id` = `jobs`.`id` WHERE `jobs`.`client_id` = 10

As you can see, INNER JOINjobsONjobs.id=job_applications.job_id`` is getting duplicated.

Contributor

henrikhodne commented Dec 18, 2011

Can you post your Job model class? I couldn't reproduce this with the information you've given.

I created an example app to demonstrate the problem, although it ends up not being exactly what I thought. It seems that my original problem of the purely duplicated joins is due to squeel (see ernie/squeel#86); without it, rails generates:

irb(main):001:0> Client.find(1).job_applications.joins(:job).to_sql
=> "SELECT `job_applications`.* FROM `job_applications` INNER JOIN `jobs` `jobs_job_applications` ON `jobs_job_applications`.`id` = `job_applications`.`job_id` INNER JOIN `jobs` ON `job_applications`.`job_id` = `jobs`.`id` WHERE `jobs`.`client_id` = 1"

So without squeel Rails still duplicates the join (which I'd still posit is an issue), but at least it uses a different alias so that there isn't a MySQL error.

Member

jonleighton commented Dec 18, 2011

I don't think this is really a bug: you are specifying that there are two joins to jobs in your query - one is implied by loading job_applications and the other by joins(:job). Incorrect aliasing would be a bug but as you say that appears to be in squeel. (AR doesn't attempt to 'compact' multiple joins to the same table - this would open up a can of worms and it's better to let the user have control.)

jsuwo commented Aug 27, 2012

While I see your argument with the OP's code, there are some other cases where this behaviour is problematic. For example, here's a scenario I'm currently encountering:

class User < ActiveRecord::Base
   has_many :badge_awards
   has_many :badges, through: :badge_awards
end

class Course < ActiveRecord::Base
    has_many :badge_awards
    has_many :badges, through: :badge_awards
end

class BadgeAward < ActiveRecord::Base
    belongs_to :user
    belongs_to :course
    belongs_to :badge
end

class Badge < ActiveRecord::Base
    has_many :badge_awards

    scope :for, lambda { |course| joins(:badge_awards).where(['badge_awards.course_id = ?', course]) }
end

So, the idea here is that I can call Badge.for(course), and this will return all the badges that have been awarded to users in a particular course. Alternatively, I can call user.badges.for(course), and this will return all the badges that have been awarded to a particular user in a particular course.

The problem is that user.badges.for(course) generates a duplicate join, and so I get duplicate records in the output:

"SELECT `badges`.* FROM `badges` INNER JOIN `badge_awards` `badge_awards_badges` ON `badge_awards_badges`.`badge_id` = `badges`.`id` INNER JOIN `badge_awards` ON `badges`.`id` = `badge_awards`.`badge_id` WHERE `badge_awards`.`user_id` = 1 AND (badge_awards.course_id = 1)"

Now, I can work around this by using .uniq in the for scope, but this causes problems when I want to get the count of a user's badges for a particular course by appending .count (e.g. user.badges.for(course).count). This is due to issue #6865.

I feel like Badge.for(course) and user.badges.for(course) should both work properly.

BM5k commented Jun 10, 2016

Bit by this with today in rails 4.2.5.1.

I think my solution will be to remove the explicit join from my scope and add it explicitly to every single call that needs it. For eternity.

To borrow the previous example:

scope :for, lambda { |course| joins(:badge_awards).where(['badge_awards.course_id = ?', course]) }

becomes

scope :for, lambda { |course| where(['badge_awards.course_id = ?', course]) }

Badge.for(course) and user.badges.for(course) become
Badge.joins(badge_awards).for(course) and user.badges.for(course)

This is harder to read, and will be more difficult to maintain long term.


I can see where @jonleighton is coming from with this comment

you are specifying that there are two joins

but I disagree. I think it was a stretch to say that the developer I'm specifying two joins. What I'm doing is specifying a relationship (that the framework handles) and a single join. Rails is helpfully providing the second join, and ideally should be smart enough to know when that's not necessary.

Another possible solution would be an idiomatic way for scopes to determine if the proper join already exists and piggyback on that. Then we could push the boilerplate into a single place (the scope definition) and then be free to use the scope as normal.

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