From 455c0c3de7c1a13fd360c4d2ac7b7412d3bde49e Mon Sep 17 00:00:00 2001 From: Barry Gordon Date: Wed, 29 Mar 2023 15:52:29 +0100 Subject: [PATCH 1/8] Add a descripton to the UpdateAllVersions operation class --- .../lib/dependabot/updater/operations/update_all_versions.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/updater/lib/dependabot/updater/operations/update_all_versions.rb b/updater/lib/dependabot/updater/operations/update_all_versions.rb index a58e49b6f60..5ca4f84762a 100644 --- a/updater/lib/dependabot/updater/operations/update_all_versions.rb +++ b/updater/lib/dependabot/updater/operations/update_all_versions.rb @@ -1,5 +1,8 @@ # frozen_string_literal: true +# This class implements our strategy for iterating over all of the dependencies +# for a specific project folder to find those that are out of date and create +# a single PR per Dependency. module Dependabot class Updater module Operations From f995e6c4df093f0fed2aaf97c01a45672e4ea303 Mon Sep 17 00:00:00 2001 From: Barry Gordon Date: Wed, 29 Mar 2023 21:19:27 +0100 Subject: [PATCH 2/8] Clone all updater code required for a refresh version update op --- updater/lib/dependabot/updater/operations.rb | 4 +- .../refresh_version_pull_request.rb | 375 ++++++++++++++++++ .../dependabot/updater/operations_spec.rb | 10 + updater/spec/dependabot/updater_spec.rb | 12 +- 4 files changed, 390 insertions(+), 11 deletions(-) create mode 100644 updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb diff --git a/updater/lib/dependabot/updater/operations.rb b/updater/lib/dependabot/updater/operations.rb index 88350193eec..c7b175c83ad 100644 --- a/updater/lib/dependabot/updater/operations.rb +++ b/updater/lib/dependabot/updater/operations.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "dependabot/updater/operations/group_update_all_versions" +require "dependabot/updater/operations/refresh_version_pull_request" require "dependabot/updater/operations/update_all_versions" # This module is responsible for determining which Operation a Job is requesting @@ -26,7 +27,8 @@ module Operations # specific preconditions go before those with more permissive checks. OPERATIONS = [ GroupUpdateAllVersions, - UpdateAllVersions + UpdateAllVersions, + RefreshVersionPullRequest ] def self.class_for(job:) diff --git a/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb b/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb new file mode 100644 index 00000000000..2f8989d8a8e --- /dev/null +++ b/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb @@ -0,0 +1,375 @@ +# frozen_string_literal: true + +# This class implements our strategy for 'refreshing' an existing Pull Request +# that updates a dependnency to the latest permitted version. +# +# It will determine if the existing diff is still relevant, in which case it +# functions similar to a "rebase", but in the case where the project folder's +# dependencies have changed or a newer version is available, it will supersede +# the existing pull request with a new one for clarity. +module Dependabot + class Updater + module Operations + class RefreshVersionPullRequest + def self.applies_to?(job:) + return false if job.security_updates_only? + # If we haven't been given metadata about the dependencies present + # in the pull request, this strategy cannot act. + return false if job.dependencies&.none? + + job.updating_a_pull_request? + end + + def initialize(service:, job:, dependency_snapshot:, error_handler:) + @service = service + @job = job + @dependency_snapshot = dependency_snapshot + @error_handler = error_handler + end + + def perform + Dependabot.logger.info("Starting PR update job for #{job.source.repo}") + check_and_update_existing_pr_with_error_handling(dependencies) + end + + private + + attr_reader :job, + :service, + :dependency_snapshot, + :error_handler, + :created_pull_requests + + def dependencies + all_deps = dependency_snapshot.dependencies + + # Rebases and security updates have dependencies, version updates don't + if job.dependencies + # Gradle, Maven and Nuget dependency names can be case-insensitive and + # the dependency name in the security advisory often doesn't match what + # users have specified in their manifest. + # + # It's technically possibly to publish case-sensitive npm packages to a + # private registry but shouldn't cause problems here as job.dependencies + # is set either from an existing PR rebase/recreate or a security + # advisory. + job_dependencies = job.dependencies.map(&:downcase) + return all_deps.select do |dep| + job_dependencies.include?(dep.name.downcase) + end + end + + allowed_deps = all_deps.select { |d| job.allowed_update?(d) } + # Return dependencies in a random order, with top-level dependencies + # considered first so that dependency runs which time out don't always hit + # the same dependencies + allowed_deps = allowed_deps.shuffle unless ENV["UPDATER_DETERMINISTIC"] + + if all_deps.any? && allowed_deps.none? + Dependabot.logger.info("Found no dependencies to update after filtering allowed " \ + "updates") + end + + # Consider updating vulnerable deps first. Only consider the first 10, + # though, to ensure they don't take up the entire update run + deps = allowed_deps.select { |d| job.vulnerable?(d) }.sample(10) + + allowed_deps.reject { |d| job.vulnerable?(d) } + + deps + end + + def check_and_update_existing_pr_with_error_handling(dependencies) + dependency = dependencies.last + check_and_update_pull_request(dependencies) + rescue StandardError => e + error_handler.handle_dependabot_error(error: e, dependency: dependency) + end + + # rubocop:disable Metrics/AbcSize + # rubocop:disable Metrics/CyclomaticComplexity + # rubocop:disable Metrics/PerceivedComplexity + # rubocop:disable Metrics/MethodLength + # + # TODO: Push checks on dependencies into Dependabot::DependencyChange + # + # Some of this logic would make more sense as interrogations of the + # DependencyChange as we build it up step-by-step. + def check_and_update_pull_request(dependencies) + if dependencies.count != job.dependencies.count + # If the job dependencies mismatch the parsed dependencies, then + # we should close the PR as at least one thing we changed has been + # removed from the project. + close_pull_request(reason: :dependency_removed) + return + end + + # NOTE: Prevent security only updates from turning into latest version + # updates if the current version is no longer vulnerable. This happens + # when a security update is applied by the user directly and the existing + # pull request is rebased. + if job.security_updates_only? && + dependencies.none? { |d| job.allowed_update?(d) } + lead_dependency = dependencies.first + if job.vulnerable?(lead_dependency) + Dependabot.logger.info( + "Dependency no longer allowed to update #{lead_dependency.name} #{lead_dependency.version}" + ) + else + Dependabot.logger.info("No longer vulnerable #{lead_dependency.name} #{lead_dependency.version}") + end + close_pull_request(reason: :up_to_date) + return + end + + # The first dependency is the "lead" dependency in a multi-dependency + # update - i.e., the one we're trying to update. + # + # Note: Gradle, Maven and Nuget dependency names can be case-insensitive + # and the dependency name in the security advisory often doesn't match + # what users have specified in their manifest. + lead_dep_name = job.dependencies.first.downcase + lead_dependency = dependencies.find do |dep| + dep.name.downcase == lead_dep_name + end + checker = update_checker_for(lead_dependency, raise_on_ignored: raise_on_ignored?(lead_dependency)) + log_checking_for_update(lead_dependency) + + return if all_versions_ignored?(lead_dependency, checker) + + return close_pull_request(reason: :up_to_date) if checker.up_to_date? + + requirements_to_unlock = requirements_to_unlock(checker) + log_requirements_for_update(requirements_to_unlock, checker) + + if requirements_to_unlock == :update_not_possible + return close_pull_request(reason: :update_no_longer_possible) + end + + updated_deps = checker.updated_dependencies( + requirements_to_unlock: requirements_to_unlock + ) + + updated_files = generate_dependency_files_for(updated_deps) + updated_deps = updated_deps.reject do |d| + next false if d.name == checker.dependency.name + next true if d.top_level? && d.requirements == d.previous_requirements + + d.version == d.previous_version + end + + # NOTE: Gradle, Maven and Nuget dependency names can be case-insensitive + # and the dependency name in the security advisory often doesn't match + # what users have specified in their manifest. + job_dependencies = job.dependencies.map(&:downcase) + if updated_deps.map(&:name).map(&:downcase) != job_dependencies + # The dependencies being updated have changed. Close the existing + # multi-dependency PR and try creating a new one. + close_pull_request(reason: :dependencies_changed) + create_pull_request(updated_deps, updated_files) + elsif existing_pull_request(updated_deps) + # The existing PR is for this version. Update it. + update_pull_request(updated_deps, updated_files) + else + # The existing PR is for a previous version. Supersede it. + create_pull_request(updated_deps, updated_files) + end + end + # rubocop:enable Metrics/AbcSize + # rubocop:enable Metrics/CyclomaticComplexity + # rubocop:enable Metrics/PerceivedComplexity + # rubocop:enable Metrics/MethodLength + + def create_pull_request(dependencies, updated_dependency_files) + Dependabot.logger.info("Submitting #{dependencies.map(&:name).join(', ')} " \ + "pull request for creation") + + dependency_change = Dependabot::DependencyChange.new( + job: job, + dependencies: dependencies, + updated_dependency_files: updated_dependency_files + ) + + service.create_pull_request(dependency_change, dependency_snapshot.base_commit_sha) + end + + def update_pull_request(dependencies, updated_dependency_files) + Dependabot.logger.info("Submitting #{dependencies.map(&:name).join(', ')} " \ + "pull request for update") + + dependency_change = Dependabot::DependencyChange.new( + job: job, + dependencies: dependencies, + updated_dependency_files: updated_dependency_files + ) + + service.update_pull_request(dependency_change, dependency_snapshot.base_commit_sha) + end + + def close_pull_request(reason:) + reason_string = reason.to_s.tr("_", " ") + Dependabot.logger.info("Telling backend to close pull request for " \ + "#{job.dependencies.join(', ')} - #{reason_string}") + service.close_pull_request(job.dependencies, reason) + end + + def raise_on_ignored?(dependency) + job.security_updates_only? || ignore_conditions_for(dependency).any? + end + + def ignore_conditions_for(dep) + update_config_ignored_versions(job.ignore_conditions, dep) + end + + def update_config_ignored_versions(ignore_conditions, dep) + ignore_conditions = ignore_conditions.map do |ic| + Dependabot::Config::IgnoreCondition.new( + dependency_name: ic["dependency-name"], + versions: [ic["version-requirement"]].compact, + update_types: ic["update-types"] + ) + end + Dependabot::Config::UpdateConfig. + new(ignore_conditions: ignore_conditions). + ignored_versions_for(dep, security_updates_only: job.security_updates_only?) + end + + def update_checker_for(dependency, raise_on_ignored:) + Dependabot::UpdateCheckers.for_package_manager(job.package_manager).new( + dependency: dependency, + dependency_files: dependency_snapshot.dependency_files, + repo_contents_path: job.repo_contents_path, + credentials: job.credentials, + ignored_versions: ignore_conditions_for(dependency), + security_advisories: security_advisories_for(dependency), + raise_on_ignored: raise_on_ignored, + requirements_update_strategy: job.requirements_update_strategy, + options: job.experiments + ) + end + + def file_updater_for(dependencies) + Dependabot::FileUpdaters.for_package_manager(job.package_manager).new( + dependencies: dependencies, + dependency_files: dependency_snapshot.dependency_files, + repo_contents_path: job.repo_contents_path, + credentials: job.credentials, + options: job.experiments + ) + end + + def security_advisories_for(dep) + relevant_advisories = + job.security_advisories. + select { |adv| adv.fetch("dependency-name").casecmp(dep.name).zero? } + + relevant_advisories.map do |adv| + vulnerable_versions = adv["affected-versions"] || [] + safe_versions = (adv["patched-versions"] || []) + + (adv["unaffected-versions"] || []) + + Dependabot::SecurityAdvisory.new( + dependency_name: dep.name, + package_manager: job.package_manager, + vulnerable_versions: vulnerable_versions, + safe_versions: safe_versions + ) + end + end + + def log_checking_for_update(dependency) + Dependabot.logger.info( + "Checking if #{dependency.name} #{dependency.version} needs updating" + ) + log_ignore_conditions(dependency) + end + + def log_ignore_conditions(dep) + conditions = job.ignore_conditions. + select { |ic| name_match?(ic["dependency-name"], dep.name) } + return if conditions.empty? + + Dependabot.logger.info("Ignored versions:") + conditions.each do |ic| + unless ic["version-requirement"].nil? + Dependabot.logger.info(" #{ic['version-requirement']} - from #{ic['source']}") + end + + ic["update-types"]&.each do |update_type| + msg = " #{update_type} - from #{ic['source']}" + msg += " (doesn't apply to security update)" if job.security_updates_only? + Dependabot.logger.info(msg) + end + end + end + + def all_versions_ignored?(dependency, checker) + Dependabot.logger.info("Latest version is #{checker.latest_version}") + false + rescue Dependabot::AllVersionsIgnored + Dependabot.logger.info("All updates for #{dependency.name} were ignored") + + # Report this error to the backend to create an update job error + raise if job.security_updates_only? + + true + end + + def requirements_to_unlock(checker) + if job.lockfile_only? || !checker.requirements_unlocked_or_can_be? + if checker.can_update?(requirements_to_unlock: :none) then :none + else + :update_not_possible + end + elsif checker.can_update?(requirements_to_unlock: :own) then :own + elsif checker.can_update?(requirements_to_unlock: :all) then :all + else + :update_not_possible + end + end + + def log_requirements_for_update(requirements_to_unlock, checker) + Dependabot.logger.info("Requirements to unlock #{requirements_to_unlock}") + + return unless checker.respond_to?(:requirements_update_strategy) + + Dependabot.logger.info( + "Requirements update strategy #{checker.requirements_update_strategy}" + ) + end + + def existing_pull_request(updated_dependencies) + new_pr_set = Set.new( + updated_dependencies.map do |dep| + { + "dependency-name" => dep.name, + "dependency-version" => dep.version, + "dependency-removed" => dep.removed? ? true : nil + }.compact + end + ) + + job.existing_pull_requests.find { |pr| Set.new(pr) == new_pr_set } + end + + def generate_dependency_files_for(updated_dependencies) + if updated_dependencies.count == 1 + updated_dependency = updated_dependencies.first + Dependabot.logger.info("Updating #{updated_dependency.name} from " \ + "#{updated_dependency.previous_version} to " \ + "#{updated_dependency.version}") + else + dependency_names = updated_dependencies.map(&:name) + Dependabot.logger.info("Updating #{dependency_names.join(', ')}") + end + + # Ignore dependencies that are tagged as information_only. These will be + # updated indirectly as a result of a parent dependency update and are + # only included here to be included in the PR info. + deps_to_update = updated_dependencies.reject(&:informational_only?) + updater = file_updater_for(deps_to_update) + updater.updated_dependency_files + end + end + end + end +end diff --git a/updater/spec/dependabot/updater/operations_spec.rb b/updater/spec/dependabot/updater/operations_spec.rb index a732b7dd563..b5f30d08f6d 100644 --- a/updater/spec/dependabot/updater/operations_spec.rb +++ b/updater/spec/dependabot/updater/operations_spec.rb @@ -46,6 +46,16 @@ end end + it "returns the RefreshVersionPullRequest class when the Job is for an existing dependency version update" do + job = instance_double(Dependabot::Job, + security_updates_only?: false, + updating_a_pull_request?: true, + dependencies: [anything], + is_a?: true) + + expect(described_class.class_for(job: job)).to be(Dependabot::Updater::Operations::RefreshVersionPullRequest) + end + it "raises an argument error with anything other than a Dependabot::Job" do expect { described_class.class_for(job: Object.new) }.to raise_error(ArgumentError) end diff --git a/updater/spec/dependabot/updater_spec.rb b/updater/spec/dependabot/updater_spec.rb index a4523a4fa8a..0ed2f2ad70d 100644 --- a/updater/spec/dependabot/updater_spec.rb +++ b/updater/spec/dependabot/updater_spec.rb @@ -1134,11 +1134,7 @@ def expect_update_checker_with_ignored_versions(versions) service = build_service updater = build_updater(service: service, job: job) - expect(updater). - to receive(:check_and_update_existing_pr_with_error_handling). - and_call_original - expect(updater). - to_not receive(:check_and_create_pr_with_error_handling) + expect(Dependabot::Updater::Operations::RefreshVersionPullRequest).to receive(:new).and_call_original expect(service).to receive(:create_pull_request).once updater.run @@ -1245,11 +1241,7 @@ def expect_update_checker_with_ignored_versions(versions) service = build_service updater = build_updater(service: service, job: job) - expect(updater). - to receive(:check_and_update_existing_pr_with_error_handling). - and_call_original - expect(updater). - to_not receive(:check_and_create_pr_with_error_handling) + expect(Dependabot::Updater::Operations::RefreshVersionPullRequest).to receive(:new).and_call_original expect(service).to receive(:create_pull_request).once expect(service).not_to receive(:close_pull_request) From 6494e21fd430f0ff5dbe3bc5005ed319ca31ebf7 Mon Sep 17 00:00:00 2001 From: Barry Gordon Date: Wed, 29 Mar 2023 21:30:08 +0100 Subject: [PATCH 3/8] Remove all security-update related concerns --- .../refresh_version_pull_request.rb | 34 ++----------------- 1 file changed, 3 insertions(+), 31 deletions(-) diff --git a/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb b/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb index 2f8989d8a8e..6257b2eea31 100644 --- a/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb +++ b/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb @@ -70,12 +70,7 @@ def dependencies "updates") end - # Consider updating vulnerable deps first. Only consider the first 10, - # though, to ensure they don't take up the entire update run - deps = allowed_deps.select { |d| job.vulnerable?(d) }.sample(10) + - allowed_deps.reject { |d| job.vulnerable?(d) } - - deps + allowed_deps end def check_and_update_existing_pr_with_error_handling(dependencies) @@ -103,24 +98,6 @@ def check_and_update_pull_request(dependencies) return end - # NOTE: Prevent security only updates from turning into latest version - # updates if the current version is no longer vulnerable. This happens - # when a security update is applied by the user directly and the existing - # pull request is rebased. - if job.security_updates_only? && - dependencies.none? { |d| job.allowed_update?(d) } - lead_dependency = dependencies.first - if job.vulnerable?(lead_dependency) - Dependabot.logger.info( - "Dependency no longer allowed to update #{lead_dependency.name} #{lead_dependency.version}" - ) - else - Dependabot.logger.info("No longer vulnerable #{lead_dependency.name} #{lead_dependency.version}") - end - close_pull_request(reason: :up_to_date) - return - end - # The first dependency is the "lead" dependency in a multi-dependency # update - i.e., the one we're trying to update. # @@ -213,7 +190,7 @@ def close_pull_request(reason:) end def raise_on_ignored?(dependency) - job.security_updates_only? || ignore_conditions_for(dependency).any? + ignore_conditions_for(dependency).any? end def ignore_conditions_for(dep) @@ -230,7 +207,7 @@ def update_config_ignored_versions(ignore_conditions, dep) end Dependabot::Config::UpdateConfig. new(ignore_conditions: ignore_conditions). - ignored_versions_for(dep, security_updates_only: job.security_updates_only?) + ignored_versions_for(dep, security_updates_only: false) end def update_checker_for(dependency, raise_on_ignored:) @@ -296,7 +273,6 @@ def log_ignore_conditions(dep) ic["update-types"]&.each do |update_type| msg = " #{update_type} - from #{ic['source']}" - msg += " (doesn't apply to security update)" if job.security_updates_only? Dependabot.logger.info(msg) end end @@ -307,10 +283,6 @@ def all_versions_ignored?(dependency, checker) false rescue Dependabot::AllVersionsIgnored Dependabot.logger.info("All updates for #{dependency.name} were ignored") - - # Report this error to the backend to create an update job error - raise if job.security_updates_only? - true end From 34fca4d8f01f9bc86c73498ecb50d226554b2306 Mon Sep 17 00:00:00 2001 From: Barry Gordon Date: Wed, 29 Mar 2023 21:35:04 +0100 Subject: [PATCH 4/8] Remove a redundant method --- .../operations/refresh_version_pull_request.rb | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb b/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb index 6257b2eea31..44a4398560b 100644 --- a/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb +++ b/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb @@ -29,7 +29,10 @@ def initialize(service:, job:, dependency_snapshot:, error_handler:) def perform Dependabot.logger.info("Starting PR update job for #{job.source.repo}") - check_and_update_existing_pr_with_error_handling(dependencies) + dependency = dependencies.last + check_and_update_pull_request(dependencies) + rescue StandardError => e + error_handler.handle_dependabot_error(error: e, dependency: dependency) end private @@ -73,13 +76,6 @@ def dependencies allowed_deps end - def check_and_update_existing_pr_with_error_handling(dependencies) - dependency = dependencies.last - check_and_update_pull_request(dependencies) - rescue StandardError => e - error_handler.handle_dependabot_error(error: e, dependency: dependency) - end - # rubocop:disable Metrics/AbcSize # rubocop:disable Metrics/CyclomaticComplexity # rubocop:disable Metrics/PerceivedComplexity From 29d6cc33bb3d25ec07d8e09e60079dcf14ebcf80 Mon Sep 17 00:00:00 2001 From: Barry Gordon Date: Wed, 29 Mar 2023 21:36:27 +0100 Subject: [PATCH 5/8] Remove some linter warnings that no longer apply --- .../updater/operations/refresh_version_pull_request.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb b/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb index 44a4398560b..810de535271 100644 --- a/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb +++ b/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb @@ -77,9 +77,7 @@ def dependencies end # rubocop:disable Metrics/AbcSize - # rubocop:disable Metrics/CyclomaticComplexity # rubocop:disable Metrics/PerceivedComplexity - # rubocop:disable Metrics/MethodLength # # TODO: Push checks on dependencies into Dependabot::DependencyChange # @@ -148,9 +146,7 @@ def check_and_update_pull_request(dependencies) end end # rubocop:enable Metrics/AbcSize - # rubocop:enable Metrics/CyclomaticComplexity # rubocop:enable Metrics/PerceivedComplexity - # rubocop:enable Metrics/MethodLength def create_pull_request(dependencies, updated_dependency_files) Dependabot.logger.info("Submitting #{dependencies.map(&:name).join(', ')} " \ From 41d7506b4679d8d2de187468667a0012c430567a Mon Sep 17 00:00:00 2001 From: Barry Gordon Date: Thu, 30 Mar 2023 11:44:45 +0100 Subject: [PATCH 6/8] Add a missed method --- .../updater/operations/refresh_version_pull_request.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb b/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb index 810de535271..fdc33943cc9 100644 --- a/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb +++ b/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb @@ -278,6 +278,13 @@ def all_versions_ignored?(dependency, checker) true end + def name_match?(name1, name2) + WildcardMatcher.match?( + job.name_normaliser.call(name1), + job.name_normaliser.call(name2) + ) + end + def requirements_to_unlock(checker) if job.lockfile_only? || !checker.requirements_unlocked_or_can_be? if checker.can_update?(requirements_to_unlock: :none) then :none From a7d99bdb34d516b0f02ea10d2318db4cff5a30dc Mon Sep 17 00:00:00 2001 From: Barry Gordon Date: Thu, 30 Mar 2023 17:14:14 +0100 Subject: [PATCH 7/8] Prefer RefreshVersionUpdatePullRequest --- updater/lib/dependabot/updater/operations.rb | 6 +++--- ...ll_request.rb => refresh_version_update_pull_request.rb} | 2 +- updater/spec/dependabot/updater/operations_spec.rb | 5 +++-- updater/spec/dependabot/updater_spec.rb | 4 ++-- 4 files changed, 9 insertions(+), 8 deletions(-) rename updater/lib/dependabot/updater/operations/{refresh_version_pull_request.rb => refresh_version_update_pull_request.rb} (99%) diff --git a/updater/lib/dependabot/updater/operations.rb b/updater/lib/dependabot/updater/operations.rb index c7b175c83ad..e9e104170d0 100644 --- a/updater/lib/dependabot/updater/operations.rb +++ b/updater/lib/dependabot/updater/operations.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "dependabot/updater/operations/group_update_all_versions" -require "dependabot/updater/operations/refresh_version_pull_request" +require "dependabot/updater/operations/refresh_version_update_pull_request" require "dependabot/updater/operations/update_all_versions" # This module is responsible for determining which Operation a Job is requesting @@ -27,8 +27,8 @@ module Operations # specific preconditions go before those with more permissive checks. OPERATIONS = [ GroupUpdateAllVersions, - UpdateAllVersions, - RefreshVersionPullRequest + RefreshVersionUpdatePullRequest, + UpdateAllVersions ] def self.class_for(job:) diff --git a/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb b/updater/lib/dependabot/updater/operations/refresh_version_update_pull_request.rb similarity index 99% rename from updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb rename to updater/lib/dependabot/updater/operations/refresh_version_update_pull_request.rb index fdc33943cc9..5fcfef9abce 100644 --- a/updater/lib/dependabot/updater/operations/refresh_version_pull_request.rb +++ b/updater/lib/dependabot/updater/operations/refresh_version_update_pull_request.rb @@ -10,7 +10,7 @@ module Dependabot class Updater module Operations - class RefreshVersionPullRequest + class RefreshVersionUpdatePullRequest def self.applies_to?(job:) return false if job.security_updates_only? # If we haven't been given metadata about the dependencies present diff --git a/updater/spec/dependabot/updater/operations_spec.rb b/updater/spec/dependabot/updater/operations_spec.rb index b5f30d08f6d..f2eea617d98 100644 --- a/updater/spec/dependabot/updater/operations_spec.rb +++ b/updater/spec/dependabot/updater/operations_spec.rb @@ -46,14 +46,15 @@ end end - it "returns the RefreshVersionPullRequest class when the Job is for an existing dependency version update" do + it "returns the RefreshVersionUpdatePullRequest class when the Job is for an existing dependency version update" do job = instance_double(Dependabot::Job, security_updates_only?: false, updating_a_pull_request?: true, dependencies: [anything], is_a?: true) - expect(described_class.class_for(job: job)).to be(Dependabot::Updater::Operations::RefreshVersionPullRequest) + expect(described_class.class_for(job: job)). + to be(Dependabot::Updater::Operations::RefreshVersionUpdatePullRequest) end it "raises an argument error with anything other than a Dependabot::Job" do diff --git a/updater/spec/dependabot/updater_spec.rb b/updater/spec/dependabot/updater_spec.rb index 0ed2f2ad70d..6755a32519e 100644 --- a/updater/spec/dependabot/updater_spec.rb +++ b/updater/spec/dependabot/updater_spec.rb @@ -1134,7 +1134,7 @@ def expect_update_checker_with_ignored_versions(versions) service = build_service updater = build_updater(service: service, job: job) - expect(Dependabot::Updater::Operations::RefreshVersionPullRequest).to receive(:new).and_call_original + expect(Dependabot::Updater::Operations::RefreshVersionUpdatePullRequest).to receive(:new).and_call_original expect(service).to receive(:create_pull_request).once updater.run @@ -1241,7 +1241,7 @@ def expect_update_checker_with_ignored_versions(versions) service = build_service updater = build_updater(service: service, job: job) - expect(Dependabot::Updater::Operations::RefreshVersionPullRequest).to receive(:new).and_call_original + expect(Dependabot::Updater::Operations::RefreshVersionUpdatePullRequest).to receive(:new).and_call_original expect(service).to receive(:create_pull_request).once expect(service).not_to receive(:close_pull_request) From 24a7a9cd3818069dfef2cf829465de9bc239a7d8 Mon Sep 17 00:00:00 2001 From: Barry Gordon Date: Fri, 31 Mar 2023 11:52:08 +0100 Subject: [PATCH 8/8] Slim down the dependencies method to only code used --- updater/lib/dependabot/updater.rb | 5 --- .../refresh_version_update_pull_request.rb | 45 +++++-------------- 2 files changed, 11 insertions(+), 39 deletions(-) diff --git a/updater/lib/dependabot/updater.rb b/updater/lib/dependabot/updater.rb index c830c54b168..a4a7e5e9973 100644 --- a/updater/lib/dependabot/updater.rb +++ b/updater/lib/dependabot/updater.rb @@ -126,11 +126,6 @@ def check_and_update_existing_pr_with_error_handling(dependencies) # rubocop:disable Metrics/CyclomaticComplexity # rubocop:disable Metrics/PerceivedComplexity # rubocop:disable Metrics/MethodLength - # - # TODO: Push checks on dependencies into Dependabot::DependencyChange - # - # Some of this logic would make more sense as interrogations of the - # DependencyChange as we build it up step-by-step. def check_and_update_pull_request(dependencies) if dependencies.count != job.dependencies.count # If the job dependencies mismatch the parsed dependencies, then diff --git a/updater/lib/dependabot/updater/operations/refresh_version_update_pull_request.rb b/updater/lib/dependabot/updater/operations/refresh_version_update_pull_request.rb index 5fcfef9abce..87505479c9a 100644 --- a/updater/lib/dependabot/updater/operations/refresh_version_update_pull_request.rb +++ b/updater/lib/dependabot/updater/operations/refresh_version_update_pull_request.rb @@ -44,45 +44,22 @@ def perform :created_pull_requests def dependencies - all_deps = dependency_snapshot.dependencies - - # Rebases and security updates have dependencies, version updates don't - if job.dependencies - # Gradle, Maven and Nuget dependency names can be case-insensitive and - # the dependency name in the security advisory often doesn't match what - # users have specified in their manifest. - # - # It's technically possibly to publish case-sensitive npm packages to a - # private registry but shouldn't cause problems here as job.dependencies - # is set either from an existing PR rebase/recreate or a security - # advisory. - job_dependencies = job.dependencies.map(&:downcase) - return all_deps.select do |dep| - job_dependencies.include?(dep.name.downcase) - end - end - - allowed_deps = all_deps.select { |d| job.allowed_update?(d) } - # Return dependencies in a random order, with top-level dependencies - # considered first so that dependency runs which time out don't always hit - # the same dependencies - allowed_deps = allowed_deps.shuffle unless ENV["UPDATER_DETERMINISTIC"] - - if all_deps.any? && allowed_deps.none? - Dependabot.logger.info("Found no dependencies to update after filtering allowed " \ - "updates") + # Gradle, Maven and Nuget dependency names can be case-insensitive and + # the dependency name in the security advisory often doesn't match what + # users have specified in their manifest. + # + # It's technically possibly to publish case-sensitive npm packages to a + # private registry but shouldn't cause problems here as job.dependencies + # is set either from an existing PR rebase/recreate or a security + # advisory. + job_dependencies = job.dependencies.map(&:downcase) + dependency_snapshot.dependencies.select do |dep| + job_dependencies.include?(dep.name.downcase) end - - allowed_deps end # rubocop:disable Metrics/AbcSize # rubocop:disable Metrics/PerceivedComplexity - # - # TODO: Push checks on dependencies into Dependabot::DependencyChange - # - # Some of this logic would make more sense as interrogations of the - # DependencyChange as we build it up step-by-step. def check_and_update_pull_request(dependencies) if dependencies.count != job.dependencies.count # If the job dependencies mismatch the parsed dependencies, then