From bbec3ae512290eaee7942d3c073285f69c7ecf0a Mon Sep 17 00:00:00 2001
From: Marcel Molina <marcel@vernix.org>
Date: Wed, 4 Jan 2006 03:43:28 +0000
Subject: [PATCH] Sanitize scoped conditions.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@3379 5ecf4fe2-1ee6-0310-87b1-e25e094e27de
---
 activerecord/CHANGELOG                    |  2 ++
 activerecord/lib/active_record/base.rb    |  2 +-
 activerecord/test/base_test.rb            | 19 ++++++++-----------
 activerecord/test/fixtures/developers.yml |  7 ++++++-
 activerecord/test/fixtures_test.rb        |  2 +-
 activerecord/test/method_scoping_test.rb  | 20 +++++++++++++++++++-
 6 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index bdd384bb3a..3b53432cc6 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Sanitize scoped conditions. [Marcel Molina Jr.]
+
 * Added option to Base.reflection_of_all_associations to specify a specific association to scope the call. For example Base.reflection_of_all_associations(:has_many) [DHH]
 
 * Added ActiveRecord::SchemaDumper.ignore_tables which tells SchemaDumper which tables to ignore. Useful for tables with funky column like the ones required for tsearch2. [TobiasLuetke]
diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index a08cd0f2df..94bdce5b19 100755
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -944,7 +944,7 @@ module ActiveRecord #:nodoc:
 
         # Adds a sanitized version of +conditions+ to the +sql+ string. Note that the passed-in +sql+ string is changed.
         def add_conditions!(sql, conditions)          
-          segments = [scope(:find, :conditions)]
+          segments = [sanitize_sql(scope(:find, :conditions))]
           segments << sanitize_sql(conditions) unless conditions.nil?
           segments << type_condition unless descends_from_active_record?        
           segments.compact!
diff --git a/activerecord/test/base_test.rb b/activerecord/test/base_test.rb
index a7b1521d89..9b5d053a98 100755
--- a/activerecord/test/base_test.rb
+++ b/activerecord/test/base_test.rb
@@ -1082,27 +1082,24 @@ class BasicsTest < Test::Unit::TestCase
   end
 
   def test_scoped_find_conditions
-    developers = Developer.with_scope(:find => { :conditions => 'salary > 90000' }) do
+    scoped_developers = Developer.with_scope(:find => { :conditions => 'salary > 90000' }) do
       Developer.find(:all, :conditions => 'id < 5')
     end
-    david = Developer.find(1)
-    assert !developers.include?(david) # David's salary is less than 90,000
-    assert_equal 3, developers.size
+    assert !scoped_developers.include?(developers(:david)) # David's salary is less than 90,000
+    assert_equal 3, scoped_developers.size
   end
   
   def test_scoped_find_limit_offset
-    developers = Developer.with_scope(:find => { :limit => 3, :offset => 2 }) do
+    scoped_developers = Developer.with_scope(:find => { :limit => 3, :offset => 2 }) do
       Developer.find(:all, :order => 'id')
     end    
-    david = Developer.find(1)
-    jamis = Developer.find(1)
-    assert !developers.include?(david) # David has id 1
-    assert !developers.include?(jamis) # Jamis has id 2
-    assert_equal 3, developers.size
+    assert !scoped_developers.include?(developers(:david))
+    assert !scoped_developers.include?(developers(:jamis))
+    assert_equal 3, scoped_developers.size
     
     # Test without scoped find conditions to ensure we get the whole thing
     developers = Developer.find(:all, :order => 'id')
-    assert_equal 10, developers.size
+    assert_equal Developer.count, developers.size
   end
   
   # FIXME: this test ought to run, but it needs to run sandboxed so that it
diff --git a/activerecord/test/fixtures/developers.yml b/activerecord/test/fixtures/developers.yml
index fc33af99b6..308bf75de2 100644
--- a/activerecord/test/fixtures/developers.yml
+++ b/activerecord/test/fixtures/developers.yml
@@ -13,4 +13,9 @@ dev_<%= digit %>:
   id: <%= digit %>
   name: fixture_<%= digit %>
   salary: 100000
-<% end %>
\ No newline at end of file
+<% end %>
+
+poor_jamis:
+  id: 11
+  name: Jamis
+  salary: 9000
\ No newline at end of file
diff --git a/activerecord/test/fixtures_test.rb b/activerecord/test/fixtures_test.rb
index 813291aa18..19664d2d34 100755
--- a/activerecord/test/fixtures_test.rb
+++ b/activerecord/test/fixtures_test.rb
@@ -136,7 +136,7 @@ class FixturesTest < Test::Unit::TestCase
   end
 
   def test_erb_in_fixtures
-    assert_equal 10, @developers.size
+    assert_equal 11, @developers.size
     assert_equal "fixture_5", @dev_5.name
   end
 
diff --git a/activerecord/test/method_scoping_test.rb b/activerecord/test/method_scoping_test.rb
index 5c62dc399f..6e843eab0b 100644
--- a/activerecord/test/method_scoping_test.rb
+++ b/activerecord/test/method_scoping_test.rb
@@ -25,9 +25,27 @@ class MethodScopingTest < Test::Unit::TestCase
     end
   end
   
+  def test_scoped_find_combines_conditions
+    Developer.with_scope(:find => { :conditions => "salary = 9000" }) do
+      assert_equal developers(:poor_jamis), Developer.find(:first, :conditions => "name = 'Jamis'")
+    end
+  end
+  
+  def test_scoped_find_sanitizes_conditions
+    Developer.with_scope(:find => { :conditions => ['salary = ?', 9000] }) do
+      assert_equal developers(:poor_jamis), Developer.find(:first)
+    end  
+  end
+  
+  def test_scoped_find_combines_and_sanitizes_conditions
+    Developer.with_scope(:find => { :conditions => ['salary = ?', 9000] }) do
+      assert_equal developers(:poor_jamis), Developer.find(:first, :conditions => ['name = ?', 'Jamis'])
+    end  
+  end
+  
   def test_scoped_find_all
     Developer.with_scope(:find => { :conditions => "name = 'David'" }) do
-      assert_equal [Developer.find(1)], Developer.find(:all)
+      assert_equal [developers(:david)], Developer.find(:all)
     end      
   end