Associations: DRY up the code which is generating conditions, and make it all use arel rather than SQL strings

7 files changed, 50 insertions, 65 deletions

diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb
index 8bb32400cf..c9f9c925b0 100644
--- a/activerecord/lib/active_record/associations/association_collection.rb
+++ b/activerecord/lib/active_record/associations/association_collection.rb
@@ -111,7 +111,7 @@ module ActiveRecord
         else
           build_record(attributes) do |record|
             block.call(record) if block_given?
-            set_belongs_to_association_for(record)
+            set_owner_attributes(record)
           end
         end
       end
diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb
index f4eceeed8c..f51275d86d 100644
--- a/activerecord/lib/active_record/associations/association_proxy.rb
+++ b/activerecord/lib/active_record/associations/association_proxy.rb
@@ -181,18 +181,41 @@ module ActiveRecord
           @reflection.klass.send(:sanitize_sql, sql, table_name)
         end
 
-        # Assigns the ID of the owner to the corresponding foreign key in +record+.
-        # If the association is polymorphic the type of the owner is also set.
-        def set_belongs_to_association_for(record)
-          if @reflection.options[:as]
-            record["#{@reflection.options[:as]}_id"]   = @owner.id if @owner.persisted?
-            record["#{@reflection.options[:as]}_type"] = @owner.class.base_class.name.to_s
+        # Sets the owner attributes on the given record
+        # Note: does not really make sense for belongs_to associations, but this method is not
+        #       used by belongs_to
+        def set_owner_attributes(record)
+          if @owner.persisted?
+            construct_owner_attributes.each { |key, value| record[key] = value }
+          end
+        end
+
+        # Returns a has linking the owner to the association represented by the reflection
+        def construct_owner_attributes(reflection = @reflection)
+          attributes = {}
+          if reflection.macro == :belongs_to
+            attributes[reflection.association_primary_key] = @owner.send(reflection.primary_key_name)
           else
-            if @owner.persisted?
-              primary_key = @reflection.options[:primary_key] || :id
-              record[@reflection.primary_key_name] = @owner.send(primary_key)
+            attributes[reflection.primary_key_name] = @owner.send(reflection.active_record_primary_key)
+
+            if reflection.options[:as]
+              attributes["#{reflection.options[:as]}_type"] = @owner.class.base_class.name
             end
           end
+          attributes
+        end
+
+        # Builds an array of arel nodes from the owner attributes hash
+        def construct_owner_conditions(table = aliased_table, reflection = @reflection)
+          construct_owner_attributes(reflection).map do |attr, value|
+            table[attr].eq(value)
+          end
+        end
+
+        def construct_conditions
+          conditions = construct_owner_conditions
+          conditions << Arel.sql(sql_conditions) if sql_conditions
+          aliased_table.create_and(conditions)
         end
 
         # Merges into +options+ the ones coming from the reflection.
@@ -232,6 +255,10 @@ module ActiveRecord
           {}
         end
 
+        def aliased_table
+          @reflection.klass.arel_table
+        end
+
       private
         # Forwards any missing method call to the \target.
         def method_missing(method, *args)
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 e17ac6f2cc..24871303eb 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
@@ -75,10 +75,12 @@ module ActiveRecord
           "INNER JOIN #{@owner.connection.quote_table_name @reflection.options[:join_table]} ON #{@reflection.quoted_table_name}.#{@reflection.klass.primary_key} = #{@owner.connection.quote_table_name @reflection.options[:join_table]}.#{@reflection.association_foreign_key}"
         end
 
-        def construct_conditions
-          sql = "#{@owner.connection.quote_table_name @reflection.options[:join_table]}.#{@reflection.primary_key_name} = #{owner_quoted_id} "
-          sql << " AND (#{conditions})" if conditions
-          sql
+        def join_table
+          Arel::Table.new(@reflection.options[:join_table])
+        end
+
+        def construct_owner_conditions
+          super(join_table)
         end
 
         def construct_find_scope
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 2d2f377e1f..d35ecfd603 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -54,7 +54,7 @@ module ActiveRecord
         end
 
         def insert_record(record, force = false, validate = true)
-          set_belongs_to_association_for(record)
+          set_owner_attributes(record)
           save_record(record, force, validate)
         end
 
@@ -79,18 +79,6 @@ module ActiveRecord
           end
         end
 
-        def construct_conditions
-          if @reflection.options[:as]
-            sql =
-              "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_id = #{owner_quoted_id} AND " +
-              "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}"
-          else
-            sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}"
-          end
-          sql << " AND (#{conditions})" if conditions
-          sql
-        end
-
         def construct_find_scope
           {
             :conditions => construct_conditions,
@@ -102,9 +90,7 @@ module ActiveRecord
         end
 
         def construct_create_scope
-          create_scoping = {}
-          set_belongs_to_association_for(create_scoping)
-          create_scoping
+          construct_owner_attributes
         end
 
         def we_can_set_the_inverse_on_this?(record)
diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb
index d08cbea199..fd3827390f 100644
--- a/activerecord/lib/active_record/associations/has_one_association.rb
+++ b/activerecord/lib/active_record/associations/has_one_association.rb
@@ -49,7 +49,7 @@ module ActiveRecord
           @target = nil
         else
           raise_on_type_mismatch(obj)
-          set_belongs_to_association_for(obj)
+          set_owner_attributes(obj)
           @target = (AssociationProxy === obj ? obj.target : obj)
         end
 
@@ -84,22 +84,11 @@ module ActiveRecord
         end
 
         def construct_find_scope
-          if @reflection.options[:as]
-            sql =
-              "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_id = #{owner_quoted_id} AND " +
-              "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}"
-          else
-            test = owner_quoted_id == "NULL" ? "IS" : "="
-            sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} #{test} #{owner_quoted_id}"
-          end
-          sql << " AND (#{conditions})" if conditions
-          { :conditions => sql }
+          { :conditions => construct_conditions }
         end
 
         def construct_create_scope
-          create_scoping = {}
-          set_belongs_to_association_for(create_scoping)
-          create_scoping
+          construct_owner_attributes
         end
 
         def new_record(replace_existing)
diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb
index 415fc53e1c..1abeb178ff 100644
--- a/activerecord/lib/active_record/associations/through_association_scope.rb
+++ b/activerecord/lib/active_record/associations/through_association_scope.rb
@@ -51,26 +51,8 @@ module ActiveRecord
           @reflection.through_reflection.klass.arel_table
       end
 
-      # Build SQL conditions from attributes, qualified by table name.
-      def construct_conditions
-        table = aliased_through_table
-        conditions = construct_owner_attributes(@reflection.through_reflection).map do |attr, value|
-          table[attr].eq(value)
-        end
-        conditions << Arel.sql(sql_conditions) if sql_conditions
-        table.create_and(conditions)
-      end
-
-      # Associate attributes pointing to owner
-      def construct_owner_attributes(reflection)
-        if as = reflection.options[:as]
-          { "#{as}_id"   => @owner[reflection.active_record_primary_key],
-            "#{as}_type" => @owner.class.base_class.name }
-        elsif reflection.macro == :belongs_to
-          { reflection.klass.primary_key => @owner[reflection.primary_key_name] }
-        else
-          { reflection.primary_key_name => @owner[reflection.active_record_primary_key] }
-        end
+      def construct_owner_conditions
+        super(aliased_through_table, @reflection.through_reflection)
       end
 
       def construct_select
diff --git a/activerecord/test/fixtures/companies.yml b/activerecord/test/fixtures/companies.yml
index c75bc5dad3..a982d3921d 100644
--- a/activerecord/test/fixtures/companies.yml
+++ b/activerecord/test/fixtures/companies.yml
@@ -9,7 +9,6 @@ first_client:
 
 first_firm:
   id: 1
-  firm_id: 1
   type: Firm
   name: 37signals
   ruby_type: Firm