Refactor Hash.from_xml.

Three basic refactors in this PR:

* We extracted the logic into a method object. We now don't define a tone of extraneous methods on Hash, even if they were private.
* Extracted blocks of the case statement into methods that do the work. This makes the logic more clear.
* Extracted complicated if clauses into their own query methods. They often have two or three terms, this makes it much easier to see what they _do_.

We took care not to refactor too much as to not break anything, and put comments where we suspect tests are missing.

We think ActiveSupport::XMLMini might be a good candidate to move to a plugin in the future.

activesupport/lib/active_support/core_ext/hash/conversions.rb

@@ -102,55 +102,41 @@ class Hash
     #   hash = Hash.from_xml(xml)
     #   # => {"hash"=>{"foo"=>1, "bar"=>2}}
     def from_xml(xml)
-      typecast_xml_value(unrename_keys(ActiveSupport::XmlMini.parse(xml)))
+      ActiveSupport::XMLConverter.new(xml).to_h
+    end
+
+  end
+end
+
+module ActiveSupport
+  class XMLConverter # :nodoc:
+    def initialize(xml)
+      @xml = normalize_keys(XmlMini.parse(xml))
+    end
+
+    def to_h
+      deep_to_h(@xml)
     end
 
     private
-      def typecast_xml_value(value)
+
+      def normalize_keys(params)
+        case params
+          when Hash
+            Hash[params.map { |k,v| [k.to_s.tr('-', '_'), normalize_keys(v)] } ]
+          when Array
+            params.map { |v| normalize_keys(v) }
+          else
+            params
+        end
+      end
+
+      def deep_to_h(value)
         case value
           when Hash
-            if value['type'] == 'array'
-              _, entries = Array.wrap(value.detect { |k,v| not v.is_a?(String) })
-              if entries.nil? || (c = value['__content__'] && c.blank?)
-                []
-              else
-                case entries  # something weird with classes not matching here.  maybe singleton methods breaking is_a?
-                when Array
-                  entries.collect { |v| typecast_xml_value(v) }
-                when Hash
-                  [typecast_xml_value(entries)]
-                else
-                  raise "can't typecast #{entries.inspect}"
-                end
-              end
-            elsif value['type'] == 'file' ||
-               (value['__content__'] && (value.keys.size == 1 || value['__content__'].present?))
-              content = value['__content__']
-              if parser = ActiveSupport::XmlMini::PARSING[value['type']]
-                parser.arity == 1 ? parser.call(content) : parser.call(content, value)
-              else
-                content
-              end
-            elsif value['type'] == 'string' && value['nil'] != 'true'
-              ''
-            # blank or nil parsed values are represented by nil
-            elsif value.blank? || value['nil'] == 'true'
-              nil
-            # If the type is the only element which makes it then
-            # this still makes the value nil, except if type is
-            # a XML node(where type['value'] is a Hash)
-            elsif value['type'] && value.size == 1 && !value['type'].is_a?(::Hash)
-              nil
-            else
-              xml_value = Hash[value.map { |k,v| [k, typecast_xml_value(v)] }]
-
-              # Turn { files: { file: #<StringIO> } } into { files: #<StringIO> } so it is compatible with
-              # how multipart uploaded files from HTML appear
-              xml_value['file'].is_a?(StringIO) ? xml_value['file'] : xml_value
-            end
+            process_hash(value)
           when Array
-            value.map! { |i| typecast_xml_value(i) }
-            value.length > 1 ? value : value.first
+            process_array(value)
           when String
             value
           else
@@ -158,15 +144,79 @@ class Hash
         end
       end
 
-      def unrename_keys(params)
-        case params
-          when Hash
-            Hash[params.map { |k,v| [k.to_s.tr('-', '_'), unrename_keys(v)] } ]
-          when Array
-            params.map { |v| unrename_keys(v) }
+      def process_hash(value)
+        if become_array?(value)
+          _, entries = Array.wrap(value.detect { |k,v| not v.is_a?(String) })
+          if entries.nil? || value['__content__'].try(:empty?)
+            []
           else
-            params
+            case entries
+            when Array
+              entries.collect { |v| deep_to_h(v) }
+            when Hash
+              [deep_to_h(entries)]
+            else
+              raise "can't typecast #{entries.inspect}"
+            end
+          end
+        elsif become_content?(value)
+          process_content(value)
+
+        elsif become_empty_string?(value)
+          ''
+        elsif become_hash?(value)
+          xml_value = Hash[value.map { |k,v| [k, deep_to_h(v)] }]
+
+          # Turn { files: { file: #<StringIO> } } into { files: #<StringIO> } so it is compatible with
+          # how multipart uploaded files from HTML appear
+          xml_value['file'].is_a?(StringIO) ? xml_value['file'] : xml_value
         end
       end
+
+      def become_content?(value)
+        value['type'] == 'file' || (value['__content__'] && (value.keys.size == 1 || value['__content__'].present?))
+      end
+
+      def become_array?(value)
+        value['type'] == 'array'
+      end
+
+      def become_empty_string?(value)
+        # {"string" => true}
+        # No tests fail when the second term is removed.
+        value['type'] == 'string' && value['nil'] != 'true'
+      end
+
+      def become_hash?(value)
+        !nothing?(value) && !garbage?(value)
+      end
+
+      def nothing?(value)
+        # blank or nil parsed values are represented by nil
+        value.blank? || value['nil'] == 'true'
+      end
+
+      def garbage?(value)
+        # If the type is the only element which makes it then
+        # this still makes the value nil, except if type is
+        # a XML node(where type['value'] is a Hash)
+        value['type'] && !value['type'].is_a?(::Hash) && value.size == 1
+      end
+
+      def process_content(value)
+        content = value['__content__']
+        if parser = ActiveSupport::XmlMini::PARSING[value['type']]
+          parser.arity == 1 ? parser.call(content) : parser.call(content, value)
+        else
+          content
+        end
+      end
+
+      def process_array(value)
+        value.map! { |i| deep_to_h(i) }
+        value.length > 1 ? value : value.first
+      end
+
   end
 end
+

activesupport/test/core_ext/hash_ext_test.rb

@@ -1330,7 +1330,7 @@ class HashToXmlTest < ActiveSupport::TestCase
 
   def test_empty_string_works_for_typecast_xml_value
     assert_nothing_raised do
-      Hash.__send__(:typecast_xml_value, "")
+      ActiveSupport::XMLConverter.new("").to_h
     end
   end