From 1e1702652859e893b1e7fd1302518a9eb59fb3c6 Mon Sep 17 00:00:00 2001 From: Jake Coffman Date: Wed, 23 Aug 2023 11:21:22 -0500 Subject: [PATCH] fix edge cases during semver grouping creating single PRs erroneously (#7867) --- common/lib/dependabot/dependency_group.rb | 11 ++++++++- updater/lib/dependabot/dependency_snapshot.rb | 9 +++----- .../updater/group_update_creation.rb | 10 +++++++- .../operations/group_update_all_versions.rb | 9 +++----- .../dependabot/dependency_snapshot_spec.rb | 4 +++- .../group_update_all_versions_spec.rb | 23 +++++++++++++++++++ 6 files changed, 51 insertions(+), 15 deletions(-) diff --git a/common/lib/dependabot/dependency_group.rb b/common/lib/dependabot/dependency_group.rb index 4fbb549cc1..4c81879228 100644 --- a/common/lib/dependabot/dependency_group.rb +++ b/common/lib/dependabot/dependency_group.rb @@ -9,12 +9,21 @@ module Dependabot class DependencyGroup - attr_reader :name, :rules, :dependencies + attr_reader :name, :rules, :dependencies, :handled_dependencies def initialize(name:, rules:) @name = name @rules = rules @dependencies = [] + @handled_dependencies = Set.new + end + + def add_to_handled(*dependencies) + @handled_dependencies += dependencies.map(&:name) + end + + def add_all_to_handled + @handled_dependencies += dependencies.map(&:name) end def contains?(dependency) diff --git a/updater/lib/dependabot/dependency_snapshot.rb b/updater/lib/dependabot/dependency_snapshot.rb index 4c6a37e8d4..2331f65a97 100644 --- a/updater/lib/dependabot/dependency_snapshot.rb +++ b/updater/lib/dependabot/dependency_snapshot.rb @@ -69,16 +69,13 @@ def groups @dependency_group_engine.dependency_groups end - def calculate_ungrouped_dependencies(dependencies_handled) - @ungrouped_dependencies = allowed_dependencies.reject { |dep| dependencies_handled.include?(dep.name) } - end - def ungrouped_dependencies # If no groups are defined, all dependencies are ungrouped by default. return allowed_dependencies unless groups.any? - return @ungrouped_dependencies if defined?(@ungrouped_dependencies) - @dependency_group_engine.ungrouped_dependencies + # Otherwise return dependencies that haven't been handled during the group update portion. + all_handled_dependencies = Set.new(groups.map { |g| g.handled_dependencies.to_a }.flatten) + allowed_dependencies.reject { |dep| all_handled_dependencies.include?(dep.name) } end private diff --git a/updater/lib/dependabot/updater/group_update_creation.rb b/updater/lib/dependabot/updater/group_update_creation.rb index b1c4a994f2..ff6666929e 100644 --- a/updater/lib/dependabot/updater/group_update_creation.rb +++ b/updater/lib/dependabot/updater/group_update_creation.rb @@ -113,7 +113,7 @@ def create_change_for(lead_dependency, updated_dependencies, dependency_files, d # # This method **must** must return an Array when it errors # - def compile_updates_for(dependency, dependency_files, group) + def compile_updates_for(dependency, dependency_files, group) # rubocop:disable Metrics/MethodLength checker = update_checker_for( dependency, dependency_files, @@ -126,6 +126,10 @@ def compile_updates_for(dependency, dependency_files, group) return [] if all_versions_ignored?(dependency, checker) return [] unless semver_rules_allow_grouping?(group, dependency, checker) + # Consider the dependency handled so no individual PR is raised since it is in this group. + # Even if update is not possible, etc. + group.add_to_handled(dependency) + if checker.up_to_date? log_up_to_date(dependency) return [] @@ -145,6 +149,7 @@ def compile_updates_for(dependency, dependency_files, group) requirements_to_unlock: requirements_to_unlock ) rescue Dependabot::InconsistentRegistryResponse => e + group.add_to_handled(dependency) error_handler.log_dependency_error( dependency: dependency, error: e, @@ -153,6 +158,9 @@ def compile_updates_for(dependency, dependency_files, group) ) [] # return an empty set rescue StandardError => e + # If there was an error we might not be able to determine if the dependency is in this + # group due to semver grouping, so we consider it handled to avoid raising an individual PR. + group.add_to_handled(dependency) error_handler.handle_dependency_error(error: e, dependency: dependency, dependency_group: group) [] # return an empty set end diff --git a/updater/lib/dependabot/updater/operations/group_update_all_versions.rb b/updater/lib/dependabot/updater/operations/group_update_all_versions.rb index 00fd542331..fdb0d34475 100644 --- a/updater/lib/dependabot/updater/operations/group_update_all_versions.rb +++ b/updater/lib/dependabot/updater/operations/group_update_all_versions.rb @@ -74,7 +74,7 @@ def run_grouped_dependency_updates dependency_snapshot.groups.each do |group| # If this group does not use update-types, then consider all dependencies as grouped. # This will prevent any failures from creating individual PRs erroneously. - @dependencies_handled += group.dependencies.map(&:name) unless group.rules&.key?("update-types") + group.add_all_to_handled unless group.rules&.key?("update-types") if pr_exists_for_dependency_group?(group) Dependabot.logger.info("Detected existing pull request for '#{group.name}'.") @@ -82,15 +82,13 @@ def run_grouped_dependency_updates "Deferring creation of a new pull request. The existing pull request will update in a separate job." ) # add the dependencies in the group so individual updates don't try to update them - @dependencies_handled += group.dependencies.map(&:name) + group.add_all_to_handled next end result = run_update_for(group) - @dependencies_handled += result.updated_dependencies.map(&:name) if result + group.add_to_handled(*result.updated_dependencies) if result end - - @dependencies_handled end def pr_exists_for_dependency_group?(group) @@ -108,7 +106,6 @@ def run_update_for(group) end def run_ungrouped_dependency_updates - dependency_snapshot.calculate_ungrouped_dependencies(@dependencies_handled) return if dependency_snapshot.ungrouped_dependencies.empty? Dependabot::Updater::Operations::UpdateAllVersions.new( diff --git a/updater/spec/dependabot/dependency_snapshot_spec.rb b/updater/spec/dependabot/dependency_snapshot_spec.rb index 4f20c5c4a8..366b71a4d9 100644 --- a/updater/spec/dependabot/dependency_snapshot_spec.rb +++ b/updater/spec/dependabot/dependency_snapshot_spec.rb @@ -117,7 +117,9 @@ expect(group.dependencies.length).to eql(1) expect(group.dependencies.first.name).to eql("dummy-pkg-a") - expect(snapshot.ungrouped_dependencies.length).to eql(1) + expect(snapshot.ungrouped_dependencies.length).to eql(2) + + group.add_to_handled(group.dependencies.find { |d| d.name == "dummy-pkg-a" }) expect(snapshot.ungrouped_dependencies.first.name).to eql("dummy-pkg-b") Dependabot::Experiments.reset! diff --git a/updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb b/updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb index 5204863fc2..ace0c82ab0 100644 --- a/updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb +++ b/updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb @@ -326,6 +326,29 @@ end end + context "when there are semver rules but an error occurs gathering versions" do + before do + allow_any_instance_of(Dependabot::Bundler::UpdateChecker). + to receive(:latest_version). + and_raise(StandardError.new("Test error while getting latest version")) + end + + let(:job_definition) do + job_definition_fixture("bundler/version_updates/group_update_all_semver_grouping") + end + + let(:dependency_files) do + original_bundler_files(fixture: "bundler_grouped_by_types") + end + + it "does not create individual PRs" do + expect(mock_service).not_to receive(:create_pull_request) + expect(mock_error_handler).to receive(:handle_dependency_error).exactly(3).times + + group_update_all.perform + end + end + context "when the snapshot is only grouping patch-level changes and major changes are ignored", :vcr do let(:job_definition) do job_definition_fixture("bundler/version_updates/group_update_all_semver_grouping_with_global_ignores")