Merge pull request puppetlabs#3214 from joshcooper/ticket/master/PUP-…

…3130-remove-hidden-timestamp

(PUP-3130) Remove hidden `_timestamp` fact

lib/puppet/node/facts.rb

@@ -21,7 +21,7 @@ def save(instance, key = nil, options={})
 
   indirects :facts, :terminus_setting => :facts_terminus, :extend => NodeExpirer
 
-  attr_accessor :name, :values
+  attr_accessor :name, :values, :timestamp
 
   def add_local_facts
     values["clientcert"] = Puppet.settings[:certname]
@@ -39,16 +39,16 @@ def initialize(name, values = {})
   def initialize_from_hash(data)
     @name = data['name']
     @values = data['values']
-    # Timestamp will be here in YAML
-    timestamp = data['values']['_timestamp']
-    @values.delete_if do |key, val|
-      key =~ /^_/
-    end
-
-    #Timestamp will be here in pson
+    # Timestamp will be here in YAML, e.g. when reading old reports
+    timestamp = @values.delete('_timestamp')
+    # Timestamp will be here in JSON
     timestamp ||= data['timestamp']
-    timestamp = Time.parse(timestamp) if timestamp.is_a? String
-    self.timestamp = timestamp
+
+    if timestamp.is_a? String
+      @timestamp = Time.parse(timestamp)
+    else
+      @timestamp = timestamp
+    end
 
     self.expiration = data['expiration']
     if expiration.is_a? String
@@ -66,7 +66,7 @@ def sanitize
 
   def ==(other)
     return false unless self.name == other.name
-    strip_internal == other.send(:strip_internal)
+    values == other.values
   end
 
   def self.from_data_hash(data)
@@ -83,14 +83,14 @@ def self.from_pson(data)
   def to_data_hash
     result = {
       'name' => name,
-      'values' => strip_internal,
+      'values' => values
     }
 
-    if timestamp
-      if timestamp.is_a? Time
-        result['timestamp'] = timestamp.iso8601(9)
+    if @timestamp
+      if @timestamp.is_a? Time
+        result['timestamp'] = @timestamp.iso8601(9)
       else
-        result['timestamp'] = timestamp
+        result['timestamp'] = @timestamp
       end
     end
 
@@ -105,24 +105,13 @@ def to_data_hash
     result
   end
 
-  # Add internal data to the facts for storage.
   def add_timestamp
-    self.timestamp = Time.now
-  end
-
-  def timestamp=(time)
-    self.values['_timestamp'] = time
-  end
-
-  def timestamp
-    self.values['_timestamp']
+    @timestamp = Time.now
   end
 
-  # Strip out that internal data.
+  # @deprecated Use {#values} instead of this method.
   def strip_internal
-    newvals = values.dup
-    newvals.find_all { |name, value| name.to_s =~ /^_/ }.each { |name, value| newvals.delete(name) }
-    newvals
+    values
   end
 
   private

lib/puppet/parser/scope.rb

@@ -655,10 +655,6 @@ def set_trusted(hash)
   end
 
   def set_facts(hash)
-    # Remove _timestamp (it has an illegal datatype). It is not allowed to mutate the given hash
-    # since it contains the facts.
-    hash = hash.dup
-    hash.delete('_timestamp')
     setvar('facts', deep_freeze(hash), :privileged => true)
   end
 

spec/unit/node/facts_spec.rb

@@ -112,9 +112,51 @@
   end
 
   describe "when storing and retrieving" do
-    it "should add metadata to the facts" do
-      facts = Puppet::Node::Facts.new("me", "one" => "two", "three" => "four")
-      facts.values['_timestamp'].should be_instance_of(Time)
+    it "doesn't manufacture a `_timestamp` fact value" do
+      values = {"one" => "two", "three" => "four"}
+      facts = Puppet::Node::Facts.new("mynode", values)
+
+      expect(facts.values).to eq(values)
+    end
+
+    describe "when deserializing from yaml" do
+      let(:timestamp)  { Time.parse("Thu Oct 28 11:16:31 -0700 2010") }
+      let(:expiration) { Time.parse("Thu Oct 28 11:21:31 -0700 2010") }
+
+      def create_facts(values = {})
+        Puppet::Node::Facts.new('mynode', values)
+      end
+
+      def deserialize_yaml_facts(facts)
+        format = Puppet::Network::FormatHandler.format('yaml')
+        format.intern(Puppet::Node::Facts, facts.to_yaml)
+      end
+
+      it 'preserves `_timestamp` value' do
+        facts = deserialize_yaml_facts(create_facts('_timestamp' => timestamp))
+
+        expect(facts.timestamp).to eq(timestamp)
+      end
+
+      it "doesn't preserve the `_timestamp` fact" do
+        facts = deserialize_yaml_facts(create_facts('_timestamp' => timestamp))
+
+        expect(facts.values['_timestamp']).to be_nil
+      end
+
+      it 'preserves expiration time if present' do
+        old_facts = create_facts
+        old_facts.expiration = expiration
+        facts = deserialize_yaml_facts(old_facts)
+
+        expect(facts.expiration).to eq(expiration)
+      end
+
+      it 'ignores expiration time if absent' do
+        facts = deserialize_yaml_facts(create_facts)
+
+        expect(facts.expiration).to be_nil
+      end
     end
 
     describe "using pson" do
@@ -129,7 +171,8 @@
         facts = format.intern(Puppet::Node::Facts,pson)
         facts.name.should == 'foo'
         facts.expiration.should == @expiration
-        facts.values.should == {'a' => '1', 'b' => '2', 'c' => '3', '_timestamp' => @timestamp}
+        facts.timestamp.should == @timestamp
+        facts.values.should == {'a' => '1', 'b' => '2', 'c' => '3'}
       end
 
       it "should generate properly formatted pson" do
@@ -138,7 +181,7 @@
         facts.expiration = @expiration
         result = PSON.parse(facts.to_pson)
         result['name'].should == facts.name
-        result['values'].should == facts.values.reject { |key, value| key.to_s =~ /_/ }
+        result['values'].should == facts.values
         result['timestamp'].should == facts.timestamp.iso8601(9)
         result['expiration'].should == facts.expiration.iso8601(9)
       end