Change relation merging to always append select, group and order values

5 files changed, 28 insertions, 55 deletions

diff --git a/activerecord/lib/active_record/relation/spawn_methods.rb b/activerecord/lib/active_record/relation/spawn_methods.rb
index e7e5f26ce0..4b2c647a26 100644
--- a/activerecord/lib/active_record/relation/spawn_methods.rb
+++ b/activerecord/lib/active_record/relation/spawn_methods.rb
@@ -6,8 +6,9 @@ module ActiveRecord
       merged_relation = clone
       return merged_relation unless r
 
-      ((Relation::ASSOCIATION_METHODS + Relation::MULTI_VALUE_METHODS) - [:joins, :where]).each do |method|
+      Relation::ASSOCIATION_METHODS.each do |method|
         value = r.send(:"#{method}_values")
+
         unless value.empty?
           if method == :includes
             merged_relation = merged_relation.includes(value)
@@ -17,15 +18,18 @@ module ActiveRecord
         end
       end
 
+      (Relation::MULTI_VALUE_METHODS - [:joins, :where]).each do |method|
+        value = r.send(:"#{method}_values")
+        merged_relation.send(:"#{method}_values=", merged_relation.send(:"#{method}_values") + value) if value.present?
+      end
+
       merged_relation = merged_relation.joins(r.joins_values)
 
       merged_wheres = @where_values
 
       r.where_values.each do |w|
         if w.respond_to?(:operator) && w.operator == :==
-          merged_wheres = merged_wheres.reject { |p|
-            p.respond_to?(:operator) && p.operator == :== && p.operand1.name == w.operand1.name
-          }
+          merged_wheres = merged_wheres.reject {|p| p.respond_to?(:operator) && p.operator == :== && p.operand1.name == w.operand1.name }
         end
 
         merged_wheres += [w]
@@ -34,9 +38,8 @@ module ActiveRecord
       merged_relation.where_values = merged_wheres
 
       Relation::SINGLE_VALUE_METHODS.reject {|m| m == :lock}.each do |method|
-        unless (value = r.send(:"#{method}_value")).nil?
-          merged_relation.send(:"#{method}_value=", value)
-        end
+        value = r.send(:"#{method}_value")
+        merged_relation.send(:"#{method}_value=", value) unless value.nil?
       end
 
       merged_relation.lock_value = r.lock_value unless merged_relation.lock_value
diff --git a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
index c726fedd13..8bb8edde0e 100644
--- a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
+++ b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
@@ -567,16 +567,6 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase
     assert_equal high_id_jamis, projects(:active_record).developers.find_by_name('Jamis')
   end
 
-  def test_dynamic_find_order_should_override_association_order
-    # Developers are ordered 'name DESC, id DESC'
-    low_id_jamis = developers(:jamis)
-    middle_id_jamis = developers(:poor_jamis)
-    high_id_jamis = projects(:active_record).developers.create(:name => 'Jamis')
-
-    assert_equal low_id_jamis, projects(:active_record).developers.find(:first, :conditions => "name = 'Jamis'", :order => 'id')
-    assert_equal low_id_jamis, projects(:active_record).developers.find_by_name('Jamis', :order => 'id')
-  end
-
   def test_dynamic_find_all_should_respect_association_order
     # Developers are ordered 'name DESC, id DESC'
     low_id_jamis = developers(:jamis)
@@ -587,14 +577,9 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase
     assert_equal [high_id_jamis, middle_id_jamis, low_id_jamis], projects(:active_record).developers.find_all_by_name('Jamis')
   end
 
-  def test_dynamic_find_all_order_should_override_association_order
-    # Developers are ordered 'name DESC, id DESC'
-    low_id_jamis = developers(:jamis)
-    middle_id_jamis = developers(:poor_jamis)
-    high_id_jamis = projects(:active_record).developers.create(:name => 'Jamis')
-
-    assert_equal [low_id_jamis, middle_id_jamis, high_id_jamis], projects(:active_record).developers.find(:all, :conditions => "name = 'Jamis'", :order => 'id')
-    assert_equal [low_id_jamis, middle_id_jamis, high_id_jamis], projects(:active_record).developers.find_all_by_name('Jamis', :order => 'id')
+  def test_find_should_append_to_association_order
+    ordered_developers = projects(:active_record).developers.order('projects.id')
+    assert_equal ['developers.name desc, developers.id desc', 'projects.id'], ordered_developers.order_values
   end
 
   def test_dynamic_find_all_should_respect_association_limit
@@ -625,11 +610,10 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase
   end
 
   def test_find_in_association_with_options
-    developers = projects(:active_record).developers.find(:all)
+    developers = projects(:active_record).developers.all
     assert_equal 3, developers.size
 
-    assert_equal developers(:poor_jamis), projects(:active_record).developers.find(:first, :conditions => "salary < 10000")
-    assert_equal developers(:jamis),      projects(:active_record).developers.find(:first, :order => "salary DESC")
+    assert_equal developers(:poor_jamis), projects(:active_record).developers.where("salary < 10000").first
   end
 
   def test_replace_with_less
diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb
index 63fc15bca3..efabf74e13 100644
--- a/activerecord/test/cases/associations/has_many_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_associations_test.rb
@@ -147,6 +147,11 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert_equal 2, companies(:first_firm).limited_clients.find(:all, :limit => nil).size
   end
 
+  def test_find_should_append_to_association_order
+    ordered_clients =  companies(:first_firm).clients_sorted_desc.order('companies.id')
+    assert_equal ['id DESC', 'companies.id'], ordered_clients.order_values
+  end
+
   def test_dynamic_find_last_without_specified_order
     assert_equal companies(:second_client), companies(:first_firm).unsorted_clients.find_last_by_type('Client')
   end
@@ -156,21 +161,11 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert_equal companies(:second_client), companies(:first_firm).clients_sorted_desc.find_by_type('Client')
   end
 
-  def test_dynamic_find_order_should_override_association_order
-    assert_equal companies(:first_client), companies(:first_firm).clients_sorted_desc.find(:first, :conditions => "type = 'Client'", :order => 'id')
-    assert_equal companies(:first_client), companies(:first_firm).clients_sorted_desc.find_by_type('Client', :order => 'id')
-  end
-
   def test_dynamic_find_all_should_respect_association_order
     assert_equal [companies(:second_client), companies(:first_client)], companies(:first_firm).clients_sorted_desc.find(:all, :conditions => "type = 'Client'")
     assert_equal [companies(:second_client), companies(:first_client)], companies(:first_firm).clients_sorted_desc.find_all_by_type('Client')
   end
 
-  def test_dynamic_find_all_order_should_override_association_order
-    assert_equal [companies(:first_client), companies(:second_client)], companies(:first_firm).clients_sorted_desc.find(:all, :conditions => "type = 'Client'", :order => 'id')
-    assert_equal [companies(:first_client), companies(:second_client)], companies(:first_firm).clients_sorted_desc.find_all_by_type('Client', :order => 'id')
-  end
-
   def test_dynamic_find_all_should_respect_association_limit
     assert_equal 1, companies(:first_firm).limited_clients.find(:all, :conditions => "type = 'Client'").length
     assert_equal 1, companies(:first_firm).limited_clients.find_all_by_type('Client').length
@@ -1018,21 +1013,11 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert_equal Comment.find(10), authors(:david).comments_desc.find_by_type('SpecialComment')
   end
 
-  def test_dynamic_find_order_should_override_association_order_for_through
-    assert_equal Comment.find(3), authors(:david).comments_desc.find(:first, :conditions => "comments.type = 'SpecialComment'", :order => 'comments.id')
-    assert_equal Comment.find(3), authors(:david).comments_desc.find_by_type('SpecialComment', :order => 'comments.id')
-  end
-
   def test_dynamic_find_all_should_respect_association_order_for_through
     assert_equal [Comment.find(10), Comment.find(7), Comment.find(6), Comment.find(3)], authors(:david).comments_desc.find(:all, :conditions => "comments.type = 'SpecialComment'")
     assert_equal [Comment.find(10), Comment.find(7), Comment.find(6), Comment.find(3)], authors(:david).comments_desc.find_all_by_type('SpecialComment')
   end
 
-  def test_dynamic_find_all_order_should_override_association_order_for_through
-    assert_equal [Comment.find(3), Comment.find(6), Comment.find(7), Comment.find(10)], authors(:david).comments_desc.find(:all, :conditions => "comments.type = 'SpecialComment'", :order => 'comments.id')
-    assert_equal [Comment.find(3), Comment.find(6), Comment.find(7), Comment.find(10)], authors(:david).comments_desc.find_all_by_type('SpecialComment', :order => 'comments.id')
-  end
-
   def test_dynamic_find_all_should_respect_association_limit_for_through
     assert_equal 1, authors(:david).limited_comments.find(:all, :conditions => "comments.type = 'SpecialComment'").length
     assert_equal 1, authors(:david).limited_comments.find_all_by_type('SpecialComment').length
diff --git a/activerecord/test/cases/relation_scoping_test.rb b/activerecord/test/cases/relation_scoping_test.rb
index d56e5a7999..cdfd62a675 100644
--- a/activerecord/test/cases/relation_scoping_test.rb
+++ b/activerecord/test/cases/relation_scoping_test.rb
@@ -363,14 +363,15 @@ class DefaultScopingTest < ActiveRecord::TestCase
     assert_equal "David", klass.first.name
     assert_equal 100000,  klass.first.salary
   end
+
   def test_method_scope
-    expected = Developer.find(:all, :order => 'name DESC').collect { |dev| dev.salary }
+    expected = Developer.find(:all, :order => 'salary DESC, name DESC').collect { |dev| dev.salary }
     received = DeveloperOrderedBySalary.all_ordered_by_name.collect { |dev| dev.salary }
     assert_equal expected, received
   end
 
   def test_nested_scope
-    expected = Developer.find(:all, :order => 'name DESC').collect { |dev| dev.salary }
+    expected = Developer.find(:all, :order => 'salary DESC, name DESC').collect { |dev| dev.salary }
     received = DeveloperOrderedBySalary.send(:with_scope, :find => { :order => 'name DESC'}) do
       DeveloperOrderedBySalary.find(:all).collect { |dev| dev.salary }
     end
@@ -378,14 +379,14 @@ class DefaultScopingTest < ActiveRecord::TestCase
   end
 
   def test_named_scope_overwrites_default
-    expected = Developer.find(:all, :order => 'name DESC').collect { |dev| dev.name }
+    expected = Developer.find(:all, :order => 'salary DESC, name DESC').collect { |dev| dev.name }
     received = DeveloperOrderedBySalary.by_name.find(:all).collect { |dev| dev.name }
     assert_equal expected, received
   end
 
-  def test_named_scope_reorders_default
-    expected = Developer.find(:all, :order => 'name DESC').collect { |dev| dev.name }
-    received = DeveloperOrderedBySalary.reordered_by_name.find(:all).collect { |dev| dev.name }
+  def test_reorder_overrides_default_scope_order
+    expected = Developer.order('name DESC').collect { |dev| dev.name }
+    received = DeveloperOrderedBySalary.reorder('name DESC').collect { |dev| dev.name }
     assert_equal expected, received
   end
 
diff --git a/activerecord/test/models/developer.rb b/activerecord/test/models/developer.rb
index a140fb8e57..f0d40e741b 100644
--- a/activerecord/test/models/developer.rb
+++ b/activerecord/test/models/developer.rb
@@ -87,7 +87,7 @@ end
 class DeveloperOrderedBySalary < ActiveRecord::Base
   self.table_name = 'developers'
   default_scope :order => 'salary DESC'
-  scope :by_name, :order => 'name DESC'
+  scope :by_name, order('name DESC')
   scope :reordered_by_name, reorder('name DESC')
 
   def self.all_ordered_by_name