From bd4fb3836fd51f1960155abe15e477ec855f2f09 Mon Sep 17 00:00:00 2001 From: kaspernj Date: Thu, 13 May 2021 11:08:04 +0200 Subject: [PATCH 01/19] Added the new strategy: double_exist_subquery --- lib/cancan/config.rb | 2 +- .../model_adapters/active_record_5_adapter.rb | 50 +++++++++++++++++-- .../has_and_belongs_to_many_spec.rb | 32 ++++++++++++ 3 files changed, 78 insertions(+), 6 deletions(-) diff --git a/lib/cancan/config.rb b/lib/cancan/config.rb index a9106526b..43eac5b91 100644 --- a/lib/cancan/config.rb +++ b/lib/cancan/config.rb @@ -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 diff --git a/lib/cancan/model_adapters/active_record_5_adapter.rb b/lib/cancan/model_adapters/active_record_5_adapter.rb index 072ed7f50..1dd9b412f 100644 --- a/lib/cancan/model_adapters/active_record_5_adapter.rb +++ b/lib/cancan/model_adapters/active_record_5_adapter.rb @@ -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) 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) + @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) 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..16d7d8034 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 @@ -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 From f77a096c11b31bc1ac937df3b0a69d1c615a7a4d Mon Sep 17 00:00:00 2001 From: kaspernj Date: Wed, 23 Jun 2021 19:50:50 +0200 Subject: [PATCH 02/19] Accessible by strategy: Each rule as exists subquery --- lib/cancan/config.rb | 2 +- .../model_adapters/active_record_5_adapter.rb | 75 +++++++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/lib/cancan/config.rb b/lib/cancan/config.rb index 43eac5b91..7f89cad1c 100644 --- a/lib/cancan/config.rb +++ b/lib/cancan/config.rb @@ -3,7 +3,7 @@ module CanCan def self.valid_accessible_by_strategies strategies = [:left_join] - strategies.push(:double_exist_subquery, :subquery) unless does_not_support_subquery_strategy? + strategies.push(:double_exist_subquery, :each_rule_as_exists_subquery, :subquery) unless does_not_support_subquery_strategy? strategies end diff --git a/lib/cancan/model_adapters/active_record_5_adapter.rb b/lib/cancan/model_adapters/active_record_5_adapter.rb index 1dd9b412f..d8eec825b 100644 --- a/lib/cancan/model_adapters/active_record_5_adapter.rb +++ b/lib/cancan/model_adapters/active_record_5_adapter.rb @@ -26,6 +26,8 @@ def self.matches_condition?(subject, name, value) def build_joins_relation(relation, *where_conditions) case CanCan.accessible_by_strategy + when :each_rule_as_doubleexists_subquery + build_joins_each_rule_as_exists_subquery(where_conditions) when :subquery build_joins_relation_subquery(where_conditions) when :left_join @@ -35,6 +37,79 @@ def build_joins_relation(relation, *where_conditions) end end + def build_joins_each_rule_as_exists_subquery(where_conditions) + double_exists_sql = + '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 " \ + '(' + + '' + + @compressed_rules.each_with_index do |rule, index| + 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) + + # binding.pry if rule.conditions.length > 0 + + puts "Conditions: #{rule.conditions}" + puts "Joins hash: #{joins_hash}" + puts "Left joins hash: #{left_joins_hash}" + + sub_query = @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) + + double_exists_sql << " OR " if index > 0 + double_exists_sql << "EXISTS (#{sub_query.to_sql})" + end + + double_exists_sql << ")" + + @model_class.unscoped.where("EXISTS (#{double_exists_sql} 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 + 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 + + if value.nil? + left_joins.deep_merge!(hash_joins) + else + joins.deep_merge!(hash_joins) + end + end + end + end + def build_joins_relation_subquery(where_conditions) inner = @model_class.unscoped do @model_class.left_joins(joins).where(*where_conditions) From 704176af6cf84bab435deb212415e9c567766f51 Mon Sep 17 00:00:00 2001 From: kaspernj Date: Thu, 24 Jun 2021 09:30:54 +0200 Subject: [PATCH 03/19] Use aliased joined table instead of double sub query --- lib/cancan/config.rb | 2 +- .../model_adapters/active_record_5_adapter.rb | 54 ++++++++----------- 2 files changed, 23 insertions(+), 33 deletions(-) diff --git a/lib/cancan/config.rb b/lib/cancan/config.rb index 7f89cad1c..e744f7c99 100644 --- a/lib/cancan/config.rb +++ b/lib/cancan/config.rb @@ -3,7 +3,7 @@ module CanCan def self.valid_accessible_by_strategies strategies = [:left_join] - strategies.push(:double_exist_subquery, :each_rule_as_exists_subquery, :subquery) unless does_not_support_subquery_strategy? + strategies.push(:joined_alias_exists_subquery, :joined_alias_each_rule_as_exists_subquery, :subquery) unless does_not_support_subquery_strategy? strategies end diff --git a/lib/cancan/model_adapters/active_record_5_adapter.rb b/lib/cancan/model_adapters/active_record_5_adapter.rb index d8eec825b..376f018b4 100644 --- a/lib/cancan/model_adapters/active_record_5_adapter.rb +++ b/lib/cancan/model_adapters/active_record_5_adapter.rb @@ -26,37 +26,25 @@ def self.matches_condition?(subject, name, value) def build_joins_relation(relation, *where_conditions) case CanCan.accessible_by_strategy - when :each_rule_as_doubleexists_subquery - build_joins_each_rule_as_exists_subquery(where_conditions) + when :joined_alias_each_rule_as_exists_subquery + build_joined_alias_each_rule_as_exists_subquery(where_conditions) + when :joined_alias_exists_subquery + build_joined_alias_exists_subquery(where_conditions) when :subquery build_joins_relation_subquery(where_conditions) 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_each_rule_as_exists_subquery(where_conditions) - double_exists_sql = - '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 " \ - '(' + - '' + def build_joined_alias_each_rule_as_exists_subquery(where_conditions) + double_exists_sql = '' + '' @compressed_rules.each_with_index do |rule, index| 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) - # binding.pry if rule.conditions.length > 0 - - puts "Conditions: #{rule.conditions}" - puts "Joins hash: #{joins_hash}" - puts "Left joins hash: #{left_joins_hash}" - sub_query = @model_class .select("1") .joins(joins_hash) @@ -72,9 +60,13 @@ def build_joins_each_rule_as_exists_subquery(where_conditions) double_exists_sql << "EXISTS (#{sub_query.to_sql})" end - double_exists_sql << ")" - - @model_class.unscoped.where("EXISTS (#{double_exists_sql} LIMIT 1)") + @model_class + .unscoped + .joins( + "INNER 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 extract_joins_from_rule(rule) @@ -117,11 +109,17 @@ def build_joins_relation_subquery(where_conditions) @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)})") + def build_joined_alias_exists_subquery(where_conditions) + @model_class + .unscoped + .joins( + "INNER 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(where_conditions).to_sql})") end - def double_exists_inner_query(where_conditions) + def joined_alias_exists_subquery_inner_query(where_conditions) @model_class .unscoped .select('1') @@ -133,14 +131,6 @@ def double_exists_inner_query(where_conditions) ) 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 From fc3c88d1d3c56765ca4106f10a190db8e769d7c6 Mon Sep 17 00:00:00 2001 From: kaspernj Date: Thu, 24 Jun 2021 11:01:13 +0200 Subject: [PATCH 04/19] Solved offences and specifically solved complexity offences by extracting code out into separate strategy classes --- lib/cancan/config.rb | 6 +- lib/cancan/model_adapters/abstract_adapter.rb | 2 + .../model_adapters/active_record_5_adapter.rb | 106 +----------------- .../model_adapters/active_record_adapter.rb | 2 + .../strategies/base_strategy.rb | 39 +++++++ .../build_joined_alias_exists_subquery.rb | 43 +++++++ ...ined_alias_each_rule_as_exists_subquery.rb | 80 +++++++++++++ .../joined_alias_exists_subquery.rb | 30 +++++ .../has_and_belongs_to_many_spec.rb | 36 +++++- 9 files changed, 241 insertions(+), 103 deletions(-) create mode 100644 lib/cancan/model_adapters/strategies/base_strategy.rb create mode 100644 lib/cancan/model_adapters/strategies/build_joined_alias_exists_subquery.rb create mode 100644 lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb create mode 100644 lib/cancan/model_adapters/strategies/joined_alias_exists_subquery.rb diff --git a/lib/cancan/config.rb b/lib/cancan/config.rb index e744f7c99..75e68bad7 100644 --- a/lib/cancan/config.rb +++ b/lib/cancan/config.rb @@ -3,7 +3,11 @@ module CanCan def self.valid_accessible_by_strategies strategies = [:left_join] - strategies.push(:joined_alias_exists_subquery, :joined_alias_each_rule_as_exists_subquery, :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 diff --git a/lib/cancan/model_adapters/abstract_adapter.rb b/lib/cancan/model_adapters/abstract_adapter.rb index 4e0e51a25..8a68382bd 100644 --- a/lib/cancan/model_adapters/abstract_adapter.rb +++ b/lib/cancan/model_adapters/abstract_adapter.rb @@ -3,6 +3,8 @@ module CanCan module ModelAdapters class AbstractAdapter + attr_reader :model_class + def self.inherited(subclass) @subclasses ||= [] @subclasses.insert(0, subclass) diff --git a/lib/cancan/model_adapters/active_record_5_adapter.rb b/lib/cancan/model_adapters/active_record_5_adapter.rb index 376f018b4..992df5ebf 100644 --- a/lib/cancan/model_adapters/active_record_5_adapter.rb +++ b/lib/cancan/model_adapters/active_record_5_adapter.rb @@ -21,85 +21,21 @@ 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 :joined_alias_each_rule_as_exists_subquery - build_joined_alias_each_rule_as_exists_subquery(where_conditions) - when :joined_alias_exists_subquery - build_joined_alias_exists_subquery(where_conditions) when :subquery build_joins_relation_subquery(where_conditions) when :left_join relation.left_joins(joins).distinct + else + strategy_class.new(adapter: self, where_conditions: where_conditions).execute! end end - def build_joined_alias_each_rule_as_exists_subquery(where_conditions) - double_exists_sql = '' + '' - - @compressed_rules.each_with_index do |rule, index| - 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 = @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) - - double_exists_sql << " OR " if index > 0 - double_exists_sql << "EXISTS (#{sub_query.to_sql})" - end - - @model_class - .unscoped - .joins( - "INNER 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 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 - 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 - - if value.nil? - left_joins.deep_merge!(hash_joins) - else - joins.deep_merge!(hash_joins) - end - end - end + def strategy_class + require_relative "strategies/#{CanCan.accessible_by_strategy}" + strategy_class_name = CanCan.accessible_by_strategy.to_s.camelize + CanCan::ModelAdapters::Strategies.const_get(strategy_class_name) end def build_joins_relation_subquery(where_conditions) @@ -109,36 +45,6 @@ def build_joins_relation_subquery(where_conditions) @model_class.where(@model_class.primary_key => inner) end - def build_joined_alias_exists_subquery(where_conditions) - @model_class - .unscoped - .joins( - "INNER 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(where_conditions).to_sql})") - end - - def joined_alias_exists_subquery_inner_query(where_conditions) - @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 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) if conditions.is_a?(Hash) sanitize_sql_activerecord5(conditions) diff --git a/lib/cancan/model_adapters/active_record_adapter.rb b/lib/cancan/model_adapters/active_record_adapter.rb index 7bd969c92..741570838 100644 --- a/lib/cancan/model_adapters/active_record_adapter.rb +++ b/lib/cancan/model_adapters/active_record_adapter.rb @@ -11,6 +11,8 @@ def self.version_lower?(version) Gem::Version.new(ActiveRecord.version).release < Gem::Version.new(version) end + attr_reader :compressed_rules + def initialize(model_class, rules) super @compressed_rules = RulesCompressor.new(@rules.reverse).rules_collapsed.reverse diff --git a/lib/cancan/model_adapters/strategies/base_strategy.rb b/lib/cancan/model_adapters/strategies/base_strategy.rb new file mode 100644 index 000000000..8c30dbb0e --- /dev/null +++ b/lib/cancan/model_adapters/strategies/base_strategy.rb @@ -0,0 +1,39 @@ +module CanCan + module ModelAdapters + class Strategies + class BaseStrategy + attr_reader :adapter, :where_conditions + + delegate( + :compressed_rules, + :extract_multiple_conditions, + :joins, + :model_class, + :quoted_primary_key, + :quoted_aliased_table_name, + :quoted_table_name, + to: :adapter + ) + delegate :connection, :quoted_primary_key, to: :model_class + delegate :quote_table_name, to: :connection + + def initialize(adapter:, where_conditions:) + @adapter = adapter + @where_conditions = where_conditions + end + + def aliased_table_name + @aliased_table_name ||= "#{model_class.table_name}_alias" + end + + def quoted_aliased_table_name + @quoted_aliased_table_name ||= quote_table_name(aliased_table_name) + end + + def quoted_table_name + @quoted_table_name ||= quote_table_name(model_class.table_name) + end + end + end + end +end diff --git a/lib/cancan/model_adapters/strategies/build_joined_alias_exists_subquery.rb b/lib/cancan/model_adapters/strategies/build_joined_alias_exists_subquery.rb new file mode 100644 index 000000000..6fde6546d --- /dev/null +++ b/lib/cancan/model_adapters/strategies/build_joined_alias_exists_subquery.rb @@ -0,0 +1,43 @@ +require_relative 'base_strategy' + +module CanCan + module ModelAdapters + class ActiveRecordAdapter + class BuildJoinedAliasExistsSubquery < BaseStrategy + attr_reader :adapter, :where_conditions + + delegate :model_class, to: :adapter + + def initialize(adapter:, where_conditions:) + @adapter = adapter + @where_conditions = where_conditions + end + + def execute! + build_joined_alias_exists_subquery + end + + def build_joined_alias_exists_subquery + 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}" + ) + end + end + end + end +end diff --git a/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb b/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb new file mode 100644 index 000000000..9833efb16 --- /dev/null +++ b/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb @@ -0,0 +1,80 @@ +require_relative 'base_strategy' + +module CanCan + module ModelAdapters + class Strategies + class JoinedAliasEachRuleAsExistsSubquery < BaseStrategy + def execute! + model_class + .unscoped + .joins( + "INNER 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) + + 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 + 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 + + if value.nil? + left_joins.deep_merge!(hash_joins) + else + joins.deep_merge!(hash_joins) + end + end + end + end + end + end + end +end diff --git a/lib/cancan/model_adapters/strategies/joined_alias_exists_subquery.rb b/lib/cancan/model_adapters/strategies/joined_alias_exists_subquery.rb new file mode 100644 index 000000000..566a78bc4 --- /dev/null +++ b/lib/cancan/model_adapters/strategies/joined_alias_exists_subquery.rb @@ -0,0 +1,30 @@ +require_relative 'base_strategy' + +module CanCan + module ModelAdapters + class Strategies + class JoinedAliasExistsSubquery < BaseStrategy + 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}" + ) + end + end + end + 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 16d7d8034..e593cd7b7 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 @@ -46,9 +46,41 @@ class House < ActiveRecord::Base end unless CanCan::ModelAdapters::ActiveRecordAdapter.version_lower?('5.0.0') - describe 'fetching of records - double_exist_subquery strategy' do + describe 'fetching of records - joined_alias_subquery strategy' do before do - CanCan.accessible_by_strategy = :double_exist_subquery + CanCan.accessible_by_strategy = :joined_alias_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 - 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 From efb09cdc05ba1913eeeb6ae8684f9cfba55116b4 Mon Sep 17 00:00:00 2001 From: kaspernj Date: Thu, 24 Jun 2021 11:05:15 +0200 Subject: [PATCH 05/19] Extracted existing strategies out into separate classes as well --- .../model_adapters/active_record_5_adapter.rb | 16 +-------------- .../strategies/base_strategy.rb | 1 + .../model_adapters/strategies/left_join.rb | 13 ++++++++++++ .../model_adapters/strategies/subquery.rb | 20 +++++++++++++++++++ 4 files changed, 35 insertions(+), 15 deletions(-) create mode 100644 lib/cancan/model_adapters/strategies/left_join.rb create mode 100644 lib/cancan/model_adapters/strategies/subquery.rb diff --git a/lib/cancan/model_adapters/active_record_5_adapter.rb b/lib/cancan/model_adapters/active_record_5_adapter.rb index 992df5ebf..8e32c6950 100644 --- a/lib/cancan/model_adapters/active_record_5_adapter.rb +++ b/lib/cancan/model_adapters/active_record_5_adapter.rb @@ -22,14 +22,7 @@ def self.matches_condition?(subject, name, value) private def build_joins_relation(relation, *where_conditions) - case CanCan.accessible_by_strategy - when :subquery - build_joins_relation_subquery(where_conditions) - when :left_join - relation.left_joins(joins).distinct - else - strategy_class.new(adapter: self, where_conditions: where_conditions).execute! - end + strategy_class.new(adapter: self, where_conditions: where_conditions).execute! end def strategy_class @@ -38,13 +31,6 @@ def strategy_class CanCan::ModelAdapters::Strategies.const_get(strategy_class_name) 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 sanitize_sql(conditions) if conditions.is_a?(Hash) sanitize_sql_activerecord5(conditions) diff --git a/lib/cancan/model_adapters/strategies/base_strategy.rb b/lib/cancan/model_adapters/strategies/base_strategy.rb index 8c30dbb0e..4e3dbb899 100644 --- a/lib/cancan/model_adapters/strategies/base_strategy.rb +++ b/lib/cancan/model_adapters/strategies/base_strategy.rb @@ -12,6 +12,7 @@ class BaseStrategy :quoted_primary_key, :quoted_aliased_table_name, :quoted_table_name, + :relation, to: :adapter ) delegate :connection, :quoted_primary_key, to: :model_class diff --git a/lib/cancan/model_adapters/strategies/left_join.rb b/lib/cancan/model_adapters/strategies/left_join.rb new file mode 100644 index 000000000..47952bf39 --- /dev/null +++ b/lib/cancan/model_adapters/strategies/left_join.rb @@ -0,0 +1,13 @@ +require_relative 'base_strategy' + +module CanCan + module ModelAdapters + class Strategies + class LeftJoin < BaseStrategy + def execute! + relation.left_joins(joins).distinct + end + end + end + end +end diff --git a/lib/cancan/model_adapters/strategies/subquery.rb b/lib/cancan/model_adapters/strategies/subquery.rb new file mode 100644 index 000000000..c95c78d6a --- /dev/null +++ b/lib/cancan/model_adapters/strategies/subquery.rb @@ -0,0 +1,20 @@ +require_relative 'base_strategy' + +module CanCan + module ModelAdapters + class Strategies + class Subquery < BaseStrategy + def execute! + build_joins_relation_subquery(where_conditions) + 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 + end + end + end +end From b4536fd78abb74f8f8ed0bfd3751b9d864c10428 Mon Sep 17 00:00:00 2001 From: kaspernj Date: Thu, 24 Jun 2021 11:18:05 +0200 Subject: [PATCH 06/19] Fixed more offences and cleaned up --- .../model_adapters/active_record_5_adapter.rb | 2 +- .../strategies/base_strategy.rb | 6 +-- ...ined_alias_each_rule_as_exists_subquery.rb | 40 +++++++++++++------ 3 files changed, 31 insertions(+), 17 deletions(-) diff --git a/lib/cancan/model_adapters/active_record_5_adapter.rb b/lib/cancan/model_adapters/active_record_5_adapter.rb index 8e32c6950..2999e6337 100644 --- a/lib/cancan/model_adapters/active_record_5_adapter.rb +++ b/lib/cancan/model_adapters/active_record_5_adapter.rb @@ -22,7 +22,7 @@ def self.matches_condition?(subject, name, value) private def build_joins_relation(relation, *where_conditions) - strategy_class.new(adapter: self, where_conditions: where_conditions).execute! + strategy_class.new(adapter: self, relation: relation, where_conditions: where_conditions).execute! end def strategy_class diff --git a/lib/cancan/model_adapters/strategies/base_strategy.rb b/lib/cancan/model_adapters/strategies/base_strategy.rb index 4e3dbb899..970c403c1 100644 --- a/lib/cancan/model_adapters/strategies/base_strategy.rb +++ b/lib/cancan/model_adapters/strategies/base_strategy.rb @@ -2,7 +2,7 @@ module CanCan module ModelAdapters class Strategies class BaseStrategy - attr_reader :adapter, :where_conditions + attr_reader :adapter, :relation, :where_conditions delegate( :compressed_rules, @@ -12,14 +12,14 @@ class BaseStrategy :quoted_primary_key, :quoted_aliased_table_name, :quoted_table_name, - :relation, to: :adapter ) delegate :connection, :quoted_primary_key, to: :model_class delegate :quote_table_name, to: :connection - def initialize(adapter:, where_conditions:) + def initialize(adapter:, relation:, where_conditions:) @adapter = adapter + @relation = relation @where_conditions = where_conditions end diff --git a/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb b/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb index 9833efb16..ad1159920 100644 --- a/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb +++ b/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb @@ -29,7 +29,10 @@ 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) @@ -57,23 +60,34 @@ def extra_joins_recursive(current_path, conditions, joins, left_joins) extra_joins_recursive(current_path, value, joins, left_joins) current_path.pop else - hash_joins = {} - current_hash_joins = hash_joins + extra_joins_recursive_merge_joins(current_path, value, joins, left_joins) + end + end + end - current_path.each do |path_part| - new_hash = {} - current_hash_joins[path_part] = new_hash - current_hash_joins = new_hash - 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 + 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 From 64f37c3f02616d7dd64aef84abff4ffc1aac3b68 Mon Sep 17 00:00:00 2001 From: kaspernj Date: Thu, 24 Jun 2021 11:26:51 +0200 Subject: [PATCH 07/19] Removed unused file --- .../build_joined_alias_exists_subquery.rb | 43 ------------------- 1 file changed, 43 deletions(-) delete mode 100644 lib/cancan/model_adapters/strategies/build_joined_alias_exists_subquery.rb diff --git a/lib/cancan/model_adapters/strategies/build_joined_alias_exists_subquery.rb b/lib/cancan/model_adapters/strategies/build_joined_alias_exists_subquery.rb deleted file mode 100644 index 6fde6546d..000000000 --- a/lib/cancan/model_adapters/strategies/build_joined_alias_exists_subquery.rb +++ /dev/null @@ -1,43 +0,0 @@ -require_relative 'base_strategy' - -module CanCan - module ModelAdapters - class ActiveRecordAdapter - class BuildJoinedAliasExistsSubquery < BaseStrategy - attr_reader :adapter, :where_conditions - - delegate :model_class, to: :adapter - - def initialize(adapter:, where_conditions:) - @adapter = adapter - @where_conditions = where_conditions - end - - def execute! - build_joined_alias_exists_subquery - end - - def build_joined_alias_exists_subquery - 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}" - ) - end - end - end - end -end From be799976541dda23be4c4834168053128d126124 Mon Sep 17 00:00:00 2001 From: kaspernj Date: Thu, 24 Jun 2021 11:26:58 +0200 Subject: [PATCH 08/19] Fixed spec --- .../has_and_belongs_to_many_spec.rb | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) 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 e593cd7b7..c59650553 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 @@ -48,7 +48,7 @@ class House < ActiveRecord::Base 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_subquery + CanCan.accessible_by_strategy = :joined_alias_exists_subquery end it 'it retreives the records correctly' do @@ -92,20 +92,17 @@ class House < ActiveRecord::Base 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\")))) - ") + FROM \"houses\" + INNER 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 From e70c734a382d0bbaf1d1fe84746d27b55e7afdea Mon Sep 17 00:00:00 2001 From: kaspernj Date: Thu, 24 Jun 2021 11:33:11 +0200 Subject: [PATCH 09/19] Fixed specs --- .../has_and_belongs_to_many_spec.rb | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) 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 c59650553..114b52996 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 @@ -93,15 +93,18 @@ class House < ActiveRecord::Base expect(ability.model_adapter(House, :read)) .to generate_sql("SELECT \"houses\".* FROM \"houses\" - INNER 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\" + FROM \"houses\" AS \"aliased_table\" WHERE - (\"houses\".\"id\" = \"houses_alias\".\"id\") AND - (\"people\".\"id\" = #{@person1.id}) - LIMIT 1))) + \"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 From 279fdb54a519a6d6d0ee78236542c57d065d482d Mon Sep 17 00:00:00 2001 From: kaspernj Date: Thu, 24 Jun 2021 11:34:21 +0200 Subject: [PATCH 10/19] Select column through table name --- .../model_adapters/accessible_by_has_many_through_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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..28cca16b5 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 @@ -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 From f18c5aa5374f4fb3a5e577dc3956bdcd71f3b2f7 Mon Sep 17 00:00:00 2001 From: kaspernj Date: Thu, 24 Jun 2021 11:39:56 +0200 Subject: [PATCH 11/19] Fixed more specs --- .../has_and_belongs_to_many_spec.rb | 36 +++++++------------ 1 file changed, 12 insertions(+), 24 deletions(-) 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 114b52996..161ceea8b 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 @@ -60,19 +60,13 @@ class House < ActiveRecord::Base 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\")))) + 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\"))) ") end end @@ -93,18 +87,12 @@ class House < ActiveRecord::Base 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\" 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\")))) + 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\"))) ") end end From 493af4c5a017955586f807ceba6f6f1515db64df Mon Sep 17 00:00:00 2001 From: kaspernj Date: Thu, 24 Jun 2021 11:46:05 +0200 Subject: [PATCH 12/19] Fixed offences --- .../cancan/model_adapters/has_and_belongs_to_many_spec.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) 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 161ceea8b..c2bbbb559 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 @@ -66,7 +66,9 @@ class House < ActiveRecord::Base 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\"))) + WHERE + \"people\".\"id\" = #{@person1.id} AND + (\"houses\".\"id\" = \"houses_alias\".\"id\"))) ") end end @@ -92,7 +94,9 @@ class House < ActiveRecord::Base 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\"))) + WHERE + \"people\".\"id\" = #{@person1.id} AND + (\"houses\".\"id\" = \"houses_alias\".\"id\"))) ") end end From 70b983dc763d4c255fb5d2f0277e0aaf30dd63bf Mon Sep 17 00:00:00 2001 From: kaspernj Date: Thu, 24 Jun 2021 14:06:24 +0200 Subject: [PATCH 13/19] Fixed SQL in spec --- .../model_adapters/has_and_belongs_to_many_spec.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) 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 c2bbbb559..30ed7dde9 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 @@ -89,14 +89,14 @@ class House < ActiveRecord::Base expect(ability.model_adapter(House, :read)) .to generate_sql("SELECT \"houses\".* FROM \"houses\" - JOIN \"houses\" AS \"houses_alias\" ON \"houses_alias\".\"id\" = \"houses\".\"id\" + INNER 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\"))) + 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 From f101181669487a1f425b59e66ed930053fc21d3d Mon Sep 17 00:00:00 2001 From: kaspernj Date: Thu, 24 Jun 2021 14:07:38 +0200 Subject: [PATCH 14/19] Unscoped might remove existing settings on the query --- .../strategies/joined_alias_each_rule_as_exists_subquery.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb b/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb index ad1159920..9e2cc1ba7 100644 --- a/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb +++ b/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb @@ -6,7 +6,6 @@ class Strategies class JoinedAliasEachRuleAsExistsSubquery < BaseStrategy def execute! model_class - .unscoped .joins( "INNER JOIN #{quoted_table_name} AS #{quoted_aliased_table_name} ON " \ "#{quoted_aliased_table_name}.#{quoted_primary_key} = #{quoted_table_name}.#{quoted_primary_key}" From 8bb0a9be6f6fce3e6526695e49b544d08dff64bc Mon Sep 17 00:00:00 2001 From: kaspernj Date: Thu, 24 Jun 2021 14:07:54 +0200 Subject: [PATCH 15/19] Limit results to 1 --- .../model_adapters/strategies/joined_alias_exists_subquery.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/cancan/model_adapters/strategies/joined_alias_exists_subquery.rb b/lib/cancan/model_adapters/strategies/joined_alias_exists_subquery.rb index 566a78bc4..6b15b45d2 100644 --- a/lib/cancan/model_adapters/strategies/joined_alias_exists_subquery.rb +++ b/lib/cancan/model_adapters/strategies/joined_alias_exists_subquery.rb @@ -23,6 +23,7 @@ def joined_alias_exists_subquery_inner_query "#{quoted_table_name}.#{quoted_primary_key} = " \ "#{quoted_aliased_table_name}.#{quoted_primary_key}" ) + .limit(1) end end end From bf86cd64e64b34542efd37f1336d62639afe89d9 Mon Sep 17 00:00:00 2001 From: kaspernj Date: Thu, 24 Jun 2021 14:11:16 +0200 Subject: [PATCH 16/19] Added limit in spec as well --- spec/cancan/model_adapters/has_and_belongs_to_many_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 30ed7dde9..8632149ca 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 @@ -68,7 +68,7 @@ class House < ActiveRecord::Base LEFT OUTER JOIN \"people\" ON \"people\".\"id\" = \"houses_people\".\"person_id\" WHERE \"people\".\"id\" = #{@person1.id} AND - (\"houses\".\"id\" = \"houses_alias\".\"id\"))) + (\"houses\".\"id\" = \"houses_alias\".\"id\")) LIMIT 1) ") end end From 7373d0b80d435d76d3c5c487525cb016898d0a07 Mon Sep 17 00:00:00 2001 From: kaspernj Date: Thu, 24 Jun 2021 14:12:44 +0200 Subject: [PATCH 17/19] Fixed SQL --- spec/cancan/model_adapters/has_and_belongs_to_many_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 8632149ca..41973514d 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 @@ -68,7 +68,7 @@ class House < ActiveRecord::Base LEFT OUTER JOIN \"people\" ON \"people\".\"id\" = \"houses_people\".\"person_id\" WHERE \"people\".\"id\" = #{@person1.id} AND - (\"houses\".\"id\" = \"houses_alias\".\"id\")) LIMIT 1) + (\"houses\".\"id\" = \"houses_alias\".\"id\") LIMIT 1)) ") end end From dd26042d06191c79e9048a9ec03fc369e49f0717 Mon Sep 17 00:00:00 2001 From: kaspernj Date: Thu, 24 Jun 2021 14:13:23 +0200 Subject: [PATCH 18/19] Just use JOIN both places --- .../strategies/joined_alias_each_rule_as_exists_subquery.rb | 2 +- spec/cancan/model_adapters/has_and_belongs_to_many_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb b/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb index 9e2cc1ba7..fe93432c5 100644 --- a/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb +++ b/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb @@ -7,7 +7,7 @@ class JoinedAliasEachRuleAsExistsSubquery < BaseStrategy def execute! model_class .joins( - "INNER JOIN #{quoted_table_name} AS #{quoted_aliased_table_name} ON " \ + "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) 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 41973514d..1df3423ca 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 @@ -89,7 +89,7 @@ class House < ActiveRecord::Base expect(ability.model_adapter(House, :read)) .to generate_sql("SELECT \"houses\".* FROM \"houses\" - INNER JOIN \"houses\" AS \"houses_alias\" ON \"houses_alias\".\"id\" = \"houses\".\"id\" + 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\" From 45a4416c8fe7d657f54a14cb6a7146445828aa4a Mon Sep 17 00:00:00 2001 From: kaspernj Date: Tue, 16 Nov 2021 09:12:12 +0000 Subject: [PATCH 19/19] Fixed several issues after merging with latest version of develop --- lib/cancan.rb | 2 + .../strategies/base_strategy.rb | 40 ------------------- ...ined_alias_each_rule_as_exists_subquery.rb | 4 +- .../joined_alias_exists_subquery.rb | 4 +- 4 files changed, 4 insertions(+), 46 deletions(-) delete mode 100644 lib/cancan/model_adapters/strategies/base_strategy.rb diff --git a/lib/cancan.rb b/lib/cancan.rb index 76d72af77..c4f92c928 100644 --- a/lib/cancan.rb +++ b/lib/cancan.rb @@ -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 diff --git a/lib/cancan/model_adapters/strategies/base_strategy.rb b/lib/cancan/model_adapters/strategies/base_strategy.rb deleted file mode 100644 index 970c403c1..000000000 --- a/lib/cancan/model_adapters/strategies/base_strategy.rb +++ /dev/null @@ -1,40 +0,0 @@ -module CanCan - module ModelAdapters - class Strategies - class BaseStrategy - attr_reader :adapter, :relation, :where_conditions - - delegate( - :compressed_rules, - :extract_multiple_conditions, - :joins, - :model_class, - :quoted_primary_key, - :quoted_aliased_table_name, - :quoted_table_name, - to: :adapter - ) - delegate :connection, :quoted_primary_key, to: :model_class - delegate :quote_table_name, to: :connection - - def initialize(adapter:, relation:, where_conditions:) - @adapter = adapter - @relation = relation - @where_conditions = where_conditions - end - - def aliased_table_name - @aliased_table_name ||= "#{model_class.table_name}_alias" - end - - def quoted_aliased_table_name - @quoted_aliased_table_name ||= quote_table_name(aliased_table_name) - end - - def quoted_table_name - @quoted_table_name ||= quote_table_name(model_class.table_name) - end - end - end - end -end diff --git a/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb b/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb index fe93432c5..5838c7e7c 100644 --- a/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb +++ b/lib/cancan/model_adapters/strategies/joined_alias_each_rule_as_exists_subquery.rb @@ -1,9 +1,7 @@ -require_relative 'base_strategy' - module CanCan module ModelAdapters class Strategies - class JoinedAliasEachRuleAsExistsSubquery < BaseStrategy + class JoinedAliasEachRuleAsExistsSubquery < Base def execute! model_class .joins( diff --git a/lib/cancan/model_adapters/strategies/joined_alias_exists_subquery.rb b/lib/cancan/model_adapters/strategies/joined_alias_exists_subquery.rb index 6b15b45d2..eb7e80b38 100644 --- a/lib/cancan/model_adapters/strategies/joined_alias_exists_subquery.rb +++ b/lib/cancan/model_adapters/strategies/joined_alias_exists_subquery.rb @@ -1,9 +1,7 @@ -require_relative 'base_strategy' - module CanCan module ModelAdapters class Strategies - class JoinedAliasExistsSubquery < BaseStrategy + class JoinedAliasExistsSubquery < Base def execute! model_class .joins(