More work on refactoring of dependency solver:respect-version-deps

 * dependency_solver renamed to cookbook_version_selector
 * all dep_selector-dependent methods removed from RunList and moved
   to cookbook_version_selector
 * error messages handled differently; still have work to do for
   getting and translating compound messages from DepSelector-
   to Chef-land
 * nodes/:node_id/cookbooks refactored to use
   CookbookVersionSelector#expand_to_cookbook_version
 * tests written for current exception handling from
   /environments/:env_id/cookbook_versions

7 files changed, 228 insertions, 139 deletions

diff --git a/chef-server-api/Gemfile.lock b/chef-server-api/Gemfile.lock
index 4a321bd2bc..5c1c42d263 100644
--- a/chef-server-api/Gemfile.lock
+++ b/chef-server-api/Gemfile.lock
@@ -4,7 +4,6 @@ PATH
     chef (0.10.0.beta.0)
       bunny (>= 0.6.0)
       erubis
-      extlib
       highline
       json (>= 1.4.4, <= 1.4.6)
       mixlib-authentication (>= 1.1.0)
diff --git a/chef-server-api/app/controllers/environments.rb b/chef-server-api/app/controllers/environments.rb
index baa7a74276..05f2d3549f 100644
--- a/chef-server-api/app/controllers/environments.rb
+++ b/chef-server-api/app/controllers/environments.rb
@@ -18,6 +18,7 @@
 #
 
 require 'chef/environment'
+require 'chef/cookbook_version_selector'
 
 class Environments < Application
 
@@ -153,10 +154,18 @@ class Environments < Application
       end
 
       # Expand the run list in the scope of the specified environment.
-      names_to_cookbook_version = run_list.expand_to_cookbook_versions(environment_input, 'couchdb')
+      names_to_cookbook_version = Chef::CookbookVersionSelector.expand_to_cookbook_versions(run_list, environment_input)
     rescue Chef::Exceptions::CouchDBNotFound
       raise NotFound, "Cannot load environment #{params[:environment_id]}"
+    rescue Chef::Exceptions::CookbookVersionConflict => e
+      error = { :message => e.message,
+                #:unsatisfiable_solution_constraint => e.unsatisfiable_solution_constraint,
+                #:non_existent_cookbooks => e.disabled_non_existent_packages,
+                #:most_constrained_cookbooks => e.disabled_most_constrained_packages
+      }
+      raise PreconditionFailed, error.to_json
     end
+      
 
     # convert the hash which is
     #  name => CookbookVersion
diff --git a/chef-server-api/app/controllers/nodes.rb b/chef-server-api/app/controllers/nodes.rb
index f726f9aee0..9147700080 100644
--- a/chef-server-api/app/controllers/nodes.rb
+++ b/chef-server-api/app/controllers/nodes.rb
@@ -17,9 +17,10 @@
 # limitations under the License.
 #
 
-require 'chef' / 'node'
+require 'chef/node'
 require 'chef/version_class'
 require 'chef/version_constraint'
+require 'chef/cookbook_version_selector'
 
 class Nodes < Application
 
@@ -81,6 +82,8 @@ class Nodes < Application
     display @node
   end
 
+  # Return a hash, cookbook_name => cookbook manifest, of the cookbooks
+  # appropriate for this node, using its run_list and environment.
   def cookbooks
     begin
       @node = Chef::Node.cdb_load(params[:id])
@@ -88,30 +91,15 @@ class Nodes < Application
       raise NotFound, "Cannot load node #{params[:id]}"
     end
 
-    display(load_all_files)
-  end
-
-  private
-
-  def load_all_files
-    all_cookbooks = Chef::Environment.cdb_load_filtered_cookbook_versions(@node.chef_environment)
-
-    included_cookbooks = cookbooks_for_node(all_cookbooks)
-    nodes_cookbooks = Hash.new
-    included_cookbooks.each do |cookbook_name, cookbook|
-      nodes_cookbooks[cookbook_name.to_s] = cookbook.generate_manifest_with_urls{|opts| absolute_url(:cookbook_file, opts) }
-    end
+    # Get the mapping of cookbook_name => CookbookVersion applicable to
+    # this node's run_list and its environment.
+    included_cookbooks = Chef::CookbookVersionSelector.expand_to_cookbook_versions(@node.run_list, @node.chef_environment)
 
-    nodes_cookbooks
-  end
-
-  # returns name -> CookbookVersion for all cookbooks included on the given node.
-  def cookbooks_for_node(all_cookbooks)
-    begin
-      @node.constrain_cookbooks(all_cookbooks, 'couchdb')
-    rescue Chef::Exceptions::CookbookVersionConflict => e
-      raise PreconditionFailed, e.message
-    end
+    # Then map it to the return format.
+    display(included_cookbooks.inject({}) do |acc, (cookbook_name, cookbook)|
+      acc[cookbook_name.to_s] = cookbook.generate_manifest_with_urls{|opts| absolute_url(:cookbook_file, opts) }
+      acc
+    end)
   end
 
 end
diff --git a/chef-server-api/spec/unit/environments_controller_spec.rb b/chef-server-api/spec/unit/environments_controller_spec.rb
index 704a33acfd..5b0391192b 100644
--- a/chef-server-api/spec/unit/environments_controller_spec.rb
+++ b/chef-server-api/spec/unit/environments_controller_spec.rb
@@ -26,17 +26,24 @@ describe "Environments controller" do
     Merb.logger.set_log(StringIO.new)
 
     @env1 = make_environment("env1")
+    
+    @filtered_cookbook_list_env1 = make_filtered_cookbook_hash(make_cookbook("cookbook1", "1.0.0"),
+                                                               make_cookbook("cookbook2", "1.0.0"))
+    @filtered_cookbook_list_env1["cookbook_noversions"] = Array.new
 
-    @filtered_cookbook_list_env1 = 
-      make_filtered_cookbook_hash(make_cookbook("cookbook1", "1.0.0"),
-                                  make_cookbook("cookbook2", "1.0.0"))
-    @filtered_cookbook_list_env2 = 
-      make_filtered_cookbook_hash(make_cookbook("cookbook1", "2.0.0"),
-                                  make_cookbook("cookbook2", "2.0.0"))
+    @filtered_cookbook_list_env2 = make_filtered_cookbook_hash(make_cookbook("cookbook1", "2.0.0"),
+                                                               make_cookbook("cookbook2", "2.0.0"))
+
+    @cookbook_deps_on_nosuch = make_cookbook("cookbook_deps_on_nosuch", "1.0.0")
+    @cookbook_deps_on_nosuch.metadata.depends("cookbook_nosuch")
+
+    @cookbook_deps_on_badver = make_cookbook("cookbook_deps_on_badver", "1.0.0")
+    @cookbook_deps_on_badver.metadata.depends("cookbook1", ">= 3.0.0")
   end
 
   describe "when handling Environments API calls" do
-    it "should expand the passed-in run_list using the correct environment" do
+    it "should expand the passed-in run_list using the correct environment: one run_list item" do
+
       # Env1 pins both versions at 1.0.0. Expect only the one we ask for, cookbook1,
       # back in the result.
       Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env1").and_return(@filtered_cookbook_list_env1)
@@ -46,7 +53,9 @@ describe "Environments controller" do
       response["cookbook1"].should_not == nil
       response["cookbook1"]['version'].should == "1.0.0"
       response["cookbook1"]['url'].should == "#{root_url}/cookbooks/cookbook1/1.0.0"
+    end
 
+    it "should expect the passed-in run_list using the correct environment: two run_list items" do
       # Ask for both cookbook1 and cookbook2 back. Expect version 2.0.0 for
       # each, as those are what's appropriate for the environment.
       Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env2").and_return(@filtered_cookbook_list_env2)
@@ -60,6 +69,45 @@ describe "Environments controller" do
       response["cookbook2"]['version'].should == "2.0.0"
       response["cookbook2"]['url'].should == "#{root_url}/cookbooks/cookbook2/2.0.0"
     end
+
+    it "should report no_such_cookbook if given a dependency on a non-existant cookbook" do
+      Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env1").and_return(@filtered_cookbook_list_env1)
+      expected_error = {
+        "message" => "Run list item (cookbook_nosuch >= 0.0.0) specifies a cookbook that does not exist in the dependency graph",
+      }.to_json
+
+      lambda {
+        response = post_json("/environments/env1/cookbook_versions", {"run_list" => ["recipe[cookbook_nosuch]"]})
+      }.should raise_error(Merb::ControllerExceptions::PreconditionFailed, expected_error)
+    end
+
+    it "should report no_such_version if given a dependency on a cookbook that doesn't have any valid versions for an environment" do
+      Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env1").and_return(@filtered_cookbook_list_env1)
+      expected_error = {
+        "message" => "Run list item (cookbook_noversions >= 0.0.0) does not match any versions",
+      }.to_json
+
+      lambda {
+        response = post_json("/environments/env1/cookbook_versions", {"run_list" => ["recipe[cookbook_noversions]"]})
+      }.should raise_error(Merb::ControllerExceptions::PreconditionFailed, expected_error)
+    end
+
+
+    # TODO; have top-level cookbooks depend on other, non-existent cookbooks,
+    # to get the other kind of exceptions.
+    it "should report multiple failures (compound exceptions) if there is more than one error in dependencies" do
+      Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env1").and_return(@filtered_cookbook_list_env1)
+      begin
+        response = post_json("/environments/env1/cookbook_versions", 
+                             {"run_list" => ["recipe[cookbook_nosuch_1]", "recipe[cookbook_nosuch_2]"]})
+      rescue => e
+        puts "e is #{e}"
+        puts "e.stacktrace =\n  #{e.backtrace.join("  \n")}"
+
+        require 'pp'
+        pp(:e => e)
+      end
+    end
   end
 end
 
diff --git a/chef-server-api/spec/unit/nodes_controller_environments_spec.rb b/chef-server-api/spec/unit/nodes_controller_environments_spec.rb
index 4ce44ada58..ecaa3f71c5 100644
--- a/chef-server-api/spec/unit/nodes_controller_environments_spec.rb
+++ b/chef-server-api/spec/unit/nodes_controller_environments_spec.rb
@@ -39,8 +39,8 @@ describe "Nodes controller - environments" do
     @node1.run_list << "role[role1]"
 
     @role1 = make_role("role1")
-    @role1.env_run_lists({"_default" => make_runlist("recipe[cb_for_default]"),
-                          "env1" => make_runlist("recipe[cb_for_env1]")})
+    @role1.env_run_lists({"env1" => make_runlist("recipe[cb_for_env1]")})
+    @role1.run_list(make_runlist("recipe[cb_for_default]"))
 
     @all_filtered_cookbook_list = 
       make_filtered_cookbook_hash(make_cookbook("cb_for_default", "1.0.0"),
@@ -52,8 +52,8 @@ describe "Nodes controller - environments" do
 
       # Test that node@_default resolves to use cookbook cb_for_default
       Chef::Node.should_receive(:cdb_load).with("node1").and_return(@node1)
-      Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("_default").and_return(@all_filtered_cookbook_list)
       Chef::Role.should_receive(:cdb_load).with("role1", nil).and_return(@role1)
+      Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("_default").and_return(@all_filtered_cookbook_list)
 
       response = get_json("/nodes/node1/cookbooks")
       response.should be_kind_of(Hash)
@@ -65,8 +65,8 @@ describe "Nodes controller - environments" do
       # Test that node@env1 resolves to use cookbook cb_for_env1
       @node1.chef_environment("env1")
       Chef::Node.should_receive(:cdb_load).with("node1").and_return(@node1)
-      Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env1").and_return(@all_filtered_cookbook_list)
       Chef::Role.should_receive(:cdb_load).with("role1", nil).and_return(@role1)
+      Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env1").and_return(@all_filtered_cookbook_list)
 
       response = get_json("/nodes/node1/cookbooks")
       response.should be_kind_of(Hash)
@@ -80,8 +80,8 @@ describe "Nodes controller - environments" do
       # because env_fallback falls back to _default
       @node1.chef_environment("env_fallback")
       Chef::Node.should_receive(:cdb_load).with("node1").and_return(@node1)
-      Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env_fallback").and_return(@all_filtered_cookbook_list)
       Chef::Role.should_receive(:cdb_load).with("role1", nil).and_return(@role1)
+      Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env_fallback").and_return(@all_filtered_cookbook_list)
 
       response = get_json("/nodes/node1/cookbooks")
       response.should be_kind_of(Hash)
diff --git a/chef/lib/chef/cookbook_version_selector.rb b/chef/lib/chef/cookbook_version_selector.rb
new file mode 100644
index 0000000000..c59b04c15d
--- /dev/null
+++ b/chef/lib/chef/cookbook_version_selector.rb
@@ -0,0 +1,146 @@
+#
+# Author:: Tim Hinderliter (<tim@opscode.com>)
+# Copyright:: Copyright (c) 2011 Opscode, Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+require 'dep_selector'
+
+class Chef
+  module CookbookVersionSelector
+    # This method replaces verbiage from DepSelector messages with
+    # Chef-domain-specific verbiage, such as replacing package with
+    # cookbook.
+    #
+    # TODO [cw, 2011/2/25]: this is a near-term hack. In the long run,
+    # we'll do this better.
+    def self.filter_dep_selector_message(message)
+      m = message
+      m.gsub!("Package", "Cookbook")
+      m.gsub!("package", "cookbook")
+      m.gsub!("Solution constraint", "Run list item")
+      m.gsub!("solution constraint", "run list item")
+      m
+    end
+
+    # all_cookbooks - a hash mapping cookbook names to an array of
+    # available CookbookVersions.
+    #
+    # Creates a DependencyGraph from CookbookVersion objects
+    def self.create_dependency_graph_from_cookbooks(all_cookbooks)
+      dep_graph = DepSelector::DependencyGraph.new
+
+      all_cookbooks.each do |cb_name, cb_versions|
+        cb_versions.each do |cb_version|
+          cb_version_deps = cb_version.metadata.dependencies
+          # TODO [cw. 2011/2/10]: CookbookVersion#version returns a
+          # String even though we're storing as a DepSelector::Version
+          # object underneath. This should be changed so that we
+          # return the object and handle proper serialization and
+          # de-serialization. For now, I'm just going to create a
+          # Version object from the String representation.
+          pv = dep_graph.package(cb_name).add_version(DepSelector::Version.new(cb_version.version))
+          cb_version_deps.each_pair do |dep_name, constraint_str|
+            constraint = DepSelector::VersionConstraint.new(constraint_str)
+            pv.dependencies << DepSelector::Dependency.new(dep_graph.package(dep_name), constraint)
+          end
+        end
+      end
+
+      dep_graph
+    end
+
+    # Return a hash mapping cookbook names to a CookbookVersion
+    # object. If there is no solution that satisfies the constraints,
+    # the first run list item that caused unsatisfiability is
+    # returned.
+    #
+    # This is the final version-resolved list of cookbooks for the
+    # RunList.
+    #
+    # all_cookbooks - a hash mapping cookbook names to an array of
+    # available CookbookVersions.
+    #
+    # recipe_constraints - an array of hashes describing the expanded
+    # run list.  Each element is a hash containing keys :name and
+    # :version_constraint. The :name component is either the
+    # fully-qualified recipe name (e.g. "cookbook1::non_default_recipe")
+    # or just a cookbook name, indicating the default recipe is to be
+    # run (e.g. "cookbook1").
+    def self.constrain(all_cookbooks, recipe_constraints)
+      dep_graph = create_dependency_graph_from_cookbooks(all_cookbooks)
+
+      # extract cookbook names from (possibly) fully-qualified recipe names
+      cookbook_constraints = recipe_constraints.map do |recipe_spec|
+        cookbook_name = (recipe_spec[:name][/^(.+)::/, 1] || recipe_spec[:name])
+        DepSelector::SolutionConstraint.new(dep_graph.package(cookbook_name),
+                                            recipe_spec[:version_constraint])
+      end
+
+      # Pass in the list of all available cookbooks (packages) so that
+      # DepSelector can distinguish between "no version available for
+      # cookbook X" and "no such cookbook X" when an environment
+      # filters out all versions for a given cookbook.
+      all_packages = all_cookbooks.inject([]) do |acc, (cookbook_name, cookbook_versions)|
+        acc << dep_graph.package(cookbook_name)
+        acc
+      end
+
+      # find a valid assignment of CoookbookVersions. If no valid
+      # assignment exists, indicate which run_list_item causes the
+      # unsatisfiability and try to hint at what might be wrong.
+      soln =
+        begin
+          DepSelector::Selector.new(dep_graph).find_solution(cookbook_constraints, all_packages)
+        rescue DepSelector::Exceptions::NoSolutionExists
+          puts <<EOM
+NoSolutionExists:
+  message = #{$!.message}
+  unsatisfiable_solution_constraint = #{$!.unsatisfiable_solution_constraint.inspect}
+  disabled_non_existent_packages = #{$!.disabled_non_existent_packages}
+  disabled_most_constrained_packages = #{$!.disabled_most_constrained_packages}
+EOM
+          raise Chef::Exceptions::CookbookVersionConflict, filter_dep_selector_message($!.message)
+        end
+        
+
+      # map assignment back to CookbookVersion objects
+      selected_cookbooks = {}
+      soln.each_pair do |cb_name, cb_version|
+        # TODO [cw, 2011/2/10]: related to the TODO in
+        # create_dependency_graph_from_cookbooks, cbv.version
+        # currently returns a String, so we must compare to
+        # cb_version.to_s, since it's a for-real Version object.
+        selected_cookbooks[cb_name] = all_cookbooks[cb_name].find{|cbv| cbv.version == cb_version.to_s}
+      end
+      selected_cookbooks
+    end
+
+    # Expands the run_list, constrained to the environment's CookbookVersion
+    # constraints.
+    # 
+    # Returns:
+    #   Hash of: name to CookbookVersion
+    def self.expand_to_cookbook_versions(run_list, environment)
+
+      # expand any roles in this run_list.
+      expanded_run_list = run_list.expand(environment, 'couchdb').recipes.with_version_constraints
+      
+      cookbooks_for_environment = Chef::Environment.cdb_load_filtered_cookbook_versions(environment)
+      constrain(cookbooks_for_environment, expanded_run_list)
+    end
+  end
+end
+
+
diff --git a/chef/lib/chef/run_list.rb b/chef/lib/chef/run_list.rb
index a5c3f95a91..5032fc7686 100644
--- a/chef/lib/chef/run_list.rb
+++ b/chef/lib/chef/run_list.rb
@@ -23,7 +23,6 @@ require 'chef/run_list/run_list_item'
 require 'chef/run_list/run_list_expansion'
 require 'chef/run_list/versioned_recipe_list'
 require 'chef/mixin/params_validate'
-require 'dep_selector'
 
 class Chef
   class RunList
@@ -137,19 +136,6 @@ class Chef
       expansion
     end
 
-    # Expands this run_list, constrained to the environment's CookbookVersion
-    # constraints.
-    # 
-    # Returns:
-    #   Hash of: name to CookbookVersion
-    def expand_to_cookbook_versions(environment, data_source='server', expansion_opts={})
-      # expand any roles in this run_list.
-      expanded_run_list = expand(environment, data_source, expansion_opts).recipes.with_version_constraints
-      
-      cookbooks_for_environment = Chef::Environment.cdb_load_filtered_cookbook_versions(environment)
-      constrain(cookbooks_for_environment, expanded_run_list)
-    end
-
     # Converts a string run list entry to a RunListItem object.
     def parse_entry(entry)
       RunListItem.new(entry)
@@ -171,93 +157,6 @@ class Chef
       end
     end
 
-    # This method replaces verbiage from DepSelector messages with
-    # Chef-domain-specific verbiage, such as replacing package with
-    # cookbook.
-    #
-    # TODO [cw, 2011/2/25]: this is a near-term hack. In the long run,
-    # we'll do this better.
-    def filter_dep_selector_message(message)
-      m = message
-      m.gsub!("Package", "Cookbook")
-      m.gsub!("package", "cookbook")
-      m.gsub!("Solution constraint", "Run list item")
-      m.gsub!("solution constraint", "run list item")
-      m
-    end
-
-    # Creates a DependencyGraph from CookbookVersion objects
-    def create_dependency_graph_from_cookbooks(all_cookbooks)
-      dep_graph = DepSelector::DependencyGraph.new
-      all_cookbooks.each do |cb_name, cb_versions|
-        cb_versions.each do |cb_version|
-          cb_version_deps = cb_version.metadata.dependencies
-          # TODO [cw. 2011/2/10]: CookbookVersion#version returns a
-          # String even though we're storing as a DepSelector::Version
-          # object underneath. This should be changed so that we
-          # return the object and handle proper serialization and
-          # de-serialization. For now, I'm just going to create a
-          # Version object from the String representation.
-          pv = dep_graph.package(cb_name).add_version(DepSelector::Version.new(cb_version.version))
-          cb_version_deps.each_pair do |dep_name, constraint_str|
-            constraint = DepSelector::VersionConstraint.new(constraint_str)
-            pv.dependencies << DepSelector::Dependency.new(dep_graph.package(dep_name), constraint)
-          end
-        end
-      end
-
-      dep_graph
-    end
-
-    # Return a hash mapping cookbook names to a CookbookVersion
-    # object. If there is no solution that satisfies the constraints,
-    # the first run list item that caused unsatisfiability is
-    # returned.
-    #
-    # This is the final version-resolved list of cookbooks for the
-    # RunList.
-    #
-    # all_cookbooks - a hash mapping cookbook names to an array of
-    # available CookbookVersions.
-    #
-    # recipe_constraints - an array of hashes describing the expanded
-    # run list.  Each element is a hash containing keys :name and
-    # :version_constraint. The :name component is either the
-    # fully-qualified recipe name (e.g. "cookbook1::non_default_recipe")
-    # or just a cookbook name, indicating the default recipe is to be
-    # run (e.g. "cookbook1").
-    def constrain(all_cookbooks, recipe_constraints)
-      dep_graph = create_dependency_graph_from_cookbooks(all_cookbooks)
-
-      # extract cookbook names from (possibly) fully-qualified recipe names
-      cookbook_constraints = recipe_constraints.map do |recipe_spec|
-        cookbook_name = (recipe_spec[:name][/^(.+)::/, 1] || recipe_spec[:name])
-        DepSelector::SolutionConstraint.new(dep_graph.package(cookbook_name),
-                                            recipe_spec[:version_constraint])
-      end
-
-      # find a valid assignment of CoookbookVersions. If no valid
-      # assignment exists, indicate which run_list_item causes the
-      # unsatisfiability and try to hint at what might be wrong.
-      soln =
-        begin
-          DepSelector::Selector.new(dep_graph).find_solution(cookbook_constraints)
-        rescue DepSelector::Exceptions::InvalidSolutionConstraint,
-               DepSelector::Exceptions::NoSolutionExists
-          raise Chef::Exceptions::CookbookVersionConflict, filter_dep_selector_message($!.message)
-        end
-
-      # map assignment back to CookbookVersion objects
-      selected_cookbooks = {}
-      soln.each_pair do |cb_name, cb_version|
-        # TODO [cw, 2011/2/10]: related to the TODO in
-        # create_dependency_graph_from_cookbooks, cbv.version
-        # currently returns a String, so we must compare to
-        # cb_version.to_s, since it's a for-real Version object.
-        selected_cookbooks[cb_name] = all_cookbooks[cb_name].find{|cbv| cbv.version == cb_version.to_s}
-      end
-      selected_cookbooks
-    end
 
   end
 end