Remove explicit prefix_options parameter for ActiveResource::Base#initialize. Allow ActiveResource::Base.delete with custom prefix. Add ActiveResource::Base#dup [Rick]

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@6568 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

activeresource/CHANGELOG

@@ -1,5 +1,12 @@
 *SVN*
 
+* Remove explicit prefix_options parameter for ActiveResource::Base#initialize. [Rick]
+  ActiveResource splits the prefix_options from it automatically.
+
+* Allow ActiveResource::Base.delete with custom prefix. [Rick]
+
+* Add ActiveResource::Base#dup [Rick]
+
 * Fixed constant warning when fetching the same object multiple times [DHH]
 
 * Added that saves which get a body response (and not just a 201) will use that response to update themselves [DHH]

activeresource/lib/active_resource/base.rb

@@ -41,10 +41,16 @@ module ActiveResource
       def prefix(options={})
         default = site.path
         default << '/' unless default[-1..-1] == '/'
+        # generate the actual method based on the current site path
         self.prefix = default
         prefix(options)
       end
 
+      def prefix_source
+        prefix # generate #prefix and #prefix_source methods first
+        prefix_source
+      end
+
       # Sets the resource prefix
       #  prefix/collectionname/1.xml
       def prefix=(value = '/')
@@ -67,12 +73,26 @@ module ActiveResource
       alias_method :set_element_name, :element_name=  #:nodoc:
       alias_method :set_collection_name, :collection_name=  #:nodoc:
 
-      def element_path(id, options = {})
+      # Gets the element path for the given ID.  If no query_options are given, they are split from the prefix options:
-        "#{prefix(options)}#{collection_name}/#{id}.xml#{query_string(options)}"
+      #
+      # Post.element_path(1) # => /posts/1.xml
+      # Comment.element_path(1, :post_id => 5) # => /posts/5/comments/1.xml
+      # Comment.element_path(1, :post_id => 5, :active => 1) # => /posts/5/comments/1.xml?active=1
+      # Comment.element_path(1, {:post_id => 5}, {:active => 1}) # => /posts/5/comments/1.xml?active=1
+      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}.xml#{query_string(query_options)}"
       end
 
-      def collection_path(options = {}) 
+      # Gets the collection path.  If no query_options are given, they are split from the prefix options:
-        "#{prefix(options)}#{collection_name}.xml#{query_string(options)}"
+      #
+      # Post.collection_path # => /posts.xml
+      # Comment.collection_path(:post_id => 5) # => /posts/5/comments.xml
+      # Comment.collection_path(:post_id => 5, :active => 1) # => /posts/5/comments.xml?active=1
+      # Comment.collection_path({:post_id => 5}, {:active => 1}) # => /posts/5/comments.xml?active=1
+      def collection_path(prefix_options = {}, query_options = nil)
+        prefix_options, query_options = split_options(prefix_options) if query_options.nil?
+        "#{prefix(prefix_options)}#{collection_name}.xml#{query_string(query_options)}"
       end
 
       alias_method :set_primary_key, :primary_key=  #:nodoc:
@@ -88,8 +108,8 @@ module ActiveResource
       # has not been saved then <tt>resource.valid?</tt> will return <tt>false</tt>,
       # while <tt>resource.new?</tt> will still return <tt>true</tt>.
       #      
-      def create(attributes = {}, prefix_options = {})
+      def create(attributes = {})
-        returning(self.new(attributes, prefix_options)) { |res| res.save }        
+        returning(self.new(attributes)) { |res| res.save }        
       end
 
       # Core method for finding resources.  Used similarly to ActiveRecord's find method.
@@ -106,8 +126,8 @@ module ActiveResource
         end
       end
 
-      def delete(id)
+      def delete(id, options = {})
-        connection.delete(element_path(id))
+        connection.delete(element_path(id, options))
       end
 
       # Evalutes to <tt>true</tt> if the resource is found.
@@ -120,14 +140,22 @@ module ActiveResource
       private
         # Find every resource.
         def find_every(options)
-          collection = connection.get(collection_path(options)) || []
+          prefix_options, query_options = split_options(options)
-          collection.collect! { |element| new(element) }
+          collection = connection.get(collection_path(prefix_options, query_options)) || []
+          collection.collect! do |element|
+            returning new(element.merge(prefix_options)) do |resource|
+              resource.prefix_options = prefix_options
+            end
+          end
         end
 
         # Find a single resource.
         #  { :person => person1 }
         def find_single(scope, options)
-          new(connection.get(element_path(scope, options)), options)
+          prefix_options, query_options = split_options(options)
+          returning new(connection.get(element_path(scope, prefix_options, query_options))) do |resource|
+            resource.prefix_options = prefix_options
+          end
         end
 
         # Accepts a URI and creates the site URI from that.
@@ -135,25 +163,34 @@ module ActiveResource
           site.is_a?(URI) ? site.dup : URI.parse(site)
         end
 
+        # contains a set of the current prefix parameters.
         def prefix_parameters
           @prefix_parameters ||= prefix_source.scan(/:\w+/).map { |key| key[1..-1].to_sym }.to_set
         end
 
         # Builds the query string for the request.
         def query_string(options)
-          # Omit parameters which appear in the URI path.
+          "?#{options.to_query}" unless options.empty? 
-          query_params = options.reject { |key, value| prefix_parameters.include?(key) }
+        end
-          "?#{query_params.to_query}" unless query_params.empty? 
+
+        # split an option hash into two hashes, one containing the prefix options, 
+        # and the other containing the leftovers.
+        def split_options(options = {})
+          prefix_options = {}; query_options = {}
+          options.each do |key, value|
+            (prefix_parameters.include?(key) ? prefix_options : query_options)[key] = value
+          end
+          [prefix_options, query_options]
         end
     end
 
     attr_accessor :attributes #:nodoc:
     attr_accessor :prefix_options #:nodoc:
 
-    def initialize(attributes = {}, prefix_options = {})
+    def initialize(attributes = {})
-      @attributes = {}
+      @attributes     = {}
+      @prefix_options = {}
       load(attributes)
-      @prefix_options = prefix_options
     end
 
     # Is the resource a new object?
@@ -186,6 +223,13 @@ module ActiveResource
     def hash
       id.hash
     end
+    
+    def dup
+      returning new do |resource|
+        resource.attributes     = @attributes
+        resource.prefix_options = @prefix_options
+      end
+    end
 
     # Delegates to +create+ if a new object, +update+ if its old. If the response to the save includes a body,
     # it will be assumed that this body is XML for the final object as it looked after the save (which would include
@@ -218,6 +262,7 @@ module ActiveResource
     # resources.
     def load(attributes)
       raise ArgumentError, "expected an attributes Hash, got #{attributes.inspect}" unless attributes.is_a?(Hash)
+      @prefix_options, attributes = split_options(attributes)
       attributes.each do |key, value|
         @attributes[key.to_s] =
           case value
@@ -227,8 +272,6 @@ module ActiveResource
             when Hash
               resource = find_or_create_resource_for(key)
               resource.new(value)
-            when ActiveResource::Base
-              value.class.new(value.attributes, value.prefix_options)
             else
               value.dup rescue value
           end
@@ -243,7 +286,7 @@ module ActiveResource
 
       # Update the resource on the remote service.
       def update
-        connection.put(element_path, to_xml)
+        connection.put(element_path(prefix_options), to_xml)
       end
 
       # Create (i.e., save to the remote service) the new resource.
@@ -287,6 +330,10 @@ module ActiveResource
         resource
       end
 
+      def split_options(options = {})
+        self.class.send(:split_options, options)
+      end
+
       def method_missing(method_symbol, *arguments) #:nodoc:
         method_name = method_symbol.to_s
 

activeresource/test/base_test.rb

@@ -112,6 +112,10 @@ class BaseTest < Test::Unit::TestCase
     assert_equal '/people/1/addresses/1.xml?type%5B%5D=work&type%5B%5D=play+time', StreetAddress.element_path(1, :person_id => 1, :type => ['work', 'play time'])
   end
 
+  def test_custom_element_path_with_prefix_and_parameters
+    assert_equal '/people/1/addresses/1.xml?type=work', StreetAddress.element_path(1, {:person_id => 1}, {:type => 'work'})
+  end
+
   def test_custom_collection_path
     assert_equal '/people/1/addresses.xml', StreetAddress.collection_path(:person_id => 1)
   end
@@ -120,6 +124,10 @@ class BaseTest < Test::Unit::TestCase
     assert_equal '/people/1/addresses.xml?type=work', StreetAddress.collection_path(:person_id => 1, :type => 'work')
   end
 
+  def test_custom_collection_path_with_prefix_and_parameters
+    assert_equal '/people/1/addresses.xml?type=work', StreetAddress.collection_path({:person_id => 1}, {:type => 'work'})
+  end
+
   def test_custom_element_name
     assert_equal 'address', StreetAddress.element_name
   end
@@ -207,7 +215,7 @@ class BaseTest < Test::Unit::TestCase
   end
 
   def test_create_with_custom_prefix
-    matzs_house = StreetAddress.new({}, { :person_id => 1 })
+    matzs_house = StreetAddress.new(:person_id => 1)
     matzs_house.save
     assert_equal '5', matzs_house.id
   end
@@ -272,11 +280,23 @@ class BaseTest < Test::Unit::TestCase
     ActiveResource::HttpMock.respond_to do |mock|
       mock.get "/people/1/addresses/1.xml", {}, nil, 404
     end
-    assert_raises(ActiveResource::ResourceNotFound) { StreetAddress.find(1, :person_id => 1).destroy }
+    assert_raises(ActiveResource::ResourceNotFound) { StreetAddress.find(1, :person_id => 1) }
   end
 
   def test_delete
     assert Person.delete(1)
+    ActiveResource::HttpMock.respond_to do |mock|
+      mock.get "/people/1.xml", {}, nil, 404
+    end
+    assert_raises(ActiveResource::ResourceNotFound) { Person.find(1) }
+  end
+  
+  def test_delete_with_custom_prefix
+    assert StreetAddress.delete(1, :person_id => 1)
+    ActiveResource::HttpMock.respond_to do |mock|
+      mock.get "/people/1/addresses/1.xml", {}, nil, 404
+    end
+    assert_raises(ActiveResource::ResourceNotFound) { StreetAddress.find(1, :person_id => 1) }
   end
 
   def test_exists
@@ -297,8 +317,8 @@ class BaseTest < Test::Unit::TestCase
 
     # Nested instance method.
     assert StreetAddress.find(1, :person_id => 1).exists?
-    assert !StreetAddress.new({:id => 1}, {:person_id => 2}).exists?
+    assert !StreetAddress.new({:id => 1, :person_id => 2}).exists?
-    assert !StreetAddress.new({:id => 2}, {:person_id => 1}).exists?
+    assert !StreetAddress.new({:id => 2, :person_id => 1}).exists?
   end
   
   def test_to_xml