From af770ede876670abd37aa5a034c3d82730a12746 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Wed, 15 Aug 2018 11:13:05 -0700 Subject: [PATCH] Ignore parameters that specify a model binder type --- .../SymbolNames.cs | 2 + .../TopLevelParameterNameAnalyzer.cs | 41 +++++++++++ ...alse_ForParametersWithCustomModelBinder.cs | 9 +++ ...SourceAttributeIsUsedToRenameParameter.cs} | 4 +- ...ngSourceAttributeIsUsedToRenameProperty.cs | 10 +++ ...elBinderAttributeIsUsedToRenameProperty.cs | 10 --- ...lBinderAttributeIsUsedToRenameParameter.cs | 9 +++ .../SpecifiesModelTypeTests.cs | 11 +++ .../TopLevelParameterNameAnalyzerTest.cs | 72 ++++++++++++++++++- 9 files changed, 154 insertions(+), 14 deletions(-) create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForParametersWithCustomModelBinder.cs rename test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/{IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter.cs => IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameParameter.cs} (52%) create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameProperty.cs delete mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfModelBinderAttributeIsUsedToRenameParameter.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/SpecifiesModelTypeTests.cs diff --git a/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolNames.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolNames.cs index 8448c01337..ad152657df 100644 --- a/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolNames.cs +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolNames.cs @@ -31,6 +31,8 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers public const string IApiBehaviorMetadata = "Microsoft.AspNetCore.Mvc.Internal.IApiBehaviorMetadata"; + public const string IBinderTypeProviderMetadata = "Microsoft.AspNetCore.Mvc.ModelBinding.IBinderTypeProviderMetadata"; + public const string IActionResult = "Microsoft.AspNetCore.Mvc.IActionResult"; public const string IConvertToActionResult = "Microsoft.AspNetCore.Mvc.IConvertToActionResult"; diff --git a/src/Microsoft.AspNetCore.Mvc.Analyzers/TopLevelParameterNameAnalyzer.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/TopLevelParameterNameAnalyzer.cs index 22f0ba92ba..ad5aeeaeab 100644 --- a/src/Microsoft.AspNetCore.Mvc.Analyzers/TopLevelParameterNameAnalyzer.cs +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/TopLevelParameterNameAnalyzer.cs @@ -91,6 +91,12 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers return false; } + if (SpecifiesModelType(symbolCache, parameter)) + { + // Ignore parameters that specify a model type. + return false; + } + var parameterName = GetName(symbolCache, parameter); var type = parameter.Type; @@ -144,6 +150,39 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers return symbol.Name; } + internal static bool SpecifiesModelType(in SymbolCache symbolCache, IParameterSymbol parameterSymbol) + { + foreach (var attribute in parameterSymbol.GetAttributes(symbolCache.IBinderTypeProviderMetadata)) + { + // Look for a attribute property named BinderType being assigned. This would match + // [ModelBinder(BinderType = typeof(SomeBinder))] + for (var i = 0; i < attribute.NamedArguments.Length; i++) + { + var namedArgument = attribute.NamedArguments[i]; + var namedArgumentValue = namedArgument.Value; + if (string.Equals(namedArgument.Key, "BinderType", StringComparison.Ordinal) && + namedArgumentValue.Kind == TypedConstantKind.Type) + { + return true; + } + } + + // Look for the binder type being specified in the constructor. This would match + // [ModelBinder(typeof(SomeBinder))] + var constructorParameters = attribute.AttributeConstructor?.Parameters ?? ImmutableArray.Empty; + for (var i = 0; i < constructorParameters.Length; i++) + { + if (string.Equals(constructorParameters[i].Name, "binderType", StringComparison.Ordinal)) + { + // A constructor that requires binderType was used. + return true; + } + } + } + + return false; + } + internal readonly struct SymbolCache { public SymbolCache(Compilation compilation) @@ -152,6 +191,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers ControllerAttribute = compilation.GetTypeByMetadataName(SymbolNames.ControllerAttribute); FromBodyAttribute = compilation.GetTypeByMetadataName(SymbolNames.FromBodyAttribute); IApiBehaviorMetadata = compilation.GetTypeByMetadataName(SymbolNames.IApiBehaviorMetadata); + IBinderTypeProviderMetadata = compilation.GetTypeByMetadataName(SymbolNames.IBinderTypeProviderMetadata); IModelNameProvider = compilation.GetTypeByMetadataName(SymbolNames.IModelNameProvider); NonControllerAttribute = compilation.GetTypeByMetadataName(SymbolNames.NonControllerAttribute); NonActionAttribute = compilation.GetTypeByMetadataName(SymbolNames.NonActionAttribute); @@ -165,6 +205,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers public INamedTypeSymbol ControllerAttribute { get; } public INamedTypeSymbol FromBodyAttribute { get; } public INamedTypeSymbol IApiBehaviorMetadata { get; } + public INamedTypeSymbol IBinderTypeProviderMetadata { get; } public INamedTypeSymbol IModelNameProvider { get; } public INamedTypeSymbol NonControllerAttribute { get; } public INamedTypeSymbol NonActionAttribute { get; } diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForParametersWithCustomModelBinder.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForParametersWithCustomModelBinder.cs new file mode 100644 index 0000000000..4e86cb1df5 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForParametersWithCustomModelBinder.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsFalse_ForParametersWithCustomModelBinder + { + public string Model { get; set; } + + public void ActionMethod([ModelBinder(typeof(object))] IsProblematicParameter_ReturnsFalse_ForParametersWithCustomModelBinder model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameParameter.cs similarity index 52% rename from test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter.cs rename to test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameParameter.cs index 4c83838f02..20c2a39c3d 100644 --- a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter.cs +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameParameter.cs @@ -1,9 +1,9 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles { - public class IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter + public class IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameParameter { public string Model { get; set; } - public void ActionMethod([FromRoute(Name = "id")] IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter model) { } + public void ActionMethod([FromRoute(Name = "id")] IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameParameter model) { } } } diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameProperty.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameProperty.cs new file mode 100644 index 0000000000..902465213f --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameProperty.cs @@ -0,0 +1,10 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameProperty + { + [FromQuery(Name = "different")] + public string Model { get; set; } + + public void ActionMethod(IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameProperty 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 deleted file mode 100644 index e4a565bd62..0000000000 --- a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty.cs +++ /dev/null @@ -1,10 +0,0 @@ -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_IfModelBinderAttributeIsUsedToRenameParameter.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfModelBinderAttributeIsUsedToRenameParameter.cs new file mode 100644 index 0000000000..0c28fb1132 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfModelBinderAttributeIsUsedToRenameParameter.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsTrue_IfModelBinderAttributeIsUsedToRenameParameter + { + public string Model { get; set; } + + public void ActionMethod([ModelBinder(Name = "model")] IsProblematicParameter_ReturnsTrue_IfModelBinderAttributeIsUsedToRenameParameter different) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/SpecifiesModelTypeTests.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/SpecifiesModelTypeTests.cs new file mode 100644 index 0000000000..e707001d87 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/SpecifiesModelTypeTests.cs @@ -0,0 +1,11 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class SpecifiesModelTypeTests + { + public void SpecifiesModelType_ReturnsFalse_IfModelBinderDoesNotSpecifyType([ModelBinder(Name = "Name")] object model) { } + + public void SpecifiesModelType_ReturnsTrue_IfModelBinderSpecifiesTypeFromConstructor([ModelBinder(typeof(object))] object model) { } + + public void SpecifiesModelType_ReturnsTrue_IfModelBinderSpecifiesTypeFromProperty([ModelBinder(BinderType = typeof(object))] object model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TopLevelParameterNameAnalyzerTest.cs b/test/Mvc.Analyzers.Test/TopLevelParameterNameAnalyzerTest.cs index e5add91ee2..7329ae9d13 100644 --- a/test/Mvc.Analyzers.Test/TopLevelParameterNameAnalyzerTest.cs +++ b/test/Mvc.Analyzers.Test/TopLevelParameterNameAnalyzerTest.cs @@ -61,14 +61,21 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers } [Fact] - public async Task IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter() + public async Task IsProblematicParameter_ReturnsTrue_IfModelBinderAttributeIsUsedToRenameParameter() + { + var result = await IsProblematicParameterTest(); + Assert.True(result); + } + + [Fact] + public async Task IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameProperty() { var result = await IsProblematicParameterTest(); Assert.False(result); } [Fact] - public async Task IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty() + public async Task IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameParameter() { var result = await IsProblematicParameterTest(); Assert.False(result); @@ -81,6 +88,13 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers Assert.False(result); } + [Fact] + public async Task IsProblematicParameter_ReturnsFalse_ForParametersWithCustomModelBinder() + { + var result = await IsProblematicParameterTest(); + Assert.False(result); + } + [Fact] public async Task IsProblematicParameter_IgnoresStaticProperties() { @@ -199,6 +213,60 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers return compilation; } + [Fact] + public async Task SpecifiesModelType_ReturnsFalse_IfModelBinderDoesNotSpecifyType() + { + var testMethod = nameof(SpecifiesModelType_ReturnsFalse_IfModelBinderDoesNotSpecifyType); + var testSource = MvcTestSource.Read(GetType().Name, "SpecifiesModelTypeTests"); + var project = DiagnosticProject.Create(GetType().Assembly, new[] { testSource.Source }); + + var compilation = await project.GetCompilationAsync(); + var symbolCache = new TopLevelParameterNameAnalyzer.SymbolCache(compilation); + + var type = compilation.GetTypeByMetadataName(typeof(SpecifiesModelTypeTests).FullName); + var method = (IMethodSymbol)type.GetMembers(testMethod).First(); + + var parameter = method.Parameters[0]; + var result = TopLevelParameterNameAnalyzer.SpecifiesModelType(symbolCache, parameter); + Assert.False(result); + } + + [Fact] + public async Task SpecifiesModelType_ReturnsTrue_IfModelBinderSpecifiesTypeFromConstructor() + { + var testMethod = nameof(SpecifiesModelType_ReturnsTrue_IfModelBinderSpecifiesTypeFromConstructor); + var testSource = MvcTestSource.Read(GetType().Name, "SpecifiesModelTypeTests"); + var project = DiagnosticProject.Create(GetType().Assembly, new[] { testSource.Source }); + + var compilation = await project.GetCompilationAsync(); + var symbolCache = new TopLevelParameterNameAnalyzer.SymbolCache(compilation); + + var type = compilation.GetTypeByMetadataName(typeof(SpecifiesModelTypeTests).FullName); + var method = (IMethodSymbol)type.GetMembers(testMethod).First(); + + var parameter = method.Parameters[0]; + var result = TopLevelParameterNameAnalyzer.SpecifiesModelType(symbolCache, parameter); + Assert.True(result); + } + + [Fact] + public async Task SpecifiesModelType_ReturnsTrue_IfModelBinderSpecifiesTypeFromProperty() + { + var testMethod = nameof(SpecifiesModelType_ReturnsTrue_IfModelBinderSpecifiesTypeFromProperty); + var testSource = MvcTestSource.Read(GetType().Name, "SpecifiesModelTypeTests"); + var project = DiagnosticProject.Create(GetType().Assembly, new[] { testSource.Source }); + + var compilation = await project.GetCompilationAsync(); + var symbolCache = new TopLevelParameterNameAnalyzer.SymbolCache(compilation); + + var type = compilation.GetTypeByMetadataName(typeof(SpecifiesModelTypeTests).FullName); + var method = (IMethodSymbol)type.GetMembers(testMethod).First(); + + var parameter = method.Parameters[0]; + var result = TopLevelParameterNameAnalyzer.SpecifiesModelType(symbolCache, parameter); + Assert.True(result); + } + private async Task RunNoDiagnosticsAreReturned([CallerMemberName] string testMethod = "") { // Arrange