Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix ActiveResource nested resources not being persisted #3107

Closed
wants to merge 1 commit into from

7 participants

@odorcicd

Any nested resources with an ActiveResource object are not persisted so actions such as destroy won't work.
This patches ActiveResource::Base#load to set any nested resources as persisted.

I was debating simply passing in true as opposed to attrs.has_key?(resource.primary_key.to_s), thoughts?

I also changed the setup fixtures keys in load_test.rb to be strings, to avoid calling stringify_keys everywhere (should be working with string keys anyways through ActiveResource).

@esfourteen

+1, would be nice to use methods like new? and reload on nested resources

@trybeee

+1, #update doesn't work on nested resources, trying to #create instead.

@cjolly

+1 this also breaks the dom_id method which can affect dependent javascripts and css

@anikkar

+1 this also breaks pithy link_to helpers for named routes

@carlosantoniodasilva

Active Resource is not part of Rails core anymore, it was extracted to its own repo. Please make sure you test with the latest version - 3.2 in this case - and if the issue still persists, feel free to open a pull request there. I'm closing the issue here, thanks!

@dguerri

+1 could you please submit this to the active resource repo ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 22, 2011
  1. @odorcicd
This page is out of date. Refresh to see the latest.
View
4 activeresource/lib/active_resource/base.rb
@@ -1262,14 +1262,14 @@ def load(attributes, remove_root = false)
value.map do |attrs|
if attrs.is_a?(Hash)
resource ||= find_or_create_resource_for_collection(key)
- resource.new(attrs)
+ resource.new(attrs, attrs.has_key?(resource.primary_key.to_s))
else
attrs.duplicable? ? attrs.dup : attrs
end
end
when Hash
resource = find_or_create_resource_for(key)
- resource.new(value)
+ resource.new(value, value.has_key?(resource.primary_key.to_s))
else
value.duplicable? ? value.dup : value
end
View
3  activeresource/test/abstract_unit.rb
@@ -23,7 +23,7 @@ def setup_response
@greg = { :person => { :id => 3, :name => 'Greg' } }.to_json
@addy = { :address => { :id => 1, :street => '12345 Street', :country => 'Australia' } }.to_json
@rick = { :person => { :name => "Rick", :age => 25 } }.to_json
- @joe = { :person => { :id => 6, :name => 'Joe', :likes_hats => true }}.to_json
+ @joe = { :person => { :id => 6, :name => 'Joe', :likes_hats => true, :street_address => { :id => 1, :street => '12345 Street', :country => 'Australia', :person_id => 6 } }}.to_json
@people = { :people => [ { :person => { :id => 1, :name => 'Matz' } }, { :person => { :id => 2, :name => 'David' } }] }.to_json
@people_david = { :people => [ { :person => { :id => 2, :name => 'David' } }] }.to_json
@addresses = { :addresses => [{ :address => { :id => 1, :street => '12345 Street', :country => 'Australia' } }] }.to_json
@@ -119,6 +119,7 @@ def setup_response
mock.get "/people/Greg/addresses/1.json", {}, @addy
mock.put "/people/1/addresses/1.json", {}, nil, 204
mock.delete "/people/1/addresses/1.json", {}, nil, 200
+ mock.delete "/people/6/addresses/1.json", {}, nil, 200
mock.post "/people/1/addresses.json", {}, nil, 201, 'Location' => '/people/1/addresses/5'
mock.get "/people/1/addresses/99.json", {}, nil, 404
mock.get "/people//addresses.xml", {}, nil, 404
View
81 activeresource/test/cases/base/load_test.rb
@@ -34,31 +34,31 @@ class Note < ActiveResource::Base
class BaseLoadTest < Test::Unit::TestCase
def setup
- @matz = { :id => 1, :name => 'Matz' }
+ @matz = { 'id' => 1, 'name' => 'Matz' }
- @first_address = { :address => { :id => 1, :street => '12345 Street' } }
- @addresses = [@first_address, { :address => { :id => 2, :street => '67890 Street' } }]
- @addresses_from_json = { :street_addresses => @addresses }
- @addresses_from_json_single = { :street_addresses => [ @first_address ] }
+ @first_address = { 'address' => { 'id' => 1, 'street' => '12345 Street' } }
+ @addresses = [@first_address, { 'address' => { 'id' => 2, 'street' => '67890 Street' } }]
+ @addresses_from_json = { 'street_addresses' => @addresses }
+ @addresses_from_json_single = { 'street_addresses' => [ @first_address ] }
- @deep = { :id => 1, :street => {
- :id => 1, :state => { :id => 1, :name => 'Oregon',
- :notable_rivers => [
- { :id => 1, :name => 'Willamette' },
- { :id => 2, :name => 'Columbia', :rafted_by => @matz }],
- :postal_codes => [ 97018, 1234567890 ],
- :dates => [ Time.now ],
- :votes => [ true, false, true ],
- :places => [ "Columbia City", "Unknown" ]}}}
+ @deep = { 'id' => 1, 'street' => {
+ 'id' => 1, 'state' => { 'id' => 1, 'name' => 'Oregon',
+ 'notable_rivers' => [
+ { 'id' => 1, 'name' => 'Willamette' },
+ { 'id' => 2, 'name' => 'Columbia', 'rafted_by' => @matz }],
+ 'postal_codes' => [ 97018, 1234567890 ],
+ 'dates' => [ Time.now ],
+ 'votes' => [ true, false, true ],
+ 'places' => [ "Columbia City", "Unknown" ]}}}
# List of books formated as [{timestamp_of_publication => name}, ...]
- @books = {:books => [
+ @books = {'books' => [
{1009839600 => "Ruby in a Nutshell"},
{1199142000 => "The Ruby Programming Language"}
]}
- @books_date = {:books => [
+ @books_date = {'books' => [
{Time.at(1009839600) => "Ruby in a Nutshell"},
{Time.at(1199142000) => "The Ruby Programming Language"}
]}
@@ -80,39 +80,41 @@ def test_load_expects_hash
def test_load_simple_hash
assert_equal Hash.new, @person.attributes
- assert_equal @matz.stringify_keys, @person.load(@matz).attributes
+ assert_equal @matz, @person.load(@matz).attributes
end
def test_after_load_attributes_are_accessible
assert_equal Hash.new, @person.attributes
- assert_equal @matz.stringify_keys, @person.load(@matz).attributes
- assert_equal @matz[:name], @person.attributes['name']
+ assert_equal @matz, @person.load(@matz).attributes
+ assert_equal @matz['name'], @person.attributes['name']
end
def test_after_load_attributes_are_accessible_via_indifferent_access
assert_equal Hash.new, @person.attributes
- assert_equal @matz.stringify_keys, @person.load(@matz).attributes
- assert_equal @matz[:name], @person.attributes['name']
- assert_equal @matz[:name], @person.attributes[:name]
+ assert_equal @matz, @person.load(@matz).attributes
+ assert_equal @matz['name'], @person.attributes['name']
+ assert_equal @matz['name'], @person.attributes[:name]
end
def test_load_one_with_existing_resource
address = @person.load(:street_address => @first_address.values.first).street_address
assert_kind_of StreetAddress, address
- assert_equal @first_address.values.first.stringify_keys, address.attributes
+ assert_equal @first_address.values.first, address.attributes
+ assert address.persisted?
end
def test_load_one_with_unknown_resource
address = silence_warnings { @person.load(@first_address).address }
assert_kind_of Person::Address, address
- assert_equal @first_address.values.first.stringify_keys, address.attributes
+ assert_equal @first_address.values.first, address.attributes
+ assert address.persisted?
end
def test_load_collection_with_existing_resource
addresses = @person.load(@addresses_from_json).street_addresses
assert_kind_of Array, addresses
addresses.each { |address| assert_kind_of StreetAddress, address }
- assert_equal @addresses.map { |a| a[:address].stringify_keys }, addresses.map(&:attributes)
+ assert_equal @addresses.map { |a| a['address'] }, addresses.map(&:attributes)
end
def test_load_collection_with_unknown_resource
@@ -121,14 +123,14 @@ def test_load_collection_with_unknown_resource
addresses = silence_warnings { @person.load(:addresses => @addresses).addresses }
assert Person.const_defined?(:Address), "Address should have been autocreated"
addresses.each { |address| assert_kind_of Person::Address, address }
- assert_equal @addresses.map { |a| a[:address].stringify_keys }, addresses.map(&:attributes)
+ assert_equal @addresses.map { |a| a['address'] }, addresses.map(&:attributes)
end
def test_load_collection_with_single_existing_resource
addresses = @person.load(@addresses_from_json_single).street_addresses
assert_kind_of Array, addresses
addresses.each { |address| assert_kind_of StreetAddress, address }
- assert_equal [ @first_address.values.first ].map(&:stringify_keys), addresses.map(&:attributes)
+ assert_equal [ @first_address.values.first ], addresses.map(&:attributes)
end
def test_load_collection_with_single_unknown_resource
@@ -137,49 +139,52 @@ def test_load_collection_with_single_unknown_resource
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.values.first ].map(&:stringify_keys), addresses.map(&:attributes)
+ assert_equal [ @first_address.values.first ], addresses.map(&:attributes)
end
def test_recursively_loaded_collections
person = @person.load(@deep)
- assert_equal @deep[:id], person.id
+ assert_equal @deep['id'], person.id
street = person.street
assert_kind_of Person::Street, street
- assert_equal @deep[:street][:id], street.id
+ assert_equal @deep['street']['id'], street.id
+ assert street.persisted?
state = street.state
assert_kind_of Person::Street::State, state
- assert_equal @deep[:street][:state][:id], state.id
+ assert_equal @deep['street']['state']['id'], state.id
+ assert state.persisted?
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].first[:id], rivers.first.id
- assert_equal @matz[:id], rivers.last.rafted_by.id
+ assert_equal @deep['street']['state']['notable_rivers'].first['id'], rivers.first.id
+ assert_equal @matz['id'], rivers.last.rafted_by.id
+ assert rivers.all?(&:persisted?)
postal_codes = state.postal_codes
assert_kind_of Array, postal_codes
assert_equal 2, postal_codes.size
assert_kind_of Fixnum, postal_codes.first
- assert_equal @deep[:street][:state][:postal_codes].first, postal_codes.first
+ assert_equal @deep['street']['state']['postal_codes'].first, postal_codes.first
assert_kind_of Numeric, postal_codes.last
- assert_equal @deep[:street][:state][:postal_codes].last, postal_codes.last
+ assert_equal @deep['street']['state']['postal_codes'].last, postal_codes.last
places = state.places
assert_kind_of Array, places
assert_kind_of String, places.first
- assert_equal @deep[:street][:state][:places].first, places.first
+ assert_equal @deep['street']['state']['places'].first, places.first
dates = state.dates
assert_kind_of Array, dates
assert_kind_of Time, dates.first
- assert_equal @deep[:street][:state][:dates].first, dates.first
+ assert_equal @deep['street']['state']['dates'].first, dates.first
votes = state.votes
assert_kind_of Array, votes
assert_kind_of TrueClass, votes.first
- assert_equal @deep[:street][:state][:votes].first, votes.first
+ assert_equal @deep['street']['state']['votes'].first, votes.first
end
def test_nested_collections_within_the_same_namespace
View
8 activeresource/test/cases/base_test.rb
@@ -859,6 +859,14 @@ def test_destroy
assert_raise(ActiveResource::ResourceNotFound) { Person.find(1).destroy }
end
+ def test_destroy_nested_resource
+ assert Person.find(6).street_address.destroy
+ ActiveResource::HttpMock.respond_to do |mock|
+ mock.get "/people/6/addresses/1.json", {}, nil, 404
+ end
+ assert_raise(ActiveResource::ResourceNotFound) { StreetAddress.find(1, :params => { :person_id => 6 }) }
+ end
+
def test_destroy_with_custom_prefix
assert StreetAddress.find(1, :params => { :person_id => 1 }).destroy
ActiveResource::HttpMock.respond_to do |mock|
Something went wrong with that request. Please try again.