Split absent file into a filter

Author: Kevin Paulisse

lib/octocatalog-diff/catalog-diff/differ.rb

@@ -147,12 +147,14 @@ def catdiff
         # Remove resources that have been explicitly ignored
         filter_diffs_for_ignored_items(result)
 
-        # If a file has ensure => absent, there are certain parameters that don't matter anymore. Filter
-        # out any such parameters from the result array.
-        filter_diffs_for_absent_files(result) if @opts[:suppress_absent_file_details]
+        # Legacy options which are now filters
+        @opts[:filters] ||= []
+        if @opts[:suppress_absent_file_details] && !@opts[:filters].include?('absent_file')
+          @opts[:filters] << 'AbsentFile'
+        end
 
         # Apply any additional pluggable filters.
-        OctocatalogDiff::CatalogDiff::Filter.apply_filters(result, @opts[:filters])
+        OctocatalogDiff::CatalogDiff::Filter.apply_filters(result, @opts[:filters]) if @opts[:filters].any?
 
         # That's it!
         @logger.debug "Exiting catdiff; change count: #{result.size}"
@@ -166,43 +168,6 @@ def filter_diffs_for_ignored_items(result)
         result.reject! { |item| ignored?(item) }
       end
 
-      # If a file has ensure => absent, there are certain parameters that don't matter anymore. Filter
-      # out any such parameters from the result array.
-      # @param result [Array] Diff result list (modified by this method)
-      def filter_diffs_for_absent_files(result)
-        @logger.debug "Entering filter_diffs_for_absent_files with #{result.size} diffs"
-
-        # Scan for files in the result that are file resources with ensure => absent.
-        absent_files = Set.new
-        result.each do |diff|
-          next unless diff[0] == '~' || diff[0] == '!'
-          next unless diff[1] =~ /^File\f([^\f]+)\fparameters\fensure$/
-          next unless ['absent', 'false', false].include?(diff[3])
-          absent_files.add Regexp.last_match(1)
-        end
-
-        # If there are any absent files, remove all diffs referencing that file, except for
-        # the change to 'ensure'.
-        if absent_files.any?
-          keep = %w(ensure backup force provider)
-          result.map! do |diff|
-            if (diff[0] == '!' || diff[0] == '~') && diff[1] =~ /^File\f(.+)\fparameters\f(.+)$/
-              if absent_files.include?(Regexp.last_match(1)) && !keep.include?(Regexp.last_match(2))
-                @logger.debug "Removing file=#{Regexp.last_match(1)} parameter=#{Regexp.last_match(2)} for absent file"
-                nil
-              else
-                diff
-              end
-            else
-              diff
-            end
-          end
-          result.compact!
-        end
-
-        @logger.debug "Exiting filter_diffs_for_absent_files with #{result.size} diffs"
-      end
-
       # Pre-processing of a catalog.
       # - Remove 'before' and 'require' from parameters
       # - Sort 'tags' array, or remove the tags array if tags are being ignored

lib/octocatalog-diff/catalog-diff/filter.rb

@@ -1,3 +1,4 @@
+require_relative 'filter/absent_file'
 require_relative 'filter/yaml'
 
 module OctocatalogDiff
@@ -24,14 +25,19 @@ def self.apply_filters(result, filter_names, options = {})
       # @param options [Hash] Additional options (optional) to pass to filtered? method
       def self.filter(result, filter_class_name, options = {})
         filter_class_name = [name.to_s, filter_class_name].join('::')
-        clazz = Kernel.const_get(filter_class_name)
-        result.reject! { |item| clazz.filtered?(item, options) }
+        obj = Kernel.const_get(filter_class_name).new(result)
+        result.reject! { |item| obj.filtered?(item, options) }
+      end
+
+      # Inherited: Constructor that does nothing. Some filters require working on the entire
+      # data set and will override this method to perform some pre-processing for efficiency.
+      def initialize(_diff_array = [])
       end
 
       # Inherited: Construct a default `filtered?` method for the subclass via inheritance.
       # Each subclass must implement this method, so the default method errors.
-      def self.filtered?(_item, _options = {})
-        raise "No `filtered?` method is implemented in #{name}"
+      def filtered?(_item, _options = {})
+        raise "No `filtered?` method is implemented in #{self.class}"
       end
     end
   end

lib/octocatalog-diff/catalog-diff/filter/absent_file.rb

@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+require 'set'
+
+module OctocatalogDiff
+  module CatalogDiff
+    class Filter
+      # Filter out changes in parameters when the "to" resource has ensure => absent.
+      class AbsentFile < OctocatalogDiff::CatalogDiff::Filter
+        # Constructor: Since this filter requires knowledge of the entire array of diffs,
+        # override the inherited method to store those diffs in an instance variable.
+        def initialize(diffs)
+          @diffs = diffs
+          @results = nil
+        end
+
+        # Public: If a file has ensure => absent, there are certain parameters that don't
+        # matter anymore. Filter out any such parameters from the result array.
+        # Return true if the difference is in a resource where `ensure => absent` has been
+        # declared. Return false if they this is not the case.
+        #
+        # @param diff [internal diff format] Difference
+        # @param _options [Hash] Additional options (there are none for this filter)
+        # @return [Boolean] true if this difference is a YAML file with identical objects, false otherwise
+        def filtered?(diff, _options = {})
+          build_results if @results.nil?
+          @results.include?(diff)
+        end
+
+        private
+
+        # Private: The first time `.filtered?` is called, build up the cache of results.
+        # Returns nothing, but populates @results.
+        def build_results
+          # Which files can we ignore?
+          @files_to_ignore = Set.new
+          @diffs.each do |diff|
+            next unless diff[0] == '~' || diff[0] == '!'
+            next unless diff[1] =~ /^File\f([^\f]+)\fparameters\fensure$/
+            next unless ['absent', 'false', false].include?(diff[3])
+            @files_to_ignore.add Regexp.last_match(1)
+          end
+
+          # Based on that, which diffs can we ignore?
+          @results = Set.new @diffs.reject { |diff| keep_diff?(diff) }
+        end
+
+        # Private: Determine whether to keep a particular diff.
+        # @param diff [OctocatalogDiff::API::V1::Diff] Difference under consideration
+        # @return [Boolean] true = keep, false = discard
+        def keep_diff?(diff)
+          keep = %w(ensure backup force provider)
+          if (diff[0] == '!' || diff[0] == '~') && diff[1] =~ /^File\f(.+)\fparameters\f(.+)$/
+            if @files_to_ignore.include?(Regexp.last_match(1)) && !keep.include?(Regexp.last_match(2))
+              return false
+            end
+          end
+          true
+        end
+      end
+    end
+  end
+end

lib/octocatalog-diff/catalog-diff/filter/yaml.rb

@@ -14,7 +14,7 @@ class YAML < OctocatalogDiff::CatalogDiff::Filter
         # @param diff [Array] Difference
         # @param _options [Hash] Additional options (there are none for this filter)
         # @return [Boolean] true if this difference is a YAML file with identical objects, false otherwise
-        def self.filtered?(diff, _options = {})
+        def filtered?(diff, _options = {})
           # Skip additions or removals - focus only on changes
           return false unless diff[0] == '~' || diff[0] == '!'
 

spec/octocatalog-diff/tests/catalog-diff/differ_spec.rb

@@ -1251,35 +1251,4 @@
       expect(result[0]).to eq(['!', "Class\fOpenssl::Package\fparameters\fcommon-array", [1, 2, 3], [1, 5, 25], fileref, fileref])
     end
   end
-
-  describe '#filter_diffs_for_absent_files' do
-    before(:each) do
-      empty_puppet_catalog_json = File.read(OctocatalogDiff::Spec.fixture_path('catalogs/catalog-empty.json'))
-      empty_puppet_catalog = OctocatalogDiff::Catalog.new(json: empty_puppet_catalog_json)
-      logger, @logger_str = OctocatalogDiff::Spec.setup_logger
-      @obj = OctocatalogDiff::CatalogDiff::Differ.new({ logger: logger }, empty_puppet_catalog, empty_puppet_catalog)
-      @orig = [
-        ['~', "File\f/tmp/foo\fparameters\fensure", 'file', 'absent'],
-        ['~', "File\f/tmp/foo\fparameters\fowner", 'root', 'nobody'],
-        ['~', "File\f/tmp/foo\fparameters\fbackup", true, nil],
-        ['~', "File\f/tmp/foo\fparameters\fforce", false, nil],
-        ['~', "File\f/tmp/foo\fparameters\fprovider", 'root', nil],
-        ['~', "File\f/tmp/bar\fparameters\fensure", 'file', 'link'],
-        ['~', "File\f/tmp/bar\fparameters\ftarget", nil, '/tmp/foo'],
-        ['~', "Exec\f/tmp/bar\fparameters\fcommand", nil, '/tmp/foo']
-      ]
-      @result = @orig.dup
-      @obj.send(:filter_diffs_for_absent_files, @result)
-    end
-
-    it 'should filter out some attributes for ensure=>absent file' do
-      expect(@result).to eq(@orig.values_at(0, 2, 3, 4, 5, 6, 7))
-    end
-
-    it 'should log messages' do
-      expect(@logger_str.string).to match(/Entering filter_diffs_for_absent_files with 8 diffs/)
-      expect(@logger_str.string).to match(%r{Removing file=/tmp/foo parameter=owner for absent file})
-      expect(@logger_str.string).to match(/Exiting filter_diffs_for_absent_files with 7 diffs/)
-    end
-  end
 end

spec/octocatalog-diff/tests/catalog-diff/filter/absent_file_spec.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+require_relative '../../spec_helper'
+require OctocatalogDiff::Spec.require_path('/catalog')
+require OctocatalogDiff::Spec.require_path('/catalog-diff/filter/absent_file')
+
+describe OctocatalogDiff::CatalogDiff::Filter::AbsentFile do
+  it 'should filter out some attributes for ensure=>absent file' do
+    orig = [
+      ['~', "File\f/tmp/foo\fparameters\fensure", 'file', 'absent'],
+      ['~', "File\f/tmp/foo\fparameters\fowner", 'root', 'nobody'],
+      ['~', "File\f/tmp/foo\fparameters\fbackup", true, nil],
+      ['~', "File\f/tmp/foo\fparameters\fforce", false, nil],
+      ['~', "File\f/tmp/foo\fparameters\fprovider", 'root', nil],
+      ['~', "File\f/tmp/bar\fparameters\fensure", 'file', 'link'],
+      ['~', "File\f/tmp/bar\fparameters\ftarget", nil, '/tmp/foo'],
+      ['~', "Exec\f/tmp/bar\fparameters\fcommand", nil, '/tmp/foo']
+    ]
+    testobj = described_class.new(orig)
+    result = orig.reject { |x| testobj.filtered?(x) }
+    expect(result).to eq(orig.values_at(0, 2, 3, 4, 5, 6, 7))
+  end
+end

spec/octocatalog-diff/tests/catalog-diff/filter/yaml_spec.rb

@@ -4,70 +4,72 @@
 require OctocatalogDiff::Spec.require_path('/catalog-diff/filter/yaml')
 
 describe OctocatalogDiff::CatalogDiff::Filter::YAML do
+  let(:subject) { described_class.new }
+
   describe '#filtered?' do
     let(:str1a) { "---\n  foo: bar" }
     let(:str1b) { "---\nfoo: bar" }
     let(:str2) { "---\n  foo: baz" }
 
     it 'should not filter out an added resource' do
       diff = ['+', "File\ffoobar.yaml", { 'parameters' => { 'content' => str1a } }]
-      result = described_class.filtered?(diff)
+      result = subject.filtered?(diff)
       expect(result).to eq(false)
     end
 
     it 'should not filter out a removed resource' do
       diff = ['-', "File\ffoobar.yaml", { 'parameters' => { 'content' => str1a } }]
-      result = described_class.filtered?(diff)
+      result = subject.filtered?(diff)
       expect(result).to eq(false)
     end
 
     it 'should not filter out a non-file resource' do
       diff = ['~', "Exec\ffoobar.yaml\fparameters\fcontent", str1a, str1b]
-      result = described_class.filtered?(diff)
+      result = subject.filtered?(diff)
       expect(result).to eq(false)
     end
 
     it 'should not filter out a file whose extension is not .yaml / .yml' do
       diff = ['~', "File\ffoobar.json\fparameters\fcontent", str1a, str1b]
-      result = described_class.filtered?(diff)
+      result = subject.filtered?(diff)
       expect(result).to eq(false)
     end
 
     it 'should not filter out a change with no content change' do
       diff = ['~', "File\ffoobar.json\fparameters\fowner", 'root', 'nobody']
-      result = described_class.filtered?(diff)
+      result = subject.filtered?(diff)
       expect(result).to eq(false)
     end
 
     it 'should not filter out a change where YAML objects are dissimilar' do
       diff = ['~', "File\ffoobar.yaml\fparameters\fcontent", str1a, str2]
-      result = described_class.filtered?(diff)
+      result = subject.filtered?(diff)
       expect(result).to eq(false)
     end
 
     it 'should not filter out a change where YAML is invalid' do
       x_str = '---{ "blah": "foo" }'
       diff = ['~', "File\ffoobar.yaml\fparameters\fcontent", x_str, x_str]
-      result = described_class.filtered?(diff)
+      result = subject.filtered?(diff)
       expect(result).to eq(false)
     end
 
     it 'should not filter out a change where YAML is unparseable' do
       x_str = "--- !ruby/object:This::Does::Not::Exist\n  foo: bar"
       diff = ['~', "File\ffoobar.yaml\fparameters\fcontent", x_str, x_str]
-      result = described_class.filtered?(diff)
+      result = subject.filtered?(diff)
       expect(result).to eq(false)
     end
 
     it 'should filter out a whitespace-only change to a .yaml file' do
       diff = ['~', "File\ffoobar.yaml\fparameters\fcontent", str1a, str1b]
-      result = described_class.filtered?(diff)
+      result = subject.filtered?(diff)
       expect(result).to eq(true)
     end
 
     it 'should filter out a whitespace-only change to a .yml file' do
       diff = ['~', "File\ffoobar.yml\fparameters\fcontent", str1a, str1b]
-      result = described_class.filtered?(diff)
+      result = subject.filtered?(diff)
       expect(result).to eq(true)
     end
   end

spec/octocatalog-diff/tests/catalog-diff/filter_spec.rb

@@ -3,10 +3,18 @@
 require_relative '../spec_helper'
 require OctocatalogDiff::Spec.require_path('/catalog-diff/filter')
 
+# rubocop:disable Style/ClassAndModuleChildren
+class OctocatalogDiff::CatalogDiff::Filter::Fake1 < OctocatalogDiff::CatalogDiff::Filter
+end
+
+class OctocatalogDiff::CatalogDiff::Filter::Fake2 < OctocatalogDiff::CatalogDiff::Filter
+end
+# rubocop:enable Style/ClassAndModuleChildren
+
 describe OctocatalogDiff::CatalogDiff::Filter do
   before(:each) do
-    @class_1 = double
-    @class_2 = double
+    @class_1 = OctocatalogDiff::CatalogDiff::Filter::Fake1
+    @class_2 = OctocatalogDiff::CatalogDiff::Filter::Fake2
     allow(Kernel).to receive(:const_get).with('OctocatalogDiff::CatalogDiff::Filter::Fake1').and_return(@class_1)
     allow(Kernel).to receive(:const_get).with('OctocatalogDiff::CatalogDiff::Filter::Fake2').and_return(@class_2)
   end
@@ -16,8 +24,8 @@
       result = [false]
       options = { 'Fake1' => { foo: 'bar' } }
       classes = %w(Fake1 Fake2)
-      expect(@class_1).to receive(:'filtered?').with(false, foo: 'bar').and_return(false)
-      expect(@class_2).to receive(:'filtered?').with(false, {}).and_return(false)
+      expect_any_instance_of(@class_1).to receive(:'filtered?').with(false, foo: 'bar').and_return(false)
+      expect_any_instance_of(@class_2).to receive(:'filtered?').with(false, {}).and_return(false)
       expect { described_class.apply_filters(result, classes, options) }.not_to raise_error
       expect(result).to eq([false])
     end
@@ -26,16 +34,17 @@
   describe '#filter' do
     it 'should call .filtered?() in a class and remove matching items' do
       result = [false, true]
-      expect(@class_1).to receive(:'filtered?').with(false, {}).and_return(false)
-      expect(@class_1).to receive(:'filtered?').with(true, {}).and_return(true)
+      expect_any_instance_of(@class_1).to receive(:'filtered?').with(false, {}).and_return(false)
+      expect_any_instance_of(@class_1).to receive(:'filtered?').with(true, {}).and_return(true)
       expect { described_class.filter(result, 'Fake1') }.not_to raise_error
       expect(result).to eq([false])
     end
   end
 
   describe '#filtered?' do
     it 'should raise error' do
-      expect { described_class.filtered?([]) }.to raise_error(RuntimeError, /No `filtered\?` method is implemented/)
+      testobj = described_class.new
+      expect { testobj.filtered?([]) }.to raise_error(RuntimeError, /No `filtered\?` method is implemented/)
     end
   end
 end