[Fix] multi-directory grouped updates sometimes create ungrouped PRs (#…

…9938)

* spike

* Scope updated_dependencies_list to the current file; fix ff checks

* explicitly set updated_dependencies to an empty array if ff is not enabled

* rubocop

* call new method

* Concat instead of +

* fix undefined method `concat' for nil

* Remove comment about adding directory to updated_dependencies_set

* still add updated_dependencies to the global list

* Add test to check the updated_dependency_files contain a list of
updated_dependencies

* lint

* Add silent ecosystem tests from fix-handled-dependencies-for-multidir

* Add #add_handled_group_dependencies method

* Update add_handled_group_dependencies to take a Hash

* Add unit tests for add_handled_grouped_dependencies from fix-handled-dependencies-for-multidir

* disable perceived complexity for run_grouped_dependency_updates

* dependency snapshot uses directory for tracking dependencies

* track directory in dependency for deduce_updated_dependencies

* add note about using dependency_snapshot.handled_dependencies

* lint

* spike

* Scope updated_dependencies_list to the current file; fix ff checks

* explicitly set updated_dependencies to an empty array if ff is not enabled

* rubocop

* call new method

* Concat instead of +

* fix undefined method `concat' for nil

* Remove comment about adding directory to updated_dependencies_set

* still add updated_dependencies to the global list

* Add test to check the updated_dependency_files contain a list of
updated_dependencies

* lint

* Change updated_dependencies to be a Symbol

Co-authored-by: Landon Grindheim <landon.grindheim@gmail.com>

* Reasign batch[file.path][:updated_dependencies] istead of zeroing it out

* Use dig instead of directly accessing Hash

* shold -> should

---------

Co-authored-by: Landon Grindheim <landon.grindheim@gmail.com>

silent/tests/testdata/vu-group-multidir-all.txt

@@ -1,7 +1,15 @@
+# Testing a grouped multi-dir configuration.
+
 dependabot update -f input.yml --local . --updater-image ghcr.io/dependabot/dependabot-updater-silent
 stderr 'created \| dependency-a \( from 1.2.3 to 1.2.5 \), dependency-b \( from 2.2.3 to 2.2.5 \)'
 pr-created foo/expected.json bar/expected.json
 
+# Testing a grouped multi-dir configuration with pre-existing PR for the group.
+# Should result in no PRs.
+
+dependabot update -f input-preexisting.yml --local . --updater-image ghcr.io/dependabot/dependabot-updater-silent
+! stdout create_pull_request
+
 -- foo/manifest.json --
 {
   "dependency-a": { "version": "1.2.3" },
@@ -76,7 +84,41 @@ job:
     api-endpoint: https://example.com/api/v3
     repo: dependabot/smoke-tests
   dependency-groups:
-    - name: first
+    - name: all
+      rules:
+        patterns:
+          - "*"
+
+-- input-preexisting.yml --
+job:
+  package-manager: "silent"
+  source:
+    directories:
+      - "/foo"
+      - "/bar"
+    provider: example
+    hostname: example.com
+    api-endpoint: https://example.com/api/v3
+    repo: dependabot/smoke-tests
+  dependency-groups:
+    - name: all
       rules:
         patterns:
           - "*"
+  existing-group-pull-requests:
+    - dependency-group-name: all
+      dependencies:
+        - dependency-name: dependency-a
+          version: 1.2.5
+          directory: "/foo"
+        - dependency-name: dependency-b
+          version: 2.2.5
+          directory: "/foo"
+        - dependency-name: dependency-b
+          version: 2.2.5
+          directory: "/bar"
+        - dependency-name: dependency-c
+          version: 3.2.5
+          directory: "/bar"
+  experiments:
+    dependency_has_directory: true

silent/tests/testdata/vu-group-multidir-semver.txt

@@ -0,0 +1,81 @@
+# Testing a grouped multi-dir configuration using semver rules.
+
+dependabot update -f input.yml --local . --updater-image ghcr.io/dependabot/dependabot-updater-silent
+stdout -count=2 create_pull_request
+pr-created foo/expected-1.json bar/expected-1.json
+pr-created foo/expected-2.json
+
+# When there is the same dependency in both directories, one is a major update the other a patch,
+# and the user asked for majors in one group and minors in the other, we should create two group PRs
+
+-- foo/manifest.json --
+{
+  "dependency-a": { "version": "1.0.0" },
+  "dependency-b": { "version": "2.0.0" }
+}
+
+-- bar/manifest.json --
+{
+  "dependency-a": { "version": "2.0.0" },
+  "dependency-b": { "version": "1.0.0" }
+}
+
+-- foo/expected-1.json --
+{
+  "dependency-a": { "version": "2.0.1" },
+  "dependency-b": { "version": "2.0.0" }
+}
+
+-- bar/expected-1.json --
+{
+  "dependency-a": { "version": "2.0.0" },
+  "dependency-b": { "version": "2.0.1" }
+}
+
+-- foo/expected-2.json --
+{
+  "dependency-a": { "version": "1.0.0" },
+  "dependency-b": { "version": "2.0.1" }
+}
+
+-- dependency-a --
+{
+  "versions": [
+    "1.0.0",
+    "2.0.0",
+    "2.0.1"
+  ]
+}
+
+-- dependency-b --
+{
+  "versions": [
+    "1.0.0",
+    "2.0.0",
+    "2.0.1"
+  ]
+}
+
+-- input.yml --
+job:
+  package-manager: "silent"
+  source:
+    directories:
+      - "/foo"
+      - "/bar"
+    provider: example
+    hostname: example.com
+    api-endpoint: https://example.com/api/v3
+    repo: dependabot/smoke-tests
+  dependency-groups:
+    - name: major
+      rules:
+        update-types:
+          - major
+    - name: minor
+      rules:
+        update-types:
+          - minor
+          - patch
+  experiments:
+    dependency_has_directory: true

updater/lib/dependabot/dependency_snapshot.rb

@@ -112,13 +112,31 @@ def add_handled_dependencies(dependency_names)
       @handled_dependencies[@current_directory] = set
     end
 
+    sig { params(dependencies: T::Array[{ name: T.nilable(String), directory: T.nilable(String) }]).void }
+    def add_handled_group_dependencies(dependencies)
+      raise "Current directory not set" if @current_directory == ""
+
+      dependencies.group_by { |d| d[:directory] }.each do |dir, dependency_hash|
+        set = @handled_dependencies[dir] || Set.new
+        set.merge(dependency_hash.map { |d| d[:name] })
+        @handled_dependencies[dir] = set
+      end
+    end
+
     sig { returns(T::Set[String]) }
     def handled_dependencies
       raise "Current directory not set" if @current_directory == ""
 
       T.must(@handled_dependencies[@current_directory])
     end
 
+    # rubocop:disable Performance/Sum
+    sig { returns(T::Set[String]) }
+    def handled_group_dependencies
+      T.must(@handled_dependencies.values.reduce(&:+))
+    end
+    # rubocop:enable Performance/Sum
+
     sig { params(dir: String).void }
     def current_directory=(dir)
       @current_directory = dir
@@ -135,6 +153,10 @@ def ungrouped_dependencies
       # If no groups are defined, all dependencies are ungrouped by default.
       return allowed_dependencies unless groups.any?
 
+      if Dependabot::Experiments.enabled?(:dependency_has_directory)
+        return allowed_dependencies.reject { |dep| handled_group_dependencies.include?(dep.name) }
+      end
+
       # Otherwise return dependencies that haven't been handled during the group update portion.
       allowed_dependencies.reject { |dep| T.must(@handled_dependencies[@current_directory]).include?(dep.name) }
     end
@@ -156,7 +178,13 @@ def initialize(job:, base_commit_sha:, dependency_files:) # rubocop:disable Metr
       @dependencies = T.let({}, T::Hash[String, T::Array[Dependabot::Dependency]])
       directories.each do |dir|
         @current_directory = dir
-        @dependencies[dir] = parse_files!
+        if Dependabot::Experiments.enabled?(:dependency_has_directory)
+          dependencies = parse_files!
+          dependencies_with_dir = dependencies.each { |dep| dep.directory = dir }
+          @dependencies[dir] = dependencies_with_dir
+        else
+          @dependencies[dir] = parse_files!
+        end
       end
 
       @dependency_group_engine = T.let(DependencyGroupEngine.from_job_config(job: job),

updater/lib/dependabot/updater/dependency_group_change_batch.rb

@@ -127,7 +127,7 @@ def merge_file_and_dependency_changes_to_batch(file, batch, updated_dependencies
                          0
                        end
 
-        previous_updated_dependencies = batch[file.path][:updated_dependencies] || []
+        previous_updated_dependencies = batch.dig(file.path, :updated_dependencies) || []
         updated_dependencies_list = previous_updated_dependencies.concat(updated_dependencies)
 
         batch[file.path] =

updater/lib/dependabot/updater/group_update_creation.rb

@@ -52,7 +52,11 @@ def compile_all_dependency_changes_for(group)
         )
         original_dependencies = dependency_snapshot.dependencies
 
+        Dependabot.logger.info("Updating the #{job.source.directory} directory.")
         group.dependencies.each do |dependency|
+          # We check dependency_snapshot.handled_dependencies instead of handled_group_dependencies here
+          # because we still want to update a dependency if it's been updated in another manifest files,
+          # but we should skip it if it's been updated in _the same_ manifest file
           if dependency_snapshot.handled_dependencies.include?(dependency.name)
             Dependabot.logger.info(
               "Skipping #{dependency.name} as it has already been handled by a previous group"
@@ -185,6 +189,7 @@ def create_change_for(lead_dependency, updated_dependencies, dependency_files, d
       #
       # This method **must** must return an Array when it errors
       #
+      # rubocop:disable Metrics/AbcSize, Metrics/PerceivedComplexity
       sig do
         params(
           dependency: Dependabot::Dependency,
@@ -208,7 +213,12 @@ def compile_updates_for(dependency, dependency_files, group) # rubocop:disable M
 
         # Consider the dependency handled so no individual PR is raised since it is in this group.
         # Even if update is not possible, etc.
-        dependency_snapshot.add_handled_dependencies(dependency.name)
+        if Dependabot::Experiments.enabled?(:dependency_has_directory)
+          dependency_snapshot.add_handled_group_dependencies([{ name: dependency.name,
+                                                                directory: job.source.directory }])
+        else
+          dependency_snapshot.add_handled_dependencies(dependency.name)
+        end
 
         if checker.up_to_date?
           log_up_to_date(dependency)
@@ -229,7 +239,12 @@ def compile_updates_for(dependency, dependency_files, group) # rubocop:disable M
           requirements_to_unlock: requirements_to_unlock
         )
       rescue Dependabot::InconsistentRegistryResponse => e
-        dependency_snapshot.add_handled_dependencies(dependency.name)
+        if Dependabot::Experiments.enabled?(:dependency_has_directory)
+          dependency_snapshot.add_handled_group_dependencies([{ name: dependency.name,
+                                                                directory: job.source.directory }])
+        else
+          dependency_snapshot.add_handled_dependencies(dependency.name)
+        end
         error_handler.log_dependency_error(
           dependency: dependency,
           error: e,
@@ -240,10 +255,16 @@ def compile_updates_for(dependency, dependency_files, group) # rubocop:disable M
       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.
-        dependency_snapshot.add_handled_dependencies(dependency.name)
+        if Dependabot::Experiments.enabled?(:dependency_has_directory)
+          dependency_snapshot.add_handled_group_dependencies([{ name: dependency.name,
+                                                                directory: job.source.directory }])
+        else
+          dependency_snapshot.add_handled_dependencies(dependency.name)
+        end
         error_handler.handle_dependency_error(error: e, dependency: dependency, dependency_group: group)
         [] # return an empty set
       end
+      # rubocop:enable Metrics/AbcSize, Metrics/PerceivedComplexity
 
       sig { params(dependency: Dependabot::Dependency).void }
       def log_up_to_date(dependency)
@@ -446,14 +467,20 @@ def deduce_updated_dependency(dependency, original_dependency)
         )
         dependency_snapshot.handled_dependencies << dependency.name
 
-        Dependabot::Dependency.new(
+        dependency_params = {
           name: dependency.name,
           version: dependency.version,
           previous_version: original_dependency.version,
           requirements: dependency.requirements,
           previous_requirements: original_dependency.requirements,
           package_manager: dependency.package_manager
-        )
+        }
+
+        if Dependabot::Experiments.enabled?(:dependency_has_directory)
+          dependency_params[:directory] = dependency.directory
+        end
+
+        Dependabot::Dependency.new(**dependency_params)
       end
     end
   end

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

@@ -99,8 +99,9 @@ def perform
         sig { returns(Dependabot::Updater::ErrorHandler) }
         attr_reader :error_handler
 
+        # rubocop:disable Metrics/AbcSize, Metrics/PerceivedComplexity
         sig { returns(T::Array[Dependabot::DependencyGroup]) }
-        def run_grouped_dependency_updates # rubocop:disable Metrics/AbcSize
+        def run_grouped_dependency_updates
           Dependabot.logger.info("Starting grouped update job for #{job.source.repo}")
           Dependabot.logger.info("Found #{dependency_snapshot.groups.count} group(s).")
 
@@ -112,13 +113,32 @@ def run_grouped_dependency_updates # rubocop:disable Metrics/AbcSize
               Dependabot.logger.info(
                 "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
-              dependency_snapshot.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
-              dependency_snapshot.add_handled_dependencies(group.dependencies.map(&:name))
+
+              if Dependabot::Experiments.enabled?(:dependency_has_directory)
+
+                # A grouped version update gets its directories from user-defined update configs.
+                # A multi-directory grouped update will iterate each group over every directory.
+                # Therefore, we can skip a grouped dependency if it's been updated in *any* directory
+                # add the dependencies in the group so individual updates don't try to update them
+                dependency_snapshot.add_handled_group_dependencies(
+                  dependencies_in_existing_pr_for_group(group)
+                   .map { |d| { name: d["dependency-name"], directory: d["directory"] } }
+                )
+                # 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
+                dependency_snapshot.add_handled_group_dependencies(
+                  group.dependencies.map { |d| { name: d.name, directory: d.directory } }
+                )
+              else
+                # add the dependencies in the group so individual updates don't try to update them
+                dependency_snapshot.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
+                dependency_snapshot.add_handled_dependencies(group.dependencies.map(&:name))
+              end
+
               next
             end
 
@@ -136,6 +156,7 @@ def run_grouped_dependency_updates # rubocop:disable Metrics/AbcSize
             end
           end
         end
+        # rubocop:enable Metrics/AbcSize, Metrics/PerceivedComplexity
 
         sig { params(group: Dependabot::DependencyGroup).returns(T.nilable(Dependabot::DependencyChange)) }
         def run_grouped_update_for(group)