BACKWARDS INCOMPATIBLE: Changed the finder API to be more extensible with :params and more strict usage of scopes [DHH] Added find-one with symbol [DHH]

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

4 files changed, 95 insertions, 44 deletions

diff --git a/activeresource/CHANGELOG b/activeresource/CHANGELOG
index 3589b89994..bb23e0d2ac 100644
--- a/activeresource/CHANGELOG
+++ b/activeresource/CHANGELOG
@@ -1,6 +1,14 @@
 *SVN*
 
-* Add support for setting custom headers per ActiveResource model [Rick]
+* Added find-one with symbol [DHH]. Example: Person.find(:one, :from => :leader) # => GET /people/leader.xml
+
+* BACKWARDS INCOMPATIBLE: Changed the finder API to be more extensible with :params and more strict usage of scopes [DHH]. Changes:
+
+    Person.find(:all, :title => "CEO")      ...becomes: Person.find(:all, :params => { :title => "CEO" })
+    Person.find(:managers)                  ...becomes: Person.find(:all, :from => :managers)
+    Person.find("/companies/1/manager.xml") ...becomes: Person.find(:one, :from => "/companies/1/manager.xml")
+
+* Add support for setting custom headers per Active Resource model [Rick]
 
   class Project
     headers['X-Token'] = 'foo'
diff --git a/activeresource/lib/active_resource/base.rb b/activeresource/lib/active_resource/base.rb
index 590993ff9d..1cbbfaccf4 100644
--- a/activeresource/lib/active_resource/base.rb
+++ b/activeresource/lib/active_resource/base.rb
@@ -117,13 +117,15 @@ module ActiveResource
       end
 
       # Core method for finding resources.  Used similarly to Active Record's find method.
-      #  Person.find(1)                                        # => GET /people/1.xml
-      #  Person.find(:all)                                     # => GET /people.xml
-      #  Person.find(:all, :title => "CEO")                    # => GET /people.xml?title=CEO
-      #  Person.find(:managers)                                # => GET /people/managers.xml
-      #  Person.find(:all, :from => "/companies/1/people.xml") # => GET /companies/1/people.xml
-      #  Person.find("/companies/1/manager.xml")               # => GET /companies/1/manager.xml
-      #  StreetAddress.find(1, :person_id => 1)                # => GET /people/1/street_addresses/1.xml
+      #
+      #   Person.find(1)                                         # => GET /people/1.xml
+      #   Person.find(:all)                                      # => GET /people.xml
+      #   Person.find(:all, :params => { :title => "CEO" })      # => GET /people.xml?title=CEO
+      #   Person.find(:all, :from => :managers)                  # => GET /people/managers.xml
+      #   Person.find(:all, :from => "/companies/1/people.xml")  # => GET /companies/1/people.xml
+      #   Person.find(:one, :from => :leader)                    # => GET /people/leader.xml
+      #   Person.find(:one, :from => "/companies/1/manager.xml") # => GET /companies/1/manager.xml
+      #   StreetAddress.find(1, :params => { :person_id => 1 })  # => GET /people/1/street_addresses/1.xml
       def find(*arguments)
         scope   = arguments.slice!(0)
         options = arguments.slice!(0) || {}
@@ -131,7 +133,7 @@ module ActiveResource
         case scope
           when :all   then find_every(options)
           when :first then find_every(options).first
-          when Symbol then instantiate_collection(get(scope, options))
+          when :one   then find_one(options)
           else             find_single(scope, options)
         end
       end
@@ -148,34 +150,50 @@ module ActiveResource
       end
 
       private
-        # Find every resource.
+        # Find every resource
         def find_every(options)
-          from = options.delete(:from)
-          prefix_options, query_options = split_options(options)
-          from ||= collection_path(prefix_options, query_options)
-
-          instantiate_collection(connection.get(from, headers) || [])
+          case from = options.delete(:from)
+          when Symbol
+            instantiate_collection(get(from, options[:params]))
+          when String
+            path = "#{from}#{query_string(options[:params])}"
+            instantiate_collection(connection.get(path, headers) || [])
+          else
+            prefix_options, query_options = split_options(options[:params])
+            path = collection_path(prefix_options, query_options)
+            instantiate_collection(connection.get(path, headers) || [])
+          end
         end
         
-        def instantiate_collection(collection, prefix_options = {})
-          collection.collect! do |element|
-            returning new(element) do |resource|
-              resource.prefix_options = prefix_options
-            end
+        # Find a single resource from a one-off URL
+        def find_one(options)
+          case from = options[:from]
+          when Symbol
+            instantiate_record(get(from, options[:params]))
+          when String
+            path = "#{from}#{query_string(options[:params])}"
+            instantiate_record(connection.get(path, headers))
           end
         end
 
-        # Find a single resource.
-        #  { :person => person1 }
+        # Find a single resource from the default URL
         def find_single(scope, options)
-          prefix_options, query_options = split_options(options)
-          from = scope.to_s.include?("/") ? scope : element_path(scope, prefix_options, query_options)
+          prefix_options, query_options = split_options(options[:params])
+          path = element_path(scope, prefix_options, query_options)
+          instantiate_record(connection.get(path, headers), prefix_options)
+        end
+        
+        def instantiate_collection(collection, prefix_options = {})
+          collection.collect! { |record| instantiate_record(record, prefix_options) }
+        end
 
-          returning new(connection.get(from, headers)) do |resource|
+        def instantiate_record(record, prefix_options = {})
+          returning new(record) do |resource|
             resource.prefix_options = prefix_options
           end
         end
 
+
         # Accepts a URI and creates the site URI from that.
         def create_site_uri_from(site)
           site.is_a?(URI) ? site.dup : URI.parse(site)
@@ -188,17 +206,19 @@ module ActiveResource
 
         # Builds the query string for the request.
         def query_string(options)
-          "?#{options.to_query}" unless options.empty? 
+          "?#{options.to_query}" unless options.nil? || options.empty? 
         end
 
         # 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_options, query_options = {}, {}
+
+          (options || {}).each do |key, value|
             (prefix_parameters.include?(key) ? prefix_options : query_options)[key] = value
           end
-          [prefix_options, query_options]
+
+          [ prefix_options, query_options ]
         end
     end
 
@@ -263,7 +283,7 @@ module ActiveResource
 
     # Evaluates to <tt>true</tt> if this resource is found.
     def exists?
-      !new? && self.class.exists?(id, prefix_options)
+      !new? && self.class.exists?(id, :params => prefix_options)
     end
 
     # Convert the resource to an XML string
diff --git a/activeresource/test/base/custom_methods_test.rb b/activeresource/test/base/custom_methods_test.rb
index 3648e0484d..f591c14260 100644
--- a/activeresource/test/base/custom_methods_test.rb
+++ b/activeresource/test/base/custom_methods_test.rb
@@ -63,10 +63,10 @@ class CustomMethodsTest < Test::Unit::TestCase
     assert_equal ActiveResource::Response.new("", 200, {}), Person.find(1).delete(:deactivate)
     
     # With nested resources
-    assert_equal StreetAddress.find(1, :person_id => 1).get(:deep),
+    assert_equal StreetAddress.find(1, :params => { :person_id => 1 }).get(:deep),
                   { "id" => 1, "street" => '12345 Street', "zip" => "27519" }
     assert_equal ActiveResource::Response.new("", 204, {}),
-                   StreetAddress.find(1, :person_id => 1).put(:normalize_phone, :locale => 'US')
+                   StreetAddress.find(1, :params => { :person_id => 1 }).put(:normalize_phone, :locale => 'US')
   end
   
   def test_custom_new_element_method
@@ -82,6 +82,6 @@ class CustomMethodsTest < Test::Unit::TestCase
   end
   
   def test_find_custom_resources
-    assert_equal 'Matz', Person.find(:managers).first.name
+    assert_equal 'Matz', Person.find(:all, :from => :managers).first.name
   end
 end
\ No newline at end of file
diff --git a/activeresource/test/base_test.rb b/activeresource/test/base_test.rb
index f8a9cd4f73..6a6ee5c64b 100644
--- a/activeresource/test/base_test.rb
+++ b/activeresource/test/base_test.rb
@@ -180,7 +180,7 @@ class BaseTest < Test::Unit::TestCase
   end
 
   def test_find_by_id_with_custom_prefix
-    addy = StreetAddress.find(1, :person_id => 1)
+    addy = StreetAddress.find(1, :params => { :person_id => 1 })
     assert_kind_of StreetAddress, addy
     assert_equal '12345 Street', addy.street
   end
@@ -219,10 +219,33 @@ class BaseTest < Test::Unit::TestCase
     assert_equal "David", people.first.name
   end
 
+  def test_find_all_by_from_with_options
+    ActiveResource::HttpMock.respond_to { |m| m.get "/companies/1/people.xml", {}, "<people>#{@david}</people>" }
+  
+    people = Person.find(:all, :from => "/companies/1/people.xml")
+    assert_equal 1, people.size
+    assert_equal "David", people.first.name
+  end
+
+  def test_find_all_by_symbol_from
+    ActiveResource::HttpMock.respond_to { |m| m.get "/people/managers.xml", {}, "<people>#{@david}</people>" }
+  
+    people = Person.find(:all, :from => :managers)
+    assert_equal 1, people.size
+    assert_equal "David", people.first.name
+  end
+
   def test_find_single_by_from
     ActiveResource::HttpMock.respond_to { |m| m.get "/companies/1/manager.xml", {}, @david }
 
-    david = Person.find("/companies/1/manager.xml")
+    david = Person.find(:one, :from => "/companies/1/manager.xml")
+    assert_equal "David", david.name
+  end
+
+  def test_find_single_by_symbol_from
+    ActiveResource::HttpMock.respond_to { |m| m.get "/people/leader.xml", {}, @david }
+
+    david = Person.find(:one, :from => :leader)
     assert_equal "David", david.name
   end
 
@@ -249,7 +272,7 @@ class BaseTest < Test::Unit::TestCase
 
   # Test that loading a resource preserves its prefix_options.
   def test_load_preserves_prefix_options
-    address = StreetAddress.find(1, :person_id => 1)
+    address = StreetAddress.find(1, :params => { :person_id => 1 })
     ryan = Person.new(:id => 1, :name => 'Ryan', :address => address)
     assert_equal address.prefix_options, ryan.address.prefix_options
   end
@@ -279,7 +302,7 @@ class BaseTest < Test::Unit::TestCase
   end
 
   def test_update_with_custom_prefix
-    addy = StreetAddress.find(1, :person_id => 1)
+    addy = StreetAddress.find(1, :params => { :person_id => 1 })
     addy.street = "54321 Street"
     assert_kind_of StreetAddress, addy
     assert_equal "54321 Street", addy.street
@@ -303,11 +326,11 @@ class BaseTest < Test::Unit::TestCase
   end
 
   def test_destroy_with_custom_prefix
-    assert StreetAddress.find(1, :person_id => 1).destroy
+    assert StreetAddress.find(1, :params => { :person_id => 1 }).destroy
     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) }
+    assert_raises(ActiveResource::ResourceNotFound) { StreetAddress.find(1, :params => { :person_id => 1 }) }
   end
 
   def test_delete
@@ -323,7 +346,7 @@ 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) }
+    assert_raises(ActiveResource::ResourceNotFound) { StreetAddress.find(1, :params => { :person_id => 1 }) }
   end
 
   def test_exists
@@ -338,12 +361,12 @@ class BaseTest < Test::Unit::TestCase
     assert !Person.new(:id => 99).exists?
 
     # Nested class method.
-    assert StreetAddress.exists?(1, :person_id => 1)
-    assert !StreetAddress.exists?(1, :person_id => 2)
-    assert !StreetAddress.exists?(2, :person_id => 1)
+    assert StreetAddress.exists?(1,  :params => { :person_id => 1 })
+    assert !StreetAddress.exists?(1, :params => { :person_id => 2 })
+    assert !StreetAddress.exists?(2, :params => { :person_id => 1 })
 
     # Nested instance method.
-    assert StreetAddress.find(1, :person_id => 1).exists?
+    assert StreetAddress.find(1, :params => { :person_id => 1 }).exists?
     assert !StreetAddress.new({:id => 1, :person_id => 2}).exists?
     assert !StreetAddress.new({:id => 2, :person_id => 1}).exists?
   end