Merge pull request #7268 from dependabot/brrygrdn/avoid-dropping-peer…

…-manifests

[Updater] Fix a bug in how we handle 'peer' manifests

script/ci-test-updater

@@ -13,7 +13,8 @@ docker run --env DEPENDABOT_TEST_ACCESS_TOKEN \
   --env VCR \
   --rm \
   -v "$(pwd)/updater/spec/fixtures/vcr_cassettes:/home/dependabot/dependabot-updater/spec/fixtures/vcr_cassettes" \
-  -v "$(pwd)/npm_and_yarn:/home/dependabot/npm_and_yarn" \
-  -v "$(pwd)/go_modules:/home/dependabot/go_modules" \
   -v "$(pwd)/composer:/home/dependabot/composer" \
+  -v "$(pwd)/docker:/home/dependabot/docker" \
+  -v "$(pwd)/go_modules:/home/dependabot/go_modules" \
+  -v "$(pwd)/npm_and_yarn:/home/dependabot/npm_and_yarn" \
   "ghcr.io/dependabot/dependabot-updater-bundler:latest" bundle exec rspec "$@"

updater/lib/dependabot/updater/group_dependency_file_batch.rb

@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+# This class is responsible for tracking changes to the original files as we
+# make changes
+module Dependabot
+  class Updater
+    class GroupDependencyFileBatch
+      def initialize(initial_dependency_files)
+        @dependency_file_batch = initial_dependency_files.each_with_object({}) do |file, hash|
+          hash[file.path] = { file: file, changed: false, changes: 0 }
+        end
+
+        Dependabot.logger.debug("Starting with '#{@dependency_file_batch.count}' dependency files:")
+        debug_current_state
+      end
+
+      # Returns an array of DependencyFile objects for the current state
+      def 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_files
+        @dependency_file_batch.filter_map do |_path, data|
+          data[:file] if data[:changed]
+        end
+      end
+
+      # Replaces the existing files
+      def merge(updated_files)
+        updated_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
+                           # Let's warn about this in debug mode, but otherwise tolerate this.
+                           Dependabot.logger.debug("Updated an unexpected file at '#{existing_file.path}'")
+                           0
+                         end
+
+          @dependency_file_batch[updated_file.path] = { file: updated_file, changed: true, changes: change_count + 1 }
+        end
+
+        Dependabot.logger.debug("Dependency files updated:")
+        debug_current_state
+      end
+
+      def debug_current_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,6 +1,7 @@
 # frozen_string_literal: true
 
 require "dependabot/dependency_change_builder"
+require "dependabot/updater/group_dependency_file_batch"
 
 # This module contains the methods required to build a DependencyChange for
 # a single DependencyGroup.
@@ -19,9 +20,14 @@ module GroupUpdateCreation
       # can be used for PR creation.
       def compile_all_dependency_changes_for(group)
         all_updated_dependencies = []
-        updated_files = []
+        # 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.dependencies.each do |dependency|
-          dependency_files = original_files_merged_with(updated_files)
+          dependency_files = dependency_file_batch.dependency_files
           updated_dependencies = compile_updates_for(dependency, dependency_files)
 
           next unless updated_dependencies.any?
@@ -49,15 +55,15 @@ def compile_all_dependency_changes_for(group)
           all_updated_dependencies.concat(dependency_change.updated_dependencies)
 
           # Store the updated files for the next loop
-          updated_files = dependency_change.updated_dependency_files
+          dependency_file_batch.merge(dependency_change.updated_dependency_files)
         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: updated_files,
+          updated_dependency_files: dependency_file_batch.updated_files,
           dependency_group: group
         )
       end
@@ -145,17 +151,6 @@ def compile_updates_for(dependency, dependency_files) # rubocop:disable Metrics/
         [] # return an empty set
       end
 
-      # This method is responsible for superimposing a set of file changes on
-      # top of the snapshot we started with. This ensures that every update
-      # has the full file list, not just those which have been modified so far
-      def original_files_merged_with(updated_files)
-        return dependency_snapshot.dependency_files if updated_files.empty?
-
-        dependency_snapshot.dependency_files.map do |original_file|
-          original_file = updated_files.find { |f| f.path == original_file.path } || original_file
-        end
-      end
-
       def log_up_to_date(dependency)
         Dependabot.logger.info(
           "No update needed for #{dependency.name} #{dependency.version}"

updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb

@@ -234,4 +234,44 @@
       group_update_all.perform
     end
   end
+
+  context "when the snapshot is updating several peer manifests", :vcr do
+    let(:job_definition) do
+      job_definition_fixture("docker/version_updates/group_update_peer_manifests")
+    end
+
+    let(:dependency_files) do
+      [
+        Dependabot::DependencyFile.new(
+          name: "Dockerfile.bundler",
+          content: fixture("docker/original/Dockerfile.bundler"),
+          directory: "/docker"
+        ),
+        Dependabot::DependencyFile.new(
+          name: "Dockerfile.cargo",
+          content: fixture("docker/original/Dockerfile.cargo"),
+          directory: "/docker"
+        )
+      ]
+    end
+
+    it "creates a DependencyChange for both of the manifests without reporting errors" do
+      expect(mock_error_handler).not_to receive(:handle_dependabot_error)
+      expect(mock_service).to receive(:create_pull_request) do |dependency_change|
+        expect(dependency_change.dependency_group.name).to eql("dependabot-core-images")
+
+        # We updated the right depednencies
+        expect(dependency_change.updated_dependencies.length).to eql(2)
+        expect(dependency_change.updated_dependencies.map(&:name)).
+          to eql(%w(dependabot/dependabot-updater-bundler dependabot/dependabot-updater-cargo))
+
+        # We updated the right files.
+        expect(dependency_change.updated_dependency_files_hash.length).to eql(2)
+        expect(dependency_change.updated_dependency_files.map(&:name)).
+          to eql(%w(Dockerfile.bundler Dockerfile.cargo))
+      end
+
+      group_update_all.perform
+    end
+  end
 end

updater/spec/fixtures/docker/original/Dockerfile.bundler

@@ -0,0 +1 @@
+FROM ghcr.io/dependabot/dependabot-updater-bundler:v2.0.20230317134336@sha256:f1a76307b9a43d4d25289852e2d0ee73f1bf2bbef1a935d9255c2e56f9db64fc

updater/spec/fixtures/docker/original/Dockerfile.cargo

@@ -0,0 +1 @@
+FROM ghcr.io/dependabot/dependabot-updater-cargo:v2.0.20230317130517@sha256:8913caf469d377ae28525b184b40f61b554d839082f594b31a36beb7344d12bc

updater/spec/fixtures/job_definitions/docker/version_updates/group_update_peer_manifests.yaml

@@ -0,0 +1,40 @@
+job:
+  package-manager: docker
+  source:
+    provider: github
+    repo: github/dependabot-action
+    directory: "/docker"
+    branch:
+    api-endpoint: https://api.github.com/
+    hostname: github.com
+    commit: 302aaa943c6507c10cbbfd1b7f0fd623c5743807
+  dependencies:
+  dependency-groups:
+  - name: dependabot-core-images
+    rules:
+      patterns:
+        - "dependabot/*"
+  existing-pull-requests: []
+  updating-a-pull-request: false
+  lockfile-only: false
+  update-subdependencies: false
+  ignore-conditions: []
+  requirements-update-strategy:
+  allowed-updates:
+  - dependency-type: direct
+    update-type: all
+  credentials-metadata:
+  - type: git_source
+    host: github.com
+  security-advisories: []
+  max-updater-run-time: 2700
+  vendor-dependencies: false
+  experiments:
+    grouped-updates-prototype: true
+  reject-external-code: false
+  commit-message-options:
+    prefix:
+    prefix-development:
+    include-scope:
+  security-updates-only: false
+