Begin refactoring ThroughReflection

This allows us to decouple AssociationReflection and
ThroughReflection making ThroughReflection it's own Reflection
bucket in a way. The benefit of this is to be able to remove
checks against the macro's symbol for exmaple `macro == :belongs_to`.

Get all tests passing again

Some of the methods that used to be inherited from MacroReflection
through AssociationReflection were no longer getting passed through.
They needed to be duplicated into the ThroughReflection. I will
extract these out into a separate class.

Refactor shared methods into strategy object

Now that we've separated ThroughReflection and AssociationReflection
we can combine shared methods into one class to avoid duplication.

Break out class for each type of reflection

This creates a class for each reflection type (has_many, has_one,
belongs_to and habtm). We then can remove the need to set the macro
symbol in each initialization.

Tests were updated to reflect these changes because creation of
these reflections is now different.

Remove need for @collection instance var

We now define `collection?` as `false` by default and set it to
`true` in `has_and_belongs_to_many` and `has_many` reflections.
This removes the need for the `@collection` instance variable.

Raise exception on unknown macro types

We shouldn't accept just any macro when creating reflections. An
unrecongnized AssociationReflection raises an error. Tests in
`reflection_test` were updated to reflect these new changes.
`:has_and_belongs_to_many` macro tests were removed because we no
longer internally return HABTM.

activerecord/lib/active_record/associations.rb

@@ -1587,7 +1587,7 @@ module ActiveRecord
           scope   = nil
         end
 
-        habtm_reflection = ActiveRecord::Reflection::HasAndBelongsToManyReflection.new(:has_and_belongs_to_many, name, scope, options, self)
+        habtm_reflection = ActiveRecord::Reflection::HasAndBelongsToManyReflection.new(name, scope, options, self)
 
         builder = Builder::HasAndBelongsToMany.new name, self, options
 

activerecord/lib/active_record/reflection.rb

@@ -13,14 +13,21 @@ module ActiveRecord
     end
 
     def self.create(macro, name, scope, options, ar)
-      case macro
+      klass = case macro
-      when :has_many, :belongs_to, :has_one
+              when :composed_of
-        klass = options[:through] ? ThroughReflection : AssociationReflection
+                AggregateReflection
-      when :composed_of
+              when :has_many
-        klass = AggregateReflection
+                HasManyReflection
-      end
+              when :has_one
+                HasOneReflection
+              when :belongs_to
+                BelongsToReflection
+              else
+                raise "Unsupported Macro: #{macro}"
+              end
 
-      klass.new(macro, name, scope, options, ar)
+      reflection = klass.new(name, scope, options, ar)
+      options[:through] ? ThroughReflection.new(reflection) : reflection
     end
 
     def self.add_reflection(ar, name, reflection)
@@ -110,6 +117,52 @@ module ActiveRecord
       end
     end
 
+    # Holds all the methods that are shared between MacroReflection, AssociationReflection
+    # and ThroughReflection
+    class AbstractReflection # :nodoc:
+      def table_name
+        klass.table_name
+      end
+
+      # Returns a new, unsaved instance of the associated class. +attributes+ will
+      # be passed to the class's constructor.
+      def build_association(attributes, &block)
+        klass.new(attributes, &block)
+      end
+
+      def quoted_table_name
+        klass.quoted_table_name
+      end
+
+      def primary_key_type
+        klass.type_for_attribute(klass.primary_key)
+      end
+
+      # Returns the class name for the macro.
+      #
+      # <tt>composed_of :balance, class_name: 'Money'</tt> returns <tt>'Money'</tt>
+      # <tt>has_many :clients</tt> returns <tt>'Client'</tt>
+      def class_name
+        @class_name ||= (options[:class_name] || derive_class_name).to_s
+      end
+
+      JoinKeys = Struct.new(:key, :foreign_key) # :nodoc:
+
+      def join_keys(assoc_klass)
+        if source_macro == :belongs_to
+          if polymorphic?
+            reflection_key = association_primary_key(assoc_klass)
+          else
+            reflection_key = association_primary_key
+          end
+          reflection_foreign_key = foreign_key
+        else
+          reflection_foreign_key = active_record_primary_key
+          reflection_key = foreign_key
+        end
+        JoinKeys.new(reflection_key, reflection_foreign_key)
+      end
+    end
     # Base class for AggregateReflection and AssociationReflection. Objects of
     # AggregateReflection and AssociationReflection are returned by the Reflection::ClassMethods.
     #
@@ -117,7 +170,7 @@ module ActiveRecord
     #     AggregateReflection
     #     AssociationReflection
     #       ThroughReflection
-    class MacroReflection
+    class MacroReflection < AbstractReflection
       # Returns the name of the macro.
       #
       # <tt>composed_of :balance, class_name: 'Money'</tt> returns <tt>:balance</tt>
@@ -142,8 +195,7 @@ module ActiveRecord
 
       attr_reader :plural_name # :nodoc:
 
-      def initialize(macro, name, scope, options, active_record)
+      def initialize(name, scope, options, active_record)
-        @macro         = macro
         @name          = name
         @scope         = scope
         @options       = options
@@ -167,15 +219,11 @@ module ActiveRecord
       # <tt>composed_of :balance, class_name: 'Money'</tt> returns the Money class
       # <tt>has_many :clients</tt> returns the Client class
       def klass
-        @klass ||= class_name.constantize
+        @klass ||= compute_class(class_name)
       end
 
-      # Returns the class name for the macro.
+      def compute_class(name)
-      #
+        name.constantize
-      # <tt>composed_of :balance, class_name: 'Money'</tt> returns <tt>'Money'</tt>
-      # <tt>has_many :clients</tt> returns <tt>'Client'</tt>
-      def class_name
-        @class_name ||= (options[:class_name] || derive_class_name).to_s
       end
 
       # Returns +true+ if +self+ and +other_aggregation+ have the same +name+ attribute, +active_record+ attribute,
@@ -188,23 +236,6 @@ module ActiveRecord
           active_record == other_aggregation.active_record
       end
 
-      JoinKeys = Struct.new(:key, :foreign_key) # :nodoc:
-
-      def join_keys(assoc_klass)
-        if source_macro == :belongs_to
-          if polymorphic?
-            reflection_key = association_primary_key(assoc_klass)
-          else
-            reflection_key = association_primary_key
-          end
-          reflection_foreign_key = foreign_key
-        else
-          reflection_key = foreign_key
-          reflection_foreign_key = active_record_primary_key
-        end
-        JoinKeys.new(reflection_key, reflection_foreign_key)
-      end
-
       private
         def derive_class_name
           name.to_s.camelize
@@ -237,15 +268,18 @@ module ActiveRecord
       # a new association object. Use +build_association+ or +create_association+
       # instead. This allows plugins to hook into association object creation.
       def klass
-        @klass ||= active_record.send(:compute_type, class_name)
+        @klass ||= compute_class(class_name)
+      end
+
+      def compute_class(name)
+        active_record.send(:compute_type, name)
       end
 
       attr_reader :type, :foreign_type
       attr_accessor :parent_reflection # [:name, Reflection]
 
-      def initialize(macro, name, scope, options, active_record)
+      def initialize(name, scope, options, active_record)
         super
-        @collection = macro == :has_many
         @automatic_inverse_of = nil
         @type         = options[:as] && "#{options[:as]}_type"
         @foreign_type = options[:foreign_type] || "#{name}_type"
@@ -264,24 +298,10 @@ module ActiveRecord
         }
       end
 
-      # Returns a new, unsaved instance of the associated class. +attributes+ will
-      # be passed to the class's constructor.
-      def build_association(attributes, &block)
-        klass.new(attributes, &block)
-      end
-
       def constructable? # :nodoc:
         @constructable
       end
 
-      def table_name
-        klass.table_name
-      end
-
-      def quoted_table_name
-        klass.quoted_table_name
-      end
-
       def join_table
         @join_table ||= options[:join_table] || derive_join_table
       end
@@ -290,10 +310,6 @@ module ActiveRecord
         @foreign_key ||= options[:foreign_key] || derive_foreign_key
       end
 
-      def primary_key_type
-        klass.type_for_attribute(klass.primary_key)
-      end
-
       def association_foreign_key
         @association_foreign_key ||= options[:association_foreign_key] || class_name.foreign_key
       end
@@ -396,7 +412,7 @@ Joining, Preloading and eager loading of these associations is deprecated and wi
       # association. Returns +true+ if the +macro+ is either +has_many+ or
       # +has_and_belongs_to_many+, +false+ otherwise.
       def collection?
-        @collection
+        false
       end
 
       # Returns whether or not the association should be validated as part of
@@ -560,22 +576,57 @@ Joining, Preloading and eager loading of these associations is deprecated and wi
         end
     end
 
+    class HasManyReflection < AssociationReflection #:nodoc:
+      def initialize(name, scope, options, active_record)
+        @macro = :has_many
+        super(name, scope, options, active_record)
+      end
+
+      def collection?
+        true
+      end
+    end
+
+    class HasOneReflection < AssociationReflection #:nodoc:
+      def initialize(name, scope, options, active_record)
+        @macro = :has_one
+        super(name, scope, options, active_record)
+      end
+    end
+
+    class BelongsToReflection < AssociationReflection #:nodoc:
+      def initialize(name, scope, options, active_record)
+        @macro = :belongs_to
+        super(name, scope, options, active_record)
+      end
+    end
+
     class HasAndBelongsToManyReflection < AssociationReflection #:nodoc:
-      def initialize(macro, name, scope, options, active_record)
+      def initialize(name, scope, options, active_record)
+        @macro = :has_and_belongs_to_many
         super
-        @collection = true
+      end
+
+      def collection?
+        true
       end
     end
 
     # Holds all the meta-data about a :through association as it was specified
     # in the Active Record class.
-    class ThroughReflection < AssociationReflection #:nodoc:
+    class ThroughReflection < AbstractReflection #:nodoc:
+      attr_reader :delegate_reflection
       delegate :foreign_key, :foreign_type, :association_foreign_key,
                :active_record_primary_key, :type, :to => :source_reflection
 
-      def initialize(macro, name, scope, options, active_record)
+      def initialize(delegate_reflection)
-        super
+        @delegate_reflection = delegate_reflection
-        @source_reflection_name = options[:source]
+        @klass         = delegate_reflection.options[:class]
+        @source_reflection_name = delegate_reflection.options[:source]
+      end
+
+      def klass
+        @klass ||= delegate_reflection.compute_class(class_name)
       end
 
       # Returns the source of the through reflection. It checks both a singularized
@@ -777,15 +828,25 @@ directive on your declaration like:
 
       protected
 
-      def actual_source_reflection # FIXME: this is a horrible name
+        def actual_source_reflection # FIXME: this is a horrible name
-        source_reflection.actual_source_reflection
+          source_reflection.send(:actual_source_reflection)
-      end
+        end
+
+        def primary_key(klass)
+          klass.primary_key || raise(UnknownPrimaryKey.new(klass))
+        end
 
       private
         def derive_class_name
           # get the class_name of the belongs_to association of the through reflection
           options[:source_type] || source_reflection.class_name
         end
+
+        delegate_methods = AssociationReflection.public_instance_methods -
+          public_instance_methods
+
+        delegate(*delegate_methods, to: :delegate_reflection)
+
     end
   end
 end

activerecord/test/cases/reflection_test.rb

@@ -87,7 +87,7 @@ class ReflectionTest < ActiveRecord::TestCase
   end
 
   def test_reflection_klass_for_nested_class_name
-    reflection = MacroReflection.new(:company, nil, nil, { :class_name => 'MyApplication::Business::Company' }, ActiveRecord::Base)
+    reflection = ActiveRecord::Reflection.create(:has_many, nil, nil, { :class_name => 'MyApplication::Business::Company' }, ActiveRecord::Base)
     assert_nothing_raised do
       assert_equal MyApplication::Business::Company, reflection.klass
     end
@@ -97,21 +97,21 @@ class ReflectionTest < ActiveRecord::TestCase
     ActiveSupport::Inflector.inflections do |inflect|
       inflect.irregular 'plural_irregular', 'plurales_irregulares'
     end
-    reflection = AssociationReflection.new(:has_many, 'plurales_irregulares', nil, {}, ActiveRecord::Base)
+    reflection = ActiveRecord::Reflection.create(:has_many, 'plurales_irregulares', nil, {}, ActiveRecord::Base)
     assert_equal 'PluralIrregular', reflection.class_name
   end
 
   def test_aggregation_reflection
     reflection_for_address = AggregateReflection.new(
-      :composed_of, :address, nil, { :mapping => [ %w(address_street street), %w(address_city city), %w(address_country country) ] }, Customer
+      :address, nil, { :mapping => [ %w(address_street street), %w(address_city city), %w(address_country country) ] }, Customer
     )
 
     reflection_for_balance = AggregateReflection.new(
-      :composed_of, :balance, nil, { :class_name => "Money", :mapping => %w(balance amount) }, Customer
+      :balance, nil, { :class_name => "Money", :mapping => %w(balance amount) }, Customer
     )
 
     reflection_for_gps_location = AggregateReflection.new(
-      :composed_of, :gps_location, nil, { }, Customer
+      :gps_location, nil, { }, Customer
     )
 
     assert Customer.reflect_on_all_aggregations.include?(reflection_for_gps_location)
@@ -135,7 +135,7 @@ class ReflectionTest < ActiveRecord::TestCase
   end
 
   def test_has_many_reflection
-    reflection_for_clients = AssociationReflection.new(:has_many, :clients, nil, { :order => "id", :dependent => :destroy }, Firm)
+    reflection_for_clients = ActiveRecord::Reflection.create(:has_many, :clients, nil, { :order => "id", :dependent => :destroy }, Firm)
 
     assert_equal reflection_for_clients, Firm.reflect_on_association(:clients)
 
@@ -147,7 +147,7 @@ class ReflectionTest < ActiveRecord::TestCase
   end
 
   def test_has_one_reflection
-    reflection_for_account = AssociationReflection.new(:has_one, :account, nil, { :foreign_key => "firm_id", :dependent => :destroy }, Firm)
+    reflection_for_account = ActiveRecord::Reflection.create(:has_one, :account, nil, { :foreign_key => "firm_id", :dependent => :destroy }, Firm)
     assert_equal reflection_for_account, Firm.reflect_on_association(:account)
 
     assert_equal Account, Firm.reflect_on_association(:account).klass
@@ -284,12 +284,12 @@ class ReflectionTest < ActiveRecord::TestCase
   end
 
   def test_association_primary_key_raises_when_missing_primary_key
-    reflection = ActiveRecord::Reflection::AssociationReflection.new(:fuu, :edge, nil, {}, Author)
+    reflection = ActiveRecord::Reflection.create(:has_many, :edge, nil, {}, Author)
     assert_raises(ActiveRecord::UnknownPrimaryKey) { reflection.association_primary_key }
 
     through = Class.new(ActiveRecord::Reflection::ThroughReflection) {
       define_method(:source_reflection) { reflection }
-    }.new(:fuu, :edge, nil, {}, Author)
+    }.new(reflection)
     assert_raises(ActiveRecord::UnknownPrimaryKey) { through.association_primary_key }
   end
 
@@ -299,7 +299,7 @@ class ReflectionTest < ActiveRecord::TestCase
   end
 
   def test_active_record_primary_key_raises_when_missing_primary_key
-    reflection = ActiveRecord::Reflection::AssociationReflection.new(:fuu, :author, nil, {}, Edge)
+    reflection = ActiveRecord::Reflection.create(:has_many, :author, nil, {}, Edge)
     assert_raises(ActiveRecord::UnknownPrimaryKey) { reflection.active_record_primary_key }
   end
 
@@ -317,32 +317,28 @@ class ReflectionTest < ActiveRecord::TestCase
   end
 
   def test_default_association_validation
-    assert AssociationReflection.new(:has_many, :clients, nil, {}, Firm).validate?
+    assert ActiveRecord::Reflection.create(:has_many, :clients, nil, {}, Firm).validate?
 
-    assert !AssociationReflection.new(:has_one, :client, nil, {}, Firm).validate?
+    assert !ActiveRecord::Reflection.create(:has_one, :client, nil, {}, Firm).validate?
-    assert !AssociationReflection.new(:belongs_to, :client, nil, {}, Firm).validate?
+    assert !ActiveRecord::Reflection.create(:belongs_to, :client, nil, {}, Firm).validate?
-    assert !AssociationReflection.new(:has_and_belongs_to_many, :clients, nil, {}, Firm).validate?
   end
 
   def test_always_validate_association_if_explicit
-    assert AssociationReflection.new(:has_one, :client, nil, { :validate => true }, Firm).validate?
+    assert ActiveRecord::Reflection.create(:has_one, :client, nil, { :validate => true }, Firm).validate?
-    assert AssociationReflection.new(:belongs_to, :client, nil, { :validate => true }, Firm).validate?
+    assert ActiveRecord::Reflection.create(:belongs_to, :client, nil, { :validate => true }, Firm).validate?
-    assert AssociationReflection.new(:has_many, :clients, nil, { :validate => true }, Firm).validate?
+    assert ActiveRecord::Reflection.create(:has_many, :clients, nil, { :validate => true }, Firm).validate?
-    assert AssociationReflection.new(:has_and_belongs_to_many, :clients, nil, { :validate => true }, Firm).validate?
   end
 
   def test_validate_association_if_autosave
-    assert AssociationReflection.new(:has_one, :client, nil, { :autosave => true }, Firm).validate?
+    assert ActiveRecord::Reflection.create(:has_one, :client, nil, { :autosave => true }, Firm).validate?
-    assert AssociationReflection.new(:belongs_to, :client, nil, { :autosave => true }, Firm).validate?
+    assert ActiveRecord::Reflection.create(:belongs_to, :client, nil, { :autosave => true }, Firm).validate?
-    assert AssociationReflection.new(:has_many, :clients, nil, { :autosave => true }, Firm).validate?
+    assert ActiveRecord::Reflection.create(:has_many, :clients, nil, { :autosave => true }, Firm).validate?
-    assert AssociationReflection.new(:has_and_belongs_to_many, :clients, nil, { :autosave => true }, Firm).validate?
   end
 
   def test_never_validate_association_if_explicit
-    assert !AssociationReflection.new(:has_one, :client, nil, { :autosave => true, :validate => false }, Firm).validate?
+    assert !ActiveRecord::Reflection.create(:has_one, :client, nil, { :autosave => true, :validate => false }, Firm).validate?
-    assert !AssociationReflection.new(:belongs_to, :client, nil, { :autosave => true, :validate => false }, Firm).validate?
+    assert !ActiveRecord::Reflection.create(:belongs_to, :client, nil, { :autosave => true, :validate => false }, Firm).validate?
-    assert !AssociationReflection.new(:has_many, :clients, nil, { :autosave => true, :validate => false }, Firm).validate?
+    assert !ActiveRecord::Reflection.create(:has_many, :clients, nil, { :autosave => true, :validate => false }, Firm).validate?
-    assert !AssociationReflection.new(:has_and_belongs_to_many, :clients, nil, { :autosave => true, :validate => false }, Firm).validate?
   end
 
   def test_foreign_key
@@ -364,11 +360,11 @@ class ReflectionTest < ActiveRecord::TestCase
     category = Struct.new(:table_name, :pluralize_table_names).new('categories', true)
     product = Struct.new(:table_name, :pluralize_table_names).new('products', true)
 
-    reflection = AssociationReflection.new(:has_and_belongs_to_many, :categories, nil, {}, product)
+    reflection = ActiveRecord::Reflection.create(:has_many, :categories, nil, {}, product)
     reflection.stubs(:klass).returns(category)
     assert_equal 'categories_products', reflection.join_table
 
-    reflection = AssociationReflection.new(:has_and_belongs_to_many, :products, nil, {}, category)
+    reflection = ActiveRecord::Reflection.create(:has_many, :products, nil, {}, category)
     reflection.stubs(:klass).returns(product)
     assert_equal 'categories_products', reflection.join_table
   end
@@ -377,11 +373,11 @@ class ReflectionTest < ActiveRecord::TestCase
     category = Struct.new(:table_name, :pluralize_table_names).new('catalog_categories', true)
     product = Struct.new(:table_name, :pluralize_table_names).new('catalog_products', true)
 
-    reflection = AssociationReflection.new(:has_and_belongs_to_many, :categories, nil, {}, product)
+    reflection = ActiveRecord::Reflection.create(:has_many, :categories, nil, {}, product)
     reflection.stubs(:klass).returns(category)
     assert_equal 'catalog_categories_products', reflection.join_table
 
-    reflection = AssociationReflection.new(:has_and_belongs_to_many, :products, nil, {}, category)
+    reflection = ActiveRecord::Reflection.create(:has_many, :products, nil, {}, category)
     reflection.stubs(:klass).returns(product)
     assert_equal 'catalog_categories_products', reflection.join_table
   end
@@ -390,11 +386,11 @@ class ReflectionTest < ActiveRecord::TestCase
     category = Struct.new(:table_name, :pluralize_table_names).new('catalog_categories', true)
     page = Struct.new(:table_name, :pluralize_table_names).new('content_pages', true)
 
-    reflection = AssociationReflection.new(:has_and_belongs_to_many, :categories, nil, {}, page)
+    reflection = ActiveRecord::Reflection.create(:has_many, :categories, nil, {}, page)
     reflection.stubs(:klass).returns(category)
     assert_equal 'catalog_categories_content_pages', reflection.join_table
 
-    reflection = AssociationReflection.new(:has_and_belongs_to_many, :pages, nil, {}, category)
+    reflection = ActiveRecord::Reflection.create(:has_many, :pages, nil, {}, category)
     reflection.stubs(:klass).returns(page)
     assert_equal 'catalog_categories_content_pages', reflection.join_table
   end
@@ -403,11 +399,11 @@ class ReflectionTest < ActiveRecord::TestCase
     category = Struct.new(:table_name, :pluralize_table_names).new('categories', true)
     product = Struct.new(:table_name, :pluralize_table_names).new('products', true)
 
-    reflection = AssociationReflection.new(:has_and_belongs_to_many, :categories, nil, { :join_table => 'product_categories' }, product)
+    reflection = ActiveRecord::Reflection.create(:has_many, :categories, nil, { :join_table => 'product_categories' }, product)
     reflection.stubs(:klass).returns(category)
     assert_equal 'product_categories', reflection.join_table
 
-    reflection = AssociationReflection.new(:has_and_belongs_to_many, :products, nil, { :join_table => 'product_categories' }, category)
+    reflection = ActiveRecord::Reflection.create(:has_many, :products, nil, { :join_table => 'product_categories' }, category)
     reflection.stubs(:klass).returns(product)
     assert_equal 'product_categories', reflection.join_table
   end