Skip to content

Commit

Permalink
Accessible by strategy: Exists subquery (#691)
Browse files Browse the repository at this point in the history
* Added the new strategy: double_exist_subquery

* Accessible by strategy: Each rule as exists subquery

* Use aliased joined table instead of double sub query

* Solved offences and specifically solved complexity offences by extracting code out into separate strategy classes

* Extracted existing strategies out into separate classes as well

* Fixed more offences and cleaned up

* Removed unused file

* Fixed spec

* Fixed specs

* Select column through table name

* Fixed more specs

* Fixed offences

* Fixed SQL in spec

* Unscoped might remove existing settings on the query

* Limit results to 1

* Added limit in spec as well

* Fixed SQL

* Just use JOIN both places

* Fixed several issues after merging with latest version of develop
  • Loading branch information
kaspernj authored Mar 23, 2022
1 parent 40bb117 commit 24ed063
Show file tree
Hide file tree
Showing 6 changed files with 184 additions and 2 deletions.
2 changes: 2 additions & 0 deletions lib/cancan.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
require 'cancan/model_adapters/active_record_4_adapter'
require 'cancan/model_adapters/active_record_5_adapter'
require 'cancan/model_adapters/strategies/base'
require 'cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery'
require 'cancan/model_adapters/strategies/joined_alias_exists_subquery'
require 'cancan/model_adapters/strategies/left_join'
require 'cancan/model_adapters/strategies/subquery'
end
6 changes: 5 additions & 1 deletion lib/cancan/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
module CanCan
def self.valid_accessible_by_strategies
strategies = [:left_join]
strategies << :subquery unless does_not_support_subquery_strategy?

unless does_not_support_subquery_strategy?
strategies.push(:joined_alias_exists_subquery, :joined_alias_each_rule_as_exists_subquery, :subquery)
end

strategies
end

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
module CanCan
module ModelAdapters
class Strategies
class JoinedAliasEachRuleAsExistsSubquery < Base
def execute!
model_class
.joins(
"JOIN #{quoted_table_name} AS #{quoted_aliased_table_name} ON " \
"#{quoted_aliased_table_name}.#{quoted_primary_key} = #{quoted_table_name}.#{quoted_primary_key}"
)
.where(double_exists_sql)
end

def double_exists_sql
double_exists_sql = ''

compressed_rules.each_with_index do |rule, index|
double_exists_sql << ' OR ' if index > 0
double_exists_sql << "EXISTS (#{sub_query_for_rule(rule).to_sql})"
end

double_exists_sql
end

def sub_query_for_rule(rule)
conditions_extractor = ConditionsExtractor.new(model_class)
rule_where_conditions = extract_multiple_conditions(conditions_extractor, [rule])
joins_hash, left_joins_hash = extract_joins_from_rule(rule)
sub_query_for_rules_and_join_hashes(rule_where_conditions, joins_hash, left_joins_hash)
end

def sub_query_for_rules_and_join_hashes(rule_where_conditions, joins_hash, left_joins_hash)
model_class
.select('1')
.joins(joins_hash)
.left_joins(left_joins_hash)
.where(
"#{quoted_table_name}.#{quoted_primary_key} = " \
"#{quoted_aliased_table_name}.#{quoted_primary_key}"
)
.where(rule_where_conditions)
.limit(1)
end

def extract_joins_from_rule(rule)
joins = {}
left_joins = {}

extra_joins_recursive([], rule.conditions, joins, left_joins)
[joins, left_joins]
end

def extra_joins_recursive(current_path, conditions, joins, left_joins)
conditions.each do |key, value|
if value.is_a?(Hash)
current_path << key
extra_joins_recursive(current_path, value, joins, left_joins)
current_path.pop
else
extra_joins_recursive_merge_joins(current_path, value, joins, left_joins)
end
end
end

def extra_joins_recursive_merge_joins(current_path, value, joins, left_joins)
hash_joins = current_path_to_hash(current_path)

if value.nil?
left_joins.deep_merge!(hash_joins)
else
joins.deep_merge!(hash_joins)
end
end

# Converts an array like [:child, :grand_child] into a hash like {child: {grand_child: {}}
def current_path_to_hash(current_path)
hash_joins = {}
current_hash_joins = hash_joins

current_path.each do |path_part|
new_hash = {}
current_hash_joins[path_part] = new_hash
current_hash_joins = new_hash
end

hash_joins
end
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
module CanCan
module ModelAdapters
class Strategies
class JoinedAliasExistsSubquery < Base
def execute!
model_class
.joins(
"JOIN #{quoted_table_name} AS #{quoted_aliased_table_name} ON " \
"#{quoted_aliased_table_name}.#{quoted_primary_key} = #{quoted_table_name}.#{quoted_primary_key}"
)
.where("EXISTS (#{joined_alias_exists_subquery_inner_query.to_sql})")
end

def joined_alias_exists_subquery_inner_query
model_class
.unscoped
.select('1')
.left_joins(joins)
.where(*where_conditions)
.where(
"#{quoted_table_name}.#{quoted_primary_key} = " \
"#{quoted_aliased_table_name}.#{quoted_primary_key}"
)
.limit(1)
end
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ class Editor < ActiveRecord::Base
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')
posts = Post.accessible_by(ability).where(published: true).select('posts.title as mytitle')
expect(posts[0].mytitle).to eq 'post1'
end
end
Expand Down
56 changes: 56 additions & 0 deletions spec/cancan/model_adapters/has_and_belongs_to_many_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,62 @@ class House < ActiveRecord::Base
end

unless CanCan::ModelAdapters::ActiveRecordAdapter.version_lower?('5.0.0')
describe 'fetching of records - joined_alias_subquery strategy' do
before do
CanCan.accessible_by_strategy = :joined_alias_exists_subquery
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\"
JOIN \"houses\" AS \"houses_alias\" ON \"houses_alias\".\"id\" = \"houses\".\"id\"
WHERE (EXISTS (SELECT 1
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} AND
(\"houses\".\"id\" = \"houses_alias\".\"id\") LIMIT 1))
")
end
end
end

describe 'fetching of records - joined_alias_each_rule_as_exists_subquery strategy' do
before do
CanCan.accessible_by_strategy = :joined_alias_each_rule_as_exists_subquery
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\"
JOIN \"houses\" AS \"houses_alias\" ON \"houses_alias\".\"id\" = \"houses\".\"id\"
WHERE (EXISTS (SELECT 1
FROM \"houses\"
INNER JOIN \"houses_people\" ON \"houses_people\".\"house_id\" = \"houses\".\"id\"
INNER JOIN \"people\" ON \"people\".\"id\" = \"houses_people\".\"person_id\"
WHERE (\"houses\".\"id\" = \"houses_alias\".\"id\") AND
(\"people\".\"id\" = #{@person1.id})
LIMIT 1))
")
end
end
end

describe 'fetching of records - subquery strategy' do
before do
CanCan.accessible_by_strategy = :subquery
Expand Down

0 comments on commit 24ed063

Please sign in to comment.