From e098ddaacdd2218dc60748127739b565030e3922 Mon Sep 17 00:00:00 2001 From: Ryan Bates Date: Tue, 20 Jul 2010 16:00:22 -0700 Subject: [PATCH] refactoring query.conditions --- lib/cancan/ability.rb | 6 +- lib/cancan/active_record_additions.rb | 5 +- lib/cancan/query.rb | 93 ++++++++------------- spec/cancan/active_record_additions_spec.rb | 8 +- spec/cancan/query_spec.rb | 82 +++++++++--------- spec/matchers.rb | 13 +++ spec/spec_helper.rb | 1 + 7 files changed, 100 insertions(+), 108 deletions(-) create mode 100644 spec/matchers.rb diff --git a/lib/cancan/ability.rb b/lib/cancan/ability.rb index aba0dd7..23eed3b 100644 --- a/lib/cancan/ability.rb +++ b/lib/cancan/ability.rb @@ -183,8 +183,10 @@ module CanCan end # Returns a CanCan::Query instance to help generate database queries based on the ability. + # If any relevant can definitions use a block then an exception will be raised because an + # SQL query cannot be generated from blocks of code. def query(action, subject) - Query.new(subject, relevant_can_definitions_without_block(action, subject)) + Query.new(subject, relevant_can_definitions_for_query(action, subject)) end private @@ -211,7 +213,7 @@ module CanCan end end - def relevant_can_definitions_without_block(action, subject) + def relevant_can_definitions_for_query(action, subject) relevant_can_definitions(action, subject).each do |can_definition| if can_definition.only_block? raise Error, "Cannot determine SQL conditions or joins from block for #{action.inspect} #{subject.inspect}" diff --git a/lib/cancan/active_record_additions.rb b/lib/cancan/active_record_additions.rb index f09eb39..24f03ae 100644 --- a/lib/cancan/active_record_additions.rb +++ b/lib/cancan/active_record_additions.rb @@ -21,11 +21,10 @@ module CanCan # internally uses Ability#conditions method, see that for more information. def accessible_by(ability, action = :read) query = ability.query(action, self) - conditions = query.sql_conditions || {:id => nil} if respond_to? :where - where(conditions).joins(query.association_joins) + where(query.conditions).joins(query.joins) else - scoped(:conditions => conditions, :joins => query.association_joins) + scoped(:conditions => query.conditions, :joins => query.joins) end end end diff --git a/lib/cancan/query.rb b/lib/cancan/query.rb index 4f99581..8411a63 100644 --- a/lib/cancan/query.rb +++ b/lib/cancan/query.rb @@ -7,82 +7,61 @@ module CanCan @sanitizer = sanitizer @can_definitions = can_definitions end - - # Returns an array of arrays composing from desired action and hash of conditions which match the given ability. - # This is useful if you need to generate a database query based on the current ability. - # - # can :read, Article, :visible => true - # conditions :read, Article # returns [ [ true, { :visible => true } ] ] - # - # can :read, Article, :visible => true - # cannot :read, Article, :blocked => true - # conditions :read, Article # returns [ [ false, { :blocked => true } ], [ true, { :visible => true } ] ] - # - # Normally you will not call this method directly, but instead go through ActiveRecordAdditions#accessible_by method. - # - # If the ability is not defined then false is returned so be sure to take that into consideration. - # If the ability is defined using a block then this will raise an exception since a hash of conditions cannot be - # determined from that. - def conditions - unless @can_definitions.empty? - @can_definitions.map do |can_definition| - [can_definition.base_behavior, can_definition.tableized_conditions] - end - else - false - end - end - # Returns sql conditions for object, which responds to :sanitize_sql . - # This is useful if you need to generate a database query based on the current ability. + # Returns a string of SQL conditions which match the ability query. # # can :manage, User, :id => 1 # can :manage, User, :manager_id => 1 # cannot :manage, User, :self_managed => true - # sql_conditions :manage, User # returns not (self_managed = 't') AND ((manager_id = 1) OR (id = 1)) + # query(:manage, User).conditions # => "not (self_managed = 't') AND ((manager_id = 1) OR (id = 1))" # - # Normally you will not call this method directly, but instead go through ActiveRecordAdditions#accessible_by method. + # Normally you will not call this method directly, but instead go through ActiveRecordAdditions#accessible_by. # - # If the ability is not defined then false is returned so be sure to take that into consideration. # If there is just one :can ability, it conditions returned untouched. - # If the ability is defined using a block then this will raise an exception since a hash of conditions cannot be - # determined from that. - def sql_conditions - conds = conditions - return false if conds == false - return (conds[0][1] || {}) if conds.size==1 && conds[0][0] == true # to match previous spec - - true_cond = sanitize_sql(['?=?', true, true]) - false_cond = sanitize_sql(['?=?', true, false]) - conds.reverse.inject(false_cond) do |sql, action| - behavior, condition = action - if condition && condition != {} - condition = sanitize_sql(condition) - case sql - when true_cond - behavior ? true_cond : "not (#{condition})" - when false_cond - behavior ? condition : false_cond - else - behavior ? "(#{condition}) OR (#{sql})" : "not (#{condition}) AND (#{sql})" - end - else - behavior ? true_cond : false_cond + def conditions + if @can_definitions.size == 1 && @can_definitions.first.base_behavior + # Return the conditions directly if there's just one definition + @can_definitions.first.tableized_conditions + else + @can_definitions.reverse.inject(false_sql) do |sql, can_definition| + merge_conditions(sql, can_definition.tableized_conditions, can_definition.base_behavior) end end end - # Returns the associations used in conditions. This is usually used in the :joins option for a search. + # Returns the associations used in conditions for the :joins option of a search # See ActiveRecordAdditions#accessible_by for use in Active Record. - def association_joins + def joins unless @can_definitions.empty? collect_association_joins(@can_definitions) - else - nil end end private + + def merge_conditions(sql, conditions_hash, behavior) + if conditions_hash.blank? + behavior ? true_sql : false_sql + else + conditions = sanitize_sql(conditions_hash) + case sql + when true_sql + behavior ? true_sql : "not (#{conditions})" + when false_sql + behavior ? conditions : false_sql + else + behavior ? "(#{conditions}) OR (#{sql})" : "not (#{conditions}) AND (#{sql})" + end + end + end + + def false_sql + sanitize_sql(['?=?', true, false]) + end + + def true_sql + sanitize_sql(['?=?', true, true]) + end def sanitize_sql(conditions) @sanitizer.sanitize_sql(conditions) diff --git a/spec/cancan/active_record_additions_spec.rb b/spec/cancan/active_record_additions_spec.rb index 14585d3..275f90d 100644 --- a/spec/cancan/active_record_additions_spec.rb +++ b/spec/cancan/active_record_additions_spec.rb @@ -9,9 +9,9 @@ describe CanCan::ActiveRecordAdditions do @ability.extend(CanCan::Ability) end - it "should call where(:id => nil) when no ability is defined so no records are found" do - stub(@model_class).where(:id => nil).stub!.joins(nil) { :no_where } - @model_class.accessible_by(@ability, :read).should == :no_where + it "should call where('true=false') when no ability is defined so no records are found" do + stub(@model_class).where('true=false').stub!.joins(nil) { :no_match } + @model_class.accessible_by(@ability, :read).should == :no_match end it "should call where with matching ability conditions" do @@ -44,7 +44,7 @@ describe CanCan::ActiveRecordAdditions do stub(@model_class).scoped( :conditions => condition, :joins => joins ) { :found_records } end end - # @ability.sql_conditions(:read, @model_class).should == '(too.car=1 AND too.far.bar=1) OR (foo.bar=1)' + # @ability.conditions(:read, @model_class).should == '(too.car=1 AND too.far.bar=1) OR (foo.bar=1)' # @ability.association_joins(:read, @model_class).should == [{:too => [:far]}, :foo] @model_class.accessible_by(@ability).should == :found_records end diff --git a/spec/cancan/query_spec.rb b/spec/cancan/query_spec.rb index 4510d46..56878ac 100644 --- a/spec/cancan/query_spec.rb +++ b/spec/cancan/query_spec.rb @@ -6,72 +6,70 @@ describe CanCan::Query do @ability.extend(CanCan::Ability) end - it "should return array of behavior and conditions for a given ability" do - @ability.can :read, Person, :first => 1, :last => 3 - @ability.query(:show, Person).conditions.should == [[true, {:first => 1, :last => 3}]] - end - - it "should raise an exception when a block is used on condition, and no hash" do - @ability.can :read, Person do |a| - true - end - lambda { @ability.query(:show, Person).conditions }.should raise_error(CanCan::Error, "Cannot determine SQL conditions or joins from block for :show Person") - end - - it "should return an array with just behavior for conditions when there are no conditions" do - @ability.can :read, Person - @ability.query(:show, Person).conditions.should == [ [true, {}] ] - end - - it "should return false when performed on an action which isn't defined" do - @ability.query(:foo, Person).conditions.should == false + it "should have false conditions if no abilities match" do + @ability.query(:destroy, Person).conditions.should == "true=false" end it "should return hash for single `can` definition" do @ability.can :read, Person, :blocked => false, :user_id => 1 - - @ability.query(:read, Person).sql_conditions.should == { :blocked => false, :user_id => 1 } + @ability.query(:read, Person).conditions.should == { :blocked => false, :user_id => 1 } end - it "should return `sql` for single `can` definition in front of default `cannot` condition" do + it "should merge multiple can definitions into single SQL string joining with OR" do + @ability.can :read, Person, :blocked => false + @ability.can :read, Person, :admin => true + @ability.query(:read, Person).conditions.should == "(admin=true) OR (blocked=false)" + end + + it "should merge multiple can definitions into single SQL string joining with OR and AND" do + @ability.can :read, Person, :blocked => false, :active => true + @ability.can :read, Person, :admin => true + @ability.query(:read, Person).conditions.should orderlessly_match("(blocked=false AND active=true) OR (admin=true)") + end + + it "should merge multiple can definitions into single SQL string joining with OR and AND" do + @ability.can :read, Person, :blocked => false, :active => true + @ability.can :read, Person, :admin => true + @ability.query(:read, Person).conditions.should orderlessly_match("(blocked=false AND active=true) OR (admin=true)") + end + + it "should return false conditions for cannot clause" do + @ability.cannot :read, Person + @ability.query(:read, Person).conditions.should == "true=false" + end + + it "should return SQL for single `can` definition in front of default `cannot` condition" do @ability.cannot :read, Person @ability.can :read, Person, :blocked => false, :user_id => 1 - result = @ability.query(:read, Person).sql_conditions - result.should include("blocked=false") - result.should include(" AND ") - result.should include("user_id=1") - end - - it "should return `true condition` for single `can` definition in front of default `can` condition" do + result = @ability.query(:read, Person).conditions.should orderlessly_match("blocked=false AND user_id=1") + end + + it "should return true condition for single `can` definition in front of default `can` condition" do @ability.can :read, Person @ability.can :read, Person, :blocked => false, :user_id => 1 - @ability.query(:read, Person).sql_conditions.should == 'true=true' + @ability.query(:read, Person).conditions.should == 'true=true' end - - it "should return `false condition` for single `cannot` definition" do + + it "should return false condition for single `cannot` definition" do @ability.cannot :read, Person, :blocked => true, :user_id => 1 - @ability.query(:read, Person).sql_conditions.should == 'true=false' + @ability.query(:read, Person).conditions.should == 'true=false' end it "should return `false condition` for single `cannot` definition in front of default `cannot` condition" do @ability.cannot :read, Person @ability.cannot :read, Person, :blocked => true, :user_id => 1 - @ability.query(:read, Person).sql_conditions.should == 'true=false' + @ability.query(:read, Person).conditions.should == 'true=false' end it "should return `not (sql)` for single `cannot` definition in front of default `can` condition" do @ability.can :read, Person @ability.cannot :read, Person, :blocked => true, :user_id => 1 - result = @ability.query(:read, Person).sql_conditions - result.should include("not ") - result.should include("blocked=true") - result.should include(" AND ") - result.should include("user_id=1") + result = @ability.query(:read, Person).conditions.should orderlessly_match("not (blocked=true AND user_id=1)") end it "should return appropriate sql conditions in complex case" do @@ -80,8 +78,8 @@ describe CanCan::Query do @ability.can :update, Person, :manager_id => 1 @ability.cannot :update, Person, :self_managed => true - @ability.query(:update, Person).sql_conditions.should == 'not (self_managed=true) AND ((manager_id=1) OR (id=1))' - @ability.query(:manage, Person).sql_conditions.should == {:id=>1} - @ability.query(:read, Person).sql_conditions.should == 'true=true' + @ability.query(:update, Person).conditions.should == 'not (self_managed=true) AND ((manager_id=1) OR (id=1))' + @ability.query(:manage, Person).conditions.should == {:id=>1} + @ability.query(:read, Person).conditions.should == 'true=true' end end diff --git a/spec/matchers.rb b/spec/matchers.rb new file mode 100644 index 0000000..a9871d7 --- /dev/null +++ b/spec/matchers.rb @@ -0,0 +1,13 @@ +Spec::Matchers.define :orderlessly_match do |original_string| + match do |given_string| + original_string.bytes.sum == given_string.bytes.sum + end + + failure_message_for_should do |given_string| + "expected \"#{given_string}\" to have the same characters as \"#{original_string}\"" + end + + failure_message_for_should_not do |given_string| + "expected \"#{given_string}\" not to have the same characters as \"#{original_string}\"" + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 3890748..766813d 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -4,6 +4,7 @@ require 'active_support' require 'active_record' require 'action_controller' require 'action_view' +require 'matchers' require 'cancan' require 'cancan/matchers'