Add analyzer and code fix to recommend against IHeaderDictionary.Add

[david-acker]

Description

• Adds an analyzer which identifies invocations of IHeaderDictionary.Add.
• Adds two code fixes, one for replacing Add with Append and another for replacing Add with the indexer.

Questions

• Is there a preferred architecture or way of organizing new analyzers and code fixes? The analyzers and codes fixes in AspNetCore.Analyzers, Mvc.Analyzers, Mvc.Api.Analyzers, and Framework/AspNetCoreAnalyzers seemed to all have slightly different styles and ways for handling the analyzers, code fixes, tests, etc. I pulled from these when creating this draft, but didn't follow the style of any one of them too rigidly. [Resolved: Moved to Framework/AspNetCoreAnalyzers and used the analyzer and code fix pattern used there.]
• What's the process for picking a rule ID for the new diagnostic? I used ASP0015 as a placeholder for now as this appeared to be the next one available, based on this list of diagnostics: Code analysis in ASP.NET Core apps. [Updated to use ASP0019]

Fixes #41362

[ghost]

Thanks for your PR, @david-acker. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[adityamandaleeka]

@david-acker Can you please put it in src/Framework/AspNetCoreAnalyzers and follow the pattern used there?

As for the ID, for now we're just incrementing the number (see https://github.com/dotnet/aspnetcore/blob/main/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs).

[captainsafia]

Can we add a statement here about the consequences of using IDictionary.Add so the user is more informed?

[david-acker]

I've updated the diagnostic message to the following:

Use IHeaderDictionary.Append or the indexer to append or set headers. IDictionary.Add will throw an ArgumentException when attempting to add a duplicate key.

Let me know if there are any adjustments you'd like me to make to this.

[Youssef1313]

I'm not sure, but could you check if SyntaxFactory.Identifier("Append").WithAdditionalAnnotations(Simplifier.AddImportsAnnotation) will avoid the need to manually add the using?

[david-acker]

Hmm, I just tried this but it doesn't seem to add the using. Is there anything else that I would need to do here besides adding the call to WithAdditionalAnnotations(Simplifier.AddImportsAnnotation) on the identifier?

[Youssef1313]

@david-acker Try getting the symbol for Microsoft.AspNetCore.Http.HeaderDictionaryExtensions and create an annotation like the following:

var annotation = new SyntaxAnnotation("SymbolId", DocumentationCommentId.CreateReferenceId(symbol));

Then use SyntaxFactory.Identifier("Append").WithAdditionalAnnotations(Simplifier.AddImportsAnnotation, annotation)

[captainsafia]

LGTM!

@david-acker You're welcome to handle the docs independently over on that repo.

Consider having a compilation start action when retrieves Microsoft.AspNetCore.Http.IHeaderDictionary and then do a symbol comparison here.

I don't mind doing the symbol comparison this way, but feel free to adjust if you're open to it.

@Youssef1313 Would appreciate your sign off too!

[Youssef1313]

Besides the comments I already added, LGTM.

[david-acker]

@captainsafia @Youssef1313 Sounds good! I'll address the remaining comments within the next few days.

I'll also file an issue in the docs repo and submit a PR to update the diagnostics doc page that's linked above.

[adityamandaleeka]

@david-acker Looks like there's some test failures.

[david-acker]

@adityamandaleeka Hmm, that's odd. I'm not getting those test failures locally. I'll dig into this a bit.

[captainsafia]

@adityamandaleeka Hmm, that's odd. I'm not getting those test failures locally. I'll dig into this a bit.

Seems to be a pesky line-ending issue. I wonder if there is a way to get the verifier to ignore these when comparing texts...

[Youssef1313]

These suggestions simplify the addition of using, but the end of line issue still persists

[Youssef1313]

Suggested change

	using Microsoft.CodeAnalysis.CSharp.Syntax;
	using Microsoft.CodeAnalysis.CSharp.Syntax;
	using Microsoft.CodeAnalysis.Simplification;

[Youssef1313]

Suggested change

	var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false) as CompilationUnitSyntax;
	var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);

[Youssef1313]

Suggested change

	return document.WithSyntaxRoot(
	AddRequiredUsingDirectiveForAppend(root.ReplaceNode(diagnosticTarget, invocation)));
	var model = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false);
	var headerDictionaryExtensionsSymbol = model.Compilation.GetTypeByMetadataName("Microsoft.AspNetCore.Http.HeaderDictionaryExtensions");
	var annotation = new SyntaxAnnotation("SymbolId", DocumentationCommentId.CreateReferenceId(headerDictionaryExtensionsSymbol));
	return document.WithSyntaxRoot(
	root.ReplaceNode(diagnosticTarget, invocation.WithAdditionalAnnotations(Simplifier.AddImportsAnnotation, annotation)));

[Youssef1313]

Suggested change

	private static CompilationUnitSyntax AddRequiredUsingDirectiveForAppend(CompilationUnitSyntax compilationUnitSyntax)
	{
	var usingDirectives = compilationUnitSyntax.Usings;
	var includesRequiredUsingDirective = false;
	var insertionIndex = 0;
	for (var i = 0; i < usingDirectives.Count; i++)
	{
	var namespaceName = usingDirectives[i].Name.ToString();
	// Always insert the new using directive after any 'System' using directives.
	if (namespaceName.StartsWith("System", StringComparison.Ordinal))
	{
	insertionIndex = i + 1;
	continue;
	}
	var result = string.Compare("Microsoft.AspNetCore.Http", namespaceName, StringComparison.Ordinal);
	if (result == 0)
	{
	includesRequiredUsingDirective = true;
	break;
	}
	if (result < 0)
	{
	insertionIndex = i;
	break;
	}
	}
	if (includesRequiredUsingDirective)
	{
	return compilationUnitSyntax;
	}
	var requiredUsingDirective =
	SyntaxFactory.UsingDirective(
	SyntaxFactory.QualifiedName(
	SyntaxFactory.QualifiedName(
	SyntaxFactory.IdentifierName("Microsoft"),
	SyntaxFactory.IdentifierName("AspNetCore")),
	SyntaxFactory.IdentifierName("Http")));
	return compilationUnitSyntax.WithUsings(
	usingDirectives.Insert(insertionIndex, requiredUsingDirective));
	}

[Youssef1313]

@adityamandaleeka Hmm, that's odd. I'm not getting those test failures locally. I'll dig into this a bit.

Seems to be a pesky line-ending issue. I wonder if there is a way to get the verifier to ignore these when comparing texts...

I'm not sure if adding an editorconfig to the tests with end_of_line = crlf can fix the failure (or if it's the correct action).

cc @sharwell @CyrusNajmabadi for suggestions.

[Youssef1313]

@captainsafia Could the failing test be skipped on Linux and get the PR merged?

[captainsafia]

@captainsafia Could the failing test be skipped on Linux and get the PR merged?

I'd like to see if trying

I'm not sure if adding an editorconfig to the tests with end_of_line = crlf can fix the failure (or if it's the correct action).

might help here.

We've had issues before where analyzer tests were not running and resulted in some unsavory unhandled exceptions. Let me see if I can tweak this PR to flow it along.

[BrennanConroy]

Could the failing test be skipped on Linux and get the PR merged?

@david-acker Could you try that out? Apply the OSSkipCondition to the affected tests:

aspnetcore/src/Middleware/Diagnostics.EntityFrameworkCore/test/FunctionalTests/DatabaseErrorPageMiddlewareTest.cs

Line 97 in de3019b

[OSSkipCondition(OperatingSystems.Linux)]

[david-acker]

I updated the failing test to be skipped on Linux and macOS. I also reverted the last commit which added the .editorconfig file.

[captainsafia]

@david-acker Thanks for submitting this PR! Documentation has already been handled in #45025 (comment). Feel free to submit any updates to the docs if you'd like.