Merge pull request #9414 from senny/9275_order_with_symbol_and_join

Expand order(:symbol) to "table".symbol to prevent broken queries on PG.

4 files changed, 34 insertions, 2 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index a135b3c91f..3fbf043c7c 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -4,6 +4,15 @@
 
     *Hiroshige UMINO*
 
+*   Fix when performing an ordered join query. The bug only
+    affected queries where the order was given with a symbol.
+    Fixes #9275.
+
+    Example:
+
+        # This will expand the order :name to "authors".name.
+        Author.joins(:books).where('books.published = 1').order(:name)
+
 *   Fixing issue #8345. Now throwing an error when one attempts to touch a
     new object that has not yet been persisted. For instance:
 
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 225677085f..4b8c40592e 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -285,6 +285,11 @@ module ActiveRecord
       references.map! { |arg| arg =~ /^([a-zA-Z]\w*)\.(\w+)/ && $1 }.compact!
       references!(references) if references.any?
 
+      # if a symbol is given we prepend the quoted table name
+      args = args.map { |arg|
+        arg.is_a?(Symbol) ? "#{quoted_table_name}.#{arg} ASC" : arg
+      }
+
       self.order_values = args + self.order_values
       self
     end
diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb
index 6ed3cb7b58..1de7ee0846 100644
--- a/activerecord/test/cases/associations/eager_test.rb
+++ b/activerecord/test/cases/associations/eager_test.rb
@@ -1173,4 +1173,9 @@ class EagerAssociationTest < ActiveRecord::TestCase
     assert_no_queries { assert_equal 2, author.comments_with_order_and_conditions.size }
     assert_no_queries { assert_equal 5, author.posts.size, "should not cache a subset of the association" }
   end
+
+  test "works in combination with order(:symbol)" do
+    author = Author.includes(:posts).references(:posts).order(:name).where('posts.title IS NOT NULL').first
+    assert_equal authors(:bob), author
+  end
 end
diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb
index 92dc575d37..fd0b05cb77 100644
--- a/activerecord/test/cases/relation_test.rb
+++ b/activerecord/test/cases/relation_test.rb
@@ -180,19 +180,32 @@ module ActiveRecord
 
   class RelationMutationTest < ActiveSupport::TestCase
     class FakeKlass < Struct.new(:table_name, :name)
+      def quoted_table_name
+        %{"#{table_name}"}
+      end
     end
 
     def relation
-      @relation ||= Relation.new FakeKlass, :b
+      @relation ||= Relation.new FakeKlass.new('posts'), :b
     end
 
-    (Relation::MULTI_VALUE_METHODS - [:references, :extending]).each do |method|
+    (Relation::MULTI_VALUE_METHODS - [:references, :extending, :order]).each do |method|
       test "##{method}!" do
         assert relation.public_send("#{method}!", :foo).equal?(relation)
         assert_equal [:foo], relation.public_send("#{method}_values")
       end
     end
 
+    test "#order!" do
+      assert relation.order!('name ASC').equal?(relation)
+      assert_equal ['name ASC'], relation.order_values
+    end
+
+    test "#order! with symbol prepends the table name" do
+      assert relation.order!(:name).equal?(relation)
+      assert_equal ['"posts".name ASC'], relation.order_values
+    end
+
     test '#references!' do
       assert relation.references!(:foo).equal?(relation)
       assert relation.references_values.include?('foo')