Do not close a PR when it supersedes other groups (#11382)

* Do not close a PR when a dependency is superseded
* Move changes behind feature flag, `allow_refresh_for_existing_pr_dependencies`
---------

Co-authored-by: Kamil Bukum <[email protected]>

updater/lib/dependabot/dependency_snapshot.rb

@@ -119,25 +119,55 @@ def job_group
       @dependency_group_engine.find_group(name: T.must(job.dependency_group_to_refresh))
     end
 
-    sig { params(group: Dependabot::DependencyGroup).void }
-    def mark_group_handled(group)
+    # rubocop:disable Metrics/PerceivedComplexity
+    sig do
+      params(
+        group: Dependabot::DependencyGroup,
+        excluding_dependencies: T::Hash[String, T::Set[String]]
+      )
+        .void
+    end
+    def mark_group_handled(group, excluding_dependencies = {})
+      Dependabot.logger.info("Marking group '#{group.name}' as handled.")
+
       directories.each do |directory|
         @current_directory = directory
 
-        # add the existing dependencies in the group so individual updates don't try to update them
-        add_handled_dependencies(dependencies_in_existing_pr_for_group(group).filter_map { |d| d["dependency-name"] })
-        # also add dependencies that might be in the group, as a rebase would add them;
-        # this avoids individual PR creation that immediately is superseded by a group PR supersede
-        add_handled_dependencies(group.dependencies.map(&:name))
+        if Dependabot::Experiments.enabled?(:allow_refresh_for_existing_pr_dependencies)
+          # add the existing dependencies in the group so individual updates don't try to update them
+          dependencies_in_existing_prs = dependencies_in_existing_pr_for_group(group)
+
+          dependencies_in_existing_prs = dependencies_in_existing_prs.filter do |dep|
+            !dep["directory"] || dep["directory"] == directory
+          end
+
+          # also add dependencies that might be in the group, as a rebase would add them;
+          # this avoids individual PR creation that immediately is superseded by a group PR supersede
+          current_dependencies = group.dependencies.map(&:name).reject do |dep|
+            excluding_dependencies[directory]&.include?(dep)
+          end
+
+          add_handled_dependencies(current_dependencies.concat(dependencies_in_existing_prs.filter_map do |dep|
+            dep["dependency-name"]
+          end))
+        else
+          # add the existing dependencies in the group so individual updates don't try to update them
+          add_handled_dependencies(dependencies_in_existing_pr_for_group(group).filter_map { |d| d["dependency-name"] })
+          # also add dependencies that might be in the group, as a rebase would add them;
+          # this avoids individual PR creation that immediately is superseded by a group PR supersede
+          add_handled_dependencies(group.dependencies.map(&:name))
+        end
       end
     end
+    # rubocop:enable Metrics/PerceivedComplexity
 
     sig { params(dependency_names: T.any(String, T::Array[String])).void }
     def add_handled_dependencies(dependency_names)
       assert_current_directory_set!
-      set = @handled_dependencies[@current_directory] || Set.new
-      set += Array(dependency_names)
-      @handled_dependencies[@current_directory] = set
+      names = Array(dependency_names)
+      Dependabot.logger.info("Adding dependencies as handled: (#{names.join(', ')}).")
+      @handled_dependencies[@current_directory] ||= Set.new
+      @handled_dependencies[@current_directory]&.merge(names)
     end
 
     sig { returns(T::Set[String]) }
@@ -166,6 +196,17 @@ def ungrouped_dependencies
       allowed_dependencies.reject { |dep| handled_dependencies.include?(dep.name) }
     end
 
+    sig { params(group: Dependabot::DependencyGroup).returns(T::Array[String]) }
+    def dependencies_in_existing_pr_for_group(group)
+      existing = job.existing_group_pull_requests.find do |pr|
+        pr["dependency-group-name"] == group.name
+      end&.fetch("dependencies", []) || []
+
+      existing.filter do |dep|
+        dep["dependency-name"]
+      end
+    end
+
     private
 
     sig do
@@ -274,13 +315,6 @@ def dependency_file_parser
       parser
     end
 
-    sig { params(group: Dependabot::DependencyGroup).returns(T::Array[T::Hash[String, String]]) }
-    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
-
     sig { void }
     def assert_current_directory_set!
       if @current_directory == "" && directories.count == 1

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

@@ -72,7 +72,7 @@ def initialize(service:, job:, dependency_snapshot:, error_handler:)
         end
 
         sig { void }
-        def perform # rubocop:disable Metrics/AbcSize
+        def perform
           # 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.
@@ -100,24 +100,43 @@ def perform # rubocop:disable Metrics/AbcSize
             # so users are informed this group is no longer actionable by Dependabot.
             warn_group_is_empty(job_group)
             close_pull_request(reason: :dependency_group_empty, group: job_group)
-          else
-            Dependabot.logger.info("Updating the '#{job_group.name}' group")
+            return
+          end
 
-            # 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 != job_group.name && pr_exists_for_dependency_group?(group)
+          process_group_dependencies(job_group)
+        end
 
-              dependency_snapshot.mark_group_handled(group)
+        sig { params(job_group: Dependabot::DependencyGroup).void }
+        def process_group_dependencies(job_group)
+          Dependabot.logger.info("Updating the '#{job_group.name}' group")
+
+          existing_pr_dependencies = {}
+
+          # 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|
+            if Dependabot::Experiments.enabled?(:allow_refresh_for_existing_pr_dependencies)
+              # Gather all dependencies in existing PRs so other groups will not consider them as handled when they
+              # are not also in the PR of the group being checked, preventing erroneous PR closures
+              group_pr_deps = dependency_snapshot.dependencies_in_existing_pr_for_group(group)
+              group_pr_deps.each do |dep|
+                dep_dir = dep["directory"] || "/"
+                existing_pr_dependencies[dep_dir] ||= Set.new
+                existing_pr_dependencies[dep_dir].add(dep["dependency-name"])
+              end
             end
 
-            if dependency_change.nil?
-              Dependabot.logger.info("Nothing could update for Dependency Group: '#{job_group.name}'")
-              return
-            end
+            next unless group.name != job_group.name && pr_exists_for_dependency_group?(group)
 
-            upsert_pull_request_with_error_handling(T.must(dependency_change), job_group)
+            dependency_snapshot.mark_group_handled(group, existing_pr_dependencies)
           end
+
+          if dependency_change.nil?
+            Dependabot.logger.info("Nothing could update for Dependency Group: '#{job_group.name}'")
+            return
+          end
+
+          upsert_pull_request_with_error_handling(T.must(dependency_change), job_group)
         end
 
         private

updater/spec/dependabot/dependency_snapshot_spec.rb

@@ -111,6 +111,9 @@
     allow(Dependabot::Experiments).to receive(:enabled?)
       .with(:enable_shared_helpers_command_timeout)
       .and_return(true)
+    allow(Dependabot::Experiments).to receive(:enabled?)
+      .with(:allow_refresh_for_existing_pr_dependencies)
+      .and_return(true)
   end
 
   after do

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

@@ -268,6 +268,39 @@
         refresh_group.perform
       end
     end
+
+    context "when there is an existing PR for the same group it has a minor version in another group" do
+      let(:job_definition) do
+        job_definition_fixture("bundler/version_updates/group_update_refresh_multiple_groups_unchaged")
+      end
+
+      let(:dependency_files) do
+        original_bundler_files(fixture: "bundler_multiple_groups")
+      end
+
+      before do
+        stub_rubygems_calls
+        allow(Dependabot::Experiments).to receive(:enabled?)
+        allow(Dependabot::Experiments).to receive(:enabled?)
+          .with(:allow_refresh_for_existing_pr_dependencies)
+          .and_return(true)
+      end
+
+      after do
+        Dependabot::Experiments.reset!
+      end
+
+      it "updates the existing pull request without errors" do
+        expect(mock_service).not_to receive(:close_pull_request)
+        expect(mock_service).to receive(:update_pull_request) do |dependency_change|
+          expect(dependency_change.dependency_group.name).to eql("major")
+          expect(dependency_change.updated_dependency_files_hash)
+            .to eql(updated_bundler_files_hash(fixture: "bundler_multiple_groups"))
+        end
+
+        refresh_group.perform
+      end
+    end
   end
 
   describe "#deduce_updated_dependency" do

updater/spec/fixtures/bundler_multiple_groups/original/Gemfile

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+source "https://rubygems.org"
+
+gem "dummy-pkg-a", "~> 1.1.0"
+gem "dummy-pkg-b", "~> 1.1.0"

updater/spec/fixtures/bundler_multiple_groups/original/Gemfile.lock

@@ -0,0 +1,15 @@
+GEM
+  remote: https://rubygems.org/
+  specs:
+    dummy-pkg-a (1.1.0)
+    dummy-pkg-b (1.1.0)
+
+PLATFORMS
+  ruby
+
+DEPENDENCIES
+  dummy-pkg-a (~> 1.1.0)
+  dummy-pkg-b (~> 1.1.0)
+
+BUNDLED WITH
+   2.4.13

updater/spec/fixtures/bundler_multiple_groups/updated/Gemfile

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+source "https://rubygems.org"
+
+gem "dummy-pkg-a", "~> 2.0.0"
+gem "dummy-pkg-b", "~> 1.1.0"

updater/spec/fixtures/bundler_multiple_groups/updated/Gemfile.lock

@@ -0,0 +1,15 @@
+GEM
+  remote: https://rubygems.org/
+  specs:
+    dummy-pkg-a (2.0.0)
+    dummy-pkg-b (1.1.0)
+
+PLATFORMS
+  ruby
+
+DEPENDENCIES
+  dummy-pkg-a (~> 2.0.0)
+  dummy-pkg-b (~> 1.1.0)
+
+BUNDLED WITH
+   2.4.13

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

@@ -0,0 +1,77 @@
+job:
+  package-manager: bundler
+  source:
+    provider: github
+    repo: dependabot/dependabot-bug-version-types
+    branch:
+    api-endpoint: https://api.github.com/
+    hostname: github.com
+    directories:
+      - "/"
+  dependencies:
+    - "dummy-pkg-a"
+  existing-pull-requests: []
+  existing-group-pull-requests:
+    - dependency-group-name: major
+      dependencies:
+        - dependency-name: dummy-pkg-a
+          dependency-version: 2.0.0
+          directory: "/"
+    - dependency-group-name: minor
+      dependencies:
+        - dependency-name: dummy-pkg-b
+          dependency-version: 1.2.0
+          directory: "/"
+  updating-a-pull-request: true
+  lockfile-only: false
+  update-subdependencies: false
+  ignore-conditions: []
+  requirements-update-strategy: null
+  allowed-updates:
+    - dependency-type: direct
+      update-type: all
+  credentials-metadata:
+    - type: git_source
+      host: github.com
+  security-advisories: []
+  security-updates-only: false
+  max-updater-run-time: 2700
+  vendor-dependencies: false
+  repo-private: false
+  proxy-log-response-body-on-auth-failure: true
+  reject-external-code: false
+  commit-message-options:
+    prefix:
+    prefix-development:
+    include-scope:
+  dependency-group-to-refresh: major
+  dependency-groups:
+    - name: major
+      rules:
+        patterns:
+          - "dummy-pkg-a"
+        update-types:
+          - "major"
+    - name: minor
+      rules:
+        patterns:
+          - "*"
+        update-types:
+          - "patch"
+          - "minor"
+  experiments:
+    dependency-change-validation: true
+    enable-file-parser-python-local: true
+    enable-fix-for-pnpm-no-change-error: true
+    enable-record-ecosystem-meta: true
+    enable-shared-helpers-command-timeout: true
+    lead-security-dependency: true
+    move-job-token: true
+    npm-v6-deprecation-warning: true
+    npm-v6-unsupported-error: true
+    nuget-native-analysis: true
+    nuget-use-direct-discovery: true
+    proxy-cached: true
+    python-3-8-deprecation-warning: true
+    record-ecosystem-versions: true
+    record-update-job-unknown-error: true

updater/spec/fixtures/rubygems-versions-a.json

@@ -1,4 +1,21 @@
 [
+  {
+    "authors": "Dependabot",
+    "built_at": "2017-06-08T00:00:00.000Z",
+    "created_at": "2017-06-08T11:17:36.474Z",
+    "description": "",
+    "downloads_count": 301,
+    "metadata": {},
+    "number": "2.0.0",
+    "summary": "A dummy package for testing Dependabot",
+    "platform": "ruby",
+    "rubygems_version": ">= 0",
+    "ruby_version": ">= 0",
+    "prerelease": false,
+    "licenses": ["MIT"],
+    "requirements": [],
+    "sha": "8c79aa73922d7f7b38cd96b7af6d489f7c354c6a9fa25bd72772e75d018fbd96"
+  },
   {
     "authors": "Dependabot",
     "built_at": "2017-06-08T00:00:00.000Z",
@@ -12,9 +29,7 @@
     "rubygems_version": ">= 0",
     "ruby_version": ">= 0",
     "prerelease": false,
-    "licenses": [
-      "MIT"
-    ],
+    "licenses": ["MIT"],
     "requirements": [],
     "sha": "51b99c7db0d39924d690e19282f63d1fba9cc002ef55a139d9b6a4b0469399a1"
   },
@@ -31,9 +46,7 @@
     "rubygems_version": ">= 0",
     "ruby_version": ">= 0",
     "prerelease": false,
-    "licenses": [
-      "MIT"
-    ],
+    "licenses": ["MIT"],
     "requirements": [],
     "sha": "c8725691239b43d5f4c343b64d30afae6dd25ff1a79cca4d80534804c638b113"
   },
@@ -50,9 +63,7 @@
     "rubygems_version": ">= 0",
     "ruby_version": ">= 0",
     "prerelease": false,
-    "licenses": [
-      "MIT"
-    ],
+    "licenses": ["MIT"],
     "requirements": [],
     "sha": "0147d64042d5ab109d185f54957fcfb88f1ff9158651944ff75c6c82d47ab444"
   }