From e91861ad3b75c321af531df53d0bfbe0c1f65722 Mon Sep 17 00:00:00 2001
From: Jeffrey Clark <h0tw1r3@users.noreply.github.com>
Date: Fri, 19 Jan 2024 09:05:45 -0600
Subject: [PATCH 1/4] optimize search file content

* read file by line until match
* only read files once per check
* prefer include over match

replacing readlines.grep happens to fix #63
---
 .rubocop_todo.yml                             |  5 ++-
 lib/puppet-ghostbuster/util.rb                | 19 +++++++++
 .../plugins/check_ghostbuster_facts.rb        | 39 ++++++++++---------
 .../plugins/check_ghostbuster_files.rb        |  5 ++-
 .../plugins/check_ghostbuster_functions.rb    |  7 ++--
 .../plugins/check_ghostbuster_templates.rb    | 10 ++---
 6 files changed, 56 insertions(+), 29 deletions(-)
 create mode 100644 lib/puppet-ghostbuster/util.rb

diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index 71c267f..31d01ee 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -90,6 +90,7 @@ Style/ClassAndModuleChildren:
 Style/ClassVars:
   Exclude:
     - 'lib/puppet-ghostbuster/puppetdb.rb'
+    - 'lib/puppet-ghostbuster/util.rb'
 
 # Offense count: 9
 # Configuration parameters: AllowedConstants.
@@ -98,6 +99,7 @@ Style/Documentation:
     - 'spec/**/*'
     - 'test/**/*'
     - 'lib/puppet-ghostbuster/puppetdb.rb'
+    - 'lib/puppet-ghostbuster/util.rb'
     - 'lib/puppet-lint/plugins/check_ghostbuster_classes.rb'
     - 'lib/puppet-lint/plugins/check_ghostbuster_defines.rb'
     - 'lib/puppet-lint/plugins/check_ghostbuster_facts.rb'
@@ -114,6 +116,7 @@ Style/Documentation:
 Style/FrozenStringLiteralComment:
   Exclude:
     - 'lib/puppet-ghostbuster/puppetdb.rb'
+    - 'lib/puppet-ghostbuster/util.rb'
     - 'lib/puppet-ghostbuster/version.rb'
     - 'lib/puppet-lint/plugins/check_ghostbuster_classes.rb'
     - 'lib/puppet-lint/plugins/check_ghostbuster_defines.rb'
@@ -167,4 +170,4 @@ Style/ZeroLengthPredicate:
 # Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, AllowedPatterns.
 # URISchemes: http, https
 Layout/LineLength:
-  Max: 189
+  Max: 195
diff --git a/lib/puppet-ghostbuster/util.rb b/lib/puppet-ghostbuster/util.rb
new file mode 100644
index 0000000..d05ad7a
--- /dev/null
+++ b/lib/puppet-ghostbuster/util.rb
@@ -0,0 +1,19 @@
+class PuppetGhostbuster
+  class Util
+    class << self
+      def search_file(name, search)
+        return search_file_regexp(name, search) if search.is_a?(Regexp)
+
+        File.foreach(name) do |line|
+          return true if line.include?(search)
+        end
+      end
+
+      def search_file_regexp(name, search)
+        File.foreach(name) do |line|
+          return true if line.match?(search)
+        end
+      end
+    end
+  end
+end
diff --git a/lib/puppet-lint/plugins/check_ghostbuster_facts.rb b/lib/puppet-lint/plugins/check_ghostbuster_facts.rb
index 263767c..cce6eb3 100644
--- a/lib/puppet-lint/plugins/check_ghostbuster_facts.rb
+++ b/lib/puppet-lint/plugins/check_ghostbuster_facts.rb
@@ -1,3 +1,5 @@
+require 'puppet-ghostbuster/util'
+
 class PuppetLint::Checks
   def load_data(path, content)
     lexer = PuppetLint::Lexer.new
@@ -25,29 +27,30 @@ def check
     m = path.match(%r{.*/([^/]+)/lib/facter/(.+)$})
     return if m.nil?
 
-    File.readlines(path).grep(/Facter.add\(["']([^"']+)["']\)/).each do |line|
-      fact_name = line.match(/Facter.add\(["']([^"']+)["']\)/).captures[0]
+    File.foreach(path) do |line|
+      if line.match?(/Facter.add\(["']([^"']+)["']\)/)
+        fact_name = line.match(/Facter.add\(["']([^"']+)["']\)/).captures[0]
 
-      found = false
+        found = false
 
-      manifests.each do |manifest|
-        found = true if File.readlines(manifest).grep(/\$\{?::#{fact_name}\}?/).size > 0
-        found = true if File.readlines(manifest).grep(/@#{fact_name}/).size > 0
-        break if found
-      end
+        manifests.each do |manifest|
+          found = true unless PuppetGhostbuster::Util.search_file(manifest, /(\$\{?::#{fact_name}\}?|@#{fact_name})/).nil?
+          break if found
+        end
 
-      templates.each do |template|
-        found = true if File.readlines(template).grep(/@#{fact_name}/).size > 0
-        break if found
-      end
+        templates.each do |template|
+          found = true unless PuppetGhostbuster::Util.search_file(template, /@#{fact_name}/).nil?
+          break if found
+        end
 
-      next if found
+        next if found
 
-      notify :warning, {
-        message: "Fact #{fact_name} seems unused",
-        line: 1,
-        column: 1,
-      }
+        notify :warning, {
+          message: "Fact #{fact_name} seems unused",
+          line: 1,
+          column: 1,
+        }
+      end
     end
   end
 end
diff --git a/lib/puppet-lint/plugins/check_ghostbuster_files.rb b/lib/puppet-lint/plugins/check_ghostbuster_files.rb
index 5e43ae7..ddc0777 100644
--- a/lib/puppet-lint/plugins/check_ghostbuster_files.rb
+++ b/lib/puppet-lint/plugins/check_ghostbuster_files.rb
@@ -1,4 +1,5 @@
 require 'puppet-ghostbuster/puppetdb'
+require 'puppet-ghostbuster/util'
 
 class PuppetLint::Checks
   def load_data(path, content)
@@ -42,9 +43,9 @@ def check
     end
 
     manifests.each do |manifest|
-      return if File.readlines(manifest).grep(%r{["']#{module_name}/#{file_name}["']}).size > 0
+      return if PuppetGhostbuster::Util.search_file(manifest, %r{["']#{module_name}/#{file_name}["']})
 
-      if (match = manifest.match(%r{.*/([^/]+)/manifests/.+$})) && (match.captures[0] == module_name) && (File.readlines(manifest).grep(%r{["']\$\{module_name\}/#{file_name}["']}).size > 0)
+      if (match = manifest.match(%r{.*/([^/]+)/manifests/.+$})) && (match.captures[0] == module_name) && PuppetGhostbuster::Util.search_file(manifest, %r{["']\$\{module_name\}/#{file_name}["']})
         return
       end
     end
diff --git a/lib/puppet-lint/plugins/check_ghostbuster_functions.rb b/lib/puppet-lint/plugins/check_ghostbuster_functions.rb
index 31853f0..33ba5ca 100644
--- a/lib/puppet-lint/plugins/check_ghostbuster_functions.rb
+++ b/lib/puppet-lint/plugins/check_ghostbuster_functions.rb
@@ -1,3 +1,5 @@
+require 'puppet-ghostbuster/util'
+
 class PuppetLint::Checks
   def load_data(path, content)
     lexer = PuppetLint::Lexer.new
@@ -28,12 +30,11 @@ def check
     function_name = m.captures[0]
 
     manifests.each do |manifest|
-      return if File.readlines(manifest).grep(/#{function_name}\(/).size > 0
+      return if PuppetGhostbuster::Util.search_file(manifest, "#{function_name}(")
     end
 
     templates.each do |template|
-      return if File.readlines(template).grep(/scope.function_#{function_name}\(/).size > 0
-      return if File.readlines(template).grep(/Puppet::Parser::Functions.function\(:#{function_name}/).size > 0
+      return if PuppetGhostbuster::Util.search_file(template, /(Puppet::Parser::Functions\.function\(:|scope\.function_)#{function_name}/)
     end
 
     notify :warning, {
diff --git a/lib/puppet-lint/plugins/check_ghostbuster_templates.rb b/lib/puppet-lint/plugins/check_ghostbuster_templates.rb
index 8377453..71f6441 100644
--- a/lib/puppet-lint/plugins/check_ghostbuster_templates.rb
+++ b/lib/puppet-lint/plugins/check_ghostbuster_templates.rb
@@ -1,3 +1,5 @@
+require 'puppet-ghostbuster/util'
+
 class PuppetLint::Checks
   def load_data(path, content)
     lexer = PuppetLint::Lexer.new
@@ -28,19 +30,17 @@ def check
     module_name, template_name = m.captures
 
     manifests.each do |manifest|
-      return if File.readlines(manifest).grep(%r{["']#{module_name}/#{template_name}["']}).size > 0
+      return if PuppetGhostbuster::Util.search_file(manifest, %r{["']#{module_name}/#{template_name}["']})
 
       next unless match = manifest.match(%r{.*/([^/]+)/manifests/.+$})
 
-      if match.captures[0] == module_name && (File.readlines(manifest).grep(%r{["']\$\{module_name\}/#{template_name}["']}).size > 0)
+      if match.captures[0] == module_name && PuppetGhostbuster::Util.search_file(manifest, %r{["']\$\{module_name\}/#{template_name}["']})
         return
       end
     end
 
     templates.each do |template|
-      if File.readlines(template).grep(%r{scope.function_template\(\['#{module_name}/#{template_name}'\]\)}).size > 0
-        return
-      end
+      return if PuppetGhostbuster::Util.search_file(template, "scope.function_template(['#{module_name}/#{template_name}'])")
     end
 
     notify :warning, {

From 592b7b3e8b5387a12aab6e2d143b4e000b124366 Mon Sep 17 00:00:00 2001
From: Jeffrey Clark <h0tw1r3@users.noreply.github.com>
Date: Fri, 19 Jan 2024 09:09:52 -0600
Subject: [PATCH 2/4] optimize puppetdb query

---
 lib/puppet-ghostbuster/puppetdb.rb | 8 ++++----
 spec/spec_helper.rb                | 1 +
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/puppet-ghostbuster/puppetdb.rb b/lib/puppet-ghostbuster/puppetdb.rb
index f6fa532..e105fdc 100644
--- a/lib/puppet-ghostbuster/puppetdb.rb
+++ b/lib/puppet-ghostbuster/puppetdb.rb
@@ -40,9 +40,9 @@ def client
 
     def self.classes
       @@classes ||= client.request('',
-                                   'resources[title] { type = "Class" and nodes { deactivated is null } }').data.map do |r|
+                                   'resources[title] { type = "Class" and nodes { deactivated is null } group by title }').data.map do |r|
         r['title']
-      end.uniq
+      end
     end
 
     def classes
@@ -50,9 +50,9 @@ def classes
     end
 
     def self.resources
-      @@resources ||= client.request('', 'resources[type] { nodes { deactivated is null } }').data.map do |r|
+      @@resources ||= client.request('', 'resources[type] { nodes { deactivated is null } group by type }').data.map do |r|
         r['type']
-      end.uniq
+      end
     end
 
     def resources
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index fceefe0..6f4af69 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -21,6 +21,7 @@ def pql_to_jgrep(query)
     endpoint_cols = query.split('{').first
     endpoint = endpoint_cols.split(/[\s\[]/).first
     query.sub!(/^#{Regexp.quote(endpoint_cols)}\{\s*/, '')
+    query.sub!(/(group\s+by\s+(type|title))/, '')
     query.sub!(/\s*}\s*/, '')
     query.sub!(/(and\s+)?nodes\s*\{\s*deactivated\s+is\s+null\s*\}/, '')
 

From b91c790d0f678d141e780fd266523a79ff5f05c7 Mon Sep 17 00:00:00 2001
From: Jeffrey Clark <h0tw1r3@users.noreply.github.com>
Date: Sat, 20 Jan 2024 10:05:00 -0600
Subject: [PATCH 3/4] support facts added by symbol

---
 lib/puppet-lint/plugins/check_ghostbuster_facts.rb |  4 ++--
 spec/fixtures/modules/foo/lib/facter/asym.rb       |  8 ++++++++
 spec/puppet-lint/plugins/ghostbuster_facts_spec.rb | 12 ++++++++++++
 3 files changed, 22 insertions(+), 2 deletions(-)
 create mode 100644 spec/fixtures/modules/foo/lib/facter/asym.rb

diff --git a/lib/puppet-lint/plugins/check_ghostbuster_facts.rb b/lib/puppet-lint/plugins/check_ghostbuster_facts.rb
index cce6eb3..4c9545f 100644
--- a/lib/puppet-lint/plugins/check_ghostbuster_facts.rb
+++ b/lib/puppet-lint/plugins/check_ghostbuster_facts.rb
@@ -28,8 +28,8 @@ def check
     return if m.nil?
 
     File.foreach(path) do |line|
-      if line.match?(/Facter.add\(["']([^"']+)["']\)/)
-        fact_name = line.match(/Facter.add\(["']([^"']+)["']\)/).captures[0]
+      if line =~ /Facter.add\(["':](?<fact>[^"'\)]+)["']?\)/
+        fact_name = Regexp.last_match(:fact)
 
         found = false
 
diff --git a/spec/fixtures/modules/foo/lib/facter/asym.rb b/spec/fixtures/modules/foo/lib/facter/asym.rb
new file mode 100644
index 0000000..a5e0bc4
--- /dev/null
+++ b/spec/fixtures/modules/foo/lib/facter/asym.rb
@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+# Fact is a symbol
+Facter.add(:asym) do
+  setcode do
+    'asym'
+  end
+end
diff --git a/spec/puppet-lint/plugins/ghostbuster_facts_spec.rb b/spec/puppet-lint/plugins/ghostbuster_facts_spec.rb
index bcb6307..1f554c0 100644
--- a/spec/puppet-lint/plugins/ghostbuster_facts_spec.rb
+++ b/spec/puppet-lint/plugins/ghostbuster_facts_spec.rb
@@ -53,5 +53,17 @@
         expect(problems.size).to eq(0)
       end
     end
+
+    context 'when added fact is a symbol and unused' do
+      let(:path) { './spec/fixtures/modules/foo/lib/facter/asym.rb' }
+
+      it 'detects one problem' do
+        expect(problems.size).to eq(1)
+      end
+
+      it 'creates a warning' do
+        expect(problems).to contain_warning('Fact asym seems unused')
+      end
+    end
   end
 end

From cba124353c69cd6d021162e68d325dc96d445d41 Mon Sep 17 00:00:00 2001
From: Jeffrey Clark <h0tw1r3@users.noreply.github.com>
Date: Sat, 20 Jan 2024 14:27:47 -0600
Subject: [PATCH 4/4] support for hiera datadir

---
 .../plugins/check_ghostbuster_hiera_files.rb  | 27 +++++++++---
 spec/fixtures/hiera.yaml                      |  6 +++
 .../plugins/ghostbuster_hiera_files_spec.rb   | 44 ++++++++++++++-----
 3 files changed, 60 insertions(+), 17 deletions(-)

diff --git a/lib/puppet-lint/plugins/check_ghostbuster_hiera_files.rb b/lib/puppet-lint/plugins/check_ghostbuster_hiera_files.rb
index 7503851..b1fd841 100644
--- a/lib/puppet-lint/plugins/check_ghostbuster_hiera_files.rb
+++ b/lib/puppet-lint/plugins/check_ghostbuster_hiera_files.rb
@@ -13,13 +13,26 @@ def load_data(path, content)
 end
 
 PuppetLint.new_check(:ghostbuster_hiera_files) do
+  def hiera
+    @hiera ||= YAML.load_file(ENV['HIERA_YAML_PATH'] || '/etc/puppetlabs/code/hiera.yaml')
+  end
+
+  def default_datadir
+    hiera.dig('defaults', 'datadir') || 'hieradata'
+  end
+
+  def path_in_datadirs?(path)
+    @data_dirs ||= hiera['hierarchy'].map { |h| (h['datadir'] || default_datadir).chomp('/') }.uniq
+    path.match?(%(./#{Regexp.union(@data_dirs)}/))
+  end
+
   def regexprs
-    hiera_yaml_file = ENV['HIERA_YAML_PATH'] || '/etc/puppetlabs/code/hiera.yaml'
-    hiera = YAML.load_file(hiera_yaml_file)
     regs = {}
     hiera['hierarchy'].each do |hierarchy|
+      path_datadir = Regexp.escape((hierarchy['datadir'] || default_datadir).chomp('/'))
       ([*hierarchy['path']] + [*hierarchy['paths']]).each do |level|
-        regex = level.gsub(/%\{(::)?(trusted|server_facts|facts)\.[^\}]+\}/, '(.+)').gsub(/%\{[^\}]+\}/, '.+')
+        level = File.join(path_datadir, level)
+        regex = Regexp.new(level.gsub(/%\{(::)?(trusted|server_facts|facts)\.[^\}]+\}/, '(.+)').gsub(/%\{[^\}]+\}/, '.+'))
         facts = level.match(regex).captures.map { |f| f[/%{(::)?(trusted|server_facts|facts)\.(.+)}/, 3] }
         regs[regex] = facts
       end
@@ -28,14 +41,14 @@ def regexprs
   end
 
   def check
-    return if path.match(%r{./hieradata/.*\.yaml}).nil?
+    return unless path_in_datadirs? path
 
     puppetdb = PuppetGhostbuster::PuppetDB.new
-    _path = path.gsub('./hieradata/', '')
 
     regexprs.each do |k, v|
-      m = _path.match(Regexp.new(k))
+      m = path.match(k)
       next if m.nil?
+
       return if m.captures.size == 0
 
       if m.captures.size > 1
@@ -68,7 +81,7 @@ def check
     end
 
     notify :warning, {
-      message: "Hiera File #{_path} seems unused",
+      message: "Hiera File #{path} seems unused",
       line: 1,
       column: 1,
     }
diff --git a/spec/fixtures/hiera.yaml b/spec/fixtures/hiera.yaml
index 7d32a83..214f10e 100644
--- a/spec/fixtures/hiera.yaml
+++ b/spec/fixtures/hiera.yaml
@@ -1,6 +1,12 @@
 ---
 version: 5
+defaults:
+  datadir: data
 hierarchy:
+  - name: "Some other location"
+    datadir: private/
+    path: "nodes/%{trusted.certname}.eyaml"
+
   - name: "Per-node data (yaml version)"
     path: "nodes/%{trusted.certname}.yaml"
 
diff --git a/spec/puppet-lint/plugins/ghostbuster_hiera_files_spec.rb b/spec/puppet-lint/plugins/ghostbuster_hiera_files_spec.rb
index 6d3a688..8e10ebf 100644
--- a/spec/puppet-lint/plugins/ghostbuster_hiera_files_spec.rb
+++ b/spec/puppet-lint/plugins/ghostbuster_hiera_files_spec.rb
@@ -9,19 +9,19 @@
 
   context 'with fix disabled' do
     context 'when a certname file is NOT used' do
-      let(:path) { './hieradata/nodes/foo.example.com.yaml' }
+      let(:path) { './data/nodes/foo.example.com.yaml' }
 
       it 'detects one problem' do
         expect(problems.size).to eq(1)
       end
 
       it 'creates a warning' do
-        expect(problems).to contain_warning('Hiera File nodes/foo.example.com.yaml seems unused')
+        expect(problems).to contain_warning("Hiera File #{path} seems unused")
       end
     end
 
     context 'when a certname file is used' do
-      let(:path) { './hieradata/nodes/bar.example.com.yaml' }
+      let(:path) { './data/nodes/bar.example.com.yaml' }
 
       it 'does not detect any problem' do
         expect(problems.size).to eq(0)
@@ -29,19 +29,19 @@
     end
 
     context 'when an environment file is NOT used' do
-      let(:path) { './hieradata/environment/foo.yaml' }
+      let(:path) { './data/environment/foo.yaml' }
 
       it 'detects one problem' do
         expect(problems.size).to eq(1)
       end
 
       it 'creates a warning' do
-        expect(problems).to contain_warning('Hiera File environment/foo.yaml seems unused')
+        expect(problems).to contain_warning("Hiera File #{path} seems unused")
       end
     end
 
     context 'when an environment file is used' do
-      let(:path) { './hieradata/environment/production.yaml' }
+      let(:path) { './data/environment/production.yaml' }
 
       it 'does not detect any problem' do
         expect(problems.size).to eq(0)
@@ -49,31 +49,55 @@
     end
 
     context 'when an fact is NOT used' do
-      let(:path) { './hieradata/virtual/false.yaml' }
+      let(:path) { './data/virtual/false.yaml' }
 
       it 'detects one problem' do
         expect(problems.size).to eq(1)
       end
 
       it 'creates a warning' do
-        expect(problems).to contain_warning('Hiera File virtual/false.yaml seems unused')
+        expect(problems).to contain_warning("Hiera File #{path} seems unused")
       end
     end
 
     context 'when an fact file is used' do
-      let(:path) { './hieradata/virtual/true.yaml' }
+      let(:path) { './data/virtual/true.yaml' }
 
       it 'does not detect any problem' do
         expect(problems.size).to eq(0)
       end
     end
 
+    context 'when an fact file is used with wrong extension' do
+      let(:path) { './data/virtual/true.eyaml' }
+
+      it 'detects one problem' do
+        expect(problems.size).to eq(1)
+      end
+
+      it 'creates a warning' do
+        expect(problems).to contain_warning("Hiera File #{path} seems unused")
+      end
+    end
+
     context 'when using a variable in hierarchy' do
-      let(:path) { './hieradata/domain/example.com.yaml' }
+      let(:path) { './data/domain/example.com.yaml' }
 
       it 'does not detect any problem' do
         expect(problems.size).to eq(0)
       end
     end
+
+    context 'when hierarchy datadir is NOT default and NOT used' do
+      let(:path) { './private/nodes/privates.example.com.eyaml' }
+
+      it 'detects one problem' do
+        expect(problems.size).to eq(1)
+      end
+
+      it 'creates a warning' do
+        expect(problems).to contain_warning("Hiera File #{path} seems unused")
+      end
+    end
   end
 end