Merge pull request #1038 from chef/ssd/dup-detect

Improve duplicate and cycle detection in resolver

lib/inspec/dependencies/resolver.rb

@@ -31,7 +31,23 @@ module Inspec
       new.resolve(reqs)
     end
 
-    def resolve(deps, top_level = true, seen_items = {}, path_string = '')
+    def detect_duplicates(deps, top_level, path_string)
+      seen_items_local = []
+      deps.each do |dep|
+        if seen_items_local.include?(dep.name)
+          problem_cookbook = if top_level
+                               'the inspec.yml for this profile.'
+                             else
+                               "the dependency information for #{path_string.split(' ').last}"
+                             end
+          fail Inspec::DuplicateDep, "The dependency #{dep.name} is listed twice in #{problem_cookbook}"
+        else
+          seen_items_local << dep.name
+        end
+      end
+    end
+
+    def resolve(deps, top_level = true, seen_items = {}, path_string = '') # rubocop:disable Metrics/AbcSize
       graph = {}
       if top_level
         Inspec::Log.debug("Starting traversal of dependencies #{deps.map(&:name)}")
@@ -39,17 +55,19 @@ module Inspec
         Inspec::Log.debug("Traversing dependency tree of transitive dependency #{deps.map(&:name)}")
       end
 
+      detect_duplicates(deps, top_level, path_string)
       deps.each do |dep|
-        path_string = if path_string.empty?
+        new_seen_items = seen_items.dup
+        new_path_string = if path_string.empty?
                             dep.name
                           else
                             path_string + " -> #{dep.name}"
                           end
 
-        if seen_items.key?(dep.resolved_source)
+        if new_seen_items.key?(dep.resolved_source)
-          fail Inspec::CyclicDependencyError, "Dependency #{dep} would cause a dependency cycle (#{path_string})"
+          fail Inspec::CyclicDependencyError, "Dependency #{dep} would cause a dependency cycle (#{new_path_string})"
         else
-          seen_items[dep.resolved_source] = true
+          new_seen_items[dep.resolved_source] = true
         end
 
         if !dep.source_satisfies_spec?
@@ -59,8 +77,7 @@ module Inspec
         Inspec::Log.debug("Adding dependency #{dep.name} (#{dep.resolved_source})")
         graph[dep.name] = dep
         if !dep.dependencies.empty?
-          # Recursively resolve any transitive dependencies.
+          resolve(dep.dependencies, false, new_seen_items.dup, new_path_string)
-          resolve(dep.dependencies, false, seen_items.dup, path_string)
         end
       end
 

lib/inspec/errors.rb

@@ -8,4 +8,5 @@ module Inspec
   # dependency resolution
   class CyclicDependencyError < Error; end
   class UnsatisfiedVersionSpecification < Error; end
+  class DuplicateDep < Error; end
 end

test/unit/dependencies/resolver_test.rb

@@ -11,6 +11,10 @@ class FakeDep
   def resolved_source
     { path: name }
   end
+
+  def source_satisfies_spec?
+    true
+  end
 end
 
 describe Inspec::Resolver do
@@ -21,6 +25,24 @@ describe Inspec::Resolver do
       subject.resolve([]).must_equal({})
     end
 
+    it "errors if a dependency is listed twice at the same level" do
+      dep = FakeDep.new("fake_dep_0")
+      lambda { subject.resolve([dep, dep]) }.must_raise Inspec::DuplicateDep
+    end
+
+    it "fails if there is a simple cycle " do
+      dep0 = FakeDep.new("fake_dep_0")
+      dep1 = FakeDep.new("fake_dep_1")
+      dep2 = FakeDep.new("fake_dep_2")
+
+      dep0.stubs(:dependencies).returns([dep1])
+
+      dep1.stubs(:dependencies).returns([dep2])
+      dep2.stubs(:dependencies).returns([dep1])
+      lambda { subject.resolve([dep0]) }.must_raise Inspec::CyclicDependencyError
+    end
+
+
     it "errors if the source version doesn't match the requirement" do
       dep = FakeDep.new("fake_dep_0")
       dep.expects(:source_satisfies_spec?).returns(false)