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

[brrygrdn]

Background

In the Updater, if file parsing fails, we get an empty value for "dependencies" at the code here.

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

We lazily evaluate dependencies in the Dependabot::Updater#run method which results in us quietly exiting the job in two difference places based on this if statement:

    def run
      return unless job


      if job.updating_a_pull_request?
        Dependabot.logger.info("Starting PR update job for #{job.source.repo}")
        check_and_update_existing_pr_with_error_handling(dependencies)
      else
        Dependabot.logger.info("Starting update job for #{job.source.repo}")
        dependencies.each { |dep| check_and_create_pr_with_error_handling(dep) }
      end
    rescue *ErrorHandler::RUN_HALTING_ERRORS.keys => e
      if e.is_a?(Dependabot::AllVersionsIgnored) && !job.security_updates_only?
        error = StandardError.new(
          "Dependabot::AllVersionsIgnored was unexpectedly raised for a non-security update job"
        )
        error.set_backtrace(e.backtrace)
        service.capture_exception(error: error, job: job)
        return
      end

For a new update, [] means we don't iterate and the job silently ends on the line:

dependencies.each { |dep| check_and_create_pr_with_error_handling(dep) }

For updating an existing PR, we end up here:

      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?
        return
      end

This will nearly always result in [] != [$at_least_one_dep], so we exit. We don't close the PR as the parser failure has placed an error on the job, but we could have the same outcome by stopping if the parse fails.

The Change

This branch yoinks all file parsing concerns to the very start of the UpdateFileCommand so we retrieve the files from the job definition, decode them and parse them before starting into any actual update logic.

This means that all exception handling around parsing failures can happen immediately and clearly around the instantiation of the Dependabot::DependencySnapshot, removing a significant concern from the Dependabot::Updater class and clarifying that we only expect update time errors to be handled inside it, not parse errors.

Rationale

The main reason to do this now is we still need to split out another 3 "Operation" classes in the style of #6866.

This work will break the Updater down further, allowing us to pull apart its responsibilities and break the test cases across some concrete classes. In its current state, file parsing would end up becoming a concern of every responsibility, but a code walk tells us that we can short-circuit this and turn it into a precondition.

This will significantly reduce the remaining work by allowing the rest of the class breakdown to be focused on just generating updates and posting PRs.

[jakecoffman]

I like this!

[landongrindheim]

🔎 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.

[brrygrdn]

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.

[landongrindheim]

Thanks for this context 😄

[landongrindheim]

🤩

[landongrindheim]

😆