Merge remote branch 'gramos/ares-missing-prefix-value'

5 files changed, 41 insertions, 1 deletions

diff --git a/activeresource/lib/active_resource/base.rb b/activeresource/lib/active_resource/base.rb
index d31db9f0ba..6135ce4ec1 100644
--- a/activeresource/lib/active_resource/base.rb
+++ b/activeresource/lib/active_resource/base.rb
@@ -166,6 +166,7 @@ module ActiveResource
   #   # GET http://api.people.com:3000/people/999.xml
   #   ryan = Person.find(999) # 404, raises ActiveResource::ResourceNotFound
   #
+  #
   # <tt>404</tt> is just one of the HTTP error response codes that Active Resource will handle with its own exception. The
   # following HTTP response codes will also result in these exceptions:
   #
@@ -194,6 +195,16 @@ module ActiveResource
   #     redirect_to :action => 'new'
   #   end
   #
+  # When a GET is requested for a nested resource and you don't provide the prefix_param
+  # an ActiveResource::MissingPrefixParam will be raised.
+  #
+  #  class Comment < ActiveResource::Base
+  #   self.site = "http://someip.com/posts/:post_id/"
+  #  end
+  #
+  #  Comment.find(1)
+  #  # => ActiveResource::MissingPrefixParam: post_id prefix_option is missing
+  #
   # === Validation errors
   #
   # Active Resource supports validations on resources and will return errors if any of these validations fail
@@ -621,6 +632,8 @@ module ActiveResource
       #   # => /posts/5/comments/1.xml?active=1
       #
       def element_path(id, prefix_options = {}, query_options = nil)
+        check_prefix_options(prefix_options)
+
         prefix_options, query_options = split_options(prefix_options) if query_options.nil?
         "#{prefix(prefix_options)}#{collection_name}/#{URI.escape id.to_s}.#{format.extension}#{query_string(query_options)}"
       end
@@ -663,6 +676,7 @@ module ActiveResource
       #   # => /posts/5/comments.xml?active=1
       #
       def collection_path(prefix_options = {}, query_options = nil)
+        check_prefix_options(prefix_options)
         prefix_options, query_options = split_options(prefix_options) if query_options.nil?
         "#{prefix(prefix_options)}#{collection_name}.#{format.extension}#{query_string(query_options)}"
       end
@@ -842,6 +856,14 @@ module ActiveResource
       end
 
       private
+
+        def check_prefix_options(prefix_options)
+          p_options = HashWithIndifferentAccess.new(prefix_options)
+          prefix_parameters.each do |p|
+            raise(MissingPrefixParam, "#{p} prefix_option is missing") if p_options[p].blank?
+          end
+        end
+
         # Find every resource
         def find_every(options)
           begin
diff --git a/activeresource/lib/active_resource/exceptions.rb b/activeresource/lib/active_resource/exceptions.rb
index 0f4549fd73..6b953b28ad 100644
--- a/activeresource/lib/active_resource/exceptions.rb
+++ b/activeresource/lib/active_resource/exceptions.rb
@@ -36,6 +36,9 @@ module ActiveResource
     def to_s; response['Location'] ? "#{super} => #{response['Location']}" : super; end
   end
 
+  # Raised when ...
+  class MissingPrefixParam < ArgumentError; end # :nodoc:
+
   # 4xx Client Error
   class ClientError < ConnectionError; end # :nodoc:
 
diff --git a/activeresource/test/abstract_unit.rb b/activeresource/test/abstract_unit.rb
index 129efeb879..544aede002 100644
--- a/activeresource/test/abstract_unit.rb
+++ b/activeresource/test/abstract_unit.rb
@@ -97,6 +97,7 @@ def setup_response
     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/1/addresses/99.xml",   {}, nil, 404
     mock.get    "/people//addresses.xml",       {}, nil, 404
     mock.get    "/people//addresses/1.xml",     {}, nil, 404
     mock.put    "/people//addresses/1.xml",     {}, nil, 404
diff --git a/activeresource/test/cases/base_test.rb b/activeresource/test/cases/base_test.rb
index 6fabeeebcd..18c732b2ab 100644
--- a/activeresource/test/cases/base_test.rb
+++ b/activeresource/test/cases/base_test.rb
@@ -475,6 +475,12 @@ class BaseTest < Test::Unit::TestCase
     assert_equal '/people/ann%20mary/addresses/ann%20mary.xml', StreetAddress.element_path(:'ann mary', 'person_id' => 'ann mary')
   end
 
+  def test_custom_element_path_without_required_prefix_param
+    assert_raise ActiveResource::MissingPrefixParam do
+      StreetAddress.element_path(1)
+    end
+  end
+
   def test_module_element_path
     assert_equal '/sounds/1.xml', Asset::Sound.element_path(1)
   end
@@ -513,6 +519,12 @@ class BaseTest < Test::Unit::TestCase
     assert_equal '/people/1/addresses/1.xml?type=work', StreetAddress.element_path(1, {:person_id => 1}, {:type => 'work'})
   end
 
+  def test_custom_collection_path_without_required_prefix_param
+    assert_raise ActiveResource::MissingPrefixParam do
+      StreetAddress.collection_path
+    end
+  end
+
   def test_custom_collection_path
     assert_equal '/people/1/addresses.xml', StreetAddress.collection_path(:person_id => 1)
     assert_equal '/people/1/addresses.xml', StreetAddress.collection_path('person_id' => 1)
@@ -560,6 +572,8 @@ class BaseTest < Test::Unit::TestCase
       assert_equal Set.new([:the_param1]), person_class.prefix_parameters
       person_class.prefix = "the_prefix/:the_param2"
       assert_equal Set.new([:the_param2]), person_class.prefix_parameters
+      person_class.prefix = "the_prefix/:the_param1/other_prefix/:the_param2"
+      assert_equal Set.new([:the_param2, :the_param1]), person_class.prefix_parameters
     end
   end
 
diff --git a/activeresource/test/cases/finder_test.rb b/activeresource/test/cases/finder_test.rb
index fd09ef46d7..ebb783996d 100644
--- a/activeresource/test/cases/finder_test.rb
+++ b/activeresource/test/cases/finder_test.rb
@@ -84,7 +84,7 @@ class FinderTest < Test::Unit::TestCase
 
   def test_find_by_id_not_found
     assert_raise(ActiveResource::ResourceNotFound) { Person.find(99) }
-    assert_raise(ActiveResource::ResourceNotFound) { StreetAddress.find(1) }
+    assert_raise(ActiveResource::ResourceNotFound) { StreetAddress.find(99, :params => {:person_id => 1}) }
   end
 
   def test_find_all_sub_objects