Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accessible by strategy: Exists subquery #691

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/cancan/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
module CanCan
def self.valid_accessible_by_strategies
strategies = [:left_join]
strategies << :subquery unless does_not_support_subquery_strategy?
strategies.push(:double_exist_subquery, :subquery) unless does_not_support_subquery_strategy?
strategies
end

Expand Down
50 changes: 45 additions & 5 deletions lib/cancan/model_adapters/active_record_5_adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,57 @@ def self.matches_condition?(subject, name, value)

private

delegate :connection, :quoted_primary_key, to: :@model_class
delegate :quote_table_name, to: :connection

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)

build_joins_relation_subquery(where_conditions)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refactor was needed to deal with Rubocop complexity offences.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably, at some point, extract these strategies

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coorasse I have extracted all strategies as separate classes (also to solve Rubocop complexity issues). What do you think?

when :left_join
relation.left_joins(joins).distinct
when :double_exist_subquery
build_joins_relation_double_exist_subquery(where_conditions)
end
end

def build_joins_relation_subquery(where_conditions)
inner = @model_class.unscoped do
@model_class.left_joins(joins).where(*where_conditions)
end
@model_class.where(@model_class.primary_key => inner)
end

def build_joins_relation_double_exist_subquery(where_conditions)
@model_class.where("EXISTS (#{double_exists_query_sql(where_conditions)})")
end

def double_exists_inner_query(where_conditions)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure the code has gotten less abstract having to use this many helper methods, but I couldn't otherwise find a way around the Rubocop complexity offences :'-(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly might get rid of rubocop and switch to standard soon :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I feel that Rubocop has forced me to make the code more abstract than it had to be here :-)

@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}"
)
end

def double_exists_query_sql(where_conditions)
'SELECT 1 ' \
"FROM #{quoted_table_name} AS #{quoted_aliased_table_name} " \
'WHERE ' \
"#{quoted_aliased_table_name}.#{quoted_primary_key} = #{quoted_table_name}.#{quoted_primary_key} AND " \
"EXISTS (#{double_exists_inner_query(where_conditions).to_sql})"
end

def quoted_aliased_table_name
@quoted_aliased_table_name ||= quote_table_name('aliased_table')
end

def quoted_table_name
@quoted_table_name ||= quote_table_name(@model_class.table_name)
end

def sanitize_sql(conditions)
Expand Down
32 changes: 32 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,38 @@ class House < ActiveRecord::Base
end

unless CanCan::ModelAdapters::ActiveRecordAdapter.version_lower?('5.0.0')
describe 'fetching of records - double_exist_subquery strategy' do
before do
CanCan.accessible_by_strategy = :double_exist_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\"
WHERE (EXISTS (SELECT 1
FROM \"houses\" AS \"aliased_table\"
WHERE
\"aliased_table\".\"id\" = \"houses\".\"id\" AND
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\" = \"aliased_table\".\"id\"))))
")
end
end
end

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