diff options
| -rw-r--r-- | activeresource/CHANGELOG | 2 | ||||
| -rw-r--r-- | activeresource/lib/active_resource/base.rb | 23 | ||||
| -rw-r--r-- | activeresource/lib/active_resource/connection.rb | 22 | ||||
| -rw-r--r-- | activeresource/test/authorization_test.rb | 4 | ||||
| -rw-r--r-- | activeresource/test/base/load_test.rb | 26 | ||||
| -rw-r--r-- | activeresource/test/connection_test.rb | 31 |
6 files changed, 82 insertions, 26 deletions
diff --git a/activeresource/CHANGELOG b/activeresource/CHANGELOG index 517e9a6458..144264cd5a 100644 --- a/activeresource/CHANGELOG +++ b/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] diff --git a/activeresource/lib/active_resource/base.rb b/activeresource/lib/active_resource/base.rb index 07933d499b..5f3820b2be 100644 --- a/activeresource/lib/active_resource/base.rb +++ b/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 diff --git a/activeresource/lib/active_resource/connection.rb b/activeresource/lib/active_resource/connection.rb index e4f733cf70..c52d4d4839 100644 --- a/activeresource/lib/active_resource/connection.rb +++ b/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 diff --git a/activeresource/test/authorization_test.rb b/activeresource/test/authorization_test.rb index bfe51ce1e3..1a1a681a59 100644 --- a/activeresource/test/authorization_test.rb +++ b/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 diff --git a/activeresource/test/base/load_test.rb b/activeresource/test/base/load_test.rb index ca8d924693..6d2f65e1c5 100644 --- a/activeresource/test/base/load_test.rb +++ b/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 diff --git a/activeresource/test/connection_test.rb b/activeresource/test/connection_test.rb index 0a42e17417..99f0b8de89 100644 --- a/activeresource/test/connection_test.rb +++ b/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 |