Throw an exception with a message when unable to read corrupted .nupkg.metadata #6077
Conversation
|
|
||
| throw; | ||
| throw new Exception(message); |
There was a problem hiding this comment.
| throw new Exception(message); | |
| throw new Exception(message, ex); |
It's good practise to always retain the original exception as an inner exception.
|
|
||
| throw; | ||
| throw new Exception(message); |
There was a problem hiding this comment.
It's also good practice to throw specific exceptions, not use the base Exception type. Perhaps an ArgumentException can be used. From a quick look at the SystemException docs' list of derived types, I don't see anything else that looks better, but I only had a very quick look.
| @@ -53,11 +53,11 @@ public static NupkgMetadataFile Read(TextReader reader, ILogger log, string path | |||
| } | |||
| catch (Exception ex) | |||
| { | |||
| log.LogWarning(string.Format(CultureInfo.CurrentCulture, | |||
| var message = string.Format(CultureInfo.CurrentCulture, | |||
There was a problem hiding this comment.
You mentioned we don't want to double log, can you please show an example of how it looked like vs how it looks like now?
The other thing I'm curious about is how it looks with your other change about failing restore gracefully.
Should this change be retested with that one?
There was a problem hiding this comment.
Good question. This was more of a conclusion I reached based on the code. We have log.logWarning statement followed by an throw statement. However, after manually testing the scenario, I did not see any double logs in the console.
microsoft-github-policy-service
bot
added
the
Status:No recent activity
|
This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch. |
dotnet-policy-service
bot
removed
the
Status:No recent activity
Bug
Fixes: NuGet/Home#13763
Description
Previously when nuget comes across a nupkg.metadata that is corrupt, it logs a warning and rethrows an exception. The exception that is rethrown does not have a message that specifies the path of the corrupt metadata. This PR does the following
PR Checklist