Refactored to use a Handler base class for server+protocol handlers. Finally eliminated dependency on Puppet.start, etc., from WEBrick HTTP server class. {webrick,mongrel}+REST now support request handling uniformly; need encode/decode next.

6 files changed, 127 insertions, 77 deletions

diff --git a/lib/puppet/network/http/handler.rb b/lib/puppet/network/http/handler.rb
new file mode 100644
index 000000000..54cec417b
--- /dev/null
+++ b/lib/puppet/network/http/handler.rb
@@ -0,0 +1,57 @@
+class Puppet::Network::HTTP::Handler
+    def initialize(args = {})
+        raise ArgumentError unless @server = args[:server]
+        raise ArgumentError unless @handler = args[:handler]
+        register_handler
+    end
+    
+    # handle an HTTP request coming from Mongrel
+    def process(request, response)
+        return @model.find     if get?(request)    and singular?(request)
+        return @model.search   if get?(request)    and plural?(request)
+        return @model.destroy  if delete?(request) and singular?(request)
+        return @model.new.save if put?(request) and singular?(request)
+        raise ArgumentError, "Did not understand HTTP #{http_method(request)} request for '#{path(request)}'"
+    end
+    
+  private
+    
+    def find_model_for_handler(handler)
+        Puppet::Indirector::Indirection.model(handler) || 
+            raise(ArgumentError, "Cannot locate indirection [#{handler}].")
+    end
+    
+    def get?(request)
+        http_method(request) == 'GET'
+    end
+    
+    def put?(request)
+        http_method(request) == 'PUT'
+    end
+    
+    def delete?(request)
+        http_method(request) == 'DELETE'
+    end
+    
+    def singular?(request)
+        %r{/#{@handler.to_s}$}.match(path(request))
+    end
+    
+    def plural?(request)
+        %r{/#{@handler.to_s}s$}.match(path(request))
+    end
+    
+  # methods specific to a given web server
+    
+    def register_handler
+        raise UnimplementedError
+    end
+    
+    def http_method(request)
+        raise UnimplementedError
+    end
+    
+    def path(request)
+        raise UnimplementedError
+    end    
+end
diff --git a/lib/puppet/network/http/mongrel/rest.rb b/lib/puppet/network/http/mongrel/rest.rb
index f9a6e3796..8f3de957e 100644
--- a/lib/puppet/network/http/mongrel/rest.rb
+++ b/lib/puppet/network/http/mongrel/rest.rb
@@ -1,47 +1,9 @@
-class Puppet::Network::HTTP::MongrelREST
-    def initialize(args = {})
-        raise ArgumentError unless @server = args[:server]
-        raise ArgumentError unless @handler = args[:handler]
-        register_handler
-    end
-    
-    # handle an HTTP request coming from Mongrel
-    def process(request, response)
-        return @model.find     if get?(request)    and singular?(request)
-        return @model.search   if get?(request)    and plural?(request)
-        return @model.destroy  if delete?(request) and singular?(request)
-        return @model.new.save if put?(request) and singular?(request)
-        raise ArgumentError, "Did not understand HTTP #{http_method(request)} request for '#{path(request)}'"
-        # TODO: here, raise an exception, or do some defaulting or something
-    end
-    
+require 'puppet/network/http/handler'
+
+class Puppet::Network::HTTP::MongrelREST < Puppet::Network::HTTP::Handler
+
   private
     
-    def find_model_for_handler(handler)
-        Puppet::Indirector::Indirection.model(handler) || 
-            raise(ArgumentError, "Cannot locate indirection [#{handler}].")
-    end
-    
-    def get?(request)
-        http_method(request) == 'GET'
-    end
-    
-    def put?(request)
-        http_method(request) == 'PUT'
-    end
-    
-    def delete?(request)
-        http_method(request) == 'DELETE'
-    end
-    
-    def singular?(request)
-        %r{/#{@handler.to_s}$}.match(path(request))
-    end
-    
-    def plural?(request)
-        %r{/#{@handler.to_s}s$}.match(path(request))
-    end
-    
     def register_handler
         @model = find_model_for_handler(@handler)
         @server.register('/' + @handler.to_s, self)
@@ -55,5 +17,4 @@ class Puppet::Network::HTTP::MongrelREST
     def path(request)
         request.params[Mongrel::Const::REQUEST_PATH]
     end
-    
 end
diff --git a/lib/puppet/network/http/webrick.rb b/lib/puppet/network/http/webrick.rb
index 53aa2e99b..c4b2ed3c6 100644
--- a/lib/puppet/network/http/webrick.rb
+++ b/lib/puppet/network/http/webrick.rb
@@ -18,19 +18,14 @@ class Puppet::Network::HTTP::WEBrick
         @protocols = args[:protocols]
         @handlers = args[:handlers]        
         @server = WEBrick::HTTPServer.new(:BindAddress => args[:address], :Port => args[:port])
-        
         setup_handlers
-        
-        # TODO / FIXME is this really necessary? -- or can we do it in both mongrel and webrick?
-        Puppet.newservice(@server)
-        Puppet.start
-        
+        @server.start
         @listening = true
     end
     
     def unlisten
         raise "WEBrick server is not listening" unless listening?
-        shutdown
+        @server.shutdown
         @listening = false
     end
     
diff --git a/lib/puppet/network/http/webrick/rest.rb b/lib/puppet/network/http/webrick/rest.rb
index 7b9a3912b..cefffd76d 100644
--- a/lib/puppet/network/http/webrick/rest.rb
+++ b/lib/puppet/network/http/webrick/rest.rb
@@ -1,8 +1,10 @@
-class Puppet::Network::HTTP::WEBrickREST
-    def initialize(args = {})
-        raise ArgumentError unless @server = args[:server]
-        raise ArgumentError unless @handler = args[:handler]
-        register_handler
+require 'puppet/network/http/handler'
+
+class Puppet::Network::HTTP::WEBrickREST < Puppet::Network::HTTP::Handler
+
+    # WEBrick uses a service() method to respond to requests.  Simply delegate to the handler response() method.
+    def service(request, response)
+        process(request, response)
     end
 
   private
@@ -13,8 +15,11 @@ class Puppet::Network::HTTP::WEBrickREST
         @server.mount('/' + @handler.to_s + 's', self)
     end
 
-    def find_model_for_handler(handler)
-        Puppet::Indirector::Indirection.model(handler) || 
-            raise(ArgumentError, "Cannot locate indirection [#{handler}].")
+    def http_method(request)
+        request.request_method
+    end
+    
+    def path(request)
+        request.path
     end
 end
\ No newline at end of file
diff --git a/spec/unit/network/http/webrick.rb b/spec/unit/network/http/webrick.rb
index 9ba04f164..3ed223e7e 100644
--- a/spec/unit/network/http/webrick.rb
+++ b/spec/unit/network/http/webrick.rb
@@ -14,10 +14,8 @@ end
 
 describe Puppet::Network::HTTP::WEBrick, "when turning on listening" do
     before do
-        Puppet.stubs(:start)
-        Puppet.stubs(:newservice)
         @mock_webrick = mock('webrick')
-        @mock_webrick.stubs(:mount)
+        [:mount, :start, :shutdown].each {|meth| @mock_webrick.stubs(meth)}        
         WEBrick::HTTPServer.stubs(:new).returns(@mock_webrick)
         @server = Puppet::Network::HTTP::WEBrick.new
         @listen_params = { :address => "127.0.0.1", :port => 31337, :handlers => [ :node, :configuration ], :protocols => [ :rest, :xmlrpc ] }
@@ -45,7 +43,7 @@ describe Puppet::Network::HTTP::WEBrick, "when turning on listening" do
     end
 
     it "should order a webrick server to start" do
-        Puppet.expects(:start)
+        @mock_webrick.expects(:start)
         @server.listen(@listen_params)
     end
     
@@ -92,13 +90,10 @@ end
 
 describe Puppet::Network::HTTP::WEBrick, "when turning off listening" do
     before do
-        Puppet.stubs(:start)
-        Puppet.stubs(:newservice)
         @mock_webrick = mock('webrick')
-        @mock_webrick.stubs(:mount)
+        [:mount, :start, :shutdown].each {|meth| @mock_webrick.stubs(meth)}
         WEBrick::HTTPServer.stubs(:new).returns(@mock_webrick)
         @server = Puppet::Network::HTTP::WEBrick.new        
-        @server.stubs(:shutdown)
         @listen_params = { :address => "127.0.0.1", :port => 31337, :handlers => [ :node, :configuration ], :protocols => [ :rest, :xmlrpc ] }
     end
     
@@ -107,8 +102,7 @@ describe Puppet::Network::HTTP::WEBrick, "when turning off listening" do
     end
     
     it "should order webrick server to stop" do
-        @server.should respond_to(:shutdown)
-        @server.expects(:shutdown)
+        @mock_webrick.expects(:shutdown)
         @server.listen(@listen_params)
         @server.unlisten
     end
diff --git a/spec/unit/network/http/webrick/rest.rb b/spec/unit/network/http/webrick/rest.rb
index 418c97b6f..472a09aca 100644
--- a/spec/unit/network/http/webrick/rest.rb
+++ b/spec/unit/network/http/webrick/rest.rb
@@ -49,19 +49,57 @@ describe Puppet::Network::HTTP::WEBrickREST, "when initializing" do
 end
 
 describe Puppet::Network::HTTP::WEBrickREST, "when receiving a request" do
-    it "should unpack request information from WEBrick"
-    
-    it "should unpack parameters from the request for passing to controller methods"    
+    before do
+        @mock_request = mock('webrick http request')
+        @mock_response = mock('webrick http response')
+        @mock_model_class = mock('indirected model class')
+        Puppet::Indirector::Indirection.stubs(:model).with(:foo).returns(@mock_model_class)
+        @mock_webrick = mock('mongrel http server')
+        @mock_webrick.stubs(:mount)
+        @handler = Puppet::Network::HTTP::WEBrickREST.new(:server => @mock_webrick, :handler => :foo)
+    end
     
-    it "should call the controller find method if the request represents a singular HTTP GET"
+    it "should call the model find method if the request represents a singular HTTP GET" do
+        @mock_request.stubs(:request_method).returns('GET')
+        @mock_request.stubs(:path).returns('/foo')
+        @mock_model_class.expects(:find)
+        @handler.service(@mock_request, @mock_response)
+    end
+
+    it "should call the model search method if the request represents a plural HTTP GET" do
+        @mock_request.stubs(:request_method).returns('GET')
+        @mock_request.stubs(:path).returns('/foos')
+        @mock_model_class.expects(:search)
+        @handler.service(@mock_request, @mock_response)
+    end
     
-    it "should call the controller search method if the request represents a plural HTTP GET"
+    it "should call the model destroy method if the request represents an HTTP DELETE" do
+        @mock_request.stubs(:request_method).returns('DELETE')
+        @mock_request.stubs(:path).returns('/foo')
+        @mock_model_class.expects(:destroy)
+        @handler.service(@mock_request, @mock_response)
+    end
+
+    it "should call the model save method if the request represents an HTTP PUT" do
+        @mock_request.stubs(:request_method).returns('PUT')
+        @mock_request.stubs(:path).returns('/foo')
+        mock_model_instance = mock('indirected model instance')
+        mock_model_instance.expects(:save)
+        @mock_model_class.expects(:new).returns(mock_model_instance)
+        @handler.service(@mock_request, @mock_response)
+    end
     
-    it "should call the controller destroy method if the request represents an HTTP DELETE"
+    it "should fail if the HTTP method isn't supported" do
+        @mock_request.stubs(:request_method).returns('POST')
+        @mock_request.stubs(:path).returns('/foo')
+        Proc.new { @handler.service(@mock_request, @mock_response) }.should raise_error(ArgumentError)
+    end
+
+    it "should unpack request information from WEBrick"
     
-    it "should call the controller save method if the request represents an HTTP PUT"
+    it "should unpack parameters from the request for passing to controller methods"    
     
-    it "should serialize the result from the controller method for return back to WEBrick"
+    it "should serialize the result from the controller method for return back to Mongrel"
     
-    it "should serialize a controller exception result for return back to WEBrick"
+    it "should serialize a controller exception result for return back to Mongrel"
 end