Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

has_many ordering breaking when using :includes and join table in :where #6769

Closed
jgadbois opened this Issue · 14 comments

10 participants

John Gadbois Rafael Mendonça França ryanwanger Niklas Närhinen Yves Senn Jon Leighton Lauro Caetano Ruby on Rails Bot Phlegx Systems OG jschwertfeger
John Gadbois

Here's the relevant code from my app

class Activity < ActiveRecord::Base
   scope :with_comments, includes(:comments=>:user)
   has_many :comments, :as=>:commentable, :order=>'created_at ASC'
end


irb(main):029:0> a = Activity.includes(:user).where(["users.privacy_level = ? OR users.privacy_level IS null", User::PRIVACY_PUBLIC]).where(:created_at=>(1.week.ago..Date.today.end_of_day)).where('comments_count > 5').with_comments.limit(10); 0
=> 0
irb(main):031:0> a.first.comments.each { |c| puts c.created_at };0
2012-06-13 18:38:36 UTC
2012-06-13 04:12:26 UTC
2012-06-12 14:49:29 UTC
2012-06-13 02:59:15 UTC
2012-06-13 02:46:14 UTC
2012-06-13 14:26:29 UTC
Rafael Mendonça França

@jgadbois can you provide the SQL output?

John Gadbois
  Activity Load (734.0ms)  SELECT DISTINCT "activities".id FROM "activities" LEFT OUTER JOIN "users" ON "users"."id" = "activities"."user_id" LEFT OUTER JOIN "comments" ON "comments"."commentable_id" = "activities"."id" AND "comments"."commentable_type" = 'Activity' LEFT OUTER JOIN "users" "users_comments" ON "users_comments"."id" = "comments"."user_id" WHERE (users.privacy_level = 1 OR users.privacy_level IS null) AND ("activities"."created_at" BETWEEN '2012-06-11 15:06:09.601417' AND '2012-06-18 23:59:59.999999') AND (comments_count > 5) LIMIT 10
  SQL (701.4ms)  SELECT "activities"."id" AS t0_r0, "activities"."user_id" AS t0_r1, "activities"."activity_type" AS t0_r2, "activities"."data" AS t0_r3, "activities"."created_at" AS t0_r4, "activities"."updated_at" AS t0_r5, "activities"."streamable_id" AS t0_r6, "activities"."streamable_type" AS t0_r7, "activities"."likes_count" AS t0_r8, "activities"."comments_count" AS t0_r9, "users"."id" AS t1_r0, "users"."email" AS t1_r1, "users"."encrypted_password" AS t1_r2, "users"."password_salt" AS t1_r3, "users"."sign_in_count" AS t1_r4, "users"."current_sign_in_at" AS t1_r5, "users"."last_sign_in_at" AS t1_r6, "users"."current_sign_in_ip" AS t1_r7, "users"."last_sign_in_ip" AS t1_r8, "users"."created_at" AS t1_r9, "users"."updated_at" AS t1_r10, "users"."roles_mask" AS t1_r11, "users"."first_name" AS t1_r12, "users"."last_name" AS t1_r13, "users"."gender" AS t1_r14, "users"."birthdate" AS t1_r15, "users"."uses_metric" AS t1_r16, "users"."nickname" AS t1_r17, "users"."premium" AS t1_r18, "users"."zip_code" AS t1_r19, "users"."profile_image_file_name" AS t1_r20, "users"."profile_image_content_type" AS t1_r21, "users"."profile_image_file_size" AS t1_r22, "users"."profile_image_updated_at" AS t1_r23, "users"."privacy_level" AS t1_r24, "users"."receive_newsletter" AS t1_r25, "users"."active" AS t1_r26, "users"."affiliate_code" AS t1_r27, "users"."subscription_id" AS t1_r28, "users"."points" AS t1_r29, "users"."level" AS t1_r30, "users"."rank" AS t1_r31, "users"."previous_rank" AS t1_r32, "users"."num_badges" AS t1_r33, "users"."num_completed_challenges" AS t1_r34, "users"."num_workouts" AS t1_r35, "users"."current_frc_score" AS t1_r36, "users"."total_weight_lifted_cache" AS t1_r37, "users"."total_cardio_miles_cache" AS t1_r38, "users"."total_calories" AS t1_r39, "users"."cached_slug" AS t1_r40, "users"."stripe_customer_token" AS t1_r41, "users"."forem_admin" AS t1_r42, "users"."user_settings" AS t1_r43, "users"."reset_password_token" AS t1_r44, "users"."reset_password_sent_at" AS t1_r45, "users"."remember_created_at" AS t1_r46, "users"."deleted_at" AS t1_r47, "users"."following_count" AS t1_r48, "users"."follower_count" AS t1_r49, "users"."authentication_token" AS t1_r50, "users"."forem_state" AS t1_r51, "users"."forem_auto_subscribe" AS t1_r52, "users"."battles_won" AS t1_r53, "users"."battles_lost" AS t1_r54, "users"."time_zone" AS t1_r55, "users"."battles_tied" AS t1_r56, "comments"."id" AS t2_r0, "comments"."content" AS t2_r1, "comments"."commentable_id" AS t2_r2, "comments"."commentable_type" AS t2_r3, "comments"."user_id" AS t2_r4, "comments"."approved" AS t2_r5, "comments"."created_at" AS t2_r6, "comments"."updated_at" AS t2_r7, "comments"."name" AS t2_r8, "comments"."email" AS t2_r9, "comments"."website" AS t2_r10, "comments"."ancestry" AS t2_r11, "users_comments"."id" AS t3_r0, "users_comments"."email" AS t3_r1, "users_comments"."encrypted_password" AS t3_r2, "users_comments"."password_salt" AS t3_r3, "users_comments"."sign_in_count" AS t3_r4, "users_comments"."current_sign_in_at" AS t3_r5, "users_comments"."last_sign_in_at" AS t3_r6, "users_comments"."current_sign_in_ip" AS t3_r7, "users_comments"."last_sign_in_ip" AS t3_r8, "users_comments"."created_at" AS t3_r9, "users_comments"."updated_at" AS t3_r10, "users_comments"."roles_mask" AS t3_r11, "users_comments"."first_name" AS t3_r12, "users_comments"."last_name" AS t3_r13, "users_comments"."gender" AS t3_r14, "users_comments"."birthdate" AS t3_r15, "users_comments"."uses_metric" AS t3_r16, "users_comments"."nickname" AS t3_r17, "users_comments"."premium" AS t3_r18, "users_comments"."zip_code" AS t3_r19, "users_comments"."profile_image_file_name" AS t3_r20, "users_comments"."profile_image_content_type" AS t3_r21, "users_comments"."profile_image_file_size" AS t3_r22, "users_comments"."profile_image_updated_at" AS t3_r23, "users_comments"."privacy_level" AS t3_r24, "users_comments"."receive_newsletter" AS t3_r25, "users_comments"."active" AS t3_r26, "users_comments"."affiliate_code" AS t3_r27, "users_comments"."subscription_id" AS t3_r28, "users_comments"."points" AS t3_r29, "users_comments"."level" AS t3_r30, "users_comments"."rank" AS t3_r31, "users_comments"."previous_rank" AS t3_r32, "users_comments"."num_badges" AS t3_r33, "users_comments"."num_completed_challenges" AS t3_r34, "users_comments"."num_workouts" AS t3_r35, "users_comments"."current_frc_score" AS t3_r36, "users_comments"."total_weight_lifted_cache" AS t3_r37, "users_comments"."total_cardio_miles_cache" AS t3_r38, "users_comments"."total_calories" AS t3_r39, "users_comments"."cached_slug" AS t3_r40, "users_comments"."stripe_customer_token" AS t3_r41, "users_comments"."forem_admin" AS t3_r42, "users_comments"."user_settings" AS t3_r43, "users_comments"."reset_password_token" AS t3_r44, "users_comments"."reset_password_sent_at" AS t3_r45, "users_comments"."remember_created_at" AS t3_r46, "users_comments"."deleted_at" AS t3_r47, "users_comments"."following_count" AS t3_r48, "users_comments"."follower_count" AS t3_r49, "users_comments"."authentication_token" AS t3_r50, "users_comments"."forem_state" AS t3_r51, "users_comments"."forem_auto_subscribe" AS t3_r52, "users_comments"."battles_won" AS t3_r53, "users_comments"."battles_lost" AS t3_r54, "users_comments"."time_zone" AS t3_r55, "users_comments"."battles_tied" AS t3_r56 FROM "activities" LEFT OUTER JOIN "users" ON "users"."id" = "activities"."user_id" LEFT OUTER JOIN "comments" ON "comments"."commentable_id" = "activities"."id" AND "comments"."commentable_type" = 'Activity' LEFT OUTER JOIN "users" "users_comments" ON "users_comments"."id" = "comments"."user_id" WHERE "activities"."id" IN (152136, 143651, 144042) AND (users.privacy_level = 1 OR users.privacy_level IS null) AND ("activities"."created_at" BETWEEN '2012-06-11 15:06:09.601417' AND '2012-06-18 23:59:59.999999') AND (comments_count > 5)

Sorry about the line breaks, not sure if there is a way to make it wrap.

ryanwanger

I reproduced this. Here is a simplified case of having a where clause that references an included association (that has a specified order):

class User < ActiveRecord::Base
  has_many :activities, :order => "created_at desc"
end

class Activity < ActiveRecord::Base
end

User.includes(:activities).where("activities.created_at < ?", Time.now).all

SELECT "users"."id" AS t0_r0, "users"."privacy_level" AS t0_r1, "users"."created_at" AS t0_r2, "users"."updated_at" AS t0_r3, "activities"."id" AS t1_r0, "activities"."user_id" AS t1_r1, "activities"."created_at" AS t1_r2, "activities"."updated_at" AS t1_r3 FROM "users" LEFT OUTER JOIN "activities" ON "activities"."user_id" = "users"."id" WHERE (activities.created_at < '2012-06-30 21:22:02.195840')

The ordering for activities is gone. Without the where clause the order is preserved:

User.includes(:activities).all

SELECT "users".* FROM "users" 
SELECT "activities".* FROM "activities" WHERE "activities"."user_id" IN (2) ORDER BY created_at desc

If the where clause doesn't reference the included association, then the order is correctly maintained:

User.includes(:activities).where("users.created_at < ?", Time.now).all

SELECT "users".* FROM "users" WHERE (users.created_at < '2012-06-30 21:26:57.875379')
SELECT "activities".* FROM "activities" WHERE "activities"."user_id" IN (2) ORDER BY created_at desc
Niklas Närhinen

Any news on this?

Yves Senn
Owner

This issue still exists on master. I created a as-is reproduction script to facilitate testing: https://gist.github.com/senny/5132876

The problem is related to the eager-loading and preloading. I have fixed some issue in these areas before but the order has been a weak point.

@jonleighton what do you think? Can we combine the order of User and Activities? Does this lead to further problems?

Jon Leighton
Collaborator

@senny yeah, supposing the User had an ordering, I guess we'd want something like ORDER BY [users_order], [activities_order]. not sure how easy that is to implement though.

John Gadbois

@jonleighton @senny if you point me in the right direction I'm happy to take a stab at it.

Lauro Caetano
Collaborator

I created an updated gist based on @senny's gist to reproduce this bug. It seems like a bug, but I couldn't write a test case on Rails codebase yet. Also, I'm not sure how hard is to fix this issue.

Btw, I think #11081 is not valid anymore. @neerajdotname, could you take a look?

Lauro Caetano
Collaborator

I created a test case on Rails to reproduce this issue:

diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb
index c79c0c8..b2775a5 100644
--- a/activerecord/test/cases/associations/has_many_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_associations_test.rb
@@ -26,6 +26,8 @@ require 'models/reference'
 require 'models/job'
 require 'models/college'
 require 'models/student'
+require 'models/subscription'
+require 'models/subscriber'

 class HasManyAssociationsTestForReorderWithJoinDependency < ActiveRecord::TestCase
   fixtures :authors, :posts, :comments
@@ -1864,4 +1866,20 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
       end
     end
   end
+
+  test 'association respects order defined by its scope' do
+    subscriber = Subscriber.create(nick: 'Test')
+    subscription_2 = subscriber.subscriptions_ordered_by_created_at.build
+    subscription_1 = subscriber.subscriptions_ordered_by_created_at.build(created_at: 1.day.ago)
+    subscription_3 = subscriber.subscriptions_ordered_by_created_at.build
+    subscriber.save
+
+    subscriber = Subscriber.includes(:subscriptions_ordered_by_created_at).
+      where('subscriptions.created_at < ?', Time.now).
+      references(:subscriptions).first
+
+    subscription_ids = subscriber.subscriptions_ordered_by_created_at.map(&:id)
+
+    assert_equal [subscription_1.id, subscription_2.id, subscription_3.id], subscription_ids
+  end
 end
diff --git a/activerecord/test/models/subscriber.rb b/activerecord/test/models/subscriber.rb
index 76e85a0..03ab913 100644
--- a/activerecord/test/models/subscriber.rb
+++ b/activerecord/test/models/subscriber.rb
@@ -1,6 +1,7 @@
 class Subscriber < ActiveRecord::Base
   self.primary_key = 'nick'
   has_many :subscriptions
+  has_many :subscriptions_ordered_by_created_at, -> { order('created_at desc') }, class_name: 'Subscription'
   has_many :books, :through => :subscriptions
 end

diff --git a/activerecord/test/models/tag.rb b/activerecord/test/models/tag.rb
index 80d4725..9e99335 100644
--- a/activerecord/test/models/tag.rb
+++ b/activerecord/test/models/tag.rb
@@ -1,5 +1,6 @@
 class Tag < ActiveRecord::Base
   has_many :taggings
+  has_many :taggings_ordered_by_created_at, -> { order('created_at desc') }, class_name: 'Tagging'
   has_many :taggables, :through => :taggings
   has_one  :tagging

diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index da3074e..5f55f5d 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -665,6 +665,7 @@ ActiveRecord::Schema.define do
   create_table :subscriptions, force: true do |t|
     t.string :subscriber_id
     t.integer :book_id
+    t.datetime :created_at
   end

   create_table :tags, force: true do |t|

I think it will be a hard fix and also I can confirm that #11081 is not valid anymore.

Ruby on Rails Bot rails-bot added the stale label
Ruby on Rails Bot
Collaborator

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

Ruby on Rails Bot rails-bot closed this
Ruby on Rails Bot
Collaborator

This issue has been automatically closed because of inactivity.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

Phlegx Systems OG

I have the same problem. Please see my issue on Casecommons/pg_search#206 for more information and example code with SQL logs.

jschwertfeger

This problem still exists in 4-1-stable. A workaround is to use preload instead of includes which causes Rails to create second query to fetch the associated objects, respecting the order specified on the association, rather than creating a single query with joins.

Rafael Mendonça França
Owner

How about master branch?

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.