Merge pull request #2204 from IrimieBogdan/FACT-2881

(FACT-2881) Exclude nil custom facts from Facter API

lib/facter.rb

@@ -312,6 +312,7 @@ def to_hash
       log_blocked_facts
 
       resolved_facts = Facter::FactManager.instance.resolve_facts
+      resolved_facts.reject! { |fact| fact.type == :custom && fact.value.nil? }
       Facter::FactCollection.new.build_fact_collection!(resolved_facts)
     end
 
@@ -361,6 +362,7 @@ def values(options, user_queries)
       Options[:show_legacy] = true
       log_blocked_facts
       resolved_facts = Facter::FactManager.instance.resolve_facts(user_queries)
+      resolved_facts.reject! { |fact| fact.type == :custom && fact.value.nil? }
 
       if user_queries.count.zero?
         Facter::FactCollection.new.build_fact_collection!(resolved_facts)
@@ -387,9 +389,8 @@ def to_user_output(cli_options, *args)
       init_cli_options(cli_options, args)
       logger.info("executed with command line: #{ARGV.drop(1).join(' ')}")
       log_blocked_facts
-      resolved_facts = Facter::FactManager.instance.resolve_facts(args)
+      resolved_facts = resolve_facts_for_user_query(args)
       fact_formatter = Facter::FormatterFactory.build(Facter::Options.get)
-
       status = error_check(resolved_facts)
 
       [fact_formatter.format(resolved_facts), status]
@@ -446,6 +447,14 @@ def warnonce(message)
 
     private
 
+    def resolve_facts_for_user_query(user_query)
+      resolved_facts = Facter::FactManager.instance.resolve_facts(user_query)
+      user_queries = resolved_facts.uniq(&:user_query).map(&:user_query)
+      resolved_facts.reject! { |fact| fact.type == :custom && fact.value.nil? } if user_queries.first.empty?
+
+      resolved_facts
+    end
+
     def parse_exception(exception, error_message)
       if exception.is_a?(Exception)
         error_message << exception.message if error_message.empty?

lib/facter/models/fact_collection.rb

@@ -9,7 +9,7 @@ def initialize
 
     def build_fact_collection!(facts)
       facts.each do |fact|
-        next if %i[custom core legacy].include?(fact.type) && fact.value.nil?
+        next if %i[core legacy].include?(fact.type) && fact.value.nil?
 
         bury_fact(fact)
       end

spec/facter/facter_spec.rb

@@ -2,11 +2,12 @@
 
 describe Facter do
   let(:fact_name) { 'os.name' }
+  let(:fact_user_query) { 'os.name' }
   let(:fact_value) { 'ubuntu' }
   let(:type) { :core }
   let(:os_fact) do
     instance_spy(Facter::ResolvedFact, name: fact_name, value: fact_value,
-                                       user_query: fact_name, filter_tokens: [], type: type)
+                                       user_query: fact_user_query, filter_tokens: [], type: type)
   end
   let(:missing_fact) do
     instance_spy(Facter::ResolvedFact, name: 'missing_fact', value: nil,
@@ -200,9 +201,25 @@ def mock_collection(method, os_name = nil, error = nil)
 
       expect(Facter.to_hash).to eq(empty_fact_collection)
     end
+
+    context 'when custom fact with nil value' do
+      let(:type) { :custom }
+      let(:fact_value) { nil }
+      let(:fact_user_query) { '' }
+
+      it 'discards the custom fact with nil value' do
+        mock_fact_manager(:resolve_facts, [os_fact])
+
+        Facter.to_hash
+
+        expect(fact_collection_spy).to have_received(:build_fact_collection!).with([])
+      end
+    end
   end
 
   describe '#to_user_output' do
+    let(:json_fact_formatter) { instance_spy(Facter::JsonFactFormatter) }
+
     before do |example|
       resolved_fact = example.metadata[:multiple_facts] ? [os_fact, missing_fact] : [os_fact]
       expected_json_output = if example.metadata[:multiple_facts]
@@ -212,7 +229,6 @@ def mock_collection(method, os_name = nil, error = nil)
                              end
 
       allow(fact_manager_spy).to receive(:resolve_facts).and_return(resolved_fact)
-      json_fact_formatter = instance_spy(Facter::JsonFactFormatter)
       allow(json_fact_formatter).to receive(:format).with(resolved_fact).and_return(expected_json_output)
       allow(Facter::FormatterFactory).to receive(:build).and_return(json_fact_formatter)
     end
@@ -258,6 +274,20 @@ def mock_collection(method, os_name = nil, error = nil)
         expect(formated_facts).to eq([expected_json_output, 0])
       end
     end
+
+    context 'when custom fact with nil value' do
+      let(:type) { :custom }
+      let(:fact_value) { nil }
+      let(:fact_user_query) { '' }
+
+      it 'discards the custom fact with nil value' do
+        user_query = ['os.name']
+
+        Facter.to_user_output({}, *user_query)
+
+        expect(json_fact_formatter).to have_received(:format).with([])
+      end
+    end
   end
 
   describe '#value' do
@@ -274,6 +304,19 @@ def mock_collection(method, os_name = nil, error = nil)
 
       expect(Facter.value('os.name')).to be nil
     end
+
+    context 'when custom fact with nill value' do
+      let(:type) { :custom }
+      let(:fact_value) { nil }
+      let(:fact_user_query) { '' }
+
+      it 'returns the custom fact' do
+        mock_fact_manager(:resolve_facts, [os_fact])
+        mock_collection(:value, 'Ubuntu')
+
+        expect(Facter.value('os.name')).to eq('Ubuntu')
+      end
+    end
   end
 
   describe '#values' do
@@ -331,6 +374,22 @@ def mock_collection(method, os_name = nil, error = nil)
         expect(Facter.values({}, [])).to eq(fact_collection_spy)
       end
     end
+
+    context 'when custom fact with nill value' do
+      let(:type) { :custom }
+      let(:fact_value) { nil }
+      let(:fact_user_query) { '' }
+
+      before do
+        mock_fact_manager(:resolve_facts, [os_fact])
+      end
+
+      it 'calls Facter::FactCollection with no facts' do
+        Facter.values({}, [])
+
+        expect(fact_collection_spy).to have_received(:build_fact_collection!).with([])
+      end
+    end
   end
 
   describe '#fact' do
@@ -371,6 +430,21 @@ def mock_collection(method, os_name = nil, error = nil)
         expect(Facter.fact('missing_fact')).to be_nil
       end
     end
+
+    context 'when custom fact with nill value' do
+      let(:type) { :custom }
+      let(:fact_value) { nil }
+      let(:fact_user_query) { '' }
+
+      it 'returns the custom fact' do
+        mock_fact_manager(:resolve_facts, [os_fact])
+        mock_collection(:value, 'Ubuntu')
+
+        expect(Facter.fact('os.name'))
+          .to be_instance_of(Facter::ResolvedFact)
+          .and have_attributes(value: 'Ubuntu')
+      end
+    end
   end
 
   describe '#[]' do
@@ -387,6 +461,21 @@ def mock_collection(method, os_name = nil, error = nil)
 
       expect(Facter['os.name']).to be_nil
     end
+
+    context 'when custom fact with nill value' do
+      let(:type) { :custom }
+      let(:fact_value) { nil }
+      let(:fact_user_query) { '' }
+
+      it 'returns the custom fact' do
+        mock_fact_manager(:resolve_facts, [os_fact])
+        mock_collection(:value, 'Ubuntu')
+
+        expect(Facter['os.name'])
+          .to be_instance_of(Facter::ResolvedFact)
+          .and have_attributes(value: 'Ubuntu')
+      end
+    end
   end
 
   describe '#core_value' do

spec/facter/model/fact_collection_spec.rb

@@ -55,19 +55,6 @@
         end
       end
 
-      context 'when fact type is :custom' do
-        let(:fact_name) { 'operatingsystem' }
-        let(:fact_value) { nil }
-        let(:type) { :custom }
-
-        it 'does not add fact to collection' do
-          fact_collection.build_fact_collection!([resolved_fact])
-          expected_hash = {}
-
-          expect(fact_collection).to eq(expected_hash)
-        end
-      end
-
       context 'when fact type is :legacy' do
         context 'when fact name contains dots' do
           let(:fact_name) { 'my.dotted.external.fact.name' }