[#4026] When --use_cached_catalog is specified on a puppetd run actully use the cache

Running puppetd with --use_cached_catalog you would see messages like:

    info: Not using expired catalog for mattmac.local from cache
    notice: Using cached catalog

Both Puppet::Util::Cacher, which extends catalogs, and Puppet::Indirector::Envelope,
which extends all Indirection objects including catalogs, have their own cache
expiring mechanisms.  The Envelope mechanism was declining to use cached
catalogs without taking into account what the --use_cached_catalog
options said.  This patch fixes so it uses the cached catalog and just logs:

    debug: Using cached catalog for mattmac.local

This commit also renames a method that makes requests from request
to instantiate request, and gets rid of the extender hook on Cacher
since it was only being used on one test and probably shouldn't be used
in general.

Reviewed by: Nick Lewis

4 files changed, 27 insertions, 20 deletions

diff --git a/spec/integration/indirector/direct_file_server.rb b/spec/integration/indirector/direct_file_server.rb
index 9843c7e65..c33d23e62 100755
--- a/spec/integration/indirector/direct_file_server.rb
+++ b/spec/integration/indirector/direct_file_server.rb
@@ -18,7 +18,7 @@ describe Puppet::Indirector::DirectFileServer, " when interacting with the files
     it "should return an instance of the model" do
         FileTest.expects(:exists?).with(@filepath).returns(true)
 
-        @terminus.find(@terminus.indirection.request(:find, "file://host#{@filepath}")).should be_instance_of(Puppet::FileServing::Content)
+        @terminus.find(@terminus.indirection.instantiate_request(:find, "file://host#{@filepath}")).should be_instance_of(Puppet::FileServing::Content)
     end
 
     it "should return an instance capable of returning its content" do
@@ -26,7 +26,7 @@ describe Puppet::Indirector::DirectFileServer, " when interacting with the files
         File.stubs(:lstat).with(@filepath).returns(stub("stat", :ftype => "file"))
         File.expects(:read).with(@filepath).returns("my content")
 
-        instance = @terminus.find(@terminus.indirection.request(:find, "file://host#{@filepath}"))
+        instance = @terminus.find(@terminus.indirection.instantiate_request(:find, "file://host#{@filepath}"))
 
         instance.content.should == "my content"
     end
@@ -45,7 +45,7 @@ describe Puppet::Indirector::DirectFileServer, " when interacting with FileServi
         File.open(File.join(@path, "one"), "w") { |f| f.print "one content" }
         File.open(File.join(@path, "two"), "w") { |f| f.print "two content" }
 
-        @request = @terminus.indirection.request(:search, "file:///%s" % @path, :recurse => true)
+        @request = @terminus.indirection.instantiate_request(:search, "file:///%s" % @path, :recurse => true)
     end
 
     after do
diff --git a/spec/unit/indirector/indirection.rb b/spec/unit/indirector/indirection.rb
index 2ed51d7c4..8049d771d 100755
--- a/spec/unit/indirector/indirection.rb
+++ b/spec/unit/indirector/indirection.rb
@@ -8,7 +8,7 @@ describe "Indirection Delegator", :shared => true do
     it "should create a request object with the appropriate method name and all of the passed arguments" do
         request = Puppet::Indirector::Request.new(:indirection, :find, "me")
 
-        @indirection.expects(:request).with(@method, "mystuff", :one => :two).returns request
+        @indirection.expects(:instantiate_request).with(@method, "mystuff", :one => :two).returns request
 
         @terminus.stubs(@method)
 
@@ -24,7 +24,7 @@ describe "Indirection Delegator", :shared => true do
 
         request = Puppet::Indirector::Request.new(:indirection, :find, "me")
 
-        @indirection.stubs(:request).returns request
+        @indirection.stubs(:instantiate_request).returns request
 
         @indirection.expects(:select_terminus).with(request).returns :test_terminus
 
@@ -43,7 +43,7 @@ describe "Indirection Delegator", :shared => true do
 
         request = stub 'request', :key => "me", :options => {}
 
-        @indirection.stubs(:request).returns request
+        @indirection.stubs(:instantiate_request).returns request
 
         @indirection.expects(:select_terminus).with(request).returns nil
 
@@ -179,34 +179,34 @@ describe Puppet::Indirector::Indirection do
         end
 
         it "should have a method for creating an indirection request instance" do
-            @indirection.should respond_to(:request)
+            @indirection.should respond_to(:instantiate_request)
         end
 
         describe "creates a request" do
             it "should create it with its name as the request's indirection name" do
                 Puppet::Indirector::Request.expects(:new).with { |name, *other| @indirection.name == name }
-                @indirection.request(:funtest, "yayness")
+                @indirection.instantiate_request(:funtest, "yayness")
             end
 
             it "should require a method and key" do
                 Puppet::Indirector::Request.expects(:new).with { |name, method, key, *other| method == :funtest and key == "yayness" }
-                @indirection.request(:funtest, "yayness")
+                @indirection.instantiate_request(:funtest, "yayness")
             end
 
             it "should support optional arguments" do
                 Puppet::Indirector::Request.expects(:new).with { |name, method, key, other| other == {:one => :two} }
-                @indirection.request(:funtest, "yayness", :one => :two)
+                @indirection.instantiate_request(:funtest, "yayness", :one => :two)
             end
 
             it "should default to the arguments being nil" do
                 Puppet::Indirector::Request.expects(:new).with { |name, method, key, args| args.nil? }
-                @indirection.request(:funtest, "yayness")
+                @indirection.instantiate_request(:funtest, "yayness")
             end
 
             it "should return the request" do
                 request = mock 'request'
                 Puppet::Indirector::Request.expects(:new).returns request
-                @indirection.request(:funtest, "yayness").should equal(request)
+                @indirection.instantiate_request(:funtest, "yayness").should equal(request)
             end
         end
 
@@ -405,7 +405,7 @@ describe Puppet::Indirector::Indirection do
                 it "should return the result of saving to the terminus" do
                     request = stub 'request', :instance => @instance, :node => nil
 
-                    @indirection.expects(:request).returns request
+                    @indirection.expects(:instantiate_request).returns request
 
                     @cache.stubs(:save)
                     @terminus.stubs(:save).returns @instance
@@ -415,7 +415,7 @@ describe Puppet::Indirector::Indirection do
                 it "should use a request to save the object to the cache" do
                     request = stub 'request', :instance => @instance, :node => nil
 
-                    @indirection.expects(:request).returns request
+                    @indirection.expects(:instantiate_request).returns request
 
                     @cache.expects(:save).with(request)
                     @terminus.stubs(:save)
@@ -425,7 +425,7 @@ describe Puppet::Indirector::Indirection do
                 it "should not save to the cache if the normal save fails" do
                     request = stub 'request', :instance => @instance, :node => nil
 
-                    @indirection.expects(:request).returns request
+                    @indirection.expects(:instantiate_request).returns request
 
                     @cache.expects(:save).never
                     @terminus.expects(:save).raises "eh"
@@ -457,8 +457,8 @@ describe Puppet::Indirector::Indirection do
                     destroy = stub 'destroy_request', :key => "/my/key", :node => nil
                     find = stub 'destroy_request', :key => "/my/key", :node => nil
 
-                    @indirection.expects(:request).with(:destroy, "/my/key").returns destroy
-                    @indirection.expects(:request).with(:find, "/my/key").returns find
+                    @indirection.expects(:instantiate_request).with(:destroy, "/my/key").returns destroy
+                    @indirection.expects(:instantiate_request).with(:find, "/my/key").returns find
 
                     cached = mock 'cache'
 
diff --git a/spec/unit/resource/catalog.rb b/spec/unit/resource/catalog.rb
index 39ddccd9b..0835bf982 100755
--- a/spec/unit/resource/catalog.rb
+++ b/spec/unit/resource/catalog.rb
@@ -35,6 +35,13 @@ describe Puppet::Resource::Catalog, "when compiling" do
         @catalog.write_class_file
     end
 
+    it "should not be expired if use_cached_catalog option is true" do
+        Puppet.settings[:use_cached_catalog] = true
+        @catalog = Puppet::Resource::Catalog.new("host")
+        @catalog.expiration = Time.now
+        @catalog.should_not be_expired
+    end
+
     it "should have a client_version attribute" do
         @catalog = Puppet::Resource::Catalog.new("host")
         @catalog.client_version = 5
diff --git a/spec/unit/util/cacher.rb b/spec/unit/util/cacher.rb
index eb8515e4d..d02852c13 100755
--- a/spec/unit/util/cacher.rb
+++ b/spec/unit/util/cacher.rb
@@ -151,11 +151,11 @@ describe Puppet::Util::Cacher do
 
         it "should not check for a ttl expiration if the class does not support that method" do
             klass = Class.new do
-                extend Puppet::Util::Cacher
+                include Puppet::Util::Cacher
             end
 
-            klass.singleton_class.cached_attr(:myattr) { "eh" }
-            klass.myattr
+            klass.cached_attr(:myattr) { "eh" }
+            lambda { klass.new.myattr }.should_not raise_error
         end
 
         it "should automatically expire cached attributes whose ttl has expired, even if no expirer is present" do