push preloaded test up to the factory method so we can eliminate

conditionals from the individual preloaded classes
rails · Sep 24, 2013 · b93d09d · b93d09d
1 parent bb9554a
commit b93d09d
Show file tree

Hide file tree

Showing 4 changed files with 21 additions and 9 deletions.
diff --git a/activerecord/lib/active_record/associations/preloader.rb b/activerecord/lib/active_record/associations/preloader.rb
@@ -133,7 +133,7 @@ def preloaders_for_hash(association, records, scope)
       def preloaders_for_one(association, records, scope)
         grouped_records(association, records).flat_map do |reflection, klasses|
           klasses.map do |rhs_klass, rs|
-            loader = preloader_for(reflection).new(rhs_klass, rs, reflection, scope)
+            loader = preloader_for(reflection, rs).new(rhs_klass, rs, reflection, scope)
             loader.run self
             loader
           end
@@ -173,7 +173,22 @@ def association_klass(reflection, record)
         end
       end
 
-      def preloader_for(reflection)
+      class NullPreloader
+        attr_reader :owners
+
+        def initialize(klass, owners, reflection, preload_scope)
+          @owners = owners
+        end
+
+        def run(preloader); end
+        def loaded?; false; end
+      end
+
+      def preloader_for(reflection, owners)
+        if owners.first.association(reflection.name).loaded?
+          return NullPreloader
+        end
+
         case reflection.macro
         when :has_many
           reflection.options[:through] ? HasManyThrough : HasMany

diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb
@@ -21,9 +21,7 @@ def initialize(klass, owners, reflection, preload_scope)
         end
 
         def run(preloader)
-          unless owners.first.association(reflection.name).loaded?
-            preload(preloader)
-          end
+          preload(preloader)
         end
 
         def preload(preloader)

diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb
@@ -49,13 +49,13 @@ def associated_records_by_owner(preloader)
           @associated_records_by_owner = through_records.each_with_object({}) { |(lhs,center),records_by_owner|
             pl_to_middle = center.group_by { |record| middle_to_pl[record] }
 
-            records_by_owner[lhs] = pl_to_middle.flat_map do |preloader, middles|
+            records_by_owner[lhs] = pl_to_middle.flat_map do |pl, middles|
               rhs_records = middles.flat_map { |r|
                 r.send(source_reflection.name)
               }.compact
 
-              if preloader && preloader.loaded?
-                loaded_records = preloader.preloaded_records
+              if pl && pl.loaded?
+                loaded_records = pl.preloaded_records
                 i = 0
                 record_index = loaded_records.each_with_object({}) { |r,indexes|
                   indexes[r] = i

diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -46,7 +46,6 @@ def test_preload_sti_middle_relation
     CurrentMembership.create! club: club, member: member2
 
     club1 = Club.includes(:members).find_by_id club.id
-    left, right = club1.members.map(&:id)
     assert_equal [member1, member2].sort_by(&:id),
                  club1.members.sort_by(&:id)
   end