Skip to content
This repository has been archived by the owner on Jun 19, 2020. It is now read-only.

(FACT-2635) Incorrect output for non existing fact #536

Merged
merged 15 commits into from
Jun 3, 2020
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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'
Expand Down
7 changes: 4 additions & 3 deletions lib/facter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -310,9 +310,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)
Expand Down
22 changes: 19 additions & 3 deletions lib/framework/core/fact/internal/internal_fact_manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,14 @@ 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_searched_facts = filter_nil_facts(searched_facts)
nil_resolved_facts = resolve_nil_facts(nil_searched_facts)

join_threads(threads, searched_facts)
resolved_facts.concat(nil_resolved_facts)
end

private
Expand All @@ -18,6 +21,19 @@ def filter_internal_facts(searched_facts)
searched_facts.select { |searched_fact| %i[core legacy].include? searched_fact.type }
end

def filter_nil_facts(searched_facts)
searched_facts.select { |searched_fact| %i[nil].include? searched_fact.type }
end

def resolve_nil_facts(searched_facts)
resolved_facts = []
searched_facts.select { |fact| fact.type == :nil }.each do |fact|
BogdanIrimie marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
6 changes: 4 additions & 2 deletions lib/framework/formatters/legacy_fact_formatter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 =>')
Expand All @@ -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
6 changes: 5 additions & 1 deletion lib/framework/parsers/query_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand Down
55 changes: 34 additions & 21 deletions spec/facter/facter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -68,47 +74,51 @@ 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)

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)
Expand Down Expand Up @@ -214,8 +224,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

Expand Down Expand Up @@ -321,8 +333,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

Expand Down
18 changes: 18 additions & 0 deletions spec/framework/core/fact/internal/internal_fact_manager_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,24 @@
expect(resolved_facts).to eq([resolved_fact])
end

context 'when resolved fac is of type nil' do
BogdanIrimie marked this conversation as resolved.
Show resolved Hide resolved
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, [])
Expand Down