diff --git a/updater/lib/dependabot/dependency_snapshot.rb b/updater/lib/dependabot/dependency_snapshot.rb new file mode 100644 index 00000000000..a4862fc1c21 --- /dev/null +++ b/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 diff --git a/updater/lib/dependabot/service.rb b/updater/lib/dependabot/service.rb index 32fe79bbb50..8be3f04937d 100644 --- a/updater/lib/dependabot/service.rb +++ b/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. # diff --git a/updater/lib/dependabot/update_files_command.rb b/updater/lib/dependabot/update_files_command.rb index 018a1008045..3502b199361 100644 --- a/updater/lib/dependabot/update_files_command.rb +++ b/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 diff --git a/updater/lib/dependabot/updater.rb b/updater/lib/dependabot/updater.rb index 8190cd5dbd2..19e8d9d5ca9 100644 --- a/updater/lib/dependabot/updater.rb +++ b/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 diff --git a/updater/spec/dependabot/service_spec.rb b/updater/spec/dependabot/service_spec.rb index 0fba7f9285e..6765b0c71c5 100644 --- a/updater/spec/dependabot/service_spec.rb +++ b/updater/spec/dependabot/service_spec.rb @@ -3,6 +3,7 @@ require "spec_helper" require "dependabot/api_client" require "dependabot/dependency_change" +require "dependabot/dependency_snapshot" require "dependabot/service" RSpec.describe Dependabot::Service do @@ -158,7 +159,6 @@ describe "Instance methods delegated to @client" do { mark_job_as_processed: %w(mock_sha), - update_dependency_list: %w(mock_dependencies mock_dependency_file), record_package_manager_version: %w(mock_ecosystem mock_package_managers) }.each do |method, arguments| before { allow(mock_client).to receive(method) } @@ -228,6 +228,80 @@ end end + describe "#update_dependency_list" do + let(:dependency_snapshot) do + instance_double(Dependabot::DependencySnapshot, + dependencies: [ + Dependabot::Dependency.new( + name: "dummy-pkg-a", + package_manager: "bundler", + version: "2.0.0", + requirements: [ + { file: "Gemfile", requirement: "~> 2.0.0", groups: [:default], source: nil } + ] + ), + Dependabot::Dependency.new( + name: "dummy-pkg-b", + package_manager: "bundler", + version: "1.1.0", + requirements: [ + { file: "Gemfile", requirement: "~> 1.1.0", groups: [:default], source: nil } + ] + ) + ], + dependency_files: [ + Dependabot::DependencyFile.new( + name: "Gemfile", + content: fixture("bundler/original/Gemfile"), + directory: "/" + ), + Dependabot::DependencyFile.new( + name: "Gemfile.lock", + content: fixture("bundler/original/Gemfile.lock"), + directory: "/" + ) + ]) + end + + let(:expected_dependency_payload) do + [ + { + name: "dummy-pkg-a", + version: "2.0.0", + requirements: [ + { + file: "Gemfile", + requirement: "~> 2.0.0", + groups: [:default], + source: nil + } + ] + }, + { + name: "dummy-pkg-b", + version: "1.1.0", + requirements: [ + { + file: "Gemfile", + requirement: "~> 1.1.0", + groups: [:default], + source: nil + } + ] + } + ] + end + let(:expected_file_paths) do + ["/Gemfile", "/Gemfile.lock"] + end + + it "extracts a payload from the DependencySnapshot and delegates to the client" do + expect(mock_client).to receive(:update_dependency_list).with(expected_dependency_payload, expected_file_paths) + + service.update_dependency_list(dependency_snapshot: dependency_snapshot) + end + end + describe "#noop?" do it "is true by default" do expect(service).to be_noop diff --git a/updater/spec/dependabot/update_files_command_spec.rb b/updater/spec/dependabot/update_files_command_spec.rb index 552f8cb4476..d2037a54b67 100644 --- a/updater/spec/dependabot/update_files_command_spec.rb +++ b/updater/spec/dependabot/update_files_command_spec.rb @@ -35,8 +35,7 @@ with( service: service, job: an_object_having_attributes(id: job_id, repo_contents_path: nil), - dependency_files: anything, - base_commit_sha: base_commit_sha + dependency_snapshot: an_object_having_attributes(base_commit_sha: base_commit_sha) ). and_return(dummy_runner) expect(dummy_runner).to receive(:run) @@ -59,8 +58,7 @@ with( service: service, job: an_object_having_attributes(id: job_id, repo_contents_path: repo_contents_path), - dependency_files: anything, - base_commit_sha: base_commit_sha + dependency_snapshot: an_object_having_attributes(base_commit_sha: base_commit_sha) ). and_return(dummy_runner) expect(dummy_runner).to receive(:run) diff --git a/updater/spec/dependabot/updater_spec.rb b/updater/spec/dependabot/updater_spec.rb index 74e5cf3331a..40852ac2454 100644 --- a/updater/spec/dependabot/updater_spec.rb +++ b/updater/spec/dependabot/updater_spec.rb @@ -5,6 +5,7 @@ require "bundler/compact_index_client/updater" require "dependabot/dependency" require "dependabot/dependency_file" +require "dependabot/dependency_snapshot" require "dependabot/file_fetchers" require "dependabot/updater" require "dependabot/service" @@ -70,36 +71,8 @@ service = build_service updater = build_updater(service: service, job: job) - dependencies = [ - { - name: "dummy-pkg-a", - version: "2.0.0", - requirements: [ - { - file: "Gemfile", - requirement: "~> 2.0.0", - groups: [:default], - source: nil - } - ] - }, - { - name: "dummy-pkg-b", - version: "1.1.0", - requirements: [ - { - file: "Gemfile", - requirement: "~> 1.1.0", - groups: [:default], - source: nil - } - ] - } - ] - dependency_files = ["/Gemfile", "/Gemfile.lock"] - - expect(service). - to receive(:update_dependency_list).with(dependencies, dependency_files) + expect(service).to receive(:update_dependency_list). + with(dependency_snapshot: an_instance_of(Dependabot::DependencySnapshot)) updater.run end @@ -1292,7 +1265,8 @@ def expect_update_checker_with_ignored_versions(versions) service = build_service updater = build_updater(service: service, job: job) - allow(updater).to receive(:dependency_files). + # TODO: Move this stub unto Dependabot::DependencySnapshot once it is better integrated + allow(Dependabot::FileParsers).to receive_message_chain(:for_package_manager, :new, :parse). and_raise(Dependabot::DependencyFileNotParseable.new("path/to/file")) expect(service).to receive(:record_update_job_error).with( @@ -1613,7 +1587,8 @@ def expect_update_checker_with_ignored_versions(versions) service = build_service updater = build_updater(service: service, job: job) - allow(updater).to receive(:dependency_files).and_raise(error) + # TODO: Move this stub unto Dependabot::DependencySnapshot once it is better integrated + allow(Dependabot::FileParsers).to receive_message_chain(:for_package_manager, :new, :parse).and_raise(error) expect(Raven).to receive(:capture_exception) @@ -1630,7 +1605,8 @@ def expect_update_checker_with_ignored_versions(versions) service = build_service updater = build_updater(service: service, job: job) - allow(updater).to receive(:dependency_files).and_raise(error) + # TODO: Move this stub unto Dependabot::DependencySnapshot once it is better integrated + allow(Dependabot::FileParsers).to receive_message_chain(:for_package_manager, :new, :parse).and_raise(error) expect(service). to receive(:record_update_job_error). @@ -1653,7 +1629,8 @@ def expect_update_checker_with_ignored_versions(versions) service = build_service updater = build_updater(service: service, job: job) - allow(updater).to receive(:dependency_files).and_raise(error) + # TODO: Move this stub unto Dependabot::DependencySnapshot once it is better integrated + allow(Dependabot::FileParsers).to receive_message_chain(:for_package_manager, :new, :parse).and_raise(error) expect(Raven).to_not receive(:capture_exception) @@ -1670,7 +1647,8 @@ def expect_update_checker_with_ignored_versions(versions) service = build_service updater = build_updater(service: service, job: job) - allow(updater).to receive(:dependency_files).and_raise(error) + # TODO: Move this stub unto Dependabot::DependencySnapshot once it is better integrated + allow(Dependabot::FileParsers).to receive_message_chain(:for_package_manager, :new, :parse).and_raise(error) expect(service). to receive(:record_update_job_error). @@ -1694,7 +1672,8 @@ def expect_update_checker_with_ignored_versions(versions) service = build_service updater = build_updater(service: service, job: job) - allow(updater).to receive(:dependency_files).and_raise(error) + # TODO: Move this stub unto Dependabot::DependencySnapshot once it is better integrated + allow(Dependabot::FileParsers).to receive_message_chain(:for_package_manager, :new, :parse).and_raise(error) expect(Raven).to_not receive(:capture_exception) @@ -1711,7 +1690,8 @@ def expect_update_checker_with_ignored_versions(versions) service = build_service updater = build_updater(service: service, job: job) - allow(updater).to receive(:dependency_files).and_raise(error) + # TODO: Move this stub unto Dependabot::DependencySnapshot once it is better integrated + allow(Dependabot::FileParsers).to receive_message_chain(:for_package_manager, :new, :parse).and_raise(error) expect(service). to receive(:record_update_job_error). @@ -1735,7 +1715,8 @@ def expect_update_checker_with_ignored_versions(versions) service = build_service updater = build_updater(service: service, job: job) - allow(updater).to receive(:dependency_files).and_raise(error) + # TODO: Move this stub unto Dependabot::DependencySnapshot once it is better integrated + allow(Dependabot::FileParsers).to receive_message_chain(:for_package_manager, :new, :parse).and_raise(error) expect(Raven).to_not receive(:capture_exception) @@ -1752,7 +1733,8 @@ def expect_update_checker_with_ignored_versions(versions) service = build_service updater = build_updater(service: service, job: job) - allow(updater).to receive(:dependency_files).and_raise(error) + # TODO: Move this stub unto Dependabot::DependencySnapshot once it is better integrated + allow(Dependabot::FileParsers).to receive_message_chain(:for_package_manager, :new, :parse).and_raise(error) expect(service). to receive(:record_update_job_error). @@ -1776,7 +1758,8 @@ def expect_update_checker_with_ignored_versions(versions) service = build_service updater = build_updater(service: service, job: job) - allow(updater).to receive(:dependency_files).and_raise(error) + # TODO: Move this stub unto Dependabot::DependencySnapshot once it is better integrated + allow(Dependabot::FileParsers).to receive_message_chain(:for_package_manager, :new, :parse).and_raise(error) expect(Raven).to_not receive(:capture_exception) @@ -1793,7 +1776,8 @@ def expect_update_checker_with_ignored_versions(versions) service = build_service updater = build_updater(service: service, job: job) - allow(updater).to receive(:dependency_files).and_raise(error) + # TODO: Move this stub unto Dependabot::DependencySnapshot once it is better integrated + allow(Dependabot::FileParsers).to receive_message_chain(:for_package_manager, :new, :parse).and_raise(error) expect(service). to receive(:record_update_job_error). @@ -2485,8 +2469,11 @@ def build_updater(service: build_service, job: build_job, dependency_files: defa Dependabot::Updater.new( service: service, job: job, - dependency_files: dependency_files, - base_commit_sha: "sha" + dependency_snapshot: Dependabot::DependencySnapshot.new( + job: job, + dependency_files: dependency_files, + base_commit_sha: "sha" + ) ) end