Rails Lighthouse ticket #1860

handle_complex_includes.diff

From 10a8510887fb5d39b39ac3cb8540451a9cb01aa0 Mon Sep 17 00:00:00 2001
From: Brian Underwood <[email protected]>
Date: Thu, 14 Oct 2010 09:34:03 -0400
Subject: [PATCH] Take care of case where different includes() (such as when calling a scope on an assocation) combine as an Array and Hash but are addressing the same assocation.  [#1860 state:resolved]

---
 activerecord/lib/active_record/associations.rb     |   14 +++++++++-
 .../cases/associations/twice_eager_loaded_test.rb  |   27 ++++++++++++++++++++
 activerecord/test/fixtures/price_estimates.yml     |    2 +
 activerecord/test/models/pirate.rb                 |    2 +-
 activerecord/test/models/price_estimate.rb         |    3 ++
 activerecord/test/models/treasure.rb               |    2 +
 activerecord/test/schema/schema.rb                 |    1 +
 7 files changed, 48 insertions(+), 3 deletions(-)
 create mode 100644 activerecord/test/cases/associations/twice_eager_loaded_test.rb

diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index affa2fb..4825aa4 100644
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -1928,7 +1928,12 @@ module ActiveRecord
                   join_association.join_type = join_type
                   @join_parts << join_association
                 when Array
-                  associations.each do |association|
+                  # Take care of case [:bar_assocation, {:bar_assocation => :foo_assocation}]
+                  keys = associations.select do |assocation|
+                    assocation.is_a?(Hash)
+                  end.collect(&:keys).flatten
+
+                  (associations - keys).each do |association|
                     build(association, parent, join_type)
                   end
                 when Hash
@@ -1962,7 +1967,12 @@ module ActiveRecord
                 join_parts.delete(join_part)
                 construct_association(parent, join_part, row)
               when Array
-                associations.each do |association|
+                # Take care of case [:bar_assocation, {:bar_assocation => :foo_assocation}]
+                keys = associations.select do |assocation|
+                  assocation.is_a?(Hash)
+                end.collect(&:keys).flatten
+
+                (associations - keys).each do |association|
                   construct(parent, association, join_parts, row)
                 end
               when Hash
diff --git a/activerecord/test/cases/associations/twice_eager_loaded_test.rb b/activerecord/test/cases/associations/twice_eager_loaded_test.rb
new file mode 100644
index 0000000..4cdae34
--- /dev/null
+++ b/activerecord/test/cases/associations/twice_eager_loaded_test.rb
@@ -0,0 +1,27 @@
+require 'models/pirate'
+require 'models/treasure'
+require 'models/price_estimate'
+
+class TwiceEagerLoadedTest < ActiveRecord::TestCase
+  fixtures :pirate, :treasure, :price_estimate
+
+  def test_loading_assocation_and_scope_with_includes
+    Pirate.all.each do |pirate|
+      # The treasures association has includes(:price_estimates)
+      # The pricey scope has includes(:price_estimates => :estimate_of)
+      # These should combine as includes(:price_estimates => :estimate_of)
+
+      assert_nothing_raised do
+        pirate.treasures.pricey
+      end
+
+      # The example above is to demonstrate why this is an important issue
+      # but in case the assocation or scope above gets changed, let's do these manually
+      # The where() clause isn't neccessary for this test, just duplicating the above
+      assert_nothing_raised do
+        Pirate.treasures.includes(:price_estimates).includes(:price_estimates => :pirate).where('price_estimates.price > 15')
+      end
+    end
+  end
+
+end
\ No newline at end of file
diff --git a/activerecord/test/fixtures/price_estimates.yml b/activerecord/test/fixtures/price_estimates.yml
index 1149ab1..9104279 100644
--- a/activerecord/test/fixtures/price_estimates.yml
+++ b/activerecord/test/fixtures/price_estimates.yml
@@ -1,7 +1,9 @@
 saphire_1:
   price: 10
   estimate_of: sapphire (Treasure)
+  pirate: redbeard
 
 sapphire_2:
   price: 20
   estimate_of: sapphire (Treasure)
+  pirate: blackbeard
diff --git a/activerecord/test/models/pirate.rb b/activerecord/test/models/pirate.rb
index d89c8cf..c73f581 100644
--- a/activerecord/test/models/pirate.rb
+++ b/activerecord/test/models/pirate.rb
@@ -14,7 +14,7 @@ class Pirate < ActiveRecord::Base
     :before_remove => proc {|p,pa| p.ship_log << "before_removing_proc_parrot_#{pa.id}"},
     :after_remove  => proc {|p,pa| p.ship_log << "after_removing_proc_parrot_#{pa.id}"}
 
-  has_many :treasures, :as => :looter
+  has_many :treasures, :includes => :price_estimates, :as => :looter
   has_many :treasure_estimates, :through => :treasures, :source => :price_estimates
 
   # These both have :autosave enabled because accepts_nested_attributes_for is used on them.
diff --git a/activerecord/test/models/price_estimate.rb b/activerecord/test/models/price_estimate.rb
index ef3bba3..1a93d22 100644
--- a/activerecord/test/models/price_estimate.rb
+++ b/activerecord/test/models/price_estimate.rb
@@ -1,3 +1,6 @@
 class PriceEstimate < ActiveRecord::Base
   belongs_to :estimate_of, :polymorphic => true
+
+  # Pirate that made the estimation
+  belongs_to :pirate
 end
diff --git a/activerecord/test/models/treasure.rb b/activerecord/test/models/treasure.rb
index 2a98e74..729c812 100644
--- a/activerecord/test/models/treasure.rb
+++ b/activerecord/test/models/treasure.rb
@@ -4,5 +4,7 @@ class Treasure < ActiveRecord::Base
 
   has_many :price_estimates, :as => :estimate_of
 
+  scope :pricey, includes(:price_estimates => :pirate).where('price_estimates.price > 15')
+
   accepts_nested_attributes_for :looter
 end
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index ea62833..ba9d892 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -436,6 +436,7 @@ ActiveRecord::Schema.define do
     t.string :estimate_of_type
     t.integer :estimate_of_id
     t.integer :price
+    t.integer :pirate_id
   end
 
   create_table :products, :force => true do |t|
-- 
1.7.1

include_error.txt

ActiveRecord::StatementInvalid: PGError: ERROR:  missing FROM-clause entry for table "users_changes"
LINE 1: ..." AS t1_r18, "users"."email_signature" AS t1_r19, "users_cha...
                                                             ^
: SELECT     "changes"."id" AS t0_r0, "changes"."changable_object_id" AS t0_r1, "changes"."changable_object_type" AS t0_r2, "changes"."user_id" AS t0_r3, "changes"."user_login" AS t0_r4, "changes"."change_list" AS t0_r5, "changes"."change_type" AS t0_r6, "changes"."backtrace" AS t0_r7, "changes"."created_at" AS t0_r8, "changes"."rake_task" AS t0_r9, "users"."id" AS t1_r0, "users"."login" AS t1_r1, "users"."password" AS t1_r2, "users"."email" AS t1_r3, "users"."firstname" AS t1_r4, "users"."lastname" AS t1_r5, "users"."props" AS t1_r6, "users"."deleted_at" AS t1_r7, "users"."phone" AS t1_r8, "users"."fax" AS t1_r9, "users"."synced_at" AS t1_r10, "users"."created_at" AS t1_r11, "users"."updated_at" AS t1_r12, "users"."filter_id" AS t1_r13, "users"."is_locked" AS t1_r14, "users"."password_expires_at" AS t1_r15, "users"."location_filter_id" AS t1_r16, "users"."promonet_id" AS t1_r17, "users"."password_hash" AS t1_r18, "users"."email_signature" AS t1_r19, "users_changes"."id" AS t2_r0, "users_changes"."login" AS t2_r1, "users_changes"."password" AS t2_r2, "users_changes"."email" AS t2_r3, "users_changes"."firstname" AS t2_r4, "users_changes"."lastname" AS t2_r5, "users_changes"."props" AS t2_r6, "users_changes"."deleted_at" AS t2_r7, "users_changes"."phone" AS t2_r8, "users_changes"."fax" AS t2_r9, "users_changes"."synced_at" AS t2_r10, "users_changes"."created_at" AS t2_r11, "users_changes"."updated_at" AS t2_r12, "users_changes"."filter_id" AS t2_r13, "users_changes"."is_locked" AS t2_r14, "users_changes"."password_expires_at" AS t2_r15, "users_changes"."location_filter_id" AS t2_r16, "users_changes"."promonet_id" AS t2_r17, "users_changes"."password_hash" AS t2_r18, "users_changes"."email_signature" AS t2_r19, "regions"."id" AS t3_r0, "regions"."name" AS t3_r1, "regions"."parent_id" AS t3_r2, "regions"."user_id" AS t3_r3, "regions"."position" AS t3_r4, "regions"."deleted_at" AS t3_r5 FROM       "changes"  LEFT OUTER JOIN "users" ON "users"."id" = "changes"."user_id" LEFT OUTER JOIN "regions_users" ON "regions_users"."user_id" = "users"."id" LEFT OUTER JOIN "regions" ON "regions"."id" = "regions_users"."region_id" WHERE     (users.deleted_at IS NULL)
	from /Users/bunderwood/Sites/vmm/vendor/bundle/ruby/1.8/gems/activerecord-3.0.0/lib/active_record/connection_adapters/abstract_adapter.rb:202:in `log'
	from /Users/bunderwood/Sites/vmm/vendor/bundle/ruby/1.8/gems/activerecord-3.0.0/lib/active_record/connection_adapters/postgresql_adapter.rb:496:in `execute'
	from /Users/bunderwood/Sites/vmm/vendor/bundle/ruby/1.8/gems/activerecord-3.0.0/lib/active_record/connection_adapters/postgresql_adapter.rb:982:in `select_raw'
	from /Users/bunderwood/Sites/vmm/vendor/bundle/ruby/1.8/gems/activerecord-3.0.0/lib/active_record/connection_adapters/postgresql_adapter.rb:975:in `select'
	from /Users/bunderwood/Sites/vmm/vendor/bundle/ruby/1.8/gems/activerecord-3.0.0/lib/active_record/connection_adapters/abstract/database_statements.rb:7:in `select_all'
	from /Users/bunderwood/Sites/vmm/vendor/bundle/ruby/1.8/gems/activerecord-3.0.0/lib/active_record/connection_adapters/abstract/query_cache.rb:56:in `select_all'
	from /Users/bunderwood/Sites/vmm/vendor/bundle/ruby/1.8/gems/activerecord-3.0.0/lib/active_record/base.rb:467:in `find_by_sql'
	from /Users/bunderwood/Sites/vmm/vendor/bundle/ruby/1.8/gems/activerecord-3.0.0/lib/active_record/relation.rb:64:in `to_a'
	from /Users/bunderwood/Sites/vmm/vendor/bundle/ruby/1.8/gems/activerecord-3.0.0/lib/active_record/relation/finder_methods.rb:189:in `find_with_associations'
	from /Users/bunderwood/Sites/vmm/vendor/bundle/ruby/1.8/gems/activerecord-3.0.0/lib/active_record/relation.rb:64:in `to_a'
	from /Users/bunderwood/Sites/vmm/vendor/bundle/ruby/1.8/gems/activerecord-3.0.0/lib/active_record/relation/finder_methods.rb:143:in `all'
	from (irb):44

includes.patch

From 014972bcf9d12f5679f656f8b06e294390342c36 Mon Sep 17 00:00:00 2001
From: Brian Underwood <[email protected]>
Date: Wed, 13 Oct 2010 19:25:49 -0400
Subject: Patch to resolve when includes parameters combine poorly

---
 activerecord/lib/active_record/associations.rb |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index affa2fb..ff8d57e 100644
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -1928,7 +1928,11 @@ module ActiveRecord
                   join_association.join_type = join_type
                   @join_parts << join_association
                 when Array
-                  associations.each do |association|
+                  keys = associations.select do |assocation|
+                    assocation.is_a?(Hash)
+                  end.collect(&:keys).flatten
+
+                  (associations - keys).each do |association|
                     build(association, parent, join_type)
                   end
                 when Hash
@@ -1962,7 +1966,11 @@ module ActiveRecord
                 join_parts.delete(join_part)
                 construct_association(parent, join_part, row)
               when Array
-                associations.each do |association|
+                keys = associations.select do |assocation|
+                  assocation.is_a?(Hash)
+                end.collect(&:keys).flatten
+
+                (associations - keys).each do |association|
                   construct(parent, association, join_parts, row)
                 end
               when Hash
-- 
1.7.1

merge_includes_merges_hashes.diff

From 9e69c36289ae84a51b2f44acc86f6366359e59ac Mon Sep 17 00:00:00 2001
From: Anselm Helbig <[email protected]>
Date: Tue, 3 Feb 2009 17:44:15 +0100
Subject: [PATCH] made ActiveRecord::Base::merge_includes merge hashes

---
 activerecord/lib/active_record/base.rb         |   23 +++++++++++++++++++++--
 activerecord/test/cases/method_scoping_test.rb |    7 +++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index f9168c8..8ca527f 100755
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -1701,8 +1701,27 @@ module ActiveRecord #:nodoc:
         end
 
         # Merges includes so that the result is a valid +include+
-        def merge_includes(first, second)
-         (safe_to_array(first) + safe_to_array(second)).uniq
+        def merge_includes(*args)
+          hash = args.inject({}) do |h, inc|
+            case inc
+            when Array
+              inc.each { |v|    h[v] = merge_includes(h[v], nil) }
+            when Hash
+              inc.each { |k, v| h[k] = merge_includes(h[k], v) }
+            when NilClass, FalseClass
+            else
+              h[inc.to_sym] ||= []
+            end
+            h
+          end
+          result = hash.map { |k, v|
+            if v.blank?
+              hash.delete(k)
+              k
+            end
+          }.compact
+          result << hash unless hash.blank?
+          result
         end
 
         def merge_joins(*joins)
diff --git a/activerecord/test/cases/method_scoping_test.rb b/activerecord/test/cases/method_scoping_test.rb
index 71e2ce8..0da0b6f 100644
--- a/activerecord/test/cases/method_scoping_test.rb
+++ b/activerecord/test/cases/method_scoping_test.rb
@@ -336,6 +336,13 @@ class NestedScopingTest < ActiveRecord::TestCase
         assert_equal('David', Developer.find(:first).name)
       end
     end
+
+    # mixing hash and array include's will merge correctly
+    Author.with_scope(:find => { :include => [:posts]}) do
+      Author.with_scope(:find => { :include => { :posts => :comments } }) do
+        assert_equal [{ :posts => [:comments] }], Author.instance_eval('current_scoped_methods')[:find][:include]
+      end
+    end
   end
 
   def test_nested_scoped_find_replace_include
-- 
1.5.4.3