diff --git a/src/Microsoft.AspNetCore.Mvc.Analyzers/DiagnosticDescriptors.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/DiagnosticDescriptors.cs index b2684297bd..8b0aa761d1 100644 --- a/src/Microsoft.AspNetCore.Mvc.Analyzers/DiagnosticDescriptors.cs +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/DiagnosticDescriptors.cs @@ -42,5 +42,15 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers "Usage", DiagnosticSeverity.Warning, isEnabledByDefault: true); + + public static readonly DiagnosticDescriptor MVC1004_ParameterNameCollidesWithTopLevelProperty = + new DiagnosticDescriptor( + "MVC1004", + "Rename model bound parameter.", + "Property on type '{0}' has the same name as parameter '{1}'. This may result in incorrect model binding. Consider renaming the parameter or using a model binding attribute to override the name.", + "Naming", + DiagnosticSeverity.Warning, + isEnabledByDefault: true, + helpLinkUri: "https://aka.ms/AA20pbc"); } } diff --git a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/MvcFacts.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/MvcFacts.cs similarity index 98% rename from src/Microsoft.AspNetCore.Mvc.Api.Analyzers/MvcFacts.cs rename to src/Microsoft.AspNetCore.Mvc.Analyzers/MvcFacts.cs index a5dfe71dcc..90651247c4 100644 --- a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/MvcFacts.cs +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/MvcFacts.cs @@ -5,7 +5,7 @@ using System; using System.Diagnostics; using Microsoft.CodeAnalysis; -namespace Microsoft.AspNetCore.Mvc.Api.Analyzers +namespace Microsoft.AspNetCore.Mvc.Analyzers { internal static class MvcFacts { diff --git a/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolNames.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolNames.cs index ca00478262..8448c01337 100644 --- a/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolNames.cs +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolNames.cs @@ -19,10 +19,14 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers public const string AuthorizeAttribute = "Microsoft.AspNetCore.Authorization.AuthorizeAttribute"; + public const string BindAttribute = "Microsoft.AspNetCore.Mvc.BindAttribute"; + public const string ControllerAttribute = "Microsoft.AspNetCore.Mvc.ControllerAttribute"; public const string DefaultStatusCodeAttribute = "Microsoft.AspNetCore.Mvc.Infrastructure.DefaultStatusCodeAttribute"; + public const string FromBodyAttribute = "Microsoft.AspNetCore.Mvc.FromBodyAttribute"; + public const string HtmlHelperPartialExtensionsType = "Microsoft.AspNetCore.Mvc.Rendering.HtmlHelperPartialExtensions"; public const string IApiBehaviorMetadata = "Microsoft.AspNetCore.Mvc.Internal.IApiBehaviorMetadata"; @@ -35,6 +39,8 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers public const string IHtmlHelperType = "Microsoft.AspNetCore.Mvc.Rendering.IHtmlHelper"; + public const string IModelNameProvider = "Microsoft.AspNetCore.Mvc.ModelBinding.IModelNameProvider"; + public const string IRouteTemplateProvider = "Microsoft.AspNetCore.Mvc.Routing.IRouteTemplateProvider"; public const string ModelStateDictionary = "Microsoft.AspNetCore.Mvc.ModelBinding.ModelStateDictionary"; diff --git a/src/Microsoft.AspNetCore.Mvc.Analyzers/TopLevelParameterNameAnalyzer.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/TopLevelParameterNameAnalyzer.cs new file mode 100644 index 0000000000..22f0ba92ba --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/TopLevelParameterNameAnalyzer.cs @@ -0,0 +1,174 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + + +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Microsoft.AspNetCore.Mvc.Analyzers +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public class TopLevelParameterNameAnalyzer : DiagnosticAnalyzer + { + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create( + DiagnosticDescriptors.MVC1004_ParameterNameCollidesWithTopLevelProperty); + + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + + context.RegisterCompilationStartAction(compilationStartAnalysisContext => + { + var typeCache = new SymbolCache(compilationStartAnalysisContext.Compilation); + if (typeCache.ControllerAttribute == null || typeCache.ControllerAttribute.TypeKind == TypeKind.Error) + { + // No-op if we can't find types we care about. + return; + } + + InitializeWorker(compilationStartAnalysisContext, typeCache); + }); + } + + private void InitializeWorker(CompilationStartAnalysisContext compilationStartAnalysisContext, SymbolCache symbolCache) + { + compilationStartAnalysisContext.RegisterSymbolAction(symbolAnalysisContext => + { + var method = (IMethodSymbol)symbolAnalysisContext.Symbol; + if (method.MethodKind != MethodKind.Ordinary) + { + return; + } + + if (method.Parameters.Length == 0) + { + return; + } + + if (!MvcFacts.IsController(method.ContainingType, symbolCache.ControllerAttribute, symbolCache.NonControllerAttribute) || + !MvcFacts.IsControllerAction(method, symbolCache.NonActionAttribute, symbolCache.IDisposableDispose)) + { + return; + } + + if (method.ContainingType.HasAttribute(symbolCache.IApiBehaviorMetadata, inherit: true)) + { + // The issue of parameter name collision with properties affects complex model-bound types + // and not input formatting. Ignore ApiController instances since they default to formatting. + return; + } + + for (var i = 0; i < method.Parameters.Length; i++) + { + var parameter = method.Parameters[i]; + if (IsProblematicParameter(symbolCache, parameter)) + { + var location = parameter.Locations.Length != 0 ? + parameter.Locations[0] : + Location.None; + + symbolAnalysisContext.ReportDiagnostic( + Diagnostic.Create( + DiagnosticDescriptors.MVC1004_ParameterNameCollidesWithTopLevelProperty, + location, + parameter.Type.Name, + parameter.Name)); + } + } + }, SymbolKind.Method); + } + + internal static bool IsProblematicParameter(in SymbolCache symbolCache, IParameterSymbol parameter) + { + if (parameter.GetAttributes(symbolCache.FromBodyAttribute).Any()) + { + // Ignore input formatted parameters. + return false; + } + + var parameterName = GetName(symbolCache, parameter); + + var type = parameter.Type; + while (type != null) + { + foreach (var member in type.GetMembers()) + { + if (member.DeclaredAccessibility != Accessibility.Public || + member.IsStatic || + member.Kind != SymbolKind.Property) + { + continue; + } + + var propertyName = GetName(symbolCache, member); + if (string.Equals(parameterName, propertyName, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + + type = type.BaseType; + } + + return false; + } + + internal static string GetName(in SymbolCache symbolCache, ISymbol symbol) + { + foreach (var attribute in symbol.GetAttributes(symbolCache.IModelNameProvider)) + { + // BindAttribute uses the Prefix property as an alias for IModelNameProvider.Name + var nameProperty = attribute.AttributeClass == symbolCache.BindAttribute ? "Prefix" : "Name"; + + // All of the built-in attributes (FromQueryAttribute, ModelBinderAttribute etc) only support setting the name via + // a property. We'll ignore constructor values. + for (var i = 0; i < attribute.NamedArguments.Length; i++) + { + var namedArgument = attribute.NamedArguments[i]; + var namedArgumentValue = namedArgument.Value; + if (string.Equals(namedArgument.Key, nameProperty, StringComparison.Ordinal) && + namedArgumentValue.Kind == TypedConstantKind.Primitive && + namedArgumentValue.Type.SpecialType == SpecialType.System_String && + namedArgumentValue.Value is string name) + { + return name; + } + } + } + + return symbol.Name; + } + + internal readonly struct SymbolCache + { + public SymbolCache(Compilation compilation) + { + BindAttribute = compilation.GetTypeByMetadataName(SymbolNames.BindAttribute); + ControllerAttribute = compilation.GetTypeByMetadataName(SymbolNames.ControllerAttribute); + FromBodyAttribute = compilation.GetTypeByMetadataName(SymbolNames.FromBodyAttribute); + IApiBehaviorMetadata = compilation.GetTypeByMetadataName(SymbolNames.IApiBehaviorMetadata); + IModelNameProvider = compilation.GetTypeByMetadataName(SymbolNames.IModelNameProvider); + NonControllerAttribute = compilation.GetTypeByMetadataName(SymbolNames.NonControllerAttribute); + NonActionAttribute = compilation.GetTypeByMetadataName(SymbolNames.NonActionAttribute); + + var disposable = compilation.GetSpecialType(SpecialType.System_IDisposable); + var members = disposable.GetMembers(nameof(IDisposable.Dispose)); + IDisposableDispose = members.Length == 1 ? (IMethodSymbol)members[0] : null; + } + + public INamedTypeSymbol BindAttribute { get; } + public INamedTypeSymbol ControllerAttribute { get; } + public INamedTypeSymbol FromBodyAttribute { get; } + public INamedTypeSymbol IApiBehaviorMetadata { get; } + public INamedTypeSymbol IModelNameProvider { get; } + public INamedTypeSymbol NonControllerAttribute { get; } + public INamedTypeSymbol NonActionAttribute { get; } + public IMethodSymbol IDisposableDispose { get; } + } + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiControllerFacts.cs b/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiControllerFacts.cs index b947b8dc5d..84534a5d9f 100644 --- a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiControllerFacts.cs +++ b/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiControllerFacts.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using Microsoft.AspNetCore.Mvc.Analyzers; using Microsoft.CodeAnalysis; namespace Microsoft.AspNetCore.Mvc.Api.Analyzers diff --git a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/Microsoft.AspNetCore.Mvc.Api.Analyzers.csproj b/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/Microsoft.AspNetCore.Mvc.Api.Analyzers.csproj index 5081b0d502..a4c48adc4b 100644 --- a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/Microsoft.AspNetCore.Mvc.Api.Analyzers.csproj +++ b/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/Microsoft.AspNetCore.Mvc.Api.Analyzers.csproj @@ -12,6 +12,7 @@ + diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_ForControllerActionsWithParametersThatMatchProperties.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_ForControllerActionsWithParametersThatMatchProperties.cs new file mode 100644 index 0000000000..85642db52a --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_ForControllerActionsWithParametersThatMatchProperties.cs @@ -0,0 +1,13 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class DiagnosticsAreReturned_ForControllerActionsWithParametersThatMatchProperties : Controller + { + [HttpPost] + public IActionResult EditPerson(DiagnosticsAreReturned_ForControllerActionsWithParametersThatMatchPropertiesModel /*MM*/model) => null; + } + + public class DiagnosticsAreReturned_ForControllerActionsWithParametersThatMatchPropertiesModel + { + public string Model { get; } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_ForModelBoundParameters.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_ForModelBoundParameters.cs new file mode 100644 index 0000000000..be2bc2d7f0 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_ForModelBoundParameters.cs @@ -0,0 +1,17 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class DiagnosticsAreReturned_ForModelBoundParameters : Controller + { + [HttpPost] + public IActionResult EditPerson( + [FromBody] DiagnosticsAreReturned_ForModelBoundParametersModel model, + [FromQuery] DiagnosticsAreReturned_ForModelBoundParametersModel /*MM*/value) => null; + } + + public class DiagnosticsAreReturned_ForModelBoundParametersModel + { + public string Model { get; } + + public string Value { get; } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_IfModelNameProviderIsUsedToModifyParameterName.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_IfModelNameProviderIsUsedToModifyParameterName.cs new file mode 100644 index 0000000000..ed15be578a --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_IfModelNameProviderIsUsedToModifyParameterName.cs @@ -0,0 +1,15 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class DiagnosticsAreReturned_IfModelNameProviderIsUsedToModifyParameterName : Controller + { + [HttpPost] + public IActionResult Edit([ModelBinder(Name = "model")] DiagnosticsAreReturned_IfModelNameProviderIsUsedToModifyParameterNameModel /*MM*/parameter) => null; + } + + public class DiagnosticsAreReturned_IfModelNameProviderIsUsedToModifyParameterNameModel + { + public string Model { get; } + + public string Value { get; } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/GetNameTests.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/GetNameTests.cs new file mode 100644 index 0000000000..2810ab1f49 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/GetNameTests.cs @@ -0,0 +1,13 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class GetNameTests + { + public void NoAttribute(int param) { } + + public void SingleAttribute([ModelBinder(Name = "testModelName")] int param) { } + + public void SingleAttributeWithoutName([ModelBinder] int param) { } + + public void MultipleAttributes([ModelBinder(Name = "name1")][Bind(Prefix = "name2")] int param) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresFields.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresFields.cs new file mode 100644 index 0000000000..e0136bc5f3 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresFields.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_IgnoresFields + { + public string model; + + public void ActionMethod(IsProblematicParameter_IgnoresFields model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresMethods.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresMethods.cs new file mode 100644 index 0000000000..b5f79812d6 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresMethods.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_IgnoresMethods + { + public string Item() => null; + + public void ActionMethod(IsProblematicParameter_IgnoresMethods item) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresNonPublicProperties.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresNonPublicProperties.cs new file mode 100644 index 0000000000..fbfb437f54 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresNonPublicProperties.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_IgnoresNonPublicProperties + { + protected string Model { get; set; } + + public void ActionMethod(IsProblematicParameter_IgnoresNonPublicProperties model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresStaticProperties.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresStaticProperties.cs new file mode 100644 index 0000000000..065fca633f --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresStaticProperties.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_IgnoresStaticProperties + { + public static string Model { get; set; } + + public void ActionMethod(IsProblematicParameter_IgnoresStaticProperties model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForFromBodyParameter.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForFromBodyParameter.cs new file mode 100644 index 0000000000..f1b0011291 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForFromBodyParameter.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsFalse_ForFromBodyParameter + { + public string Model { get; set; } + + public void ActionMethod([FromBody] IsProblematicParameter_ReturnsFalse_ForFromBodyParameter model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter.cs new file mode 100644 index 0000000000..4c83838f02 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter + { + public string Model { get; set; } + + public void ActionMethod([FromRoute(Name = "id")] IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty.cs new file mode 100644 index 0000000000..e4a565bd62 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty.cs @@ -0,0 +1,10 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty + { + [FromQuery(Name = "different")] + public string Model { get; set; } + + public void ActionMethod([Bind(Prefix = nameof(Model))] IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty different) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfParameterNameIsTheSameAsModelProperty.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfParameterNameIsTheSameAsModelProperty.cs new file mode 100644 index 0000000000..adfcf507d0 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfParameterNameIsTheSameAsModelProperty.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsTrue_IfParameterNameIsTheSameAsModelProperty + { + public string Model { get; set; } + + public void ActionMethod(IsProblematicParameter_ReturnsTrue_IfParameterNameIsTheSameAsModelProperty model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfParameterNameWithBinderAttributeIsTheSameNameAsModelProperty.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfParameterNameWithBinderAttributeIsTheSameNameAsModelProperty.cs new file mode 100644 index 0000000000..2b96ecc579 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfParameterNameWithBinderAttributeIsTheSameNameAsModelProperty.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsTrue_IfParameterNameWithBinderAttributeIsTheSameNameAsModelProperty + { + public string Model { get; set; } + + public void ActionMethod([Bind(Prefix = "model")] IsProblematicParameter_ReturnsTrue_IfParameterNameWithBinderAttributeIsTheSameNameAsModelProperty different) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfPropertyWithModelBindingAttributeHasSameNameAsParameter.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfPropertyWithModelBindingAttributeHasSameNameAsParameter.cs new file mode 100644 index 0000000000..7f3d9c499d --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfPropertyWithModelBindingAttributeHasSameNameAsParameter.cs @@ -0,0 +1,10 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsTrue_IfPropertyWithModelBindingAttributeHasSameNameAsParameter + { + [ModelBinder(typeof(object), Name = "model")] + public string Different { get; set; } + + public void ActionMethod(IsProblematicParameter_ReturnsTrue_IfPropertyWithModelBindingAttributeHasSameNameAsParameter model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedForApiControllers.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedForApiControllers.cs new file mode 100644 index 0000000000..3be2d2a07a --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedForApiControllers.cs @@ -0,0 +1,14 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + [ApiController] + public class NoDiagnosticsAreReturnedForApiControllers : Controller + { + [HttpPost] + public IActionResult EditPerson(NoDiagnosticsAreReturnedForApiControllersModel model) => null; + } + + public class NoDiagnosticsAreReturnedForApiControllersModel + { + public string Model { get; } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedForNonActions.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedForNonActions.cs new file mode 100644 index 0000000000..d6e7edce31 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedForNonActions.cs @@ -0,0 +1,13 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class NoDiagnosticsAreReturnedForNonActions : Controller + { + [NonAction] + public IActionResult EditPerson(NoDiagnosticsAreReturnedForNonActionsModel model) => null; + } + + public class NoDiagnosticsAreReturnedForNonActionsModel + { + public string Model { get; } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedIfParameterIsRenamedUsingBindingAttribute.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedIfParameterIsRenamedUsingBindingAttribute.cs new file mode 100644 index 0000000000..e870af3704 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedIfParameterIsRenamedUsingBindingAttribute.cs @@ -0,0 +1,13 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class NoDiagnosticsAreReturnedIfParameterIsRenamedUsingBindingAttribute : Controller + { + [HttpPost] + public IActionResult EditPerson([FromForm(Name = "")] NoDiagnosticsAreReturnedIfParameterIsRenamedUsingBindingAttributeModel model) => null; + } + + public class NoDiagnosticsAreReturnedIfParameterIsRenamedUsingBindingAttributeModel + { + public string Model { get; } + } +} diff --git a/test/Mvc.Analyzers.Test/TopLevelParameterNameAnalyzerTest.cs b/test/Mvc.Analyzers.Test/TopLevelParameterNameAnalyzerTest.cs new file mode 100644 index 0000000000..e5add91ee2 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TopLevelParameterNameAnalyzerTest.cs @@ -0,0 +1,237 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Linq; +using System.Runtime.CompilerServices; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Analyzer.Testing; +using Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles; +using Microsoft.CodeAnalysis; +using Xunit; + +namespace Microsoft.AspNetCore.Mvc.Analyzers +{ + public class TopLevelParameterNameAnalyzerTest + { + private MvcDiagnosticAnalyzerRunner Runner { get; } = new MvcDiagnosticAnalyzerRunner(new TopLevelParameterNameAnalyzer()); + + [Fact] + public Task DiagnosticsAreReturned_ForControllerActionsWithParametersThatMatchProperties() + => RunTest(nameof(DiagnosticsAreReturned_ForControllerActionsWithParametersThatMatchPropertiesModel), "model"); + + [Fact] + public Task DiagnosticsAreReturned_ForModelBoundParameters() + => RunTest(nameof(DiagnosticsAreReturned_ForModelBoundParametersModel), "value"); + + [Fact] + public Task DiagnosticsAreReturned_IfModelNameProviderIsUsedToModifyParameterName() + => RunTest(nameof(DiagnosticsAreReturned_IfModelNameProviderIsUsedToModifyParameterNameModel), "parameter"); + + [Fact] + public Task NoDiagnosticsAreReturnedForApiControllers() + => RunNoDiagnosticsAreReturned(); + + [Fact] + public Task NoDiagnosticsAreReturnedIfParameterIsRenamedUsingBindingAttribute() + => RunNoDiagnosticsAreReturned(); + + [Fact] + public Task NoDiagnosticsAreReturnedForNonActions() + => RunNoDiagnosticsAreReturned(); + + [Fact] + public async Task IsProblematicParameter_ReturnsTrue_IfParameterNameIsTheSameAsModelProperty() + { + var result = await IsProblematicParameterTest(); + Assert.True(result); + } + + [Fact] + public async Task IsProblematicParameter_ReturnsTrue_IfParameterNameWithBinderAttributeIsTheSameNameAsModelProperty() + { + var result = await IsProblematicParameterTest(); + Assert.True(result); + } + + [Fact] + public async Task IsProblematicParameter_ReturnsTrue_IfPropertyWithModelBindingAttributeHasSameNameAsParameter() + { + var result = await IsProblematicParameterTest(); + Assert.True(result); + } + + [Fact] + public async Task IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter() + { + var result = await IsProblematicParameterTest(); + Assert.False(result); + } + + [Fact] + public async Task IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty() + { + var result = await IsProblematicParameterTest(); + Assert.False(result); + } + + [Fact] + public async Task IsProblematicParameter_ReturnsFalse_ForFromBodyParameter() + { + var result = await IsProblematicParameterTest(); + Assert.False(result); + } + + [Fact] + public async Task IsProblematicParameter_IgnoresStaticProperties() + { + var result = await IsProblematicParameterTest(); + Assert.False(result); + } + + [Fact] + public async Task IsProblematicParameter_IgnoresFields() + { + var result = await IsProblematicParameterTest(); + Assert.False(result); + } + + [Fact] + public async Task IsProblematicParameter_IgnoresMethods() + { + var result = await IsProblematicParameterTest(); + Assert.False(result); + } + + [Fact] + public async Task IsProblematicParameter_IgnoresNonPublicProperties() + { + var result = await IsProblematicParameterTest(); + Assert.False(result); + } + + private async Task IsProblematicParameterTest([CallerMemberName] string testMethod = "") + { + var testSource = MvcTestSource.Read(GetType().Name, testMethod); + var project = DiagnosticProject.Create(GetType().Assembly, new[] { testSource.Source }); + + var compilation = await project.GetCompilationAsync(); + + var modelType = compilation.GetTypeByMetadataName($"Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles.{testMethod}"); + var method = (IMethodSymbol)modelType.GetMembers("ActionMethod").First(); + var parameter = method.Parameters[0]; + + var symbolCache = new TopLevelParameterNameAnalyzer.SymbolCache(compilation); + + var result = TopLevelParameterNameAnalyzer.IsProblematicParameter(symbolCache, parameter); + return result; + } + + [Fact] + public async Task GetName_ReturnsValueFromFirstAttributeWithValue() + { + var methodName = nameof(GetNameTests.SingleAttribute); + var compilation = await GetCompilationForGetName(); + var symbolCache = new TopLevelParameterNameAnalyzer.SymbolCache(compilation); + + var type = compilation.GetTypeByMetadataName(typeof(GetNameTests).FullName); + var method = (IMethodSymbol)type.GetMembers(methodName).First(); + + var parameter = method.Parameters[0]; + var name = TopLevelParameterNameAnalyzer.GetName(symbolCache, parameter); + + Assert.Equal("testModelName", name); + } + + [Fact] + public async Task GetName_ReturnsName_IfNoAttributesAreSpecified() + { + var methodName = nameof(GetNameTests.NoAttribute); + var compilation = await GetCompilationForGetName(); + var symbolCache = new TopLevelParameterNameAnalyzer.SymbolCache(compilation); + + var type = compilation.GetTypeByMetadataName(typeof(GetNameTests).FullName); + var method = (IMethodSymbol)type.GetMembers(methodName).First(); + + var parameter = method.Parameters[0]; + var name = TopLevelParameterNameAnalyzer.GetName(symbolCache, parameter); + + Assert.Equal("param", name); + } + + [Fact] + public async Task GetName_ReturnsName_IfAttributeDoesNotSpecifyName() + { + var methodName = nameof(GetNameTests.SingleAttributeWithoutName); + var compilation = await GetCompilationForGetName(); + var symbolCache = new TopLevelParameterNameAnalyzer.SymbolCache(compilation); + + var type = compilation.GetTypeByMetadataName(typeof(GetNameTests).FullName); + var method = (IMethodSymbol)type.GetMembers(methodName).First(); + + var parameter = method.Parameters[0]; + var name = TopLevelParameterNameAnalyzer.GetName(symbolCache, parameter); + + Assert.Equal("param", name); + } + + [Fact] + public async Task GetName_ReturnsFirstName_IfMultipleAttributesAreSpecified() + { + var methodName = nameof(GetNameTests.MultipleAttributes); + var compilation = await GetCompilationForGetName(); + var symbolCache = new TopLevelParameterNameAnalyzer.SymbolCache(compilation); + + var type = compilation.GetTypeByMetadataName(typeof(GetNameTests).FullName); + var method = (IMethodSymbol)type.GetMembers(methodName).First(); + + var parameter = method.Parameters[0]; + var name = TopLevelParameterNameAnalyzer.GetName(symbolCache, parameter); + + Assert.Equal("name1", name); + } + + private async Task GetCompilationForGetName() + { + var testSource = MvcTestSource.Read(GetType().Name, "GetNameTests"); + var project = DiagnosticProject.Create(GetType().Assembly, new[] { testSource.Source }); + + var compilation = await project.GetCompilationAsync(); + return compilation; + } + + private async Task RunNoDiagnosticsAreReturned([CallerMemberName] string testMethod = "") + { + // Arrange + var testSource = MvcTestSource.Read(GetType().Name, testMethod); + var expectedLocation = testSource.DefaultMarkerLocation; + + // Act + var result = await Runner.GetDiagnosticsAsync(testSource.Source); + + // Assert + Assert.Empty(result); + } + + private async Task RunTest(string typeName, string parameterName, [CallerMemberName] string testMethod = "") + { + // Arrange + var descriptor = DiagnosticDescriptors.MVC1004_ParameterNameCollidesWithTopLevelProperty; + var testSource = MvcTestSource.Read(GetType().Name, testMethod); + var expectedLocation = testSource.DefaultMarkerLocation; + + // Act + var result = await Runner.GetDiagnosticsAsync(testSource.Source); + + // Assert + Assert.Collection( + result, + diagnostic => + { + Assert.Equal(descriptor.Id, diagnostic.Id); + Assert.Same(descriptor, diagnostic.Descriptor); + AnalyzerAssert.DiagnosticLocation(expectedLocation, diagnostic.Location); + Assert.Equal(string.Format(descriptor.MessageFormat.ToString(), typeName, parameterName), diagnostic.GetMessage()); + }); + } + } +} diff --git a/test/Mvc.Api.Analyzers.Test/MvcFactsTest.cs b/test/Mvc.Api.Analyzers.Test/MvcFactsTest.cs index e6009e7125..0544262951 100644 --- a/test/Mvc.Api.Analyzers.Test/MvcFactsTest.cs +++ b/test/Mvc.Api.Analyzers.Test/MvcFactsTest.cs @@ -5,6 +5,7 @@ using System; using System.Linq; using System.Threading.Tasks; using Microsoft.AspNetCore.Analyzer.Testing; +using Microsoft.AspNetCore.Mvc.Analyzers; using Microsoft.CodeAnalysis; using Xunit;