From 1790ce799de13814e6f3e423258ff8cdfaf22a00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Wed, 24 Aug 2022 20:24:26 +0200 Subject: [PATCH 1/2] Fix lockfile-only versioning strategy not creating expected updates The lockfile-only versioning strategy is currently handled by only allowing the dependency itself to be unlocked, but not other dependencies that might depend on it. This is incorrect, because it could perfectly happen that updating a dependency needs other dependencies to be unlocked too to succeed, yet the end result does not involve anything other than lockfile changes. See #5569 for an example. This commit reimplements the lockfile-only versioning strategy by passing it to UpdateCheckers as an update strategy and avoid updating any requirements in that case. I also think this is a good move, because it makes the lockfile-only versioning strategy actually testable, since it now lives inside a core class. --- bin/dry-run.rb | 9 ++------- .../bundler/update_checker/requirements_updater.rb | 4 +++- .../bundler/update_checker/requirements_updater_spec.rb | 8 ++++++++ .../cargo/update_checker/requirements_updater.rb | 4 +++- .../cargo/update_checker/requirements_updater_spec.rb | 8 ++++++++ .../composer/update_checker/requirements_updater.rb | 3 ++- .../composer/update_checker/requirements_updater_spec.rb | 8 ++++++++ .../npm_and_yarn/update_checker/requirements_updater.rb | 4 +++- .../update_checker/requirements_updater_spec.rb | 8 ++++++++ .../python/update_checker/requirements_updater.rb | 2 ++ .../python/update_checker/requirements_updater_spec.rb | 8 ++++++++ 11 files changed, 55 insertions(+), 11 deletions(-) diff --git a/bin/dry-run.rb b/bin/dry-run.rb index 811b5ed9087..6e1e110cc48 100755 --- a/bin/dry-run.rb +++ b/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 diff --git a/bundler/lib/dependabot/bundler/update_checker/requirements_updater.rb b/bundler/lib/dependabot/bundler/update_checker/requirements_updater.rb index 9ae4bd1e051..bf5975384b8 100644 --- a/bundler/lib/dependabot/bundler/update_checker/requirements_updater.rb +++ b/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) diff --git a/bundler/spec/dependabot/bundler/update_checker/requirements_updater_spec.rb b/bundler/spec/dependabot/bundler/update_checker/requirements_updater_spec.rb index d0ca3660cae..a9c63c9ba94 100644 --- a/bundler/spec/dependabot/bundler/update_checker/requirements_updater_spec.rb +++ b/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 diff --git a/cargo/lib/dependabot/cargo/update_checker/requirements_updater.rb b/cargo/lib/dependabot/cargo/update_checker/requirements_updater.rb index 77eac69f596..0c67fcea5e2 100644 --- a/cargo/lib/dependabot/cargo/update_checker/requirements_updater.rb +++ b/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. diff --git a/cargo/spec/dependabot/cargo/update_checker/requirements_updater_spec.rb b/cargo/spec/dependabot/cargo/update_checker/requirements_updater_spec.rb index 57fc55d99e1..7cdda35cff7 100644 --- a/cargo/spec/dependabot/cargo/update_checker/requirements_updater_spec.rb +++ b/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 diff --git a/composer/lib/dependabot/composer/update_checker/requirements_updater.rb b/composer/lib/dependabot/composer/update_checker/requirements_updater.rb index 8d708e0363b..45b4d3cb891 100644 --- a/composer/lib/dependabot/composer/update_checker/requirements_updater.rb +++ b/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) } diff --git a/composer/spec/dependabot/composer/update_checker/requirements_updater_spec.rb b/composer/spec/dependabot/composer/update_checker/requirements_updater_spec.rb index 395a57f72bc..1c9e8f984c7 100644 --- a/composer/spec/dependabot/composer/update_checker/requirements_updater_spec.rb +++ b/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 diff --git a/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/requirements_updater.rb b/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/requirements_updater.rb index 634295aab65..4899d4eeea4 100644 --- a/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/requirements_updater.rb +++ b/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 diff --git a/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/requirements_updater_spec.rb b/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/requirements_updater_spec.rb index e41b5e85baa..17ed59075ed 100644 --- a/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/requirements_updater_spec.rb +++ b/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 diff --git a/python/lib/dependabot/python/update_checker/requirements_updater.rb b/python/lib/dependabot/python/update_checker/requirements_updater.rb index fb81d111f7e..bf1a5df6b05 100644 --- a/python/lib/dependabot/python/update_checker/requirements_updater.rb +++ b/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) diff --git a/python/spec/dependabot/python/update_checker/requirements_updater_spec.rb b/python/spec/dependabot/python/update_checker/requirements_updater_spec.rb index d8b51dfcbcd..ae71e69c02d 100644 --- a/python/spec/dependabot/python/update_checker/requirements_updater_spec.rb +++ b/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 From e14d5f9cb1d633e005e31ebb6b73f017432ef077 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Mon, 5 Sep 2022 18:43:17 +0200 Subject: [PATCH 2/2] Bare support for `lockfile_only` strategy in updater Hopefully in a backwards compatible way. --- updater/lib/dependabot/job.rb | 12 +++++++----- updater/lib/dependabot/updater.rb | 2 +- updater/spec/dependabot/job_spec.rb | 11 ++++++++++- updater/spec/dependabot/updater_spec.rb | 14 ++++++++++++++ 4 files changed, 32 insertions(+), 7 deletions(-) diff --git a/updater/lib/dependabot/job.rb b/updater/lib/dependabot/job.rb index 04b186dd9c2..962a3cee609 100644 --- a/updater/lib/dependabot/job.rb +++ b/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 } diff --git a/updater/lib/dependabot/updater.rb b/updater/lib/dependabot/updater.rb index 26e2ffa2c08..217ae7fe6df 100644 --- a/updater/lib/dependabot/updater.rb +++ b/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 diff --git a/updater/spec/dependabot/job_spec.rb b/updater/spec/dependabot/job_spec.rb index eb8d1d3a6b7..3e1e490ae9a 100644 --- a/updater/spec/dependabot/job_spec.rb +++ b/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 diff --git a/updater/spec/dependabot/updater_spec.rb b/updater/spec/dependabot/updater_spec.rb index fc74362631e..ea187114aea 100644 --- a/updater/spec/dependabot/updater_spec.rb +++ b/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(" Requirements to unlock own") + updater.run + end + end + context "when no dependencies are allowed" do let(:allowed_updates) { [{ "dependency-name" => "typoed-dep-name" }] }