From 348af0f8b93abd9642d5bf0c5bf86edc23600ca0 Mon Sep 17 00:00:00 2001
From: oanatmaria <49147761+oanatmaria@users.noreply.github.com>
Date: Wed, 3 Jun 2020 17:18:01 +0300
Subject: [PATCH 1/2] (maint) Test commit message action (#542)

---
 .github/workflows/checks.yaml | 2 ++
 tasks/commits.rake            | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/.github/workflows/checks.yaml b/.github/workflows/checks.yaml
index 6feef8c27..e53d21ac6 100644
--- a/.github/workflows/checks.yaml
+++ b/.github/workflows/checks.yaml
@@ -37,6 +37,8 @@ jobs:
     steps:
       - name: Checkout current PR
         uses: actions/checkout@v2
+        with:
+          fetch-depth: 2
 
       - name: Commit message checks
         uses: ruby/setup-ruby@v1
diff --git a/tasks/commits.rake b/tasks/commits.rake
index 2192b2e20..7c53faa5e 100644
--- a/tasks/commits.rake
+++ b/tasks/commits.rake
@@ -8,7 +8,7 @@ task(:commits) do
   # populated with the range of commits the PR contains. If not available, this
   # falls back to `master..HEAD` as a next best bet as `master` is unlikely to
   # ever be absent.
-  commit_range = ENV['TRAVIS_COMMIT_RANGE'].nil? ? 'master..HEAD' : ENV['TRAVIS_COMMIT_RANGE'].sub(/\.\.\./, '..')
+  commit_range = 'HEAD^..HEAD'
   puts "Checking commits #{commit_range}"
   `git log --no-merges --pretty=%s #{commit_range}`.each_line do |commit_summary|
     # This regex tests for the currently supported commit summary tokens: maint, doc, gem, or fact-<number>.

From 9ccba540d6a32d9d17c6b62d88e42d3f0d9731ca Mon Sep 17 00:00:00 2001
From: Bogdan Irimie <bogdan.irimie@puppet.com>
Date: Wed, 3 Jun 2020 17:50:02 +0300
Subject: [PATCH 2/2] (FACT-2635) Incorrect output for non existing fact (#536)

* (FACT-2635) Add SearchFact and ResolvedFact with nil type.

* (FACT-2635) Move nil fact resolution to internal fact manager. Fix tests.

* (FACT-2634) Add test for nil fact.

* (FACT-2635) Rubocop fixes.

* (FACT-2635) Remove unused arguments from  error_check.

* (FACT-2635) Fix formatter for keys that contain ":" in their name.

* (FACT-2635) Remove commented code.

* (FACT-2635) Remove filter_nil_facts method as it was redundant.

* (FACT-2635) Reject nil facts when resolving facts fro API calls.

* (FACT-2635) Add test for nil fact in query parser.

* (FACT0-2635) Add unit tests for rejection of nil facts in `[]` and `value` API methods.

* (FACT-2635) Fix typo.

* (FACT-2635) Make 'merge' a valid word for github action

Co-authored-by: Oana Tanasoiu <oana.tanasoiu@puppet.com>
---
 .rubocop_todo.yml                             |  2 -
 lib/facter.rb                                 | 10 ++-
 .../fact/internal/internal_fact_manager.rb    | 17 ++++-
 .../formatters/legacy_fact_formatter.rb       |  6 +-
 lib/framework/parsers/query_parser.rb         |  6 +-
 spec/facter/facter_spec.rb                    | 73 +++++++++++++------
 spec/facter/query_parser_spec.rb              | 12 +++
 .../internal/internal_fact_manager_spec.rb    | 18 +++++
 tasks/commits.rake                            |  2 +-
 9 files changed, 113 insertions(+), 33 deletions(-)

diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index efbc96bdb..d0f27d646 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -129,7 +129,6 @@ RSpec/MessageSpies:
     - 'spec/custom_facts/util/loader_spec.rb'
     - 'spec/custom_facts/util/parser_spec.rb'
     - 'spec/custom_facts/util/resolution_spec.rb'
-    - 'spec/facter/facter_spec.rb'
     - 'spec/facter/facts/aix/os/name_spec.rb'
     - 'spec/facter/facts/aix/os/release_spec.rb'
     - 'spec/facter/facts/macosx/is_virtual_spec.rb'
@@ -188,7 +187,6 @@ RSpec/VerifiedDoubles:
     - 'spec/custom_facts/util/directory_loader_spec.rb'
     - 'spec/custom_facts/util/fact_spec.rb'
     - 'spec/custom_facts/util/resolution_spec.rb'
-    - 'spec/facter/facter_spec.rb'
     - 'spec/facter/facts/aix/ssh_spec.rb'
     - 'spec/facter/facts/macosx/memory/swap/capacity_spec.rb'
     - 'spec/facter/facts/macosx/memory/swap/used_bytes_spec.rb'
diff --git a/lib/facter.rb b/lib/facter.rb
index b92503022..61ea55369 100644
--- a/lib/facter.rb
+++ b/lib/facter.rb
@@ -245,7 +245,7 @@ def to_user_output(cli_options, *args)
       SessionCache.invalidate_all_caches
       fact_formatter = Facter::FormatterFactory.build(Facter::Options.get)
 
-      status = error_check(args, resolved_facts)
+      status = error_check(resolved_facts)
 
       [fact_formatter.format(resolved_facts), status || 0]
     end
@@ -289,6 +289,9 @@ def resolve_fact(user_query)
       user_query = user_query.to_s
       resolved_facts = Facter::FactManager.instance.resolve_facts([user_query])
       SessionCache.invalidate_all_caches
+      # we must make a distinction between custom facts that return nil and nil facts
+      # Nil facts should not be packaged as ResolvedFacts! (add_fact_to_searched_facts packages facts)
+      resolved_facts = resolved_facts.reject { |fact| fact.type == :nil }
       fact_collection = FactCollection.new.build_fact_collection!(resolved_facts)
       splitted_user_query = Facter::Utils.split_user_query(user_query)
 
@@ -310,9 +313,10 @@ def resolve_fact(user_query)
     #  facts that are not found or resolved, otherwise it will return nil
     #
     # @api private
-    def error_check(args, resolved_facts)
+    def error_check(resolved_facts)
       if Options[:strict]
-        missing_names = args - resolved_facts.map(&:user_query).uniq
+        missing_names = resolved_facts.select { |fact| fact.type == :nil }.map(&:user_query)
+
         if missing_names.count.positive?
           status = 1
           log_errors(missing_names)
diff --git a/lib/framework/core/fact/internal/internal_fact_manager.rb b/lib/framework/core/fact/internal/internal_fact_manager.rb
index 55d70ce47..cf249d799 100644
--- a/lib/framework/core/fact/internal/internal_fact_manager.rb
+++ b/lib/framework/core/fact/internal/internal_fact_manager.rb
@@ -5,11 +5,13 @@ class InternalFactManager
     @@log = Facter::Log.new(self)
 
     def resolve_facts(searched_facts)
-      searched_facts = filter_internal_facts(searched_facts)
+      internal_searched_facts = filter_internal_facts(searched_facts)
+      threads = start_threads(internal_searched_facts)
+      resolved_facts = join_threads(threads, internal_searched_facts)
 
-      threads = start_threads(searched_facts)
+      nil_resolved_facts = resolve_nil_facts(searched_facts)
 
-      join_threads(threads, searched_facts)
+      resolved_facts.concat(nil_resolved_facts)
     end
 
     private
@@ -18,6 +20,15 @@ def filter_internal_facts(searched_facts)
       searched_facts.select { |searched_fact| %i[core legacy].include? searched_fact.type }
     end
 
+    def resolve_nil_facts(searched_facts)
+      resolved_facts = []
+      searched_facts.select { |fact| fact.type == :nil }.each do |fact|
+        resolved_facts << ResolvedFact.new(fact.name, nil, :nil, fact.name)
+      end
+
+      resolved_facts
+    end
+
     def start_threads(searched_facts)
       threads = []
       # only resolve a fact once, even if multiple search facts depend on that fact
diff --git a/lib/framework/formatters/legacy_fact_formatter.rb b/lib/framework/formatters/legacy_fact_formatter.rb
index d61f6a62a..8a06fc9c8 100644
--- a/lib/framework/formatters/legacy_fact_formatter.rb
+++ b/lib/framework/formatters/legacy_fact_formatter.rb
@@ -60,7 +60,7 @@ def hash_to_facter_format(facts_hash)
       pretty_json = JSON.pretty_generate(facts_hash)
 
       @log.debug('Change key value delimiter from : to =>')
-      pretty_json.gsub!(/^(.*?)(:)/, '\1 =>')
+      pretty_json.gsub!(/":/, '" =>')
 
       @log.debug('Remove quotes from parent nodes')
       pretty_json.gsub!(/\"(.*)\"\ =>/, '\1 =>')
@@ -84,8 +84,10 @@ def remove_enclosing_accolades(pretty_fact_json)
     end
 
     def remove_comma_and_quation(output)
+      # quotation marks that come after \ are not removed
       @log.debug('Remove unnecessary comma and quotation marks on root facts')
-      output.split("\n").map! { |line| line =~ /^[\s]+/ ? line : line.gsub(/,$|\"/, '') }.join("\n")
+      output.split("\n")
+            .map! { |line| line =~ /^[\s]+/ ? line : line.gsub(/,$|(?<!\\)\"/, '').gsub('\\"', '"') }.join("\n")
     end
   end
 end
diff --git a/lib/framework/parsers/query_parser.rb b/lib/framework/parsers/query_parser.rb
index c4c53de79..ed925b6b9 100644
--- a/lib/framework/parsers/query_parser.rb
+++ b/lib/framework/parsers/query_parser.rb
@@ -50,6 +50,8 @@ def search_for_facts(query, loaded_fact_hash)
           return resolvable_fact_list if resolvable_fact_list.any?
         end
 
+        resolvable_fact_list << SearchedFact.new(query, nil, [], query, :nil) if resolvable_fact_list.empty?
+
         resolvable_fact_list
       end
 
@@ -73,9 +75,11 @@ def get_facts_matching_tokens(query_tokens, query_token_range, loaded_fact_hash)
       def found_fact?(fact_name, query_fact)
         fact_with_wildcard = fact_name.include?('.*') && !query_fact.include?('.')
 
+        processed_equery_fact = query_fact.gsub('\\', '\\\\\\\\')
+
         return false if fact_with_wildcard && !query_fact.match("^#{fact_name}$")
 
-        return false unless fact_with_wildcard || fact_name.match("^#{query_fact}($|\\.)")
+        return false unless fact_with_wildcard || fact_name.match("^#{processed_equery_fact}($|\\.)")
 
         true
       end
diff --git a/spec/facter/facter_spec.rb b/spec/facter/facter_spec.rb
index 0151fbd2f..4759bd49d 100644
--- a/spec/facter/facter_spec.rb
+++ b/spec/facter/facter_spec.rb
@@ -3,15 +3,21 @@
 describe Facter do
   let(:fact_name) { 'os.name' }
   let(:fact_value) { 'ubuntu' }
+  let(:type) { :core }
   let(:os_fact) do
-    double(Facter::ResolvedFact, name: fact_name, value: fact_value, user_query: fact_name, filter_tokens: [])
+    instance_spy(Facter::ResolvedFact, name: fact_name, value: fact_value,
+                                       user_query: fact_name, filter_tokens: [], type: type)
+  end
+  let(:missing_fact) do
+    instance_spy(Facter::ResolvedFact, name: 'missing_fact', value: nil,
+                                       user_query: 'missing_fact', filter_tokens: [], type: :nil)
   end
   let(:empty_fact_collection) { Facter::FactCollection.new }
   let(:logger) { instance_spy(Facter::Log) }
   let(:fact_manager_spy) { instance_spy(Facter::FactManager) }
   let(:fact_collection_spy) { instance_spy(Facter::FactCollection) }
   let(:key_error) { KeyError.new('key error') }
-  let(:config_reader_double) { double(Facter::ConfigReader) }
+  let(:config_reader_double) { class_spy(Facter::ConfigReader) }
 
   before do
     allow(Facter::ConfigReader).to receive(:init).and_return(config_reader_double)
@@ -68,37 +74,41 @@ def mock_collection(method, os_name = nil, error = nil)
 
   describe '#to_user_output' do
     before do |example|
-      resolved_fact = example.metadata[:resolved_fact] ? [os_fact] : []
-      expected_json_output = example.metadata[:resolved_fact] ? '{"os" : {"name": "ubuntu"}' : '{}'
+      resolved_fact = example.metadata[:multiple_facts] ? [os_fact, missing_fact] : [os_fact]
+      expected_json_output = if example.metadata[:multiple_facts]
+                               '{"os" : {"name": "ubuntu"}, "missing_fact": null}'
+                             else
+                               '{"os" : {"name": "ubuntu"}}'
+                             end
 
       allow(fact_manager_spy).to receive(:resolve_facts).and_return(resolved_fact)
-      json_fact_formatter = double(Facter::JsonFactFormatter)
+      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
 
-    it 'returns one fact and status 0', resolved_fact: true do
-      user_query = 'os.name'
-      expected_json_output = '{"os" : {"name": "ubuntu"}'
+    it 'returns one fact with value and status 0', multiple_facts: false do
+      user_query = ['os.name']
+      expected_json_output = '{"os" : {"name": "ubuntu"}}'
 
-      formated_facts = Facter.to_user_output({}, [user_query])
+      formatted_facts = Facter.to_user_output({}, [user_query])
 
-      expect(formated_facts).to eq([expected_json_output, 0])
+      expect(formatted_facts).to eq([expected_json_output, 0])
     end
 
-    it 'returns no facts and status 0', resolved_fact: false do
-      user_query = 'os.name'
-      expected_json_output = '{}'
+    it 'returns one fact with value, one without and status 0', multiple_facts: true do
+      user_query = ['os.name', 'missing_fact']
+      expected_json_output = '{"os" : {"name": "ubuntu"}, "missing_fact": null}'
 
-      formatted_facts = Facter.to_user_output({}, [user_query])
+      formated_facts = Facter.to_user_output({}, [user_query])
 
-      expect(formatted_facts).to eq([expected_json_output, 0])
+      expect(formated_facts).to eq([expected_json_output, 0])
     end
 
     context 'when provided with --strict option' do
-      it 'returns no fact and status 1', resolved_fact: false do
+      it 'returns one fact with value, one without and status 1', multiple_facts: true do
         user_query = ['os.name', 'missing_fact']
-        expected_json_output = '{}'
+        expected_json_output = '{"os" : {"name": "ubuntu"}, "missing_fact": null}'
         allow(Facter::Options).to receive(:[]).with(:strict).and_return(true)
 
         formatted_facts = Facter.to_user_output({}, *user_query)
@@ -106,9 +116,9 @@ def mock_collection(method, os_name = nil, error = nil)
         expect(formatted_facts).to eq([expected_json_output, 1])
       end
 
-      it 'returns one fact and status 0', resolved_fact: true do
+      it 'returns one fact and status 0', multiple_facts: false do
         user_query = 'os.name'
-        expected_json_output = '{"os" : {"name": "ubuntu"}'
+        expected_json_output = '{"os" : {"name": "ubuntu"}}'
         allow(Facter::Options).to receive(:[]).with(anything)
         allow(Facter::Options).to receive(:[]).with(:block_list).and_return([])
         allow(Facter::Options).to receive(:[]).with(:strict).and_return(true)
@@ -156,6 +166,24 @@ def mock_collection(method, os_name = nil, error = nil)
 
       expect(Facter.fact('os.name')).to be_nil
     end
+
+    context 'when there is a resolved fact with type nil' do
+      before do
+        allow(fact_manager_spy).to receive(:resolve_facts).and_return([missing_fact])
+        allow(fact_collection_spy).to receive(:build_fact_collection!).with([]).and_return(empty_fact_collection)
+        allow(fact_collection_spy).to receive(:value).and_raise(KeyError)
+      end
+
+      it 'is rejected' do
+        Facter.fact('missing_fact')
+
+        expect(fact_collection_spy).to have_received(:build_fact_collection!).with([])
+      end
+
+      it 'returns nil' do
+        expect(Facter.fact('missing_fact')).to be_nil
+      end
+    end
   end
 
   describe '#[]' do
@@ -214,8 +242,10 @@ def mock_collection(method, os_name = nil, error = nil)
 
     describe '#search_path' do
       it 'sends call to Facter::Options' do
-        expect(Facter::Options).to receive(:custom_dir).once
+        allow(Facter::Options).to receive(:custom_dir)
         Facter.search_path
+
+        expect(Facter::Options).to have_received(:custom_dir).once
       end
     end
 
@@ -321,8 +351,9 @@ def mock_collection(method, os_name = nil, error = nil)
 
   describe '#debugging?' do
     it 'returns that log_level is not debug' do
-      expect(Facter::Options).to receive(:[]).with(:debug).and_return(false)
       Facter.debugging?
+
+      expect(Facter::Options).to have_received(:[]).with(:debug)
     end
   end
 
diff --git a/spec/facter/query_parser_spec.rb b/spec/facter/query_parser_spec.rb
index 359187173..64c6895b0 100644
--- a/spec/facter/query_parser_spec.rb
+++ b/spec/facter/query_parser_spec.rb
@@ -88,5 +88,17 @@
       expect(matched_facts).to be_an_instance_of(Array).and \
         contain_exactly(an_instance_of(Facter::SearchedFact).and(having_attributes(fact_class: path_class)))
     end
+
+    context 'when fact does not exist' do
+      let(:query_list) { ['non_existing_fact'] }
+      let(:loaded_facts) { [] }
+
+      it 'creates a nil fact' do
+        matched_facts = Facter::QueryParser.parse(query_list, loaded_facts)
+        expect(matched_facts).to be_an_instance_of(Array).and contain_exactly(
+          an_object_having_attributes(name: 'non_existing_fact', user_query: 'non_existing_fact', type: :nil)
+        )
+      end
+    end
   end
 end
diff --git a/spec/framework/core/fact/internal/internal_fact_manager_spec.rb b/spec/framework/core/fact/internal/internal_fact_manager_spec.rb
index d3d16295e..59aec6e90 100644
--- a/spec/framework/core/fact/internal/internal_fact_manager_spec.rb
+++ b/spec/framework/core/fact/internal/internal_fact_manager_spec.rb
@@ -32,6 +32,24 @@
       expect(resolved_facts).to eq([resolved_fact])
     end
 
+    context 'when resolved fact is of type nil' do
+      let(:searched_fact) do
+        instance_spy(Facter::SearchedFact, name: 'missing_fact', fact_class: nil,
+                                           filter_tokens: [], user_query: '', type: :nil)
+      end
+      let(:resolved_fact) { instance_spy(Facter::ResolvedFact) }
+
+      before do
+        allow(Facter::ResolvedFact).to receive(:new).and_return(resolved_fact)
+      end
+
+      it 'resolved one nil fact' do
+        resolved_facts = internal_fact_manager.resolve_facts([searched_fact])
+
+        expect(resolved_facts).to eq([resolved_fact])
+      end
+    end
+
     context 'when there are multiple search facts pointing to the same fact' do
       before do
         resolved_fact = mock_resolved_fact('os', 'Debian', nil, [])
diff --git a/tasks/commits.rake b/tasks/commits.rake
index 7c53faa5e..5025a78b1 100644
--- a/tasks/commits.rake
+++ b/tasks/commits.rake
@@ -13,7 +13,7 @@ task(:commits) do
   `git log --no-merges --pretty=%s #{commit_range}`.each_line do |commit_summary|
     # This regex tests for the currently supported commit summary tokens: maint, doc, gem, or fact-<number>.
     # The exception tries to explain it in more full.
-    if /^\((maint|doc|docs|gem|fact-\d+)\)|revert/i.match(commit_summary).nil?
+    if /^\((maint|doc|docs|gem|fact-\d+)\)|revert|merge/i.match(commit_summary).nil?
       raise "\n\n\n\tThis commit summary didn't match CONTRIBUTING.md guidelines:\n" \
         "\n\t\t#{commit_summary}\n" \
         "\tThe commit summary (i.e. the first line of the commit message) should start with one of:\n"  \