All error and format handling works over REST except searching.

Searching operates on multiple instances, and I have not
yet figured out how we should handle converting multiple
instances to a given format -- we can't use the instance
method (e.g., to_yaml), because it would be on Array
instead of the class we're operating on.  That would work
for yaml, but not, for instance, for xml.

9 files changed, 122 insertions, 64 deletions

diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb
index 8889dbc3e..20c837fc9 100644
--- a/lib/puppet/indirector/rest.rb
+++ b/lib/puppet/indirector/rest.rb
@@ -6,11 +6,21 @@ 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)
-        # Raise the http error if we didn't get a 'success' of some kind.
-        response.error! unless response.code =~ /^2/
+        case response.code
+        when "404"
+            return nil
+        when /^2/
+            unless response['content-type']
+                raise "No content type in http response; cannot parse"
+            end
 
-        # Convert the response to a deserialized object.
-        model.convert_from(response['content-type'], response.body)
+            # Convert the response to a deserialized object.
+            model.convert_from(response['content-type'], response.body)
+        else
+            # Raise the http error if we didn't get a 'success' of some kind.
+            message = "Server returned %s: %s" % [response.code, response.message]
+            raise Net::HTTPError.new(message, response)
+        end
     end
 
     # Provide appropriate headers.
@@ -32,9 +42,9 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
     
     def search(request)
         if request.key
-            path = "/#{indirection.name}/#{request.key}"
+            path = "/#{indirection.name}s/#{request.key}"
         else
-            path = "/#{indirection.name}"
+            path = "/#{indirection.name}s"
         end
         deserialize network.get(path, headers)
     end
diff --git a/lib/puppet/network/http.rb b/lib/puppet/network/http.rb
index c219859b6..3b81d38b5 100644
--- a/lib/puppet/network/http.rb
+++ b/lib/puppet/network/http.rb
@@ -1,4 +1,4 @@
-class Puppet::Network::HTTP
+module Puppet::Network::HTTP
     def self.server_class_by_type(kind)
         case kind.to_sym
         when :webrick:
diff --git a/lib/puppet/network/http/handler.rb b/lib/puppet/network/http/handler.rb
index ba95d3d11..1a21bfea9 100644
--- a/lib/puppet/network/http/handler.rb
+++ b/lib/puppet/network/http/handler.rb
@@ -29,7 +29,7 @@ module Puppet::Network::HTTP::Handler
         return do_save(request, response)       if put?(request)    and singular?(request)
         raise ArgumentError, "Did not understand HTTP #{http_method(request)} request for '#{path(request)}'"
     rescue Exception => e
-        return do_exception(request, response, e)
+        return do_exception(response, e)
     end
 
     # Are we interacting with a singular instance?
@@ -52,11 +52,22 @@ module Puppet::Network::HTTP::Handler
         raise NotImplementedError
     end
 
+    def do_exception(response, exception, status=400)
+        if exception.is_a?(Exception)
+            puts exception.backtrace if Puppet[:trace]
+            puts exception if Puppet[:trace]
+        end
+        set_content_type(response, "text/plain")
+        set_response(response, exception.to_s, status)
+    end
+
     # Execute our find.
     def do_find(request, response)
         key = request_key(request) || raise(ArgumentError, "Could not locate lookup key in request path [#{path(request)}]")
         args = params(request)
-        result = model.find(key, args)
+        unless result = model.find(key, args)
+            return do_exception(response, "Could not find %s %s" % [model.name, key], 404)
+        end
 
         # The encoding of the result must include the format to use,
         # and it needs to be used for both the rendering and as
@@ -114,10 +125,6 @@ module Puppet::Network::HTTP::Handler
         object.save(args)
     end
 
-    def do_exception(request, response, exception, status=400)
-        set_response(response, exception.to_s, status)
-    end
-
     def find_model_for_handler(handler)
         Puppet::Indirector::Indirection.model(handler) || 
             raise(ArgumentError, "Cannot locate indirection [#{handler}].")
diff --git a/lib/puppet/network/http/mongrel/rest.rb b/lib/puppet/network/http/mongrel/rest.rb
index d6c2e4679..d265dde86 100644
--- a/lib/puppet/network/http/mongrel/rest.rb
+++ b/lib/puppet/network/http/mongrel/rest.rb
@@ -49,7 +49,15 @@ class Puppet::Network::HTTP::MongrelREST < Mongrel::HttpHandler
 
     # produce the body of the response
     def set_response(response, result, status = 200)
-        response.start(status) do |head, body|
+        args = [status]
+
+        # Set the 'reason' (or 'message', as it's called in Webrick), when
+        # we have a failure.
+        if status >= 300
+            args << false << result
+        end
+
+        response.start(*args) do |head, body|
             body.write(result)
         end
     end
diff --git a/lib/puppet/network/http/webrick/rest.rb b/lib/puppet/network/http/webrick/rest.rb
index c7cc06916..13f795fb2 100644
--- a/lib/puppet/network/http/webrick/rest.rb
+++ b/lib/puppet/network/http/webrick/rest.rb
@@ -22,7 +22,7 @@ class Puppet::Network::HTTP::WEBrickREST < WEBrick::HTTPServlet::AbstractServlet
     end
 
     def accept_header(request)
-        request[:accept]
+        request["accept"]
     end
 
     def http_method(request)
@@ -45,12 +45,16 @@ class Puppet::Network::HTTP::WEBrickREST < WEBrick::HTTPServlet::AbstractServlet
 
     # Set the specified format as the content type of the response.
     def set_content_type(response, format)
-        response[:content_type] = format
+        response["content-type"] = format
     end
 
     def set_response(response, result, status = 200)
         response.status = status
-        response.body = result
+        if status >= 200 and status < 300
+            response.body = result
+        else
+            response.reason_phrase = result
+        end
     end
 
     # Retrieve node/cert/ip information from the request object.
diff --git a/spec/integration/indirector/rest.rb b/spec/integration/indirector/rest.rb
index 63291dc5c..6b0fa972b 100755
--- a/spec/integration/indirector/rest.rb
+++ b/spec/integration/indirector/rest.rb
@@ -41,16 +41,18 @@ describe Puppet::Indirector::REST do
 
         Puppet.settings[:confdir] = @dir
         Puppet.settings[:vardir] = @dir
+        Puppet.settings[:server] = "127.0.0.1"
+        Puppet.settings[:masterport] = "34343"
         Puppet.settings[:http_enable_post_connection_check] = false
 
         Puppet::SSL::Host.ca_location = :local
 
         Puppet::TestIndirectedFoo.terminus_class = :rest
-        Puppet::TestIndirectedFoo.indirection.terminus.stubs(:rest_connection_details).returns(:host => "127.0.0.1", :port => "34343")
     end
 
     after do
         Puppet::Network::HttpPool.instance_variable_set("@ssl_host", nil)
+        Puppet.settings.clear
     end
 
     describe "when using webrick" do
@@ -73,7 +75,8 @@ 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', :find => @model_instance)
+                    @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
             
@@ -96,7 +99,7 @@ describe Puppet::Indirector::REST do
         
             describe "when no matching model instance can be found" do
                 before :each do
-                    @mock_model = stub('faked model', :find => nil)
+                    @mock_model = stub('faked model', :name => "foo", :find => nil)
                     Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:model).returns(@mock_model)
                 end
             
@@ -107,13 +110,13 @@ 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')
+                    @mock_model = stub('faked model', :name => "foo")
                     @mock_model.stubs(:find).raises(RuntimeError)
                     Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:model).returns(@mock_model)                
                 end
             
                 it "should raise an exception" do
-                    lambda { Puppet::TestIndirectedFoo.find('bar') }.should raise_error(RuntimeError) 
+                    lambda { Puppet::TestIndirectedFoo.find('bar') }.should raise_error(Net::HTTPError)
                 end
             end
         end
@@ -122,7 +125,7 @@ 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', :search => @model_instances)
+                    @mock_model = stub('faked model', :name => "foo", :search => @model_instances)
                     Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:model).returns(@mock_model)                
                 end
             
@@ -147,7 +150,7 @@ describe Puppet::Indirector::REST do
 
             describe "when no matching model instance can be found" do
                 before :each do
-                    @mock_model = stub('faked model', :find => nil)
+                    @mock_model = stub('faked model', :name => "foo", :find => nil)
                     Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:model).returns(@mock_model)
                 end
             
@@ -164,7 +167,7 @@ describe Puppet::Indirector::REST do
                 end
             
                 it "should raise an exception" do
-                    lambda { Puppet::TestIndirectedFoo.find('bar') }.should raise_error(RuntimeError) 
+                    lambda { Puppet::TestIndirectedFoo.find('bar') }.should raise_error(Net::HTTPError) 
                 end
             end
         end
@@ -172,7 +175,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', :destroy => true)
+                    @mock_model = stub('faked model', :name => "foo", :destroy => true)
                     Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:model).returns(@mock_model)                
                 end
             
@@ -187,7 +190,7 @@ describe Puppet::Indirector::REST do
         
             describe "when no matching model instance can be found" do
                 before :each do
-                    @mock_model = stub('faked model', :destroy => false)
+                    @mock_model = stub('faked model', :name => "foo", :destroy => false)
                     Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:model).returns(@mock_model)
                 end
             
@@ -198,13 +201,13 @@ 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')
+                    @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
-                    lambda { Puppet::TestIndirectedFoo.destroy('bar') }.should raise_error(RuntimeError) 
+                    lambda { Puppet::TestIndirectedFoo.destroy('bar') }.should raise_error(Net::HTTPError) 
                 end
             end
         end
@@ -212,7 +215,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', :from_yaml => @instance)
+                @mock_model = stub('faked model', :name => "foo", :convert_from => @instance)
                 Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:model).returns(@mock_model)                
                 Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:save_object).returns(@instance)                
             end
@@ -250,7 +253,7 @@ describe Puppet::Indirector::REST do
                 end
             
                 it "should raise an exception" do
-                    lambda { @instance.save }.should raise_error(RuntimeError) 
+                    lambda { @instance.save }.should raise_error(Net::HTTPError) 
                 end
             end
         end
@@ -285,7 +288,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', :find => @model_instance)
+                    @mock_model = stub('faked model', :name => "foo", :find => @model_instance)
                     Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)                
                 end
             
@@ -308,7 +311,7 @@ describe Puppet::Indirector::REST do
         
             describe "when no matching model instance can be found" do
                 before :each do
-                    @mock_model = stub('faked model', :find => nil)
+                    @mock_model = stub('faked model', :name => "foo", :find => nil)
                     Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)
                 end
             
@@ -319,13 +322,13 @@ 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')
+                    @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
-                    lambda { Puppet::TestIndirectedFoo.find('bar') }.should raise_error(RuntimeError) 
+                    lambda { Puppet::TestIndirectedFoo.find('bar') }.should raise_error(Net::HTTPError) 
                 end
             end
         end
@@ -334,7 +337,7 @@ 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', :search => @model_instances)
+                    @mock_model = stub('faked model', :name => "foo", :search => @model_instances)
                     Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)                
                 end
             
@@ -365,7 +368,7 @@ describe Puppet::Indirector::REST do
         
             describe "when no matching model instance can be found" do
                 before :each do
-                    @mock_model = stub('faked model', :find => nil)
+                    @mock_model = stub('faked model', :name => "foo", :find => nil)
                     Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)
                 end
             
@@ -376,13 +379,13 @@ 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')
+                    @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
-                    lambda { Puppet::TestIndirectedFoo.find('bar') }.should raise_error(RuntimeError) 
+                    lambda { Puppet::TestIndirectedFoo.find('bar') }.should raise_error(Net::HTTPError) 
                 end
             end
         end
@@ -390,7 +393,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', :destroy => true)
+                    @mock_model = stub('faked model', :name => "foo", :destroy => true)
                     Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)                
                 end
             
@@ -405,7 +408,7 @@ describe Puppet::Indirector::REST do
         
             describe "when no matching model instance can be found" do
                 before :each do
-                    @mock_model = stub('faked model', :destroy => false)
+                    @mock_model = stub('faked model', :name => "foo", :destroy => false)
                     Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)
                 end
             
@@ -416,13 +419,13 @@ 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')
+                    @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
-                    lambda { Puppet::TestIndirectedFoo.destroy('bar') }.should raise_error(RuntimeError) 
+                    lambda { Puppet::TestIndirectedFoo.destroy('bar') }.should raise_error(Net::HTTPError) 
                 end
             end
         end
@@ -430,7 +433,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', :from_yaml => @instance)
+                @mock_model = stub('faked model', :name => "foo", :convert_from => @instance)
                 Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:model).returns(@mock_model)                
 
                 # LAK:NOTE This stub is necessary to prevent the REST call from calling
@@ -471,7 +474,7 @@ describe Puppet::Indirector::REST do
                 end
             
                 it "should raise an exception" do
-                    lambda { @instance.save }.should raise_error(RuntimeError) 
+                    lambda { @instance.save }.should raise_error(Net::HTTPError) 
                 end
             end
         end
diff --git a/spec/unit/indirector/rest.rb b/spec/unit/indirector/rest.rb
index 350b8a1d0..8d88ca7bb 100755
--- a/spec/unit/indirector/rest.rb
+++ b/spec/unit/indirector/rest.rb
@@ -46,25 +46,32 @@ describe Puppet::Indirector::REST do
     end
 
     describe "when deserializing responses" do
-        it "should return the results of converting from the format specified by the content-type header" do
-            @model.expects(:convert_from).with("myformat", "mydata").returns "myobject"
-
+        it "should return nil if the response code is 404" do
             response = mock 'response'
-            response.expects(:[]).with("content-type").returns "myformat"
-            response.expects(:body).returns "mydata"
-            response.stubs(:code).returns "200"
+            response.expects(:code).returns "404"
 
-            @searcher.deserialize(response)
+            @searcher.deserialize(response).should be_nil
         end
 
-        it "should fail if the response is not a success" do
+        it "should fail if the response code is not in the 200s" do
             @model.expects(:convert_from).never
 
             response = mock 'response'
-            response.expects(:code).returns "300"
-            response.expects(:error!).raises ArgumentError
+            response.stubs(:code).returns "300"
+            response.stubs(:message).returns "There was a problem"
 
-            lambda { @searcher.deserialize(response) }.should raise_error(ArgumentError)
+            lambda { @searcher.deserialize(response) }.should raise_error(Net::HTTPError)
+        end
+
+        it "should return the results of converting from the format specified by the content-type header if the response code is in the 200s" do
+            @model.expects(:convert_from).with("myformat", "mydata").returns "myobject"
+
+            response = mock 'response'
+            response.stubs(:[]).with("content-type").returns "myformat"
+            response.stubs(:body).returns "mydata"
+            response.stubs(:code).returns "200"
+
+            @searcher.deserialize(response)
         end
     end
 
@@ -147,15 +154,15 @@ describe Puppet::Indirector::REST do
             @searcher.search(@request).should == 'myobject'
         end        
 
-        it "should use the indirection name as the path if there is no request key" do
-            should_path = "/%s" % [@indirection.name.to_s]
+        it "should use the plural indirection name as the path if there is no request key" do
+            should_path = "/%ss" % [@indirection.name.to_s]
             @request.stubs(:key).returns nil
             @connection.expects(:get).with { |path, args| path == should_path }.returns(@response)
             @searcher.search(@request)
         end
 
-        it "should use the indirection name and request key to create the path if the request key is set" do
-            should_path = "/%s/%s" % [@indirection.name.to_s, "foo"]
+        it "should use the plural indirection name and request key to create the path if the request key is set" do
+            should_path = "/%ss/%s" % [@indirection.name.to_s, "foo"]
             @connection.expects(:get).with { |path, args| path == should_path }.returns(@response)
             @searcher.search(@request)
         end
diff --git a/spec/unit/network/http/handler.rb b/spec/unit/network/http/handler.rb
index 36e566624..0f60b9b10 100755
--- a/spec/unit/network/http/handler.rb
+++ b/spec/unit/network/http/handler.rb
@@ -212,6 +212,11 @@ describe Puppet::Network::HTTP::Handler do
             @handler.process(@request, @response)
         end
 
+        it "should set the format to text/plain when serializing an exception" do
+            @handler.expects(:set_content_type).with(@response, "text/plain")
+            @handler.do_exception(@response, "A test", 404)
+        end
+
         describe "when finding a model instance" do
             before do
                 @handler.stubs(:http_method).returns('GET')
@@ -262,6 +267,13 @@ describe Puppet::Network::HTTP::Handler do
                 @handler.do_find(@request, @response)
             end
 
+            it "should return a 404 when no model instance can be found" do
+                @model_class.stubs(:name).returns "my name"
+                @handler.expects(:set_response).with { |response, body, status| status == 404 }
+                @model_class.stubs(:find).returns(nil)
+                @handler.do_find(@request, @response)
+            end
+
             it "should serialize the result in with the appropriate format" do
                 @model_instance = stub('model instance')
 
diff --git a/spec/unit/network/http/webrick/rest.rb b/spec/unit/network/http/webrick/rest.rb
index b42053d53..4c72ec545 100755
--- a/spec/unit/network/http/webrick/rest.rb
+++ b/spec/unit/network/http/webrick/rest.rb
@@ -33,7 +33,7 @@ describe Puppet::Network::HTTP::WEBrickREST do
 
         describe "when using the Handler interface" do
             it "should use the 'accept' request parameter as the Accept header" do
-                @request.expects(:[]).with(:accept).returns "foobar"
+                @request.expects(:[]).with("accept").returns "foobar"
                 @handler.accept_header(@request).should == "foobar"
             end
 
@@ -57,17 +57,24 @@ describe Puppet::Network::HTTP::WEBrickREST do
                 @handler.body(@request).should == "my body"
             end
 
-            it "should set the response's :content_type header when setting the content type" do
-                @response.expects(:[]=).with(:content_type, "text/html")
+            it "should set the response's 'content-type' header when setting the content type" do
+                @response.expects(:[]=).with("content-type", "text/html")
                 @handler.set_content_type(@response, "text/html")
             end
 
-            it "should set the status and body on the response when setting the response" do
+            it "should set the status and body on the response when setting the response for a successful query" do
                 @response.expects(:status=).with 200
                 @response.expects(:body=).with "mybody"
 
                 @handler.set_response(@response, "mybody", 200)
             end
+
+            it "should set the status and message on the response when setting the response for a failed query" do
+                @response.expects(:status=).with 400
+                @response.expects(:reason_phrase=).with "mybody"
+
+                @handler.set_response(@response, "mybody", 400)
+            end
         end
 
         describe "and determining the request parameters" do