Wrap :conditions in parentheses to prevent problems with OR's #1871

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@2324 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

7 files changed, 39 insertions, 11 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index cce58f7587..1983a18e50 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Wrap :conditions in parentheses to prevent problems with OR's #1871
+
 * Allow the postgresql adapter to work with the SchemaDumper.
 
 * Add ActiveRecord::SchemaDumper for dumping a DB schema to a pure-ruby file, making it easier to consolidate large migration lists and port database schemas between databases.
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index fdeadfc181..f9ca655560 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -68,7 +68,7 @@ module ActiveRecord
         else
           conditions = "#{@finder_sql}"
           if sanitized_conditions = sanitize_sql(options[:conditions])
-            conditions << " AND #{sanitized_conditions}"
+            conditions << " AND (#{sanitized_conditions})"
           end
           options[:conditions] = conditions
 
diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index 03123758ef..6f655df26f 100755
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -350,7 +350,7 @@ module ActiveRecord #:nodoc:
             return args.first if args.first.kind_of?(Array) && args.first.empty?
             expects_array = args.first.kind_of?(Array)
             
-            conditions = " AND #{sanitize_sql(options[:conditions])}" if options[:conditions]
+            conditions = " AND (#{sanitize_sql(options[:conditions])})" if options[:conditions]
 
             ids = args.flatten.compact.uniq
             case ids.size
diff --git a/activerecord/test/associations_go_eager_test.rb b/activerecord/test/associations_go_eager_test.rb
index 750e8bd208..ec088eebb7 100644
--- a/activerecord/test/associations_go_eager_test.rb
+++ b/activerecord/test/associations_go_eager_test.rb
@@ -20,14 +20,19 @@ class EagerAssociationTest < Test::Unit::TestCase
     assert post.comments.include?(comments(:greetings))
   end
 
+  def test_loading_conditions_with_or
+    posts = authors(:david).posts.find(:all, :include => :comments, :conditions => "comments.body like 'Normal%' OR comments.type = 'SpecialComment'")
+    assert_nil posts.detect { |p| p.author_id != authors(:david).id },
+      "expected to find only david's posts"
+  end
+
   def test_with_ordering
-    posts = Post.find(:all, :include => :comments, :order => "posts.id DESC")
-    assert_equal posts(:sti_habtm), posts[0]
-    assert_equal posts(:sti_post_and_comments), posts[1]
-    assert_equal posts(:sti_comments), posts[2]
-    assert_equal posts(:authorless), posts[3]
-    assert_equal posts(:thinking), posts[4]
-    assert_equal posts(:welcome), posts[5]
+    list = Post.find(:all, :include => :comments, :order => "posts.id DESC")
+    [:eager_other, :sti_habtm, :sti_post_and_comments, :sti_comments,
+     :authorless, :thinking, :welcome
+    ].each_with_index do |post, index|
+      assert_equal posts(post), list[index]
+    end
   end
 
   def test_loading_with_multiple_associations
@@ -48,7 +53,7 @@ class EagerAssociationTest < Test::Unit::TestCase
 
   def test_eager_association_loading_with_belongs_to
     comments = Comment.find(:all, :include => :post)
-    assert_equal 9, comments.length
+    assert_equal 10, comments.length
     titles = comments.map { |c| c.post.title }
     assert titles.include?(posts(:welcome).title)
     assert titles.include?(posts(:sti_post_and_comments).title)
diff --git a/activerecord/test/finder_test.rb b/activerecord/test/finder_test.rb
index 8fc89f3808..35c96288fe 100644
--- a/activerecord/test/finder_test.rb
+++ b/activerecord/test/finder_test.rb
@@ -3,9 +3,10 @@ require 'fixtures/company'
 require 'fixtures/topic'
 require 'fixtures/entrant'
 require 'fixtures/developer'
+require 'fixtures/post'
 
 class FinderTest < Test::Unit::TestCase
-  fixtures :companies, :topics, :entrants, :developers
+  fixtures :companies, :topics, :entrants, :developers, :posts
 
   def test_find
     assert_equal(topics(:first).title, Topic.find(1).title)
@@ -311,6 +312,13 @@ class FinderTest < Test::Unit::TestCase
     assert developer_names.include?('Jamis')
   end
 
+  def test_find_by_id_with_conditions_with_or
+    assert_nothing_raised do
+      Post.find([1,2,3],
+        :conditions => "posts.id <= 3 OR posts.type = 'Post'")
+    end
+  end
+
   def test_select_value
     assert_equal "37signals", Company.connection.select_value("SELECT name FROM companies WHERE id = 1")
     assert_nil Company.connection.select_value("SELECT name FROM companies WHERE id = -1")
diff --git a/activerecord/test/fixtures/comments.yml b/activerecord/test/fixtures/comments.yml
index bfe61068e1..758eaf6dbf 100644
--- a/activerecord/test/fixtures/comments.yml
+++ b/activerecord/test/fixtures/comments.yml
@@ -57,3 +57,9 @@ check_eager_sti_on_associations2:
   post_id: 5
   body: Special Type
   type: SpecialComment
+
+eager_other_comment1:
+  id: 11
+  post_id: 7
+  body: go crazy
+  type: SpecialComment
diff --git a/activerecord/test/fixtures/posts.yml b/activerecord/test/fixtures/posts.yml
index 190993dcdd..0f1445b638 100644
--- a/activerecord/test/fixtures/posts.yml
+++ b/activerecord/test/fixtures/posts.yml
@@ -39,3 +39,10 @@ sti_habtm:
   title: habtm sti test
   body: hello
   type: Post
+
+eager_other:
+  id: 7
+  author_id: 2
+  title: eager loading with OR'd conditions
+  body: hello
+  type: Post