Refactoring the Cacher interface to always require attribute declaration.

Previously you could dynamically use cached values, but the new interface
requires a single static declaration of the attribute:

    cached_attr(:myattr) { my_init_code() }

This is cleaner, because it makes it easy to turn the code into an init method
and generally makes the whole thing easier to think about.

Most of this commit is going through the different classes that already using the
Caching engine.

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

7 files changed, 119 insertions, 190 deletions

diff --git a/lib/puppet/file_serving/configuration.rb b/lib/puppet/file_serving/configuration.rb
index bceecc30c..907186ac3 100644
--- a/lib/puppet/file_serving/configuration.rb
+++ b/lib/puppet/file_serving/configuration.rb
@@ -10,7 +10,10 @@ require 'puppet/util/cacher'
 class Puppet::FileServing::Configuration
     require 'puppet/file_serving/configuration/parser'
 
-    extend Puppet::Util::Cacher
+    class << self
+        include Puppet::Util::Cacher
+        cached_attr(:configuration) { new() }
+    end
 
     @config_fileuration = nil
 
@@ -18,7 +21,7 @@ class Puppet::FileServing::Configuration
 
     # Create our singleton configuration.
     def self.create
-        attr_cache(:configuration) { new() }
+        configuration
     end
 
     private_class_method  :new
diff --git a/lib/puppet/file_serving/mount.rb b/lib/puppet/file_serving/mount.rb
index c52cedbfb..552cf33f2 100644
--- a/lib/puppet/file_serving/mount.rb
+++ b/lib/puppet/file_serving/mount.rb
@@ -13,16 +13,17 @@ require 'puppet/file_serving/content'
 # or content objects.
 class Puppet::FileServing::Mount < Puppet::Network::AuthStore
     include Puppet::Util::Logging
-    extend Puppet::Util::Cacher
 
-    def self.localmap
-        attr_cache(:localmap) { 
+    class << self
+        include Puppet::Util::Cacher
+
+        cached_attr(:localmap) do
             {   "h" =>  Facter.value("hostname"),
                 "H" => [Facter.value("hostname"),
                         Facter.value("domain")].join("."),
                 "d" =>  Facter.value("domain")
             }
-        }
+        end
     end
 
     attr_reader :name
diff --git a/lib/puppet/indirector/indirection.rb b/lib/puppet/indirector/indirection.rb
index db2772f25..d4f8af7f7 100644
--- a/lib/puppet/indirector/indirection.rb
+++ b/lib/puppet/indirector/indirection.rb
@@ -296,8 +296,6 @@ class Puppet::Indirector::Indirection
         return klass.new
     end
 
-    # Use cached termini.
-    def termini
-        attr_cache(:termini) { Hash.new }
-    end
+    # Cache our terminus instances indefinitely, but make it easy to clean them up.
+    cached_attr(:termini) { Hash.new }
 end
diff --git a/lib/puppet/network/http_pool.rb b/lib/puppet/network/http_pool.rb
index 4b371abe7..ee28a3116 100644
--- a/lib/puppet/network/http_pool.rb
+++ b/lib/puppet/network/http_pool.rb
@@ -6,7 +6,14 @@ module Puppet::Network; end
 
 # Manage Net::HTTP instances for keep-alive.
 module Puppet::Network::HttpPool
-    extend Puppet::Util::Cacher
+    class << self
+        include Puppet::Util::Cacher
+        cached_attr(:ssl_host) { Puppet::SSL::Host.new }
+
+        private
+        
+        cached_attr(:http_cache) { Hash.new }
+    end
 
     # 2008/03/23
     # LAK:WARNING: Enabling this has a high propability of
@@ -17,12 +24,6 @@ module Puppet::Network::HttpPool
         HTTP_KEEP_ALIVE
     end
 
-    # Create an ssl host instance for getting certificate
-    # information.
-    def self.ssl_host
-        attr_cache(:ssl_host) { Puppet::SSL::Host.new }
-    end
-
     # Clear our http cache, closing all connections.
     def self.clear_http_instances
         http_cache.each do |name, connection|
@@ -102,11 +103,4 @@ module Puppet::Network::HttpPool
 
         return http
     end
-
-    private
-    
-    def self.http_cache
-        # Default to an empty hash.
-        attr_cache(:http) { Hash.new }
-    end
 end
diff --git a/lib/puppet/ssl/certificate_authority.rb b/lib/puppet/ssl/certificate_authority.rb
index 6947af11c..08feff0ac 100644
--- a/lib/puppet/ssl/certificate_authority.rb
+++ b/lib/puppet/ssl/certificate_authority.rb
@@ -17,7 +17,11 @@ class Puppet::SSL::CertificateAuthority
 
     require 'puppet/ssl/certificate_authority/interface'
 
-    extend Puppet::Util::Cacher
+    class << self
+        include Puppet::Util::Cacher
+
+        cached_attr(:singleton_instance) { new }
+    end
 
     def self.ca?
         return false unless Puppet[:ca]
@@ -30,7 +34,7 @@ class Puppet::SSL::CertificateAuthority
     def self.instance
         return nil unless ca?
 
-        attr_cache(:instance) { new }
+        singleton_instance
     end
 
     attr_reader :name, :host
diff --git a/lib/puppet/util/cacher.rb b/lib/puppet/util/cacher.rb
index 139aa3659..0406b12f1 100644
--- a/lib/puppet/util/cacher.rb
+++ b/lib/puppet/util/cacher.rb
@@ -1,25 +1,30 @@
 module Puppet::Util::Cacher
-    # It's basically not possible to test that this is set,
-    # but we need to start with a value so that all initial values
-    # start out valid -- that is, everything's valid until the
-    # first call to 'expire'.
-    @timestamp = Time.now
+    module Expirer
+        attr_reader :timestamp
 
-    # Cause all cached values to be considered expired.
-    def self.expire
-        @timestamp = Time.now
-    end
+        # Cause all cached values to be considered expired.
+        def expire
+            @timestamp = Time.now
+        end
 
-    # Is the provided timestamp later than or equal to our global timestamp?
-    # If it is, then the associated value is valid, otherwise it should be flushed.
-    def self.valid?(timestamp)
-        return timestamp >= @timestamp
+        # Is the provided timestamp earlier than our expiration timestamp?
+        # If it is, then the associated value is expired.
+        def expired?(ts)
+            return false unless timestamp
+
+            return timestamp > ts
+        end
     end
 
+    extend Expirer
+
     # Our module has been extended in a class; we can only add the Instance methods,
     # which become *class* methods in the class.
     def self.extended(other)
-        other.extend(InstanceMethods)
+        class << other
+            extend ClassMethods
+            include InstanceMethods
+        end
     end
 
     # Our module has been included in a class, which means the class gets the class methods
@@ -40,72 +45,46 @@ module Puppet::Util::Cacher
             define_method(init_method, &block)
 
             define_method(name) do
-                cacher_caches.value(name) { send(init_method) }
+                cached_value(name)
             end
         end
     end
 
     # Methods that get added to instances.
     module InstanceMethods
-        private
-
-        # Use/define a cached value.  We just use the Cache class to do all
-        # of the thinking.  Note that we're using a single Cache instance
-        # for all of this instance's cached values.
-        def attr_cache(name, &block)
-            cacher_caches.value(name, &block)
-        end
-
-        def cacher_caches
-            unless defined?(@cacher_caches) and @cacher_caches
-                @cacher_caches = Cache.new
-            end
-            @cacher_caches
+        def expire
+            expirer.expire
         end
-    end
-
-    # An internal class that does all of our comparisons and calculations.
-    # This both caches a given value, and determines whether a given cache is
-    # still valid.
-    class Cache
-        attr_accessor :caches, :timestamp
 
-        def clear
-            caches.clear
-            self.timestamp = Time.now
+        def expirer
+            Puppet::Util::Cacher
         end
 
-        def initialize
-            @caches = {}
-            @timestamp = Time.now
-        end
+        private
 
-        # If our timestamp is out of date, our cached data is expired.
-        def expired?
-            ! Puppet::Util::Cacher.valid?(timestamp)
+        def cache_timestamp
+            unless defined?(@cache_timestamp)
+                @cache_timestamp = Time.now
+            end
+            @cache_timestamp
         end
 
-        # Return a value; use the cached version if the associated timestamp is recent enough,
-        # else calculate and store a new a value using the provided block.
-        def value(name)
-            raise ArgumentError, "You must provide a block when using the cache" unless block_given?
-
-            # If the cached data is expired, clear the cache and get a new
-            # value.  Note that if we clear the cache here, we potentially
-            # clear other cached values, too (if this instance is caching more
-            # than one value).
-            if expired?
-                clear
+        def cached_value(name)
+            if expirer.expired?(cache_timestamp)
+                value_cache.clear
+                @cache_timestamp = Time.now
             end
-
-            # Generate a new value if we don't have one.  Use 'include?' here
-            # rather than testing for truth, so we can cache false values.
-            unless caches.include?(name)
-                caches[name] = yield
+            unless value_cache.include?(name)
+                value_cache[name] = send("init_%s" % name)
             end
+            value_cache[name]
+        end
 
-            # Finally, return our cached value.
-            caches[name]
+        def value_cache
+            unless defined?(@value_cache) and @value_cache
+                @value_cache = {}
+            end
+            @value_cache
         end
     end
 end
diff --git a/spec/unit/util/cacher.rb b/spec/unit/util/cacher.rb
index 08c4c14d5..5bdaaaa65 100755
--- a/spec/unit/util/cacher.rb
+++ b/spec/unit/util/cacher.rb
@@ -4,141 +4,91 @@ require File.dirname(__FILE__) + '/../../spec_helper'
 
 require 'puppet/util/cacher'
 
-class CacheClassTest
-    include Puppet::Util::Cacher
-
-    cached_attr(:testing) { Time.now }
-
-    def sa_cache
-        attr_cache(:ca_cache) { Time.now }
-    end
+class ExpirerTest
+    include Puppet::Util::Cacher::Expirer
 end
 
-class CacheInstanceTest
-    extend Puppet::Util::Cacher
+class CacheTest
+    @@init_count = 0
 
-    def self.sa_cache
-        attr_cache(:ca_cache) { Time.now }
-    end
+    include Puppet::Util::Cacher
+    cached_attr(:instance_cache) { Time.now }
 end
 
-describe "a cacher user using cached values", :shared => true do
-    it "should use the block to generate a new value if none is present" do
-        now = Time.now
-        Time.stubs(:now).returns now
-        @object.sa_cache.should equal(now)
+describe Puppet::Util::Cacher::Expirer do
+    before do
+        @expirer = ExpirerTest.new
     end
 
-    it "should not consider cached false values to be missing values" do
-        Puppet::Util::Cacher.stubs(:valid?).returns true
-
-        # This is only necessary in the class, since it has this value kicking
-        # around.
-        @object.instance_variable_set("@cacher_caches", nil)
-        Time.stubs(:now).returns false
-        @object.sa_cache
-        @object.sa_cache.should be_false
+    it "should be able to test whether a timestamp is expired" do
+        @expirer.should respond_to(:expired?)
     end
 
-    it "should return cached values if they are still valid" do
-        Puppet::Util::Cacher.stubs(:valid?).returns true
-
-        @object.sa_cache.should equal(@object.sa_cache)
+    it "should be able to expire all values" do
+        @expirer.should respond_to(:expire)
     end
 
-    it "should use the block to generate new values if the cached values are expired" do
-        Puppet::Util::Cacher.stubs(:valid?).returns false
-
-        @object.sa_cache.should_not equal(@object.sa_cache)
+    it "should consider any value to be valid if it has never been expired" do
+        @expirer.should_not be_expired(Time.now)
     end
 
-    it "should still cache values after an expiration" do
-        # Load the cache
-        @object.sa_cache
-
-        Puppet::Util::Cacher.expire
-        @object.sa_cache.should equal(@object.sa_cache)
+    it "should consider any value created after expiration to be expired" do
+        @expirer.expire
+        @expirer.should be_expired(Time.now - 1)
     end
 end
 
 describe Puppet::Util::Cacher do
-    before do
-        Puppet::Util::Cacher.expire
-    end
-    after do
-        Puppet::Util::Cacher.expire
+    it "should be extended with the Expirer module" do
+        Puppet::Util::Cacher.metaclass.ancestors.should be_include(Puppet::Util::Cacher::Expirer)
     end
 
-    it "should have a method for expiring caches" do
-        Puppet::Util::Cacher.should respond_to(:expire)
+    it "should support defining cached attributes" do
+        CacheTest.private_methods.should be_include("cached_attr")
     end
 
-    it "should have a method for determining whether a cached value is valid" do
-        Puppet::Util::Cacher.should respond_to(:valid?)
+    it "should default to the Cacher module as its expirer" do
+        CacheTest.new.expirer.should equal(Puppet::Util::Cacher)
     end
 
-    it "should consider cached values valid if the cached value was created since the last expiration" do
-        Puppet::Util::Cacher.expire
-
-        Puppet::Util::Cacher.should be_valid(Time.now + 1)
-    end
-
-    it "should consider cached values expired if the cache was expired after the cached value was created" do 
-        Puppet::Util::Cacher.expire
-
-        Puppet::Util::Cacher.should_not be_valid(Time.now - 1)
-    end
-
-    describe "when used to extend a class" do
+    describe "when defining cached attributes" do
         before do
-            @object = CacheClassTest.new
-        end
-
-        it_should_behave_like "a cacher user using cached values"
+            @expirer = ExpirerTest.new
+            @object = CacheTest.new
 
-        it "should provide a class method for defining cached attributes" do
-            CacheClassTest.private_methods.should be_include("cached_attr")
+            @object.stubs(:expirer).returns @expirer
         end
 
-        describe "and defining cached attributes" do
-            it "should create an accessor for the cached attribute" do
-                @object.should respond_to(:testing)
-            end
-
-            it "should return a value calculated from the provided block" do
-                time = Time.now
-                Time.stubs(:now).returns time
-                @object.testing.should equal(time)
-            end
-
-            it "should return the cached value from the getter if the value is still valid" do
-                value = @object.testing
-                Puppet::Util::Cacher.expects(:valid?).returns true
-                @object.testing.should equal(value)
-            end
-
-            it "should regenerate and return a new value using the provided block if the value is no longer valid" do
-                value = @object.testing
-                Puppet::Util::Cacher.expects(:valid?).returns false
-                @object.testing.should_not equal(value)
-            end
+        it "should create a getter for the cached attribute" do
+            @object.should respond_to(:instance_cache)
         end
 
-        it "should provide a private instance method for caching values" do
-            @object.private_methods.should be_include("attr_cache")
+        it "should return a value calculated from the provided block" do
+            time = Time.now
+            Time.stubs(:now).returns time
+            @object.instance_cache.should equal(time)
         end
 
-    end
+        it "should return the cached value from the getter every time if the value is not expired" do
+            @object.instance_cache.should equal(@object.instance_cache)
+        end
 
-    describe "when included in a class" do
-        before do
-            @object = CacheInstanceTest
+        it "should regenerate and return a new value using the provided block if the value has been expired" do
+            value = @object.instance_cache
+            @expirer.expire
+            @object.instance_cache.should_not equal(value)
         end
 
-        it "should provide a private instance method for caching values" do
-            CacheInstanceTest.private_methods.should be_include("attr_cache")
+        it "should be able to cache false values" do
+            @object.expects(:init_instance_cache).returns false
+            @object.instance_cache.should be_false
+            @object.instance_cache.should be_false
         end
 
-        it_should_behave_like "a cacher user using cached values"
+        it "should cache values again after expiration" do
+            @object.instance_cache
+            @expirer.expire
+            @object.instance_cache.should equal(@object.instance_cache)
+        end
     end
 end