Update gem_package with version constraint on :upgrade action

Add Package#version_requirement_satisfied? to separate fuzzy-match (:install) and exact-match (:upgrade) version checks when supported by the package subclass.
Wrap Package#candidate_version in a lazy-initialization object to minimize unnecessary API queries when candidate_version is not needed.
(fixes #3867)

4 files changed, 107 insertions, 31 deletions

diff --git a/lib/chef/provider/package.rb b/lib/chef/provider/package.rb
index ca9b526920..4a37ef533d 100644
--- a/lib/chef/provider/package.rb
+++ b/lib/chef/provider/package.rb
@@ -256,9 +256,16 @@ class Chef
         options ? " #{options}" : ""
       end
 
-      # this is public and overridden by subclasses (rubygems package implements '>=' and '~>' operators)
-      def target_version_already_installed?(current_version, new_version)
-        new_version == current_version
+      # this is public and overridden by subclasses
+      def target_version_already_installed?(current_version, target_version)
+        current_version && target_version &&
+          current_version == target_version
+      end
+
+      # this is public and overridden by subclasses
+      # (rubygems package implements '>=' and '~>' operators)
+      def version_requirement_satisfied?(current_version, version_requirement)
+        target_version_already_installed?(current_version, version_requirement)
       end
 
       # @todo: extract apt/dpkg specific preseeding to a helper class
@@ -355,11 +362,12 @@ class Chef
               case action
               when :upgrade
 
-                if !candidate_version
-                  Chef::Log.debug("#{new_resource} #{package_name} has no candidate_version to upgrade to")
+                if target_version_already_installed?(current_version, new_version) ||
+                    target_version_already_installed?(current_version, candidate_version)
+                  Chef::Log.debug("#{new_resource} #{package_name} #{new_version} is already installed")
                   target_version_array.push(nil)
-                elsif current_version == candidate_version
-                  Chef::Log.debug("#{new_resource} #{package_name} the #{candidate_version} is already installed")
+                elsif candidate_version.nil?
+                  Chef::Log.debug("#{new_resource} #{package_name} has no candidate_version to upgrade to")
                   target_version_array.push(nil)
                 else
                   Chef::Log.debug("#{new_resource} #{package_name} is out of date, will upgrade to #{candidate_version}")
@@ -369,7 +377,7 @@ class Chef
               when :install
 
                 if new_version
-                  if target_version_already_installed?(current_version, new_version)
+                  if version_requirement_satisfied?(current_version, new_version)
                     Chef::Log.debug("#{new_resource} #{package_name} #{current_version} satisifies #{new_version} requirement")
                     target_version_array.push(nil)
                   else
@@ -411,7 +419,7 @@ class Chef
           begin
             missing = []
             each_package do |package_name, new_version, current_version, candidate_version|
-              missing.push(package_name) if candidate_version.nil? && current_version.nil?
+              missing.push(package_name) if current_version.nil? && candidate_version.nil?
             end
             missing
           end
@@ -436,7 +444,7 @@ class Chef
             missing = []
             each_package do |package_name, new_version, current_version, candidate_version|
               next if new_version.nil? || current_version.nil?
-              if candidate_version.nil? && !target_version_already_installed?(current_version, new_version)
+              if !version_requirement_satisfied?(current_version, new_version) && candidate_version.nil?
                 missing.push(package_name)
               end
             end
@@ -468,7 +476,32 @@ class Chef
 
       # @return [Array] candidate_version(s) as an array
       def candidate_version_array
-        [ candidate_version ].flatten
+        use_multipackage_api? ?
+          [ candidate_version ].flatten :
+          [ LazyObject.new { candidate_version } ]
+      end
+
+      # Wrap single candidate_version in a lazy object to minimize unnecessary API queries.
+      class LazyObject < BasicObject
+        NULL = ::Object.new
+
+        def initialize(&block)
+          @block = block
+          @target = NULL
+        end
+
+        def __obj
+          @target = @block.call if @target == NULL
+          @target
+        end
+
+        def ==(other_object)
+          __obj == other_object
+        end
+
+        def method_missing(method_name, *args, &block)
+          __obj.send(method_name, *args, &block)
+        end
       end
 
       # @return [Array] current_version(s) as an array
diff --git a/lib/chef/provider/package/rubygems.rb b/lib/chef/provider/package/rubygems.rb
index a15ee1bdea..cb3ad89bba 100644
--- a/lib/chef/provider/package/rubygems.rb
+++ b/lib/chef/provider/package/rubygems.rb
@@ -482,21 +482,28 @@ class Chef
 
         def candidate_version
           @candidate_version ||= begin
-                                   if target_version_already_installed?(@current_resource.version, @new_resource.version)
-                                     nil
-                                   elsif source_is_remote?
-                                     @gem_env.candidate_version_from_remote(gem_dependency, *gem_sources).to_s
-                                   else
-                                     @gem_env.candidate_version_from_file(gem_dependency, @new_resource.source).to_s
-                                   end
-                                 end
+                                  if source_is_remote?
+                                    @gem_env.candidate_version_from_remote(gem_dependency, *gem_sources).to_s
+                                  else
+                                    @gem_env.candidate_version_from_file(gem_dependency, @new_resource.source).to_s
+                                  end
+                                end
         end
 
         def target_version_already_installed?(current_version, new_version)
-          return false unless current_version
-          return false if new_version.nil?
+          match_version(current_version, new_version, false)
+        end
+
+        def version_requirement_satisfied?(current_version, version_requirement)
+          match_version(current_version, version_requirement, true)
+        end
+
+        def match_version(current_version, new_version, fuzzy_match)
+          return false unless current_version && new_version
 
-          Gem::Requirement.new(new_version).satisfied_by?(Gem::Version.new(current_version))
+          requirement = Gem::Requirement.new(new_version)
+          (fuzzy_match || requirement.exact?) &&
+            requirement.satisfied_by?(Gem::Version.new(current_version))
         end
 
         ##
diff --git a/spec/unit/provider/package/rubygems_spec.rb b/spec/unit/provider/package/rubygems_spec.rb
index 6e77e7c112..29de4b4d11 100644
--- a/spec/unit/provider/package/rubygems_spec.rb
+++ b/spec/unit/provider/package/rubygems_spec.rb
@@ -496,12 +496,35 @@ describe Chef::Provider::Package::Rubygems do
         provider.load_current_resource
       end
 
-      it "does not query for available versions when the current version is the target version" do
-        expect(provider.candidate_version).to be_nil
+      context "when the current version is the target version" do
+        it "does not query for available versions" do
+          expect(provider.gem_env).not_to receive(:candidate_version_from_remote)
+          expect(provider.gem_env).not_to receive(:install)
+          provider.run_action(:upgrade)
+          expect(new_resource).not_to be_updated_by_last_action
+        end
       end
 
-      context "when the current version is not the target version" do
-        let(:target_version) { "9000.0.2" }
+      context "when the current version satisfies the target version requirement" do
+        let(:target_version) { ">= 0" }
+
+        it "does not query for available versions on install" do
+          expect(provider.gem_env).not_to receive(:candidate_version_from_remote)
+          expect(provider.gem_env).not_to receive(:install)
+          provider.run_action(:install)
+          expect(new_resource).not_to be_updated_by_last_action
+        end
+
+        it "queries for available versions on upgrade" do
+          expect(provider.gem_env).to receive(:candidate_version_from_remote).
+            and_return(Gem::Version.new("9000.0.2"))
+          expect(provider.gem_env).to receive(:install)
+          provider.run_action(:upgrade)
+          expect(new_resource).to be_updated_by_last_action
+        end
+      end
+
+      context "when the requested source is a remote server" do
         let(:source) { "http://mygems.example.com" }
 
         it "determines the candidate version by querying the remote gem servers" do
@@ -669,6 +692,11 @@ describe Chef::Provider::Package::Rubygems do
               expect(provider.gem_env).not_to receive(:install)
               provider.run_action(:install)
             end
+
+            it "performs the upgrade" do
+              expect(provider.gem_env).to receive(:install)
+              provider.run_action(:upgrade)
+            end
           end
 
           context "if the fuzzy operator is used" do
@@ -679,6 +707,11 @@ describe Chef::Provider::Package::Rubygems do
               expect(provider.gem_env).not_to receive(:install)
               provider.run_action(:install)
             end
+
+            it "it upgrades an existing gem" do
+              expect(provider.gem_env).to receive(:install)
+              provider.run_action(:upgrade)
+            end
           end
         end
       end
diff --git a/spec/unit/provider/package_spec.rb b/spec/unit/provider/package_spec.rb
index abf0322868..e4354d0db4 100644
--- a/spec/unit/provider/package_spec.rb
+++ b/spec/unit/provider/package_spec.rb
@@ -566,8 +566,11 @@ describe "Chef::Provider::Package - Multi" do
   let(:new_resource) { Chef::Resource::Package.new(%w{emacs vi}) }
   let(:current_resource) { Chef::Resource::Package.new(%w{emacs vi}) }
   let(:candidate_version) { [ "1.0", "6.2" ] }
+  class MyPackageProvider < Chef::Provider::Package
+    use_multipackage_api
+  end
   let(:provider) do
-    provider = Chef::Provider::Package.new(new_resource, run_context)
+    provider = MyPackageProvider.new(new_resource, run_context)
     provider.current_resource = current_resource
     provider.candidate_version = candidate_version
     provider
@@ -731,7 +734,7 @@ describe "Chef::Provider::Package - Multi" do
 
     it "should remove the packages if all are installed" do
       expect(provider).to be_removing_package
-      expect(provider).to receive(:remove_package).with(%w{emacs vi}, nil)
+      expect(provider).to receive(:remove_package).with(%w{emacs vi}, [nil])
       provider.run_action(:remove)
       expect(new_resource).to be_updated
       expect(new_resource).to be_updated_by_last_action
@@ -740,7 +743,7 @@ describe "Chef::Provider::Package - Multi" do
     it "should remove the packages if some are installed" do
       current_resource.version ["1.0", nil]
       expect(provider).to be_removing_package
-      expect(provider).to receive(:remove_package).with(%w{emacs vi}, nil)
+      expect(provider).to receive(:remove_package).with(%w{emacs vi}, [nil])
       provider.run_action(:remove)
       expect(new_resource).to be_updated
       expect(new_resource).to be_updated_by_last_action
@@ -787,7 +790,7 @@ describe "Chef::Provider::Package - Multi" do
 
     it "should purge the packages if all are installed" do
       expect(provider).to be_removing_package
-      expect(provider).to receive(:purge_package).with(%w{emacs vi}, nil)
+      expect(provider).to receive(:purge_package).with(%w{emacs vi}, [nil])
       provider.run_action(:purge)
       expect(new_resource).to be_updated
       expect(new_resource).to be_updated_by_last_action
@@ -796,7 +799,7 @@ describe "Chef::Provider::Package - Multi" do
     it "should purge the packages if some are installed" do
       current_resource.version ["1.0", nil]
       expect(provider).to be_removing_package
-      expect(provider).to receive(:purge_package).with(%w{emacs vi}, nil)
+      expect(provider).to receive(:purge_package).with(%w{emacs vi}, [nil])
       provider.run_action(:purge)
       expect(new_resource).to be_updated
       expect(new_resource).to be_updated_by_last_action