Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Fix issues with ActiveResource collection handling. Closes #6291. [bm…

…ilekic]

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@5714 5ecf4fe2-1ee6-0310-87b1-e25e094e27de
  • Loading branch information...
commit 55d4dbb8df9b4e6e46d461352f97e35ba69b417e 1 parent 2f5ee5a
@technoweenie technoweenie authored
View
2  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]
View
23 activeresource/lib/active_resource/base.rb
@@ -20,7 +20,7 @@ def connection(refresh = false)
@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 @@ def delete(id)
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 @@ def load(attributes)
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 @@ def find_or_create_resource_for(name)
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
View
22 activeresource/lib/active_resource/connection.rb
@@ -49,7 +49,7 @@ def site=(site)
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 @@ def authorization_header
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
View
4 activeresource/test/authorization_test.rb
@@ -10,7 +10,7 @@ def setup
@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 @@ def test_authorization_header_with_password_but_no_username
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
View
26 activeresource/test/base/load_test.rb
@@ -8,13 +8,14 @@ def setup
@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 @@ def test_load_collection_with_existing_resource
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 @@ def test_load_collection_with_unknown_resource
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 @@ def test_recursively_loaded_collections
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
View
31 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 @@ def test_site_accessor_accepts_uri_or_string_argument
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
Please sign in to comment.
Something went wrong with that request. Please try again.