Identify where was the broken reference was declared

lib/octocatalog-diff/catalog.rb

@@ -201,29 +201,56 @@ def validate_references
       end
       return if missing.empty?
 
-      # At this point there is at least one broken/missing reference. Format an error message and
-      # raise. Error message will look like this:
-      # ---
-      # Catalog has broken references: exec[subscribe caller 1] -> subscribe[Exec[subscribe target]];
-      # exec[subscribe caller 2] -> subscribe[Exec[subscribe target]]; exec[subscribe caller 2] ->
-      # subscribe[Exec[subscribe target 2]]
-      # ---
+      # At this point there is at least one broken/missing reference. Format an error message and raise.
+      errors = format_missing_references(missing)
+      plural = errors =~ /;/ ? 's' : ''
+      raise OctocatalogDiff::Errors::ReferenceValidationError, "Catalog has broken reference#{plural}: #{errors}"
+    end
+
+    private
+
+    # Private method: Format the name of the source file and line number, based on compilation directory and
+    # other settings. This is used by format_missing_references.
+    # @param source_file [String] Raw source file name from catalog
+    # @param line_number [Fixnum] Line number from catalog
+    # @return [String] Formatted source file
+    def format_source_file_line(source_file, line_number)
+      return '' if source_file.nil? || source_file.empty?
+      filename = if compilation_dir && source_file.start_with?(compilation_dir)
+        stripped_file = source_file[compilation_dir.length..-1]
+        stripped_file.start_with?('/') ? stripped_file[1..-1] : stripped_file
+      else
+        source_file
+      end
+      "(#{filename.sub(%r{^environments/production/}, '')}:#{line_number})"
+    end
+
+    # Private method: Format the missing references into human-readable text
+    # Error message will look like this:
+    # ---
+    # Catalog has broken references: exec[subscribe caller 1](file:line) -> subscribe[Exec[subscribe target]];
+    # exec[subscribe caller 2](file:line) -> subscribe[Exec[subscribe target]]; exec[subscribe caller 2](file:line) ->
+    # subscribe[Exec[subscribe target 2]]
+    # ---
+    # @param missing [Array] Array of missing references
+    # @return [String] Formatted references
+    def format_missing_references(missing)
+      unless missing.is_a?(Array) && missing.any?
+        raise ArgumentError, 'format_missing_references() requires a non-empty array as input'
+      end
+
       formatted_references = missing.map do |obj|
         # obj[:target_value] can be a string or an array. If it's an array, create a
         # separate error message per element of that array. This allows the total number
         # of errors to be correct.
-        src = "#{obj[:source]['type'].downcase}[#{obj[:source]['title']}]"
+        src_ref = "#{obj[:source]['type'].downcase}[#{obj[:source]['title']}]"
+        src_file = format_source_file_line(obj[:source]['file'], obj[:source]['line'])
         target_val = obj[:target_value].is_a?(Array) ? obj[:target_value] : [obj[:target_value]]
-        target_val.map { |tv| "#{src} -> #{obj[:target_type].downcase}[#{tv}]" }
-      end
-      formatted_references.flatten!
-      plural = formatted_references.size == 1 ? '' : 's'
-      errors = formatted_references.join('; ')
-      raise OctocatalogDiff::Errors::ReferenceValidationError, "Catalog has broken reference#{plural}: #{errors}"
+        target_val.map { |tv| "#{src_ref}#{src_file} -> #{obj[:target_type].downcase}[#{tv}]" }
+      end.flatten
+      formatted_references.join('; ')
     end
 
-    private
-
     # Private method: Given a list of resources to check, return the references from
     # that list that are missing from the catalog. (An empty array returned would indicate
     # all references are present in the catalog.)

spec/octocatalog-diff/integration/reference_validation_spec.rb

@@ -102,14 +102,17 @@ def self.catalog_contains_resource(result, type, title)
       expect(@result.exception).to be_a_kind_of(OctocatalogDiff::Errors::ReferenceValidationError)
     end
 
+    # Multiple line numbers given because Puppet 4.x and 3.8 correspond to first and last line of resource, respectively.
+    # rubocop:disable Metrics/LineLength
     it 'should have formatted error messages' do
       msg = @result.exception.message
-      expect(msg).to match(/exec\[subscribe caller 1\] -> subscribe\[Exec\[subscribe target\]\]/)
-      expect(msg).to match(/exec\[subscribe caller 2\] -> subscribe\[Exec\[subscribe target\]\]/)
-      expect(msg).to match(/exec\[subscribe caller 2\] -> subscribe\[Exec\[subscribe target 2\]\]/)
-      expect(msg).to match(/exec\[subscribe caller 3\] -> subscribe\[Exec\[subscribe target\]\]/)
-      expect(msg).not_to match(/exec\[subscribe caller 3\] -> subscribe\[Exec\[subscribe caller 1\]\]/)
+      expect(msg).to match(%r{exec\[subscribe caller 1\]\(modules/test/manifests/subscribe_callers.pp:(2|5)\) -> subscribe\[Exec\[subscribe target\]\]})
+      expect(msg).to match(%r{exec\[subscribe caller 2\]\(modules/test/manifests/subscribe_callers.pp:(7|13)\) -> subscribe\[Exec\[subscribe target\]\]})
+      expect(msg).to match(%r{exec\[subscribe caller 2\]\(modules/test/manifests/subscribe_callers.pp:(7|13)\) -> subscribe\[Exec\[subscribe target 2\]\]})
+      expect(msg).to match(%r{exec\[subscribe caller 3\]\(modules/test/manifests/subscribe_callers.pp:(15|21)\) -> subscribe\[Exec\[subscribe target\]\]})
+      expect(msg).not_to match(/exec\[subscribe caller 3\].+subscribe\[Exec\[subscribe caller 1\]\]/)
     end
+    # rubocop:enable Metrics/LineLength
   end
 
   context 'with broken before' do
@@ -125,10 +128,12 @@ def self.catalog_contains_resource(result, type, title)
       expect(@result.exception).to be_a_kind_of(OctocatalogDiff::Errors::ReferenceValidationError)
     end
 
+    # rubocop:disable Metrics/LineLength
     it 'should have formatted error messages' do
       msg = @result.exception.message
-      expect(msg).to eq('Catalog has broken reference: exec[before caller] -> before[Exec[before target]]')
+      expect(msg).to match(%r{Catalog has broken reference: exec\[before caller\]\(modules/test/manifests/before_callers.pp:(2|5)\) -> before\[Exec\[before target\]\]})
     end
+    # rubocop:enable Metrics/LineLength
   end
 
   context 'with broken notify' do
@@ -144,10 +149,12 @@ def self.catalog_contains_resource(result, type, title)
       expect(@result.exception).to be_a_kind_of(OctocatalogDiff::Errors::ReferenceValidationError)
     end
 
+    # rubocop:disable Metrics/LineLength
     it 'should have formatted error messages' do
       msg = @result.exception.message
-      expect(msg).to match(/exec\[notify caller\] -> notify\[Test::Foo::Bar\[notify target\]\]/)
+      expect(msg).to match(%r{exec\[notify caller\]\(modules/test/manifests/notify_callers.pp:(2|4)\) -> notify\[Test::Foo::Bar\[notify target\]\]})
     end
+    # rubocop:enable Metrics/LineLength
   end
 
   context 'with broken require' do
@@ -163,14 +170,16 @@ def self.catalog_contains_resource(result, type, title)
       expect(@result.exception).to be_a_kind_of(OctocatalogDiff::Errors::ReferenceValidationError)
     end
 
+    # rubocop:disable Metrics/LineLength
     it 'should have formatted error messages' do
       msg = @result.exception.message
-      expect(msg).to match(/exec\[require caller\] -> require\[Exec\[require target\]\]/)
-      expect(msg).to match(/exec\[require caller 3\] -> require\[Exec\[require target\]\]/)
-      expect(msg).to match(/exec\[require caller 4\] -> require\[Exec\[require target\]\]/)
+      expect(msg).to match(%r{exec\[require caller\]\(modules/test/manifests/require_callers.pp:(2|5)\) -> require\[Exec\[require target\]\]})
+      expect(msg).to match(%r{exec\[require caller 3\]\(modules/test/manifests/require_callers.pp:(12|18)\) -> require\[Exec\[require target\]\]})
+      expect(msg).to match(%r{exec\[require caller 4\]\(modules/test/manifests/require_callers.pp:(12|18)\) -> require\[Exec\[require target\]\]})
       expect(msg).not_to match(/exec\[require caller 2\]/)
       expect(msg).not_to match(/-> require\[Exec\[require caller\]\]/)
     end
+    # rubocop:enable Metrics/LineLength
   end
 
   context 'with broken subscribe but subscribe not checked' do
@@ -223,13 +232,15 @@ def self.catalog_contains_resource(result, type, title)
       expect(@result.exception).to be_a_kind_of(OctocatalogDiff::Errors::ReferenceValidationError)
     end
 
+    # rubocop:disable Metrics/LineLength
     it 'should have formatted error messages' do
       msg = @result.exception.message
-      expect(msg).to match(/exec\[before alias caller\] -> before\[Exec\[before alias target\]\]/)
-      expect(msg).to match(/exec\[notify alias caller\] -> before\[Exec\[notify alias target\]\]/)
-      expect(msg).to match(/exec\[require alias caller\] -> before\[Exec\[require alias target\]\]/)
-      expect(msg).to match(/exec\[subscribe alias caller\] -> before\[Exec\[subscribe alias target\]\]/)
+      expect(msg).to match(%r{exec\[before alias caller\]\(modules/test/manifests/alias_callers.pp:(2|5)\) -> before\[Exec\[before alias target\]\]})
+      expect(msg).to match(%r{exec\[notify alias caller\]\(modules/test/manifests/alias_callers.pp:(7|10)\) -> before\[Exec\[notify alias target\]\]})
+      expect(msg).to match(%r{exec\[require alias caller\]\(modules/test/manifests/alias_callers.pp:(12|15)\) -> before\[Exec\[require alias target\]\]})
+      expect(msg).to match(%r{exec\[subscribe alias caller\]\(modules/test/manifests/alias_callers.pp:(17|20)\) -> before\[Exec\[subscribe alias target\]\]})
     end
+    # rubocop:enable Metrics/LineLength
   end
 end
 
@@ -277,13 +288,15 @@ def self.catalog_contains_resource(result, type, title)
       expect(@result.exception).to be_a_kind_of(OctocatalogDiff::Errors::ReferenceValidationError)
     end
 
+    # rubocop:disable Metrics/LineLength
     it 'should have formatted error messages' do
       msg = @result.exception.message
-      expect(msg).to match(/exec\[subscribe caller 1\] -> subscribe\[Exec\[subscribe target\]\]/)
-      expect(msg).to match(/exec\[subscribe caller 2\] -> subscribe\[Exec\[subscribe target\]\]/)
-      expect(msg).to match(/exec\[subscribe caller 2\] -> subscribe\[Exec\[subscribe target 2\]\]/)
-      expect(msg).to match(/exec\[subscribe caller 3\] -> subscribe\[Exec\[subscribe target\]\]/)
+      expect(msg).to match(%r{exec\[subscribe caller 1\]\(.+/modules/test/manifests/subscribe_callers.pp:(2|5)\) -> subscribe\[Exec\[subscribe target\]\]})
+      expect(msg).to match(%r{exec\[subscribe caller 2\]\(.+/modules/test/manifests/subscribe_callers.pp:(7|13)\) -> subscribe\[Exec\[subscribe target\]\]})
+      expect(msg).to match(%r{exec\[subscribe caller 2\]\(.+/modules/test/manifests/subscribe_callers.pp:(7|13)\) -> subscribe\[Exec\[subscribe target 2\]\]})
+      expect(msg).to match(%r{exec\[subscribe caller 3\]\(.+/modules/test/manifests/subscribe_callers.pp:(15|21)\) -> subscribe\[Exec\[subscribe target\]\]})
     end
+    # rubocop:enable Metrics/LineLength
   end
 
   context 'with broken references in from-catalog' do
@@ -321,14 +334,16 @@ def self.catalog_contains_resource(result, type, title)
       expect(@result.exception).to be_a_kind_of(OctocatalogDiff::Errors::ReferenceValidationError)
     end
 
+    # rubocop:disable Metrics/LineLength
     it 'should have formatted error messages from to-catalog only' do
       msg = @result.exception.message
-      expect(msg).to match(/exec\[subscribe caller 1\] -> subscribe\[Exec\[subscribe target\]\]/)
-      expect(msg).to match(/exec\[subscribe caller 2\] -> subscribe\[Exec\[subscribe target\]\]/)
-      expect(msg).to match(/exec\[subscribe caller 2\] -> subscribe\[Exec\[subscribe target 2\]\]/)
-      expect(msg).to match(/exec\[subscribe caller 3\] -> subscribe\[Exec\[subscribe target\]\]/)
+      expect(msg).to match(%r{exec\[subscribe caller 1\]\(.+/modules/test/manifests/subscribe_callers.pp:(2|5)\) -> subscribe\[Exec\[subscribe target\]\]})
+      expect(msg).to match(%r{exec\[subscribe caller 2\]\(.+/modules/test/manifests/subscribe_callers.pp:(7|13)\) -> subscribe\[Exec\[subscribe target\]\]})
+      expect(msg).to match(%r{exec\[subscribe caller 2\]\(.+/modules/test/manifests/subscribe_callers.pp:(7|13)\) -> subscribe\[Exec\[subscribe target 2\]\]})
+      expect(msg).to match(%r{exec\[subscribe caller 3\]\(.+/modules/test/manifests/subscribe_callers.pp:(15|21)\) -> subscribe\[Exec\[subscribe target\]\]})
       expect(msg).not_to match(/require target/)
     end
+    # rubocop:enable Metrics/LineLength
   end
 
   context 'with broken references, but checking not enabled' do

spec/octocatalog-diff/tests/catalog_spec.rb

@@ -471,16 +471,125 @@
         json: File.read(OctocatalogDiff::Spec.fixture_path('catalogs/reference-validation-broken.json'))
       }
       catalog = OctocatalogDiff::Catalog.new(opts)
+      catalog.compilation_dir = '/var/folders/dw/5ftmkqk972j_kw2fdjyzdqdw0000gn/T/d20161223-46780-x10xaf/environments/production'
       error_str = [
-        'Catalog has broken references: exec[subscribe caller 1] -> subscribe[Exec[subscribe target]]',
-        'exec[subscribe caller 2] -> subscribe[Exec[subscribe target]]',
-        'exec[subscribe caller 2] -> subscribe[Exec[subscribe target 2]]',
-        'exec[subscribe caller 3] -> subscribe[Exec[subscribe target]]'
+        'Catalog has broken references: exec[subscribe caller 1](modules/test/manifests/subscribe_callers.pp:2)' \
+          ' -> subscribe[Exec[subscribe target]]',
+        'exec[subscribe caller 2](modules/test/manifests/subscribe_callers.pp:7) -> subscribe[Exec[subscribe target]]',
+        'exec[subscribe caller 2](modules/test/manifests/subscribe_callers.pp:7) -> subscribe[Exec[subscribe target 2]]',
+        'exec[subscribe caller 3](modules/test/manifests/subscribe_callers.pp:15) -> subscribe[Exec[subscribe target]]'
       ].join('; ')
       expect { catalog.validate_references }.to raise_error(OctocatalogDiff::Errors::ReferenceValidationError, error_str)
     end
   end
 
+  describe '#format_missing_references' do
+    before(:each) do
+      opts = { json: File.read(OctocatalogDiff::Spec.fixture_path('catalogs/reference-validation-broken.json')) }
+      @test_obj = OctocatalogDiff::Catalog.new(opts)
+    end
+
+    context 'with invalid input' do
+      it 'should raise ArgumentError if non-array is provided' do
+        expect do
+          @test_obj.send(:format_missing_references, 'Hi there')
+        end.to raise_error(ArgumentError, /format_missing_references\(\) requires a non-empty array as input/)
+      end
+
+      it 'should raise ArgumentError if empty array is provided' do
+        expect do
+          @test_obj.send(:format_missing_references, [])
+        end.to raise_error(ArgumentError, /format_missing_references\(\) requires a non-empty array as input/)
+      end
+    end
+
+    context 'with compilation directory specified and matching' do
+      it 'should strip compilation directory' do
+        allow(@test_obj).to receive(:compilation_dir)
+          .and_return('/var/folders/dw/foo/environments/production')
+        obj = {
+          source: {
+            'file' => '/var/folders/dw/foo/environments/production/modules/foo/manifests/bar.pp',
+            'line' => 23,
+            'type' => 'Baz',
+            'title' => 'buzz'
+          },
+          target_type: 'Foo',
+          target_value: 'bar'
+        }
+        result = @test_obj.send(:format_missing_references, [obj])
+        expect(result).to eq('baz[buzz](modules/foo/manifests/bar.pp:23) -> foo[bar]')
+      end
+    end
+
+    context 'with compilation directory specified and not matching' do
+      it 'should not strip compilation directory' do
+        allow(@test_obj).to receive(:compilation_dir)
+          .and_return('/var/folders/dw/bar/environments/production')
+        obj = {
+          source: {
+            'file' => '/var/folders/dw/foo/environments/production/modules/foo/manifests/bar.pp',
+            'line' => 23,
+            'type' => 'Baz',
+            'title' => 'buzz'
+          },
+          target_type: 'Foo',
+          target_value: 'bar'
+        }
+        result = @test_obj.send(:format_missing_references, [obj])
+        expect(result).to eq('baz[buzz](/var/folders/dw/foo/environments/production/modules/foo/manifests/bar.pp:23) -> foo[bar]')
+      end
+    end
+
+    context 'with compilation directory not specified' do
+      it 'should not strip compilation directory' do
+        allow(@test_obj).to receive(:compilation_dir).and_return(nil)
+        obj = {
+          source: {
+            'file' => '/var/folders/dw/foo/environments/production/modules/foo/manifests/bar.pp',
+            'line' => 23,
+            'type' => 'Baz',
+            'title' => 'buzz'
+          },
+          target_type: 'Foo',
+          target_value: 'bar'
+        }
+        result = @test_obj.send(:format_missing_references, [obj])
+        expect(result).to eq('baz[buzz](/var/folders/dw/foo/environments/production/modules/foo/manifests/bar.pp:23) -> foo[bar]')
+      end
+    end
+
+    context 'with multiple targets for the same resource' do
+      it 'should display each target separately' do
+        allow(@test_obj).to receive(:compilation_dir).and_return(nil)
+        src = {
+          'file' => '/var/folders/dw/foo/environments/production/modules/foo/manifests/bar.pp',
+          'line' => 23,
+          'type' => 'Baz',
+          'title' => 'buzz'
+        }
+        obj = [
+          {
+            source: src,
+            target_type: 'Foo',
+            target_value: 'bar'
+          },
+          {
+            source: src,
+            target_type: 'Fizz',
+            target_value: 'buzz'
+          }
+        ]
+        answer = [
+          'baz[buzz](/var/folders/dw/foo/environments/production/modules/foo/manifests/bar.pp:23) -> foo[bar]',
+          'baz[buzz](/var/folders/dw/foo/environments/production/modules/foo/manifests/bar.pp:23) -> fizz[buzz]'
+        ].join('; ')
+        result = @test_obj.send(:format_missing_references, obj)
+        expect(result).to eq(answer)
+      end
+    end
+  end
+
   describe '#build_resource_hash' do
     before(:each) do
       resource_array = [