[Updater] Start splitting the Updater by extracting DependencySnapshot as an input class

updater/lib/dependabot/dependency_snapshot.rb

@@ -0,0 +1,81 @@
+# frozen_string_literal: true
+
+require "base64"
+require "dependabot/file_parsers"
+
+# This class describes the dependencies obtained from a project at a specific commit SHA
+# including both the Dependabot::DependencyFile objects at that reference as well as
+# means to parse them into a set of Dependabot::Dependency objects.
+#
+# This class is the input for a Dependabot::Updater process with Dependabot::DependencyChange
+# representing the output.
+module Dependabot
+  class DependencySnapshot
+    # TODO: Enforce non-nil values for job_definition["base64_dependency_files"]
+    #       and job_definition["base_commit_sha"]
+    #
+    # We historically tolerate nil values for both these keys from the `job_definition`
+    # but it doesn't seem like we should. Rather than introduce a behaviour change
+    # as part of the change introducing this class, let's do it as a follow-up.
+    def self.create_from_job_definition(job:, job_definition:)
+      decoded_dependency_files = job_definition.fetch("base64_dependency_files", []).map do |a|
+        file = Dependabot::DependencyFile.new(**a.transform_keys(&:to_sym))
+        file.content = Base64.decode64(file.content).force_encoding("utf-8") unless file.binary? && !file.deleted?
+        file
+      end
+
+      new(
+        job: job,
+        base_commit_sha: job_definition.fetch("base_commit_sha", nil),
+        dependency_files: decoded_dependency_files
+      )
+    end
+
+    attr_reader :base_commit_sha, :dependency_files
+
+    def initialize(job:, base_commit_sha:, dependency_files:)
+      @job = job
+      @base_commit_sha = base_commit_sha
+      @dependency_files = dependency_files
+    end
+
+    def dependencies
+      return @dependencies if defined?(@dependencies)
+
+      parse_files!
+    end
+
+    private
+
+    attr_reader :job
+
+    # TODO: Parse files during instantiation?
+    #
+    # To avoid having to re-home Dependabot::Updater#handle_parser_error,
+    # we perform the parsing lazily when the `dependencies` method is first
+    # referenced.
+    #
+    # We have some unusual behaviour where we handle a parse error by
+    # returning an empty dependency array in Dependabot::Updater#dependencies
+    # in order to 'fall through' to an error outcome elsewhere in the class.
+    #
+    # Given this uncertainity, and the need to significantly refactor tests,
+    # it makes sense to introduce this shim and then deal with the call
+    # site once we've split out the downstream behaviour in the updater.
+    #
+    def parse_files!
+      @dependencies = dependency_file_parser.parse
+    end
+
+    def dependency_file_parser
+      Dependabot::FileParsers.for_package_manager(job.package_manager).new(
+        dependency_files: dependency_files,
+        repo_contents_path: job.repo_contents_path,
+        source: job.source,
+        credentials: job.credentials,
+        reject_external_code: job.reject_external_code?,
+        options: job.experiments
+      )
+    end
+  end
+end

updater/lib/dependabot/service.rb

@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require "raven"
 require "terminal-table"
 require "dependabot/api_client"
 
@@ -42,6 +43,19 @@ def record_update_job_error(error_type:, error_details:, dependency: nil)
       client.record_update_job_error(error_type: error_type, error_details: error_details)
     end
 
+    def update_dependency_list(dependency_snapshot:)
+      dependency_payload = dependency_snapshot.dependencies.map do |dep|
+        {
+          name: dep.name,
+          version: dep.version,
+          requirements: dep.requirements
+        }
+      end
+      dependency_file_paths = dependency_snapshot.dependency_files.reject(&:support_file).map(&:path)
+
+      client.update_dependency_list(dependency_payload, dependency_file_paths)
+    end
+
     # This method wraps the Raven client as the Application error tracker
     # the service uses to notice errors.
     #

updater/lib/dependabot/update_files_command.rb

@@ -2,19 +2,43 @@
 
 require "base64"
 require "dependabot/base_command"
+require "dependabot/dependency_snapshot"
 require "dependabot/updater"
 
 module Dependabot
   class UpdateFilesCommand < BaseCommand
     def perform_job
+      # We expect the FileFetcherCommand to have been executed beforehand to place
+      # encoded files and commit information in the environment, so let's retrieve
+      # and decode them into an object.
+
+      # TODO: Parse the dependency files when instantiated
+      #
+      # We can pull the error handling for parser exceptions up into this class to
+      # completely remove the concern from Dependabot::Updater.
+      #
+      # This should happen separately to introducing the class as a shim.
+      #
+      # See: updater/lib/dependabot/dependency_snapshot.rb:52
+      dependency_snapshot = Dependabot::DependencySnapshot.create_from_job_definition(
+        job: job,
+        job_definition: Environment.job_definition
+      )
+
+      # TODO: Pull fatal error handling handling up into this class
+      #
+      # As above, we can remove the responsibility for handling fatal/job halting
+      # errors from Dependabot::Updater entirely.
       Dependabot::Updater.new(
         service: service,
         job: job,
-        dependency_files: dependency_files,
-        base_commit_sha: base_commit_sha
+        dependency_snapshot: dependency_snapshot
       ).run
 
-      service.mark_job_as_processed(base_commit_sha)
+      # Finally, mark the job as processed. The Dependabot::Updater may have
+      # reported errors to the service, but we always consider the job as
+      # successfully processed unless it actually raises.
+      service.mark_job_as_processed(dependency_snapshot.base_commit_sha)
     end
 
     private
@@ -26,24 +50,5 @@ def job
         repo_contents_path: Environment.repo_contents_path
       )
     end
-
-    def dependency_files
-      @dependency_files ||=
-        Environment.job_definition["base64_dependency_files"].map do |a|
-          file = Dependabot::DependencyFile.new(**a.transform_keys(&:to_sym))
-          file.content = Base64.decode64(file.content).force_encoding("utf-8") unless file.binary? && !file.deleted?
-          file
-        end
-    end
-
-    def repo_contents_path
-      return nil unless job.clone?
-
-      Environment.repo_contents_path
-    end
-
-    def base_commit_sha
-      Environment.job_definition["base_commit_sha"]
-    end
   end
 end

updater/lib/dependabot/updater.rb

@@ -1,13 +1,11 @@
 # frozen_string_literal: true
 
-require "raven"
 require "dependabot/config/ignore_condition"
 require "dependabot/config/update_config"
 require "dependabot/dependency_change"
 require "dependabot/environment"
 require "dependabot/experiments"
 require "dependabot/file_fetchers"
-require "dependabot/file_parsers"
 require "dependabot/file_updaters"
 require "dependabot/logger"
 require "dependabot/python"
@@ -55,11 +53,14 @@ def initialize(message, raven_context:)
       Octokit::Unauthorized => "octokit_unauthorized"
     }.freeze
 
-    def initialize(service:, job:, dependency_files:, base_commit_sha:)
+    # To do work, this class needs three arguments:
+    # - The Dependabot::Service to send events and outcomes to
+    # - The Dependabot::Job that describes the work to be done
+    # - The Dependabot::DependencySnapshot which encapsulates the starting state of the project
+    def initialize(service:, job:, dependency_snapshot:)
       @service = service
       @job = job
-      @dependency_files = dependency_files
-      @base_commit_sha = base_commit_sha
+      @dependency_snapshot = dependency_snapshot
       # TODO: Collect @created_pull_requests on the Job object?
       @created_pull_requests = []
     end
@@ -94,7 +95,7 @@ def run
     private
 
     attr_accessor :created_pull_requests
-    attr_reader :service, :job, :dependency_files, :base_commit_sha
+    attr_reader :service, :job, :dependency_snapshot
 
     def check_and_create_pr_with_error_handling(dependency)
       check_and_create_pull_request(dependency)
@@ -604,10 +605,10 @@ def existing_pull_request(updated_dependencies)
 
     # rubocop:disable Metrics/PerceivedComplexity
     def dependencies
-      all_deps = dependency_file_parser.parse
+      all_deps = dependency_snapshot.dependencies
 
       # Tell the backend about the current dependencies on the target branch
-      update_dependency_list(all_deps)
+      service.update_dependency_list(dependency_snapshot: dependency_snapshot)
 
       # Rebases and security updates have dependencies, version updates don't
       if job.dependencies
@@ -648,21 +649,10 @@ def dependencies
     end
     # rubocop:enable Metrics/PerceivedComplexity
 
-    def dependency_file_parser
-      Dependabot::FileParsers.for_package_manager(job.package_manager).new(
-        dependency_files: dependency_files,
-        repo_contents_path: job.repo_contents_path,
-        source: job.source,
-        credentials: job.credentials,
-        reject_external_code: job.reject_external_code?,
-        options: job.experiments
-      )
-    end
-
     def update_checker_for(dependency, raise_on_ignored:)
       Dependabot::UpdateCheckers.for_package_manager(job.package_manager).new(
         dependency: dependency,
-        dependency_files: dependency_files,
+        dependency_files: dependency_snapshot.dependency_files,
         repo_contents_path: job.repo_contents_path,
         credentials: job.credentials,
         ignored_versions: ignore_conditions_for(dependency),
@@ -676,7 +666,7 @@ def update_checker_for(dependency, raise_on_ignored:)
     def file_updater_for(dependencies)
       Dependabot::FileUpdaters.for_package_manager(job.package_manager).new(
         dependencies: dependencies,
-        dependency_files: dependency_files,
+        dependency_files: dependency_snapshot.dependency_files,
         repo_contents_path: job.repo_contents_path,
         credentials: job.credentials,
         options: job.experiments
@@ -755,7 +745,7 @@ def create_pull_request(dependencies, updated_dependency_files)
         updated_dependency_files: updated_dependency_files
       )
 
-      service.create_pull_request(dependency_change, base_commit_sha)
+      service.create_pull_request(dependency_change, dependency_snapshot.base_commit_sha)
 
       created_pull_requests << dependencies.map do |dep|
         {
@@ -776,7 +766,7 @@ def update_pull_request(dependencies, updated_dependency_files)
         updated_dependency_files: updated_dependency_files
       )
 
-      service.update_pull_request(dependency_change, base_commit_sha)
+      service.update_pull_request(dependency_change, dependency_snapshot.base_commit_sha)
     end
 
     def close_pull_request(reason:)
@@ -974,19 +964,6 @@ def handle_parser_error(error)
       )
     end
     # rubocop:enable Metrics/MethodLength
-
-    def update_dependency_list(dependencies)
-      service.update_dependency_list(
-        dependencies.map do |dep|
-          {
-            name: dep.name,
-            version: dep.version,
-            requirements: dep.requirements
-          }
-        end,
-        dependency_files.reject(&:support_file).map(&:path)
-      )
-    end
   end
 end
 # rubocop:enable Metrics/ClassLength