Merge pull request #65 from github/kpaulisse-filter-validation

Filter validation

Author: kpaulisse

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

@@ -10,6 +10,24 @@ module CatalogDiff
     class Filter
       attr_accessor :logger
 
+      # List the available filters here (by class name) for use in the validator method.
+      AVAILABLE_FILTERS = %w(AbsentFile CompilationDir YAML).freeze
+
+      # Public: Determine whether a particular filter exists. This can be used to validate
+      # a user-submitted filter.
+      # @param filter_name [String] Proposed filter name
+      # @return [Boolean] True if filter is valid; false otherwise
+      def self.filter?(filter_name)
+        AVAILABLE_FILTERS.include?(filter_name)
+      end
+
+      # Public: Assert that a filter exists, and raise an error if it does not.
+      # @param filter_name [String] Proposed filter name
+      def self.assert_that_filter_exists(filter_name)
+        return if filter?(filter_name)
+        raise ArgumentError, "The filter #{filter_name} is not valid"
+      end
+
       # Public: Apply multiple filters by repeatedly calling the `filter` method for each
       # filter in an array. This method returns nothing.
       #
@@ -29,6 +47,7 @@ def self.apply_filters(result, filter_names, options = {})
       # @param filter_class_name [String] Filter class name (from `filter` subdirectory)
       # @param options [Hash] Additional options (optional) to pass to filtered? method
       def self.filter(result, filter_class_name, options = {})
+        assert_that_filter_exists(filter_class_name)
         filter_class_name = [name.to_s, filter_class_name].join('::')
         obj = Kernel.const_get(filter_class_name).new(result, options[:logger])
         result.reject! { |item| obj.filtered?(item, options) }

lib/octocatalog-diff/cli/options/filters.rb

@@ -12,6 +12,9 @@ def parse(parser, options)
     parser.on('--filters FILTER1[,FILTER2[,...]]', Array, 'Filters to apply') do |x|
       options[:filters] ||= []
       options[:filters].concat x
+
+      require_relative '../../catalog-diff/filter'
+      options[:filters].each { |filter| OctocatalogDiff::CatalogDiff::Filter.assert_that_filter_exists(filter) }
     end
   end
 end

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

@@ -19,11 +19,35 @@ class OctocatalogDiff::CatalogDiff::Filter::Fake2 < OctocatalogDiff::CatalogDiff
     allow(Kernel).to receive(:const_get).with('OctocatalogDiff::CatalogDiff::Filter::Fake2').and_return(@class_2)
   end
 
+  describe '#filter?' do
+    it 'should return false for non-existent filter' do
+      expect(described_class.filter?('BlahBlahBlah')).to eq(false)
+    end
+
+    it 'should return true for valid filter' do
+      expect(described_class.filter?('AbsentFile')).to eq(true)
+    end
+  end
+
+  describe '#assert_that_filter_exists' do
+    it 'should raise error for non-existent filter' do
+      expect do
+        described_class.assert_that_filter_exists('BlahBlahBlah')
+      end.to raise_error(ArgumentError, 'The filter BlahBlahBlah is not valid')
+    end
+
+    it 'should not raise error for valid filter' do
+      expect { described_class.assert_that_filter_exists('AbsentFile') }.not_to raise_error
+    end
+  end
+
   describe '#apply_filters' do
     it 'should call self.filter() with appropriate options for each class' do
       result = [false]
       options = { foo: 'bar' }
       classes = %w(Fake1 Fake2)
+      allow(described_class).to receive(:"filter?").with('Fake1').and_return(true)
+      allow(described_class).to receive(:"filter?").with('Fake2').and_return(true)
       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, foo: 'bar').and_return(false)
       expect { described_class.apply_filters(result, classes, options) }.not_to raise_error
@@ -34,6 +58,8 @@ class OctocatalogDiff::CatalogDiff::Filter::Fake2 < OctocatalogDiff::CatalogDiff
   describe '#filter' do
     it 'should call .filtered?() in a class and remove matching items' do
       result = [false, true]
+      allow(described_class).to receive(:"filter?").with('Fake1').and_return(true)
+      allow(described_class).to receive(:"filter?").with('Fake2').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

spec/octocatalog-diff/tests/cli/options/filters_spec.rb

@@ -5,13 +5,19 @@
 describe OctocatalogDiff::Cli::Options do
   describe '#opt_ignore_equivalent_yaml_files' do
     it 'should accept comma delimited parameters for --filters' do
-      result = run_optparse(['--filters', 'fizzbuzz,barbuzz'])
-      expect(result[:filters]).to eq(%w(fizzbuzz barbuzz))
+      result = run_optparse(['--filters', 'AbsentFile,YAML'])
+      expect(result[:filters]).to eq(%w(AbsentFile YAML))
     end
 
     it 'should accept multiple parameters for --filters' do
-      result = run_optparse(['--filters', 'fizzbuzz', '--filters', 'barbuzz'])
-      expect(result[:filters]).to eq(%w(fizzbuzz barbuzz))
+      result = run_optparse(['--filters', 'AbsentFile', '--filters', 'YAML'])
+      expect(result[:filters]).to eq(%w(AbsentFile YAML))
+    end
+
+    it 'should raise ArgumentError if invalid filter is specified' do
+      expect do
+        run_optparse(['--filters', 'AbsentFile,FizzBuzzDoesNotExist,YAML'])
+      end.to raise_error(ArgumentError, 'The filter FizzBuzzDoesNotExist is not valid')
     end
   end
 end