Merge pull request #5294 from robinroestenburg/master

Refactoring of `arel_attributes_values` method

3 files changed, 53 insertions, 26 deletions

diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb
index 93c243e7b1..bebf6a1a05 100644
--- a/activerecord/lib/active_record/attribute_methods.rb
+++ b/activerecord/lib/active_record/attribute_methods.rb
@@ -214,37 +214,64 @@ module ActiveRecord
       value
     end
 
-    # Returns a copy of the attributes hash where all the values have been safely quoted for use in
-    # an Arel insert/update method.
-    def arel_attributes_values(include_primary_key = true, include_readonly_attributes = true, attribute_names = @attributes.keys)
-      attrs      = {}
-      klass      = self.class
-      arel_table = klass.arel_table
+    def arel_attributes_with_values_for_create(pk_attribute_allowed)
+      arel_attributes_with_values(attributes_for_create(pk_attribute_allowed))
+    end
 
-      attribute_names.each do |name|
-        if (column = column_for_attribute(name)) && (include_primary_key || !column.primary)
+    def arel_attributes_with_values_for_update(attribute_names)
+      arel_attributes_with_values(attributes_for_update(attribute_names))
+    end
 
-          if include_readonly_attributes || !self.class.readonly_attributes.include?(name)
+    def attribute_method?(attr_name)
+      defined?(@attributes) && @attributes.include?(attr_name)
+    end
 
-            value = if klass.serialized_attributes.include?(name)
-                      @attributes[name].serialized_value
-                    else
-                      # FIXME: we need @attributes to be used consistently.
-                      # If the values stored in @attributes were already type
-                      # casted, this code could be simplified
-                      read_attribute(name)
-                    end
+    private
 
-            attrs[arel_table[name]] = value
-          end
-        end
-      end
+    # Returns a Hash of the Arel::Attributes and attribute values that have been
+    # type casted for use in an Arel insert/update method.
+    def arel_attributes_with_values(attribute_names)
+      attrs = {}
+      arel_table = self.class.arel_table
 
+      attribute_names.each do |name|
+        attrs[arel_table[name]] = typecasted_attribute_value(name)
+      end
       attrs
     end
 
-    def attribute_method?(attr_name)
-      defined?(@attributes) && @attributes.include?(attr_name)
+    # Filters the primary keys and readonly attributes from the attribute names.
+    def attributes_for_update(attribute_names)
+      attribute_names.select do |name|
+        column_for_attribute(name) && !pk_attribute?(name) && !readonly_attribute?(name)
+      end
+    end
+
+    # Filters out the primary keys, from the attribute names, when the primary
+    # key is to be generated (e.g. the id attribute has no value).
+    def attributes_for_create(pk_attribute_allowed)
+      @attributes.keys.select do |name|
+        column_for_attribute(name) && (pk_attribute_allowed || !pk_attribute?(name))
+      end
+    end
+
+    def readonly_attribute?(name)
+      self.class.readonly_attributes.include?(name)
+    end
+
+    def pk_attribute?(name)
+      column_for_attribute(name).primary
+    end
+
+    def typecasted_attribute_value(name)
+      if self.class.serialized_attributes.include?(name)
+        @attributes[name].serialized_value
+      else
+        # FIXME: we need @attributes to be used consistently.
+        # If the values stored in @attributes were already typecasted, this code
+        # could be simplified
+        read_attribute(name)
+      end
     end
   end
 end
diff --git a/activerecord/lib/active_record/locking/optimistic.rb b/activerecord/lib/active_record/locking/optimistic.rb
index 9b2dc096a0..8266427b71 100644
--- a/activerecord/lib/active_record/locking/optimistic.rb
+++ b/activerecord/lib/active_record/locking/optimistic.rb
@@ -84,7 +84,7 @@ module ActiveRecord
               relation.table[self.class.primary_key].eq(id).and(
                 relation.table[lock_col].eq(quote_value(previous_lock_value))
               )
-            ).arel.compile_update(arel_attributes_values(false, false, attribute_names))
+            ).arel.compile_update(arel_attributes_with_values_for_update(attribute_names))
 
             affected_rows = connection.update stmt
 
diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
index c4bce87311..09502bb52c 100644
--- a/activerecord/lib/active_record/persistence.rb
+++ b/activerecord/lib/active_record/persistence.rb
@@ -350,7 +350,7 @@ module ActiveRecord
     # Updates the associated record with values matching those of the instance attributes.
     # Returns the number of affected rows.
     def update(attribute_names = @attributes.keys)
-      attributes_with_values = arel_attributes_values(false, false, attribute_names)
+      attributes_with_values = arel_attributes_with_values_for_update(attribute_names)
       return 0 if attributes_with_values.empty?
       klass = self.class
       stmt = klass.unscoped.where(klass.arel_table[klass.primary_key].eq(id)).arel.compile_update(attributes_with_values)
@@ -360,7 +360,7 @@ module ActiveRecord
     # Creates a record with values matching those of the instance attributes
     # and returns its id.
     def create
-      attributes_values = arel_attributes_values(!id.nil?)
+      attributes_values = arel_attributes_with_values_for_create(!id.nil?)
 
       new_id = self.class.unscoped.insert attributes_values