Adding caching support to parameters, and using cached attributes for file source and metadata.

As hoped, this drastically simplifies the code around retaining
this data.

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

6 files changed, 77 insertions, 46 deletions

diff --git a/lib/puppet/parameter.rb b/lib/puppet/parameter.rb
index f82438903..5f68d44a5 100644
--- a/lib/puppet/parameter.rb
+++ b/lib/puppet/parameter.rb
@@ -2,6 +2,7 @@ require 'puppet/util/methodhelper'
 require 'puppet/util/log_paths'
 require 'puppet/util/logging'
 require 'puppet/util/docs'
+require 'puppet/util/cacher'
 
 class Puppet::Parameter
     include Puppet::Util
@@ -9,6 +10,7 @@ class Puppet::Parameter
     include Puppet::Util::LogPaths
     include Puppet::Util::Logging
     include Puppet::Util::MethodHelper
+    include Puppet::Util::Cacher
 
     # A collection of values and regexes, used for specifying
     # what values are allowed in a given parameter.
@@ -354,6 +356,10 @@ class Puppet::Parameter
         self.fail(Puppet::DevError, msg)
     end
 
+    def expirer
+        resource.catalog
+    end
+
     def fail(*args)
         type = nil
         if args[0].is_a?(Class)
diff --git a/lib/puppet/type/file/source.rb b/lib/puppet/type/file/source.rb
index ba5fcbd0e..4e67b1a8b 100755
--- a/lib/puppet/type/file/source.rb
+++ b/lib/puppet/type/file/source.rb
@@ -101,16 +101,13 @@ module Puppet
         end
 
         # Look up (if necessary) and return remote content.
-        def content
+        cached_attr(:content) do
             raise Puppet::DevError, "No source for content was stored with the metadata" unless metadata.source
 
-            unless defined?(@content) and @content
-                unless tmp = Puppet::FileServing::Content.find(metadata.source)
-                    fail "Could not find any content at %s" % metadata.source
-                end
-                @content = tmp.content
+            unless tmp = Puppet::FileServing::Content.find(metadata.source)
+                fail "Could not find any content at %s" % metadata.source
             end
-            @content
+            tmp.content
         end
 
         # Copy the values from the source to the resource.  Yay.
@@ -135,12 +132,6 @@ module Puppet
             end
         end
 
-        # Remove any temporary attributes we manage.
-        def flush
-            @metadata = nil
-            @content = nil
-        end
-
         def pinparams
             [:mode, :type, :owner, :group, :content]
         end
@@ -152,24 +143,22 @@ module Puppet
         # Provide, and retrieve if necessary, the metadata for this file.  Fail
         # if we can't find data about this host, and fail if there are any
         # problems in our query.
-        attr_writer :metadata
-        def metadata
-            unless defined?(@metadata) and @metadata
-                return @metadata = nil unless value
-                value.each do |source|
-                    begin
-                        if data = Puppet::FileServing::Metadata.find(source)
-                            @metadata = data
-                            @metadata.source = source
-                            break
-                        end
-                    rescue => detail
-                        fail detail, "Could not retrieve file metadata for %s: %s" % [source, detail]
+        cached_attr(:metadata) do
+            return nil unless value
+            result = nil
+            value.each do |source|
+                begin
+                    if data = Puppet::FileServing::Metadata.find(source)
+                        result = data
+                        result.source = source
+                        break
                     end
+                rescue => detail
+                    fail detail, "Could not retrieve file metadata for %s: %s" % [source, detail]
                 end
-                fail "Could not retrieve information from source(s) %s" % value.join(", ") unless @metadata
             end
-            return @metadata
+            fail "Could not retrieve information from source(s) %s" % value.join(", ") unless result
+            result
         end
 
         # Make sure we're also checking the checksum
diff --git a/lib/puppet/util/cacher.rb b/lib/puppet/util/cacher.rb
index af5c5ebfc..c1b3bad7a 100644
--- a/lib/puppet/util/cacher.rb
+++ b/lib/puppet/util/cacher.rb
@@ -47,6 +47,12 @@ module Puppet::Util::Cacher
             define_method(name) do
                 cached_value(name)
             end
+
+            define_method(name.to_s + "=") do |value|
+                # Make sure the cache timestamp is set
+                cache_timestamp
+                value_cache[name] = value
+            end
         end
     end
 
diff --git a/spec/unit/parameter.rb b/spec/unit/parameter.rb
index 53ea07648..94f5cfd7b 100755
--- a/spec/unit/parameter.rb
+++ b/spec/unit/parameter.rb
@@ -20,6 +20,16 @@ describe Puppet::Parameter do
         @class.value_collection.should be_instance_of(Puppet::Parameter::ValueCollection)
     end
 
+    it "should be able to use cached attributes" do
+        Puppet::Parameter.ancestors.should be_include(Puppet::Util::Cacher)
+    end
+
+    it "should use the resource catalog for expiration" do
+        catalog = mock 'catalog'
+        @resource.stubs(:catalog).returns catalog
+        @parameter.expirer.should equal(catalog)
+    end
+
     describe "when returning the value" do
         it "should return nil if no value is set" do
             @parameter.value.should be_nil
diff --git a/spec/unit/type/file/source.rb b/spec/unit/type/file/source.rb
index 2dcc1a557..6f2bbb8fa 100755
--- a/spec/unit/type/file/source.rb
+++ b/spec/unit/type/file/source.rb
@@ -6,7 +6,7 @@ source = Puppet::Type.type(:file).attrclass(:source)
 describe Puppet::Type.type(:file).attrclass(:source) do
     before do
         # Wow that's a messy interface to the resource.
-        @resource = stub 'resource', :[]= => nil, :property => nil
+        @resource = stub 'resource', :[]= => nil, :property => nil, :catalog => stub("catalog", :expired? => false)
     end
 
     it "should be a subclass of Parameter" do
@@ -91,6 +91,20 @@ describe Puppet::Type.type(:file).attrclass(:source) do
 
             lambda { @source.metadata }.should raise_error(RuntimeError)
         end
+
+        it "should expire the metadata appropriately" do
+            expirer = stub 'expired', :expired? => true
+
+            metadata = stub 'metadata', :source= => nil
+            Puppet::FileServing::Metadata.expects(:find).with("/fee/booz").returns metadata
+
+            @source = source.new(:resource => @resource, :value => ["/fee/booz"])
+            @source.metadata = "foo"
+
+            @source.stubs(:expirer).returns expirer
+
+            @source.metadata.should_not == "foo"
+        end
     end
 
     it "should have a method for setting the desired values on the resource" do
@@ -200,22 +214,6 @@ describe Puppet::Type.type(:file).attrclass(:source) do
         end
     end
 
-    describe "when flushing" do
-        it "should set its metadata to nil" do
-            @source = source.new(:resource => @resource)
-            @source.metadata = "foo"
-            @source.flush
-            @source.instance_variable_get("@metadata").should be_nil
-        end
-
-        it "should reset its content" do
-            @source = source.new(:resource => @resource)
-            @source.instance_variable_set("@content", "foo")
-            @source.flush
-            @source.instance_variable_get("@content").should be_nil
-        end
-    end
-
     it "should have a method for returning the content" do
         source.new(:resource => @resource).must respond_to(:content)
     end
@@ -224,7 +222,7 @@ describe Puppet::Type.type(:file).attrclass(:source) do
         before do
             @source = source.new(:resource => @resource)
             @metadata = stub 'metadata', :source => "/my/source"
-            @source.metadata = @metadata
+            @source.stubs(:metadata).returns @metadata
 
             @content = stub 'content', :content => "foobar"
         end
@@ -250,5 +248,17 @@ describe Puppet::Type.type(:file).attrclass(:source) do
             @source.expects(:fail).raises RuntimeError
             lambda { @source.content }.should raise_error(RuntimeError)
         end
+
+        it "should expire the content appropriately" do
+            expirer = stub 'expired', :expired? => true
+
+            content2 = stub 'content', :content => "secondrun"
+            Puppet::FileServing::Content.expects(:find).with("/my/source").times(2).returns(@content).then.returns(content2)
+            @source.content.should == "foobar"
+
+            @source.stubs(:expirer).returns expirer
+
+            @source.content.should == "secondrun"
+        end
     end
 end
diff --git a/spec/unit/util/cacher.rb b/spec/unit/util/cacher.rb
index 26fe20f84..3e8d31a24 100755
--- a/spec/unit/util/cacher.rb
+++ b/spec/unit/util/cacher.rb
@@ -95,5 +95,15 @@ describe Puppet::Util::Cacher do
             @object.stubs(:expirer).returns nil
             @object.instance_cache.should_not equal(@object.instance_cache)
         end
+
+        it "should allow writing of the attribute" do
+            @object.should respond_to(:instance_cache=)
+        end
+
+        it "should correctly configure timestamps for expiration when the cached attribute is written to" do
+            @object.instance_cache = "foo"
+            @expirer.expire
+            @object.instance_cache.should_not == "foo"
+        end
     end
 end