Permalink
Browse files

use arel nodes to represent non-string `order_values`.

This fixes a bug when merging relations of different classes.

```
Given:
  Post.joins(:author).merge(Author.order(name: :desc)).to_sql

Before:
 SELECT "posts".* FROM "posts"
   INNER JOIN "authors" ON "authors"."id" = "posts"."author_id"
   ORDER BY "posts"."name" DESC

After:
 SELECT "posts".* FROM "posts"
   INNER JOIN "authors" ON "authors"."id" = "posts"."author_id"
   ORDER BY "authors"."name" DESC
```
  • Loading branch information...
1 parent a7afcee commit f83c9b10b4c92b0d8deacb30d6fdfa2b1252d6dd @senny senny committed Nov 13, 2013
View
4 activerecord/CHANGELOG.md
@@ -1,3 +1,7 @@
+* Use strings to represent non-string `order_values`.
+
+ *Yves Senn*
+
* Checks to see if the record contains the foreign key to set the inverse automatically.
*Edo Balvers*
View
30 activerecord/lib/active_record/relation/query_methods.rb
@@ -995,12 +995,6 @@ def reverse_sql_order(order_query)
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
@@ -1016,17 +1010,6 @@ def build_order(arel)
orders.reject!(&:blank?)
orders = reverse_sql_order(orders) if reverse_order_value
- orders = orders.flat_map do |order|
- case order
- when Symbol
- table[order].asc
- when Hash
- order.map { |field, dir| table[field].send(dir) }
- else
- order
- end
- end
-
arel.order(*orders) unless orders.empty?
end
@@ -1048,8 +1031,17 @@ def preprocess_order_args(order_args)
# if a symbol is given we prepend the quoted table name
order_args.map! do |arg|
- arg.is_a?(Symbol) ? Arel::Nodes::Ascending.new(klass.arel_table[arg]) : arg
- end
+ case arg
+ when Symbol
+ table[arg].asc
+ when Hash
+ arg.map { |field, dir|
+ table[field].send(dir)
+ }
+ else
+ arg
+ end
+ end.flatten!
end
# Checks to make sure that the arguments are not blank. Note that if some
View
12 activerecord/test/cases/relation/merging_test.rb
@@ -146,5 +146,17 @@ class MergingDifferentRelationsTest < ActiveRecord::TestCase
merge(Author.order(:name)).pluck("authors.name")
assert_equal ["Bob", "Bob", "David"], posts_by_author_name
+
+ posts_by_author_name = Post.limit(3).joins(:author).
+ merge(Author.order("name")).pluck("authors.name")
+
+ assert_equal ["Bob", "Bob", "David"], posts_by_author_name
+ end
+
+ test "merging order relations (using a hash argument)" do
+ posts_by_author_name = Post.limit(4).joins(:author).
+ merge(Author.order(name: :desc)).pluck("authors.name")
+
+ assert_equal ["Mary", "Mary", "Mary", "David"], posts_by_author_name
end
end
View
6 activerecord/test/cases/relation/mutation_test.rb
@@ -7,10 +7,6 @@ class FakeKlass < Struct.new(:table_name, :name)
extend ActiveRecord::Delegation::DelegateCache
inherited self
- def arel_table
- Post.arel_table
- end
-
def connection
Post.connection
end
@@ -21,7 +17,7 @@ def relation_delegate_class(klass)
end
def relation
- @relation ||= Relation.new FakeKlass.new('posts'), :b
+ @relation ||= Relation.new FakeKlass.new('posts'), Post.arel_table
end
(Relation::MULTI_VALUE_METHODS - [:references, :extending, :order]).each do |method|

2 comments on commit f83c9b1

@senny
Ruby on Rails member

I already discussed this change with @rafaelfranca. I was not sure if there was a reason to not store the nodes directly in order_values. Since d345ed4 already stored some nodes directly I thought it should be ok.

This not only simplifies the implementation but also solves a bug when merging relations of different tables. See the test-case for an example.

As it includes a bugfix I'd like to backport the patch.

@tenderlove Can you take a look and see if this patch breaks anything?

@tenderlove
Ruby on Rails member

Looks good to me. 👍 if the tests pass!

Please sign in to comment.