Much larger commit than I would like to land at once. This is all REST-related code. Two specs are failing related to how Mongrel is initialized for REST; will fix those shortly.

REST indirector now supports find, with deserialization.  Network code in indirector now.  Will still need to un-hardwire address/port for outbound connections.  Will still need to urlencode path parameters.
Code for search, destroy, update is coming, should be similar to find.

Reworked how the Handler module is used.  Needed to be included, rather than inherited.  Needed to sidestep initializers for actual web servers (webrick, mongrel), needed to be possible to have handler-including class be used as a class (aka servlet) instead of as an instance.

Webrick handler registration is now abstracted to "above" the servlet.

Provided a #model method to use instead of @model in handler module.  This allows neutering during testing.

Brought class_for_protocol up into http/webrick class as a (tested) class method.

Integration tests for rest indirection.  Split server integration tests into mongrel and webrick tests.

Got Node/REST working properly wrt the crazy-ass autoloader thing.

We're now actually passing traffic w/ webrick, fwiw.

8 files changed, 200 insertions, 95 deletions

diff --git a/spec/integration/indirector/rest.rb b/spec/integration/indirector/rest.rb
index a8c6f5609..aed17d977 100644
--- a/spec/integration/indirector/rest.rb
+++ b/spec/integration/indirector/rest.rb
@@ -3,32 +3,88 @@ require 'puppet/network/server'
 require 'puppet/indirector'
 require 'puppet/indirector/rest'
 
+# a fake class that will be indirected via REST
 class Puppet::TestIndirectedFoo
   extend Puppet::Indirector  
   indirects :test_indirected_foo, :terminus_setting => :test_indirected_foo_terminus
   
-  def initialize(foo)
-    STDERR.puts "foo!"
+  attr_reader :value
+  
+  def initialize(value = 0)
+    @value = value
+  end
+  
+  def self.from_yaml(yaml)
+    YAML.load(yaml)
   end
 end
 
+# empty Terminus class -- this would normally have to be in a directory findable by the autoloader, but we short-circuit that below
 class Puppet::TestIndirectedFoo::Rest < Puppet::Indirector::REST
 end
 
+
 describe Puppet::Indirector::REST do
   before :each do
-    Puppet::Indirector::Terminus.stubs(:terminus_class).returns(Puppet::TestIndirectedFoo::Rest)
-    Puppet::TestIndirectedFoo.terminus_class = :rest
-    Puppet[:servertype] = 'mongrel'
-    @params = { :address => "127.0.0.1", :port => 34346, :handlers => [ :test_indirected_foo ] }
+    Puppet[:servertype] = 'webrick'
+    @params = { :address => "127.0.0.1", :port => 34343, :handlers => [ :test_indirected_foo ] }
     @server = Puppet::Network::Server.new(@params)
     @server.listen
+
+    # the autoloader was clearly not written test-first.  We subvert the integration test to get around its bullshit.
+    Puppet::Indirector::Terminus.stubs(:terminus_class).returns(Puppet::TestIndirectedFoo::Rest)
+    Puppet::TestIndirectedFoo.terminus_class = :rest
   end
   
-  it "should not fail to find an instance over REST" do
-    lambda { Puppet::TestIndirectedFoo.find('bar') }.should_not raise_error
-  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', :find => @model_instance)
+        Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:model).returns(@mock_model)        
+      end
+      
+      it "should not fail" do
+        lambda { Puppet::TestIndirectedFoo.find('bar') }.should_not raise_error
+      end
+  
+      it 'should return an instance of the model class' do
+        Puppet::TestIndirectedFoo.find('bar').class.should == Puppet::TestIndirectedFoo
+      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
+      end
+  
+      it 'should set a version timestamp on model instance' do
+        Puppet::TestIndirectedFoo.find('bar').version.should_not be_nil
+      end
+    end
+    
+    describe "when no matching model instance can be found" do
+      before :each do
+        @mock_model = stub('faked model', :find => nil)
+        Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:model).returns(@mock_model)
+      end
+      
+      it "should return nil" do
+        Puppet::TestIndirectedFoo.find('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')
+        @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)     
+      # end
+    end
+  end
+
   after :each do
     @server.unlisten
   end
diff --git a/spec/integration/network/server/mongrel.rb b/spec/integration/network/server/mongrel.rb
new file mode 100644
index 000000000..7bf8291a3
--- /dev/null
+++ b/spec/integration/network/server/mongrel.rb
@@ -0,0 +1,44 @@
+require File.dirname(__FILE__) + '/../../../spec_helper'
+require 'puppet/network/server'
+require 'socket'
+
+describe Puppet::Network::Server do
+  describe "when using mongrel" do
+    before :each do
+      Puppet[:servertype] = 'mongrel'
+      @params = { :address => "127.0.0.1", :port => 34346, :handlers => [ :node ] }
+      @server = Puppet::Network::Server.new(@params)      
+    end
+
+    describe "before listening" do
+      it "should not be reachable at the specified address and port" do
+        lambda { TCPSocket.new('127.0.0.1', 34346) }.should raise_error(Errno::ECONNREFUSED)
+      end
+    end
+
+    describe "when listening" do
+      it "should be reachable on the specified address and port" do
+        @server.listen
+        lambda { TCPSocket.new('127.0.0.1', 34346) }.should_not raise_error      
+      end
+
+      it "should not allow multiple servers to listen on the same address and port" do
+        @server.listen
+        @server2 = Puppet::Network::Server.new(@params)
+        lambda { @server2.listen }.should raise_error
+      end
+    end
+    
+    describe "after unlistening" do
+      it "should not be reachable on the port and address assigned" do
+        @server.listen
+        @server.unlisten
+        lambda { TCPSocket.new('127.0.0.1', 34346) }.should raise_error(Errno::ECONNREFUSED)        
+      end
+    end
+      
+    after :each do
+      @server.unlisten if @server.listening?
+    end  
+  end
+end
\ No newline at end of file
diff --git a/spec/integration/network/server.rb b/spec/integration/network/server/webrick.rb
index 932161b08..0ab8d89e4 100644
--- a/spec/integration/network/server.rb
+++ b/spec/integration/network/server/webrick.rb
@@ -1,4 +1,4 @@
-require File.dirname(__FILE__) + '/../../spec_helper'
+require File.dirname(__FILE__) + '/../../../spec_helper'
 require 'puppet/network/server'
 require 'socket'
 
@@ -43,43 +43,4 @@ describe Puppet::Network::Server do
       end
     end
   end
-  
-  describe "when using mongrel" do
-    before :each do
-      Puppet[:servertype] = 'mongrel'
-      @params = { :address => "127.0.0.1", :port => 34346, :handlers => [ :node ] }
-      @server = Puppet::Network::Server.new(@params)      
-    end
-
-    describe "before listening" do
-      it "should not be reachable at the specified address and port" do
-        lambda { TCPSocket.new('127.0.0.1', 34346) }.should raise_error(Errno::ECONNREFUSED)
-      end
-    end
-
-    describe "when listening" do
-      it "should be reachable on the specified address and port" do
-        @server.listen
-        lambda { TCPSocket.new('127.0.0.1', 34346) }.should_not raise_error      
-      end
-
-      it "should not allow multiple servers to listen on the same address and port" do
-        @server.listen
-        @server2 = Puppet::Network::Server.new(@params)
-        lambda { @server2.listen }.should raise_error
-      end
-    end
-    
-    describe "after unlistening" do
-      it "should not be reachable on the port and address assigned" do
-        @server.listen
-        @server.unlisten
-        lambda { TCPSocket.new('127.0.0.1', 34346) }.should raise_error(Errno::ECONNREFUSED)        
-      end
-    end
-      
-    after :each do
-      @server.unlisten if @server.listening?
-    end  
-  end
 end
\ No newline at end of file
diff --git a/spec/unit/indirector/node/rest.rb b/spec/unit/indirector/node/rest.rb
index 33cfda426..3b99e33c4 100755
--- a/spec/unit/indirector/node/rest.rb
+++ b/spec/unit/indirector/node/rest.rb
@@ -4,9 +4,9 @@ require File.dirname(__FILE__) + '/../../../spec_helper'
 
 require 'puppet/indirector/node/rest'
 
-describe Puppet::Node::REST do
+describe Puppet::Node::Rest do
     before do
-        @searcher = Puppet::Node::REST.new
+        @searcher = Puppet::Node::Rest.new
     end
     
     
diff --git a/spec/unit/indirector/rest.rb b/spec/unit/indirector/rest.rb
index 3c4716dc8..56db8f509 100755
--- a/spec/unit/indirector/rest.rb
+++ b/spec/unit/indirector/rest.rb
@@ -4,7 +4,6 @@ require File.dirname(__FILE__) + '/../../spec_helper'
 require 'puppet/indirector/rest'
 
 describe Puppet::Indirector::REST do
-    # FIXME : TODO / look through this, does this make sense?
     before do
         Puppet::Indirector::Terminus.stubs(:register_terminus_class)
         @model = mock 'model'
@@ -20,11 +19,57 @@ describe Puppet::Indirector::REST do
         @searcher = @rest_class.new
     end
 
-    it "should return an instance of the indirected model"    
-    it "should deserialize result data after a call into a Model instance for find"
-    it "should deserialize result data after a call into a list of Model instances for search"
-    it "should deserialize result data after a call into a boolean for save"
-    it "should deserialize result data after a call into a boolean for destroy"
-    it "should generate an error when result data deserializes improperly"
-    it "should generate an error when result data specifies an error"
+    describe "when doing a find" do
+      before :each do
+        @searcher.stubs(:network_fetch).returns({:foo => 'bar'}.to_yaml)  # neuter the network connection
+        @model.stubs(:from_yaml).returns(@instance)
+      end
+      
+      it "should look up the model instance over the network" do
+        @searcher.expects(:network_fetch).returns({:foo => 'bar'}.to_yaml)
+        @searcher.find('foo')
+      end
+      
+      it "should deserialize result data to a Model instance" do
+        @model.expects(:from_yaml)
+        @searcher.find('foo')
+      end
+      
+      it "should return the deserialized Model instance" do
+        @searcher.find('foo').should == @instance     
+      end
+      
+      it "should return nil when deserialized model instance is nil" do
+        @model.stubs(:from_yaml).returns(@instance)
+        @searcher.find('foo').should be_nil
+      end
+      
+      it "should generate an error when result data deserializes improperly" do
+        @model.stubs(:from_yaml).raises(ArgumentError)
+        lambda { @searcher.find('foo') }.should raise_error(ArgumentError)
+      end
+      
+      it "should generate an error when result data specifies an error" do
+        @searcher.stubs(:network_fetch).returns(RuntimeError.new("bogus").to_yaml)
+        lambda { @searcher.find('foo') }.should raise_error(RuntimeError)        
+      end      
+    end
+
+    describe "when doing a search" do
+      it "should deserialize result data into a list of Model instances"
+      it "should generate an error when result data deserializes improperly"
+      it "should generate an error when result data specifies an error"      
+    end
+    
+    describe "when doing a save" do
+      it "should deserialize result data into a boolean"
+      it "should generate an error when result data deserializes improperly"
+      it "should generate an error when result data specifies an error"      
+    end
+    
+    describe "when doing a destroy" do
+      it "should deserialize result data into a boolean"
+      it "should generate an error when result data deserializes improperly"
+      it "should generate an error when result data specifies an error"      
+    end
 end
diff --git a/spec/unit/network/http/webrick.rb b/spec/unit/network/http/webrick.rb
index 6a4c50142..05ed2f0e2 100644
--- a/spec/unit/network/http/webrick.rb
+++ b/spec/unit/network/http/webrick.rb
@@ -14,7 +14,7 @@ end
 
 describe Puppet::Network::HTTP::WEBrick, "when turning on listening" do
     before do
-        @mock_webrick = mock('webrick')
+        @mock_webrick = stub('webrick', :[] => {})
         [:mount, :start, :shutdown].each {|meth| @mock_webrick.stubs(meth)}        
         WEBrick::HTTPServer.stubs(:new).returns(@mock_webrick)
         @server = Puppet::Network::HTTP::WEBrick.new
@@ -64,28 +64,44 @@ describe Puppet::Network::HTTP::WEBrick, "when turning on listening" do
             mock_handler = mock("handler instance for [#{protocol}]")
             mock_handler_class = mock("handler class for [#{protocol}]")
             @listen_params[:handlers].each do |handler|
-                mock_handler_class.expects(:new).with {|args| 
-                    args[:server] == @mock_webrick and args[:handler] == handler
-                }.returns(mock_handler)
+                @mock_webrick.expects(:mount)
             end
-            @server.expects(:class_for_protocol).with(protocol).at_least_once.returns(mock_handler_class)
         end
         @server.listen(@listen_params)        
     end
 
     it "should use a WEBrick + REST class to configure WEBrick when REST services are requested" do
-        Puppet::Network::HTTP::WEBrickREST.expects(:new).at_least_once
+        Puppet::Network::HTTP::WEBrick.expects(:class_for_protocol).with(:rest).at_least_once
         @server.listen(@listen_params.merge(:protocols => [:rest]))
     end
     
     it "should fail if services from an unknown protocol are requested" do
-        Proc.new { @server.listen(@listen_params.merge(:protocols => [ :foo ]))}.should raise_error(ArgumentError)
+        Proc.new { @server.listen(@listen_params.merge(:protocols => [ :foo ]))}.should raise_error
     end
 end
 
+
+describe Puppet::Network::HTTP::WEBrick, "when looking up the class to handle a protocol" do
+  it "should require a protocol" do
+    lambda { Puppet::Network::HTTP::WEBrick.class_for_protocol }.should raise_error(ArgumentError)
+  end
+  
+  it "should accept a protocol" do
+    lambda { Puppet::Network::HTTP::WEBrick.class_for_protocol("bob") }.should_not raise_error(ArgumentError)    
+  end
+  
+  it "should use a WEBrick + REST class when a REST protocol is specified" do
+    Puppet::Network::HTTP::WEBrick.class_for_protocol("rest").should == Puppet::Network::HTTP::WEBrickREST
+  end
+  
+  it "should fail when an unknown protocol is specified" do
+    lambda { Puppet::Network::HTTP::WEBrick.class_for_protocol("abcdefg") }.should raise_error
+  end
+end
+
 describe Puppet::Network::HTTP::WEBrick, "when turning off listening" do
     before do
-        @mock_webrick = mock('webrick')
+        @mock_webrick = stub('webrick', :[] => {})
         [:mount, :start, :shutdown].each {|meth| @mock_webrick.stubs(meth)}
         WEBrick::HTTPServer.stubs(:new).returns(@mock_webrick)
         @server = Puppet::Network::HTTP::WEBrick.new        
diff --git a/spec/unit/network/http/webrick/rest.rb b/spec/unit/network/http/webrick/rest.rb
index aa7a3d53a..cafd38b82 100644
--- a/spec/unit/network/http/webrick/rest.rb
+++ b/spec/unit/network/http/webrick/rest.rb
@@ -8,42 +8,31 @@ require 'puppet/network/http'
 
 describe Puppet::Network::HTTP::WEBrickREST, "when initializing" do
     before do
-        @mock_webrick = stub('WEBrick server', :mount => true)
+        @mock_webrick = stub('WEBrick server', :mount => true, :[] => {})
         @mock_model = mock('indirected model')
         Puppet::Indirector::Indirection.stubs(:model).returns(@mock_model)
-        @params = { :server => @mock_webrick, :handler => :foo }
+        @params = [ @mock_webrick, :foo ]
     end
     
     it "should require access to a WEBrick server" do
-        Proc.new { Puppet::Network::HTTP::WEBrickREST.new(@params.delete_if {|k,v| :server == k })}.should raise_error(ArgumentError)
+        Proc.new { 
+            @params[0] = nil
+            Puppet::Network::HTTP::WEBrickREST.new(*@params)
+        }.should raise_error(ArgumentError)
     end
     
     it "should require an indirection name" do
-        Proc.new { Puppet::Network::HTTP::WEBrickREST.new(@params.delete_if {|k,v| :handler == k })}.should raise_error(ArgumentError)        
+        Proc.new { Puppet::Network::HTTP::WEBrickREST.new(@params.first) }.should raise_error(ArgumentError)        
     end
     
     it "should look up the indirection model from the indirection name" do
         Puppet::Indirector::Indirection.expects(:model).returns(@mock_model)
-        Puppet::Network::HTTP::WEBrickREST.new(@params)
+        Puppet::Network::HTTP::WEBrickREST.new(*@params)
     end
     
     it "should fail if the indirection is not known" do
         Puppet::Indirector::Indirection.expects(:model).returns(nil)
-        Proc.new { Puppet::Network::HTTP::WEBrickREST.new(@params) }.should raise_error(ArgumentError)
-    end
-    
-    it "should register itself with the WEBrick server for the singular HTTP methods" do
-        @mock_webrick.expects(:mount).with do |*args|
-            args.first == '/foo' and args.last.is_a?(Puppet::Network::HTTP::WEBrickREST)
-        end
-        Puppet::Network::HTTP::WEBrickREST.new(@params)
-    end
-
-    it "should register itself with the WEBrick server for the plural GET method" do
-        @mock_webrick.expects(:mount).with do |*args|
-            args.first == '/foos' and args.last.is_a?(Puppet::Network::HTTP::WEBrickREST)
-        end
-        Puppet::Network::HTTP::WEBrickREST.new(@params)
+        Proc.new { Puppet::Network::HTTP::WEBrickREST.new(*@params) }.should raise_error(ArgumentError)
     end
 end
 
@@ -52,9 +41,9 @@ describe Puppet::Network::HTTP::WEBrickREST, "when receiving a request" do
         @mock_request     = stub('webrick http request', :query => {})
         @mock_response    = stub('webrick http response', :status= => true, :body= => true)
         @mock_model_class = stub('indirected model class')
-        @mock_webrick     = stub('mongrel http server', :mount => true)
+        @mock_webrick     = stub('webrick http server', :mount => true, :[] => {})
         Puppet::Indirector::Indirection.stubs(:model).with(:foo).returns(@mock_model_class)
-        @handler = Puppet::Network::HTTP::WEBrickREST.new(:server => @mock_webrick, :handler => :foo)
+        @handler = Puppet::Network::HTTP::WEBrickREST.new(@mock_webrick, :foo)
     end
 
     def setup_find_request
diff --git a/spec/unit/network/server.rb b/spec/unit/network/server.rb
index e4afbc3c2..2e02ffbc3 100644
--- a/spec/unit/network/server.rb
+++ b/spec/unit/network/server.rb
@@ -315,10 +315,4 @@ describe Class.new, "put these somewhere" do
     it "should allow from_* on the inbound :data packet (look at its content_type) when doing a PUT/.new.save"
     it "should prepend a rest version number on the path (w00t)"
     it "should ... on server side, .save should from_yaml, then foo.save(args) instead of just Foo.new.save(args)"
-    it "should have a from_yaml class_method in the indirector (... default:  yaml.load(data) => instance, but can be overridden)"
-end
-
-describe Puppet::Indirector, "stuff required by HTTP servers" do
-    it "should provide the model with the ability to serialize to XML"
-    it "should provide the model with the ability to deserialize from XML"
 end