Create separate active_record_5_adapter and update rails 5.2 from bet…

…a to rc1 CanCanCommunity#478

.travis.yml

@@ -12,7 +12,7 @@ gemfile:
   - gemfiles/activerecord_4.2.gemfile
   - gemfiles/activerecord_5.0.2.gemfile
   - gemfiles/activerecord_5.1.0.gemfile
-  - gemfiles/activerecord_5.2.0.beta2.gemfile
+  - gemfiles/activerecord_5.2.0.rc1.gemfile
 services:
   - mongodb
 matrix:
@@ -35,9 +35,9 @@ matrix:
     - rvm: jruby-9.1.9.0
       gemfile: gemfiles/activerecord_5.1.0.gemfile
     - rvm: jruby-9.0.5.0
-      gemfile: gemfiles/activerecord_5.2.0.beta2.gemfile
+      gemfile: gemfiles/activerecord_5.2.0.rc1.gemfile
     - rvm: jruby-9.1.9.0
-      gemfile: gemfiles/activerecord_5.2.0.beta2.gemfile
+      gemfile: gemfiles/activerecord_5.2.0.rc1.gemfile
 notifications:
   email:
     recipients:

Appraisals

@@ -47,10 +47,10 @@ appraise 'activerecord_5.1.0' do
   end
 end
 
-appraise 'activerecord_5.2.0.beta2' do
-  gem 'activerecord', '~> 5.2.0.beta2', require: 'active_record'
-  gem 'activesupport', '~> 5.2.0.beta2', require: 'active_support/all'
-  gem 'actionpack', '~> 5.2.0.beta2', require: 'action_pack'
+appraise 'activerecord_5.2.0.rc1' do
+  gem 'activerecord', '~> 5.2.0.rc1', require: 'active_record'
+  gem 'activesupport', '~> 5.2.0.rc1', require: 'active_support/all'
+  gem 'actionpack', '~> 5.2.0.rc1', require: 'action_pack'
 
   gemfile.platforms :jruby do
     gem 'activerecord-jdbcsqlite3-adapter'

gemfiles/activerecord_5.2.0.beta2.gemfile → gemfiles/activerecord_5.2.0.rc1.gemfile

@@ -2,9 +2,9 @@
 
 source "https://rubygems.org"
 
-gem "activerecord", "~> 5.2.0.beta2", require: "active_record"
-gem "activesupport", "~> 5.2.0.beta2", require: "active_support/all"
-gem "actionpack", "~> 5.2.0.beta2", require: "action_pack"
+gem "activerecord", "~> 5.2.0.rc1", require: "active_record"
+gem "activesupport", "~> 5.2.0.rc1", require: "active_support/all"
+gem "actionpack", "~> 5.2.0.rc1", require: "action_pack"
 
 platforms :jruby do
   gem "activerecord-jdbcsqlite3-adapter"

lib/cancan.rb

@@ -11,5 +11,6 @@
 
 if defined? ActiveRecord
   require 'cancan/model_adapters/active_record_adapter'
-  require 'cancan/model_adapters/active_record_4_adapter'
+  require 'cancan/model_adapters/active_record_4_adapter' if ActiveRecord.version < Gem::Version.new('5')
+  require 'cancan/model_adapters/active_record_5_adapter' if ActiveRecord.version > Gem::Version.new('5')
 end

lib/cancan/model_adapters/active_record_4_adapter.rb

@@ -39,11 +39,8 @@ def build_relation(*where_conditions)
 
       # Rails 4.2 deprecates `sanitize_sql_hash_for_conditions`
       def sanitize_sql(conditions)
-        if ActiveRecord::VERSION::MAJOR > 4 && conditions.is_a?(Hash)
-          sanitize_sql_activerecord5(conditions)
-        elsif ActiveRecord::VERSION::MINOR >= 2 && conditions.is_a?(Hash)
+        if ActiveRecord::VERSION::MINOR >= 2 && conditions.is_a?(Hash)
           sanitize_sql_activerecord4(conditions)
-
         else
           @model_class.send(:sanitize_sql, conditions)
         end
@@ -59,32 +56,6 @@ def sanitize_sql_activerecord4(conditions)
           @model_class.send(:connection).visitor.compile b
         end.join(' AND ')
       end
-
-      def sanitize_sql_activerecord5(conditions)
-        table = @model_class.send(:arel_table)
-        table_metadata = ActiveRecord::TableMetadata.new(@model_class, table)
-        predicate_builder = ActiveRecord::PredicateBuilder.new(table_metadata)
-
-        conditions = predicate_builder.resolve_column_aliases(conditions)
-        conditions = @model_class.send(:expand_hash_conditions_for_aggregates, conditions)
-
-        conditions.stringify_keys!
-
-        predicate_builder.build_from_hash(conditions).map do |b|
-          visit_nodes(b)
-        end.join(' AND ')
-      end
-
-      def visit_nodes(b)
-        # Rails 5.2 adds a BindParam node that prevents the visitor method from properly compiling the SQL query
-        if ActiveRecord::VERSION::MINOR >= 2
-          connection = @model_class.send(:connection)
-          collector = Arel::Collectors::SubstituteBinds.new(connection, Arel::Collectors::SQLString.new)
-          connection.visitor.accept(b, collector).value
-        else
-          @model_class.send(:connection).visitor.compile(b)
-        end
-      end
     end
   end
 end

lib/cancan/model_adapters/active_record_5_adapter.rb

@@ -0,0 +1,75 @@
+module CanCan
+  module ModelAdapters
+    class ActiveRecord5Adapter < AbstractAdapter
+      include ActiveRecordAdapter
+      def self.for_class?(model_class)
+        model_class <= ActiveRecord::Base
+      end
+
+      # TODO: this should be private
+      def self.override_condition_matching?(subject, name, _value)
+        subject.class.defined_enums.include?(name.to_s)
+      end
+
+      # TODO: this should be private
+      def self.matches_condition?(subject, name, value)
+        # Get the mapping from enum strings to values.
+        enum = subject.class.send(name.to_s.pluralize)
+        # Get the value of the attribute as an integer.
+        attribute = enum[subject.send(name)]
+        # Check to see if the value matches the condition.
+        if value.is_a?(Enumerable)
+          value.include? attribute
+        else
+          attribute == value
+        end
+      end
+
+      private
+
+      # As of rails 4, `includes()` no longer causes active record to
+      # 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
+      end
+
+      # Rails 4.2 deprecates `sanitize_sql_hash_for_conditions`
+      def sanitize_sql(conditions)
+        if conditions.is_a?(Hash)
+          sanitize_sql_activerecord5(conditions)
+        else
+          @model_class.send(:sanitize_sql, conditions)
+        end
+      end
+
+      def sanitize_sql_activerecord5(conditions)
+        table = @model_class.send(:arel_table)
+        table_metadata = ActiveRecord::TableMetadata.new(@model_class, table)
+        predicate_builder = ActiveRecord::PredicateBuilder.new(table_metadata)
+
+        conditions = predicate_builder.resolve_column_aliases(conditions)
+
+        conditions.stringify_keys!
+
+        predicate_builder.build_from_hash(conditions).map do |b|
+          visit_nodes(b)
+        end.join(' AND ')
+      end
+
+      def visit_nodes(b)
+        # Rails 5.2 adds a BindParam node that prevents the visitor method from properly compiling the SQL query
+        if ActiveRecord::VERSION::MINOR >= 2
+          connection = @model_class.send(:connection)
+          collector = Arel::Collectors::SubstituteBinds.new(connection, Arel::Collectors::SQLString.new)
+          connection.visitor.accept(b, collector).value
+        else
+          @model_class.send(:connection).visitor.compile(b)
+        end
+      end
+    end
+  end
+end

spec/cancan/model_adapters/active_record_5_adapter_spec.rb

@@ -0,0 +1,158 @@
+require 'spec_helper'
+
+if defined? CanCan::ModelAdapters::ActiveRecord5Adapter
+  describe CanCan::ModelAdapters::ActiveRecord5Adapter do
+    context 'with sqlite3' do
+      before :each do
+        ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
+        ActiveRecord::Migration.verbose = false
+        ActiveRecord::Schema.define do
+          create_table(:parents) do |t|
+            t.timestamps null: false
+          end
+
+          create_table(:children) do |t|
+            t.timestamps null: false
+            t.integer :parent_id
+          end
+        end
+
+        class Parent < ActiveRecord::Base
+          has_many :children, -> { order(id: :desc) }
+        end
+
+        class Child < ActiveRecord::Base
+          belongs_to :parent
+        end
+
+        (@ability = double).extend(CanCan::Ability)
+      end
+
+      it 'respects scope on included associations' do
+        @ability.can :read, [Parent, Child]
+
+        parent = Parent.create!
+        child1 = Child.create!(parent: parent, created_at: 1.hours.ago)
+        child2 = Child.create!(parent: parent, created_at: 2.hours.ago)
+
+        expect(Parent.accessible_by(@ability).order(created_at: :asc).includes(:children).first.children)
+          .to eq [child2, child1]
+      end
+
+      it 'allows filters on enums' do
+        ActiveRecord::Schema.define do
+          create_table(:shapes) do |t|
+            t.integer :color, default: 0, null: false
+          end
+        end
+
+        class Shape < ActiveRecord::Base
+          enum color: %i[red green blue]
+        end
+
+        red = Shape.create!(color: :red)
+        green = Shape.create!(color: :green)
+        blue = Shape.create!(color: :blue)
+
+        # A condition with a single value.
+        @ability.can :read, Shape, color: Shape.colors[:green]
+
+        expect(@ability.cannot?(:read, red)).to be true
+        expect(@ability.can?(:read, green)).to be true
+        expect(@ability.cannot?(:read, blue)).to be true
+
+        accessible = Shape.accessible_by(@ability)
+        expect(accessible).to contain_exactly(green)
+
+        # A condition with multiple values.
+        @ability.can :update, Shape, color: [Shape.colors[:red],
+                                             Shape.colors[:blue]]
+
+        expect(@ability.can?(:update, red)).to be true
+        expect(@ability.cannot?(:update, green)).to be true
+        expect(@ability.can?(:update, blue)).to be true
+
+        accessible = Shape.accessible_by(@ability, :update)
+        expect(accessible).to contain_exactly(red, blue)
+      end
+
+      it 'allows dual filter on enums' do
+        ActiveRecord::Schema.define do
+          create_table(:discs) do |t|
+            t.integer :color, default: 0, null: false
+            t.integer :shape, default: 3, null: false
+          end
+        end
+
+        class Disc < ActiveRecord::Base
+          enum color: %i[red green blue]
+          enum shape: { triangle: 3, rectangle: 4 }
+        end
+
+        red_triangle = Disc.create!(color: Disc.colors[:red], shape: Disc.shapes[:triangle])
+        green_triangle = Disc.create!(color: Disc.colors[:green], shape: Disc.shapes[:triangle])
+        green_rectangle = Disc.create!(color: Disc.colors[:green], shape: Disc.shapes[:rectangle])
+        blue_rectangle = Disc.create!(color: Disc.colors[:blue], shape: Disc.shapes[:rectangle])
+
+        # A condition with a dual filter.
+        @ability.can :read, Disc, color: Disc.colors[:green], shape: Disc.shapes[:rectangle]
+
+        expect(@ability.cannot?(:read, red_triangle)).to be true
+        expect(@ability.cannot?(:read, green_triangle)).to be true
+        expect(@ability.can?(:read, green_rectangle)).to be true
+        expect(@ability.cannot?(:read, blue_rectangle)).to be true
+
+        accessible = Disc.accessible_by(@ability)
+        expect(accessible).to contain_exactly(green_rectangle)
+      end
+    end
+
+    if Gem::Specification.find_all_by_name('pg').any?
+      context 'with postgresql' do
+        before :each do
+          ActiveRecord::Base.establish_connection(adapter: 'postgresql',
+                                                  database: 'postgres',
+                                                  schema_search_path: 'public')
+          ActiveRecord::Base.connection.drop_database('cancan_postgresql_spec')
+          ActiveRecord::Base.connection.create_database('cancan_postgresql_spec',
+                                                        'encoding' => 'utf-8',
+                                                        'adapter' => 'postgresql')
+          ActiveRecord::Base.establish_connection(adapter: 'postgresql',
+                                                  database: 'cancan_postgresql_spec')
+          ActiveRecord::Migration.verbose = false
+          ActiveRecord::Schema.define do
+            create_table(:parents) do |t|
+              t.timestamps null: false
+            end
+
+            create_table(:children) do |t|
+              t.timestamps null: false
+              t.integer :parent_id
+            end
+          end
+
+          class Parent < ActiveRecord::Base
+            has_many :children, -> { order(id: :desc) }
+          end
+
+          class Child < ActiveRecord::Base
+            belongs_to :parent
+          end
+
+          (@ability = double).extend(CanCan::Ability)
+        end
+
+        it 'allows overlapping conditions in SQL and merge with hash conditions' do
+          @ability.can :read, Parent, children: { parent_id: 1 }
+          @ability.can :read, Parent, children: { parent_id: 1 }
+
+          parent = Parent.create!
+          Child.create!(parent: parent, created_at: 1.hours.ago)
+          Child.create!(parent: parent, created_at: 2.hours.ago)
+
+          expect(Parent.accessible_by(@ability)).to eq([parent])
+        end
+      end
+    end
+  end
+end

spec/cancan/model_adapters/active_record_adapter_spec.rb

@@ -81,16 +81,17 @@ class User < ActiveRecord::Base
 
     it 'is for only active record classes' do
       if ActiveRecord.respond_to?(:version) &&
-         ActiveRecord.version > Gem::Version.new('4')
+         ActiveRecord.version < Gem::Version.new('5')
         expect(CanCan::ModelAdapters::ActiveRecord4Adapter).to_not be_for_class(Object)
         expect(CanCan::ModelAdapters::ActiveRecord4Adapter).to be_for_class(Article)
         expect(CanCan::ModelAdapters::AbstractAdapter.adapter_class(Article))
           .to eq(CanCan::ModelAdapters::ActiveRecord4Adapter)
-      else
-        expect(CanCan::ModelAdapters::ActiveRecord3Adapter).to_not be_for_class(Object)
-        expect(CanCan::ModelAdapters::ActiveRecord3Adapter).to be_for_class(Article)
+      elsif ActiveRecord.respond_to?(:version) &&
+            ActiveRecord.version > Gem::Version.new('5')
+        expect(CanCan::ModelAdapters::ActiveRecord5Adapter).to_not be_for_class(Object)
+        expect(CanCan::ModelAdapters::ActiveRecord5Adapter).to be_for_class(Article)
         expect(CanCan::ModelAdapters::AbstractAdapter.adapter_class(Article))
-          .to eq(CanCan::ModelAdapters::ActiveRecord3Adapter)
+          .to eq(CanCan::ModelAdapters::ActiveRecord5Adapter)
       end
     end