From 8b32f039b4314e7543a158edf3f7aff68d47f267 Mon Sep 17 00:00:00 2001 From: Lizzy Date: Thu, 15 Feb 2018 15:14:52 -0800 Subject: [PATCH] Add active_record_5_adapter and update rails 5.2 final version #478 --- .travis.yml | 6 +- Appraisals | 8 +- CHANGELOG.rdoc | 1 + ...ta2.gemfile => activerecord_5.2.0.gemfile} | 6 +- lib/cancan.rb | 1 + .../model_adapters/active_record_4_adapter.rb | 33 +----- .../model_adapters/active_record_5_adapter.rb | 70 ++++++++++++ .../active_record_4_adapter_spec.rb | 10 +- .../active_record_5_adapter_spec.rb | 100 ++++++++++++++++++ .../active_record_adapter_spec.rb | 13 +-- 10 files changed, 197 insertions(+), 51 deletions(-) rename gemfiles/{activerecord_5.2.0.beta2.gemfile => activerecord_5.2.0.gemfile} (57%) create mode 100644 lib/cancan/model_adapters/active_record_5_adapter.rb create mode 100644 spec/cancan/model_adapters/active_record_5_adapter_spec.rb diff --git a/.travis.yml b/.travis.yml index 30b972b2..fe7bb807 100644 --- a/.travis.yml +++ b/.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.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.gemfile - rvm: jruby-9.1.9.0 - gemfile: gemfiles/activerecord_5.2.0.beta2.gemfile + gemfile: gemfiles/activerecord_5.2.0.gemfile notifications: email: recipients: diff --git a/Appraisals b/Appraisals index ad14e0a7..383d04c2 100644 --- a/Appraisals +++ b/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' do + gem 'activerecord', '~> 5.2.0', require: 'active_record' + gem 'activesupport', '~> 5.2.0', require: 'active_support/all' + gem 'actionpack', '~> 5.2.0', require: 'action_pack' gemfile.platforms :jruby do gem 'activerecord-jdbcsqlite3-adapter' diff --git a/CHANGELOG.rdoc b/CHANGELOG.rdoc index e8155dc8..4b1a1056 100644 --- a/CHANGELOG.rdoc +++ b/CHANGELOG.rdoc @@ -1,6 +1,7 @@ Unreleased * Removed support for dynamic finders (coorasse) +* Fix cancancan#478 - Fixes SQL exception from rails 5.2 upgrade (lizzyaustad & kevintyll) 2.1.3 (Jan 16th, 2018) diff --git a/gemfiles/activerecord_5.2.0.beta2.gemfile b/gemfiles/activerecord_5.2.0.gemfile similarity index 57% rename from gemfiles/activerecord_5.2.0.beta2.gemfile rename to gemfiles/activerecord_5.2.0.gemfile index fb43149d..b3ad6223 100644 --- a/gemfiles/activerecord_5.2.0.beta2.gemfile +++ b/gemfiles/activerecord_5.2.0.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", require: "active_record" +gem "activesupport", "~> 5.2.0", require: "active_support/all" +gem "actionpack", "~> 5.2.0", require: "action_pack" platforms :jruby do gem "activerecord-jdbcsqlite3-adapter" diff --git a/lib/cancan.rb b/lib/cancan.rb index 0545d184..dc06e4ad 100644 --- a/lib/cancan.rb +++ b/lib/cancan.rb @@ -12,4 +12,5 @@ if defined? ActiveRecord require 'cancan/model_adapters/active_record_adapter' require 'cancan/model_adapters/active_record_4_adapter' + require 'cancan/model_adapters/active_record_5_adapter' end diff --git a/lib/cancan/model_adapters/active_record_4_adapter.rb b/lib/cancan/model_adapters/active_record_4_adapter.rb index af4b0f10..d89c3a59 100644 --- a/lib/cancan/model_adapters/active_record_4_adapter.rb +++ b/lib/cancan/model_adapters/active_record_4_adapter.rb @@ -3,7 +3,7 @@ module ModelAdapters class ActiveRecord4Adapter < AbstractAdapter include ActiveRecordAdapter def self.for_class?(model_class) - model_class <= ActiveRecord::Base + ActiveRecord::VERSION::MAJOR == 4 && model_class <= ActiveRecord::Base end # TODO: this should be private @@ -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 diff --git a/lib/cancan/model_adapters/active_record_5_adapter.rb b/lib/cancan/model_adapters/active_record_5_adapter.rb new file mode 100644 index 00000000..94558921 --- /dev/null +++ b/lib/cancan/model_adapters/active_record_5_adapter.rb @@ -0,0 +1,70 @@ +module CanCan + module ModelAdapters + class ActiveRecord5Adapter < ActiveRecord4Adapter + AbstractAdapter.inherited(self) + + def self.for_class?(model_class) + ActiveRecord::VERSION::MAJOR == 5 && model_class <= ActiveRecord::Base + end + + # rails 5 is capable of using strings in enum + # but often people use symbols in rules + def self.matches_condition?(subject, name, value) + return super if Array.wrap(value).all? { |x| x.is_a? Integer } + + attribute = subject.send(name) + if value.is_a?(Enumerable) + value.map(&:to_s).include? attribute + else + attribute == value.to_s + 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 diff --git a/spec/cancan/model_adapters/active_record_4_adapter_spec.rb b/spec/cancan/model_adapters/active_record_4_adapter_spec.rb index 9016fdc1..0508b607 100644 --- a/spec/cancan/model_adapters/active_record_4_adapter_spec.rb +++ b/spec/cancan/model_adapters/active_record_4_adapter_spec.rb @@ -39,7 +39,7 @@ class Child < ActiveRecord::Base .to eq [child2, child1] end - if ActiveRecord::VERSION::MINOR >= 1 + if ActiveRecord::VERSION::MINOR >= 1 || ActiveRecord::VERSION::MAJOR >= 5 it 'allows filters on enums' do ActiveRecord::Schema.define do create_table(:shapes) do |t| @@ -48,7 +48,9 @@ class Child < ActiveRecord::Base end class Shape < ActiveRecord::Base - enum color: %i[red green blue] + unless defined_enums.keys.include? 'color' + enum color: %i[red green blue] + end end red = Shape.create!(color: :red) @@ -86,8 +88,8 @@ class Shape < ActiveRecord::Base end class Disc < ActiveRecord::Base - enum color: %i[red green blue] - enum shape: { triangle: 3, rectangle: 4 } + enum color: %i[red green blue] unless defined_enums.keys.include? 'color' + enum shape: { triangle: 3, rectangle: 4 } unless defined_enums.keys.include? 'shape' end red_triangle = Disc.create!(color: Disc.colors[:red], shape: Disc.shapes[:triangle]) diff --git a/spec/cancan/model_adapters/active_record_5_adapter_spec.rb b/spec/cancan/model_adapters/active_record_5_adapter_spec.rb new file mode 100644 index 00000000..de94a23b --- /dev/null +++ b/spec/cancan/model_adapters/active_record_5_adapter_spec.rb @@ -0,0 +1,100 @@ +require 'spec_helper' + +if ActiveRecord::VERSION::MAJOR == 5 && 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 '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 + unless defined_enums.keys.include? 'color' + enum color: %i[red green blue] + end + 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: :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: %i[red 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] unless defined_enums.keys.include? 'color' + enum shape: { triangle: 3, rectangle: 4 } unless defined_enums.keys.include? 'shape' + end + + red_triangle = Disc.create!(color: :red, shape: :triangle) + green_triangle = Disc.create!(color: :green, shape: :triangle) + green_rectangle = Disc.create!(color: :green, shape: :rectangle) + blue_rectangle = Disc.create!(color: :blue, shape: :rectangle) + + # A condition with a dual filter. + @ability.can :read, Disc, color: :green, shape: :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 + end +end diff --git a/spec/cancan/model_adapters/active_record_adapter_spec.rb b/spec/cancan/model_adapters/active_record_adapter_spec.rb index 17417b57..b50f2e2e 100644 --- a/spec/cancan/model_adapters/active_record_adapter_spec.rb +++ b/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::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::ActiveRecord5Adapter) + elsif ActiveRecord.respond_to?(:version) && + ActiveRecord.version > Gem::Version.new('4') 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) - expect(CanCan::ModelAdapters::AbstractAdapter.adapter_class(Article)) - .to eq(CanCan::ModelAdapters::ActiveRecord3Adapter) end end