Skip to content

Ensure we log what the HTTP error message is if we get a failure #78418

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+39 −21
Open

Ensure we log what the HTTP error message is if we get a failure #78418

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f8a3a2d
Ensure we log what the HTTP error message is if we get a failure
jasonmalinowski May 2, 2025
43ebe36
Keep validating the rest of a file
jasonmalinowski May 2, 2025
0f66c8a
We don't need to call IsNullOrWhiteSpace -- we just did that
jasonmalinowski May 2, 2025
2258f7b
Include headers if there is a failure
jasonmalinowski May 2, 2025
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
60 changes: 39 additions & 21 deletions src/RoslynAnalyzers/Tools/GenerateDocumentationAndConfigFiles/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.IO;
using System.Linq;
Expand All @@ -19,6 +20,7 @@
using Analyzer.Utilities.PooledObjects;
using Analyzer.Utilities.PooledObjects.Extensions;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.ReleaseTracking;
using Microsoft.CodeAnalysis.Text;
Expand Down Expand Up @@ -52,7 +54,7 @@ public static async Task<int> Main(string[] args)
validateOnly = false;
}

var fileNamesWithValidationFailures = new List<string>();
var fileNamesWithValidationFailures = new HashSet<string>();

string analyzerRulesetsDir = args[1];
string analyzerEditorconfigsDir = args[2];
Expand Down Expand Up @@ -211,7 +213,9 @@ public static async Task<int> Main(string[] args)
if (fileNamesWithValidationFailures.Count > 0)
{
await Console.Error.WriteLineAsync("One or more auto-generated documentation files were either edited manually, or not updated. Please revert changes made to the following files (if manually edited) and run `dotnet msbuild /t:pack` at the root of the repo to automatically update them:").ConfigureAwait(false);
fileNamesWithValidationFailures.ForEach(fileName => Console.Error.WriteLine($" {fileName}"));
foreach (var fileName in fileNamesWithValidationFailures)
Console.Error.WriteLine($" {fileName}");

return 1;
}

Expand Down Expand Up @@ -615,19 +619,17 @@ Rule ID | Missing Help Link | Title |
DiagnosticDescriptor descriptor = ruleById.Value;

var helpLinkUri = descriptor.HelpLinkUri;
if (!string.IsNullOrWhiteSpace(helpLinkUri) &&
await checkHelpLinkAsync(helpLinkUri).ConfigureAwait(false))
{
// Rule with valid documentation link
if (string.IsNullOrWhiteSpace(helpLinkUri))
continue;
Copy link
Member

Choose a reason for hiding this comment

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

unless I am misreading this, we will now pass validation if the help link is missing, which isn't what we did before. seems like a missing link should fail validation

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, you're right. Will fix.


string? checkError = await checkHelpLinkAsync(helpLinkUri).ConfigureAwait(false);

if (checkError is null)
continue;
}

// The angle brackets around helpLinkUri are added to follow MD034 rule:
// https://github.com/DavidAnson/markdownlint/blob/82cf68023f7dbd2948a65c53fc30482432195de4/doc/Rules.md#md034---bare-url-used
if (!string.IsNullOrWhiteSpace(helpLinkUri))
{
helpLinkUri = $"<{helpLinkUri}>";
}
helpLinkUri = $"<{helpLinkUri}>";

var escapedTitle = descriptor.Title.ToString(CultureInfo.InvariantCulture).Replace("<", "\\<");
var line = $"{ruleId} | {helpLinkUri} | {escapedTitle} |";
Expand All @@ -638,11 +640,13 @@ await checkHelpLinkAsync(helpLinkUri).ConfigureAwait(false))
// However, we consider "missing" entries as invalid. This is to force updating the file when new rules are added.
if (!actualContent.Contains(line))
{
// The file is missing an entry.
await Console.Error.WriteLineAsync($"Missing entry in {fileWithPath}").ConfigureAwait(false);
await Console.Error.WriteLineAsync(line).ConfigureAwait(false);
// The file is missing an entry. Mark it as invalid and break the loop as there is no need to continue validating.
Copy link
Member Author

Choose a reason for hiding this comment

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

Stopping further validation seems odd, because it means you might not necessarily know the extent of a potential problem.

await Console.Error.WriteLineAsync(" " + line).ConfigureAwait(false);
await Console.Error.WriteLineAsync("HTTP error while checking the URI: ").ConfigureAwait(false);
await Console.Error.WriteLineAsync(checkError).ConfigureAwait(false);

fileNamesWithValidationFailures.Add(fileWithPath);
break;
}
}
else
Expand All @@ -658,27 +662,41 @@ await checkHelpLinkAsync(helpLinkUri).ConfigureAwait(false))

return;

async Task<bool> checkHelpLinkAsync(string helpLink)
// Returns null if the URI is valid, or an error otherwise
async Task<string?> checkHelpLinkAsync(string helpLink)
{
try
{
if (!Uri.TryCreate(helpLink, UriKind.Absolute, out var uri))
{
return false;
return null;
}

if (validateOffline)
{
return true;
return null;
}

var request = new HttpRequestMessage(HttpMethod.Head, uri);
using var response = await httpClient.SendAsync(request).ConfigureAwait(false);
return response?.StatusCode == HttpStatusCode.OK;

// If it succeeded, we're good
if (response.StatusCode == HttpStatusCode.OK)
return null;

var errorMessage = new StringBuilder();
errorMessage.AppendLine("StatusCode: " + response.StatusCode);
errorMessage.AppendLine("Content: " + await response.Content.ReadAsStringAsync().ConfigureAwait(false));

foreach (var header in response.Headers)
foreach (var headerValue in header.Value)
errorMessage.AppendLine(header.Key + ": " + headerValue);

return errorMessage.ToString();
}
catch (WebException)
catch (HttpRequestException exception)
{
return false;
return exception.ToString();
}
}
}
Expand Down Expand Up @@ -1180,7 +1198,7 @@ string getRuleActionCore(bool enable, bool enableAsWarning = false)
/// <remarks>
/// Don't call this method with auto-generated files that are part of the artifacts because it's expected that they don't initially exist.
/// </remarks>
private static void Validate(string fileWithPath, string fileContents, List<string> fileNamesWithValidationFailures)
private static void Validate(string fileWithPath, string fileContents, HashSet<string> fileNamesWithValidationFailures)
{
string actual = File.ReadAllText(fileWithPath);
if (actual != fileContents)
Expand Down
Loading