Use the dynamic policy builder everywhere

10 files changed, 91 insertions, 111 deletions

diff --git a/lib/chef/client.rb b/lib/chef/client.rb
index 621ce3d489..7d5d463242 100644
--- a/lib/chef/client.rb
+++ b/lib/chef/client.rb
@@ -496,7 +496,7 @@ class Chef
     # @api private
     #
     def policy_builder
-      @policy_builder ||= Chef::PolicyBuilder.strategy.new(node_name, ohai.data, json_attribs, override_runlist, events)
+      @policy_builder ||= Chef::PolicyBuilder::Dynamic.new(node_name, ohai.data, json_attribs, override_runlist, events)
     end
 
     #
diff --git a/lib/chef/exceptions.rb b/lib/chef/exceptions.rb
index e3649c068b..7862d9c160 100644
--- a/lib/chef/exceptions.rb
+++ b/lib/chef/exceptions.rb
@@ -116,6 +116,8 @@ class Chef
       end
     end
 
+    class InvalidPolicybuilderCall < ArgumentError; end
+
     class InvalidResourceSpecification < ArgumentError; end
     class SolrConnectionError < RuntimeError; end
     class IllegalChecksumRevert < RuntimeError; end
diff --git a/lib/chef/policy_builder.rb b/lib/chef/policy_builder.rb
index 036d8bc051..56415dbbd0 100644
--- a/lib/chef/policy_builder.rb
+++ b/lib/chef/policy_builder.rb
@@ -38,13 +38,5 @@ class Chef
   # * cookbook_hash is stored in run_context
   module PolicyBuilder
 
-    def self.strategy
-      if Chef::Config[:use_policyfile]
-        Policyfile
-      else
-        ExpandNodeObject
-      end
-    end
-
   end
 end
diff --git a/lib/chef/policy_builder/dynamic.rb b/lib/chef/policy_builder/dynamic.rb
index e976ddd87f..d2849dd4f1 100644
--- a/lib/chef/policy_builder/dynamic.rb
+++ b/lib/chef/policy_builder/dynamic.rb
@@ -21,6 +21,7 @@ require 'chef/rest'
 require 'chef/run_context'
 require 'chef/config'
 require 'chef/node'
+require 'chef/exceptions'
 
 class Chef
   module PolicyBuilder
@@ -56,7 +57,12 @@ class Chef
         events.node_load_start(node_name, config)
         Chef::Log.debug("Building node object for #{node_name}")
 
-        node = Chef::Node.find_or_create(node_name)
+        @node =
+          if Chef::Config[:solo]
+            Chef::Node.build(node_name)
+          else
+            Chef::Node.find_or_create(node_name)
+          end
         select_implementation(node)
         implementation.finish_load_node(node)
         node
@@ -102,7 +108,7 @@ class Chef
       ## Internal Public API ##
 
       def implementation
-        @implementation
+        @implementation or raise Exceptions::InvalidPolicybuilderCall, "#load_node must be called before other policy builder methods"
       end
 
       def select_implementation(node)
diff --git a/lib/chef/policy_builder/expand_node_object.rb b/lib/chef/policy_builder/expand_node_object.rb
index 524bdd95b1..543d6a0a7b 100644
--- a/lib/chef/policy_builder/expand_node_object.rb
+++ b/lib/chef/policy_builder/expand_node_object.rb
@@ -93,26 +93,10 @@ class Chef
         run_context
       end
 
-
-      # In client-server operation, loads the node state from the server. In
-      # chef-solo operation, builds a new node object.
-      def load_node
-        events.node_load_start(node_name, Chef::Config)
-        Chef::Log.debug("Building node object for #{node_name}")
-
-        if Chef::Config[:solo]
-          @node = Chef::Node.build(node_name)
-        else
-          @node = Chef::Node.find_or_create(node_name)
-        end
-      rescue Exception => e
-        # TODO: wrap this exception so useful error info can be given to the
-        # user.
-        events.node_load_failed(node_name, e, Chef::Config)
-        raise
+      def finish_load_node(node)
+        @node = node
       end
 
-
       # Applies environment, external JSON attributes, and override run list to
       # the node, Then expands the run_list.
       #
diff --git a/lib/chef/policy_builder/policyfile.rb b/lib/chef/policy_builder/policyfile.rb
index 5991e3ce10..5bcdd5f52f 100644
--- a/lib/chef/policy_builder/policyfile.rb
+++ b/lib/chef/policy_builder/policyfile.rb
@@ -112,18 +112,10 @@ class Chef
 
       ## PolicyBuilder API ##
 
-      # Loads the node state from the server.
-      def load_node
-        events.node_load_start(node_name, Chef::Config)
-        Chef::Log.debug("Building node object for #{node_name}")
-
-        @node = Chef::Node.find_or_create(node_name)
+      def finish_load_node(node)
+        @node = node
         validate_policyfile
         events.policyfile_loaded(policy)
-        node
-      rescue Exception => e
-        events.node_load_failed(node_name, e, Chef::Config)
-        raise
       end
 
       # Applies environment, external JSON attributes, and override run list to
diff --git a/spec/unit/client_spec.rb b/spec/unit/client_spec.rb
index 8146774764..f736c38859 100644
--- a/spec/unit/client_spec.rb
+++ b/spec/unit/client_spec.rb
@@ -364,6 +364,8 @@ describe Chef::Client do
       expect(node[:expanded_run_list]).to be_nil
 
       allow(client.policy_builder).to receive(:node).and_return(node)
+      client.policy_builder.select_implementation(node)
+      allow(client.policy_builder.implementation).to receive(:node).and_return(node)
 
       # chefspec and possibly others use the return value of this method
       expect(client.build_node).to eq(node)
@@ -391,6 +393,8 @@ describe Chef::Client do
       expect(mock_chef_rest).to receive(:get_rest).with("environments/A").and_return(test_env)
       expect(Chef::REST).to receive(:new).and_return(mock_chef_rest)
       allow(client.policy_builder).to receive(:node).and_return(node)
+      client.policy_builder.select_implementation(node)
+      allow(client.policy_builder.implementation).to receive(:node).and_return(node)
       expect(client.build_node).to eq(node)
 
       expect(node.chef_environment).to eq("A")
diff --git a/spec/unit/policy_builder/dynamic_spec.rb b/spec/unit/policy_builder/dynamic_spec.rb
index 74a0272f42..105a34390e 100644
--- a/spec/unit/policy_builder/dynamic_spec.rb
+++ b/spec/unit/policy_builder/dynamic_spec.rb
@@ -190,15 +190,66 @@ describe Chef::PolicyBuilder::Dynamic do
 
       before do
         allow(policy_builder).to receive(:implementation).and_return(implementation)
+      end
+
+      context "when not running chef solo" do
+
+
+        context "when successful" do
+
+          before do
+            expect(Chef::Node).to receive(:find_or_create).with(node_name).and_return(node)
+            expect(policy_builder).to receive(:select_implementation).with(node)
+            expect(implementation).to receive(:finish_load_node).with(node)
+          end
+
+          it "selects the backend implementation and continues node loading" do
+            policy_builder.load_node
+          end
+
+        end
+
+        context "when an error occurs finding the node" do
+
+          before do
+            expect(Chef::Node).to receive(:find_or_create).with(node_name).and_raise("oops")
+          end
+
+          it "sends a node_load_failed event and re-raises" do
+            expect(events).to receive(:node_load_failed)
+            expect { policy_builder.load_node }.to raise_error("oops")
+          end
+
+        end
+
+        context "when an error occurs in the implementation's finish_load_node call" do
+
+          before do
+            expect(Chef::Node).to receive(:find_or_create).with(node_name).and_return(node)
+            expect(policy_builder).to receive(:select_implementation).with(node)
+            expect(implementation).to receive(:finish_load_node).and_raise("oops")
+          end
+
+
+          it "sends a node_load_failed event and re-raises" do
+            expect(events).to receive(:node_load_failed)
+            expect { policy_builder.load_node }.to raise_error("oops")
+          end
+
+        end
 
-        expect(Chef::Node).to receive(:find_or_create).with(node_name).and_return(node)
-        expect(policy_builder).to receive(:select_implementation).with(node)
-        expect(implementation).to receive(:finish_load_node).with(node)
       end
 
-      context "when successful" do
+      context "when running chef solo" do
+
+        before do
+          Chef::Config[:solo] = true
+          expect(Chef::Node).to receive(:build).with(node_name).and_return(node)
+          expect(policy_builder).to receive(:select_implementation).with(node)
+          expect(implementation).to receive(:finish_load_node).with(node)
+        end
 
-        it "selects the backend implementation and continues node loading", :pending do
+        it "selects the backend implementation and continues node loading" do
           policy_builder.load_node
         end
 
diff --git a/spec/unit/policy_builder/expand_node_object_spec.rb b/spec/unit/policy_builder/expand_node_object_spec.rb
index 8e9fdc305e..815926ffb7 100644
--- a/spec/unit/policy_builder/expand_node_object_spec.rb
+++ b/spec/unit/policy_builder/expand_node_object_spec.rb
@@ -34,8 +34,8 @@ describe Chef::PolicyBuilder::ExpandNodeObject do
       expect(policy_builder).to respond_to(:node)
     end
 
-    it "implements a load_node method" do
-      expect(policy_builder).to respond_to(:load_node)
+    it "implements a finish_load_node method" do
+      expect(policy_builder).to respond_to(:finish_load_node)
     end
 
     it "implements  a build_node method" do
@@ -63,39 +63,13 @@ describe Chef::PolicyBuilder::ExpandNodeObject do
       expect(policy_builder).to respond_to(:temporary_policy?)
     end
 
-    describe "loading the node" do
+    describe "finishing loading the node" do
 
-      context "on chef-solo" do
-
-        before do
-          Chef::Config[:solo] = true
-        end
-
-        it "creates a new in-memory node object with the given name" do
-          policy_builder.load_node
-          expect(policy_builder.node.name).to eq(node_name)
-        end
+      let(:node) { Chef::Node.new.tap { |n| n.name(node_name) } }
 
-      end
-
-      context "on chef-client" do
-
-        let(:node) { Chef::Node.new.tap { |n| n.name(node_name) } }
-
-        it "loads or creates a node on the server" do
-          expect(Chef::Node).to receive(:find_or_create).with(node_name).and_return(node)
-          policy_builder.load_node
-          expect(policy_builder.node).to eq(node)
-        end
-
-      end
-    end
-
-    describe "building the node" do
-
-      # XXX: Chef::Client just needs to be able to call this, it doesn't depend on the return value.
-      it "builds the node and returns the updated node object" do
-        skip
+      it "stores the node" do
+        policy_builder.finish_load_node(node)
+        expect(policy_builder.node).to eq(node)
       end
 
     end
@@ -133,8 +107,7 @@ describe Chef::PolicyBuilder::ExpandNodeObject do
     end
 
     before do
-      expect(Chef::Node).to receive(:find_or_create).with(node_name).and_return(node)
-      policy_builder.load_node
+      policy_builder.finish_load_node(node)
     end
 
     it "expands the run_list" do
@@ -167,8 +140,7 @@ describe Chef::PolicyBuilder::ExpandNodeObject do
 
     before do
       Chef::Config[:environment] = configured_environment
-      expect(Chef::Node).to receive(:find_or_create).with(node_name).and_return(node)
-      policy_builder.load_node
+      policy_builder.finish_load_node(node)
       policy_builder.build_node
     end
 
@@ -302,11 +274,9 @@ describe Chef::PolicyBuilder::ExpandNodeObject do
     let(:cookbook_synchronizer) { double("CookbookSynchronizer") }
 
     before do
-      expect(Chef::Node).to receive(:find_or_create).with(node_name).and_return(node)
-
       allow(policy_builder).to receive(:api_service).and_return(chef_http)
 
-      policy_builder.load_node
+      policy_builder.finish_load_node(node)
       policy_builder.build_node
 
       run_list_expansion = policy_builder.run_list_expansion
diff --git a/spec/unit/policy_builder/policyfile_spec.rb b/spec/unit/policy_builder/policyfile_spec.rb
index 5fa00d8f2b..82ca1a2bb3 100644
--- a/spec/unit/policy_builder/policyfile_spec.rb
+++ b/spec/unit/policy_builder/policyfile_spec.rb
@@ -181,19 +181,13 @@ describe Chef::PolicyBuilder::Policyfile do
         let(:error404) { Net::HTTPServerException.new("404 message", :body) }
 
         before do
-          expect(Chef::Node).to receive(:find_or_create).with(node_name).and_return(node)
           expect(http_api).to receive(:get).
             with("data/policyfiles/example-policy-stage").
             and_raise(error404)
         end
 
         it "raises an error" do
-          expect { policy_builder.load_node }.to raise_error(err_namespace::ConfigurationError)
-        end
-
-        it "sends error message to the event system" do
-          expect(events).to receive(:node_load_failed).with(node_name, an_instance_of(err_namespace::ConfigurationError), Chef::Config)
-          expect { policy_builder.load_node }.to raise_error(err_namespace::ConfigurationError)
+          expect { policy_builder.finish_load_node(node) }.to raise_error(err_namespace::ConfigurationError)
         end
 
       end
@@ -201,20 +195,12 @@ describe Chef::PolicyBuilder::Policyfile do
       context "when the deployment_group is not configured" do
         before do
           Chef::Config[:deployment_group] = nil
-          expect(Chef::Node).to receive(:find_or_create).with(node_name).and_return(node)
         end
 
         it "errors while loading the node" do
-          expect { policy_builder.load_node }.to raise_error(err_namespace::ConfigurationError)
+          expect { policy_builder.finish_load_node(node) }.to raise_error(err_namespace::ConfigurationError)
         end
 
-
-        it "passes error information to the event system" do
-          # TODO: also make sure something acceptable happens with the error formatters
-          err_class = err_namespace::ConfigurationError
-          expect(events).to receive(:node_load_failed).with(node_name, an_instance_of(err_class), Chef::Config)
-          expect { policy_builder.load_node }.to raise_error(err_class)
-        end
       end
 
       context "when deployment_group is correctly configured" do
@@ -307,8 +293,7 @@ describe Chef::PolicyBuilder::Policyfile do
       end
 
       it "implements #expand_run_list in a manner compatible with ExpandNodeObject" do
-        expect(Chef::Node).to receive(:find_or_create).with(node_name).and_return(node)
-        policy_builder.load_node
+        policy_builder.finish_load_node(node)
         expect(policy_builder.expand_run_list).to respond_to(:recipes)
         expect(policy_builder.expand_run_list.recipes).to eq(["example1::default", "example2::server"])
         expect(policy_builder.expand_run_list.roles).to eq([])
@@ -346,9 +331,7 @@ describe Chef::PolicyBuilder::Policyfile do
       describe "building the node object" do
 
         before do
-          expect(Chef::Node).to receive(:find_or_create).with(node_name).and_return(node)
-
-          policy_builder.load_node
+          policy_builder.finish_load_node(node)
           policy_builder.build_node
         end
 
@@ -414,9 +397,7 @@ describe Chef::PolicyBuilder::Policyfile do
             let(:error404) { Net::HTTPServerException.new("404 message", :body) }
 
             before do
-              expect(Chef::Node).to receive(:find_or_create).with(node_name).and_return(node)
-
-              policy_builder.load_node
+              policy_builder.finish_load_node(node)
               policy_builder.build_node
 
               expect(http_api).to receive(:get).with(cookbook1_url).
@@ -433,9 +414,7 @@ describe Chef::PolicyBuilder::Policyfile do
         shared_examples_for "fetching cookbooks when they exist" do
           context "and the cookbooks can be fetched" do
             before do
-              expect(Chef::Node).to receive(:find_or_create).with(node_name).and_return(node)
-
-              policy_builder.load_node
+              policy_builder.finish_load_node(node)
               policy_builder.build_node
 
               allow(Chef::CookbookSynchronizer).to receive(:new).