fix refreshing a grouped PR causes dependency duplication (dependabot…

…#8150)

updater/lib/dependabot/updater/group_update_creation.rb

@@ -305,6 +305,16 @@ def cleanup_workspace
 
         Dependabot::Workspace.cleanup!
       end
+
+      def pr_exists_for_dependency_group?(group)
+        job.existing_group_pull_requests&.any? { |pr| pr["dependency-group-name"] == group.name }
+      end
+
+      def dependencies_in_existing_pr_for_group(group)
+        job.existing_group_pull_requests.find do |pr|
+          pr["dependency-group-name"] == group.name
+        end.fetch("dependencies", [])
+      end
     end
   end
 end

updater/lib/dependabot/updater/operations/group_update_all_versions.rb

@@ -18,6 +18,8 @@ module Dependabot
   class Updater
     module Operations
       class GroupUpdateAllVersions
+        include GroupUpdateCreation
+
         def self.applies_to?(job:)
           return false if job.security_updates_only?
           return false if job.updating_a_pull_request?
@@ -73,7 +75,7 @@ def run_grouped_dependency_updates # rubocop:disable Metrics/AbcSize
           Dependabot.logger.info("Found #{dependency_snapshot.groups.count} group(s).")
 
           # Preprocess to discover existing group PRs and add their dependencies to the handled list before processing
-          # the rest of the groups. This prevents multiple PRs from being created for the same group.
+          # the rest of the groups. This prevents multiple PRs from being created for the same dependency.
           groups_without_pr = dependency_snapshot.groups.filter_map do |group|
             if pr_exists_for_dependency_group?(group)
               Dependabot.logger.info("Detected existing pull request for '#{group.name}'.")
@@ -102,16 +104,6 @@ def run_grouped_dependency_updates # rubocop:disable Metrics/AbcSize
           end
         end
 
-        def pr_exists_for_dependency_group?(group)
-          job.existing_group_pull_requests&.any? { |pr| pr["dependency-group-name"] == group.name }
-        end
-
-        def dependencies_in_existing_pr_for_group(group)
-          job.existing_group_pull_requests.find do |pr|
-            pr["dependency-group-name"] == group.name
-          end.fetch("dependencies", [])
-        end
-
         def run_update_for(group)
           Dependabot::Updater::Operations::CreateGroupUpdatePullRequest.new(
             service: service,

updater/lib/dependabot/updater/operations/refresh_group_update_pull_request.rb

@@ -46,7 +46,7 @@ def initialize(service:, job:, dependency_snapshot:, error_handler:)
           @error_handler = error_handler
         end
 
-        def perform
+        def perform # rubocop:disable Metrics/AbcSize
           # This guards against any jobs being performed where the data is malformed, this should not happen unless
           # there was is defect in the service and we emitted a payload where the job and configuration data objects
           # were out of sync.
@@ -75,6 +75,16 @@ def perform
           else
             Dependabot.logger.info("Updating the '#{dependency_snapshot.job_group.name}' group")
 
+            # Preprocess to discover existing group PRs and add their dependencies to the handled list before processing
+            # the refresh. This prevents multiple PRs from being created for the same dependency during the refresh.
+            dependency_snapshot.groups.each do |group|
+              next unless group.name != dependency_snapshot.job_group.name && pr_exists_for_dependency_group?(group)
+
+              dependency_snapshot.add_handled_dependencies(
+                dependencies_in_existing_pr_for_group(group).map { |d| d["dependency-name"] }
+              )
+            end
+
             dependency_change = compile_all_dependency_changes_for(dependency_snapshot.job_group)
 
             upsert_pull_request_with_error_handling(dependency_change)
@@ -96,7 +106,7 @@ def upsert_pull_request_with_error_handling(dependency_change)
             close_pull_request(reason: :up_to_date)
           end
         rescue StandardError => e
-          error_handler.handle_job_error(error: e, group: job_group)
+          error_handler.handle_job_error(error: e, group: dependency_snapshot.job_group)
         end
 
         # Having created the dependency_change, we need to determine the right strategy to apply it to the project:

updater/spec/dependabot/updater/operations/refresh_group_update_pull_request_spec.rb

@@ -148,6 +148,8 @@
     end
   end
 
+  # This shouldn't be possible as the grouped update shouldn't put a dependency in more than one group.
+  # But it's useful to test what will happen on refresh if it does get in this state.
   context "when there is a pull request for an overlapping group" do
     let(:job_definition) do
       job_definition_fixture("bundler/version_updates/group_update_refresh_similar_pr")
@@ -161,13 +163,14 @@
       stub_rubygems_calls
     end
 
-    it "does not attempt to update the other group's pull request" do
-      expect(mock_service).to receive(:create_pull_request) do |dependency_change|
-        expect(dependency_change.dependency_group.name).to eql("everything-everywhere-all-at-once")
-        expect(dependency_change.updated_dependency_files_hash).to eql(updated_bundler_files_hash)
-      end
+    it "considers the dependencies in the other PRs as handled, and closes the duplicate PR" do
+      expect(mock_service).to receive(:close_pull_request).with(["dummy-pkg-b"], :up_to_date)
 
       group_update_all.perform
+
+      # It added all of the other existing grouped PRs to the handled list
+      expect(dependency_snapshot.handled_dependencies).to match_array(%w(dummy-pkg-a dummy-pkg-b dummy-pkg-c
+                                                                         dummy-pkg-d))
     end
   end
 

updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_refresh_similar_pr.yaml

@@ -19,6 +19,12 @@ job:
       dependencies:
         - dependency-name: dummy-pkg-b
           dependency-version: 1.2.0
+        - dependency-name: dummy-pkg-c
+          dependency-version: 1.0.0
+    - dependency-group-name: something-else
+      dependencies:
+        - dependency-name: dummy-pkg-d
+          dependency-version: 0.1.0
   updating-a-pull-request: true
   lockfile-only: false
   update-subdependencies: false
@@ -50,4 +56,8 @@ job:
       rules:
         patterns:
           - "dummy-pkg-*"
+    - name: something-else
+      rules:
+        patterns:
+          - "dummy-pkg-d"
   dependency-group-to-refresh: everything-everywhere-all-at-once