Adding parameter and URL support to the REST terminus.

Previously, the server side correctly pulled parameters out of
the query strings, but the REST terminus never passed them on.  It does
now, at least for finding and searching.  It appears that at least
WEBrick doesn't support parameters for anything other than forms
and GET.

I've also added the ability for the REST terminus to pull
host/port information from the request key, if it's a URI.

Signed-off-by: Luke Kanies <luke@madstop.com>

7 files changed, 145 insertions, 20 deletions

diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb
index a2767d05b..4389dfb7e 100644
--- a/lib/puppet/indirector/rest.rb
+++ b/lib/puppet/indirector/rest.rb
@@ -32,31 +32,54 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
         {"Accept" => model.supported_formats.join(", ")}
     end
   
-    def network
-        Puppet::Network::HttpPool.http_instance(Puppet[:server], Puppet[:masterport].to_i)
+    def network(request)
+        if request.key =~ /^\w+:\/\// # it looks like a URI
+            begin
+                uri = URI.parse(URI.escape(request.key))
+            rescue => detail
+                raise ArgumentError, "Could not understand URL %s: %s" % [source, detail.to_s]
+            end
+            server = uri.host || Puppet[:server]
+            port = uri.port.to_i == 0 ? Puppet[:masterport].to_i : uri.port.to_i
+        else
+            server = Puppet[:server]
+            port = Puppet[:masterport].to_i
+        end
+
+        Puppet::Network::HttpPool.http_instance(server, port)
     end
 
     def find(request)
-        deserialize network.get("/#{indirection.name}/#{request.key}", headers)
+        deserialize network(request).get("/#{indirection.name}/#{request.key}#{query_string(request)}", headers)
     end
     
     def search(request)
         if request.key
-            path = "/#{indirection.name}s/#{request.key}"
+            path = "/#{indirection.name}s/#{request.key}#{query_string(request)}"
         else
-            path = "/#{indirection.name}s"
+            path = "/#{indirection.name}s#{query_string(request)}"
         end
-        unless result = deserialize(network.get(path, headers), true)
+        unless result = deserialize(network(request).get(path, headers), true)
             return []
         end
         return result
     end
     
     def destroy(request)
-        deserialize network.delete("/#{indirection.name}/#{request.key}", headers)
+        raise ArgumentError, "DELETE does not accept options" unless request.options.empty?
+        deserialize network(request).delete("/#{indirection.name}/#{request.key}", headers)
     end
     
     def save(request)
-        deserialize network.put("/#{indirection.name}/", request.instance.render, headers)
+        raise ArgumentError, "PUT does not accept options" unless request.options.empty?
+        deserialize network(request).put("/#{indirection.name}/", request.instance.render, headers)
+    end
+
+    private
+
+    # Create the qurey string, if options are present.
+    def query_string(request)
+        return "" unless request.options and ! request.options.empty?
+        "?" + request.options.collect { |key, value| "%s=%s" % [key, value] }.join("&")
     end
 end
diff --git a/lib/puppet/network/http/handler.rb b/lib/puppet/network/http/handler.rb
index 291481acd..6f5117b16 100644
--- a/lib/puppet/network/http/handler.rb
+++ b/lib/puppet/network/http/handler.rb
@@ -81,7 +81,11 @@ module Puppet::Network::HTTP::Handler
     # Execute our search.
     def do_search(request, response)
         args = params(request)
-        result = model.search(args)
+        if key = request_key(request)
+            result = model.search(key, args)
+        else
+            result = model.search(args)
+        end
         if result.nil? or (result.is_a?(Array) and result.empty?)
             return do_exception(response, "Could not find instances in %s with '%s'" % [model.name, args.inspect], 404)
         end
diff --git a/spec/integration/indirector/rest.rb b/spec/integration/indirector/rest.rb
index b307e3cab..95c2759b9 100755
--- a/spec/integration/indirector/rest.rb
+++ b/spec/integration/indirector/rest.rb
@@ -91,6 +91,11 @@ describe Puppet::Indirector::REST do
                 it 'should return an instance of the model class' do
                     Puppet::TestIndirectedFoo.find('bar').class.should == Puppet::TestIndirectedFoo
                 end
+
+                it "should pass options all the way through" do
+                    @mock_model.expects(:find).with { |key, args| args["one"] == "two" and args["three"] == "four" }.returns @model_instance
+                    Puppet::TestIndirectedFoo.find('bar', :one => "two", :three => "four")
+                end
     
                 it 'should return the instance of the model class associated with the provided lookup key' do
                     Puppet::TestIndirectedFoo.find('bar').value.should == @model_instance.value
@@ -151,6 +156,11 @@ describe Puppet::Indirector::REST do
                 it 'should return all matching results' do
                     Puppet::TestIndirectedFoo.search('bar').length.should == @model_instances.length
                 end
+
+                it "should pass options all the way through" do
+                    @mock_model.expects(:search).with { |key, args| args["one"] == "two" and args["three"] == "four" }.returns @model_instances
+                    Puppet::TestIndirectedFoo.search("foo", :one => "two", :three => "four")
+                end
     
                 it 'should return model instances' do
                     Puppet::TestIndirectedFoo.search('bar').each do |result| 
@@ -315,6 +325,11 @@ describe Puppet::Indirector::REST do
                 it 'should return an instance of the model class' do
                     Puppet::TestIndirectedFoo.find('bar').class.should == Puppet::TestIndirectedFoo
                 end
+
+                it "should pass options all the way through" do
+                    @mock_model.expects(:find).with { |key, args| args["one"] == "two" and args["three"] == "four" }.returns @model_instance
+                    Puppet::TestIndirectedFoo.find('bar', :one => "two", :three => "four")
+                end
     
                 it 'should return the instance of the model class associated with the provided lookup key' do
                     Puppet::TestIndirectedFoo.find('bar').value.should == @model_instance.value
@@ -372,6 +387,11 @@ describe Puppet::Indirector::REST do
                 it 'should return all matching results' do
                     Puppet::TestIndirectedFoo.search('bar').length.should == @model_instances.length
                 end
+
+                it "should pass options all the way through" do
+                    @mock_model.expects(:search).with { |key, args| args["one"] == "two" and args["three"] == "four" }.returns @model_instances
+                    Puppet::TestIndirectedFoo.search('bar', :one => "two", :three => "four")
+                end
     
                 it 'should return model instances' do
                     Puppet::TestIndirectedFoo.search('bar').each do |result| 
@@ -465,7 +485,7 @@ describe Puppet::Indirector::REST do
                 it "should not fail" do
                     lambda { @instance.save }.should_not raise_error
                 end
-    
+
                 it 'should return an instance of the model class' do
                     @instance.save.class.should == Puppet::TestIndirectedFoo
                 end
diff --git a/spec/unit/indirector/rest.rb b/spec/unit/indirector/rest.rb
index 28ceb693a..3bd63359d 100755
--- a/spec/unit/indirector/rest.rb
+++ b/spec/unit/indirector/rest.rb
@@ -91,11 +91,40 @@ describe Puppet::Indirector::REST do
             Puppet.settings.stubs(:value).returns("rest_testing")
         end
 
-        it "should use the :server setting as the host and the :masterport setting (as an Integer) as the port" do
-            Puppet.settings.expects(:value).with(:server).returns "myserver"
-            Puppet.settings.expects(:value).with(:masterport).returns "1234"
-            Puppet::Network::HttpPool.expects(:http_instance).with("myserver", 1234).returns "myconn"
-            @searcher.network.should == "myconn"
+        describe "and the request key is not a URL" do
+            it "should use the :server setting as the host and the :masterport setting (as an Integer) as the port" do
+                @request = stub 'request', :key => "foo"
+                Puppet.settings.expects(:value).with(:server).returns "myserver"
+                Puppet.settings.expects(:value).with(:masterport).returns "1234"
+                Puppet::Network::HttpPool.expects(:http_instance).with("myserver", 1234).returns "myconn"
+                @searcher.network(@request).should == "myconn"
+            end
+        end
+
+        describe "and the request key is a URL" do
+            it "should use the :server setting and masterport setting, as an Integer, as the host if no values are provided" do
+                @request = stub 'request', :key => "puppet:///key"
+
+                Puppet.settings.expects(:value).with(:server).returns "myserver"
+                Puppet.settings.expects(:value).with(:masterport).returns "1234"
+                Puppet::Network::HttpPool.expects(:http_instance).with("myserver", 1234).returns "myconn"
+                @searcher.network(@request).should == "myconn"
+            end
+
+            it "should use the server if one is provided" do
+                @request.stubs(:key).returns "puppet://host/key"
+
+                Puppet.settings.expects(:value).with(:masterport).returns "1234"
+                Puppet::Network::HttpPool.expects(:http_instance).with("host", 1234).returns "myconn"
+                @searcher.network(@request).should == "myconn"
+            end
+
+            it "should use the server and port if they are provided" do
+                @request.stubs(:key).returns "puppet://host:123/key"
+
+                Puppet::Network::HttpPool.expects(:http_instance).with("host", 123).returns "myconn"
+                @searcher.network(@request).should == "myconn"
+            end
         end
     end
 
@@ -104,7 +133,7 @@ describe Puppet::Indirector::REST do
             @connection = stub('mock http connection', :get => @response)
             @searcher.stubs(:network).returns(@connection)    # neuter the network connection
 
-            @request = stub 'request', :key => 'foo'
+            @request = stub 'request', :key => 'foo', :options => {}
         end
 
         it "should call the GET http method on a network connection" do
@@ -126,6 +155,13 @@ describe Puppet::Indirector::REST do
             @searcher.find(@request)
         end
 
+        it "should include all options in the query string" do
+            @request.stubs(:options).returns(:one => "two", :three => "four")
+            should_path = "/%s/%s" % [@indirection.name.to_s, "foo"]
+            @connection.expects(:get).with { |path, args| path =~ /\?one=two&three=four$/ }.returns(@response)
+            @searcher.find(@request)
+        end
+
         it "should provide an Accept header containing the list of supported formats joined with commas" do
             @connection.expects(:get).with { |path, args| args["Accept"] == "supported, formats" }.returns(@response)
 
@@ -151,7 +187,7 @@ describe Puppet::Indirector::REST do
 
             @model.stubs(:convert_from_multiple)
 
-            @request = stub 'request', :key => 'foo'
+            @request = stub 'request', :key => 'foo', :options => {}
         end
 
         it "should call the GET http method on a network connection" do
@@ -180,6 +216,14 @@ describe Puppet::Indirector::REST do
             @searcher.search(@request)
         end
 
+        it "should include all options in the query string" do
+            @request.stubs(:options).returns(:one => "two", :three => "four")
+
+            should_path = "/%s/%s" % [@indirection.name.to_s, "foo"]
+            @connection.expects(:get).with { |path, args| path =~ /\?one=two&three=four$/ }.returns(@response)
+            @searcher.search(@request)
+        end
+
         it "should provide an Accept header containing the list of supported formats joined with commas" do
             @connection.expects(:get).with { |path, args| args["Accept"] == "supported, formats" }.returns(@response)
 
@@ -204,7 +248,7 @@ describe Puppet::Indirector::REST do
             @connection = stub('mock http connection', :delete => @response)
             @searcher.stubs(:network).returns(@connection)    # neuter the network connection
 
-            @request = stub 'request', :key => 'foo'
+            @request = stub 'request', :key => 'foo', :options => {}
         end
 
         it "should call the DELETE http method on a network connection" do
@@ -213,6 +257,12 @@ describe Puppet::Indirector::REST do
             @searcher.destroy(@request)
         end
 
+        it "should fail if any options are provided, since DELETE apparently does not support query options" do
+            @request.stubs(:options).returns(:one => "two", :three => "four")
+
+            lambda { @searcher.destroy(@request) }.should raise_error(ArgumentError)
+        end
+
         it "should deserialize and return the http response" do
             @connection.expects(:delete).returns @response
             @searcher.expects(:deserialize).with(@response).returns "myobject"
@@ -250,7 +300,7 @@ describe Puppet::Indirector::REST do
             @searcher.stubs(:network).returns(@connection)    # neuter the network connection
 
             @instance = stub 'instance', :render => "mydata"
-            @request = stub 'request', :instance => @instance
+            @request = stub 'request', :instance => @instance, :options => {}
         end
 
         it "should call the PUT http method on a network connection" do
@@ -259,6 +309,12 @@ describe Puppet::Indirector::REST do
             @searcher.save(@request)
         end
 
+        it "should fail if any options are provided, since DELETE apparently does not support query options" do
+            @request.stubs(:options).returns(:one => "two", :three => "four")
+
+            lambda { @searcher.save(@request) }.should raise_error(ArgumentError)
+        end
+
         it "should use the indirection name as the path for the request" do
             @connection.expects(:put).with { |path, data, args| path == "/#{@indirection.name.to_s}/" }.returns @response
 
diff --git a/spec/unit/network/http/handler.rb b/spec/unit/network/http/handler.rb
index 816c0ea2e..6fc932091 100755
--- a/spec/unit/network/http/handler.rb
+++ b/spec/unit/network/http/handler.rb
@@ -302,12 +302,24 @@ describe Puppet::Network::HTTP::Handler do
 
             it "should use a common method for determining the request parameters" do
                 @handler.stubs(:params).returns(:foo => :baz, :bar => :xyzzy)
-                @model_class.expects(:search).with do |args|
+                @model_class.expects(:search).with do |key, args|
                     args[:foo] == :baz and args[:bar] == :xyzzy
                 end.returns @result
                 @handler.do_search(@request, @response)
             end
 
+            it "should use a request key if one is provided" do
+                @handler.expects(:request_key).with(@request).returns "foo"
+                @model_class.expects(:search).with { |key, args| key == "foo" }.returns @result
+                @handler.do_search(@request, @response)
+            end
+
+            it "should work with no request key if none is provided" do
+                @handler.expects(:request_key).with(@request).returns nil
+                @model_class.expects(:search).with { |args| args.is_a?(Hash) }.returns @result
+                @handler.do_search(@request, @response)
+            end
+
             it "should use the default status when a model search call succeeds" do
                 @model_class.stubs(:search).returns(@result)
                 @handler.do_search(@request, @response)
diff --git a/spec/unit/network/http/mongrel/rest.rb b/spec/unit/network/http/mongrel/rest.rb
index 0d4ce36bd..3b248dcfe 100755
--- a/spec/unit/network/http/mongrel/rest.rb
+++ b/spec/unit/network/http/mongrel/rest.rb
@@ -58,6 +58,11 @@ describe "Puppet::Network::HTTP::MongrelREST" do
                 @handler.request_key(@request).should == "bar"
             end
 
+            it "should return nil as the request key if no second field is present" do
+                @params.expects(:[]).with(Mongrel::Const::REQUEST_PATH).returns "/foo"
+                @handler.request_key(@request).should be_nil
+            end
+
             it "should return the request body as the body" do
                 @request.expects(:body).returns "mybody"
                 @handler.body(@request).should == "mybody"
diff --git a/spec/unit/network/http/webrick/rest.rb b/spec/unit/network/http/webrick/rest.rb
index 4c72ec545..620201472 100755
--- a/spec/unit/network/http/webrick/rest.rb
+++ b/spec/unit/network/http/webrick/rest.rb
@@ -52,6 +52,11 @@ describe Puppet::Network::HTTP::WEBrickREST do
                 @handler.request_key(@request).should == "bar"
             end
 
+            it "should return nil as the request key if there is no second field" do
+                @request.expects(:path).returns "/foo"
+                @handler.request_key(@request).should be_nil
+            end
+
             it "should return the request body as the body" do
                 @request.expects(:body).returns "my body"
                 @handler.body(@request).should == "my body"