Fix issues with ActiveResource collection handling. Closes #6291. [bmilekic]

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

activeresource/CHANGELOG

@@ -1,5 +1,7 @@
 *SVN*
 
+* Fix issues with ActiveResource collection handling.  Closes #6291. [bmilekic]
+
 * Use attr_accessor_with_default to dry up attribute initialization. References #6538. [Stuart Halloway]
 
 * Add basic logging support for logging outgoing requests. [Jamis Buck]

activeresource/lib/active_resource/base.rb

@@ -20,7 +20,7 @@ module ActiveResource
         @connection
       end
 
-      attr_accessor_with_default(:element_name) { to_s.underscore }
+      attr_accessor_with_default(:element_name)    { to_s.underscore }
       attr_accessor_with_default(:collection_name) { element_name.pluralize }
       attr_accessor_with_default(:primary_key, 'id')
       
@@ -69,14 +69,14 @@ module ActiveResource
       end
 
       private
-        # { :people => { :person => [ person1, person2 ] } }
         def find_every(options)
-          connection.get(collection_path(options)).values.first.values.first.collect { |element| new(element, options) }
+          collection = connection.get(collection_path(options)) || []
+          collection.collect! { |element| new(element, options) }
         end
 
         # { :person => person1 }
         def find_single(scope, options)
-          new(connection.get(element_path(scope, options)).values.first, options)
+          new(connection.get(element_path(scope, options)), options)
         end
     end
 
@@ -129,17 +129,8 @@ module ActiveResource
               resource = find_or_create_resource_for_collection(key)
               value.map { |attrs| resource.new(attrs) }
             when Hash
-              # Workaround collections loaded as Hash
-              #   :persons => { :person => [
-              #     { :id => 1, :name => 'a' },
-              #     { :id => 2, :name => 'b' } ]}
-              if value.keys.size == 1 and value.values.first.is_a?(Array)
-                resource = find_or_create_resource_for(value.keys.first)
-                value.values.first.map { |attrs| resource.new(attrs) }
-              else
-                resource = find_or_create_resource_for(key)
-                resource.new(value)
-              end
+              resource = find_or_create_resource_for(key)
+              resource.new(value)
             when ActiveResource::Base
               value.class.new(value.attributes)
             else
@@ -189,7 +180,7 @@ module ActiveResource
       rescue NameError
         resource = self.class.const_set(resource_name, Class.new(ActiveResource::Base))
         resource.prefix = self.class.prefix
-        resource.site = self.class.site
+        resource.site   = self.class.site
         resource
       end
 

activeresource/lib/active_resource/connection.rb

@@ -49,7 +49,7 @@ module ActiveResource
     end
 
     def get(path)
-      Hash.from_xml(request(:get, path, build_request_headers).body)
+      from_xml_data(Hash.from_xml(request(:get, path, build_request_headers).body).values.first)
     end
 
     def delete(path)
@@ -113,5 +113,25 @@ module ActiveResource
       def logger
         ActiveResource::Base.logger
       end
+
+      # 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
+              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
+        end
+      end
   end
 end

activeresource/test/authorization_test.rb

@@ -10,7 +10,7 @@ class AuthorizationTest < Test::Unit::TestCase
     @david = { :id => 2, :name => 'David' }.to_xml(:root => 'person')
     @authenticated_conn = ActiveResource::Connection.new("http://david:test123@localhost")
     @authorization_request_header = { 'Authorization' => 'Basic ZGF2aWQ6dGVzdDEyMw==' }
-    
+
     ActiveResource::HttpMock.respond_to do |mock|
       mock.get    "/people/2.xml",           @authorization_request_header, @david
       mock.put    "/people/2.xml",           @authorization_request_header, nil, 204
@@ -48,7 +48,7 @@ class AuthorizationTest < Test::Unit::TestCase
   
   def test_get
     david = @authenticated_conn.get("/people/2.xml")
-    assert_equal "David", david["person"]["name"]
+    assert_equal "David", david["name"]
   end
   
   def test_post

activeresource/test/base/load_test.rb

@@ -8,13 +8,14 @@ class BaseLoadTest < Test::Unit::TestCase
 
     @first_address = { :id => 1, :street => '12345 Street' }
     @addresses = [@first_address, { :id => 2, :street => '67890 Street' }]
-    @addresses_from_xml = { :street_addresses => { :street_address => @addresses }}
+    @addresses_from_xml = { :street_addresses => @addresses }
+    @addresses_from_xml_single = { :street_addresses => [ @first_address ] }
 
     @deep  = { :id => 1, :street => {
       :id => 1, :state => { :id => 1, :name => 'Oregon',
-        :notable_rivers => { :notable_river => [
+        :notable_rivers => [
           { :id => 1, :name => 'Willamette' },
-          { :id => 2, :name => 'Columbia', :rafted_by => @matz }] }}}}
+          { :id => 2, :name => 'Columbia', :rafted_by => @matz }] }}}
 
     @person = Person.new
   end
@@ -49,6 +50,7 @@ class BaseLoadTest < Test::Unit::TestCase
   end
 
   def test_load_collection_with_unknown_resource
+    Person.send(:remove_const, :Address) if Person.const_defined?(:Address)
     assert !Person.const_defined?(:Address), "Address shouldn't exist until autocreated"
     addresses = silence_warnings { @person.load(:addresses => @addresses).addresses }
     assert Person.const_defined?(:Address), "Address should have been autocreated"
@@ -56,6 +58,22 @@ class BaseLoadTest < Test::Unit::TestCase
     assert_equal @addresses.map(&:stringify_keys), addresses.map(&:attributes)
   end
 
+  def test_load_collection_with_single_existing_resource
+    addresses = @person.load(@addresses_from_xml_single).street_addresses
+    assert_kind_of Array, addresses
+    addresses.each { |address| assert_kind_of StreetAddress, address }
+    assert_equal [ @first_address ].map(&:stringify_keys), addresses.map(&:attributes)
+  end
+
+  def test_load_collection_with_single_unknown_resource
+    Person.send(:remove_const, :Address) if Person.const_defined?(:Address)
+    assert !Person.const_defined?(:Address), "Address shouldn't exist until autocreated"
+    addresses = silence_warnings { @person.load(:addresses => [ @first_address ]).addresses }
+    assert Person.const_defined?(:Address), "Address should have been autocreated"
+    addresses.each { |address| assert_kind_of Person::Address, address }
+    assert_equal [ @first_address ].map(&:stringify_keys), addresses.map(&:attributes)
+  end
+
   def test_recursively_loaded_collections
     person = @person.load(@deep)
     assert_equal @deep[:id], person.id
@@ -71,7 +89,7 @@ class BaseLoadTest < Test::Unit::TestCase
     rivers = state.notable_rivers
     assert_kind_of Array, rivers
     assert_kind_of Person::Street::State::NotableRiver, rivers.first
-    assert_equal @deep[:street][:state][:notable_rivers][:notable_river].first[:id], rivers.first.id
+    assert_equal @deep[:street][:state][:notable_rivers].first[:id], rivers.first.id
     assert_equal @matz[:id], rivers.last.rafted_by.id
   end
 end

activeresource/test/connection_test.rb

@@ -6,10 +6,19 @@ class ConnectionTest < Test::Unit::TestCase
 
   def setup
     @conn = ActiveResource::Connection.new('http://localhost')
-    @matz  = { :id => 1, :name => 'Matz' }.to_xml(:root => 'person')
-    @david = { :id => 2, :name => 'David' }.to_xml(:root => 'person')
+    @matz  = { :id => 1, :name => 'Matz' }
+    @david = { :id => 2, :name => 'David' }
+    @people = [ @matz, @david ].to_xml(:root => 'people')
+    @people_single = [ @matz ].to_xml(:root => 'people-single-elements')
+    @people_empty = [ ].to_xml(:root => 'people-empty-elements')
+    @matz = @matz.to_xml(:root => 'person')
+    @david = @david.to_xml(:root => 'person')
+
     @default_request_headers = { 'Content-Type' => 'application/xml' }
     ActiveResource::HttpMock.respond_to do |mock|
+      mock.get    "/people.xml", {}, @people
+      mock.get    "/people_single_elements.xml", {}, @people_single
+      mock.get    "/people_empty_elements.xml", {}, @people_empty
       mock.get    "/people/1.xml", {}, @matz
       mock.put    "/people/1.xml", {}, nil, 204
       mock.delete "/people/1.xml", {}, nil, 200
@@ -67,7 +76,23 @@ class ConnectionTest < Test::Unit::TestCase
 
   def test_get
     matz = @conn.get("/people/1.xml")
-    assert_equal "Matz", matz["person"]["name"]
+    assert_equal "Matz", matz["name"]
+  end
+
+  def test_get_collection
+    people = @conn.get("/people.xml")
+    assert_equal "Matz", people[0]["name"]
+    assert_equal "David", people[1]["name"]
+  end
+  
+  def test_get_collection_single
+    people = @conn.get("/people_single_elements.xml")
+    assert_equal "Matz", people[0]["name"]
+  end
+  
+  def test_get_collection_empty
+    people = @conn.get("/people_empty_elements.xml")
+    assert_equal people, nil
   end
 
   def test_post