Use include with references to build relations instead of left_join

[azzz]

Hello dear community,

Preambule

ActiveRecord handles multiple .joins calls carefully. For example, User.joins(:profile).joins(:profile) generates only one INNER JOIN statement. ActiveRecord::QueryMethods#references generates LEFT OUTER JOIN, but combination of .joins and .references with the same relation name produces correct SQL query with single INNER JOIN.
Some people often write controllers like

user = User.where(condition)
user = user.joins(:profile).where(profile: {first_name: params[:first_name]}) if params[:first_name]
user = user.joins(:profile).where(profile: {last_name: params[:last_name]}) if params[:last_name]
user = user.joins(profile).where(profile: {active: true})

and if we always use the same join style, the SQL builder will handle it and generate single INNER JOIN profiles ON ... statement. It works as "some people" expect.

Everything goes wrong when we use mix of left_joins and joins methods. The builder builds "LEFT OUTER JOIN profiles" with "INNER JOIN profiles" in the same time, in the same query. And databases (PG, for example) raise exception when meet several joins on the same table with (or without) the same aliases.

Problem

CanCanCan 2.0 used include with references to build relations for accessible_by method. CanCanCan 3.0 replaced it with left_join. As the result, a lot of legacy smelly code is broken.
For example, this code will generate invalid "LEFT OUTER JOIN profiles ON ... LEFT JOIN profiles ON` query:

# Controller:
user = User.accessible_by(current_ability)
user = user.joins(:profile).where(profile: {first_name: params[:first_name]}) if params[:first_name]
user = user.joins(:profile).where(profile: {last_name: params[:last_name]}) if params[:last_name]

# Ability:
can :read, User, profile: { active: true }

Fix

I returned old behavior. I cannot see valuable advantages to use left_join instead of the old includes.references chain. Please correct me if I'm mistaken.

Thank you.

[ghiculescu]

The change to left_joins was made in #512, and I think the rationale there in terms of minimising how much loading cancan actually does makes sense. I agree there's been some teething issues though. Assuming it's causing you problems, can you please check if #600 fixes them?

[coorasse]

There are different advantages in using left_joins over joins. the main issue that was tackled was the problem of eager loading too many associations when not needed.
I would consider adding an option to the accessible_by method, like accessible_by(current_ability, :index, [true|false]) to use joins instead of left_joins but I'd like to have a regression test for a case that is currently not working. I'd suggest you to open a PR, with a regression test, simulating the case that is causing the issue, or provide a gist. I'll therefore look into it more closely and decide how to proceed. Thank you!