Skip to content
Browse files

Fixed that to_param should be used and honored instead of hardcoding …

…the id (closes #11406) [gspiers]

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@9114 5ecf4fe2-1ee6-0310-87b1-e25e094e27de
  • Loading branch information...
1 parent 388e5d3 commit 9300ebd86fa39c6ea96cf39ef50ba5337dca6157 @dhh dhh committed Mar 28, 2008
Showing with 112 additions and 32 deletions.
  1. +2 −0 activeresource/CHANGELOG
  2. +4 −4 activeresource/lib/active_resource/base.rb
  3. +106 −28 activeresource/test/base_test.rb
View
2 activeresource/CHANGELOG
@@ -1,5 +1,7 @@
*SVN*
+* Fixed that to_param should be used and honored instead of hardcoding the id #11406 [gspiers]
+
* Improve documentation. [Radar, Jan De Poorter, chuyeow, xaviershay, danger, miloops, Xavier Noria, Sunny Ripert]
* Use HEAD instead of GET in exists? [bscofield]
View
8 activeresource/lib/active_resource/base.rb
@@ -350,7 +350,7 @@ def prefix(options={}) "#{prefix_call}" end
#
def element_path(id, prefix_options = {}, query_options = nil)
prefix_options, query_options = split_options(prefix_options) if query_options.nil?
- "#{prefix(prefix_options)}#{collection_name}/#{id}.#{format.extension}#{query_string(query_options)}"
+ "#{prefix(prefix_options)}#{collection_name}/#{id}.#{format.extension}#{query_string(query_options)}"
end
# Gets the collection path for the REST resources. If the +query_options+ parameter is omitted, Rails
@@ -760,7 +760,7 @@ def destroy
# that_guy.exists?
# # => false
def exists?
- !new? && self.class.exists?(id, :params => prefix_options)
+ !new? && self.class.exists?(to_param, :params => prefix_options)
end
# A method to convert the the resource to an XML string.
@@ -807,7 +807,7 @@ def to_xml(options={})
# my_branch.name
# # => Wilson Road
def reload
- self.load(self.class.find(id, :params => @prefix_options).attributes)
+ self.load(self.class.find(to_param, :params => @prefix_options).attributes)
end
# A method to manually load attributes from a hash. Recursively loads collections of
@@ -903,7 +903,7 @@ def id_from_response(response)
end
def element_path(options = nil)
- self.class.element_path(id, options || prefix_options)
+ self.class.element_path(to_param, options || prefix_options)
end
def collection_path(options = nil)
View
134 activeresource/test/base_test.rb
@@ -7,40 +7,45 @@ class BaseTest < Test::Unit::TestCase
def setup
@matz = { :id => 1, :name => 'Matz' }.to_xml(:root => 'person')
@david = { :id => 2, :name => 'David' }.to_xml(:root => 'person')
+ @greg = { :id => 3, :name => 'Greg' }.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
- mock.get "/people/2.xml", {}, @david
- mock.get "/people/3.xml", {'key' => 'value'}, nil, 404
- mock.put "/people/1.xml", {}, nil, 204
- 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", {}, @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
- mock.put "/people/1/addresses/1.xml", {}, nil, 204
- mock.delete "/people/1/addresses/1.xml", {}, nil, 200
- mock.post "/people/1/addresses.xml", {}, nil, 201, 'Location' => '/people/1/addresses/5'
- mock.get "/people//addresses.xml", {}, nil, 404
- mock.get "/people//addresses/1.xml", {}, nil, 404
- mock.put "/people//addresses/1.xml", {}, nil, 404
- mock.delete "/people//addresses/1.xml", {}, nil, 404
- mock.post "/people//addresses.xml", {}, nil, 404
- mock.head "/people/1.xml", {}, nil, 200
- mock.head "/people/99.xml", {}, nil, 404
- mock.head "/people/1/addresses/1.xml", {}, nil, 200
- mock.head "/people/1/addresses/2.xml", {}, nil, 404
- mock.head "/people/2/addresses/1.xml", {}, nil, 404
+ mock.get "/people/1.xml", {}, @matz
+ mock.get "/people/2.xml", {}, @david
+ mock.get "/people/Greg.xml", {}, @greg
+ mock.get "/people/4.xml", {'key' => 'value'}, nil, 404
+ mock.put "/people/1.xml", {}, nil, 204
+ 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", {}, @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
+ mock.get "/people/Greg/addresses/1.xml", {}, @addy
+ mock.put "/people/1/addresses/1.xml", {}, nil, 204
+ mock.delete "/people/1/addresses/1.xml", {}, nil, 200
+ mock.post "/people/1/addresses.xml", {}, nil, 201, 'Location' => '/people/1/addresses/5'
+ mock.get "/people//addresses.xml", {}, nil, 404
+ mock.get "/people//addresses/1.xml", {}, nil, 404
+ mock.put "/people//addresses/1.xml", {}, nil, 404
+ mock.delete "/people//addresses/1.xml", {}, nil, 404
+ mock.post "/people//addresses.xml", {}, nil, 404
+ mock.head "/people/1.xml", {}, nil, 200
+ mock.head "/people/Greg.xml", {}, nil, 200
+ mock.head "/people/99.xml", {}, nil, 404
+ mock.head "/people/1/addresses/1.xml", {}, nil, 200
+ mock.head "/people/1/addresses/2.xml", {}, nil, 404
+ mock.head "/people/2/addresses/1.xml", {}, nil, 404
+ mock.head "/people/Greg/addresses/1.xml", {}, nil, 200
end
Person.user = nil
@@ -302,6 +307,30 @@ def test_collection_path_with_parameters
def test_custom_element_path
assert_equal '/people/1/addresses/1.xml', StreetAddress.element_path(1, :person_id => 1)
assert_equal '/people/1/addresses/1.xml', StreetAddress.element_path(1, 'person_id' => 1)
+ assert_equal '/people/Greg/addresses/1.xml', StreetAddress.element_path(1, 'person_id' => 'Greg')
+ end
+
+ def test_custom_element_path_with_redefined_to_param
+ Person.module_eval do
+ alias_method :original_to_param_element_path, :to_param
+ def to_param
+ name
+ end
+ end
+
+ # Class method.
+ assert_equal '/people/Greg.xml', Person.element_path('Greg')
+
+ # Protected Instance method.
+ assert_equal '/people/Greg.xml', Person.find('Greg').send(:element_path)
+
+ ensure
+ # revert back to original
+ Person.module_eval do
+ # save the 'new' to_param so we don't get a warning about discarding the method
+ alias_method :element_path_to_param, :to_param
+ alias_method :to_param, :original_to_param_element_path
+ end
end
def test_custom_element_path_with_parameters
@@ -406,7 +435,7 @@ def test_find_first
def test_custom_header
Person.headers['key'] = 'value'
- assert_raises(ActiveResource::ResourceNotFound) { Person.find(3) }
+ assert_raises(ActiveResource::ResourceNotFound) { Person.find(4) }
ensure
Person.headers.delete('key')
end
@@ -487,6 +516,26 @@ def test_reload_works_with_prefix_options
assert_equal address, address.reload
end
+ def test_reload_with_redefined_to_param
+ Person.module_eval do
+ alias_method :original_to_param_reload, :to_param
+ def to_param
+ name
+ end
+ end
+
+ person = Person.find('Greg')
+ assert_equal person, person.reload
+
+ ensure
+ # revert back to original
+ Person.module_eval do
+ # save the 'new' to_param so we don't get a warning about discarding the method
+ alias_method :reload_to_param, :to_param
+ alias_method :to_param, :original_to_param_reload
+ end
+ end
+
def test_reload_works_without_prefix_options
person = Person.find(:first)
assert_equal person, person.reload
@@ -595,6 +644,35 @@ def test_exists
assert !StreetAddress.new({:id => 2, :person_id => 1}).exists?
end
+ def test_exists_with_redefined_to_param
+ Person.module_eval do
+ alias_method :original_to_param_exists, :to_param
+ def to_param
+ name
+ end
+ end
+
+ # Class method.
+ assert Person.exists?('Greg')
+
+ # Instance method.
+ assert Person.find('Greg').exists?
+
+ # Nested class method.
+ assert StreetAddress.exists?(1, :params => { :person_id => Person.find('Greg').to_param })
+
+ # Nested instance method.
+ assert StreetAddress.find(1, :params => { :person_id => Person.find('Greg').to_param }).exists?
+
+ ensure
+ # revert back to original
+ Person.module_eval do
+ # save the 'new' to_param so we don't get a warning about discarding the method
+ alias_method :exists_to_param, :to_param
+ alias_method :to_param, :original_to_param_exists
+ end
+ end
+
def test_to_xml
matz = Person.find(1)
xml = matz.to_xml

0 comments on commit 9300ebd

Please sign in to comment.
Something went wrong with that request. Please try again.