Allow :having conditions to be sanitized like regular :condition. [#2158

state:resolved] Signed-off-by: Pratik Naik <pratiknaik@gmail.com>
rails · Mar 6, 2009 · 7fb7b48 · 7fb7b48
1 parent 3ca5a0f
commit 7fb7b48
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 5 deletions.
diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
@@ -1754,12 +1754,12 @@ def add_order!(sql, order, scope = :auto)
         def add_group!(sql, group, having, scope = :auto)
           if group
             sql << " GROUP BY #{group}"
-            sql << " HAVING #{having}" if having
+            sql << " HAVING #{sanitize_sql_for_conditions(having)}" if having
           else
             scope = scope(:find) if :auto == scope
             if scope && (scoped_group = scope[:group])
               sql << " GROUP BY #{scoped_group}"
-              sql << " HAVING #{scope[:having]}" if scope[:having]
+              sql << " HAVING #{sanitize_sql_for_conditions(scope[:having])}" if scope[:having]
             end
           end
         end

diff --git a/activerecord/lib/active_record/calculations.rb b/activerecord/lib/active_record/calculations.rb
@@ -214,13 +214,15 @@ def construct_calculation_sql(operation, column_name, options) #:nodoc:
           end
 
           if options[:group] && options[:having]
+            having = sanitize_sql_for_conditions(options[:having])
+
             # FrontBase requires identifiers in the HAVING clause and chokes on function calls
             if connection.adapter_name == 'FrontBase'
-              options[:having].downcase!
-              options[:having].gsub!(/#{operation}\s*\(\s*#{column_name}\s*\)/, aggregate_alias)
+              having.downcase!
+              having.gsub!(/#{operation}\s*\(\s*#{column_name}\s*\)/, aggregate_alias)
             end
 
-            sql << " HAVING #{options[:having]} "
+            sql << " HAVING #{having} "
           end
 
           sql << " ORDER BY #{options[:order]} "       if options[:order]

diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb
@@ -92,6 +92,14 @@ def test_should_group_by_summed_field_having_condition
     assert_equal 60,  c[2]
   end
 
+  def test_should_group_by_summed_field_having_sanitized_condition
+    c = Account.sum(:credit_limit, :group => :firm_id,
+                                   :having => ['sum(credit_limit) > ?', 50])
+    assert_nil        c[1]
+    assert_equal 105, c[6]
+    assert_equal 60,  c[2]
+  end
+
   def test_should_group_by_summed_association
     c = Account.sum(:credit_limit, :group => :firm)
     assert_equal 50,   c[companies(:first_firm)]

diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb
@@ -191,6 +191,13 @@ def test_find_with_group_and_having
     assert developers.all? { |developer|  developer.salary > 10000 }
   end
 
+  def test_find_with_group_and_sanitized_having
+    developers =  Developer.find(:all, :group => "salary", :having => ["sum(salary) > ?", 10000], :select => "salary")
+    assert_equal 3, developers.size
+    assert_equal 3, developers.map(&:salary).uniq.size
+    assert developers.all? { |developer|  developer.salary > 10000 }
+  end
+
   def test_find_with_entire_select_statement
     topics = Topic.find_by_sql "SELECT * FROM topics WHERE author_name = 'Mary'"