From 3c17def3a058c7f20175388998c82690d6eebe6d Mon Sep 17 00:00:00 2001 From: Alex Ghiculescu Date: Mon, 9 Nov 2020 13:49:38 -0600 Subject: [PATCH] accessible_by_strategy only works for Rails 5+ --- lib/cancan/config.rb | 30 ++++-- .../model_adapters/active_record_4_adapter.rb | 9 +- .../accessible_by_has_many_through_spec.rb | 18 ++-- .../active_record_4_adapter_spec.rb | 3 +- .../active_record_5_adapter_spec.rb | 2 +- .../active_record_adapter_spec.rb | 96 ++++++++++--------- .../has_and_belongs_to_many_spec.rb | 40 ++++---- spec/spec_helper.rb | 3 +- 8 files changed, 109 insertions(+), 92 deletions(-) diff --git a/lib/cancan/config.rb b/lib/cancan/config.rb index 9334226f..74f7253a 100644 --- a/lib/cancan/config.rb +++ b/lib/cancan/config.rb @@ -1,10 +1,11 @@ # frozen_string_literal: true module CanCan - VALID_ACCESSIBLE_BY_STRATEGIES = %i[ - subquery - left_join - ].freeze + 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`. @@ -20,12 +21,27 @@ module CanCan # `distinct` is not reliable in some cases. See # https://github.com/CanCanCommunity/cancancan/pull/605 def self.accessible_by_strategy - @accessible_by_strategy || :subquery + @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(', ')}" + 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 diff --git a/lib/cancan/model_adapters/active_record_4_adapter.rb b/lib/cancan/model_adapters/active_record_4_adapter.rb index b4461950..b2e9185f 100644 --- a/lib/cancan/model_adapters/active_record_4_adapter.rb +++ b/lib/cancan/model_adapters/active_record_4_adapter.rb @@ -35,14 +35,7 @@ def matches_condition?(subject, name, value) # 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) - case CanCan.accessible_by_strategy - when :subquery - # subquery mode doesn't work with Rails 4.x - relation.includes(joins).references(joins) - - when :left_join - relation.includes(joins).references(joins) - end + relation.includes(joins).references(joins) end # Rails 4.2 deprecates `sanitize_sql_hash_for_conditions` 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 da22e6b1..3f5eb212 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 @@ -72,7 +72,7 @@ class Editor < ActiveRecord::Base ability.can :read, Post, editors: { user_id: @user1 } end - CanCan::VALID_ACCESSIBLE_BY_STRATEGIES.each do |strategy| + CanCan.valid_accessible_by_strategies.each do |strategy| context "using #{strategy} strategy" do before :each do CanCan.accessible_by_strategy = strategy @@ -104,14 +104,16 @@ class Editor < ActiveRecord::Base end end - describe 'filtering of results - subquery' do - before :each do - CanCan.accessible_by_strategy = :subquery - 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 - 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 + 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 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 59b3977d..f439db5b 100644 --- a/spec/cancan/model_adapters/active_record_4_adapter_spec.rb +++ b/spec/cancan/model_adapters/active_record_4_adapter_spec.rb @@ -4,7 +4,8 @@ if CanCan::ModelAdapters::ActiveRecordAdapter.version_lower?('5.0.0') describe CanCan::ModelAdapters::ActiveRecord4Adapter do - CanCan::VALID_ACCESSIBLE_BY_STRATEGIES.each do |strategy| + # 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 diff --git a/spec/cancan/model_adapters/active_record_5_adapter_spec.rb b/spec/cancan/model_adapters/active_record_5_adapter_spec.rb index 0b7450b9..87786007 100644 --- a/spec/cancan/model_adapters/active_record_5_adapter_spec.rb +++ b/spec/cancan/model_adapters/active_record_5_adapter_spec.rb @@ -4,7 +4,7 @@ if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') describe CanCan::ModelAdapters::ActiveRecord5Adapter do - CanCan::VALID_ACCESSIBLE_BY_STRATEGIES.each do |strategy| + CanCan.valid_accessible_by_strategies.each do |strategy| context "with sqlite3 and #{strategy} strategy" do before :each do CanCan.accessible_by_strategy = strategy diff --git a/spec/cancan/model_adapters/active_record_adapter_spec.rb b/spec/cancan/model_adapters/active_record_adapter_spec.rb index a29ca57c..e53227ce 100644 --- a/spec/cancan/model_adapters/active_record_adapter_spec.rb +++ b/spec/cancan/model_adapters/active_record_adapter_spec.rb @@ -109,7 +109,7 @@ class User < ActiveRecord::Base @comment_table = Comment.table_name end - CanCan::VALID_ACCESSIBLE_BY_STRATEGIES.each do |strategy| + CanCan.valid_accessible_by_strategies.each do |strategy| context "base functionality with #{strategy} strategy" do before :each do CanCan.accessible_by_strategy = strategy @@ -444,30 +444,32 @@ class User < ActiveRecord::Base end end - context 'base behaviour subquery specific' do - before :each do - CanCan.accessible_by_strategy = :subquery - 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))) + 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))) - # 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')) + # 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 + + # 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 @@ -612,7 +614,7 @@ class Transaction < ActiveRecord::Base end end - CanCan::VALID_ACCESSIBLE_BY_STRATEGIES.each do |strategy| + CanCan.valid_accessible_by_strategies.each do |strategy| context "when a table is referenced multiple times with #{strategy} strategy" do before :each do CanCan.accessible_by_strategy = strategy @@ -635,33 +637,35 @@ class Transaction < ActiveRecord::Base end end - context 'has_many through is defined and referenced differently - subquery strategy' do - before do - CanCan.accessible_by_strategy = :subquery - 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 - 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" + 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" - 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'))) - )) + 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 end end @@ -694,7 +698,7 @@ class Transaction < ActiveRecord::Base end end - CanCan::VALID_ACCESSIBLE_BY_STRATEGIES.each do |strategy| + CanCan.valid_accessible_by_strategies.each do |strategy| context "when a model has renamed primary_key with #{strategy} strategy" do before :each do CanCan.accessible_by_strategy = strategy 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 5d233a7c..0d112e9e 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 @@ -45,28 +45,30 @@ class House < ActiveRecord::Base ability.can :read, House, people: { id: @person1.id } end - describe 'fetching of records - subquery strategy' do - before do - CanCan.accessible_by_strategy = :subquery - 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 - 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\" + 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\" - 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}) - ") + 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 end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 09038fea..7f2a4ab8 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -29,8 +29,7 @@ config.include SQLHelpers config.after :each do - # set default values for all config - CanCan.accessible_by_strategy = :subquery + CanCan.accessible_by_strategy = CanCan.default_accessible_by_strategy end end