Searching again works over REST, including full content-type translation.

The format management is a bit clunky right now, though, so I
need to fix how they're managed.  Some of these tests fail,
but 99% of the remaining work is in other classes so I wanted
this as a discrete commit.

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

5 files changed, 74 insertions, 49 deletions

diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb
index 20c837fc9..c33b8a949 100644
--- a/lib/puppet/indirector/rest.rb
+++ b/lib/puppet/indirector/rest.rb
@@ -5,7 +5,7 @@ require 'uri'
 class Puppet::Indirector::REST < Puppet::Indirector::Terminus
     # Figure out the content type, turn that into a format, and use the format
     # to extract the body of the response.
-    def deserialize(response)
+    def deserialize(response, multiple = false)
         case response.code
         when "404"
             return nil
@@ -15,7 +15,11 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
             end
 
             # Convert the response to a deserialized object.
-            model.convert_from(response['content-type'], response.body)
+            if multiple
+                model.convert_from_multiple(response['content-type'], response.body)
+            else
+                model.convert_from(response['content-type'], response.body)
+            end
         else
             # Raise the http error if we didn't get a 'success' of some kind.
             message = "Server returned %s: %s" % [response.code, response.message]
@@ -46,7 +50,7 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
         else
             path = "/#{indirection.name}s"
         end
-        deserialize network.get(path, headers)
+        deserialize(network.get(path, headers), true)
     end
     
     def destroy(request)
diff --git a/lib/puppet/network/http/handler.rb b/lib/puppet/network/http/handler.rb
index 1a21bfea9..0069933bd 100644
--- a/lib/puppet/network/http/handler.rb
+++ b/lib/puppet/network/http/handler.rb
@@ -81,13 +81,13 @@ module Puppet::Network::HTTP::Handler
     # Execute our search.
     def do_search(request, response)
         args = params(request)
-        result = model.search(args).collect {|result| result.to_yaml }.to_yaml
+        result = model.search(args)
 
         # LAK:FAIL This doesn't work.
         format = format_to_use(request)
         set_content_type(response, format)
 
-        set_response(response, result) 
+        set_response(response, model.render_multiple(format, result))
     end
 
     # Execute our destroy.
diff --git a/spec/integration/indirector/rest.rb b/spec/integration/indirector/rest.rb
index 6b0fa972b..86d4d4291 100755
--- a/spec/integration/indirector/rest.rb
+++ b/spec/integration/indirector/rest.rb
@@ -69,15 +69,19 @@ describe Puppet::Indirector::REST do
             @params = { :address => "127.0.0.1", :port => 34343, :handlers => [ :test_indirected_foo ], :xmlrpc_handlers => [ :status ] }
             @server = Puppet::Network::Server.new(@params)
             @server.listen
+
+            # LAK:NOTE We need to have a fake model here so that our indirected methods get
+            # passed through REST; otherwise we'd be stubbing 'find', which would cause an immediate
+            # return.
+            @mock_model = stub('faked model', :name => "foo")
+            Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:model).returns(@mock_model)                
         end
     
         describe "when finding a model instance over REST" do
             describe "when a matching model instance can be found" do
                 before :each do
                     @model_instance = Puppet::TestIndirectedFoo.new(23)
-                    @mock_model = stub('faked model', :name => "foo", :find => @model_instance, :supported_formats => %w{one two})
                     @mock_model.stubs(:find).returns @model_instance
-                    Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:model).returns(@mock_model)                
                 end
             
                 it "should not fail" do
@@ -95,6 +99,13 @@ describe Puppet::Indirector::REST do
                 it 'should set an expiration on model instance' do
                     Puppet::TestIndirectedFoo.find('bar').expiration.should_not be_nil
                 end
+
+                it "should use a supported format" do
+                    Puppet::TestIndirectedFoo.expects(:supported_formats).returns ["marshal"]
+                    text = Marshal.dump(@model_instance)
+                    @model_instance.expects(:render).with("marshal").returns text
+                    Puppet::TestIndirectedFoo.find('bar')
+                end
             end
         
             describe "when no matching model instance can be found" do
@@ -125,8 +136,9 @@ describe Puppet::Indirector::REST do
             describe "when matching model instances can be found" do
                 before :each do
                     @model_instances = [ Puppet::TestIndirectedFoo.new(23), Puppet::TestIndirectedFoo.new(24) ]
-                    @mock_model = stub('faked model', :name => "foo", :search => @model_instances)
-                    Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:model).returns(@mock_model)                
+                    @mock_model.stubs(:search).returns @model_instances
+
+                    @mock_model.stubs(:render_multiple).returns @model_instances.to_yaml
                 end
             
                 it "should not fail" do
@@ -175,8 +187,7 @@ describe Puppet::Indirector::REST do
         describe "when destroying a model instance over REST" do
             describe "when a matching model instance can be found" do
                 before :each do
-                    @mock_model = stub('faked model', :name => "foo", :destroy => true)
-                    Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:model).returns(@mock_model)                
+                    @mock_model.stubs(:destroy).returns true
                 end
             
                 it "should not fail" do
@@ -190,8 +201,7 @@ describe Puppet::Indirector::REST do
         
             describe "when no matching model instance can be found" do
                 before :each do
-                    @mock_model = stub('faked model', :name => "foo", :destroy => false)
-                    Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:model).returns(@mock_model)
+                    @mock_model.stubs(:destroy).returns false
                 end
             
                 it "should return failure" do
@@ -201,9 +211,7 @@ describe Puppet::Indirector::REST do
         
             describe "when an exception is encountered in destroying a model instance" do
                 before :each do
-                    @mock_model = stub('faked model', :name => "foo")
                     @mock_model.stubs(:destroy).raises(RuntimeError)
-                    Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:model).returns(@mock_model)                
                 end
             
                 it "should raise an exception" do
@@ -215,8 +223,8 @@ describe Puppet::Indirector::REST do
         describe "when saving a model instance over REST" do
             before :each do
                 @instance = Puppet::TestIndirectedFoo.new(42)
-                @mock_model = stub('faked model', :name => "foo", :convert_from => @instance)
-                Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:model).returns(@mock_model)                
+                @mock_model.stubs(:save_object).returns @instance
+                @mock_model.stubs(:convert_from).returns @instance
                 Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:save_object).returns(@instance)                
             end
             
@@ -278,6 +286,12 @@ describe Puppet::Indirector::REST do
 
             @server = Puppet::Network::Server.new(@params)
             @server.listen
+
+            # LAK:NOTE We need to have a fake model here so that our indirected methods get
+            # passed through REST; otherwise we'd be stubbing 'find', which would cause an immediate
+            # return.
+            @mock_model = stub('faked model', :name => "foo")
+            Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)                
         end
 
         after do
@@ -288,8 +302,7 @@ describe Puppet::Indirector::REST do
             describe "when a matching model instance can be found" do
                 before :each do
                     @model_instance = Puppet::TestIndirectedFoo.new(23)
-                    @mock_model = stub('faked model', :name => "foo", :find => @model_instance)
-                    Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)                
+                    @mock_model.stubs(:find).returns @model_instance
                 end
             
                 it "should not fail" do
@@ -307,12 +320,18 @@ describe Puppet::Indirector::REST do
                 it 'should set an expiration on model instance' do
                     Puppet::TestIndirectedFoo.find('bar').expiration.should_not be_nil
                 end
+
+                it "should use a supported format" do
+                    Puppet::TestIndirectedFoo.expects(:supported_formats).returns ["marshal"]
+                    text = Marshal.dump(@model_instance)
+                    @model_instance.expects(:render).with("marshal").returns text
+                    Puppet::TestIndirectedFoo.find('bar')
+                end
             end
         
             describe "when no matching model instance can be found" do
                 before :each do
-                    @mock_model = stub('faked model', :name => "foo", :find => nil)
-                    Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)
+                    @mock_model.stubs(:find).returns nil
                 end
             
                 it "should return nil" do
@@ -322,9 +341,7 @@ describe Puppet::Indirector::REST do
         
             describe "when an exception is encountered in looking up a model instance" do
                 before :each do
-                    @mock_model = stub('faked model', :name => "foo")
                     @mock_model.stubs(:find).raises(RuntimeError)
-                    Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)                
                 end
             
                 it "should raise an exception" do
@@ -337,8 +354,8 @@ describe Puppet::Indirector::REST do
             describe "when matching model instances can be found" do
                 before :each do
                     @model_instances = [ Puppet::TestIndirectedFoo.new(23), Puppet::TestIndirectedFoo.new(24) ]
-                    @mock_model = stub('faked model', :name => "foo", :search => @model_instances)
-                    Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)                
+                    @mock_model.stubs(:search).returns @model_instances
+                    @mock_model.stubs(:render_multiple).returns @model_instances.to_yaml
                 end
             
                 it "should not fail" do
@@ -368,20 +385,18 @@ describe Puppet::Indirector::REST do
         
             describe "when no matching model instance can be found" do
                 before :each do
-                    @mock_model = stub('faked model', :name => "foo", :find => nil)
-                    Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)
+                    @mock_model.stubs(:search).returns nil
+                    @mock_model.stubs(:render_multiple).returns nil.to_yaml
                 end
             
                 it "should return nil" do
-                    Puppet::TestIndirectedFoo.find('bar').should be_nil
+                    Puppet::TestIndirectedFoo.search('bar').should be_nil
                 end
             end
         
             describe "when an exception is encountered in looking up a model instance" do
                 before :each do
-                    @mock_model = stub('faked model', :name => "foo")
                     @mock_model.stubs(:find).raises(RuntimeError)
-                    Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)                
                 end
             
                 it "should raise an exception" do
@@ -393,8 +408,7 @@ describe Puppet::Indirector::REST do
         describe "when destroying a model instance over REST" do
             describe "when a matching model instance can be found" do
                 before :each do
-                    @mock_model = stub('faked model', :name => "foo", :destroy => true)
-                    Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)                
+                    @mock_model.stubs(:destroy).returns true
                 end
             
                 it "should not fail" do
@@ -408,8 +422,7 @@ describe Puppet::Indirector::REST do
         
             describe "when no matching model instance can be found" do
                 before :each do
-                    @mock_model = stub('faked model', :name => "foo", :destroy => false)
-                    Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)
+                    @mock_model.stubs(:destroy).returns false
                 end
             
                 it "should return failure" do
@@ -419,9 +432,7 @@ describe Puppet::Indirector::REST do
         
             describe "when an exception is encountered in destroying a model instance" do
                 before :each do
-                    @mock_model = stub('faked model', :name => "foo")
                     @mock_model.stubs(:destroy).raises(RuntimeError)
-                    Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)                
                 end
             
                 it "should raise an exception" do
@@ -433,8 +444,7 @@ describe Puppet::Indirector::REST do
         describe "when saving a model instance over REST" do
             before :each do
                 @instance = Puppet::TestIndirectedFoo.new(42)
-                @mock_model = stub('faked model', :name => "foo", :convert_from => @instance)
-                Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)                
+                @mock_model.stubs(:convert_from).returns @instance
 
                 # LAK:NOTE This stub is necessary to prevent the REST call from calling
                 # REST.save again, thus producing painful infinite recursion.
diff --git a/spec/unit/indirector/rest.rb b/spec/unit/indirector/rest.rb
index 8d88ca7bb..801e8888e 100755
--- a/spec/unit/indirector/rest.rb
+++ b/spec/unit/indirector/rest.rb
@@ -71,7 +71,18 @@ describe Puppet::Indirector::REST do
             response.stubs(:body).returns "mydata"
             response.stubs(:code).returns "200"
 
-            @searcher.deserialize(response)
+            @searcher.deserialize(response).should == "myobject"
+        end
+
+        it "should convert and return multiple instances if the return code is in the 200s and 'multiple' is specified" do
+            @model.expects(:convert_from_multiple).with("myformat", "mydata").returns "myobjects"
+
+            response = mock 'response'
+            response.stubs(:[]).with("content-type").returns "myformat"
+            response.stubs(:body).returns "mydata"
+            response.stubs(:code).returns "200"
+
+            @searcher.deserialize(response, true).should == "myobjects"
         end
     end
 
@@ -138,6 +149,8 @@ describe Puppet::Indirector::REST do
             @connection = stub('mock http connection', :get => @response)
             @searcher.stubs(:network).returns(@connection)    # neuter the network connection
 
+            @model.stubs(:convert_from_multiple)
+
             @request = stub 'request', :key => 'foo'
         end
 
@@ -147,9 +160,9 @@ describe Puppet::Indirector::REST do
             @searcher.search(@request)
         end
 
-        it "should deserialize and return the http response" do
+        it "should deserialize as multiple instances and return the http response" do
             @connection.expects(:get).returns @response
-            @searcher.expects(:deserialize).with(@response).returns "myobject"
+            @searcher.expects(:deserialize).with(@response, true).returns "myobject"
 
             @searcher.search(@request).should == 'myobject'
         end        
@@ -174,11 +187,6 @@ describe Puppet::Indirector::REST do
             @searcher.search(@request)
         end
 
-        it "should deserialize and return the network response" do
-            @searcher.expects(:deserialize).with(@response).returns @instance
-            @searcher.search(@request).should equal(@instance)
-        end
-
         it "should generate an error when result data deserializes fails" do
             @searcher.expects(:deserialize).raises(ArgumentError)
             lambda { @searcher.search(@request) }.should raise_error(ArgumentError)
diff --git a/spec/unit/network/http/handler.rb b/spec/unit/network/http/handler.rb
index 0f60b9b10..1ed816d97 100755
--- a/spec/unit/network/http/handler.rb
+++ b/spec/unit/network/http/handler.rb
@@ -296,6 +296,7 @@ describe Puppet::Network::HTTP::Handler do
                 @result2 = mock 'results'
 
                 @result = [@result1, @result2]
+                @model_class.stubs(:render_multiple).returns "my rendered instances"
                 @model_class.stubs(:search).returns(@result)
             end
 
@@ -320,11 +321,13 @@ describe Puppet::Network::HTTP::Handler do
             end
 
             it "should return a list of serialized objects when a model search call succeeds" do
-                pending "I have not figured out how to do this yet"
-                @result1.expects(:render).returns "result1"
-                @result2.expects(:render).returns "result2"
+                @handler.expects(:accept_header).with(@request).returns "one,two"
 
                 @model_class.stubs(:search).returns(@result)
+
+                @model_class.expects(:render_multiple).with("one", @result).returns "my rendered instances"
+
+                @handler.expects(:set_response).with { |response, data| data == "my rendered instances" }
                 @handler.do_search(@request, @response)
             end
         end