Let's be less blasé about method visibility on association proxies

9 files changed, 135 insertions, 117 deletions

diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb
index a37c3dd432..3c939b0ef0 100644
--- a/activerecord/lib/active_record/associations/association_collection.rb
+++ b/activerecord/lib/active_record/associations/association_collection.rb
@@ -333,6 +333,24 @@ module ActiveRecord
         super || @reflection.klass.respond_to?(method, include_private)
       end
 
+      def method_missing(method, *args, &block)
+        match = DynamicFinderMatch.match(method)
+        if match && match.creator?
+          attributes = match.attribute_names
+          return send(:"find_by_#{attributes.join('_and_')}", *args) || create(Hash[attributes.zip(args)])
+        end
+
+        if @target.respond_to?(method) || (!@reflection.klass.respond_to?(method) && Class.respond_to?(method))
+          super
+        elsif @reflection.klass.scopes[method]
+          @_scopes_cache ||= {}
+          @_scopes_cache[method] ||= {}
+          @_scopes_cache[method][args] ||= scoped.readonly(nil).send(method, *args)
+        else
+          scoped.readonly(nil).send(method, *args, &block)
+        end
+      end
+
       protected
 
         def association_scope
@@ -340,14 +358,6 @@ module ActiveRecord
           super.apply_finder_options(options)
         end
 
-        def select_value
-          super || uniq_select_value
-        end
-
-        def uniq_select_value
-          @reflection.options[:uniq] && "DISTINCT #{@reflection.quoted_table_name}.*"
-        end
-
         def load_target
           if (!@owner.new_record? || foreign_key_present?) && !loaded?
             targets = []
@@ -365,22 +375,28 @@ module ActiveRecord
           target
         end
 
-        def method_missing(method, *args, &block)
-          match = DynamicFinderMatch.match(method)
-          if match && match.creator?
-            attributes = match.attribute_names
-            return send(:"find_by_#{attributes.join('_and_')}", *args) || create(Hash[attributes.zip(args)])
-          end
-
-          if @target.respond_to?(method) || (!@reflection.klass.respond_to?(method) && Class.respond_to?(method))
-            super
-          elsif @reflection.klass.scopes[method]
-            @_scopes_cache ||= {}
-            @_scopes_cache[method] ||= {}
-            @_scopes_cache[method][args] ||= scoped.readonly(nil).send(method, *args)
+        def add_record_to_target_with_callbacks(record)
+          callback(:before_add, record)
+          yield(record) if block_given?
+          @target ||= [] unless loaded?
+          if @reflection.options[:uniq] && index = @target.index(record)
+            @target[index] = record
           else
-            scoped.readonly(nil).send(method, *args, &block)
+            @target << record
           end
+          callback(:after_add, record)
+          set_inverse_instance(record)
+          record
+        end
+
+      private
+
+        def select_value
+          super || uniq_select_value
+        end
+
+        def uniq_select_value
+          @reflection.options[:uniq] && "DISTINCT #{@reflection.quoted_table_name}.*"
         end
 
         def custom_counter_sql
@@ -419,21 +435,6 @@ module ActiveRecord
           records
         end
 
-        def add_record_to_target_with_callbacks(record)
-          callback(:before_add, record)
-          yield(record) if block_given?
-          @target ||= [] unless loaded?
-          if @reflection.options[:uniq] && index = @target.index(record)
-            @target[index] = record
-          else
-            @target << record
-          end
-          callback(:after_add, record)
-          set_inverse_instance(record)
-          record
-        end
-
-      private
         def merge_target_lists(loaded, existing)
           return loaded if existing.empty?
           return existing if loaded.empty?
diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb
index addc64cb42..e7c9bbd192 100644
--- a/activerecord/lib/active_record/associations/association_proxy.rb
+++ b/activerecord/lib/active_record/associations/association_proxy.rb
@@ -84,6 +84,16 @@ module ActiveRecord
         super || (load_target && @target.respond_to?(*args))
       end
 
+      # Forwards any missing method call to the \target.
+      def method_missing(method, *args, &block)
+        if load_target
+          return super unless @target.respond_to?(method)
+          @target.send(method, *args, &block)
+        end
+      rescue NoMethodError => e
+        raise e, e.message.sub(/ for #<.*$/, " via proxy for #{@target}")
+      end
+
       # Forwards <tt>===</tt> explicitly to the \target because the instance method
       # removal above doesn't catch it. Loads the \target if needed.
       def ===(other)
@@ -167,14 +177,6 @@ module ActiveRecord
       end
 
       protected
-        def interpolate_sql(sql, record = nil)
-          @owner.send(:interpolate_sql, sql, record)
-        end
-
-        # Forwards the call to the reflection class.
-        def sanitize_sql(sql, table_name = @reflection.klass.table_name)
-          @reflection.klass.send(:sanitize_sql, sql, table_name)
-        end
 
         # Construct the scope for this association.
         #
@@ -190,19 +192,12 @@ module ActiveRecord
           scope = target_klass.unscoped
           scope = scope.create_with(creation_attributes)
           scope = scope.apply_finder_options(@reflection.options.slice(:conditions, :readonly, :include))
-          scope = scope.select(select_value) if select_value = self.select_value
+          if select = select_value
+            scope = scope.select(select)
+          end
           scope.where(construct_owner_conditions)
         end
 
-        def select_value
-          @reflection.options[:select]
-        end
-
-        # Implemented by (some) subclasses
-        def creation_attributes
-          { }
-        end
-
         def aliased_table
           target_klass.arel_table
         end
@@ -227,6 +222,47 @@ module ActiveRecord
           target_klass.scoped
         end
 
+        # Loads the \target if needed and returns it.
+        #
+        # This method is abstract in the sense that it relies on +find_target+,
+        # which is expected to be provided by descendants.
+        #
+        # If the \target is already \loaded it is just returned. Thus, you can call
+        # +load_target+ unconditionally to get the \target.
+        #
+        # ActiveRecord::RecordNotFound is rescued within the method, and it is
+        # not reraised. The proxy is \reset and +nil+ is the return value.
+        def load_target
+          if !loaded? && (!@owner.new_record? || foreign_key_present?) && target_klass
+            @target = find_target
+          end
+
+          loaded
+          @target
+        rescue ActiveRecord::RecordNotFound
+          reset
+        end
+
+      private
+
+        def interpolate_sql(sql, record = nil)
+          @owner.send(:interpolate_sql, sql, record)
+        end
+
+        # Forwards the call to the reflection class.
+        def sanitize_sql(sql, table_name = @reflection.klass.table_name)
+          @reflection.klass.send(:sanitize_sql, sql, table_name)
+        end
+
+        def select_value
+          @reflection.options[:select]
+        end
+
+        # Implemented by (some) subclasses
+        def creation_attributes
+          { }
+        end
+
         # Returns a hash linking the owner to the association represented by the reflection
         def construct_owner_attributes(reflection = @reflection)
           attributes = {}
@@ -257,39 +293,6 @@ module ActiveRecord
           end
         end
 
-        # Loads the \target if needed and returns it.
-        #
-        # This method is abstract in the sense that it relies on +find_target+,
-        # which is expected to be provided by descendants.
-        #
-        # If the \target is already \loaded it is just returned. Thus, you can call
-        # +load_target+ unconditionally to get the \target.
-        #
-        # ActiveRecord::RecordNotFound is rescued within the method, and it is
-        # not reraised. The proxy is \reset and +nil+ is the return value.
-        def load_target
-          if !loaded? && (!@owner.new_record? || foreign_key_present?) && target_klass
-            @target = find_target
-          end
-
-          loaded
-          @target
-        rescue ActiveRecord::RecordNotFound
-          reset
-        end
-
-      private
-
-        # Forwards any missing method call to the \target.
-        def method_missing(method, *args, &block)
-          if load_target
-            return super unless @target.respond_to?(method)
-            @target.send(method, *args, &block)
-          end
-        rescue NoMethodError => e
-          raise e, e.message.sub(/ for #<.*$/, " via proxy for #{@target}")
-        end
-
         # Should be true if there is a foreign key present on the @owner which
         # references the target. This is used to determine whether we can load
         # the target if the @owner is currently a new record (and therefore
diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb
index e80b945dda..178c7204f8 100644
--- a/activerecord/lib/active_record/associations/belongs_to_association.rb
+++ b/activerecord/lib/active_record/associations/belongs_to_association.rb
@@ -19,6 +19,7 @@ module ActiveRecord
       end
 
       private
+
         def update_counters(record)
           counter_cache_name = @reflection.counter_cache_column
 
diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
index b28554dce1..6ca287a4e3 100644
--- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
@@ -12,10 +12,6 @@ module ActiveRecord
 
       protected
 
-        def count_records
-          load_target.size
-        end
-
         def insert_record(record, force = true, validate = true)
           if record.new_record?
             return false unless save_record(record, force, validate)
@@ -35,6 +31,16 @@ module ActiveRecord
           true
         end
 
+        def association_scope
+          super.joins(construct_joins)
+        end
+
+      private
+
+        def count_records
+          load_target.size
+        end
+
         def delete_records(records)
           if sql = @reflection.options[:delete_sql]
             records.each { |record| @owner.connection.delete(interpolate_sql(sql, record)) }
@@ -61,15 +67,10 @@ module ActiveRecord
           super(join_table)
         end
 
-        def association_scope
-          super.joins(construct_joins)
-        end
-
         def select_value
           super || @reflection.klass.arel_table[Arel.star]
         end
 
-      private
         def invertible_for?(record)
           false
         end
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index b07441f3c6..002e1a9ae8 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -7,6 +7,14 @@ module ActiveRecord
     # is provided by its child HasManyThroughAssociation.
     class HasManyAssociation < AssociationCollection #:nodoc:
       protected
+
+        def insert_record(record, force = false, validate = true)
+          set_owner_attributes(record)
+          save_record(record, force, validate)
+        end
+
+      private
+
         # Returns the number of records in this collection.
         #
         # If the association has a counter cache it gets that value. Otherwise
@@ -45,11 +53,6 @@ module ActiveRecord
           "#{@reflection.name}_count"
         end
 
-        def insert_record(record, force = false, validate = true)
-          set_owner_attributes(record)
-          save_record(record, force, validate)
-        end
-
         # Deletes the records according to the <tt>:dependent</tt> option.
         def delete_records(records)
           case @reflection.options[:dependent]
diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb
index 400db6baf1..d5b901beff 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -30,13 +30,6 @@ module ActiveRecord
       end
 
       protected
-        def target_reflection_has_associated_record?
-          if @reflection.through_reflection.macro == :belongs_to && @owner[@reflection.through_reflection.foreign_key].blank?
-            false
-          else
-            true
-          end
-        end
 
         def insert_record(record, force = true, validate = true)
           if record.new_record?
@@ -47,6 +40,16 @@ module ActiveRecord
           through_association.create!(construct_join_attributes(record))
         end
 
+      private
+
+        def target_reflection_has_associated_record?
+          if @reflection.through_reflection.macro == :belongs_to && @owner[@reflection.through_reflection.foreign_key].blank?
+            false
+          else
+            true
+          end
+        end
+
         # TODO - add dependent option support
         def delete_records(records)
           through_association = @owner.send(@reflection.through_reflection.name)
diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb
index 6614cbbf18..a0828dcdea 100644
--- a/activerecord/lib/active_record/associations/has_one_association.rb
+++ b/activerecord/lib/active_record/associations/has_one_association.rb
@@ -26,11 +26,14 @@ module ActiveRecord
         self.target = record
       end
 
-      private
+      protected
+
         def association_scope
           super.order(@reflection.options[:order])
         end
 
+      private
+
         alias creation_attributes construct_owner_attributes
 
         # The reason that the save param for replace is false, if for create (not just build),
diff --git a/activerecord/lib/active_record/associations/singular_association.rb b/activerecord/lib/active_record/associations/singular_association.rb
index b43b52fc52..7f92d9712a 100644
--- a/activerecord/lib/active_record/associations/singular_association.rb
+++ b/activerecord/lib/active_record/associations/singular_association.rb
@@ -14,6 +14,7 @@ module ActiveRecord
       end
 
       private
+
         def find_target
           scoped.first.tap { |record| set_inverse_instance(record) }
         end
diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb
index d2112fb2b6..9c62ace4fb 100644
--- a/activerecord/lib/active_record/associations/through_association.rb
+++ b/activerecord/lib/active_record/associations/through_association.rb
@@ -3,6 +3,13 @@ module ActiveRecord
   module Associations
     module ThroughAssociation
 
+      def conditions
+        @conditions = build_conditions unless defined?(@conditions)
+        @conditions
+      end
+
+      alias_method :sql_conditions, :conditions
+
       protected
 
       def target_scope
@@ -17,6 +24,8 @@ module ActiveRecord
         scope
       end
 
+      private
+
       # This scope affects the creation of the associated records (not the join records). At the
       # moment we only support creating on a :through association when the source reflection is a
       # belongs_to. Thus it's not necessary to set a foreign key on the associated record(s), so
@@ -92,11 +101,6 @@ module ActiveRecord
         join_attributes
       end
 
-      def conditions
-        @conditions = build_conditions unless defined?(@conditions)
-        @conditions
-      end
-
       def build_conditions
         through_conditions = build_through_conditions
         source_conditions = @reflection.source_reflection.options[:conditions]
@@ -127,8 +131,6 @@ module ActiveRecord
         @reflection.through_reflection.klass.send(:type_condition).to_sql
       end
 
-      alias_method :sql_conditions, :conditions
-
       def stale_state
         if @reflection.through_reflection.macro == :belongs_to
           @owner[@reflection.through_reflection.foreign_key].to_s