ActiveRecord::QueryMethods#order and ActiveRecord::QueryMethods#reorder doesn't work on association columns when :asc or :desc is specified #14311

Closed
jhephs opened this Issue Mar 7, 2014 · 3 comments

Projects

None yet

3 participants

@jhephs
jhephs commented Mar 7, 2014

I am using Rails 4.1.0.rc1 and I have the following:

class User < ActiveRecord::Base
  default_scope { order(:created_at => :desc) }
  has_many :orders
end

class Order < ActiveRecord::Base
  belongs_to :user
end
> User.includes(:orders).order('orders.id' => :asc).to_sql
 => "SELECT `users`.* FROM `users`   ORDER BY `users`.`created_at` DESC, `users`.`orders.id` ASC"
> User.includes(:orders).reorder('orders.id' => :asc).to_sql
 => "SELECT `users`.* FROM `users`   ORDER BY `users`.`orders.id` ASC"

As can be seen above, orders.id is being assumed to be a column of the users table. Unlike below, where it's showing the expected order.

> User.includes(:orders).order('orders.id').to_sql
 => "SELECT `users`.* FROM `users` LEFT OUTER JOIN `orders` ON `orders`.`user_id` = `users`.`id`  ORDER BY `users`.`created_at` DESC, orders.id"
> User.includes(:orders).reorder('orders.id').to_sql
"SELECT `users`.* FROM `users` LEFT OUTER JOIN `orders` ON `orders`.`user_id` = `users`.`id`  ORDER BY orders.id"

The output above has been edited for readability.

@robin850
Member
robin850 commented Mar 7, 2014

Hello @jerielabayon,

Thanks for reporting the problem but it looks like your example is not expected to work according to ActiveRecord::Associations::ClassMethods' documentation:

# Since only one table is loaded at a time, conditions or orders cannot reference tables
# other than the main one. If this is the case Active Record falls back to the previously
# used LEFT OUTER JOIN based strategy.
# ...
# You must disambiguate column references for this fallback to happen, for example
# <tt>order: "author.name DESC"</tt> will work but <tt>order: "name DESC"</tt> will not.

You should either use order("orders.id DESC") or pass a lambda as the second argument of your has_many statement:

has_many :orders, -> { order(id: :desc) }

As the first workaround (i.e. with order) doesn't work with 4-0-stable, I will leave it as open until we know whether the resolving commit hasn't been backported intentionally.

@robin850
Member
robin850 commented Mar 7, 2014

Ok, after bisecting the repository, it looks like the following commits are involved:

I don't know if it's safe to backport these changes to the 4-0-stable branch though.

@laurocaetano
Contributor

The correct is User.includes(:orders).reorder('orders.id asc') and it will work as expected.

@jerielabayon thanks for reporting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment