From 50bbef9b6117b189d57c705bca3eed52e879829e Mon Sep 17 00:00:00 2001 From: Aria Li Date: Tue, 11 Jul 2023 12:14:57 -0700 Subject: [PATCH] (FACT-3207) Don't rescue NoMethodError in base resolver This commit removes the logic to specifically rescue for just NoMethodError. Instead, since NameError is a superclass of NoMethodError, NoMethodErrors will be rescued when NameError is rescued and log at the error level instead of the debug level. By doing this, bugs in resolvers are no longer hidden. This commit also fixes various bugs in resolvers such as calling methods on nil objects and requiring/relying gems that may not be installed to resolve facts. --- lib/facter/resolvers/augeas.rb | 3 +++ lib/facter/resolvers/base_resolver.rb | 5 +---- lib/facter/resolvers/dmi.rb | 2 +- lib/facter/resolvers/solaris/mountpoints.rb | 2 +- lib/facter/resolvers/windows/timezone.rb | 2 +- lib/facter/util/resolvers/filesystem_helper.rb | 2 +- spec/facter/resolvers/augeas_spec.rb | 2 +- spec/facter/resolvers/base_resolver_spec.rb | 6 +++--- 8 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/facter/resolvers/augeas.rb b/lib/facter/resolvers/augeas.rb index 5fe71536b1..277cab78bd 100644 --- a/lib/facter/resolvers/augeas.rb +++ b/lib/facter/resolvers/augeas.rb @@ -17,6 +17,9 @@ def read_augeas_version(fact_name) @fact_list[:augeas_version] ||= read_augeas_from_gem @fact_list[fact_name] + rescue LoadError => e + log.debug("Resolving fact #{fact_name}, but got #{e} at #{e.backtrace[0]}") + @fact_list[:augeas_version] = nil end def read_augeas_from_cli diff --git a/lib/facter/resolvers/base_resolver.rb b/lib/facter/resolvers/base_resolver.rb index 35610a54be..7b67fed0e2 100644 --- a/lib/facter/resolvers/base_resolver.rb +++ b/lib/facter/resolvers/base_resolver.rb @@ -27,11 +27,8 @@ def self.resolve(fact_name, options = {}) cache_nil_for_unresolved_facts(fact_name) end - rescue NoMethodError => e - log.debug("Could not resolve #{fact_name}, got #{e} at #{e.backtrace[0]}") - @fact_list[fact_name] = nil rescue LoadError, NameError => e - log.debug("Resolving fact #{fact_name}, but got #{e} at #{e.backtrace[0]}") + log.error("Resolving fact #{fact_name}, but got #{e} at #{e.backtrace[0]}") @fact_list[fact_name] = nil end diff --git a/lib/facter/resolvers/dmi.rb b/lib/facter/resolvers/dmi.rb index d66ce5a20c..bbe8f259bf 100644 --- a/lib/facter/resolvers/dmi.rb +++ b/lib/facter/resolvers/dmi.rb @@ -36,7 +36,7 @@ def read_facts(fact_name) return unless File.directory?('/sys/class/dmi') file_content = Facter::Util::FileHelper.safe_read("/sys/class/dmi/id/#{fact_name}", nil) - .encode('UTF-8', invalid: :replace) + file_content = file_content.encode('UTF-8', invalid: :replace) if file_content if files.include?(fact_name.to_s) && file_content file_content = file_content.strip @fact_list[fact_name] = file_content unless file_content.empty? diff --git a/lib/facter/resolvers/solaris/mountpoints.rb b/lib/facter/resolvers/solaris/mountpoints.rb index 8ca69c5da6..fc2649df61 100644 --- a/lib/facter/resolvers/solaris/mountpoints.rb +++ b/lib/facter/resolvers/solaris/mountpoints.rb @@ -25,7 +25,7 @@ def read_mounts(fact_name) # rubocop:disable Metrics/MethodLength @mounts = [] @auto_home_paths = [] - Facter::Util::Resolvers::FilesystemHelper.read_mountpoints.each do |fs| + Facter::Util::Resolvers::FilesystemHelper.read_mountpoints&.each do |fs| if fs.name == 'auto_home' @auto_home_paths << fs.mount_point next diff --git a/lib/facter/resolvers/windows/timezone.rb b/lib/facter/resolvers/windows/timezone.rb index 4c9224b67b..1fd25c4e31 100644 --- a/lib/facter/resolvers/windows/timezone.rb +++ b/lib/facter/resolvers/windows/timezone.rb @@ -22,7 +22,7 @@ def determine_timezone def codepage result = codepage_from_api - result.empty? ? codepage_from_registry : result + result&.empty? ? codepage_from_registry : result end def codepage_from_registry diff --git a/lib/facter/util/resolvers/filesystem_helper.rb b/lib/facter/util/resolvers/filesystem_helper.rb index efb502ce2b..51b64d4ee2 100644 --- a/lib/facter/util/resolvers/filesystem_helper.rb +++ b/lib/facter/util/resolvers/filesystem_helper.rb @@ -33,7 +33,7 @@ def compute_capacity(used, total) private def force_utf(mounts) - mounts.each do |mount| + mounts&.each do |mount| mount.name.force_encoding('UTF-8') mount.mount_type.force_encoding('UTF-8') mount.mount_point.force_encoding('UTF-8') diff --git a/spec/facter/resolvers/augeas_spec.rb b/spec/facter/resolvers/augeas_spec.rb index 04c2a1de38..65af91ea46 100644 --- a/spec/facter/resolvers/augeas_spec.rb +++ b/spec/facter/resolvers/augeas_spec.rb @@ -77,7 +77,7 @@ allow(Facter::Resolvers::Augeas).to receive(:require).with('augeas').and_raise(exception) end - it 'raises a LoadError error' do + it 'rescues LoadError error and logs at debug level' do augeas.resolve(:augeas_version) expect(log_spy).to have_received(:debug).with(/Resolving fact augeas_version, but got load_error_message/) diff --git a/spec/facter/resolvers/base_resolver_spec.rb b/spec/facter/resolvers/base_resolver_spec.rb index 43cdf2b17f..ae6a49c192 100644 --- a/spec/facter/resolvers/base_resolver_spec.rb +++ b/spec/facter/resolvers/base_resolver_spec.rb @@ -77,13 +77,13 @@ def self.post_resolve(fact_name, _options) context 'when Load Error is raised' do before do allow(resolver).to receive(:post_resolve).and_raise(LoadError) - allow(Facter::Log).to receive(:new).with(resolver).and_return(instance_double(Facter::Log, debug: nil)) + allow(Facter::Log).to receive(:new).with(resolver).and_return(instance_double(Facter::Log, error: nil)) end - it 'logs the Load Error exception' do + it 'logs the Load Error exception at the error level' do resolver.resolve(fact) - expect(resolver.log).to have_received(:debug).with(/Resolving fact #{fact}, but got LoadError/) + expect(resolver.log).to have_received(:error).with(/Resolving fact #{fact}, but got LoadError/) end it 'sets the fact to nil' do