Fixed #1595 - Internally, Property#retrieve is no longer called

when no 'should' value is available for a resource.

5 files changed, 57 insertions, 23 deletions

diff --git a/CHANGELOG b/CHANGELOG
index cbbc2573f..2b61921d4 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,4 +1,7 @@
 0.24.x
+    Fixed #1595 - Internally, Property#retrieve is no longer called
+    when no 'should' value is available for a resource.
+
     Fixed #1584 - Added support for appended variables
 
     Fixed #1554 - Added support for multiple template directories
diff --git a/lib/puppet/parameter.rb b/lib/puppet/parameter.rb
index 31e009af5..ef7f1c1ad 100644
--- a/lib/puppet/parameter.rb
+++ b/lib/puppet/parameter.rb
@@ -413,29 +413,7 @@ class Puppet::Parameter
         @shadow = nil
     end
 
-    # This should only be called for parameters, but go ahead and make
-    # it possible to call for properties, too.
-    def value
-        if self.is_a?(Puppet::Property)
-            # We should return the 'is' value if there's not 'should'
-            # value.  This might be bad, though, because the 'should'
-            # method knows whether to return an array or not and that info
-            # is not exposed, and the 'is' value could be a symbol.  I
-            # can't seem to create a test in which this is a problem, but
-            # that doesn't mean it's not one.
-            if self.should
-                return self.should
-            else
-                return self.retrieve
-            end
-        else
-            if defined? @value
-                return @value
-            else
-                return nil
-            end
-        end
-    end
+    attr_reader :value
 
     # Store the value provided.  All of the checking should possibly be
     # late-binding (e.g., users might not exist when the value is assigned
diff --git a/lib/puppet/property.rb b/lib/puppet/property.rb
index 9e8bae7a4..913f43977 100644
--- a/lib/puppet/property.rb
+++ b/lib/puppet/property.rb
@@ -441,6 +441,11 @@ class Property < Puppet::Parameter
         return "%s(%s)" % [@resource.name,self.name]
     end
 
+    # Just return any should value we might have.
+    def value
+        self.should
+    end
+
     # Provide a common hook for setting @should, just like params.
     def value=(value)
         self.should = value
diff --git a/spec/unit/parameter.rb b/spec/unit/parameter.rb
new file mode 100755
index 000000000..d6858c29d
--- /dev/null
+++ b/spec/unit/parameter.rb
@@ -0,0 +1,24 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../spec_helper'
+
+require 'puppet/parameter'
+
+describe Puppet::Parameter do
+    describe "when returning the value" do
+        before do
+            @class = Class.new(Puppet::Parameter)
+            @class.initvars
+            @parameter = @class.new :resource => mock('resource')
+        end
+
+        it "should return nil if no value is set" do
+            @parameter.value.should be_nil
+        end
+
+        it "should return any set value" do
+            @parameter.value = "foo"
+            @parameter.value.should == "foo"
+        end
+    end
+end
diff --git a/spec/unit/property.rb b/spec/unit/property.rb
new file mode 100755
index 000000000..a562f8bb4
--- /dev/null
+++ b/spec/unit/property.rb
@@ -0,0 +1,24 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../spec_helper'
+
+require 'puppet/property'
+
+describe Puppet::Property do
+    describe "when returning the value" do
+        before do
+            @class = Class.new(Puppet::Property)
+            @class.initvars
+            @property = @class.new :resource => mock('resource')
+        end
+
+        it "should return nil if no value is set" do
+            @property.value.should be_nil
+        end
+
+        it "should return any set 'should' value" do
+            @property.should = "foo"
+            @property.value.should == "foo"
+        end
+    end
+end