(FACT-2561) Fix blocking mechanism

.rubocop_todo.yml

@@ -1,12 +1,12 @@
 # This configuration was generated by
 # `rubocop --auto-gen-config --exclude-limit 1000`
-# on 2020-08-13 15:12:10 +0300 using RuboCop version 0.81.0.
+# on 2020-08-26 01:26:00 +0300 using RuboCop version 0.81.0.
 # The point is for the user to remove these configuration records
 # one by one as the offenses are removed from the code base.
 # Note that changes in the inspected code, or installation of new
 # versions of RuboCop, may require this file to be generated again.
 
-# Offense count: 81
+# Offense count: 76
 # Configuration parameters: CustomTransform, IgnoreMethods, SpecSuffixOnly.
 RSpec/FilePath:
   Exclude:
@@ -45,13 +45,10 @@ RSpec/FilePath:
     - 'spec/facter/resolvers/macosx/dmi_resolver_spec.rb'
     - 'spec/facter/resolvers/macosx/utils/system_profile_executor_spec.rb'
     - 'spec/facter/resolvers/memory_resolver_spec.rb'
-    - 'spec/facter/resolvers/mountpoints_resolver_spec.rb'
     - 'spec/facter/resolvers/processors_resolver_spec.rb'
     - 'spec/facter/resolvers/redhat_release_resolver_spec.rb'
     - 'spec/facter/resolvers/selinux_resolver_spec.rb'
     - 'spec/facter/resolvers/solaris/solaris_release_resolver_spec.rb'
-    - 'spec/facter/resolvers/solaris/solaris_zone_name_spec.rb'
-    - 'spec/facter/resolvers/solaris/zone_resolver_spec.rb'
     - 'spec/facter/resolvers/suse_relese_resolver_spec.rb'
     - 'spec/facter/resolvers/system_profile_resolver_spec.rb'
     - 'spec/facter/resolvers/utils/aix/odm_query_spec.rb'
@@ -64,13 +61,11 @@ RSpec/FilePath:
     - 'spec/facter/resolvers/windows/kernel_resolver_spec.rb'
     - 'spec/facter/resolvers/windows/memory_resolver_spec.rb'
     - 'spec/facter/resolvers/windows/netkvm_resolver_spec.rb'
-    - 'spec/facter/resolvers/windows/networking_resolver_spec.rb'
     - 'spec/facter/resolvers/windows/processors_resolver_spec.rb'
     - 'spec/facter/resolvers/windows/product_release_resolver_spec.rb'
     - 'spec/facter/resolvers/windows/system32_resolver_spec.rb'
     - 'spec/facter/resolvers/windows/virtualization_resolver_spec.rb'
     - 'spec/facter/resolvers/windows/win_os_description_resolver_spec.rb'
-    - 'spec/facter/resolvers/zpool_resolver_spec.rb'
     - 'spec/framework/config/config_reader_spec.rb'
     - 'spec/framework/config/fact_groups_spec.rb'
     - 'spec/framework/core/fact/external/external_fact_manager_spec.rb'
@@ -176,7 +171,7 @@ RSpec/SubjectStub:
     - 'spec/custom_facts/util/fact_spec.rb'
     - 'spec/custom_facts/util/resolution_spec.rb'
 
-# Offense count: 130
+# Offense count: 123
 # Configuration parameters: IgnoreNameless, IgnoreSymbolicNames.
 RSpec/VerifiedDoubles:
   Exclude:
@@ -225,7 +220,6 @@ RSpec/VerifiedDoubles:
     - 'spec/facter/resolvers/windows/uptime_resolver_spec.rb'
     - 'spec/facter/resolvers/windows/win_os_description_resolver_spec.rb'
     - 'spec/framework/core/fact_loaders/external_fact_loader_spec.rb'
-    - 'spec/framework/core/fact_loaders/fact_loader_spec.rb'
     - 'spec/framework/core/fact_manager_spec.rb'
     - 'spec/framework/core/session_cache_spec.rb'
     - 'spec/framework/formatters/hocon_fact_formatter_spec.rb'

lib/facter/framework/core/fact_filter.rb

@@ -7,6 +7,8 @@ module Facter
   class FactFilter
     def filter_facts!(searched_facts)
       filter_legacy_facts!(searched_facts)
+      filter_blocked_legacy_facts!(searched_facts)
+
       searched_facts.each do |fact|
         fact.value = if fact.filter_tokens.any? && fact.value.respond_to?(:dig)
                        fact.value.dig(*fact.filter_tokens)
@@ -18,6 +20,17 @@ def filter_facts!(searched_facts)
 
     private
 
+    # This will filter out the legacy facts that should be blocked. Because some legacy facts are just aliases
+    # to the core ones, even if they are blocked, facter will resolved them but they won't be displayed.
+
+    def filter_blocked_legacy_facts!(facts)
+      blocked_facts = Options[:blocked_facts] || []
+
+      facts.reject! do |fact|
+        blocked_facts.select { |blocked_fact| fact.name.match(/^#{blocked_fact}/) && fact.type == :legacy }.any?
+      end
+    end
+
     def filter_legacy_facts!(resolved_facts)
       return unless !Options[:show_legacy] && Options[:user_query].empty?
 

lib/facter/framework/core/fact_loaders/fact_loader.rb

@@ -64,11 +64,34 @@ def load_external_facts(options)
     def block_facts(facts, options)
       blocked_facts = options[:blocked_facts] || []
 
+      reject_list_core, reject_list_legacy = construct_reject_lists(blocked_facts, facts)
+
       facts = facts.reject do |fact|
-        blocked_facts.select { |blocked_fact| fact.name.match(/^#{blocked_fact}/) }.any?
+        reject_list_core.include?(fact) || reject_list_core.find do |fact_to_block|
+          fact_to_block.klass == fact.klass
+        end || reject_list_legacy.include?(fact)
       end
 
       facts
     end
+
+    def construct_reject_lists(blocked_facts, facts)
+      reject_list_core = []
+      reject_list_legacy = []
+
+      blocked_facts.each do |blocked|
+        facts.each do |fact|
+          next unless fact.name =~ /^#{blocked}/
+
+          if fact.type == :core
+            reject_list_core << fact
+          else
+            reject_list_legacy << fact
+          end
+        end
+      end
+
+      [reject_list_core, reject_list_legacy]
+    end
   end
 end

spec/facter/fact_filter_spec.rb

@@ -48,6 +48,25 @@
         expect(resolved_fact.value).to eq('value_1')
       end
     end
+
+    context 'with legacy fact that should be blocked' do
+      let(:fact_value) { 'value_1' }
+      let(:resolved_fact) { Facter::ResolvedFact.new('my_fact', fact_value, :legacy) }
+
+      before do
+        resolved_fact.user_query = ''
+        resolved_fact.filter_tokens = []
+        allow(Facter::Options).to receive(:[]).with(:blocked_facts).and_return(['my_fact'])
+        allow(Facter::Options).to receive(:[]).with(:show_legacy).and_return(true)
+        allow(Facter::Options).to receive(:[]).with(:user_query).and_return('')
+      end
+
+      it 'filters blocked legacy facts' do
+        fact_filter_input = [resolved_fact]
+        Facter::FactFilter.new.filter_facts!(fact_filter_input)
+        expect(fact_filter_input).to eq([])
+      end
+    end
   end
 
   it 'filters value inside fact when value is array' do

spec/framework/core/fact_loaders/fact_loader_spec.rb

@@ -2,17 +2,21 @@
 
 describe Facter::FactLoader do
   describe '#load' do
-    let(:internal_fact_loader_double) { double(Facter::InternalFactLoader) }
-    let(:external_fact_loader_double) { double(Facter::ExternalFactLoader) }
+    let(:internal_fact_loader_double) { instance_spy(Facter::InternalFactLoader) }
+    let(:external_fact_loader_double) { instance_spy(Facter::ExternalFactLoader) }
 
-    let(:ubuntu_os_name) { double(Facts::Linux::Os::Name) }
-    let(:networking_class) { double(Facts::Windows::NetworkInterfaces) }
+    let(:ubuntu_os_name) { instance_spy(Facts::Linux::Os::Name) }
+    let(:networking_class) { instance_spy(Facts::Windows::NetworkInterfaces) }
 
-    let(:loaded_fact_os_name) { double(Facter::LoadedFact, name: 'os.name', klass: ubuntu_os_name, type: :core) }
+    let(:loaded_fact_os_name) { instance_spy(Facter::LoadedFact, name: 'os.name', klass: ubuntu_os_name, type: :core) }
+    let(:loaded_fact_os_name_legacy) do
+      instance_spy(Facter::LoadedFact, name: 'operatingsystem', klass: ubuntu_os_name,
+                                       type: :legacy)
+    end
     let(:loaded_fact_networking) do
-      double(Facter::LoadedFact, name: 'network_.*', klass: networking_class, type: :legacy)
+      instance_spy(Facter::LoadedFact, name: 'network_.*', klass: networking_class, type: :legacy)
     end
-    let(:loaded_fact_custom_fact) { double(Facter::LoadedFact, name: 'custom_fact', klass: nil, type: :custom) }
+    let(:loaded_fact_custom_fact) { instance_spy(Facter::LoadedFact, name: 'custom_fact', klass: nil, type: :custom) }
 
     before do
       Singleton.__init__(Facter::FactLoader)
@@ -50,7 +54,7 @@
     it 'blocks one internal fact' do
       options = { blocked_facts: ['os.name'] }
 
-      facts_to_load = [loaded_fact_os_name]
+      facts_to_load = [loaded_fact_os_name, loaded_fact_os_name_legacy]
 
       allow(internal_fact_loader_double).to receive(:core_facts).and_return(facts_to_load)
       allow(external_fact_loader_double).to receive(:custom_facts).and_return([])
@@ -60,6 +64,19 @@
       expect(loaded_facts.size).to eq(0)
     end
 
+    it 'blocks one legacy fact' do
+      options = { blocked_facts: ['operatingsystem'] }
+
+      facts_to_load = [loaded_fact_os_name, loaded_fact_os_name_legacy]
+
+      allow(internal_fact_loader_double).to receive(:core_facts).and_return(facts_to_load)
+      allow(external_fact_loader_double).to receive(:custom_facts).and_return([])
+      allow(external_fact_loader_double).to receive(:external_facts).and_return([])
+
+      loaded_facts = Facter::FactLoader.instance.load(options)
+      expect(loaded_facts.size).to eq(1)
+    end
+
     context 'when blocking custom facts' do
       before do
         facts_to_load = [loaded_fact_custom_fact]