better testing

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

2 files changed, 205 insertions, 52 deletions

diff --git a/lib/chef/provider/rest_resource.rb b/lib/chef/provider/rest_resource.rb
index 72500d6965..02801f7efd 100644
--- a/lib/chef/provider/rest_resource.rb
+++ b/lib/chef/provider/rest_resource.rb
@@ -1,4 +1,6 @@
 require_relative "../provider"
+require "rest-client" unless defined?(RestClient)
+require "jmespath" unless defined?(JMESPath)
 
 class Chef
   class Provider
@@ -13,9 +15,9 @@ class Chef
           current_resource.send(name, requested)
         end
 
-        return @current_resource if rest_get_all.empty?
+        return @current_resource if rest_get_all.data.empty?
 
-        resource_data = rest_get
+        resource_data = rest_get.data rescue nil
         return @current_resource if resource_data.nil? || resource_data.empty?
 
         @resource_exists = true
@@ -23,33 +25,48 @@ class Chef
         # Map JSON contents to defined properties
         current_resource.class.rest_property_map.each do |property, match_instruction|
           property_value = json_to_property(match_instruction, property, resource_data)
-
           current_resource.send(property, property_value) unless property_value.nil?
         end
 
         current_resource
       end
 
-      def action_configure
-        converge_if_changed do
-          data = {}
+      action :configure do
+        if resource_exists?
+          converge_if_changed do
+            data = {}
+
+            new_resource.class.rest_property_map.each do |property, match_instruction|
+              # Skip "creation-only" properties on modifications
+              next if new_resource.class.rest_post_only_properties.include?(property)
+
+              deep_merge! data, property_to_json(property, match_instruction)
+            end
 
-          new_resource.class.rest_property_map.each do |property, match_instruction|
-            # Skip "creation-only" properties on modifications
-            next if resource_exists? && new_resource.class.rest_post_only_properties.include?(property)
+            deep_compact!(data)
 
-            deep_merge! data, property_to_json(property, match_instruction)
+            rest_patch(data)
           end
+        else
+          converge_by "creating resource" do
+            data = {}
+
+            new_resource.class.rest_property_map.each do |property, match_instruction|
+              deep_merge! data, property_to_json(property, match_instruction)
+            end
 
-          deep_compact!(data)
+            deep_compact!(data)
 
-          @resource_exists ? rest_patch(data) : rest_post(data)
+            rest_post(data)
+          end
         end
       end
 
-      def action_delete
+      action :delete do
         if resource_exists?
-          rest_delete
+          converge_by "deleting resource" do
+            rest_delete
+          end
         else
           logger.debug format("REST resource %<name>s of type %<type>s does not exist. Skipping.",
                               type: new_resource.name, name: id_property)
diff --git a/spec/unit/provider/rest_resource_spec.rb b/spec/unit/provider/rest_resource_spec.rb
index 7f43f70c35..d4c8f0e24b 100644
--- a/spec/unit/provider/rest_resource_spec.rb
+++ b/spec/unit/provider/rest_resource_spec.rb
@@ -1,145 +1,280 @@
 require "spec_helper"
+require "train"
+require "train-rest"
 
 class RestResourceByQuery < Chef::Resource::RestResource
+  provides :rest_resource_by_query
+
   property :address, String, required: true
   property :prefix, Integer, required: true
   property :gateway, String
 
   rest_api_collection "/api/v1/addresses"
   rest_api_document   "/api/v1/address/?ip={address}"
+  rest_property_map({
+    address: "address",
+    prefix: "prefix",
+    gateway: "gateway",
+  })
+end
+
+class RestProviderByQuery < Chef::Provider::RestResource
+  provides :rest_resource_by_query
 end
 
 class RestResourceByPath < RestResourceByQuery
-  rest_api_document   "/api/v1/address/{address}"
+  rest_api_document "/api/v1/address/{address}"
 end
 
 describe "rest_resource using query-based addressing" do
-  before(:each) do
-    @cookbook_collection = Chef::CookbookCollection.new([])
-    @node = Chef::Node.new
-    @node.name "node1"
-    @events = Chef::EventDispatch::Dispatcher.new
-    @run_context = Chef::RunContext.new(@node, @cookbook_collection, @events)
+  let(:train) {
+    Train.create(
+    "rest", {
+      endpoint:   "https://api.example.com/api/v1/",
+      debug_rest: true,
+      logger:     Chef::Log,
+    }
+  ).connection
+  }
+
+  let(:run_context) do
+    cookbook_collection = Chef::CookbookCollection.new([])
+    node = Chef::Node.new
+    node.name "node1"
+    events = Chef::EventDispatch::Dispatcher.new
+    Chef::RunContext.new(node, cookbook_collection, events)
+  end
 
-    @resource = RestResourceByQuery.new("set_address", @run_context)
-    @resource.address = "192.0.2.1"
-    @resource.prefix = 24
+  let(:resource) do
+    RestResourceByQuery.new("set_address", run_context).tap do |resource|
+      resource.address = "192.0.2.1"
+      resource.prefix = 24
+      resource.action :configure
+    end
+  end
 
-    @provider = Chef::Provider::RestResource.new(@resource, @run_context)
-    @provider.current_resource = @resource
+  let(:provider) do
+    Chef::Provider::RestResource.new(resource, run_context).tap do |provider|
+      provider.current_resource = resource # for some stubby tests that don't call LCR
+      allow(provider).to receive(:api_connection).and_return(train)
+    end
+  end
+
+  before(:each) do
+    allow(Chef::Provider::RestResource).to receive(:new).and_return(provider)
   end
 
   it "should include :configure action" do
-    expect(@provider).to respond_to(:action_configure)
+    expect(provider).to respond_to(:action_configure)
   end
 
   it "should include :delete action" do
-    expect(@provider).to respond_to(:action_delete)
+    expect(provider).to respond_to(:action_delete)
   end
 
   it "should include :nothing action" do
-    expect(@provider).to respond_to(:action_nothing)
+    expect(provider).to respond_to(:action_nothing)
   end
 
   describe "#rest_postprocess" do
     before do
-      @provider.singleton_class.send(:public, :rest_postprocess)
+      provider.singleton_class.send(:public, :rest_postprocess)
     end
     it "should have a default rest_postprocess implementation" do
-      expect(@provider).to respond_to(:rest_postprocess)
+      expect(provider).to respond_to(:rest_postprocess)
     end
 
     it "should have a non-mutating rest_postprocess implementation" do
       response = "{ data: nil }"
 
-      expect(@provider.rest_postprocess(response.dup)).to eq(response)
+      expect(provider.rest_postprocess(response.dup)).to eq(response)
     end
   end
 
   describe "#rest_errorhandler" do
     before do
-      @provider.singleton_class.send(:public, :rest_errorhandler)
+      provider.singleton_class.send(:public, :rest_errorhandler)
     end
 
     it "should have a default rest_errorhandler implementation" do
-      expect(@provider).to respond_to(:rest_errorhandler)
+      expect(provider).to respond_to(:rest_errorhandler)
     end
 
     it "should have a non-mutating rest_errorhandler implementation" do
       error_obj = StandardError.new
 
-      expect(@provider.rest_errorhandler(error_obj.dup)).to eq(error_obj)
+      expect(provider.rest_errorhandler(error_obj.dup)).to eq(error_obj)
     end
   end
 
   describe "#required_properties" do
     before do
-      @provider.singleton_class.send(:public, :required_properties)
+      provider.singleton_class.send(:public, :required_properties)
     end
 
     it "should include required properties only" do
-      expect(@provider.required_properties).to contain_exactly(:address, :prefix)
+      expect(provider.required_properties).to contain_exactly(:address, :prefix)
     end
   end
 
   describe "#property_map" do
     before do
-      @provider.singleton_class.send(:public, :property_map)
+      provider.singleton_class.send(:public, :property_map)
     end
 
     it "should map resource properties to values properly" do
-      expect(@provider.property_map).to eq({
-                                             address: "192.0.2.1",
-                                             prefix: 24,
-                                             gateway: nil,
-                                             name: "set_address",
-                                           })
+      expect(provider.property_map).to eq({
+        address: "192.0.2.1",
+        prefix: 24,
+        gateway: nil,
+        name: "set_address",
+      })
     end
   end
 
   describe "#rest_url_collection" do
     before do
-      @provider.singleton_class.send(:public, :rest_url_collection)
+      provider.singleton_class.send(:public, :rest_url_collection)
     end
 
     it "should return collection URLs properly" do
-      expect(@provider.rest_url_collection).to eq("/api/v1/addresses")
+      expect(provider.rest_url_collection).to eq("/api/v1/addresses")
     end
   end
 
   describe "#rest_url_document" do
     before do
-      @provider.singleton_class.send(:public, :rest_url_document)
+      provider.singleton_class.send(:public, :rest_url_document)
     end
 
     it "should apply URI templates to document URLs using query syntax properly" do
-      expect(@provider.rest_url_document).to eq("/api/v1/address/?ip=192.0.2.1")
+      expect(provider.rest_url_document).to eq("/api/v1/address/?ip=192.0.2.1")
     end
   end
 
   # TODO: Test with path-style URLs
   describe "#rest_identity_implicit" do
     before do
-      @provider.singleton_class.send(:public, :rest_identity_implicit)
+      provider.singleton_class.send(:public, :rest_identity_implicit)
     end
 
     it "should return implicit identity properties properly" do
-      expect(@provider.rest_identity_implicit).to eq({ "ip" => :address })
+      expect(provider.rest_identity_implicit).to eq({ "ip" => :address })
     end
   end
 
   describe "#rest_identity_values" do
     before do
-      @provider.singleton_class.send(:public, :rest_identity_values)
+      provider.singleton_class.send(:public, :rest_identity_values)
     end
 
     it "should return implicit identity properties and values properly" do
-      expect(@provider.rest_identity_values).to eq({ "ip" => "192.0.2.1" })
+      expect(provider.rest_identity_values).to eq({ "ip" => "192.0.2.1" })
     end
   end
 
   # TODO: changed_value
   # TODO: load_current_value
+
+  # this might be a functional test, but it runs on any O/S so I leave it here
+  describe "when managing a resource" do
+    before { WebMock.disable_net_connect! }
+    let(:addresses_exists) { JSON.generate([{ "address": "192.0.2.1" }]) }
+    let(:addresses_other) { JSON.generate([{ "address": "172.16.32.85" }]) }
+    let(:address_exists) { JSON.generate({ "address": "192.0.2.1", "prefix": 24, "gateway": "192.0.2.1" }) }
+    let(:prefix_wrong) { JSON.generate({ "address": "192.0.2.1", "prefix": 25, "gateway": "192.0.2.1" }) }
+
+    it "should be idempotent" do
+      stub_request(:get, "https://api.example.com/api/v1/addresses")
+        .to_return(status: 200, body: addresses_exists, headers: { "Content-Type" => "application/json" })
+      stub_request(:get, "https://api.example.com/api/v1/address/?ip=192.0.2.1")
+        .to_return(status: 200, body: address_exists, headers: { "Content-Type" => "application/json" })
+      resource.run_action(:configure)
+      expect(resource.updated_by_last_action?).to be false
+    end
+
+    it "should PATCH if a property is incorrect" do
+      stub_request(:get, "https://api.example.com/api/v1/addresses")
+        .to_return(status: 200, body: addresses_exists, headers: { "Content-Type" => "application/json" })
+      stub_request(:get, "https://api.example.com/api/v1/address/?ip=192.0.2.1")
+        .to_return(status: 200, body: prefix_wrong, headers: { "Content-Type" => "application/json" })
+      stub_request(:patch, "https://api.example.com/api/v1/address/?ip=192.0.2.1")
+        .with(
+          body: "{\"address\":\"192.0.2.1\",\"prefix\":25}",
+          headers: {
+            "Accept" => "application/json",
+            "Content-Type" => "application/json",
+          }
+        )
+        .to_return(status: 200, body: address_exists, headers: { "Content-Type" => "application/json" })
+      resource.run_action(:configure)
+      expect(resource.updated_by_last_action?).to be true
+    end
+
+    it "should POST if there's no resources at all" do
+      stub_request(:get, "https://api.example.com/api/v1/addresses")
+        .to_return(status: 200, body: "[]", headers: { "Content-Type" => "application/json" })
+      stub_request(:post, "https://api.example.com/api/v1/addresses")
+        .with(
+          body: "{\"address\":\"192.0.2.1\",\"prefix\":24,\"ip\":\"192.0.2.1\"}"
+        )
+        .to_return(status: 200, body: address_exists, headers: { "Content-Type" => "application/json" })
+      resource.run_action(:configure)
+      expect(resource.updated_by_last_action?).to be true
+    end
+
+    it "should POST if the specific resource does not exist" do
+      stub_request(:get, "https://api.example.com/api/v1/addresses")
+        .to_return(status: 200, body: addresses_other, headers: { "Content-Type" => "application/json" })
+      stub_request(:get, "https://api.example.com/api/v1/address/?ip=192.0.2.1")
+        .to_return(status: 404, body: "", headers: {})
+      stub_request(:post, "https://api.example.com/api/v1/addresses")
+        .with(
+          body: "{\"address\":\"192.0.2.1\",\"prefix\":24,\"ip\":\"192.0.2.1\"}"
+        )
+        .to_return(status: 200, body: address_exists, headers: { "Content-Type" => "application/json" })
+      resource.run_action(:configure)
+      expect(resource.updated_by_last_action?).to be true
+    end
+
+    it "should be idempotent if the resouces needs deleting and there are no resources at all" do
+      stub_request(:get, "https://api.example.com/api/v1/addresses")
+        .to_return(status: 200, body: "[]", headers: { "Content-Type" => "application/json" })
+      resource.run_action(:delete)
+      expect(resource.updated_by_last_action?).to be false
+    end
+
+    it "should be idempotent if the resource doesn't exist" do
+      stub_request(:get, "https://api.example.com/api/v1/addresses")
+        .to_return(status: 200, body: addresses_other, headers: { "Content-Type" => "application/json" })
+      stub_request(:get, "https://api.example.com/api/v1/address/?ip=192.0.2.1")
+        .to_return(status: 404, body: "", headers: {})
+      resource.run_action(:delete)
+      expect(resource.updated_by_last_action?).to be false
+    end
+
+    it "should DELETE the resource if it exists and matches" do
+      stub_request(:get, "https://api.example.com/api/v1/addresses")
+        .to_return(status: 200, body: addresses_exists, headers: { "Content-Type" => "application/json" })
+      stub_request(:get, "https://api.example.com/api/v1/address/?ip=192.0.2.1")
+        .to_return(status: 200, body: address_exists, headers: { "Content-Type" => "application/json" })
+      stub_request(:delete, "https://api.example.com/api/v1/address/?ip=192.0.2.1")
+        .to_return(status: 200, body: "", headers: {})
+      resource.run_action(:delete)
+      expect(resource.updated_by_last_action?).to be true
+    end
+
+    it "should DELETE the resource if it exists and doesn't match" do
+      stub_request(:get, "https://api.example.com/api/v1/addresses")
+        .to_return(status: 200, body: addresses_exists, headers: { "Content-Type" => "application/json" })
+      stub_request(:get, "https://api.example.com/api/v1/address/?ip=192.0.2.1")
+        .to_return(status: 200, body: prefix_wrong, headers: { "Content-Type" => "application/json" })
+      stub_request(:delete, "https://api.example.com/api/v1/address/?ip=192.0.2.1")
+        .to_return(status: 200, body: "", headers: {})
+      resource.run_action(:delete)
+      expect(resource.updated_by_last_action?).to be true
+    end
+  end
 end
 
 describe "rest_resource using path-based addressing" do
@@ -187,4 +322,5 @@ describe "rest_resource using path-based addressing" do
       expect(@provider.rest_identity_values).to eq({ "address" => "192.0.2.1" })
     end
   end
+
 end