Skip to content

has_many through obeys order on through association #10087

Merged
merged 1 commit into from Apr 4, 2013

3 participants

@neerajdotname
Ruby on Rails member

fixes #10016

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Apr 4, 2013
activerecord/CHANGELOG.md
@@ -1,5 +1,10 @@
## Rails 4.0.0 (unreleased) ##
+* has_many using :through now obyes the order clause mentioned in
@carlosantoniodasilva
Ruby on Rails member

Typo: obeys

@carlosantoniodasilva
Ruby on Rails member

Also, it might be good to put has_many and :through inside backticks for better formatting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Apr 4, 2013
activerecord/CHANGELOG.md
@@ -1,5 +1,10 @@
## Rails 4.0.0 (unreleased) ##
+* has_many using :through now obyes the order clause mentioned in
+ through association. Fixes #10016
@carlosantoniodasilva
Ruby on Rails member

. at the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Apr 4, 2013
...es/associations/has_many_through_associations_test.rb
@@ -882,6 +882,10 @@ def test_has_many_through_with_polymorphic_source
assert_equal [tags(:general)], post.reload.tags
end
+ def test_has_many_through_obeys_order_on_through_association
+ assert Owner.first.toys.to_sql.include?("pets.name desc")
@carlosantoniodasilva
Ruby on Rails member

Can we actually test that records are coming back ordered? I'm afraid this could lead to false positives in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@egilburg
egilburg commented Apr 4, 2013

Would this cause issues if there are no explicit table name specified and both tables have the same column name? E.g. both just say "order by name". Both orders would be unambiguous on their own, but could ambiguous when joined automatically.

@neerajdotname
Ruby on Rails member

@egilburg this is what I found.

class Physician < ActiveRecord::Base
  has_many :appointments, -> { order 'id desc' }
  has_many :patients, -> { order 'id asc' }, through: :appointments
end

$ Physician.first.patients.to_a
#=> SELECT "patients".* FROM "patients" INNER JOIN "appointments" ON "patients"."id" = 
"appointments"."patient_id" WHERE "appointments"."physician_id" = ? ORDER BY id asc, id desc 
 [["physician_id", 42]]

I tested that sql in sqlite3, mysql2 and pg . And it worked ok.

@neerajdotname
Ruby on Rails member

@carlosantoniodasilva Please take a look. Thanks.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Apr 4, 2013
...es/associations/has_many_through_associations_test.rb
@@ -882,6 +882,11 @@ def test_has_many_through_with_polymorphic_source
assert_equal [tags(:general)], post.reload.tags
end
+ def test_has_many_through_obeys_order_on_through_association
+ assert Owner.first.toys.to_sql.include?("pets.name desc")
+ assert_equal ["parrot", "bulbul"], owners(:blackbeard).toys.map { |r| r.pet.name }
@carlosantoniodasilva
Ruby on Rails member

Are owners(:blackbeard) and Owner.first the same? Perhaps the first line could use the fixture as well to avoid an extra query.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva
Ruby on Rails member

@neerajdotname looks good, just a minor comment.

@neerajdotname
Ruby on Rails member

@carlosantoniodasilva good catch. Fixed.

@carlosantoniodasilva carlosantoniodasilva merged commit ba4c274 into rails:master Apr 4, 2013
@carlosantoniodasilva
Ruby on Rails member

Thanks!

@egilburg egilburg commented on the diff Apr 4, 2013
...d/lib/active_record/associations/association_scope.rb
@@ -101,6 +101,7 @@ def add_constraints(scope)
scope.includes! item.includes_values
scope.where_values += item.where_values
+ scope.order_values += (item.order_values - scope.order_values)
@egilburg
egilburg added a note Apr 4, 2013

belated notice, but i think this line is equivalent to simplified:

scope.order_values |= item.order_values

note |= rather than ||=

@neerajdotname
Ruby on Rails member
neerajdotname added a note Apr 4, 2013

@egilburg good call. That is much more intent revealing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.