(FACT-2498) Internal fact loader should only load facts once

.rubocop_todo.yml

@@ -25,7 +25,6 @@ RSpec/AnyInstance:
     - 'spec/facter/resolvers/windows/virtualization_resolver_spec.rb'
     - 'spec/facter/resolvers/windows/win_os_description_resolver_spec.rb'
     - 'spec/framework/core/fact_loaders/class_discoverer_spec.rb'
-    - 'spec/framework/core/fact_loaders/internal_fact_loader_spec.rb'
     - 'spec/framework/core/fact_manager_spec.rb'
     - 'spec/framework/core/options/options_validator_spec.rb'
     - 'spec/mocks/util.rb'
@@ -51,7 +50,6 @@ RSpec/ContextWording:
     - 'spec/facter/resolvers/utils/windows/network_utils_spec.rb'
     - 'spec/framework/config/config_reader_spec.rb'
     - 'spec/framework/core/fact_loaders/external_fact_loader_spec.rb'
-    - 'spec/framework/core/fact_loaders/internal_fact_loader_spec.rb'
     - 'spec/framework/core/options_spec.rb'
     - 'spec/framework/formatters/legacy_fact_formatter_spec.rb'
 

lib/framework/core/fact_loaders/internal_fact_loader.rb

@@ -16,13 +16,13 @@ def initialize
       @facts = []
 
       os_descendents = OsDetector.instance.hierarchy
-      load_all_oses(os_descendents)
+      load_all_oses_in_descending_order(os_descendents)
     end
 
     private
 
-    def load_all_oses(os_descendents)
-      os_descendents.each do |os|
+    def load_all_oses_in_descending_order(os_descendents)
+      os_descendents.reverse_each do |os|
         load_for_os(os)
       end
     end
@@ -34,15 +34,19 @@ def load_for_os(operating_system)
       classes.each do |class_name|
         fact_name = class_name::FACT_NAME
 
-        load_fact(fact_name, class_name)
+        # if fact is already loaded, skip it
+        next if @facts.any? { |fact| fact.name == fact_name }
+
+        type = fact_name.end_with?('.*') ? :legacy : :core
+        load_fact(fact_name, class_name, type)
         next unless class_name.const_defined?('ALIASES')
 
-        [*class_name::ALIASES].each { |fact_alias| load_fact(fact_alias, class_name) }
+        [*class_name::ALIASES].each { |fact_alias| load_fact(fact_alias, class_name, :legacy) }
       end
     end
 
-    def load_fact(fact_name, klass)
-      loaded_fact = LoadedFact.new(fact_name, klass)
+    def load_fact(fact_name, klass, type)
+      loaded_fact = LoadedFact.new(fact_name, klass, type)
       @facts << loaded_fact
     end
   end

spec/framework/core/fact_loaders/internal_fact_loader_spec.rb

@@ -1,74 +1,247 @@
 # frozen_string_literal: true
 
 describe Facter::InternalFactLoader do
-  before do
-    allow_any_instance_of(OsDetector).to receive(:hierarchy).and_return([:Debian])
-  end
-
   describe '#initialize' do
-    context 'load facts' do
-      it 'loads one legacy fact and sees it as core' do
-        allow_any_instance_of(OsDetector).to receive(:hierarchy).and_return([:Windows])
-        allow_any_instance_of(Facter::ClassDiscoverer)
+    let(:os_detector_mock) { instance_spy(OsDetector) }
+
+    before do
+      allow(os_detector_mock).to receive(:hierarchy).and_return([:Debian])
+      allow(OsDetector).to receive(:instance).and_return(os_detector_mock)
+    end
+
+    context 'when loading one legacy facts' do
+      before do
+        allow(os_detector_mock).to receive(:hierarchy).and_return([:Windows])
+        allow(OsDetector).to receive(:instance).and_return(os_detector_mock)
+
+        class_discoverer_mock = instance_spy(Facter::ClassDiscoverer)
+        allow(class_discoverer_mock)
           .to receive(:discover_classes)
           .with(:Windows)
           .and_return([Facts::Windows::NetworkInterfaces])
+        allow(Facter::ClassDiscoverer).to receive(:instance).and_return(class_discoverer_mock)
 
-        stub_const('Facter::Windows::NetworkInterfaces::FACT_NAME', 'network_.*')
+        stub_const('Facts::Windows::NetworkInterfaces::FACT_NAME', 'network_.*')
+      end
 
+      it 'loads no core facts' do
         internal_fact_loader = Facter::InternalFactLoader.new
-        legacy_facts = internal_fact_loader.legacy_facts
-        core_facts = internal_fact_loader.core_facts
 
-        expect(legacy_facts.size).to eq(0)
-        expect(core_facts.size).to eq(1)
-        expect(core_facts.first.type).to eq(:core)
+        expect(internal_fact_loader.core_facts).to be_empty
       end
 
-      it 'loads one core fact' do
-        allow_any_instance_of(Facter::ClassDiscoverer)
+      it 'loads one legacy fact' do
+        internal_fact_loader = Facter::InternalFactLoader.new
+
+        expect(internal_fact_loader.legacy_facts.size).to eq(1)
+      end
+
+      it 'loads one fact with :legacy type' do
+        internal_fact_loader = Facter::InternalFactLoader.new
+
+        expect(internal_fact_loader.legacy_facts.first.type).to eq(:legacy)
+      end
+    end
+
+    context 'when loading one core fact' do
+      before do
+        class_discoverer_mock = instance_spy(Facter::ClassDiscoverer)
+        allow(class_discoverer_mock)
           .to receive(:discover_classes)
           .with(:Debian)
-          .and_return([Facts::Debian::Os::Name])
+          .and_return([Facts::Debian::Path])
+        allow(Facter::ClassDiscoverer).to receive(:instance).and_return(class_discoverer_mock)
+
+        stub_const('Facts::Debian::Path::FACT_NAME', 'path')
+      end
+
+      it 'loads no legacy facts' do
+        internal_fact_loader = Facter::InternalFactLoader.new
 
-        stub_const('Facter::Debian::OsName::FACT_NAME', 'os.name')
+        expect(internal_fact_loader.legacy_facts).to be_empty
+      end
 
+      it 'loads one core fact' do
         internal_fact_loader = Facter::InternalFactLoader.new
-        core_facts = internal_fact_loader.core_facts
 
-        expect(core_facts.size).to eq(2)
-        expect(core_facts.first.type).to eq(:core)
+        expect(internal_fact_loader.core_facts.size).to eq(1)
       end
 
-      it 'loads one legacy fact and one core fact' do
-        allow_any_instance_of(OsDetector).to receive(:hierarchy).and_return([:Windows])
+      it 'loads one fact with :core type' do
+        internal_fact_loader = Facter::InternalFactLoader.new
+
+        expect(internal_fact_loader.core_facts.first.type).to eq(:core)
+      end
+    end
+
+    context 'when loading one legacy fact and one core fact' do
+      before do
+        allow(os_detector_mock).to receive(:hierarchy).and_return([:Windows])
+        allow(OsDetector).to receive(:instance).and_return(os_detector_mock)
 
-        allow_any_instance_of(Facter::ClassDiscoverer)
+        class_discoverer_mock = instance_spy(Facter::ClassDiscoverer)
+        allow(class_discoverer_mock)
           .to receive(:discover_classes)
           .with(:Windows)
-          .and_return([Facts::Windows::NetworkInterfaces, Facts::Windows::Os::Name])
+          .and_return([Facts::Windows::NetworkInterfaces, Facts::Windows::Path])
+        allow(Facter::ClassDiscoverer).to receive(:instance).and_return(class_discoverer_mock)
 
-        stub_const('Facter::Windows::NetworkInterface::FACT_NAME', 'network_.*')
-        stub_const('Facter::Windows::OsName::FACT_NAME', 'os.name')
+        stub_const('Facts::Windows::NetworkInterface::FACT_NAME', 'network_.*')
+        stub_const('Facts::Windows::OsName::FACT_NAME', 'path')
+      end
 
+      it 'loads two facts' do
         internal_fact_loader = Facter::InternalFactLoader.new
-        all_facts = internal_fact_loader.facts
 
-        expect(all_facts.size).to eq(3)
-        all_facts.each do |fact|
-          expect(fact.type).to eq(:core)
-        end
+        expect(internal_fact_loader.facts.size).to eq(2)
       end
 
-      it 'loads no facts' do
-        allow_any_instance_of(Facter::ClassDiscoverer)
+      it 'loads one legacy fact' do
+        internal_fact_loader = Facter::InternalFactLoader.new
+
+        expect(internal_fact_loader.facts.count { |lf| lf.type == :legacy }).to eq(1)
+      end
+
+      it 'loads one core fact' do
+        internal_fact_loader = Facter::InternalFactLoader.new
+
+        expect(internal_fact_loader.facts.count { |lf| lf.type == :core }).to eq(1)
+      end
+    end
+
+    context 'when loading no facts' do
+      before do
+        class_discoverer_mock = instance_spy(Facter::ClassDiscoverer)
+        allow(class_discoverer_mock)
           .to receive(:discover_classes)
           .with(:Debian)
           .and_return([])
+        allow(Facter::ClassDiscoverer).to receive(:instance).and_return(class_discoverer_mock)
+      end
+
+      it 'loads no facts' do
+        internal_fact_loader = Facter::InternalFactLoader.new
+
+        expect(internal_fact_loader.facts).to be_empty
+      end
+    end
+
+    context 'when loading hierarchy of facts' do
+      before do
+        allow(os_detector_mock).to receive(:hierarchy).and_return(%i[Debian El])
+        allow(OsDetector).to receive(:instance).and_return(os_detector_mock)
+
+        class_discoverer_mock = instance_spy(Facter::ClassDiscoverer)
+        allow(class_discoverer_mock)
+          .to receive(:discover_classes)
+          .with(:Debian)
+          .and_return([Facts::Debian::Path])
+        allow(class_discoverer_mock)
+          .to receive(:discover_classes)
+          .with(:El)
+          .and_return([Facts::El::Path])
+        allow(Facter::ClassDiscoverer).to receive(:instance).and_return(class_discoverer_mock)
+
+        stub_const('Facts::Debian::Path::FACT_NAME', 'path')
+        stub_const('Facts::El::Path::FACT_NAME', 'path')
+      end
+
+      let(:internal_fact_loader) { Facter::InternalFactLoader.new }
+
+      it 'loads one fact' do
+        internal_fact_loader = Facter::InternalFactLoader.new
+
+        expect(internal_fact_loader.facts.size).to eq(1)
+      end
+
+      it 'loads path fact' do
+        internal_fact_loader = Facter::InternalFactLoader.new
+
+        expect(internal_fact_loader.facts.first.name).to eq('path')
+      end
+
+      it 'loads only el path' do
+        internal_fact_loader = Facter::InternalFactLoader.new
+
+        expect(internal_fact_loader.facts.first.klass).to eq(Facts::El::Path)
+      end
+    end
+
+    context 'when loading fact with aliases' do
+      before do
+        class_discoverer_mock = instance_spy(Facter::ClassDiscoverer)
+        allow(class_discoverer_mock)
+          .to receive(:discover_classes)
+          .with(:Debian)
+          .and_return([Facts::Debian::Os::Name])
+        allow(Facter::ClassDiscoverer).to receive(:instance).and_return(class_discoverer_mock)
+
+        stub_const('Facts::Debian::Os::Name::FACT_NAME', 'os.name')
+        stub_const('Facts::Debian::Os::Name::ALIASES', 'operatingsystem')
+      end
+
+      it 'loads two facts' do
+        internal_fact_loader = Facter::InternalFactLoader.new
+
+        expect(internal_fact_loader.facts.size).to eq(2)
+      end
+
+      it 'loads one core fact' do
+        internal_fact_loader = Facter::InternalFactLoader.new
+
+        expect(internal_fact_loader.core_facts.size).to eq(1)
+      end
+
+      it 'loads one legacy fact' do
+        internal_fact_loader = Facter::InternalFactLoader.new
+
+        expect(internal_fact_loader.legacy_facts.size).to eq(1)
+      end
+
+      it 'loads a core fact with the fact name' do
+        internal_fact_loader = Facter::InternalFactLoader.new
+
+        expect(internal_fact_loader.core_facts.first.name).to eq('os.name')
+      end
+
+      it 'loads a legacy fact with the alias name' do
+        internal_fact_loader = Facter::InternalFactLoader.new
+
+        expect(internal_fact_loader.legacy_facts.first.name).to eq('operatingsystem')
+      end
+    end
+
+    context 'when loading wildcard facts' do
+      before do
+        allow(os_detector_mock).to receive(:hierarchy).and_return([:Windows])
+        allow(OsDetector).to receive(:instance).and_return(os_detector_mock)
+
+        class_discoverer_mock = instance_spy(Facter::ClassDiscoverer)
+        allow(class_discoverer_mock)
+          .to receive(:discover_classes)
+          .with(:Windows)
+          .and_return([Facts::Windows::NetworkInterfaces])
+        allow(Facter::ClassDiscoverer).to receive(:instance).and_return(class_discoverer_mock)
+
+        stub_const('Facts::Windows::NetworkInterfaces::FACT_NAME', 'network_.*')
+      end
+
+      it 'loads one fact' do
+        internal_fact_loader = Facter::InternalFactLoader.new
+
+        expect(internal_fact_loader.facts.size).to eq(1)
+      end
+
+      it 'loads one legacy fact' do
+        internal_fact_loader = Facter::InternalFactLoader.new
+
+        expect(internal_fact_loader.legacy_facts.size).to eq(1)
+      end
+
+      it 'loads no core facts' do
         internal_fact_loader = Facter::InternalFactLoader.new
-        all_facts_hash = internal_fact_loader.facts
 
-        expect(all_facts_hash.size).to eq(0)
+        expect(internal_fact_loader.core_facts).to be_empty
       end
     end
   end