Added proper handling of arrays. Closes #8537 [hasmanyjosh]

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@7074 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

9 files changed, 106 insertions, 38 deletions

diff --git a/activeresource/lib/active_resource/connection.rb b/activeresource/lib/active_resource/connection.rb
index d41171270c..8245c797b3 100644
--- a/activeresource/lib/active_resource/connection.rb
+++ b/activeresource/lib/active_resource/connection.rb
@@ -81,11 +81,7 @@ module ActiveResource
     end
 
     def xml_from_response(response)
-      if response = from_xml_data(Hash.from_xml(response.body))
-        response.first
-      else
-        nil
-      end
+      from_xml_data(Hash.from_xml(response.body))
     end
 
 
@@ -150,21 +146,12 @@ module ActiveResource
       # Manipulate from_xml Hash, because xml_simple is not exactly what we
       # want for ActiveResource.
       def from_xml_data(data)
-        case data
-          when Hash
-            if data.keys.size == 1
-              case data.values.first
-                when Hash  then [ from_xml_data(data.values.first) ]
-                when Array then from_xml_data(data.values.first)
-                else       data.values.first
-              end
-            else
-              data.each_key { |key| data[key] = from_xml_data(data[key]) }
-              data
-            end
-          when Array then data.collect { |val| from_xml_data(val) }
-          else data
+        if data.is_a?(Hash) && data.keys.size == 1
+          data.values.first
+        else
+          data
         end
       end
+      
   end
 end
diff --git a/activeresource/test/base/custom_methods_test.rb b/activeresource/test/base/custom_methods_test.rb
index f591c14260..0e1c00c478 100644
--- a/activeresource/test/base/custom_methods_test.rb
+++ b/activeresource/test/base/custom_methods_test.rb
@@ -6,6 +6,7 @@ class CustomMethodsTest < Test::Unit::TestCase
   def setup
     @matz  = { :id => 1, :name => 'Matz' }.to_xml(:root => 'person')
     @matz_deep  = { :id => 1, :name => 'Matz', :other => 'other' }.to_xml(:root => 'person')
+    @matz_array = [{ :id => 1, :name => 'Matz' }].to_xml(:root => 'people')
     @ryan  = { :name => 'Ryan' }.to_xml(:root => 'person')
     @addy  = { :id => 1, :street => '12345 Street' }.to_xml(:root => 'address')
     @addy_deep  = { :id => 1, :street => '12345 Street', :zip => "27519" }.to_xml(:root => 'address')
@@ -15,8 +16,8 @@ class CustomMethodsTest < Test::Unit::TestCase
       mock.get    "/people/1.xml",             {}, @matz
       mock.get    "/people/1/shallow.xml", {}, @matz
       mock.get    "/people/1/deep.xml", {}, @matz_deep
-      mock.get    "/people/retrieve.xml?name=Matz", {}, "<people>#{@matz}</people>"
-      mock.get    "/people/managers.xml", {}, "<people>#{@matz}</people>"
+      mock.get    "/people/retrieve.xml?name=Matz", {}, @matz_array
+      mock.get    "/people/managers.xml", {}, @matz_array
       mock.put    "/people/1/promote.xml?position=Manager", {}, nil, 204
       mock.put    "/people/promote.xml?name=Matz", {}, nil, 204, {}
       mock.put    "/people/sort.xml?by=name", {}, nil, 204
diff --git a/activeresource/test/base_test.rb b/activeresource/test/base_test.rb
index 6c924447de..328d3ae133 100644
--- a/activeresource/test/base_test.rb
+++ b/activeresource/test/base_test.rb
@@ -9,6 +9,10 @@ class BaseTest < Test::Unit::TestCase
     @david = { :id => 2, :name => 'David' }.to_xml(:root => 'person')
     @addy  = { :id => 1, :street => '12345 Street' }.to_xml(:root => 'address')
     @default_request_headers = { 'Content-Type' => 'application/xml' }
+    @rick = { :name => "Rick", :age => 25 }.to_xml(:root => "person")
+    @people = [{ :id => 1, :name => 'Matz' }, { :id => 2, :name => 'David' }].to_xml(:root => 'people')
+    @people_david = [{ :id => 2, :name => 'David' }].to_xml(:root => 'people')
+    @addresses = [{ :id => 1, :street => '12345 Street' }].to_xml(:root => 'addresses')
     
     ActiveResource::HttpMock.respond_to do |mock|
       mock.get    "/people/1.xml",             {}, @matz
@@ -18,9 +22,9 @@ class BaseTest < Test::Unit::TestCase
       mock.delete "/people/1.xml",             {}, nil, 200
       mock.delete "/people/2.xml",             {}, nil, 400
       mock.get    "/people/99.xml",            {}, nil, 404
-      mock.post   "/people.xml",               {}, "<person><name>Rick</name><age type='integer'>25</age></person>", 201, 'Location' => '/people/5.xml'
-      mock.get    "/people.xml",               {}, "<people>#{@matz}#{@david}</people>"
-      mock.get    "/people/1/addresses.xml",   {}, "<addresses>#{@addy}</addresses>"
+      mock.post   "/people.xml",               {}, @rick, 201, 'Location' => '/people/5.xml'
+      mock.get    "/people.xml",               {}, @people
+      mock.get    "/people/1/addresses.xml",   {}, @addresses
       mock.get    "/people/1/addresses/1.xml", {}, @addy
       mock.get    "/people/1/addresses/2.xml", {}, nil, 404
       mock.get    "/people/2/addresses/1.xml", {}, nil, 404
@@ -225,7 +229,7 @@ class BaseTest < Test::Unit::TestCase
   end
 
   def test_find_all_by_from
-    ActiveResource::HttpMock.respond_to { |m| m.get "/companies/1/people.xml", {}, "<people>#{@david}</people>" }
+    ActiveResource::HttpMock.respond_to { |m| m.get "/companies/1/people.xml", {}, @people_david }
   
     people = Person.find(:all, :from => "/companies/1/people.xml")
     assert_equal 1, people.size
@@ -233,7 +237,7 @@ class BaseTest < Test::Unit::TestCase
   end
 
   def test_find_all_by_from_with_options
-    ActiveResource::HttpMock.respond_to { |m| m.get "/companies/1/people.xml", {}, "<people>#{@david}</people>" }
+    ActiveResource::HttpMock.respond_to { |m| m.get "/companies/1/people.xml", {}, @people_david }
   
     people = Person.find(:all, :from => "/companies/1/people.xml")
     assert_equal 1, people.size
@@ -241,7 +245,7 @@ class BaseTest < Test::Unit::TestCase
   end
 
   def test_find_all_by_symbol_from
-    ActiveResource::HttpMock.respond_to { |m| m.get "/people/managers.xml", {}, "<people>#{@david}</people>" }
+    ActiveResource::HttpMock.respond_to { |m| m.get "/people/managers.xml", {}, @people_david }
   
     people = Person.find(:all, :from => :managers)
     assert_equal 1, people.size
diff --git a/activeresource/test/connection_test.rb b/activeresource/test/connection_test.rb
index 13b518d782..24893f8cd3 100644
--- a/activeresource/test/connection_test.rb
+++ b/activeresource/test/connection_test.rb
@@ -115,7 +115,7 @@ class ConnectionTest < Test::Unit::TestCase
   
   def test_get_collection_empty
     people = @conn.get("/people_empty_elements.xml")
-    assert_nil people
+    assert_equal [], people
   end
 
   def test_post
diff --git a/activesupport/CHANGELOG b/activesupport/CHANGELOG
index 208b276624..75ff34dce9 100644
--- a/activesupport/CHANGELOG
+++ b/activesupport/CHANGELOG
@@ -1,5 +1,27 @@
 *SVN*
 
+* Added proper handling of arrays #8537 [hasmanyjosh]
+
+  Before:
+    Hash.from_xml '<images></images>'
+    # => {:images => nil}
+
+    Hash.from_xml '<images><image>foo.jpg</image></images>'
+    # => {:images => {:image => "foo.jpg"}}
+
+    Hash.from_xml '<images><image>foo.jpg</image><image>bar.jpg</image></images>'
+    # => {:images => {:image => ["foo.jpg", "bar.jpg"]}}
+
+  After:
+    Hash.from_xml '<images type="array"></images>'
+    # => {:images => []}
+
+    Hash.from_xml '<images type="array"><image>foo.jpg</image></images>'
+    # => {:images => ["foo.jpg"]}
+
+    Hash.from_xml '<images type="array"><image>foo.jpg</image><image>bar.jpg</image></images>'
+    # => {:images => ["foo.jpg", "bar.jpg"]}
+
 * Improve Time and Date test coverage.  #8646 [Josh Peek]
 
 * Add Date#since, ago, beginning_of_day, and end_of_day. Date + seconds works now.  #8575 [Geoff Buesing]
diff --git a/activesupport/lib/active_support/core_ext/array/conversions.rb b/activesupport/lib/active_support/core_ext/array/conversions.rb
index a4d056e81c..426e53ea1c 100644
--- a/activesupport/lib/active_support/core_ext/array/conversions.rb
+++ b/activesupport/lib/active_support/core_ext/array/conversions.rb
@@ -63,10 +63,15 @@ module ActiveSupport #:nodoc:
 
           opts = options.merge({ :root => children })
 
-          options[:builder].tag!(root) {
-            yield options[:builder] if block_given?
-            each { |e| e.to_xml(opts.merge!({ :skip_instruct => true })) }
-          }
+          xml = options[:builder]
+          if empty?
+            xml.tag!(root, options[:skip_types] ? {} : {:type => "array"})
+          else
+            xml.tag!(root, options[:skip_types] ? {} : {:type => "array"}) {
+              yield xml if block_given?
+              each { |e| e.to_xml(opts.merge!({ :skip_instruct => true })) }
+            }
+          end
         end
 
       end
diff --git a/activesupport/lib/active_support/core_ext/hash/conversions.rb b/activesupport/lib/active_support/core_ext/hash/conversions.rb
index 2334bb671b..2cccd9c30e 100644
--- a/activesupport/lib/active_support/core_ext/hash/conversions.rb
+++ b/activesupport/lib/active_support/core_ext/hash/conversions.rb
@@ -163,6 +163,20 @@ module ActiveSupport #:nodoc:
                     else
                       content
                     end
+                  elsif value['type'] == 'array'
+                    child_key, entries = value.detect { |k,v| k != 'type' }   # child_key is throwaway
+                    if entries.nil?
+                      []
+                    else
+                      case entries.class.to_s   # 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'] == 'string' && value['nil'] != 'true'
                     ""
                   else
diff --git a/activesupport/test/core_ext/array_ext_test.rb b/activesupport/test/core_ext/array_ext_test.rb
index e5ca4c424f..8c5356ac78 100644
--- a/activesupport/test/core_ext/array_ext_test.rb
+++ b/activesupport/test/core_ext/array_ext_test.rb
@@ -121,7 +121,7 @@ class ArrayToXmlTests < Test::Unit::TestCase
       { :name => "Jason", :age => 31, :age_in_millis => BigDecimal.new('1.0') }
     ].to_xml(:skip_instruct => true, :indent => 0)
 
-    assert_equal "<records><record>", xml.first(17), xml
+    assert_equal '<records type="array"><record>', xml.first(30)
     assert xml.include?(%(<age type="integer">26</age>)), xml
     assert xml.include?(%(<age-in-millis type="integer">820497600000</age-in-millis>)), xml
     assert xml.include?(%(<name>David</name>)), xml
@@ -135,7 +135,7 @@ class ArrayToXmlTests < Test::Unit::TestCase
       { :name => "David", :age => 26, :age_in_millis => 820497600000 }, { :name => "Jason", :age => 31 }
     ].to_xml(:skip_instruct => true, :indent => 0, :root => "people")
 
-    assert_equal "<people><person>", xml.first(16)
+    assert_equal '<people type="array"><person>', xml.first(29)
   end
 
   def test_to_xml_with_options
diff --git a/activesupport/test/core_ext/hash_ext_test.rb b/activesupport/test/core_ext/hash_ext_test.rb
index a428fe5061..cae19a9335 100644
--- a/activesupport/test/core_ext/hash_ext_test.rb
+++ b/activesupport/test/core_ext/hash_ext_test.rb
@@ -370,7 +370,7 @@ class HashToXmlTest < Test::Unit::TestCase
   def test_two_levels_with_array
     xml = { :name => "David", :addresses => [{ :street => "Paulina" }, { :street => "Evergreen" }] }.to_xml(@xml_options)
     assert_equal "<person>", xml.first(8)
-    assert xml.include?(%(<addresses><address>))
+    assert xml.include?(%(<addresses type="array"><address>))
     assert xml.include?(%(<address><street>Paulina</street></address>))
     assert xml.include?(%(<address><street>Evergreen</street></address>))
     assert xml.include?(%(<name>David</name>))
@@ -378,7 +378,7 @@ class HashToXmlTest < Test::Unit::TestCase
 
   def test_three_levels_with_array
     xml = { :name => "David", :addresses => [{ :streets => [ { :name => "Paulina" }, { :name => "Paulina" } ] } ] }.to_xml(@xml_options)
-    assert xml.include?(%(<addresses><address><streets><street><name>))
+    assert xml.include?(%(<addresses type="array"><address><streets type="array"><street><name>))
   end
 
   def test_single_record_from_xml
@@ -447,7 +447,7 @@ class HashToXmlTest < Test::Unit::TestCase
 
   def test_multiple_records_from_xml
     topics_xml = <<-EOT
-      <topics>
+      <topics type="array">
         <topic>
           <title>The First Topic</title>
           <author-name>David</author-name>
@@ -491,7 +491,7 @@ class HashToXmlTest < Test::Unit::TestCase
       :parent_id => nil
     }.stringify_keys
 
-    assert_equal expected_topic_hash, Hash.from_xml(topics_xml)["topics"]["topic"].first
+    assert_equal expected_topic_hash, Hash.from_xml(topics_xml)["topics"].first
   end
 
   def test_single_record_from_xml_with_attributes_other_than_type
@@ -516,6 +516,41 @@ class HashToXmlTest < Test::Unit::TestCase
 
     assert_equal expected_topic_hash, Hash.from_xml(topic_xml)["rsp"]["photos"]["photo"]
   end
+  
+  def test_empty_array_from_xml
+    blog_xml = <<-XML
+      <blog>
+        <posts type="array"></posts>
+      </blog>
+    XML
+    expected_blog_hash = {"blog" => {"posts" => []}}
+    assert_equal expected_blog_hash, Hash.from_xml(blog_xml)
+  end
+
+  def test_array_with_one_entry_from_xml
+    blog_xml = <<-XML
+      <blog>
+        <posts type="array">
+          <post>a post</post>
+        </posts>
+      </blog>
+    XML
+    expected_blog_hash = {"blog" => {"posts" => ["a post"]}}
+    assert_equal expected_blog_hash, Hash.from_xml(blog_xml)
+  end
+
+  def test_array_with_multiple_entries_from_xml
+    blog_xml = <<-XML
+      <blog>
+        <posts type="array">
+          <post>a post</post>
+          <post>another post</post>
+        </posts>
+      </blog>
+    XML
+    expected_blog_hash = {"blog" => {"posts" => ["a post", "another post"]}}
+    assert_equal expected_blog_hash, Hash.from_xml(blog_xml)
+  end
 
   def test_xsd_like_types_from_xml
     bacon_xml = <<-EOT