diff --git a/src/Analyzers/Analyzers/test/Microsoft.AspNetCore.Analyzers.Test.csproj b/src/Analyzers/Analyzers/test/Microsoft.AspNetCore.Analyzers.Test.csproj index 9ef68dad7e29..5c89ff96a301 100644 --- a/src/Analyzers/Analyzers/test/Microsoft.AspNetCore.Analyzers.Test.csproj +++ b/src/Analyzers/Analyzers/test/Microsoft.AspNetCore.Analyzers.Test.csproj @@ -1,4 +1,4 @@ - + $(DefaultNetCoreTargetFramework) @@ -8,7 +8,6 @@ - diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs index d9b830b01e3b..bb01a4919baa 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs @@ -151,4 +151,13 @@ internal static class DiagnosticDescriptors DiagnosticSeverity.Info, isEnabledByDefault: true, helpLinkUri: "https://aka.ms/aspnet/analyzers"); + + internal static readonly DiagnosticDescriptor DoNotUseIHeaderDictionaryAdd = new( + "ASP0019", + new LocalizableResourceString(nameof(Resources.Analyzer_HeaderDictionaryAdd_Title), Resources.ResourceManager, typeof(Resources)), + new LocalizableResourceString(nameof(Resources.Analyzer_HeaderDictionaryAdd_Message), Resources.ResourceManager, typeof(Resources)), + "Usage", + DiagnosticSeverity.Warning, + isEnabledByDefault: true, + helpLinkUri: "https://aka.ms/aspnet/analyzers"); } diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Http/HeaderDictionaryAddAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Http/HeaderDictionaryAddAnalyzer.cs new file mode 100644 index 000000000000..989bb538ade8 --- /dev/null +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Http/HeaderDictionaryAddAnalyzer.cs @@ -0,0 +1,92 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; + +namespace Microsoft.AspNetCore.Analyzers.Http; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class HeaderDictionaryAddAnalyzer : DiagnosticAnalyzer +{ + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(DiagnosticDescriptors.DoNotUseIHeaderDictionaryAdd); + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterCompilationStartAction(OnCompilationStart); + } + + private static void OnCompilationStart(CompilationStartAnalysisContext context) + { + var symbols = new HeaderDictionarySymbols(context.Compilation); + + if (!symbols.HasRequiredSymbols) + { + return; + } + + context.RegisterOperationAction(context => + { + var invocation = (IInvocationOperation)context.Operation; + + if (SymbolEqualityComparer.Default.Equals(symbols.IHeaderDictionary, invocation.Instance?.Type) + && IsAddMethod(invocation.TargetMethod) + && invocation.TargetMethod.Parameters.Length == 2) + { + AddDiagnosticWarning(context, invocation.Syntax.GetLocation()); + } + + }, OperationKind.Invocation); + } + + private static bool IsAddMethod(IMethodSymbol method) + { + return method is + { + Name: "Add", + ContainingType: + { + Name: "IDictionary", + ContainingNamespace: + { + Name: "Generic", + ContainingNamespace: + { + Name: "Collections", + ContainingNamespace: + { + Name: "System", + ContainingNamespace: + { + IsGlobalNamespace: true + } + } + } + } + } + }; + } + + private static void AddDiagnosticWarning(OperationAnalysisContext context, Location location) + { + context.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.DoNotUseIHeaderDictionaryAdd, + location)); + } + + private sealed class HeaderDictionarySymbols + { + public HeaderDictionarySymbols(Compilation compilation) + { + IHeaderDictionary = compilation.GetTypeByMetadataName("Microsoft.AspNetCore.Http.IHeaderDictionary"); + } + + public bool HasRequiredSymbols => IHeaderDictionary is not null; + + public INamedTypeSymbol IHeaderDictionary { get; } + } +} diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Resources.resx b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Resources.resx index 09b7e852e242..7b858c8e70df 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Resources.resx +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Resources.resx @@ -201,4 +201,10 @@ Unused route parameter + + Use IHeaderDictionary.Append or the indexer to append or set headers. IDictionary.Add will throw an ArgumentException when attempting to add a duplicate key. + + + Suggest using IHeaderDictionary.Append or the indexer + \ No newline at end of file diff --git a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Http/HeaderDictionaryAddFixer.cs b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Http/HeaderDictionaryAddFixer.cs new file mode 100644 index 000000000000..08826af2b0e7 --- /dev/null +++ b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Http/HeaderDictionaryAddFixer.cs @@ -0,0 +1,129 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Immutable; +using System.Composition; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Simplification; + +namespace Microsoft.AspNetCore.Analyzers.Http.Fixers; + +[ExportCodeFixProvider(LanguageNames.CSharp), Shared] +public sealed class HeaderDictionaryAddFixer : CodeFixProvider +{ + public override ImmutableArray FixableDiagnosticIds { get; } = ImmutableArray.Create(DiagnosticDescriptors.DoNotUseIHeaderDictionaryAdd.Id); + + public sealed override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public sealed override Task RegisterCodeFixesAsync(CodeFixContext context) + { + foreach (var diagnostic in context.Diagnostics) + { + context.Document.TryGetSyntaxRoot(out var root); + + if (CanReplaceWithAppend(diagnostic, root, out var invocation)) + { + var appendTitle = "Use 'IHeaderDictionary.Append'"; + context.RegisterCodeFix( + CodeAction.Create(appendTitle, + cancellationToken => ReplaceWithAppend(diagnostic, context.Document, invocation, cancellationToken), + equivalenceKey: appendTitle), + diagnostic); + } + + if (CanReplaceWithIndexer(diagnostic, root, out var assignment)) + { + var indexerTitle = "Use indexer"; + context.RegisterCodeFix( + CodeAction.Create(indexerTitle, + cancellationToken => ReplaceWithIndexer(diagnostic, context.Document, assignment, cancellationToken), + equivalenceKey: indexerTitle), + diagnostic); + } + } + + return Task.CompletedTask; + } + + private static async Task ReplaceWithAppend(Diagnostic diagnostic, Document document, InvocationExpressionSyntax invocation, CancellationToken cancellationToken) + { + var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); + + var diagnosticTarget = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true); + + 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))); + } + + private static bool CanReplaceWithAppend(Diagnostic diagnostic, SyntaxNode root, out InvocationExpressionSyntax invocation) + { + invocation = null; + + if (root is not CompilationUnitSyntax) + { + return false; + } + + var diagnosticTarget = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true); + + if (diagnosticTarget is InvocationExpressionSyntax { Expression: MemberAccessExpressionSyntax { Name.Identifier: { } identifierToken } } invocationExpression) + { + invocation = invocationExpression.ReplaceToken(identifierToken, SyntaxFactory.Identifier("Append")); + + return true; + } + + return false; + } + + private static async Task ReplaceWithIndexer(Diagnostic diagnostic, Document document, AssignmentExpressionSyntax assignment, CancellationToken cancellationToken) + { + var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); + + var diagnosticTarget = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true); + + return document.WithSyntaxRoot(root.ReplaceNode(diagnosticTarget, assignment)); + } + + private static bool CanReplaceWithIndexer(Diagnostic diagnostic, SyntaxNode root, out AssignmentExpressionSyntax assignment) + { + assignment = null; + + if (root is null) + { + return false; + } + + var diagnosticTarget = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true); + + if (diagnosticTarget is InvocationExpressionSyntax + { + Expression: MemberAccessExpressionSyntax memberAccessExpression, + ArgumentList.Arguments: { Count: 2 } arguments + }) + { + assignment = + SyntaxFactory.AssignmentExpression( + SyntaxKind.SimpleAssignmentExpression, + SyntaxFactory.ElementAccessExpression( + memberAccessExpression.Expression, + SyntaxFactory.BracketedArgumentList( + SyntaxFactory.SeparatedList(new[] { arguments[0] }))), + arguments[1].Expression); + + return true; + } + + return false; + } +} diff --git a/src/Framework/AspNetCoreAnalyzers/test/Http/HeaderDictionaryAddTest.cs b/src/Framework/AspNetCoreAnalyzers/test/Http/HeaderDictionaryAddTest.cs new file mode 100644 index 000000000000..5eaa129e66fe --- /dev/null +++ b/src/Framework/AspNetCoreAnalyzers/test/Http/HeaderDictionaryAddTest.cs @@ -0,0 +1,272 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.AspNetCore.Testing; +using Microsoft.CodeAnalysis.Testing; +using VerifyCS = Microsoft.AspNetCore.Analyzers.Verifiers.CSharpCodeFixVerifier< + Microsoft.AspNetCore.Analyzers.Http.HeaderDictionaryAddAnalyzer, + Microsoft.AspNetCore.Analyzers.Http.Fixers.HeaderDictionaryAddFixer>; + +namespace Microsoft.AspNetCore.Analyzers.Http; + +public class HeaderDictionaryAddTest +{ + private const string AppendCodeActionEquivalenceKey = "Use 'IHeaderDictionary.Append'"; + private const string IndexerCodeActionEquivalenceKey = "Use indexer"; + + public static TheoryData FixedWithAppendTestData => new() + { + // Single diagnostic + { + @" +using Microsoft.AspNetCore.Http; + +var context = new DefaultHttpContext(); +{|#0:context.Request.Headers.Add(""Accept"", ""text/html"")|};", + new[] + { + new DiagnosticResult(DiagnosticDescriptors.DoNotUseIHeaderDictionaryAdd) + .WithLocation(0) + .WithMessage(Resources.Analyzer_HeaderDictionaryAdd_Message) + }, + @" +using Microsoft.AspNetCore.Http; + +var context = new DefaultHttpContext(); +context.Request.Headers.Append(""Accept"", ""text/html"");" + }, + + // Multiple diagnostics + { + @" +using Microsoft.AspNetCore.Http; + +var context = new DefaultHttpContext(); +{|#0:context.Request.Headers.Add(""Accept"", ""text/html"")|}; +{|#1:context.Request.Headers.Add(""Accept"", ""text/html"")|};", + new[] + { + new DiagnosticResult(DiagnosticDescriptors.DoNotUseIHeaderDictionaryAdd) + .WithLocation(0) + .WithMessage(Resources.Analyzer_HeaderDictionaryAdd_Message), + new DiagnosticResult(DiagnosticDescriptors.DoNotUseIHeaderDictionaryAdd) + .WithLocation(1) + .WithMessage(Resources.Analyzer_HeaderDictionaryAdd_Message) + }, + @" +using Microsoft.AspNetCore.Http; + +var context = new DefaultHttpContext(); +context.Request.Headers.Append(""Accept"", ""text/html""); +context.Request.Headers.Append(""Accept"", ""text/html"");" + }, + + // Missing semicolon + { + @" +using Microsoft.AspNetCore.Http; + +var context = new DefaultHttpContext(); +{|#0:context.Request.Headers.Add(""Accept"", ""text/html"")|}{|CS1002:|}", + new[] + { + new DiagnosticResult(DiagnosticDescriptors.DoNotUseIHeaderDictionaryAdd) + .WithLocation(0) + .WithMessage(Resources.Analyzer_HeaderDictionaryAdd_Message) + }, + @" +using Microsoft.AspNetCore.Http; + +var context = new DefaultHttpContext(); +context.Request.Headers.Append(""Accept"", ""text/html""){|CS1002:|}" + } + }; + + [Theory] + [MemberData(nameof(FixedWithAppendTestData))] + public async Task IHeaderDictionary_WithAdd_FixedWithAppend(string source, DiagnosticResult[] expectedDiagnostics, string fixedSource) + { + // Arrange & Act & Assert + await VerifyCS.VerifyCodeFixAsync(source, expectedDiagnostics, fixedSource, codeActionEquivalenceKey: AppendCodeActionEquivalenceKey); + } + + public static IEnumerable FixedWithAppendAddsUsingDirectiveTestData() + { + // No existing using directives + yield return new[] +{ + @" +var context = new Microsoft.AspNetCore.Http.DefaultHttpContext(); +{|#0:context.Request.Headers.Add(""Accept"", ""text/html"")|};", + + @" +using Microsoft.AspNetCore.Http; + +var context = new Microsoft.AspNetCore.Http.DefaultHttpContext(); +context.Request.Headers.Append(""Accept"", ""text/html"");" + }; + + // Inserted alphabetically based on existing using directives + yield return new[] + { + @" +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Mvc; + +var context = new Microsoft.AspNetCore.Http.DefaultHttpContext(); +{|#0:context.Request.Headers.Add(""Accept"", ""text/html"")|};", + + @" +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; + +var context = new Microsoft.AspNetCore.Http.DefaultHttpContext(); +context.Request.Headers.Append(""Accept"", ""text/html"");" + }; + + // Inserted after 'System' using directives + yield return new[] + { + @" +using System.Collections.Generic; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Mvc; + +var context = new Microsoft.AspNetCore.Http.DefaultHttpContext(); +{|#0:context.Request.Headers.Add(""Accept"", ""text/html"")|};", + + @" +using System.Collections.Generic; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; + +var context = new Microsoft.AspNetCore.Http.DefaultHttpContext(); +context.Request.Headers.Append(""Accept"", ""text/html"");" + }; + } + + [ConditionalTheory] + [OSSkipCondition(OperatingSystems.Linux)] + [OSSkipCondition(OperatingSystems.MacOSX)] + [MemberData(nameof(FixedWithAppendAddsUsingDirectiveTestData))] + public async Task IHeaderDictionary_WithAdd_FixedWithAppend_AddsUsingDirective(string source, string fixedSource) + { + // Arrange & Act & Assert + await VerifyCS.VerifyCodeFixAsync( + source.TrimStart(), + new[] + { + new DiagnosticResult(DiagnosticDescriptors.DoNotUseIHeaderDictionaryAdd) + .WithLocation(0) + .WithMessage(Resources.Analyzer_HeaderDictionaryAdd_Message) + }, + fixedSource.TrimStart(), + codeActionEquivalenceKey: AppendCodeActionEquivalenceKey); + } + + public static TheoryData FixedWithIndexerTestData => new() + { + // Single diagnostic + { + @" +using Microsoft.AspNetCore.Http; + +var context = new DefaultHttpContext(); +{|#0:context.Request.Headers.Add(""Accept"", ""text/html"")|};", + new[] + { + new DiagnosticResult(DiagnosticDescriptors.DoNotUseIHeaderDictionaryAdd) + .WithLocation(0) + .WithMessage(Resources.Analyzer_HeaderDictionaryAdd_Message) + }, + @" +using Microsoft.AspNetCore.Http; + +var context = new DefaultHttpContext(); +context.Request.Headers[""Accept""] = ""text/html"";" + }, + + // Multiple diagnostics + { + @" +using Microsoft.AspNetCore.Http; + +var context = new DefaultHttpContext(); +{|#0:context.Request.Headers.Add(""Accept"", ""text/html"")|}; +{|#1:context.Request.Headers.Add(""Accept"", ""text/html"")|};", + new[] + { + new DiagnosticResult(DiagnosticDescriptors.DoNotUseIHeaderDictionaryAdd) + .WithLocation(0) + .WithMessage(Resources.Analyzer_HeaderDictionaryAdd_Message), + new DiagnosticResult(DiagnosticDescriptors.DoNotUseIHeaderDictionaryAdd) + .WithLocation(1) + .WithMessage(Resources.Analyzer_HeaderDictionaryAdd_Message) + }, + @" +using Microsoft.AspNetCore.Http; + +var context = new DefaultHttpContext(); +context.Request.Headers[""Accept""] = ""text/html""; +context.Request.Headers[""Accept""] = ""text/html"";" + }, + + // Missing semicolon + { + @" +using Microsoft.AspNetCore.Http; + +var context = new DefaultHttpContext(); +{|#0:context.Request.Headers.Add(""Accept"", ""text/html"")|}{|CS1002:|}", + new[] + { + new DiagnosticResult(DiagnosticDescriptors.DoNotUseIHeaderDictionaryAdd) + .WithLocation(0) + .WithMessage(Resources.Analyzer_HeaderDictionaryAdd_Message) + }, + @" +using Microsoft.AspNetCore.Http; + +var context = new DefaultHttpContext(); +context.Request.Headers[""Accept""] = ""text/html""{|CS1002:|}" + } + }; + + [Theory] + [MemberData(nameof(FixedWithIndexerTestData))] + public async Task IHeaderDictionary_WithAdd_FixedWithIndexer(string source, DiagnosticResult[] expectedDiagnostics, string fixedSource) + { + // Arrange & Act & Assert + await VerifyCS.VerifyCodeFixAsync(source, expectedDiagnostics, fixedSource, codeActionEquivalenceKey: IndexerCodeActionEquivalenceKey); + } + + [Fact] + public async Task IHeaderDictionary_WithAppend_DoesNotProduceDiagnostics() + { + // Arrange + var source = @" +using Microsoft.AspNetCore.Http; + +var context = new DefaultHttpContext(); +context.Request.Headers.Append(""Accept"", ""text/html"");"; + + // Act & Assert + await VerifyCS.VerifyCodeFixAsync(source, source); + } + + [Fact] + public async Task IHeaderDictionary_WithIndexer_DoesNotProduceDiagnostics() + { + // Arrange + var source = @" +using Microsoft.AspNetCore.Http; + +var context = new DefaultHttpContext(); +context.Request.Headers[""Accept""] = ""text/html"";"; + + // Act & Assert + await VerifyCS.VerifyCodeFixAsync(source, source); + } +} diff --git a/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpCodeFixVerifier.cs b/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpCodeFixVerifier.cs index 1a0bd0874dbb..124b40083885 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpCodeFixVerifier.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpCodeFixVerifier.cs @@ -52,7 +52,7 @@ public static async Task VerifyCodeFixAsync(string source, DiagnosticResult expe => await VerifyCodeFixAsync(source, new[] { expected }, fixedSource); /// - public static async Task VerifyCodeFixAsync(string source, DiagnosticResult[] expected, string fixedSource, int? expectedIterations = null, string usageSource = null) + public static async Task VerifyCodeFixAsync(string source, DiagnosticResult[] expected, string fixedSource, int? expectedIterations = null, string usageSource = null, string codeActionEquivalenceKey = null) { var test = new CSharpCodeFixTest { @@ -63,6 +63,7 @@ public static async Task VerifyCodeFixAsync(string source, DiagnosticResult[] ex FixedState = { Sources = { fixedSource } }, ReferenceAssemblies = CSharpAnalyzerVerifier.GetReferenceAssemblies(), NumberOfFixAllIterations = expectedIterations, + CodeActionEquivalenceKey = codeActionEquivalenceKey }; if (usageSource != null) diff --git a/src/Grpc/JsonTranscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.Tests/JsonTranscodingServerCallContextTests.cs b/src/Grpc/JsonTranscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.Tests/JsonTranscodingServerCallContextTests.cs index 57e541206348..c2ae97a78a1e 100644 --- a/src/Grpc/JsonTranscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.Tests/JsonTranscodingServerCallContextTests.cs +++ b/src/Grpc/JsonTranscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.Tests/JsonTranscodingServerCallContextTests.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Net; -using Google.Protobuf.Reflection; using Grpc.AspNetCore.Server; using Grpc.Core; using Grpc.Shared; @@ -38,11 +37,11 @@ public void RequestHeaders_Get_PopulatedFromHttpContext() { // Arrange var httpContext = CreateHttpContext(); - httpContext.Request.Headers.Add("TestName", "TestValue"); - httpContext.Request.Headers.Add(":method", "GET"); - httpContext.Request.Headers.Add("grpc-encoding", "identity"); - httpContext.Request.Headers.Add("grpc-timeout", "1S"); - httpContext.Request.Headers.Add("hello-bin", Convert.ToBase64String(new byte[] { 1, 2, 3 })); + httpContext.Request.Headers.Append("TestName", "TestValue"); + httpContext.Request.Headers.Append(":method", "GET"); + httpContext.Request.Headers.Append("grpc-encoding", "identity"); + httpContext.Request.Headers.Append("grpc-timeout", "1S"); + httpContext.Request.Headers.Append("hello-bin", Convert.ToBase64String(new byte[] { 1, 2, 3 })); var serverCallContext = CreateServerCallContext(httpContext); // Act