[Updater] Remove an unused ApiClient method, Make Dependabot::Job an explicit receiver for more parameters

updater/lib/dependabot/api_client.rb

@@ -22,36 +22,6 @@ def initialize(base_url, job_id, job_token)
       @job_token = job_token
     end
 
-    # TODO: Remove
-    #
-    # We don't seem to use this anymore and always read the job description
-    # from the file system.
-    def fetch_job
-      response = fetch_job_details_from_backend
-
-      # If the job has already been accessed then we can safely return quietly.
-      # This happens when the backend isn't sure if the updater has enqueued a
-      # job (because Heroku served a 500, for example) and enqueues a second to
-      # be on the safe side.
-      return if response.code == 400 && response.body.include?("been accessed")
-
-      # For other errors from the backend, just raise.
-      raise ApiError, response.body if response.code >= 400
-
-      job_data =
-        response.parse["data"]["attributes"].
-        transform_keys { |k| k.tr("-", "_").to_sym }.
-        slice(
-          :credentials, :dependencies, :package_manager, :ignore_conditions,
-          :existing_pull_requests, :source, :lockfile_only, :allowed_updates,
-          :update_subdependencies, :updating_a_pull_request,
-          :requirements_update_strategy, :security_advisories,
-          :vendor_dependencies, :security_updates_only
-        )
-
-      Job.new(job_data.merge(id: job_id, token: job_token))
-    end
-
     # TODO: Make `base_commit_sha` part of Dependabot::DependencyChange
     def create_pull_request(dependency_change, base_commit_sha)
       api_url = "#{base_url}/update_jobs/#{job_id}/create_pull_request"
@@ -181,18 +151,6 @@ def http_client
       client
     end
 
-    def fetch_job_details_from_backend
-      api_url = "#{base_url}/update_jobs/#{job_id}"
-      http_client.get(api_url)
-    rescue HTTP::ConnectionError, OpenSSL::SSL::SSLError
-      # Retry connection errors (which are almost certainly transitory)
-      retry_count ||= 0
-      retry_count += 1
-      raise if retry_count > 3
-
-      sleep(rand(3.0..10.0)) && retry
-    end
-
     def create_pull_request_data(dependency_change, base_commit_sha)
       data = {
         dependencies: dependency_change.dependencies.map do |dep|

updater/lib/dependabot/file_fetcher_command.rb

@@ -47,6 +47,8 @@ def perform_job
       save_job_details
     end
 
+    private
+
     def save_job_details
       # TODO: Use the Dependabot::Environment helper for this
       return unless ENV["UPDATER_ONE_CONTAINER"]
@@ -81,20 +83,11 @@ def base64_dependency_files
     end
 
     def job
-      attrs =
-        Environment.job_definition["job"].
-        transform_keys { |key| key.tr("-", "_") }.
-        transform_keys(&:to_sym).
-        slice(
-          :dependencies, :package_manager, :ignore_conditions,
-          :existing_pull_requests, :source, :lockfile_only, :allowed_updates,
-          :update_subdependencies, :updating_a_pull_request,
-          :requirements_update_strategy, :security_advisories,
-          :vendor_dependencies, :experiments, :reject_external_code,
-          :commit_message_options, :security_updates_only
-        )
-
-      @job ||= Job.new(attrs.merge(id: job_id))
+      @job ||= Job.new_fetch_job(
+        job_id: job_id,
+        job_definition: Environment.job_definition,
+        repo_contents_path: Environment.repo_contents_path
+      )
     end
 
     def file_fetcher
@@ -105,10 +98,18 @@ def file_fetcher
         credentials: Environment.job_definition.fetch("credentials", []),
         options: job.experiments
       }
-      args[:repo_contents_path] = Environment.repo_contents_path if job.clone? || job.already_cloned?
-      @file_fetcher ||=
-        Dependabot::FileFetchers.for_package_manager(job.package_manager).
-        new(**args)
+      # This bypasses the `job.repo_contents_path` presenter to ensure we fetch
+      # from the file system if the repository contents are mounted even if
+      # cloning is disabled.
+      args[:repo_contents_path] = Environment.repo_contents_path if job.clone? || already_cloned?
+      @file_fetcher ||= Dependabot::FileFetchers.for_package_manager(job.package_manager).new(**args)
+    end
+
+    def already_cloned?
+      return false unless Environment.repo_contents_path
+
+      # For testing, the source repo may already be mounted.
+      @already_cloned ||= File.directory?(File.join(Environment.repo_contents_path, ".git"))
     end
 
     # rubocop:disable Metrics/MethodLength

updater/lib/dependabot/job.rb

@@ -17,11 +17,58 @@
 module Dependabot
   class Job
     TOP_LEVEL_DEPENDENCY_TYPES = %w(direct production development).freeze
-
-    attr_reader :id, :token, :dependencies, :package_manager, :ignore_conditions,
-                :existing_pull_requests, :source, :credentials,
-                :requirements_update_strategy, :security_advisories,
-                :allowed_updates, :vendor_dependencies, :security_updates_only
+    PERMITTED_KEYS = %i(
+      allowed_updates
+      commit_message_options
+      dependencies
+      existing_pull_requests
+      experiments
+      ignore_conditions
+      lockfile_only
+      package_manager
+      reject_external_code
+      repo_contents_path
+      requirements_update_strategy
+      security_advisories
+      security_updates_only
+      source
+      update_subdependencies
+      updating_a_pull_request
+      vendor_dependencies
+    )
+
+    attr_reader :allowed_updates,
+                :credentials,
+                :dependencies,
+                :existing_pull_requests,
+                :id,
+                :ignore_conditions,
+                :package_manager,
+                :requirements_update_strategy,
+                :security_advisories,
+                :security_updates_only,
+                :source,
+                :token,
+                :vendor_dependencies
+
+    def self.new_fetch_job(job_id:, job_definition:, repo_contents_path: nil)
+      attrs = standardise_keys(job_definition["job"]).slice(*PERMITTED_KEYS)
+
+      new(attrs.merge(id: job_id, repo_contents_path: repo_contents_path))
+    end
+
+    def self.new_update_job(job_id:, job_definition:, repo_contents_path: nil)
+      attrs = standardise_keys(job_definition["job"]).slice(*PERMITTED_KEYS)
+      # The Updater should NOT have access to credentials. Let's use metadata, which
+      # can be used by the proxy for matching and applying the real credentials
+      attrs[:credentials] = job_definition.dig("job", "credentials_metadata") || []
+
+      new(attrs.merge(id: job_id, repo_contents_path: repo_contents_path))
+    end
+
+    def self.standardise_keys(hash)
+      hash.transform_keys { |key| key.tr("-", "_").to_sym }
+    end
 
     # NOTE: "attributes" are fetched and injected at run time from
     # dependabot-api using the UpdateJobPrivateSerializer
@@ -37,6 +84,7 @@ def initialize(attributes)
       @lockfile_only                = attributes.fetch(:lockfile_only)
       @package_manager              = attributes.fetch(:package_manager)
       @reject_external_code         = attributes.fetch(:reject_external_code, false)
+      @repo_contents_path           = attributes.fetch(:repo_contents_path, nil)
       @requirements_update_strategy = attributes.fetch(:requirements_update_strategy)
       @security_advisories          = attributes.fetch(:security_advisories)
       @security_updates_only        = attributes.fetch(:security_updates_only)
@@ -54,11 +102,13 @@ def clone?
         Dependabot::Utils.always_clone_for_package_manager?(@package_manager)
     end
 
-    def already_cloned?
-      return unless Environment.repo_contents_path
+    # Some Core components test for a non-nil repo_contents_path as an implicit
+    # signal they should use cloning behaviour, so we present it as nil unless
+    # cloning is enabled to avoid unexpected behaviour.
+    def repo_contents_path
+      return nil unless clone?
 
-      # For testing, the source repo may already be mounted.
-      @already_cloned ||= File.directory?(File.join(Environment.repo_contents_path, ".git"))
+      @repo_contents_path
     end
 
     def lockfile_only?
@@ -140,25 +190,19 @@ def security_fix?(dependency)
     end
 
     def name_normaliser
-      Dependabot::Dependency.
-        name_normaliser_for_package_manager(package_manager)
+      Dependabot::Dependency.name_normaliser_for_package_manager(package_manager)
     end
 
     def experiments
       return {} unless @experiments
 
-      @experiments.
-        transform_keys { |key| key.tr("-", "_") }.
-        transform_keys(&:to_sym)
+      self.class.standardise_keys(@experiments)
     end
 
     def commit_message_options
       return {} unless @commit_message_options
 
-      @commit_message_options.
-        transform_keys { |key| key.tr("-", "_") }.
-        transform_keys(&:to_sym).
-        compact
+      self.class.standardise_keys(@commit_message_options).compact
     end
 
     private

updater/lib/dependabot/service.rb

@@ -17,7 +17,7 @@ def initialize(client:)
       @errors = []
     end
 
-    def_delegators :client, :fetch_job, :mark_job_as_processed, :update_dependency_list, :record_package_manager_version
+    def_delegators :client, :mark_job_as_processed, :update_dependency_list, :record_package_manager_version
 
     def create_pull_request(dependency_change, base_commit_sha)
       client.create_pull_request(dependency_change, base_commit_sha)

updater/lib/dependabot/update_files_command.rb

@@ -9,32 +9,22 @@ class UpdateFilesCommand < BaseCommand
     def perform_job
       Dependabot::Updater.new(
         service: service,
-        job_id: job_id,
         job: job,
         dependency_files: dependency_files,
-        repo_contents_path: repo_contents_path,
         base_commit_sha: base_commit_sha
       ).run
 
       service.mark_job_as_processed(base_commit_sha)
     end
 
+    private
+
     def job
-      attrs =
-        Environment.job_definition["job"].
-        transform_keys { |key| key.tr("-", "_") }.
-        transform_keys(&:to_sym).
-        tap { |h| h[:credentials] = h.delete(:credentials_metadata) || [] }.
-        slice(
-          :dependencies, :package_manager, :ignore_conditions,
-          :existing_pull_requests, :source, :lockfile_only, :allowed_updates,
-          :update_subdependencies, :updating_a_pull_request, :credentials,
-          :requirements_update_strategy, :security_advisories,
-          :vendor_dependencies, :experiments, :reject_external_code,
-          :commit_message_options, :security_updates_only
-        )
-
-      @job ||= Job.new(attrs.merge(id: job_id))
+      @job ||= Job.new_update_job(
+        job_id: job_id,
+        job_definition: Environment.job_definition,
+        repo_contents_path: Environment.repo_contents_path
+      )
     end
 
     def dependency_files

updater/lib/dependabot/updater.rb

@@ -55,15 +55,12 @@ def initialize(message, raven_context:)
       Octokit::Unauthorized => "octokit_unauthorized"
     }.freeze
 
-    def initialize(service:, job_id:, job:, dependency_files:,
-                   base_commit_sha:, repo_contents_path:)
+    def initialize(service:, job:, dependency_files:, base_commit_sha:)
       @service = service
-      @job_id = job_id
       @job = job
       @dependency_files = dependency_files
       @base_commit_sha = base_commit_sha
-      @repo_contents_path = repo_contents_path
-      # TODO: Collect @created_pull_requests and @errors on the Job object
+      # TODO: Collect @created_pull_requests and @errors on the Job object?
       @errors = []
       @created_pull_requests = []
     end
@@ -96,8 +93,7 @@ def run
     private
 
     attr_accessor :errors, :created_pull_requests
-    attr_reader :service, :job_id, :job, :dependency_files, :base_commit_sha,
-                :repo_contents_path
+    attr_reader :service, :job, :dependency_files, :base_commit_sha
 
     def check_and_create_pr_with_error_handling(dependency)
       check_and_create_pull_request(dependency)
@@ -658,9 +654,9 @@ def dependencies
     def dependency_file_parser
       Dependabot::FileParsers.for_package_manager(job.package_manager).new(
         dependency_files: dependency_files,
-        repo_contents_path: repo_contents_path,
+        repo_contents_path: job.repo_contents_path,
         source: job.source,
-        credentials: credentials,
+        credentials: job.credentials,
         reject_external_code: job.reject_external_code?,
         options: job.experiments
       )
@@ -670,8 +666,8 @@ def update_checker_for(dependency, raise_on_ignored:)
       Dependabot::UpdateCheckers.for_package_manager(job.package_manager).new(
         dependency: dependency,
         dependency_files: dependency_files,
-        repo_contents_path: repo_contents_path,
-        credentials: credentials,
+        repo_contents_path: job.repo_contents_path,
+        credentials: job.credentials,
         ignored_versions: ignore_conditions_for(dependency),
         security_advisories: security_advisories_for(dependency),
         raise_on_ignored: raise_on_ignored,
@@ -684,8 +680,8 @@ def file_updater_for(dependencies)
       Dependabot::FileUpdaters.for_package_manager(job.package_manager).new(
         dependencies: dependencies,
         dependency_files: dependency_files,
-        repo_contents_path: repo_contents_path,
-        credentials: credentials,
+        repo_contents_path: job.repo_contents_path,
+        credentials: job.credentials,
         options: job.experiments
       )
     end
@@ -985,11 +981,7 @@ def update_dependency_list(dependencies)
     end
 
     def error_context(dependency)
-      { dependency_name: dependency.name, update_job_id: job_id }
-    end
-
-    def credentials
-      job.credentials
+      { dependency_name: dependency.name, update_job_id: job.id }
     end
 
     def record_error(error_details, dependency: nil)
@@ -1003,8 +995,13 @@ def record_error(error_details, dependency: nil)
     end
 
     def raven_context(dependency: nil)
-      context = { tags: {}, extra: { update_job_id: job_id } }
-      context[:tags][:package_manager] = @job.package_manager if @job
+      context = {
+        tags: {},
+        extra: {
+          update_job_id: job.id,
+          package_manager: job.package_manager
+        }
+      }
       context[:extra][:dependency_name] = dependency.name if dependency
       context
     end

updater/spec/dependabot/api_client_spec.rb

@@ -9,26 +9,6 @@
   subject(:client) { Dependabot::ApiClient.new("http://example.com", 1, "token") }
   let(:headers) { { "Content-Type" => "application/json" } }
 
-  describe "fetch_job" do
-    before do
-      stub_request(:get, "http://example.com/update_jobs/1").
-        to_return(body: fixture("fetch_job.json"), headers: headers)
-    end
-
-    it "hits the correct endpoint" do
-      client.fetch_job
-
-      expect(WebMock).
-        to have_requested(:get, "http://example.com/update_jobs/1").
-        with(headers: { "Authorization" => "token" })
-    end
-
-    it "returns a job" do
-      job = client.fetch_job
-      expect(job).to be_a(Dependabot::Job)
-    end
-  end
-
   describe "create_pull_request" do
     let(:dependency_change) do
       Dependabot::DependencyChange.new(