Merge pull request #7414 from dependabot/brrygrdn/cleaner-management-…

…of-updated-dependencies

[Grouped Updates] Cleaner management of the update dependency list

updater/lib/dependabot/updater/dependency_group_change_batch.rb

@@ -0,0 +1,93 @@
+# frozen_string_literal: true
+
+# This class is responsible for aggregating individual DependencyChange objects
+# by tracking changes to individual files and the overall dependency list.
+module Dependabot
+  class Updater
+    class DependencyGroupChangeBatch
+      attr_reader :updated_dependencies
+
+      def initialize(initial_dependency_files:)
+        @updated_dependencies = []
+
+        @dependency_file_batch = initial_dependency_files.each_with_object({}) do |file, hsh|
+          hsh[file.path] = { file: file, changed: false, changes: 0 }
+        end
+
+        Dependabot.logger.debug("Starting with '#{@dependency_file_batch.count}' dependency files:")
+        debug_current_file_state
+      end
+
+      # Returns an array of DependencyFile objects for the current state
+      def current_dependency_files
+        @dependency_file_batch.map do |_path, data|
+          data[:file]
+        end
+      end
+
+      # Returns an array of DependencyFile objects that have changed at least once
+      def updated_dependency_files
+        @dependency_file_batch.filter_map do |_path, data|
+          data[:file] if data[:changed]
+        end
+      end
+
+      def merge(dependency_change)
+        merge_dependency_changes(dependency_change.updated_dependencies)
+        merge_file_changes(dependency_change.updated_dependency_files)
+
+        Dependabot.logger.debug("Dependencies updated:")
+        debug_updated_dependencies
+
+        Dependabot.logger.debug("Dependency files updated:")
+        debug_current_file_state
+      end
+
+      private
+
+      # We should retain a list of all dependencies that we change, in future we may need to account for the folder
+      # in which these changes are made to permit-cross folder updates of the same dependency.
+      #
+      # This list may contain duplicates if we make iterative updates to a Dependency within a single group, but
+      # rather than re-write the Dependency objects to account for the changes from the lowest previous version
+      # to the final version, we should defer it to the Dependabot::PullRequestCreator::MessageBuilder as a
+      # presentation concern.
+      def merge_dependency_changes(updated_dependencies)
+        @updated_dependencies.concat(updated_dependencies)
+      end
+
+      def merge_file_changes(updated_dependency_files)
+        updated_dependency_files.each do |updated_file|
+          existing_file = @dependency_file_batch[updated_file.path]
+
+          change_count = if existing_file
+                           existing_file.fetch(:change_count, 0)
+                         else
+                           Dependabot.logger.debug("New file added: '#{updated_file.path}'")
+                           0
+                         end
+
+          @dependency_file_batch[updated_file.path] = { file: updated_file, changed: true, changes: change_count + 1 }
+        end
+      end
+
+      def debug_updated_dependencies
+        return unless Dependabot.logger.debug?
+
+        @updated_dependencies.each do |dependency|
+          version_change = "#{dependency.humanized_previous_version} to #{dependency.humanized_version}"
+          Dependabot.logger.debug(" - #{dependency.name} ( #{version_change} )")
+        end
+      end
+
+      def debug_current_file_state
+        return unless Dependabot.logger.debug?
+
+        @dependency_file_batch.each do |path, data|
+          changed_string = data[:changed] ? "( Changed #{data[:changes]} times )" : ""
+          Dependabot.logger.debug("  - #{path} #{changed_string}")
+        end
+      end
+    end
+  end
+end

updater/lib/dependabot/updater/group_update_creation.rb

@@ -1,16 +1,15 @@
 # frozen_string_literal: true
 
 require "dependabot/dependency_change_builder"
-require "dependabot/updater/group_dependency_file_batch"
+require "dependabot/updater/dependency_group_change_batch"
 
 # This module contains the methods required to build a DependencyChange for
 # a single DependencyGroup.
 #
 # When included in an Operation it expects the following to be available:
 # - job: the current Dependabot::Job object
-# - dependency_snapshot: the Dependabot::DependencySnapshot of the current
-#   repo state
-# - error_handler: a Dependabot::UpdaterErrorHandler to report
+# - dependency_snapshot: the Dependabot::DependencySnapshot of the current state
+# - error_handler: a Dependabot::UpdaterErrorHandler to report any problems to
 #
 module Dependabot
   class Updater
@@ -19,15 +18,15 @@ module GroupUpdateCreation
       # outcome of attempting to update every dependency iteratively which
       # can be used for PR creation.
       def compile_all_dependency_changes_for(group)
-        all_updated_dependencies = []
-        # TODO: Iterate to a GroupDependencyBatch?
-        #
-        # It might make sense for this class to take on responsibility for `all_updated_dependencies` as well,
-        # but I'm deferring on that for compatability with other work in progress.
-        dependency_file_batch = Dependabot::Updater::GroupDependencyFileBatch.new(dependency_snapshot.dependency_files)
+        group_changes = Dependabot::Updater::DependencyGroupChangeBatch.new(
+          initial_dependency_files: dependency_snapshot.dependency_files
+        )
 
         group.dependencies.each do |dependency|
-          dependency_files = dependency_file_batch.dependency_files
+          # Get the current state of the dependency files for use in this iteration
+          dependency_files = group_changes.current_dependency_files
+
+          # Reparse the current files
           reparsed_dependencies = dependency_file_parser(dependency_files).parse
           dependency = reparsed_dependencies.find { |d| d.name == dependency.name }
 
@@ -49,28 +48,16 @@ def compile_all_dependency_changes_for(group)
           # could not create a change for any reason
           next unless dependency_change
 
-          # FIXME: all_updated_dependencies may need to be de-duped
-          #
-          # To start out with, using a variant on the 'existing_pull_request'
-          # logic might make sense -or- we could employ a one-and-done rule
-          # where the first update to a dependency blocks subsequent changes.
-          #
-          # In a follow-up iteration, a 'shared workspace' could provide the
-          # filtering for us assuming we iteratively make file changes for
-          # each Array of dependencies in the batch and the FileUpdater tells
-          # us which cannot be applied.
-          all_updated_dependencies.concat(dependency_change.updated_dependencies)
-
           # Store the updated files for the next loop
-          dependency_file_batch.merge(dependency_change.updated_dependency_files)
+          group_changes.merge(dependency_change)
         end
 
         # Create a single Dependabot::DependencyChange that aggregates everything we've updated
         # into a single object we can pass to PR creation.
         Dependabot::DependencyChange.new(
           job: job,
-          updated_dependencies: all_updated_dependencies,
-          updated_dependency_files: dependency_file_batch.updated_files,
+          updated_dependencies: group_changes.updated_dependencies,
+          updated_dependency_files: group_changes.updated_dependency_files,
           dependency_group: group
         )
       end