capture and report job deserialization errors #11179
Conversation
common/lib/dependabot/errors.rb
Outdated
| { | ||
| "error-type": "illformed_requirement", | ||
| "error-detail": { message: error.message } | ||
| } |
There was a problem hiding this comment.
This block of code already existed in updater_error_details but this error occurs during the fetch phase of a NuGet update so it was simply copied here, too. This is a well-known error with a known shape.
brettfo
force-pushed
the
dev/brettfo/nuget-requirement-parse-error
branch
from
13f3ed2 to
9161873
Compare
| ErrorDetails = errorResult.ErrorDetails, | ||
| }; | ||
| await DiscoveryWorker.WriteResultsAsync(repoRoot.FullName, outputPath.FullName, discoveryErrorResult); | ||
| return; |
There was a problem hiding this comment.
We want this to always report error code 0 so that we get a chance to properly elevate the error type and details in native_helpers.rb below.
JamieMagee
force-pushed
the
dev/brettfo/nuget-requirement-parse-error
branch
from
9161873 to
fb9c62d
Compare
| @@ -228,37 +227,33 @@ public void DeserializeExperimentsManager_NoExperiments() | |||
| Assert.False(experimentsManager.UseDirectDiscovery); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public async Task DeserializeExperimentsManager_UnsupportedJobFileShape_InfoIsReportedAndEmptyExperimentSetIsReturned() | |||
| [Theory] | |||
| { | ||
| return Requirement.Parse(text); | ||
| } | ||
| catch |
There was a problem hiding this comment.
Can we narrow this down? I would have guessed ArgumentException would be most cases.
| @@ -7,7 +7,20 @@ public class RequirementConverter : JsonConverter<Requirement> | |||
| { | |||
| public override Requirement? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) | |||
| { | |||
| return Requirement.Parse(reader.GetString()!); | |||
| var text = reader.GetString(); | |||
There was a problem hiding this comment.
Nitpick: Probably best to check reader.TokenType first (even though hypothetically that was checked before calling the converter).
| return 1; | ||
| } | ||
|
|
||
| var result = await RunAsync(jobFile!.Job, repoContentsPath.FullName); |
There was a problem hiding this comment.
I'm surprised the ! is needed, seems like it ought to know we'll have it populated in this case (unless RunWorker.Deserialize can return null in which case we ought to actually handle it.)
The
job.jsonfile is passed to all update steps which means we need to be able to report all parsing errors as they occur.Originally this work started because a requirement was specified in the job file that could not be parsed, so part of this PR is to return the well-known error
illformed_requirement, represented asBadRequirementError.Most of this PR was to make testing possible, but ultimately it comes down to 2 major changes:
discovercommand. All subsequent commands also parse the job file, but any failure to parse is fatal so we only need to actually report it there; all other instances can assume it's acceptable.clonecommand is the first time the job file is parsed, so the error handling was expanded there to specifically report the parse error and fail the entire update process.While performing some manual tests, I discovered that the shape of the error objects returned by the end-to-end parser wasn't always correct, so I checked in the
dependabot/clirepo to ensure the errors had the correct shape and added serialization tests for all error types to ensure this doesn't regress.