Add option for accessible_by querying strategy

CHANGELOG.md

@@ -1,6 +1,7 @@
 ## Unreleased
 
 * [#649](https://github.com/CanCanCommunity/cancancan/pull/649): Add support for Single Table Inheritance. ([@Liberatys][])
+* [#655](https://github.com/CanCanCommunity/cancancan/pull/655): Add option for `accessible_by` querying strategy. ([@ghiculescu][])
 
 ## 3.1.0
 
@@ -670,3 +671,4 @@ Please read the [guide on migrating from CanCanCan 2.x to 3.0](https://github.co
 [@albb0920]: https://github.com/albb0920
 [@ayumu838]: https://github.com/ayumu838
 [@Liberatys]: https://github.com/Liberatys
+[@ghiculescu]: https://github.com/ghiculescu

lib/cancan.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require 'cancan/version'
+require 'cancan/config'
 require 'cancan/parameter_validators'
 require 'cancan/ability'
 require 'cancan/rule'

lib/cancan/config.rb

@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+module CanCan
+  def self.valid_accessible_by_strategies
+    strategies = [:left_join]
+    strategies << :subquery unless CanCan::ModelAdapters::ActiveRecordAdapter.version_lower?('5.0.0')
+    strategies
+  end
+
+  # Determines how CanCan should build queries when calling accessible_by,
+  # if the query will contain a join. The default strategy is `:subquery`.
+  #
+  #   # config/initializers/cancan.rb
+  #   CanCan.accessible_by_strategy = :subquery
+  #
+  # Valid strategies are:
+  # - :subquery - Creates a nested query with all joins, wrapped by a
+  #               WHERE IN query.
+  # - :left_join - Calls the joins directly using `left_joins`, and
+  #                ensures records are unique using `distinct`. Note that
+  #                `distinct` is not reliable in some cases. See
+  #                https://github.com/CanCanCommunity/cancancan/pull/605
+  def self.accessible_by_strategy
+    @accessible_by_strategy || default_accessible_by_strategy
+  end
+
+  def self.default_accessible_by_strategy
+    if CanCan::ModelAdapters::ActiveRecordAdapter.version_lower?('5.0.0')
+      # see https://github.com/CanCanCommunity/cancancan/pull/655 for where this was added
+      # the `subquery` strategy (from https://github.com/CanCanCommunity/cancancan/pull/619
+      # only works in Rails 5 and higher
+      :left_join
+    else
+      :subquery
+    end
+  end
+
+  def self.accessible_by_strategy=(value)
+    unless valid_accessible_by_strategies.include?(value)
+      raise ArgumentError, "accessible_by_strategy must be one of #{valid_accessible_by_strategies.join(', ')}"
+    end
+
+    if value == :subquery && CanCan::ModelAdapters::ActiveRecordAdapter.version_lower?('5.0.0')
+      raise ArgumentError, 'accessible_by_strategy = :subquery requires ActiveRecord 5 or newer'
+    end
+
+    @accessible_by_strategy = value
+  end
+end

lib/cancan/model_adapters/active_record_4_adapter.rb

@@ -34,10 +34,8 @@ def matches_condition?(subject, name, value)
       # look inside the where clause to decide to outer join tables
       # you're using in the where. Instead, `references()` is required
       # in addition to `includes()` to force the outer join.
-      def build_relation(*where_conditions)
-        relation = @model_class.where(*where_conditions)
-        relation = relation.includes(joins).references(joins) if joins.present?
-        relation
+      def build_joins_relation(relation, *_where_conditions)
+        relation.includes(joins).references(joins)
       end
 
       # Rails 4.2 deprecates `sanitize_sql_hash_for_conditions`

lib/cancan/model_adapters/active_record_5_adapter.rb

@@ -21,14 +21,16 @@ def self.matches_condition?(subject, name, value)
 
       private
 
-      def build_relation(*where_conditions)
-        if joins.present?
+      def build_joins_relation(relation, *where_conditions)
+        case CanCan.accessible_by_strategy
+        when :subquery
           inner = @model_class.unscoped do
             @model_class.left_joins(joins).where(*where_conditions)
           end
           @model_class.where(@model_class.primary_key => inner)
-        else
-          @model_class.where(*where_conditions)
+
+        when :left_join
+          relation.left_joins(joins).distinct
         end
       end
 

lib/cancan/model_adapters/active_record_adapter.rb

@@ -59,6 +59,14 @@ def database_records
         end
       end
 
+      def build_relation(*where_conditions)
+        relation = @model_class.where(*where_conditions)
+        return relation unless joins.present?
+
+        # subclasses must implement `build_joins_relation`
+        build_joins_relation(relation, *where_conditions)
+      end
+
       # Returns the associations used in conditions for the :joins option of a search.
       # See ModelAdditions#accessible_by
       def joins

spec/cancan/model_adapters/accessible_by_has_many_through_spec.rb

@@ -72,31 +72,66 @@ class Editor < ActiveRecord::Base
     ability.can :read, Post, editors: { user_id: @user1 }
   end
 
-  describe 'preloading of associations' do
-    it 'preloads associations correctly' do
-      posts = Post.accessible_by(ability).where(published: true).includes(likes: :user)
-      expect(posts[0].association(:likes)).to be_loaded
-      expect(posts[0].likes[0].association(:user)).to be_loaded
+  CanCan.valid_accessible_by_strategies.each do |strategy|
+    context "using #{strategy} strategy" do
+      before :each do
+        CanCan.accessible_by_strategy = strategy
+      end
+
+      describe 'preloading of associations' do
+        it 'preloads associations correctly' do
+          posts = Post.accessible_by(ability).where(published: true).includes(likes: :user)
+          expect(posts[0].association(:likes)).to be_loaded
+          expect(posts[0].likes[0].association(:user)).to be_loaded
+        end
+      end
+
+      if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0')
+        describe 'selecting custom columns' do
+          it 'extracts custom columns correctly' do
+            posts = Post.accessible_by(ability).where(published: true).select('title as mytitle')
+            expect(posts[0].mytitle).to eq 'post1'
+          end
+        end
+      end
+
+      describe 'filtering of results' do
+        it 'adds the where clause correctly' do
+          posts = Post.accessible_by(ability).where(published: true)
+          expect(posts.length).to eq 1
+        end
+      end
     end
   end
 
-  describe 'filtering of results' do
-    it 'adds the where clause correctly' do
-      posts = Post.accessible_by(ability).where(published: true)
-      expect(posts.length).to eq 1
-    end
-    it 'adds the where clause correctly with joins' do
-      posts = Post.joins(:editors).where('editors.user_id': @user1.id).accessible_by(ability)
-      expect(posts.length).to eq 1
+  unless CanCan::ModelAdapters::ActiveRecordAdapter.version_lower?('5.0.0')
+    describe 'filtering of results - subquery' do
+      before :each do
+        CanCan.accessible_by_strategy = :subquery
+      end
+
+      it 'adds the where clause correctly with joins' do
+        posts = Post.joins(:editors).where('editors.user_id': @user1.id).accessible_by(ability)
+        expect(posts.length).to eq 1
+      end
     end
   end
 
-  if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0')
-    describe 'selecting custom columns' do
-      it 'extracts custom columns correctly' do
-        posts = Post.accessible_by(ability).where(published: true).select('title as mytitle')
-        expect(posts[0].mytitle).to eq 'post1'
+  describe 'filtering of results - left_joins' do
+    before :each do
+      CanCan.accessible_by_strategy = :left_join
+    end
+
+    if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.2.0')
+      it 'adds the where clause correctly with joins on AR 5.2+' do
+        posts = Post.joins(:editors).where('editors.user_id': @user1.id).accessible_by(ability)
+        expect(posts.length).to eq 1
       end
     end
+
+    it 'adds the where clause correctly without joins' do
+      posts = Post.where('editors.user_id': @user1.id).accessible_by(ability)
+      expect(posts.length).to eq 1
+    end
   end
 end