From f8d2899d12d59360f29c5eb6a1b1a8fe4ec82ca0 Mon Sep 17 00:00:00 2001 From: eileencodes Date: Mon, 9 Jun 2014 18:45:29 -0400 Subject: [PATCH] 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. --- .../lib/active_record/associations.rb | 2 +- activerecord/lib/active_record/reflection.rb | 191 ++++++++++++------ activerecord/test/cases/reflection_test.rb | 64 +++--- 3 files changed, 157 insertions(+), 100 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index d9b339a1f6..7870e01dc6 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/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 diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 28c39bdd5c..12208aecf7 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -13,14 +13,21 @@ module ActiveRecord end def self.create(macro, name, scope, options, ar) - case macro - when :has_many, :belongs_to, :has_one - klass = options[:through] ? ThroughReflection : AssociationReflection - when :composed_of - klass = AggregateReflection - end + klass = case macro + when :composed_of + AggregateReflection + when :has_many + HasManyReflection + 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. + # + # composed_of :balance, class_name: 'Money' returns 'Money' + # has_many :clients returns 'Client' + 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. # # composed_of :balance, class_name: 'Money' returns :balance @@ -142,8 +195,7 @@ module ActiveRecord attr_reader :plural_name # :nodoc: - def initialize(macro, name, scope, options, active_record) - @macro = macro + def initialize(name, scope, options, active_record) @name = name @scope = scope @options = options @@ -167,15 +219,11 @@ module ActiveRecord # composed_of :balance, class_name: 'Money' returns the Money class # has_many :clients returns the Client class def klass - @klass ||= class_name.constantize + @klass ||= compute_class(class_name) end - # Returns the class name for the macro. - # - # composed_of :balance, class_name: 'Money' returns 'Money' - # has_many :clients returns 'Client' - def class_name - @class_name ||= (options[:class_name] || derive_class_name).to_s + def compute_class(name) + name.constantize 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) - super - @source_reflection_name = options[:source] + def initialize(delegate_reflection) + @delegate_reflection = delegate_reflection + @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 - source_reflection.actual_source_reflection - end + def actual_source_reflection # FIXME: this is a horrible name + source_reflection.send(:actual_source_reflection) + 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 diff --git a/activerecord/test/cases/reflection_test.rb b/activerecord/test/cases/reflection_test.rb index b41a7ee787..84abaf0291 100644 --- a/activerecord/test/cases/reflection_test.rb +++ b/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 !AssociationReflection.new(:belongs_to, :client, nil, {}, Firm).validate? - assert !AssociationReflection.new(:has_and_belongs_to_many, :clients, nil, {}, Firm).validate? + assert !ActiveRecord::Reflection.create(:has_one, :client, nil, {}, Firm).validate? + assert !ActiveRecord::Reflection.create(:belongs_to, :client, nil, {}, Firm).validate? end def test_always_validate_association_if_explicit - assert AssociationReflection.new(:has_one, :client, nil, { :validate => true }, Firm).validate? - assert AssociationReflection.new(:belongs_to, :client, nil, { :validate => true }, Firm).validate? - assert AssociationReflection.new(:has_many, :clients, nil, { :validate => true }, Firm).validate? - assert AssociationReflection.new(:has_and_belongs_to_many, :clients, nil, { :validate => true }, Firm).validate? + assert ActiveRecord::Reflection.create(:has_one, :client, nil, { :validate => true }, Firm).validate? + assert ActiveRecord::Reflection.create(:belongs_to, :client, nil, { :validate => true }, Firm).validate? + assert ActiveRecord::Reflection.create(:has_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 AssociationReflection.new(:belongs_to, :client, nil, { :autosave => true }, Firm).validate? - assert AssociationReflection.new(:has_many, :clients, nil, { :autosave => true }, Firm).validate? - assert AssociationReflection.new(:has_and_belongs_to_many, :clients, nil, { :autosave => true }, Firm).validate? + assert ActiveRecord::Reflection.create(:has_one, :client, nil, { :autosave => true }, Firm).validate? + assert ActiveRecord::Reflection.create(:belongs_to, :client, nil, { :autosave => true }, Firm).validate? + assert ActiveRecord::Reflection.create(:has_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 !AssociationReflection.new(:belongs_to, :client, nil, { :autosave => true, :validate => false }, Firm).validate? - assert !AssociationReflection.new(: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? + assert !ActiveRecord::Reflection.create(:has_one, :client, nil, { :autosave => true, :validate => false }, Firm).validate? + assert !ActiveRecord::Reflection.create(:belongs_to, :client, nil, { :autosave => true, :validate => false }, Firm).validate? + assert !ActiveRecord::Reflection.create(:has_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