Skip to content

[Updater] Perform Dependency file parsing early, and fail fast if there's a problem #6906

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
brrygrdn merged 9 commits into from
Mar 28, 2023
Merged

[Updater] Perform Dependency file parsing early, and fail fast if there's a problem #6906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8cd7ff8
Enforce non-nil expectations for base commit and files
brrygrdn Mar 24, 2023
bc9a74f
Dependabot::Snapshot hydrates dependency files at instantiation
brrygrdn Mar 24, 2023
8f3342a
Handle parser errors in the UpdateFilesCommand
brrygrdn Mar 24, 2023
d675286
Migrate experiment test for the file parser into the snapshot spec
brrygrdn Mar 24, 2023
c28fb5b
Remove a test case that can no longer happen for updating PRs
brrygrdn Mar 24, 2023
f9522c0
Remove parse-specific tests from the Updater, Remove duplicates
brrygrdn Mar 24, 2023
d841375
Remove a test for a nil job
brrygrdn Mar 24, 2023
f16f959
Get rid of some rubocop exceptions :tada:
brrygrdn Mar 24, 2023
d4ebbcc
Assign dependencies during init
brrygrdn Mar 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 7 additions & 31 deletions updater/lib/dependabot/dependency_snapshot.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,60 +11,36 @@
# 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|
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),
base_commit_sha: job_definition.fetch("base_commit_sha"),
dependency_files: decoded_dependency_files
)
end

attr_reader :base_commit_sha, :dependency_files
attr_reader :base_commit_sha, :dependency_files, :dependencies

private

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!
@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
dependency_file_parser.parse
end

def dependency_file_parser
Expand Down
115 changes: 101 additions & 14 deletions updater/lib/dependabot/update_files_command.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,19 @@ 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
)
# them, decode and parse them into an object that knows the current state
# of the project's dependencies.
begin
dependency_snapshot = Dependabot::DependencySnapshot.create_from_job_definition(
job: job,
job_definition: Environment.job_definition
)
rescue StandardError => e
handle_parser_error(e)
Copy link
Copy Markdown
Contributor

@landongrindheim landongrindheim Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔎 What do you think of calling ErrorHandler here and leaving the logic in that class? I really like the idea of isolating error handling, but I also see the benefit of leaving the logic close to where error handling needs to happen.

Copy link
Copy Markdown
Contributor Author

@brrygrdn brrygrdn Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm split on this as I originally created that class to contain these two long methods, but because it is namespaced within the Updater it felt right to remove the concern from it as being out of bounds.

I'm still a bit uncertain of the long term place for that helper, ideally we'd push the need to do the dance to get from DependabotError class to error-details hash to be a convention where every error implemented the "presentation" hash directly but that's a big lift and pretty outside the critical path.

# If dependency file parsing has failed, there's nothing more we can do,
# so let's mark the job as processed and stop.
return service.mark_job_as_processed(Environment.job_definition["base_commit_sha"])
end

# TODO: Pull fatal error handling handling up into this class
#
Expand All @@ -50,5 +49,93 @@ def job
repo_contents_path: Environment.repo_contents_path
)
end

# rubocop:disable Metrics/MethodLength
def handle_parser_error(error)
# This happens if the repo gets removed after a job gets kicked off.
# The service will handle the removal without any prompt from the updater,
# so no need to add an error to the errors array
return if error.is_a? Dependabot::RepoNotFound

error_details =
case error
when Dependabot::DependencyFileNotEvaluatable
{
"error-type": "dependency_file_not_evaluatable",
"error-detail": { message: error.message }
}
when Dependabot::DependencyFileNotResolvable
{
"error-type": "dependency_file_not_resolvable",
"error-detail": { message: error.message }
}
when Dependabot::BranchNotFound
{
"error-type": "branch_not_found",
"error-detail": { "branch-name": error.branch_name }
}
when Dependabot::DependencyFileNotParseable
{
"error-type": "dependency_file_not_parseable",
"error-detail": {
message: error.message,
"file-path": error.file_path
}
}
when Dependabot::DependencyFileNotFound
{
"error-type": "dependency_file_not_found",
"error-detail": { "file-path": error.file_path }
}
when Dependabot::PathDependenciesNotReachable
{
"error-type": "path_dependencies_not_reachable",
"error-detail": { dependencies: error.dependencies }
}
when Dependabot::PrivateSourceAuthenticationFailure
{
"error-type": "private_source_authentication_failure",
"error-detail": { source: error.source }
}
when Dependabot::GitDependenciesNotReachable
{
"error-type": "git_dependencies_not_reachable",
"error-detail": { "dependency-urls": error.dependency_urls }
}
when Dependabot::NotImplemented
{
"error-type": "not_implemented",
"error-detail": {
message: error.message
}
}
when Octokit::ServerError
# If we get a 500 from GitHub there's very little we can do about it,
# and responsibility for fixing it is on them, not us. As a result we
# quietly log these as errors
{ "error-type": "unknown_error" }
else
# Check if the error is a known "run halting" state we should handle
if (error_type = Updater::ErrorHandler::RUN_HALTING_ERRORS[error.class])
{ "error-type": error_type }
else
# If it isn't, then log all the details and let the application error
# tracker know about it
Dependabot.logger.error error.message
error.backtrace.each { |line| Dependabot.logger.error line }

service.capture_exception(error: error, job: job)

# Set an unknown error type to be added to the job
{ "error-type": "unknown_error" }
end
end

service.record_update_job_error(
error_type: error_details.fetch(:"error-type"),
error_details: error_details[:"error-detail"]
)
end
# rubocop:enable Metrics/MethodLength
end
end
21 changes: 4 additions & 17 deletions updater/lib/dependabot/updater.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,16 +133,10 @@ def check_and_update_existing_pr_with_error_handling(dependencies)
# DependencyChange as we build it up step-by-step.
def check_and_update_pull_request(dependencies)
if dependencies.count != job.dependencies.count
# TODO: Remove the unless statement
#
# This check is to determine if there was an error parsing the dependency
# dependency file.
#
# For update existing PR operations we should early exit after a failed
# parse instead, but we currently share the `#dependencies` method
# with other code paths. This will be fixed as we break out Operation
# classes.
close_pull_request(reason: :dependency_removed) unless service.errors.any?
# If the job dependencies mismatch the parsed dependencies, then
# we should close the PR as at least one thing we changed has been
# removed from the project.
Comment on lines 136 to 138
Copy link
Copy Markdown
Contributor

@landongrindheim landongrindheim Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this context 😄

close_pull_request(reason: :dependency_removed)
return
end

Expand Down Expand Up @@ -591,8 +585,6 @@ def existing_pull_request(updated_dependencies)
created_pull_requests.find { |pr| Set.new(pr) == new_pr_set }
end

# rubocop:disable Metrics/AbcSize
# rubocop:disable Metrics/PerceivedComplexity
Comment on lines 594 to 595
Copy link
Copy Markdown
Contributor

@landongrindheim landongrindheim Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤩

def dependencies
all_deps = dependency_snapshot.dependencies

Expand Down Expand Up @@ -632,12 +624,7 @@ def dependencies
allowed_deps.reject { |d| job.vulnerable?(d) }

deps
rescue StandardError => e
error_handler.handle_parser_error(e)
[]
end
# rubocop:enable Metrics/PerceivedComplexity
# rubocop:enable Metrics/AbcSize

def update_checker_for(dependency, raise_on_ignored:)
Dependabot::UpdateCheckers.for_package_manager(job.package_manager).new(
Expand Down
81 changes: 0 additions & 81 deletions updater/lib/dependabot/updater/error_handler.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,87 +145,6 @@ def handle_dependabot_error(error:, dependency:)
# rubocop:enable Metrics/MethodLength
# rubocop:enable Metrics/AbcSize

# rubocop:disable Metrics/MethodLength
def handle_parser_error(error)
# This happens if the repo gets removed after a job gets kicked off.
# The service will handle the removal without any prompt from the updater,
# so no need to add an error to the errors array
return if error.is_a? Dependabot::RepoNotFound

error_details =
case error
when Dependabot::DependencyFileNotEvaluatable
{
"error-type": "dependency_file_not_evaluatable",
"error-detail": { message: error.message }
}
when Dependabot::DependencyFileNotResolvable
{
"error-type": "dependency_file_not_resolvable",
"error-detail": { message: error.message }
}
when Dependabot::BranchNotFound
{
"error-type": "branch_not_found",
"error-detail": { "branch-name": error.branch_name }
}
when Dependabot::DependencyFileNotParseable
{
"error-type": "dependency_file_not_parseable",
"error-detail": {
message: error.message,
"file-path": error.file_path
}
}
when Dependabot::DependencyFileNotFound
{
"error-type": "dependency_file_not_found",
"error-detail": { "file-path": error.file_path }
}
when Dependabot::PathDependenciesNotReachable
{
"error-type": "path_dependencies_not_reachable",
"error-detail": { dependencies: error.dependencies }
}
when Dependabot::PrivateSourceAuthenticationFailure
{
"error-type": "private_source_authentication_failure",
"error-detail": { source: error.source }
}
when Dependabot::GitDependenciesNotReachable
{
"error-type": "git_dependencies_not_reachable",
"error-detail": { "dependency-urls": error.dependency_urls }
}
when Dependabot::NotImplemented
{
"error-type": "not_implemented",
"error-detail": {
message: error.message
}
}
when Octokit::ServerError
# If we get a 500 from GitHub there's very little we can do about it,
# and responsibility for fixing it is on them, not us. As a result we
# quietly log these as errors
{ "error-type": "unknown_error" }
else
raise if RUN_HALTING_ERRORS.keys.any? { |e| error.is_a?(e) }

Dependabot.logger.error error.message
error.backtrace.each { |line| Dependabot.logger.error line }

service.capture_exception(error: error, job: job)
{ "error-type": "unknown_error" }
end

service.record_update_job_error(
error_type: error_details.fetch(:"error-type"),
error_details: error_details[:"error-detail"]
)
end
# rubocop:enable Metrics/MethodLength

def log_error(dependency:, error:, error_type:, error_detail: nil)
if error_type == "unknown_error"
Dependabot.logger.error "Error processing #{dependency.name} (#{error.class.name})"
Expand Down
3 changes: 0 additions & 3 deletions updater/lib/dependabot/updater/operations/group_update_all_versions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,6 @@ def dependencies
end

allowed_deps
rescue StandardError => e
error_handler.handle_parser_error(e)
[]
end

# Returns a Dependabot::DependencyChange object that encapsulates the
Expand Down
3 changes: 0 additions & 3 deletions updater/lib/dependabot/updater/operations/update_all_versions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,6 @@ def dependencies
end

allowed_deps
rescue StandardError => e
error_handler.handle_parser_error(e)
[]
end

def check_and_create_pr_with_error_handling(dependency)
Expand Down
Loading