Merge pull request #5581 from dependabot/deivid-rodriguez/lockfile-on…

…ly-bug

Fix `lockfile-only` versioning strategy not creating some updates that are expected

bin/dry-run.rb

@@ -114,7 +114,6 @@
   cache_steps: [],
   write: false,
   clone: false,
-  lockfile_only: false,
   reject_external_code: false,
   requirements_update_strategy: nil,
   commit: nil,
@@ -185,15 +184,11 @@
     $options[:write] = true
   end
 
-  opts.on("--lockfile-only", "Only update the lockfile") do |_value|
-    $options[:lockfile_only] = true
-  end
-
   opts.on("--reject-external-code", "Reject external code") do |_value|
     $options[:reject_external_code] = true
   end
 
-  opts_req_desc = "Options: auto, widen_ranges, bump_versions or " \
+  opts_req_desc = "Options: lockfile_only, auto, widen_ranges, bump_versions or " \
                   "bump_versions_if_necessary"
   opts.on("--requirements-update-strategy STRATEGY", opts_req_desc) do |value|
     value = nil if value == "auto"
@@ -701,7 +696,7 @@ def security_fix?(dependency)
   end
 
   requirements_to_unlock =
-    if $options[:lockfile_only] || !checker.requirements_unlocked_or_can_be?
+    if !checker.requirements_unlocked_or_can_be?
       if checker.can_update?(requirements_to_unlock: :none) then :none
       else
         :update_not_possible

bundler/lib/dependabot/bundler/update_checker/requirements_updater.rb

@@ -9,7 +9,7 @@ class RequirementsUpdater
         class UnfixableRequirement < StandardError; end
 
         ALLOWED_UPDATE_STRATEGIES =
-          %i(bump_versions bump_versions_if_necessary).freeze
+          %i(lockfile_only bump_versions bump_versions_if_necessary).freeze
 
         def initialize(requirements:, update_strategy:, updated_source:,
                        latest_version:, latest_resolvable_version:)
@@ -27,6 +27,8 @@ def initialize(requirements:, update_strategy:, updated_source:,
         end
 
         def updated_requirements
+          return requirements if update_strategy == :lockfile_only
+
           requirements.map do |req|
             if req[:file].include?(".gemspec")
               update_gemspec_requirement(req)

bundler/spec/dependabot/bundler/update_checker/requirements_updater_spec.rb

@@ -505,5 +505,13 @@
         end
       end
     end
+
+    context "when lockfile_only configured" do
+      let(:update_strategy) { :lockfile_only }
+
+      it "does not change any requirements" do
+        expect(updated_requirements).to eq(requirements)
+      end
+    end
   end
 end

cargo/lib/dependabot/cargo/update_checker/requirements_updater.rb

@@ -18,7 +18,7 @@ class UnfixableRequirement < StandardError; end
 
         VERSION_REGEX = /[0-9]+(?:\.[A-Za-z0-9\-*]+)*/.freeze
         ALLOWED_UPDATE_STRATEGIES =
-          %i(bump_versions bump_versions_if_necessary).freeze
+          %i(lockfile_only bump_versions bump_versions_if_necessary).freeze
 
         def initialize(requirements:, updated_source:, update_strategy:,
                        target_version:)
@@ -34,6 +34,8 @@ def initialize(requirements:, updated_source:, update_strategy:,
         end
 
         def updated_requirements
+          return requirements if update_strategy == :lockfile_only
+
           # NOTE: Order is important here. The FileUpdater needs the updated
           # requirement at index `i` to correspond to the previous requirement
           # at the same index.

cargo/spec/dependabot/cargo/update_checker/requirements_updater_spec.rb

@@ -363,5 +363,13 @@
         end
       end
     end
+
+    context "for a lockfile_only strategy" do
+      let(:update_strategy) { :lockfile_only }
+
+      it "does not change any requirements" do
+        expect(updater.updated_requirements).to eq(requirements)
+      end
+    end
   end
 end

composer/lib/dependabot/composer/update_checker/requirements_updater.rb

@@ -21,7 +21,7 @@ class RequirementsUpdater
         OR_SEPARATOR = /(?<=[a-zA-Z0-9*])[\s,]*\|\|?\s*/.freeze
         SEPARATOR = /(?:#{AND_SEPARATOR})|(?:#{OR_SEPARATOR})/.freeze
         ALLOWED_UPDATE_STRATEGIES =
-          %i(widen_ranges bump_versions bump_versions_if_necessary).freeze
+          %i(lockfile_only widen_ranges bump_versions bump_versions_if_necessary).freeze
 
         def initialize(requirements:, update_strategy:,
                        latest_resolvable_version:)
@@ -37,6 +37,7 @@ def initialize(requirements:, update_strategy:,
         end
 
         def updated_requirements
+          return requirements if update_strategy == :lockfile_only
           return requirements unless latest_resolvable_version
 
           requirements.map { |req| updated_requirement(req) }

composer/spec/dependabot/composer/update_checker/requirements_updater_spec.rb

@@ -705,5 +705,13 @@
         end
       end
     end
+
+    context "with lockfile_only as the update strategy" do
+      let(:update_strategy) { :lockfile_only }
+
+      it "does not update any requirements" do
+        expect(updater.updated_requirements).to eq(requirements)
+      end
+    end
   end
 end

npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/requirements_updater.rb

@@ -16,7 +16,7 @@ class RequirementsUpdater
         VERSION_REGEX = /[0-9]+(?:\.[A-Za-z0-9\-_]+)*/.freeze
         SEPARATOR = /(?<=[a-zA-Z0-9*])[\s|]+(?![\s|-])/.freeze
         ALLOWED_UPDATE_STRATEGIES =
-          %i(widen_ranges bump_versions bump_versions_if_necessary).freeze
+          %i(lockfile_only widen_ranges bump_versions bump_versions_if_necessary).freeze
 
         def initialize(requirements:, updated_source:, update_strategy:,
                        latest_resolvable_version:)
@@ -33,6 +33,8 @@ def initialize(requirements:, updated_source:, update_strategy:,
         end
 
         def updated_requirements
+          return requirements if update_strategy == :lockfile_only
+
           requirements.map do |req|
             req = req.merge(source: updated_source)
             next req unless latest_resolvable_version

npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/requirements_updater_spec.rb

@@ -606,5 +606,13 @@
         end
       end
     end
+
+    context "for a requirement being left alone" do
+      let(:update_strategy) { :lockfile_only }
+
+      it "does not update any requirements" do
+        expect(updater.updated_requirements).to eq(requirements)
+      end
+    end
   end
 end

python/lib/dependabot/python/update_checker/requirements_updater.rb

@@ -30,6 +30,8 @@ def initialize(requirements:, update_strategy:, has_lockfile:,
         end
 
         def updated_requirements
+          return requirements if update_strategy == :lockfile_only
+
           requirements.map do |req|
             case req[:file]
             when /setup\.(?:py|cfg)$/ then updated_setup_requirement(req)

python/spec/dependabot/python/update_checker/requirements_updater_spec.rb

@@ -635,6 +635,14 @@
           end
         end
       end
+
+      context "when asked to not change requirements" do
+        let(:update_strategy) { :lockfile_only }
+
+        it "does not update any requirements" do
+          expect(updated_requirements).to eq(requirements)
+        end
+      end
     end
   end
 end

updater/lib/dependabot/job.rb

@@ -26,7 +26,7 @@ def initialize(attributes)
       @lockfile_only                = attributes.fetch(:lockfile_only)
       @package_manager              = attributes.fetch(:package_manager)
       @reject_external_code         = attributes.fetch(:reject_external_code, false)
-      @requirements_update_strategy = attributes.fetch(:requirements_update_strategy)
+      @requirements_update_strategy = build_update_strategy(attributes.fetch(:requirements_update_strategy))
       @security_advisories          = attributes.fetch(:security_advisories)
       @security_updates_only        = attributes.fetch(:security_updates_only)
       @source                       = build_source(attributes.fetch(:source))
@@ -43,10 +43,6 @@ def clone?
         Dependabot::Utils.always_clone_for_package_manager?(@package_manager)
     end
 
-    def lockfile_only?
-      @lockfile_only
-    end
-
     def updating_a_pull_request?
       @updating_a_pull_request
     end
@@ -160,6 +156,12 @@ def name_match?(name1, name2)
       )
     end
 
+    def build_update_strategy(requirements_update_strategy)
+      return requirements_update_strategy unless requirements_update_strategy.nil?
+
+      @lockfile_only ? "lockfile_only" : nil
+    end
+
     def build_source(source_details)
       Dependabot::Source.new(
         **source_details.transform_keys { |k| k.tr("-", "_").to_sym }

updater/lib/dependabot/updater.rb

@@ -472,7 +472,7 @@ def security_update_not_possible_message(checker, latest_allowed_version,
     end
 
     def requirements_to_unlock(checker)
-      if job.lockfile_only? || !checker.requirements_unlocked_or_can_be?
+      if !checker.requirements_unlocked_or_can_be?
         if checker.can_update?(requirements_to_unlock: :none) then :none
         else
           :update_not_possible

updater/spec/dependabot/job_spec.rb

@@ -31,7 +31,7 @@
         "username" => "x-access-token",
         "password" => "github-token"
       }],
-      lockfile_only: false,
+      lockfile_only: lockfile_only,
       requirements_update_strategy: nil,
       update_subdependencies: false,
       updating_a_pull_request: false,
@@ -45,6 +45,7 @@
   let(:dependencies) { nil }
   let(:security_advisories) { [] }
   let(:package_manager) { "bundler" }
+  let(:lockfile_only) { false }
   let(:security_updates_only) { false }
   let(:allowed_updates) do
     [
@@ -62,6 +63,14 @@
   let(:commit_message_options) { nil }
   let(:vendor_dependencies) { false }
 
+  context "when lockfile_only is passed as true" do
+    let(:lockfile_only) { true }
+
+    it "infers a lockfile_only requirements_update_strategy" do
+      expect(subject.requirements_update_strategy).to eq("lockfile_only")
+    end
+  end
+
   describe "#allowed_update?" do
     subject { job.allowed_update?(dependency) }
     let(:dependency) do

updater/spec/dependabot/updater_spec.rb

@@ -291,6 +291,20 @@
       end
     end
 
+    context "when lockfile_only is set in the job" do
+      let(:lockfile_only) { true }
+
+      it "still tries to unlock requirements of dependencies" do
+        allow(checker).
+          to receive(:can_update?).with(requirements_to_unlock: :own).
+          and_return(true)
+        expect(logger).
+          to receive(:info).
+          with("<job_1> Requirements to unlock own")
+        updater.run
+      end
+    end
+
     context "when no dependencies are allowed" do
       let(:allowed_updates) { [{ "dependency-name" => "typoed-dep-name" }] }