diff --git a/Appraisals b/Appraisals index f7b89691a..6864259a3 100644 --- a/Appraisals +++ b/Appraisals @@ -1,55 +1,3 @@ -appraise 'activerecord_4.2.0' do - gem 'activerecord', '~> 4.2.0', require: 'active_record' - gem 'activesupport', '~> 4.2.0', require: 'active_support/all' - gem 'actionpack', '~> 4.2.0', require: 'action_pack' - gem 'nokogiri', '~> 1.6.8', require: 'nokogiri' # TODO: fix for ruby 2.0.0 - - gemfile.platforms :jruby do - gem 'activerecord-jdbcsqlite3-adapter', '~> 1.3.24' - gem 'jdbc-sqlite3' - gem 'jdbc-postgres' - end - - gemfile.platforms :ruby, :mswin, :mingw do - gem 'sqlite3', '~> 1.3.0' - gem 'pg', '~> 0.21' - end -end - -appraise 'activerecord_5.0.2' do - gem 'activerecord', '~> 5.0.2', require: 'active_record' - gem 'activesupport', '~> 5.0.2', require: 'active_support/all' - gem 'actionpack', '~> 5.0.2', require: 'action_pack' - - gemfile.platforms :jruby do - gem 'activerecord-jdbcsqlite3-adapter' - gem 'jdbc-sqlite3' - gem 'jdbc-postgres' - end - - gemfile.platforms :ruby, :mswin, :mingw do - gem 'sqlite3', '~> 1.3.0' - gem 'pg', '~> 0.21' - end -end - -appraise 'activerecord_5.1.0' do - gem 'activerecord', '~> 5.1.0', require: 'active_record' - gem 'activesupport', '~> 5.1.0', require: 'active_support/all' - gem 'actionpack', '~> 5.1.0', require: 'action_pack' - - gemfile.platforms :jruby do - gem 'activerecord-jdbcsqlite3-adapter' - gem 'jdbc-sqlite3' - gem 'jdbc-postgres' - end - - gemfile.platforms :ruby, :mswin, :mingw do - gem 'sqlite3', '~> 1.3.0' - gem 'pg', '~> 0.21' - end -end - appraise 'activerecord_5.2.2' do gem 'activerecord', '~> 5.2.2', require: 'active_record' gem 'activesupport', '~> 5.2.2', require: 'active_support/all' diff --git a/docs/Fetching-Records.md b/docs/Fetching-Records.md index 8f64115cc..f4d4f365b 100644 --- a/docs/Fetching-Records.md +++ b/docs/Fetching-Records.md @@ -8,7 +8,7 @@ Sometimes you need to restrict which records are returned from the database base You can change the action by passing it as the second argument. Here we find only the records the user has permission to update. ```ruby -@articles = Article.accessible_by(current_ability, :update) +@articles = Article.accessible_by(current_ability, :update) ``` If you want to use the current controller's action, make sure to call `to_sym` on it: @@ -17,7 +17,7 @@ If you want to use the current controller's action, make sure to call `to_sym` o @articles = Article.accessible_by(current_ability, params[:action].to_sym) ``` -This is an Active Record scope so other scopes and pagination can be chained onto it. +This is an Active Record scope so other scopes and pagination can be chained onto it. It can also be chained onto existing scopes. This works with multiple `can` definitions, which allows you to define complex permission logic and have it translated properly to SQL. @@ -29,7 +29,7 @@ class Ability can :manage, User, id: user.id end ``` -a call to User.accessible_by(current_ability) generates the following SQL +a call to User.accessible_by(current_ability) generates the following or equivalent SQL, depending on your version of ActiveRecord. ```sql SELECT * @@ -37,11 +37,11 @@ FROM users WHERE (id = 1) OR (not (self_managed = 't') AND (manager_id = 1)) ``` -It will raise an exception if any requested model's ability definition is defined using just block. +It will raise an exception if any requested model's ability definition is defined using just block. You can define SQL fragment in addition to block (look for more examples in [[Defining Abilities with Blocks]]). If you are using something other than Active Record you can fetch the conditions hash directly from the current ability. ```ruby current_ability.model_adapter(TargetClass, :read).conditions -``` \ No newline at end of file +``` diff --git a/lib/cancan.rb b/lib/cancan.rb index 80d7a24dc..730eeaf1f 100644 --- a/lib/cancan.rb +++ b/lib/cancan.rb @@ -19,6 +19,4 @@ require 'cancan/model_adapters/conditions_normalizer' require 'cancan/model_adapters/sti_normalizer' 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/ability.rb b/lib/cancan/ability.rb index 679ffc909..d0b6cb2ef 100644 --- a/lib/cancan/ability.rb +++ b/lib/cancan/ability.rb @@ -167,9 +167,10 @@ def validate_target(target) raise Error, error_message if aliased_actions.values.flatten.include? target end - def model_adapter(model_class, action) + def model_adapter(relation_or_class, action) + model_class = relation_or_class.respond_to?(:klass) ? relation_or_class.klass : relation_or_class adapter_class = ModelAdapters::AbstractAdapter.adapter_class(model_class) - adapter_class.new(model_class, relevant_rules_for_query(action, model_class)) + adapter_class.new(relation_or_class, relevant_rules_for_query(action, model_class)) end # See ControllerAdditions#authorize! for documentation. diff --git a/lib/cancan/config.rb b/lib/cancan/config.rb index a9106526b..cf31843fe 100644 --- a/lib/cancan/config.rb +++ b/lib/cancan/config.rb @@ -2,9 +2,7 @@ module CanCan def self.valid_accessible_by_strategies - strategies = [:left_join] - strategies << :subquery unless does_not_support_subquery_strategy? - strategies + %i[left_join subquery] end # Determines how CanCan should build queries when calling accessible_by, @@ -27,31 +25,18 @@ def self.accessible_by_strategy end def self.default_accessible_by_strategy - if does_not_support_subquery_strategy? - # 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 + # see https://github.com/CanCanCommunity/cancancan/pull/655 for where this was added + # the `subquery` strategy (from https://github.com/CanCanCommunity/cancancan/pull/619 + :left_join end def self.accessible_by_strategy=(value) - validate_accessible_by_strategy!(value) - - if value == :subquery && does_not_support_subquery_strategy? - raise ArgumentError, 'accessible_by_strategy = :subquery requires ActiveRecord 5 or newer' - end - @accessible_by_strategy = value end def self.with_accessible_by_strategy(value) return yield if value == accessible_by_strategy - validate_accessible_by_strategy!(value) - begin strategy_was = accessible_by_strategy @accessible_by_strategy = value @@ -60,15 +45,4 @@ def self.with_accessible_by_strategy(value) @accessible_by_strategy = strategy_was end end - - def self.validate_accessible_by_strategy!(value) - return if valid_accessible_by_strategies.include?(value) - - raise ArgumentError, "accessible_by_strategy must be one of #{valid_accessible_by_strategies.join(', ')}" - end - - def self.does_not_support_subquery_strategy? - !defined?(CanCan::ModelAdapters::ActiveRecordAdapter) || - CanCan::ModelAdapters::ActiveRecordAdapter.version_lower?('5.0.0') - end end diff --git a/lib/cancan/model_adapters/active_record_4_adapter.rb b/lib/cancan/model_adapters/active_record_4_adapter.rb deleted file mode 100644 index b2e9185fa..000000000 --- a/lib/cancan/model_adapters/active_record_4_adapter.rb +++ /dev/null @@ -1,62 +0,0 @@ -# frozen_string_literal: true - -module CanCan - module ModelAdapters - class ActiveRecord4Adapter < ActiveRecordAdapter - AbstractAdapter.inherited(self) - - class << self - def for_class?(model_class) - version_lower?('5.0.0') && model_class <= ActiveRecord::Base - end - - def override_condition_matching?(subject, name, _value) - subject.class.defined_enums.include?(name.to_s) - end - - def 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 - 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_joins_relation(relation, *_where_conditions) - relation.includes(joins).references(joins) - end - - # Rails 4.2 deprecates `sanitize_sql_hash_for_conditions` - def sanitize_sql(conditions) - if self.class.version_greater_or_equal?('4.2.0') && conditions.is_a?(Hash) - sanitize_sql_activerecord4(conditions) - else - @model_class.send(:sanitize_sql, conditions) - end - end - - def sanitize_sql_activerecord4(conditions) - table = Arel::Table.new(@model_class.send(:table_name)) - - conditions = ActiveRecord::PredicateBuilder.resolve_column_aliases @model_class, conditions - conditions = @model_class.send(:expand_hash_conditions_for_aggregates, conditions) - - ActiveRecord::PredicateBuilder.build_from_hash(@model_class, conditions, table).map do |b| - @model_class.send(:connection).visitor.compile b - end.join(' AND ') - 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 deleted file mode 100644 index 072ed7f50..000000000 --- a/lib/cancan/model_adapters/active_record_5_adapter.rb +++ /dev/null @@ -1,65 +0,0 @@ -# frozen_string_literal: true - -module CanCan - module ModelAdapters - class ActiveRecord5Adapter < ActiveRecord4Adapter - AbstractAdapter.inherited(self) - - def self.for_class?(model_class) - version_greater_or_equal?('5.0.0') && 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) - raw_attribute = subject.class.send(name.to_s.pluralize)[attribute] - !(Array(value).map(&:to_s) & [attribute, raw_attribute]).empty? - end - - private - - 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) - - when :left_join - relation.left_joins(joins).distinct - end - end - - 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) - - predicate_builder.build_from_hash(conditions.stringify_keys).map { |b| visit_nodes(b) }.join(' AND ') - end - - def visit_nodes(node) - # Rails 5.2 adds a BindParam node that prevents the visitor method from properly compiling the SQL query - if self.class.version_greater_or_equal?('5.2.0') - connection = @model_class.send(:connection) - collector = Arel::Collectors::SubstituteBinds.new(connection, Arel::Collectors::SQLString.new) - connection.visitor.accept(node, collector).value - else - @model_class.send(:connection).visitor.compile(node) - end - end - end - end -end diff --git a/lib/cancan/model_adapters/active_record_adapter.rb b/lib/cancan/model_adapters/active_record_adapter.rb index 7bd969c92..650d30d8b 100644 --- a/lib/cancan/model_adapters/active_record_adapter.rb +++ b/lib/cancan/model_adapters/active_record_adapter.rb @@ -3,68 +3,40 @@ module CanCan module ModelAdapters class ActiveRecordAdapter < AbstractAdapter - def self.version_greater_or_equal?(version) - Gem::Version.new(ActiveRecord.version).release >= Gem::Version.new(version) + def self.for_class?(klass) + klass < ActiveRecord::Base end - def self.version_lower?(version) - Gem::Version.new(ActiveRecord.version).release < Gem::Version.new(version) - end - - def initialize(model_class, rules) + def initialize(relation_or_class, rules) super + @model_class = relation_or_class.respond_to?(:klass) ? relation_or_class.klass : relation_or_class + @relation = relation_or_class.all @compressed_rules = RulesCompressor.new(@rules.reverse).rules_collapsed.reverse StiNormalizer.normalize(@compressed_rules) - ConditionsNormalizer.normalize(model_class, @compressed_rules) - end - - # Returns conditions intended to be used inside a database query. Normally you will not call this - # method directly, but instead go through ModelAdditions#accessible_by. - # - # If there is only one "can" definition, a hash of conditions will be returned matching the one defined. - # - # can :manage, User, :id => 1 - # query(:manage, User).conditions # => { :id => 1 } - # - # If there are multiple "can" definitions, a SQL string will be returned to handle complex cases. - # - # can :manage, User, :id => 1 - # can :manage, User, :manager_id => 1 - # cannot :manage, User, :self_managed => true - # query(:manage, User).conditions # => "not (self_managed = 't') AND ((manager_id = 1) OR (id = 1))" - # - def conditions - conditions_extractor = ConditionsExtractor.new(@model_class) - if @compressed_rules.size == 1 && @compressed_rules.first.base_behavior - # Return the conditions directly if there's just one definition - conditions_extractor.tableize_conditions(@compressed_rules.first.conditions).dup - else - extract_multiple_conditions(conditions_extractor, @compressed_rules) - end - end - - def extract_multiple_conditions(conditions_extractor, rules) - rules.reverse.inject(false_sql) do |sql, rule| - merge_conditions(sql, conditions_extractor.tableize_conditions(rule.conditions).dup, rule.base_behavior) + ConditionsNormalizer.normalize(@model_class, @compressed_rules) + # run the extractor on the reversed set of rules to fill the cache properly + @conditions_extractor = ConditionsExtractor.new(@model_class).tap do |extractor| + @compressed_rules.reverse_each { |rule| extractor.tableize_conditions(rule.conditions) } end end def database_records - if override_scope - @model_class.where(nil).merge(override_scope) - elsif @model_class.respond_to?(:where) && @model_class.respond_to?(:joins) - build_relation(conditions) + if @model_class.respond_to?(:where) && @model_class.respond_to?(:joins) + build_relation else @model_class.all(conditions: conditions, joins: joins) end end - def build_relation(*where_conditions) - relation = @model_class.where(*where_conditions) - return relation unless joins.present? + def build_relation + @build_relation ||= begin + return @model_class.none if @compressed_rules.empty? - # subclasses must implement `build_joins_relation` - build_joins_relation(relation, *where_conditions) + @relation = merge_positive_conditions + @relation = merge_negative_conditions + + build_joins_relation(@relation) + end end # Returns the associations used in conditions for the :joins option of a search. @@ -79,67 +51,77 @@ def joins private - # Removes empty hashes and moves everything into arrays. - def deep_clean(joins_hash) - joins_hash.map { |name, nested| nested.empty? ? name : { name => deep_clean(nested) } } - end - - # Takes two hashes and does a deep merge. - def deep_merge(base_hash, added_hash) - added_hash.each do |key, value| - if base_hash[key].is_a?(Hash) - deep_merge(base_hash[key], value) unless value.empty? - else - base_hash[key] = value - end + def rule_to_relation(rule) + if rule.conditions.is_a? ActiveRecord::Relation + rule.conditions + elsif rule.conditions.present? + @model_class.where(@conditions_extractor.tableize_conditions(rule.conditions)) end end - def override_scope - conditions = @compressed_rules.map(&:conditions).compact - return unless conditions.any? { |c| c.is_a?(ActiveRecord::Relation) } - return conditions.first if conditions.size == 1 - - raise_override_scope_error + def positive_conditions + @positive_conditions ||= begin + @compressed_rules + .select(&:base_behavior) + .map { |rule| rule_to_relation(rule) } + .compact + end end - def raise_override_scope_error - rule_found = @compressed_rules.detect { |rule| rule.conditions.is_a?(ActiveRecord::Relation) } - raise Error, - 'Unable to merge an Active Record scope with other conditions. '\ - "Instead use a hash or SQL for #{rule_found.actions.first} #{rule_found.subjects.first} ability." + def negative_conditions + @negative_conditions ||= begin + @compressed_rules + .reject(&:base_behavior) + .map { |rule| rule_to_relation(rule) } + .compact + end end - def merge_conditions(sql, conditions_hash, behavior) - if conditions_hash.blank? - behavior ? true_sql : false_sql + def merge_positive_conditions + if positive_conditions.present? + @relation.merge(positive_conditions.reduce(&:or)) else - merge_non_empty_conditions(behavior, conditions_hash, sql) + @relation end end - def merge_non_empty_conditions(behavior, conditions_hash, sql) - conditions = sanitize_sql(conditions_hash) - case sql - when true_sql - behavior ? true_sql : "not (#{conditions})" - when false_sql - behavior ? conditions : false_sql + def merge_negative_conditions + if negative_conditions.present? + @relation.where.not(negative_conditions.reduce(:or).where_clause.ast) else - behavior ? "(#{conditions}) OR (#{sql})" : "not (#{conditions}) AND (#{sql})" + @relation end end - def false_sql - sanitize_sql(['?=?', true, false]) + def build_joins_relation(relation) + return relation unless joins.present? + + case CanCan.accessible_by_strategy + when :subquery + inner = @model_class.unscoped do + @model_class.left_joins(joins).where(relation.where_clause.ast) + end + @model_class.where(@model_class.primary_key => inner) + + when :left_join + relation.left_joins(joins).distinct + end end - def true_sql - sanitize_sql(['?=?', true, true]) + # Removes empty hashes and moves everything into arrays. + def deep_clean(joins_hash) + joins_hash.map { |name, nested| nested.empty? ? name : { name => deep_clean(nested) } } end - def sanitize_sql(conditions) - @model_class.send(:sanitize_sql, conditions) + # Takes two hashes and does a deep merge. + def deep_merge(base_hash, added_hash) + added_hash.each do |key, value| + if base_hash[key].is_a?(Hash) + deep_merge(base_hash[key], value) unless value.empty? + else + base_hash[key] = value + end + end end end end diff --git a/lib/cancan/model_additions.rb b/lib/cancan/model_additions.rb index ef2d26eb3..74ba9b1db 100644 --- a/lib/cancan/model_additions.rb +++ b/lib/cancan/model_additions.rb @@ -22,7 +22,7 @@ module ClassMethods # Here only the articles which the user can update are returned. def accessible_by(ability, action = :index, strategy: CanCan.accessible_by_strategy) CanCan.with_accessible_by_strategy(strategy) do - ability.model_adapter(self, action).database_records + ability.model_adapter(all, action).database_records end end end diff --git a/spec/cancan/model_adapters/accessible_by_has_many_through_spec.rb b/spec/cancan/model_adapters/accessible_by_has_many_through_spec.rb index 3f5eb2124..d046151dc 100644 --- a/spec/cancan/model_adapters/accessible_by_has_many_through_spec.rb +++ b/spec/cancan/model_adapters/accessible_by_has_many_through_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' # integration tests for latest ActiveRecord version. -RSpec.describe CanCan::ModelAdapters::ActiveRecord5Adapter do +RSpec.describe CanCan::ModelAdapters::ActiveRecordAdapter do let(:ability) { double.extend(CanCan::Ability) } let(:users_table) { Post.table_name } let(:posts_table) { Post.table_name } @@ -86,12 +86,10 @@ class Editor < ActiveRecord::Base 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 + 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 @@ -104,16 +102,14 @@ class Editor < ActiveRecord::Base end end - unless CanCan::ModelAdapters::ActiveRecordAdapter.version_lower?('5.0.0') - describe 'filtering of results - subquery' do - before :each do - CanCan.accessible_by_strategy = :subquery - end + 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 + 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 @@ -122,11 +118,9 @@ class Editor < ActiveRecord::Base 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 + 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 it 'adds the where clause correctly without joins' do diff --git a/spec/cancan/model_adapters/accessible_by_integration_spec.rb b/spec/cancan/model_adapters/accessible_by_integration_spec.rb index 89f766591..126fc81a7 100644 --- a/spec/cancan/model_adapters/accessible_by_integration_spec.rb +++ b/spec/cancan/model_adapters/accessible_by_integration_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' # integration tests for latest ActiveRecord version. -RSpec.describe CanCan::ModelAdapters::ActiveRecord5Adapter do +RSpec.describe CanCan::ModelAdapters::ActiveRecordAdapter do let(:ability) { double.extend(CanCan::Ability) } let(:users_table) { Post.table_name } let(:posts_table) { Post.table_name } @@ -89,12 +89,10 @@ class Editor < ActiveRecord::Base 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 + 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 diff --git a/spec/cancan/model_adapters/active_record_4_adapter_spec.rb b/spec/cancan/model_adapters/active_record_4_adapter_spec.rb deleted file mode 100644 index f439db5be..000000000 --- a/spec/cancan/model_adapters/active_record_4_adapter_spec.rb +++ /dev/null @@ -1,157 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -if CanCan::ModelAdapters::ActiveRecordAdapter.version_lower?('5.0.0') - describe CanCan::ModelAdapters::ActiveRecord4Adapter do - # only the `left_join` strategy works in AR4 - CanCan.valid_accessible_by_strategies.each do |strategy| - context "with sqlite3 and #{strategy} strategy" do - before :each do - CanCan.accessible_by_strategy = strategy - - connect_db - 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 - - if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('4.1.0') - 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] unless defined_enums.key? 'color' - 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] unless defined_enums.key? 'color' - enum shape: { triangle: 3, rectangle: 4 } unless defined_enums.key? 'shape' - 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 - end - - context 'with postgresql' do - before :each do - connect_db - 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 diff --git a/spec/cancan/model_adapters/active_record_5_adapter_spec.rb b/spec/cancan/model_adapters/active_record_5_adapter_spec.rb deleted file mode 100644 index 877860071..000000000 --- a/spec/cancan/model_adapters/active_record_5_adapter_spec.rb +++ /dev/null @@ -1,165 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') - describe CanCan::ModelAdapters::ActiveRecord5Adapter do - CanCan.valid_accessible_by_strategies.each do |strategy| - context "with sqlite3 and #{strategy} strategy" do - before :each do - CanCan.accessible_by_strategy = strategy - - connect_db - ActiveRecord::Migration.verbose = false - - ActiveRecord::Schema.define do - create_table(:shapes) do |t| - t.integer :color, default: 0, null: false - end - - create_table(:things) do |t| - t.string :size, default: 'big', null: false - end - - create_table(:discs) do |t| - t.integer :color, default: 0, null: false - t.integer :shape, default: 3, null: false - end - end - - unless defined?(Thing) - class Thing < ActiveRecord::Base - enum size: { big: 'big', medium: 'average', small: 'small' } - end - end - - unless defined?(Shape) - class Shape < ActiveRecord::Base - enum color: %i[red green blue] - end - end - - unless defined?(Disc) - class Disc < ActiveRecord::Base - enum color: %i[red green blue] - enum shape: { triangle: 3, rectangle: 4 } - end - end - end - - subject(:ability) { Ability.new(nil) } - - context 'when enums use integers as values' do - let(:red) { Shape.create!(color: :red) } - let(:green) { Shape.create!(color: :green) } - let(:blue) { Shape.create!(color: :blue) } - - context 'when the condition contains a single value' do - before do - ability.can :read, Shape, color: :green - end - - it 'can check ability on single models' do - is_expected.not_to be_able_to(:read, red) - is_expected.to be_able_to(:read, green) - is_expected.not_to be_able_to(:read, blue) - end - - it 'can use accessible_by helper' do - accessible = Shape.accessible_by(ability) - expect(accessible).to contain_exactly(green) - end - end - - context 'when the condition contains multiple values' do - before do - ability.can :update, Shape, color: %i[red blue] - end - - it 'can check ability on single models' do - is_expected.to be_able_to(:update, red) - is_expected.not_to be_able_to(:update, green) - is_expected.to be_able_to(:update, blue) - end - - it 'can use accessible_by helper' do - accessible = Shape.accessible_by(ability, :update) - expect(accessible).to contain_exactly(red, blue) - end - end - end - - context 'when enums use strings as values' do - let(:big) { Thing.create!(size: :big) } - let(:medium) { Thing.create!(size: :medium) } - let(:small) { Thing.create!(size: :small) } - - context 'when the condition contains a single value' do - before do - ability.can :read, Thing, size: :medium - end - - it 'can check ability on single models' do - is_expected.not_to be_able_to(:read, big) - is_expected.to be_able_to(:read, medium) - is_expected.not_to be_able_to(:read, small) - end - - it 'can use accessible_by helper' do - expect(Thing.accessible_by(ability)).to contain_exactly(medium) - end - - context 'when a rule is overriden' do - before do - ability.cannot :read, Thing, size: 'average' - end - - it 'is recognised correctly' do - is_expected.not_to be_able_to(:read, medium) - expect(Thing.accessible_by(ability)).to be_empty - end - end - end - - context 'when the condition contains multiple values' do - before do - ability.can :update, Thing, size: %i[big small] - end - - it 'can check ability on single models' do - is_expected.to be_able_to(:update, big) - is_expected.not_to be_able_to(:update, medium) - is_expected.to be_able_to(:update, small) - end - - it 'can use accessible_by helper' do - expect(Thing.accessible_by(ability, :update)).to contain_exactly(big, small) - end - end - end - - context 'when multiple enums are present' do - let(:red_triangle) { Disc.create!(color: :red, shape: :triangle) } - let(:green_triangle) { Disc.create!(color: :green, shape: :triangle) } - let(:green_rectangle) { Disc.create!(color: :green, shape: :rectangle) } - let(:blue_rectangle) { Disc.create!(color: :blue, shape: :rectangle) } - - before do - ability.can :read, Disc, color: :green, shape: :rectangle - end - - it 'can check ability on single models' do - is_expected.not_to be_able_to(:read, red_triangle) - is_expected.not_to be_able_to(:read, green_triangle) - is_expected.to be_able_to(:read, green_rectangle) - is_expected.not_to be_able_to(:read, blue_rectangle) - end - - it 'can use accessible_by helper' do - expect(Disc.accessible_by(ability)).to contain_exactly(green_rectangle) - end - end - 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 2ac33756b..177ad3322 100644 --- a/spec/cancan/model_adapters/active_record_adapter_spec.rb +++ b/spec/cancan/model_adapters/active_record_adapter_spec.rb @@ -134,17 +134,8 @@ class User < ActiveRecord::Base end it 'is for only active record classes' do - if 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.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) - end + expect(CanCan::ModelAdapters::AbstractAdapter.adapter_class(Article)) + .to eq(CanCan::ModelAdapters::ActiveRecordAdapter) end it 'finds record' do @@ -261,12 +252,13 @@ class User < ActiveRecord::Base end it 'raises an exception when trying to merge scope with other conditions' do + published_article = Article.create!(published: true) + Article.create!(secret: true) + unpublished_not_secret_article = Article.create(published: false, secret: false) @ability.can :read, Article, published: true - @ability.can :read, Article, Article.where(secret: true) - expect(-> { Article.accessible_by(@ability) }) - .to raise_error(CanCan::Error, - 'Unable to merge an Active Record scope with other conditions. '\ - 'Instead use a hash or SQL for read Article ability.') + @ability.can :read, Article, Article.where(secret: false) + expect(-> { Article.accessible_by(@ability) }).not_to raise_error + expect(Article.accessible_by(@ability)).to match_array([published_article, unpublished_not_secret_article]) end it 'does not raise an exception when the rule with scope is suppressed' do @@ -303,13 +295,15 @@ class User < ActiveRecord::Base expect(-> { @ability.can? :read, Article.new }).to raise_error(CanCan::Error) end - it 'has false conditions if no abilities match' do - expect(@ability.model_adapter(Article, :read).conditions).to eq(false_condition) + it 'returns an empty scope if no abilities match' do + Article.create! + expect(@ability.model_adapter(Article, :read).database_records).to eq(Article.none) end it 'returns false conditions for cannot clause' do + Article.create! @ability.cannot :read, Article - expect(@ability.model_adapter(Article, :read).conditions).to eq(false_condition) + expect(@ability.model_adapter(Article, :read).database_records).to eq(Article.none) end it 'returns SQL for single `can` definition in front of default `cannot` condition' do @@ -324,57 +318,111 @@ class User < ActiveRecord::Base it 'returns true condition for single `can` definition in front of default `can` condition' do @ability.can :read, Article @ability.can :read, Article, published: false, secret: true - expect(@ability.model_adapter(Article, :read).conditions).to eq({}) expect(@ability.model_adapter(Article, :read)).to generate_sql(%(SELECT "articles".* FROM "articles")) end it 'returns `false condition` for single `cannot` definition in front of default `cannot` condition' do @ability.cannot :read, Article @ability.cannot :read, Article, published: false, secret: true - expect(@ability.model_adapter(Article, :read).conditions).to eq(false_condition) + expect(@ability.model_adapter(Article, :read).database_records).to eq(Article.none) end it 'returns `not (sql)` for single `cannot` definition in front of default `can` condition' do @ability.can :read, Article @ability.cannot :read, Article, published: false, secret: true - expect(@ability.model_adapter(Article, :read).conditions) - .to orderlessly_match( - %["not (#{@article_table}"."published" = #{false_v} AND "#{@article_table}"."secret" = #{true_v})] + expect(@ability.model_adapter(Article, :read)) + .to generate_sql( + <<-SQL + SELECT "#{@article_table}".* FROM "#{@article_table}" WHERE NOT ("#{@article_table}"."published" = #{false_v} AND "articles"."secret" = #{true_v}) + SQL ) end - it 'returns appropriate sql conditions in complex case' do + it 'returns appropriate records in complex case' do + priority1 = Article.create!(priority: 1, secret: false) + Article.create!(published: false, secret: false) + public_article = Article.create!(published: true, secret: false) + Article.create!(secret: true) @ability.can :read, Article - @ability.can :manage, Article, id: 1 + @ability.can :manage, Article, priority: 1 @ability.can :update, Article, published: true @ability.cannot :update, Article, secret: true - expect(@ability.model_adapter(Article, :update).conditions) - .to eq(%[not ("#{@article_table}"."secret" = #{true_v}) ] + - %[AND (("#{@article_table}"."published" = #{true_v}) ] + - %[OR ("#{@article_table}"."id" = 1))]) - expect(@ability.model_adapter(Article, :manage).conditions).to eq(id: 1) - expect(@ability.model_adapter(Article, :read).conditions).to eq({}) + expect(@ability.model_adapter(Article, :update).database_records).to match_array( + [ + priority1, + public_article + ] + ) expect(@ability.model_adapter(Article, :read)).to generate_sql(%(SELECT "articles".* FROM "articles")) + expect(@ability.model_adapter(Article, :manage)).to generate_sql( + %(SELECT "articles".* FROM "articles" WHERE "articles"."priority" = 1) + ) end it 'returns appropriate sql conditions in complex case with nested joins' do @ability.can :read, Comment, article: { category: { visible: true } } - expect(@ability.model_adapter(Comment, :read).conditions).to eq(Category.table_name.to_sym => { visible: true }) + if CanCan.accessible_by_strategy == :left_join + expect(@ability.model_adapter(Comment, :read)).to generate_sql( + <<-SQL + SELECT DISTINCT "comments".* FROM "comments" + LEFT OUTER JOIN "articles" ON "articles"."id" = "comments"."article_id" + LEFT OUTER JOIN "categories" ON "categories"."id" = "articles"."category_id" + WHERE "categories"."visible" = #{true_v} + SQL + ) + else + expect(@ability.model_adapter(Comment, :read)).to generate_sql( + <<-SQL + SELECT "comments".* FROM "comments" WHERE "comments"."id" IN + (SELECT "comments"."id" FROM "comments" + LEFT OUTER JOIN "articles" ON "articles"."id" = "comments"."article_id" + LEFT OUTER JOIN "categories" ON "categories"."id" = "articles"."category_id" + WHERE "categories"."visible" = #{true_v}) + SQL + ) + end end it 'returns appropriate sql conditions in complex case with nested joins of different depth' do @ability.can :read, Comment, article: { published: true, category: { visible: true } } - expect(@ability.model_adapter(Comment, :read).conditions) - .to eq(Article.table_name.to_sym => { published: true }, Category.table_name.to_sym => { visible: true }) + if CanCan.accessible_by_strategy == :left_join + expect(@ability.model_adapter(Comment, :read)).to generate_sql( + <<-SQL + SELECT DISTINCT "comments".* FROM "comments" + LEFT OUTER JOIN "articles" ON "articles"."id" = "comments"."article_id" + LEFT OUTER JOIN "categories" ON "categories"."id" = "articles"."category_id" + WHERE "articles"."published" = #{true_v} AND "categories"."visible" = #{true_v} + SQL + ) + else + expect(@ability.model_adapter(Comment, :read)).to generate_sql( + <<-SQL + SELECT "comments".* FROM "comments" WHERE "comments"."id" IN + (SELECT "comments"."id" FROM "comments" + LEFT OUTER JOIN "articles" ON "articles"."id" = "comments"."article_id" + LEFT OUTER JOIN "categories" ON "categories"."id" = "articles"."category_id" + WHERE "articles"."published" = #{true_v} AND "categories"."visible" = #{true_v}) + SQL + ) + end end it 'does not forget conditions when calling with SQL string' do + priority1 = Article.create!(priority: 1, secret: false) + unpublished_not_secret_article = Article.create!(published: false, secret: false) + public_article = Article.create!(published: true, secret: false) + Article.create!(secret: true) + @ability.can :read, Article, published: true @ability.can :read, Article, ['secret = ?', false] - adapter = @ability.model_adapter(Article, :read) - 2.times do - expect(adapter.conditions).to eq(%[(secret = #{false_v}) OR ("#{@article_table}"."published" = #{true_v})]) - end + + expect(@ability.model_adapter(Article, :read).database_records).to match_array( + [ + priority1, + unpublished_not_secret_article, + public_article + ] + ) end it 'has nil joins if no rules' do @@ -444,32 +492,20 @@ class User < ActiveRecord::Base end end - unless CanCan::ModelAdapters::ActiveRecordAdapter.version_lower?('5.0.0') - context 'base behaviour subquery specific' do - before :each do - CanCan.accessible_by_strategy = :subquery - end - - it 'allows ordering via relations' do - @ability.can :read, Comment, article: { category: { visible: true } } - comment1 = Comment.create!(article: Article.create!(name: 'B', category: Category.create!(visible: true))) - comment2 = Comment.create!(article: Article.create!(name: 'A', category: Category.create!(visible: true))) - Comment.create!(article: Article.create!(category: Category.create!(visible: false))) + context 'base behaviour subquery specific' do + before :each do + CanCan.accessible_by_strategy = :subquery + end - # doesn't work without explicitly calling a join on AR 5+, - # but does before that (where we don't use subqueries at all) - if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') - expect { Comment.accessible_by(@ability).order('articles.name').to_a } - .to raise_error(ActiveRecord::StatementInvalid) - else - expect(Comment.accessible_by(@ability).order('articles.name')) - .to match_array([comment2, comment1]) - end + it 'allows ordering via relations' do + @ability.can :read, Comment, article: { category: { visible: true } } + comment1 = Comment.create!(article: Article.create!(name: 'B', category: Category.create!(visible: true))) + comment2 = Comment.create!(article: Article.create!(name: 'A', category: Category.create!(visible: true))) + Comment.create!(article: Article.create!(category: Category.create!(visible: false))) - # works with the explicit join - expect(Comment.accessible_by(@ability).joins(:article).order('articles.name')) - .to match_array([comment2, comment1]) - end + # works with the explicit join + expect(Comment.accessible_by(@ability).joins(:article).order('articles.name')) + .to match_array([comment2, comment1]) end end @@ -490,16 +526,8 @@ class User < ActiveRecord::Base expect(Comment.accessible_by(@ability).order('articles.name')).to match_array([comment2, comment1]) # works with the explicit join in AR 5.2+ and AR 4.2 - if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.2.0') - expect(Comment.accessible_by(@ability).joins(:article).order('articles.name')) - .to match_array([comment2, comment1]) - elsif CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') - expect { Comment.accessible_by(@ability).joins(:article).order('articles.name').to_a } - .to raise_error(ActiveRecord::StatementInvalid) - else - expect(Comment.accessible_by(@ability).joins(:article).order('articles.name')) - .to match_array([comment2, comment1]) - end + expect(Comment.accessible_by(@ability).joins(:article).order('articles.name')) + .to match_array([comment2, comment1]) end # this fails on Postgres. see https://github.com/CanCanCommunity/cancancan/pull/608 @@ -511,18 +539,10 @@ class User < ActiveRecord::Base comment2 = Comment.create!(article: Article.create!(name: 'A', category: Category.create!(visible: true))) Comment.create!(article: Article.create!(category: Category.create!(visible: false))) - if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') - # doesn't work with or without the join - expect { Comment.accessible_by(@ability).order('articles.name').to_a } - .to raise_error(ActiveRecord::StatementInvalid) - expect { Comment.accessible_by(@ability).joins(:article).order('articles.name').to_a } - .to raise_error(ActiveRecord::StatementInvalid) - else - expect(Comment.accessible_by(@ability).order('articles.name')) - .to match_array([comment2, comment1]) - expect(Comment.accessible_by(@ability).joins(:article).order('articles.name')) - .to match_array([comment2, comment1]) - end + expect(Comment.accessible_by(@ability).select('"comments".*, "articles"."name"').order('articles.name')) + .to match_array([comment2, comment1]) + expect(Comment.accessible_by(@ability).select('"comments".*, "articles"."name"').order('articles.name')) + .to match_array([comment2, comment1]) end end @@ -756,36 +776,32 @@ class Transaction < ActiveRecord::Base end end - unless CanCan::ModelAdapters::ActiveRecordAdapter.version_lower?('5.0.0') - context 'has_many through is defined and referenced differently - subquery strategy' do - before do - CanCan.accessible_by_strategy = :subquery - end + context 'has_many through is defined and referenced differently - subquery strategy' do + before do + CanCan.accessible_by_strategy = :subquery + end - it 'recognises it and simplifies the query' do - u1 = User.create!(name: 'pippo') - u2 = User.create!(name: 'paperino') + it 'recognises it and simplifies the query' do + u1 = User.create!(name: 'pippo') + u2 = User.create!(name: 'paperino') - a1 = Article.create!(mentioned_users: [u1]) - a2 = Article.create!(mentioned_users: [u2]) + a1 = Article.create!(mentioned_users: [u1]) + a2 = Article.create!(mentioned_users: [u2]) - ability = Ability.new(u1) - ability.can :read, Article, mentioned_users: { name: u1.name } - ability.can :read, Article, mentions: { user: { name: u2.name } } - expect(Article.accessible_by(ability)).to match_array([a1, a2]) - if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') - expect(ability.model_adapter(Article, :read)).to generate_sql(%( - SELECT "articles".* - FROM "articles" - WHERE "articles"."id" IN - (SELECT "articles"."id" - FROM "articles" - LEFT OUTER JOIN "legacy_mentions" ON "legacy_mentions"."article_id" = "articles"."id" - LEFT OUTER JOIN "users" ON "users"."id" = "legacy_mentions"."user_id" - WHERE (("users"."name" = 'paperino') OR ("users"."name" = 'pippo'))) - )) - end - end + ability = Ability.new(u1) + ability.can :read, Article, mentioned_users: { name: u1.name } + ability.can :read, Article, mentions: { user: { name: u2.name } } + expect(Article.accessible_by(ability)).to match_array([a1, a2]) + expect(ability.model_adapter(Article, :read)).to generate_sql(%( +SELECT "articles".* +FROM "articles" +WHERE "articles"."id" IN +(SELECT "articles"."id" + FROM "articles" + LEFT OUTER JOIN "legacy_mentions" ON "legacy_mentions"."article_id" = "articles"."id" + LEFT OUTER JOIN "users" ON "users"."id" = "legacy_mentions"."user_id" + WHERE ("users"."name" = 'paperino' OR "users"."name" = 'pippo')) +)) end end @@ -806,109 +822,105 @@ class Transaction < ActiveRecord::Base ability.can :read, Article, mentions: { user: { name: u2.name } } expect(Article.accessible_by(ability)).to match_array([a1, a2]) - if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') - expect(ability.model_adapter(Article, :read)).to generate_sql(%( - SELECT DISTINCT "articles".* - FROM "articles" - LEFT OUTER JOIN "legacy_mentions" ON "legacy_mentions"."article_id" = "articles"."id" - LEFT OUTER JOIN "users" ON "users"."id" = "legacy_mentions"."user_id" - WHERE (("users"."name" = 'paperino') OR ("users"."name" = 'pippo')))) - end + expect(ability.model_adapter(Article, :read)).to generate_sql(%( +SELECT DISTINCT "articles".* +FROM "articles" +LEFT OUTER JOIN "legacy_mentions" ON "legacy_mentions"."article_id" = "articles"."id" +LEFT OUTER JOIN "users" ON "users"."id" = "legacy_mentions"."user_id" +WHERE ("users"."name" = 'paperino' OR "users"."name" = 'pippo'))) end end - if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') - context 'switching strategies' do - before do - CanCan.accessible_by_strategy = :left_join # default - should be ignored in these tests - end + context 'switching strategies' do + before do + CanCan.accessible_by_strategy = :left_join # default - should be ignored in these tests + end - it 'allows you to switch strategies with a keyword argument' do - u = User.create!(name: 'pippo') - Article.create!(mentioned_users: [u]) + it 'allows you to switch strategies with a keyword argument' do + u = User.create!(name: 'pippo') + Article.create!(mentioned_users: [u]) - ability = Ability.new(u) - ability.can :read, Article, mentions: { user: { name: u.name } } + ability = Ability.new(u) + ability.can :read, Article, mentions: { user: { name: u.name } } - subquery_sql = Article.accessible_by(ability, strategy: :subquery).to_sql - left_join_sql = Article.accessible_by(ability, strategy: :left_join).to_sql + subquery_sql = Article.accessible_by(ability, strategy: :subquery).to_sql + left_join_sql = Article.accessible_by(ability, strategy: :left_join).to_sql - expect(subquery_sql.strip.squeeze(' ')).to eq(%( - SELECT "articles".* - FROM "articles" - WHERE "articles"."id" IN - (SELECT "articles"."id" - FROM "articles" - LEFT OUTER JOIN "legacy_mentions" ON "legacy_mentions"."article_id" = "articles"."id" - LEFT OUTER JOIN "users" ON "users"."id" = "legacy_mentions"."user_id" - WHERE "users"."name" = 'pippo') - ).gsub(/\s+/, ' ').strip) - - expect(left_join_sql.strip.squeeze(' ')).to eq(%( - SELECT DISTINCT "articles".* + expect(subquery_sql.strip.squeeze(' ')).to eq(%( + SELECT "articles".* FROM "articles" - LEFT OUTER JOIN "legacy_mentions" ON "legacy_mentions"."article_id" = "articles"."id" - LEFT OUTER JOIN "users" ON "users"."id" = "legacy_mentions"."user_id" - WHERE "users"."name" = 'pippo').gsub(/\s+/, ' ').strip) - end + WHERE "articles"."id" IN + (SELECT "articles"."id" + FROM "articles" + LEFT OUTER JOIN "legacy_mentions" ON "legacy_mentions"."article_id" = "articles"."id" + LEFT OUTER JOIN "users" ON "users"."id" = "legacy_mentions"."user_id" + WHERE "users"."name" = 'pippo') + ).gsub(/\s+/, ' ').strip) + + expect(left_join_sql.strip.squeeze(' ')).to eq(%( +SELECT DISTINCT "articles".* +FROM "articles" +LEFT OUTER JOIN "legacy_mentions" ON "legacy_mentions"."article_id" = "articles"."id" +LEFT OUTER JOIN "users" ON "users"."id" = "legacy_mentions"."user_id" +WHERE "users"."name" = 'pippo').gsub(/\s+/, ' ').strip) + end - it 'allows you to switch strategies with a block' do - u = User.create!(name: 'pippo') - Article.create!(mentioned_users: [u]) + it 'allows you to switch strategies with a block' do + u = User.create!(name: 'pippo') + Article.create!(mentioned_users: [u]) - ability = Ability.new(u) - ability.can :read, Article, mentions: { user: { name: u.name } } + ability = Ability.new(u) + ability.can :read, Article, mentions: { user: { name: u.name } } - subquery_sql = CanCan.with_accessible_by_strategy(:subquery) { Article.accessible_by(ability).to_sql } - left_join_sql = CanCan.with_accessible_by_strategy(:left_join) { Article.accessible_by(ability).to_sql } + subquery_sql = CanCan.with_accessible_by_strategy(:subquery) { Article.accessible_by(ability).to_sql } + left_join_sql = CanCan.with_accessible_by_strategy(:left_join) { Article.accessible_by(ability).to_sql } - expect(subquery_sql.strip.squeeze(' ')).to eq(%( - SELECT "articles".* - FROM "articles" - WHERE "articles"."id" IN - (SELECT "articles"."id" - FROM "articles" - LEFT OUTER JOIN "legacy_mentions" ON "legacy_mentions"."article_id" = "articles"."id" - LEFT OUTER JOIN "users" ON "users"."id" = "legacy_mentions"."user_id" - WHERE "users"."name" = 'pippo') - ).gsub(/\s+/, ' ').strip) - - expect(left_join_sql.strip.squeeze(' ')).to eq(%( - SELECT DISTINCT "articles".* + expect(subquery_sql.strip.squeeze(' ')).to eq(%( + SELECT "articles".* FROM "articles" - LEFT OUTER JOIN "legacy_mentions" ON "legacy_mentions"."article_id" = "articles"."id" - LEFT OUTER JOIN "users" ON "users"."id" = "legacy_mentions"."user_id" - WHERE "users"."name" = 'pippo').gsub(/\s+/, ' ').strip) - end + WHERE "articles"."id" IN + (SELECT "articles"."id" + FROM "articles" + LEFT OUTER JOIN "legacy_mentions" ON "legacy_mentions"."article_id" = "articles"."id" + LEFT OUTER JOIN "users" ON "users"."id" = "legacy_mentions"."user_id" + WHERE "users"."name" = 'pippo') + ).gsub(/\s+/, ' ').strip) + + expect(left_join_sql.strip.squeeze(' ')).to eq(%( +SELECT DISTINCT "articles".* +FROM "articles" +LEFT OUTER JOIN "legacy_mentions" ON "legacy_mentions"."article_id" = "articles"."id" +LEFT OUTER JOIN "users" ON "users"."id" = "legacy_mentions"."user_id" +WHERE "users"."name" = 'pippo').gsub(/\s+/, ' ').strip) + end - it 'allows you to switch strategies with a block, and to_sql called outside the block' do - u = User.create!(name: 'pippo') - Article.create!(mentioned_users: [u]) + it 'allows you to switch strategies with a block, and to_sql called outside the block' do + u = User.create!(name: 'pippo') + Article.create!(mentioned_users: [u]) - ability = Ability.new(u) - ability.can :read, Article, mentions: { user: { name: u.name } } + ability = Ability.new(u) + ability.can :read, Article, mentions: { user: { name: u.name } } - subquery_sql = CanCan.with_accessible_by_strategy(:subquery) { Article.accessible_by(ability) }.to_sql - left_join_sql = CanCan.with_accessible_by_strategy(:left_join) { Article.accessible_by(ability) }.to_sql + subquery_sql = CanCan.with_accessible_by_strategy(:subquery) { Article.accessible_by(ability) }.to_sql + left_join_sql = CanCan.with_accessible_by_strategy(:left_join) { Article.accessible_by(ability) }.to_sql - expect(subquery_sql.strip.squeeze(' ')).to eq(%( - SELECT "articles".* - FROM "articles" - WHERE "articles"."id" IN - (SELECT "articles"."id" - FROM "articles" - LEFT OUTER JOIN "legacy_mentions" ON "legacy_mentions"."article_id" = "articles"."id" - LEFT OUTER JOIN "users" ON "users"."id" = "legacy_mentions"."user_id" - WHERE "users"."name" = 'pippo') - ).gsub(/\s+/, ' ').strip) - - expect(left_join_sql.strip.squeeze(' ')).to eq(%( - SELECT DISTINCT "articles".* + expect(subquery_sql.strip.squeeze(' ')).to eq(%( + SELECT "articles".* FROM "articles" - LEFT OUTER JOIN "legacy_mentions" ON "legacy_mentions"."article_id" = "articles"."id" - LEFT OUTER JOIN "users" ON "users"."id" = "legacy_mentions"."user_id" - WHERE "users"."name" = 'pippo').gsub(/\s+/, ' ').strip) - end + WHERE "articles"."id" IN + (SELECT "articles"."id" + FROM "articles" + LEFT OUTER JOIN "legacy_mentions" ON "legacy_mentions"."article_id" = "articles"."id" + LEFT OUTER JOIN "users" ON "users"."id" = "legacy_mentions"."user_id" + WHERE "users"."name" = 'pippo') + ).gsub(/\s+/, ' ').strip) + + expect(left_join_sql.strip.squeeze(' ')).to eq(%( +SELECT DISTINCT "articles".* +FROM "articles" +LEFT OUTER JOIN "legacy_mentions" ON "legacy_mentions"."article_id" = "articles"."id" +LEFT OUTER JOIN "users" ON "users"."id" = "legacy_mentions"."user_id" +WHERE "users"."name" = 'pippo').gsub(/\s+/, ' ').strip) end end diff --git a/spec/cancan/model_adapters/has_and_belongs_to_many_spec.rb b/spec/cancan/model_adapters/has_and_belongs_to_many_spec.rb index 0d112e9e0..44a230733 100644 --- a/spec/cancan/model_adapters/has_and_belongs_to_many_spec.rb +++ b/spec/cancan/model_adapters/has_and_belongs_to_many_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -RSpec.describe CanCan::ModelAdapters::ActiveRecord5Adapter do +RSpec.describe CanCan::ModelAdapters::ActiveRecordAdapter do let(:ability) { double.extend(CanCan::Ability) } let(:users_table) { User.table_name } let(:posts_table) { Post.table_name } @@ -45,31 +45,27 @@ class House < ActiveRecord::Base ability.can :read, House, people: { id: @person1.id } end - unless CanCan::ModelAdapters::ActiveRecordAdapter.version_lower?('5.0.0') - describe 'fetching of records - subquery strategy' do - before do - CanCan.accessible_by_strategy = :subquery - end + describe 'fetching of records - subquery strategy' do + before do + CanCan.accessible_by_strategy = :subquery + end - it 'it retreives the records correctly' do - houses = House.accessible_by(ability) - expect(houses).to match_array [@house2, @house1] - end + it 'it retreives the records correctly' do + houses = House.accessible_by(ability) + expect(houses).to match_array [@house2, @house1] + end - if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') - it 'generates the correct query' do - expect(ability.model_adapter(House, :read)) - .to generate_sql("SELECT \"houses\".* - FROM \"houses\" - WHERE \"houses\".\"id\" IN - (SELECT \"houses\".\"id\" - FROM \"houses\" - LEFT OUTER JOIN \"houses_people\" ON \"houses_people\".\"house_id\" = \"houses\".\"id\" - LEFT OUTER JOIN \"people\" ON \"people\".\"id\" = \"houses_people\".\"person_id\" - WHERE \"people\".\"id\" = #{@person1.id}) - ") - end - end + it 'generates the correct query' do + expect(ability.model_adapter(House, :read)) + .to generate_sql("SELECT \"houses\".* + FROM \"houses\" + WHERE \"houses\".\"id\" IN + (SELECT \"houses\".\"id\" + FROM \"houses\" + LEFT OUTER JOIN \"houses_people\" ON \"houses_people\".\"house_id\" = \"houses\".\"id\" + LEFT OUTER JOIN \"people\" ON \"people\".\"id\" = \"houses_people\".\"person_id\" + WHERE \"people\".\"id\" = #{@person1.id}) + ") end end @@ -83,15 +79,13 @@ class House < ActiveRecord::Base expect(houses).to match_array [@house2, @house1] end - if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') - it 'generates the correct query' do - expect(ability.model_adapter(House, :read)).to generate_sql(%( - SELECT DISTINCT "houses".* - FROM "houses" - LEFT OUTER JOIN "houses_people" ON "houses_people"."house_id" = "houses"."id" - LEFT OUTER JOIN "people" ON "people"."id" = "houses_people"."person_id" - WHERE "people"."id" = #{@person1.id})) - end + it 'generates the correct query' do + expect(ability.model_adapter(House, :read)).to generate_sql(%( + SELECT DISTINCT "houses".* + FROM "houses" + LEFT OUTER JOIN "houses_people" ON "houses_people"."house_id" = "houses"."id" + LEFT OUTER JOIN "people" ON "people"."id" = "houses_people"."person_id" + WHERE "people"."id" = #{@person1.id})) end end end