[Updater] Break out handler methods for Dependabot::Error classes into a small utility class

[brrygrdn]

Follows up on #6847 which must merge first

This is small and somewhat imperfect extract-to-class refactor to move aside some long methods we use for presenting Dependabot::Errors via the Dependabot::Service and/or logging.

As we split out distinct operator classes, duplicating these methods is really undesirable as they have a lot of specific behaviour that would be hard to keep in alignment.

I think there's a deeper refactor to be had here since methods ultimately derive an "error detail" hash based on the subclass of Dependabot::Error being handled, e.g.

{
  "error-type": "private_source_timed_out",
  "error-detail": { source: error.source }
}

I think this could just become a method on each Dependabot::Error subclass, but that would require a significant change in core with substantial testing to be confident.

Likewise, I think the handle_dependabot_error and handle_parser_error could be DRY'd out, but the parsing itself should be pulled up out of the Updater so doing a deeper change at this time might be counter-productive.

[landongrindheim]

🤩

[landongrindheim]

🤔 Would it make sense to expose a method that signals that parsing has completed? I understand defining @dependencies signals this internally, and that we may not want to have collaborators checking this externally. Maybe something like a result object would be appropriate, given your note about handling parse errors.

None of this is necessary, but maybe it would be useful? I'm interested in your thoughts as we're still paving the path for further changes.

[brrygrdn]

we may not want to have collaborators checking this externally.

I think the call order we have right now means we can proceed if parsing fails, but from reasoning through the code I don't think we ever actually should? i.e. there shouldn't be any collaboration if parsing fails, it's always a sign to stop the job.

dependabot-core/updater/lib/dependabot/updater.rb

Lines 71 to 77 in 8f9e473

	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

In the existing behaviour, if parsing fails, we get an empty value for "dependencies" at the code above.

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

For updating an existing PR, we end up here:

dependabot-core/updater/lib/dependabot/updater.rb

Lines 134 to 146 in 8f9e473

	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.

This is feels an awful lot like cleanly closing the job accidentally/on a weak signal. If we stop immediately when parsing fails, it isn't actually a behaviour change, but it does mean reshuffling a lot of tests around so I'm reluctant to follow through for now.

[landongrindheim]

I think a no-op of sorts, probably in the shape of error forwarding, makes sense here.

[landongrindheim]

👍 This feels like a solid place for this logic 🏡

[landongrindheim]

I'm appreciating this as a place to put error handling. The reasons for it to change will be limited to new errors and new requirements to handle those errors. That feels right to me, especially compared to the previous location 🙂