learn ActiveRecord::QueryMethods#order work with hash arguments

3 files changed, 73 insertions, 4 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 9abfe2e6fd..7b80598171 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,5 +1,15 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   Learn ActiveRecord::QueryMethods#order work with hash arguments
+
+    When symbol or hash passed we convert it to Arel::Nodes::Ordering.
+    If we pass invalid direction(like name: :DeSc) ActiveRecord::QueryMethods#order will raise an exception
+    
+        User.order(:name, email: :desc)
+        # SELECT "users".* FROM "users" ORDER BY "users"."name" ASC, "users"."email" DESC
+
+    *Tima Maslyuchenko*
+
 *   Rename `ActiveRecord::Fixtures` class to `ActiveRecord::FixtureSet`.
     Instances of this class normally hold a collection of fixtures (records)
     loaded either from a single YAML file, or from a file and a folder
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 3c59bd8a68..2b12e66ed0 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -202,6 +202,15 @@ module ActiveRecord
     #
     #   User.order('name DESC, email')
     #   => SELECT "users".* FROM "users" ORDER BY name DESC, email
+    # 
+    #   User.order(:name)
+    #   => SELECT "users".* FROM "users" ORDER BY "users"."name" ASC
+    #   
+    #   User.order(email: :desc)
+    #   => SELECT "users".* FROM "users" ORDER BY "users"."email" DESC
+    #   
+    #   User.order(:name, email: :desc)
+    #   => SELECT "users".* FROM "users" ORDER BY "users"."name" ASC, "users"."email" DESC
     def order(*args)
       args.blank? ? self : spawn.order!(*args)
     end
@@ -209,6 +218,8 @@ module ActiveRecord
     # Like #order, but modifies relation in place.
     def order!(*args)
       args.flatten!
+      
+      validate_order_args args
 
       references = args.reject { |arg| Arel::Node === arg }
       references.map! { |arg| arg =~ /^([a-zA-Z]\w*)\.(\w+)/ && $1 }.compact!
@@ -234,6 +245,8 @@ module ActiveRecord
     # Like #reorder, but modifies relation in place.
     def reorder!(*args)
       args.flatten!
+      
+      validate_order_args args
 
       self.reordering_value = true
       self.order_values = args
@@ -658,9 +671,7 @@ module ActiveRecord
 
       arel.group(*group_values.uniq.reject{|g| g.blank?}) unless group_values.empty?
 
-      order = order_values
-      order = reverse_sql_order(order) if reverse_order_value
-      arel.order(*order.uniq.reject{|o| o.blank?}) unless order.empty?
+      build_order(arel)
 
       build_select(arel, select_values.uniq)
 
@@ -786,11 +797,17 @@ module ActiveRecord
         case o
         when Arel::Nodes::Ordering
           o.reverse
-        when String, Symbol
+        when String
           o.to_s.split(',').collect do |s|
             s.strip!
             s.gsub!(/\sasc\Z/i, ' DESC') || s.gsub!(/\sdesc\Z/i, ' ASC') || s.concat(' DESC')
           end
+        when Symbol
+          { o => :desc } 
+        when Hash
+          o.each_with_object({}) do |(field, dir), memo| 
+            memo[field] = (dir == :asc ? :desc : :asc )
+          end
         else
           o
         end
@@ -800,6 +817,32 @@ module ActiveRecord
     def array_of_strings?(o)
       o.is_a?(Array) && o.all?{|obj| obj.is_a?(String)}
     end
+    
+    def build_order(arel)
+      orders = order_values
+      orders = reverse_sql_order(orders) if reverse_order_value
+      
+      orders = orders.uniq.reject(&:blank?).map do |order|
+        case order
+        when Symbol
+          table[order].asc
+        when Hash
+          order.map { |field, dir| table[field].send(dir) }
+        else    
+          order
+        end
+      end.flatten
+      
+      arel.order(*orders) unless orders.empty?
+    end
+    
+    def validate_order_args(args)
+      args.select { |a| Hash === a  }.each do |h|
+        unless (h.values - [:asc, :desc]).empty?
+          raise ArgumentError, 'Direction should be :asc or :desc'
+        end
+      end
+    end
 
   end
 end
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index d801770118..fdbc0a3fdb 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -157,6 +157,22 @@ class RelationTest < ActiveRecord::TestCase
     assert_equal 4, topics.to_a.size
     assert_equal topics(:first).title, topics.first.title
   end
+  
+  def test_finding_with_assoc_order
+    topics = Topic.order(:id => :desc)
+    assert_equal 4, topics.to_a.size
+    assert_equal topics(:fourth).title, topics.first.title
+  end
+  
+  def test_finding_with_reverted_assoc_order
+    topics = Topic.order(:id => :asc).reverse_order
+    assert_equal 4, topics.to_a.size
+    assert_equal topics(:fourth).title, topics.first.title
+  end
+  
+  def test_raising_exception_on_invalid_hash_params
+    assert_raise(ArgumentError) { Topic.order(:name, "id DESC", :id => :DeSc) }
+  end
 
   def test_finding_last_with_arel_order
     topics = Topic.order(Topic.arel_table[:id].asc)