From dfbdb37979353c12274697b0228d6c113e00e483 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Fri, 13 Jul 2018 13:05:49 -0700 Subject: [PATCH 1/2] Refactorings for codefix (#8067) * Refactorings for codefix - Move some common code in SymbolApiResponseMetadataProvider - Don't run diagnostics for 1006 if we are unable to parse a return type --- .../ActualApiResponseMetadata.cs | 30 +++ .../ApiConventionAnalyzer.cs | 174 ++++------------- ...data.cs => DeclaredApiResponseMetadata.cs} | 4 +- .../SymbolApiResponseMetadataProvider.cs | 116 +++++++++++- .../ApiConventionAnalyzerIntegrationTest.cs | 37 +--- .../ApiConventionAnalyzerTest.cs | 175 ------------------ .../SymbolApiResponseMetadataProviderTest.cs | 125 ++++++++++++- ...ribute_DoesNotDocumentSuccessStatusCode.cs | 20 ++ .../ApiConventionAnalyzerTestFile.cs | 32 +--- .../GetDefaultStatusCodeTest.cs | 12 ++ ...etadata_IfReturnedTypeIsNotActionResult.cs | 12 ++ ...efaultStatusCodeAttributeOnActionResult.cs | 10 + 12 files changed, 361 insertions(+), 386 deletions(-) create mode 100644 src/Microsoft.AspNetCore.Mvc.Analyzers/ActualApiResponseMetadata.cs rename src/Microsoft.AspNetCore.Mvc.Analyzers/{ApiResponseMetadata.cs => DeclaredApiResponseMetadata.cs} (75%) create mode 100644 test/Mvc.Analyzers.Test/TestFiles/ApiConventionAnalyzerIntegrationTest/DiagnosticsAreReturned_IfMethodWithProducesResponseTypeAttribute_DoesNotDocumentSuccessStatusCode.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/SymbolApiResponseMetadataProviderTest/GetDefaultStatusCodeTest.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/SymbolApiResponseMetadataProviderTest/InspectReturnExpression_ReturnsDefaultResponseMetadata_IfReturnedTypeIsNotActionResult.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/SymbolApiResponseMetadataProviderTest/InspectReturnExpression_ReturnsStatusCodeFromDefaultStatusCodeAttributeOnActionResult.cs diff --git a/src/Microsoft.AspNetCore.Mvc.Analyzers/ActualApiResponseMetadata.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/ActualApiResponseMetadata.cs new file mode 100644 index 0000000000..3772f290e7 --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/ActualApiResponseMetadata.cs @@ -0,0 +1,30 @@ +// 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.CodeAnalysis.CSharp.Syntax; + +namespace Microsoft.AspNetCore.Mvc.Analyzers +{ + internal readonly struct ActualApiResponseMetadata + { + private readonly int? _statusCode; + + public ActualApiResponseMetadata(ReturnStatementSyntax returnStatement) + { + ReturnStatement = returnStatement; + _statusCode = null; + } + + public ActualApiResponseMetadata(ReturnStatementSyntax returnStatement, int statusCode) + { + ReturnStatement = returnStatement; + _statusCode = statusCode; + } + + public ReturnStatementSyntax ReturnStatement { get; } + + public int StatusCode => _statusCode.Value; + + public bool IsDefaultResponse => _statusCode == null; + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.Analyzers/ApiConventionAnalyzer.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/ApiConventionAnalyzer.cs index 4efc083a19..57ce1b1e26 100644 --- a/src/Microsoft.AspNetCore.Mvc.Analyzers/ApiConventionAnalyzer.cs +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/ApiConventionAnalyzer.cs @@ -1,7 +1,6 @@ // 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; @@ -15,8 +14,6 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers [DiagnosticAnalyzer(LanguageNames.CSharp)] public class ApiConventionAnalyzer : DiagnosticAnalyzer { - private static readonly Func _shouldDescendIntoChildren = ShouldDescendIntoChildren; - public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create( DiagnosticDescriptors.MVC1004_ActionReturnsUndocumentedStatusCode, DiagnosticDescriptors.MVC1005_ActionReturnsUndocumentedSuccessResult, @@ -44,6 +41,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers { compilationStartAnalysisContext.RegisterSyntaxNodeAction(syntaxNodeContext => { + var cancellationToken = syntaxNodeContext.CancellationToken; var methodSyntax = (MethodDeclarationSyntax)syntaxNodeContext.Node; var semanticModel = syntaxNodeContext.SemanticModel; var method = semanticModel.GetDeclaredSymbol(methodSyntax, syntaxNodeContext.CancellationToken); @@ -54,34 +52,47 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers } var conventionAttributes = GetConventionTypeAttributes(symbolCache, method); - var expectedResponseMetadata = SymbolApiResponseMetadataProvider.GetResponseMetadata(symbolCache, method, conventionAttributes); - var actualResponseMetadata = new HashSet(); - - var context = new ApiConventionContext( - symbolCache, - syntaxNodeContext, - expectedResponseMetadata, - actualResponseMetadata); + var declaredResponseMetadata = SymbolApiResponseMetadataProvider.GetDeclaredResponseMetadata(symbolCache, method, conventionAttributes); + var hasUnreadableStatusCodes = SymbolApiResponseMetadataProvider.TryGetActualResponseMetadata(symbolCache, semanticModel, methodSyntax, cancellationToken, out var actualResponseMetadata); var hasUndocumentedStatusCodes = false; - foreach (var returnStatementSyntax in methodSyntax.DescendantNodes(_shouldDescendIntoChildren).OfType()) + foreach (var item in actualResponseMetadata) { - hasUndocumentedStatusCodes |= VisitReturnStatementSyntax(context, returnStatementSyntax); + var location = item.ReturnStatement.GetLocation(); + + if (item.IsDefaultResponse) + { + if (!(HasStatusCode(declaredResponseMetadata, 200) || HasStatusCode(declaredResponseMetadata, 201))) + { + hasUndocumentedStatusCodes = true; + syntaxNodeContext.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.MVC1005_ActionReturnsUndocumentedSuccessResult, + location)); + } + } + else if (!HasStatusCode(declaredResponseMetadata, item.StatusCode)) + { + hasUndocumentedStatusCodes = true; + syntaxNodeContext.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.MVC1004_ActionReturnsUndocumentedStatusCode, + location, + item.StatusCode)); + } } - if (hasUndocumentedStatusCodes) + if (hasUndocumentedStatusCodes || hasUnreadableStatusCodes) { // If we produced analyzer warnings about undocumented status codes, don't attempt to determine // if there are documented status codes that are missing from the method body. return; } - for (var i = 0; i < expectedResponseMetadata.Count; i++) + for (var i = 0; i < declaredResponseMetadata.Count; i++) { - var expectedStatusCode = expectedResponseMetadata[i].StatusCode; - if (!actualResponseMetadata.Contains(expectedStatusCode)) + var expectedStatusCode = declaredResponseMetadata[i].StatusCode; + if (!HasStatusCode(actualResponseMetadata, expectedStatusCode)) { - context.SyntaxNodeContext.ReportDiagnostic(Diagnostic.Create( + syntaxNodeContext.ReportDiagnostic(Diagnostic.Create( DiagnosticDescriptors.MVC1006_ActionDoesNotReturnDocumentedStatusCode, methodSyntax.Identifier.GetLocation(), expectedStatusCode)); @@ -102,86 +113,6 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers return attributes; } - // Returns true if the return statement returns an undocumented status code. - private static bool VisitReturnStatementSyntax( - in ApiConventionContext context, - ReturnStatementSyntax returnStatementSyntax) - { - var returnExpression = returnStatementSyntax.Expression; - if (returnExpression.IsMissing) - { - return false; - } - - var syntaxNodeContext = context.SyntaxNodeContext; - - var typeInfo = syntaxNodeContext.SemanticModel.GetTypeInfo(returnExpression, syntaxNodeContext.CancellationToken); - if (typeInfo.Type.TypeKind == TypeKind.Error) - { - return false; - } - - var location = returnStatementSyntax.GetLocation(); - var diagnostic = InspectReturnExpression(context, typeInfo.Type, location); - if (diagnostic != null) - { - context.SyntaxNodeContext.ReportDiagnostic(diagnostic); - return true; - } - - return false; - } - - internal static Diagnostic InspectReturnExpression(in ApiConventionContext context, ITypeSymbol type, Location location) - { - var defaultStatusCodeAttribute = type - .GetAttributes(context.SymbolCache.DefaultStatusCodeAttribute, inherit: true) - .FirstOrDefault(); - - if (defaultStatusCodeAttribute != null) - { - var statusCode = GetDefaultStatusCode(defaultStatusCodeAttribute); - if (statusCode == null) - { - // Unable to read the status code. Treat this as valid. - return null; - } - - context.ActualResponseMetadata.Add(statusCode.Value); - if (!HasStatusCode(context.ExpectedResponseMetadata, statusCode.Value)) - { - return Diagnostic.Create( - DiagnosticDescriptors.MVC1004_ActionReturnsUndocumentedStatusCode, - location, - statusCode); - } - } - else if (!context.SymbolCache.IActionResult.IsAssignableFrom(type)) - { - if (!HasStatusCode(context.ExpectedResponseMetadata, 200) && !HasStatusCode(context.ExpectedResponseMetadata, 201)) - { - return Diagnostic.Create( - DiagnosticDescriptors.MVC1005_ActionReturnsUndocumentedSuccessResult, - location); - } - } - - return null; - } - - internal static int? GetDefaultStatusCode(AttributeData attribute) - { - if (attribute != null && - attribute.ConstructorArguments.Length == 1 && - attribute.ConstructorArguments[0].Kind == TypedConstantKind.Primitive && - attribute.ConstructorArguments[0].Value is int statusCode) - { - return statusCode; - } - - return null; - } - internal static bool ShouldEvaluateMethod(ApiControllerSymbolCache symbolCache, IMethodSymbol method) { if (method == null) @@ -212,7 +143,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers return true; } - internal static bool HasStatusCode(IList declaredApiResponseMetadata, int statusCode) + internal static bool HasStatusCode(IList declaredApiResponseMetadata, int statusCode) { if (declaredApiResponseMetadata.Count == 0) { @@ -231,47 +162,22 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers return false; } - private static bool ShouldDescendIntoChildren(SyntaxNode syntaxNode) + internal static bool HasStatusCode(IList actualResponseMetadata, int statusCode) { - return !syntaxNode.IsKind(SyntaxKind.LocalFunctionStatement) && - !syntaxNode.IsKind(SyntaxKind.ParenthesizedLambdaExpression) && - !syntaxNode.IsKind(SyntaxKind.SimpleLambdaExpression) && - !syntaxNode.IsKind(SyntaxKind.AnonymousMethodExpression); - } - - - internal readonly struct ApiConventionContext - { - public ApiConventionContext( - ApiControllerSymbolCache symbolCache, - SyntaxNodeAnalysisContext syntaxNodeContext, - IList expectedResponseMetadata, - HashSet actualResponseMetadata, - Action reportDiagnostic = null) + for (var i = 0; i < actualResponseMetadata.Count; i++) { - SymbolCache = symbolCache; - SyntaxNodeContext = syntaxNodeContext; - ExpectedResponseMetadata = expectedResponseMetadata; - ActualResponseMetadata = actualResponseMetadata; - ReportDiagnosticAction = reportDiagnostic; - } - - public ApiControllerSymbolCache SymbolCache { get; } - public SyntaxNodeAnalysisContext SyntaxNodeContext { get; } - public IList ExpectedResponseMetadata { get; } - public HashSet ActualResponseMetadata { get; } - private Action ReportDiagnosticAction { get; } - - public void ReportDiagnostic(Diagnostic diagnostic) - { - if (ReportDiagnosticAction != null) + if (actualResponseMetadata[i].IsDefaultResponse) { - ReportDiagnosticAction(diagnostic); + return statusCode == 200 || statusCode == 201; } - SyntaxNodeContext.ReportDiagnostic(diagnostic); + else if(actualResponseMetadata[i].StatusCode == statusCode) + { + return true; + } } - } + return false; + } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Analyzers/ApiResponseMetadata.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/DeclaredApiResponseMetadata.cs similarity index 75% rename from src/Microsoft.AspNetCore.Mvc.Analyzers/ApiResponseMetadata.cs rename to src/Microsoft.AspNetCore.Mvc.Analyzers/DeclaredApiResponseMetadata.cs index 29ecd0ca27..75aa335209 100644 --- a/src/Microsoft.AspNetCore.Mvc.Analyzers/ApiResponseMetadata.cs +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/DeclaredApiResponseMetadata.cs @@ -5,9 +5,9 @@ using Microsoft.CodeAnalysis; namespace Microsoft.AspNetCore.Mvc.Analyzers { - internal readonly struct ApiResponseMetadata + internal readonly struct DeclaredApiResponseMetadata { - public ApiResponseMetadata(int statusCode, AttributeData attributeData, IMethodSymbol convention) + public DeclaredApiResponseMetadata(int statusCode, AttributeData attributeData, IMethodSymbol convention) { StatusCode = statusCode; Attribute = attributeData; diff --git a/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolApiResponseMetadataProvider.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolApiResponseMetadataProvider.cs index c5c1367d43..9fbe1916ff 100644 --- a/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolApiResponseMetadataProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolApiResponseMetadataProvider.cs @@ -4,16 +4,20 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Threading; using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; namespace Microsoft.AspNetCore.Mvc.Analyzers { - internal class SymbolApiResponseMetadataProvider + internal static class SymbolApiResponseMetadataProvider { private const string StatusCodeProperty = "StatusCode"; private const string StatusCodeConstructorParameter = "statusCode"; + private static readonly Func _shouldDescendIntoChildren = ShouldDescendIntoChildren; - internal static IList GetResponseMetadata( + internal static IList GetDeclaredResponseMetadata( ApiControllerSymbolCache symbolCache, IMethodSymbol method, IReadOnlyList conventionTypeAttributes) @@ -28,7 +32,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers return metadataItems; } - private static IList GetResponseMetadataFromConventions( + private static IList GetResponseMetadataFromConventions( ApiControllerSymbolCache symbolCache, IMethodSymbol method, IReadOnlyList attributes) @@ -58,17 +62,17 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers } } - return Array.Empty(); + return Array.Empty(); } - private static IList GetResponseMetadataFromMethodAttributes(ApiControllerSymbolCache symbolCache, IMethodSymbol methodSymbol) + private static IList GetResponseMetadataFromMethodAttributes(ApiControllerSymbolCache symbolCache, IMethodSymbol methodSymbol) { - var metadataItems = new List(); + var metadataItems = new List(); var responseMetadataAttributes = methodSymbol.GetAttributes(symbolCache.ProducesResponseTypeAttribute, inherit: true); foreach (var attribute in responseMetadataAttributes) { var statusCode = GetStatusCode(attribute); - var metadata = new ApiResponseMetadata(statusCode, attribute, convention: null); + var metadata = new DeclaredApiResponseMetadata(statusCode, attribute, convention: null); metadataItems.Add(metadata); } @@ -119,5 +123,103 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers return DefaultStatusCode; } + + internal static bool TryGetActualResponseMetadata( + in ApiControllerSymbolCache symbolCache, + SemanticModel semanticModel, + MethodDeclarationSyntax methodSyntax, + CancellationToken cancellationToken, + out IList actualResponseMetadata) + { + actualResponseMetadata = new List(); + + var hasUnreadableReturnStatements = false; + + foreach (var returnStatementSyntax in methodSyntax.DescendantNodes(_shouldDescendIntoChildren).OfType()) + { + var responseMetadata = InspectReturnStatementSyntax( + symbolCache, + semanticModel, + returnStatementSyntax, + cancellationToken); + + if (responseMetadata != null) + { + actualResponseMetadata.Add(responseMetadata.Value); + } + else + { + hasUnreadableReturnStatements = true; + } + } + + return hasUnreadableReturnStatements; + } + + internal static ActualApiResponseMetadata? InspectReturnStatementSyntax( + in ApiControllerSymbolCache symbolCache, + SemanticModel semanticModel, + ReturnStatementSyntax returnStatementSyntax, + CancellationToken cancellationToken) + { + var returnExpression = returnStatementSyntax.Expression; + if (returnExpression.IsMissing) + { + return null; + } + + var typeInfo = semanticModel.GetTypeInfo(returnExpression, cancellationToken); + if (typeInfo.Type.TypeKind == TypeKind.Error) + { + return null; + } + + var statementReturnType = typeInfo.Type; + + var defaultStatusCodeAttribute = statementReturnType + .GetAttributes(symbolCache.DefaultStatusCodeAttribute, inherit: true) + .FirstOrDefault(); + + if (defaultStatusCodeAttribute != null) + { + var statusCode = GetDefaultStatusCode(defaultStatusCodeAttribute); + if (statusCode == null) + { + // Unable to read the status code even though the attribute exists. + return null; + } + + return new ActualApiResponseMetadata(returnStatementSyntax, statusCode.Value); + } + else if (!symbolCache.IActionResult.IsAssignableFrom(statementReturnType)) + { + // Return expression does not have a DefaultStatusCodeAttribute and it is not + // an instance of IActionResult. Must be returning the "model". + return new ActualApiResponseMetadata(returnStatementSyntax); + } + + return null; + } + + private static bool ShouldDescendIntoChildren(SyntaxNode syntaxNode) + { + return !syntaxNode.IsKind(SyntaxKind.LocalFunctionStatement) && + !syntaxNode.IsKind(SyntaxKind.ParenthesizedLambdaExpression) && + !syntaxNode.IsKind(SyntaxKind.SimpleLambdaExpression) && + !syntaxNode.IsKind(SyntaxKind.AnonymousMethodExpression); + } + + internal static int? GetDefaultStatusCode(AttributeData attribute) + { + if (attribute != null && + attribute.ConstructorArguments.Length == 1 && + attribute.ConstructorArguments[0].Kind == TypedConstantKind.Primitive && + attribute.ConstructorArguments[0].Value is int statusCode) + { + return statusCode; + } + + return null; + } } } \ No newline at end of file diff --git a/test/Mvc.Analyzers.Test/ApiConventionAnalyzerIntegrationTest.cs b/test/Mvc.Analyzers.Test/ApiConventionAnalyzerIntegrationTest.cs index 75307db01f..0fdd22a66a 100644 --- a/test/Mvc.Analyzers.Test/ApiConventionAnalyzerIntegrationTest.cs +++ b/test/Mvc.Analyzers.Test/ApiConventionAnalyzerIntegrationTest.cs @@ -13,7 +13,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers { public class ApiConventionAnalyzerIntegrationTest { - private MvcDiagnosticAnalyzerRunner Executor { get; } = new MvcDiagnosticAnalyzerRunner(new ApiConventionAnalyzer()); + private MvcDiagnosticAnalyzerRunner Executor { get; } = new ApiCoventionWith1006DiagnosticEnabledRunner(); [Fact] public Task NoDiagnosticsAreReturned_ForNonApiController() @@ -77,11 +77,15 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers [Fact] public Task DiagnosticsAreReturned_IfMethodWithProducesResponseTypeAttribute_DoesNotReturnDocumentedStatusCode() - => RunTestFor1006(400); + => RunTest(DiagnosticDescriptors.MVC1006_ActionDoesNotReturnDocumentedStatusCode, 400); [Fact] public Task DiagnosticsAreReturned_IfMethodWithConvention_DoesNotReturnDocumentedStatusCode() - => RunTestFor1006(404); + => RunTest(DiagnosticDescriptors.MVC1006_ActionDoesNotReturnDocumentedStatusCode, 404); + + [Fact] + public Task DiagnosticsAreReturned_IfMethodWithProducesResponseTypeAttribute_DoesNotDocumentSuccessStatusCode() + => RunTest(DiagnosticDescriptors.MVC1006_ActionDoesNotReturnDocumentedStatusCode, 200); private async Task RunNoDiagnosticsAreReturned([CallerMemberName] string testMethod = "") { @@ -122,30 +126,6 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers Assert.Equal(string.Format(descriptor.MessageFormat.ToString(), args), diagnostic.GetMessage()); }); } - - private async Task RunTestFor1006(int statusCode, [CallerMemberName] string testMethod = "") - { - // Arrange - var descriptor = DiagnosticDescriptors.MVC1006_ActionDoesNotReturnDocumentedStatusCode; - var testSource = MvcTestSource.Read(GetType().Name, testMethod); - var expectedLocation = testSource.DefaultMarkerLocation; - var executor = new ApiCoventionWith1006DiagnosticEnabledRunner(); - - // Act - var result = await executor.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(), new[] { statusCode.ToString() }), diagnostic.GetMessage()); - }); - } - private class ApiCoventionWith1006DiagnosticEnabledRunner : MvcDiagnosticAnalyzerRunner { public ApiCoventionWith1006DiagnosticEnabledRunner() : base(new ApiConventionAnalyzer()) @@ -155,6 +135,9 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers protected override CompilationOptions ConfigureCompilationOptions(CompilationOptions options) { var compilationOptions = base.ConfigureCompilationOptions(options); + + // 10006 is disabled by default. Explicitly enable it so we can correctly validate no diagnostics + // are returned scenarios. var specificDiagnosticOptions = compilationOptions.SpecificDiagnosticOptions.Add( DiagnosticDescriptors.MVC1006_ActionDoesNotReturnDocumentedStatusCode.Id, ReportDiagnostic.Info); diff --git a/test/Mvc.Analyzers.Test/ApiConventionAnalyzerTest.cs b/test/Mvc.Analyzers.Test/ApiConventionAnalyzerTest.cs index 65283bc5f8..b1e7578557 100644 --- a/test/Mvc.Analyzers.Test/ApiConventionAnalyzerTest.cs +++ b/test/Mvc.Analyzers.Test/ApiConventionAnalyzerTest.cs @@ -1,181 +1,17 @@ // 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.Linq; using System.Threading.Tasks; using Microsoft.AspNetCore.Analyzer.Testing; using Microsoft.AspNetCore.Mvc.Analyzers.Infrastructure; using Microsoft.CodeAnalysis; using Xunit; -using static Microsoft.AspNetCore.Mvc.Analyzers.ApiConventionAnalyzer; namespace Microsoft.AspNetCore.Mvc.Analyzers { public class ApiConventionAnalyzerTest { - [Fact] - public async Task GetDefaultStatusCode_ReturnsValueDefinedUsingStatusCodeConstants() - { - // Arrange - var compilation = await GetCompilation(); - var attribute = compilation.GetTypeByMetadataName(typeof(TestActionResultUsingStatusCodesConstants).FullName).GetAttributes()[0]; - - // Act - var actual = ApiConventionAnalyzer.GetDefaultStatusCode(attribute); - - // Assert - Assert.Equal(412, actual); - } - - [Fact] - public async Task GetDefaultStatusCode_ReturnsValueDefinedUsingHttpStatusCast() - { - // Arrange - var compilation = await GetCompilation(); - var attribute = compilation.GetTypeByMetadataName(typeof(TestActionResultUsingHttpStatusCodeCast).FullName).GetAttributes()[0]; - - // Act - var actual = ApiConventionAnalyzer.GetDefaultStatusCode(attribute); - - // Assert - Assert.Equal(302, actual); - } - - [Fact] - public async Task InspectReturnExpression_ReturnsNull_ForReturnTypeIf200StatusCodeIsDeclared() - { - // Arrange - var compilation = await GetCompilation(); - - var returnType = compilation.GetTypeByMetadataName(typeof(ApiConventionAnalyzerBaseModel).FullName); - var context = GetContext(compilation, new[] { 200 }); - - // Act - var diagnostic = ApiConventionAnalyzer.InspectReturnExpression(context, returnType, Location.None); - - // Assert - Assert.Null(diagnostic); - } - - [Fact] - public async Task InspectReturnExpression_ReturnsNull_ForReturnTypeIf201StatusCodeIsDeclared() - { - // Arrange - var compilation = await GetCompilation(); - - var returnType = compilation.GetTypeByMetadataName(typeof(ApiConventionAnalyzerBaseModel).FullName); - var context = GetContext(compilation, new[] { 201 }); - - // Act - var diagnostic = ApiConventionAnalyzer.InspectReturnExpression(context, returnType, Location.None); - - // Assert - Assert.Null(diagnostic); - } - - [Fact] - public async Task InspectReturnExpression_ReturnsNull_ForDerivedReturnTypeIf200StatusCodeIsDeclared() - { - // Arrange - var compilation = await GetCompilation(); - - var declaredReturnType = compilation.GetTypeByMetadataName(typeof(ApiConventionAnalyzerBaseModel).FullName); - var context = GetContext(compilation, new[] { 201 }); - var actualReturnType = compilation.GetTypeByMetadataName(typeof(ApiConventionAnalyzerDerivedModel).FullName); - - // Act - var diagnostic = ApiConventionAnalyzer.InspectReturnExpression(context, actualReturnType, Location.None); - - // Assert - Assert.Null(diagnostic); - } - - [Fact] - public async Task InspectReturnExpression_ReturnsDiagnostic_If200IsNotDocumented() - { - // Arrange - var compilation = await GetCompilation(); - - var context = GetContext(compilation, new[] { 404 }); - var actualReturnType = compilation.GetTypeByMetadataName(typeof(ApiConventionAnalyzerDerivedModel).FullName); - - // Act - var diagnostic = ApiConventionAnalyzer.InspectReturnExpression(context, actualReturnType, Location.None); - - // Assert - Assert.NotNull(diagnostic); - Assert.Same(DiagnosticDescriptors.MVC1005_ActionReturnsUndocumentedSuccessResult, diagnostic.Descriptor); - } - - [Fact] - public async Task InspectReturnExpression_ReturnsDiagnostic_IfReturnTypeIsActionResultReturningUndocumentedStatusCode() - { - // Arrange - var compilation = await GetCompilation(); - - var declaredReturnType = compilation.GetTypeByMetadataName(typeof(ApiConventionAnalyzerBaseModel).FullName); - var context = GetContext(compilation, new[] { 200, 404 }); - var actualReturnType = compilation.GetTypeByMetadataName(typeof(BadRequestObjectResult).FullName); - - // Act - var diagnostic = ApiConventionAnalyzer.InspectReturnExpression(context, actualReturnType, Location.None); - - // Assert - Assert.NotNull(diagnostic); - Assert.Same(DiagnosticDescriptors.MVC1004_ActionReturnsUndocumentedStatusCode, diagnostic.Descriptor); - } - - [Fact] - public async Task InspectReturnExpression_DoesNotReturnDiagnostic_IfReturnTypeDoesNotHaveStatusCodeAttribute() - { - // Arrange - var compilation = await GetCompilation(); - - var context = GetContext(compilation, new[] { 200, 404 }); - var actualReturnType = compilation.GetTypeByMetadataName(typeof(EmptyResult).FullName); - - // Act - var diagnostic = ApiConventionAnalyzer.InspectReturnExpression(context, actualReturnType, Location.None); - - // Assert - Assert.Null(diagnostic); - } - - [Fact] - public async Task InspectReturnExpression_DoesNotReturnDiagnostic_IfDeclaredAndActualReturnTypeAreIActionResultInstances() - { - // Arrange - var compilation = await GetCompilation(); - - var declaredReturnType = compilation.GetTypeByMetadataName(typeof(IActionResult).FullName); - var context = GetContext(compilation, new[] { 404 }); - var actualReturnType = compilation.GetTypeByMetadataName(typeof(EmptyResult).FullName); - - // Act - var diagnostic = ApiConventionAnalyzer.InspectReturnExpression(context, actualReturnType, Location.None); - - // Assert - Assert.Null(diagnostic); - } - - [Fact] - public async Task InspectReturnExpression_DoesNotReturnDiagnostic_IfDeclaredAndActualReturnTypeAreIActionResult() - { - // Arrange - var compilation = await GetCompilation(); - - var context = GetContext(compilation, new[] { 404 }); - var actualReturnType = compilation.GetTypeByMetadataName(typeof(IActionResult).FullName); - - // Act - var diagnostic = ApiConventionAnalyzer.InspectReturnExpression(context, actualReturnType, Location.None); - - // Assert - Assert.Null(diagnostic); - } - [Fact] public async Task ShouldEvaluateMethod_ReturnsFalse_IfMethodReturnTypeIsInvalid() { @@ -275,17 +111,6 @@ namespace TestNamespace Assert.True(result); } - private static ApiConventionContext GetContext(Compilation compilation, int[] expectedStatusCodes) - { - var symbolCache = new ApiControllerSymbolCache(compilation); - var context = new ApiConventionContext( - symbolCache, - default, - expectedStatusCodes.Select(s => new ApiResponseMetadata(s, null, null)).ToArray(), - new HashSet()); - return context; - } - private Task GetCompilation() { var testSource = MvcTestSource.Read(GetType().Name, "ApiConventionAnalyzerTestFile"); diff --git a/test/Mvc.Analyzers.Test/SymbolApiResponseMetadataProviderTest.cs b/test/Mvc.Analyzers.Test/SymbolApiResponseMetadataProviderTest.cs index c31846a88d..32f764fb67 100644 --- a/test/Mvc.Analyzers.Test/SymbolApiResponseMetadataProviderTest.cs +++ b/test/Mvc.Analyzers.Test/SymbolApiResponseMetadataProviderTest.cs @@ -2,11 +2,15 @@ // 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.Linq; +using System.Runtime.CompilerServices; +using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Analyzer.Testing; using Microsoft.AspNetCore.Mvc.Analyzers.Infrastructure; using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; using Xunit; namespace Microsoft.AspNetCore.Mvc.Analyzers @@ -25,7 +29,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers var symbolCache = new ApiControllerSymbolCache(compilation); // Act - var result = SymbolApiResponseMetadataProvider.GetResponseMetadata(symbolCache, method, Array.Empty()); + var result = SymbolApiResponseMetadataProvider.GetDeclaredResponseMetadata(symbolCache, method, Array.Empty()); // Assert Assert.Empty(result); @@ -41,7 +45,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers var symbolCache = new ApiControllerSymbolCache(compilation); // Act - var result = SymbolApiResponseMetadataProvider.GetResponseMetadata(symbolCache, method, Array.Empty()); + var result = SymbolApiResponseMetadataProvider.GetDeclaredResponseMetadata(symbolCache, method, Array.Empty()); // Assert Assert.Empty(result); @@ -57,7 +61,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers var symbolCache = new ApiControllerSymbolCache(compilation); // Act - var result = SymbolApiResponseMetadataProvider.GetResponseMetadata(symbolCache, method, Array.Empty()); + var result = SymbolApiResponseMetadataProvider.GetDeclaredResponseMetadata(symbolCache, method, Array.Empty()); // Assert Assert.Empty(result); @@ -73,7 +77,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers var symbolCache = new ApiControllerSymbolCache(compilation); // Act - var result = SymbolApiResponseMetadataProvider.GetResponseMetadata(symbolCache, method, Array.Empty()); + var result = SymbolApiResponseMetadataProvider.GetDeclaredResponseMetadata(symbolCache, method, Array.Empty()); // Assert Assert.Collection( @@ -96,7 +100,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers var symbolCache = new ApiControllerSymbolCache(compilation); // Act - var result = SymbolApiResponseMetadataProvider.GetResponseMetadata(symbolCache, method, Array.Empty()); + var result = SymbolApiResponseMetadataProvider.GetDeclaredResponseMetadata(symbolCache, method, Array.Empty()); // Assert Assert.Collection( @@ -119,7 +123,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers var symbolCache = new ApiControllerSymbolCache(compilation); // Act - var result = SymbolApiResponseMetadataProvider.GetResponseMetadata(symbolCache, method, Array.Empty()); + var result = SymbolApiResponseMetadataProvider.GetDeclaredResponseMetadata(symbolCache, method, Array.Empty()); // Assert Assert.Collection( @@ -142,7 +146,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers var symbolCache = new ApiControllerSymbolCache(compilation); // Act - var result = SymbolApiResponseMetadataProvider.GetResponseMetadata(symbolCache, method, Array.Empty()); + var result = SymbolApiResponseMetadataProvider.GetDeclaredResponseMetadata(symbolCache, method, Array.Empty()); // Assert Assert.Collection( @@ -165,7 +169,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers var symbolCache = new ApiControllerSymbolCache(compilation); // Act - var result = SymbolApiResponseMetadataProvider.GetResponseMetadata(symbolCache, method, Array.Empty()); + var result = SymbolApiResponseMetadataProvider.GetDeclaredResponseMetadata(symbolCache, method, Array.Empty()); // Assert Assert.Collection( @@ -188,7 +192,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers var symbolCache = new ApiControllerSymbolCache(compilation); // Act - var result = SymbolApiResponseMetadataProvider.GetResponseMetadata(symbolCache, method, Array.Empty()); + var result = SymbolApiResponseMetadataProvider.GetDeclaredResponseMetadata(symbolCache, method, Array.Empty()); // Assert Assert.Empty(result); @@ -219,7 +223,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers var symbolCache = new ApiControllerSymbolCache(compilation); // Act - var result = SymbolApiResponseMetadataProvider.GetResponseMetadata(symbolCache, method, Array.Empty()); + var result = SymbolApiResponseMetadataProvider.GetDeclaredResponseMetadata(symbolCache, method, Array.Empty()); // Assert Assert.Collection( @@ -299,6 +303,107 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers Assert.Equal(expected, statusCode); } + [Fact] + public async Task GetDefaultStatusCode_ReturnsValueDefinedUsingStatusCodeConstants() + { + // Arrange + var compilation = await GetCompilation("GetDefaultStatusCodeTest"); + var attribute = compilation.GetTypeByMetadataName(typeof(TestActionResultUsingStatusCodesConstants).FullName).GetAttributes()[0]; + + // Act + var actual = SymbolApiResponseMetadataProvider.GetDefaultStatusCode(attribute); + + // Assert + Assert.Equal(412, actual); + } + + [Fact] + public async Task GetDefaultStatusCode_ReturnsValueDefinedUsingHttpStatusCast() + { + // Arrange + var compilation = await GetCompilation("GetDefaultStatusCodeTest"); + var attribute = compilation.GetTypeByMetadataName(typeof(TestActionResultUsingHttpStatusCodeCast).FullName).GetAttributes()[0]; + + // Act + var actual = SymbolApiResponseMetadataProvider.GetDefaultStatusCode(attribute); + + // Assert + Assert.Equal(302, actual); + } + + [Fact] + public async Task InspectReturnExpression_ReturnsNull_IfReturnExpressionCannotBeFound() + { + // Arrange & Act + var source = @" + using Microsoft.AspNetCore.Mvc; + +namespace Microsoft.AspNetCore.Mvc.Analyzers +{ + [ApiController] + public class InspectReturnExpression_ReturnsNull_IfReturnExpressionCannotBeFound : ControllerBase + { + public IActionResult Get(int id) + { + return new DoesNotExist(id); + } + } +}"; + var actualResponseMetadata = await RunInspectReturnStatementSyntax(source, nameof(InspectReturnExpression_ReturnsNull_IfReturnExpressionCannotBeFound)); + + // Assert + Assert.Null(actualResponseMetadata); + } + + [Fact] + public async Task InspectReturnExpression_ReturnsStatusCodeFromDefaultStatusCodeAttributeOnActionResult() + { + // Arrange & Act + var actualResponseMetadata = await RunInspectReturnStatementSyntax(); + + // Assert + Assert.NotNull(actualResponseMetadata); + Assert.Equal(401, actualResponseMetadata.Value.StatusCode); + } + + [Fact] + public async Task InspectReturnExpression_ReturnsDefaultResponseMetadata_IfReturnedTypeIsNotActionResult() + { + // Arrange & Act + var actualResponseMetadata = await RunInspectReturnStatementSyntax(); + + // Assert + Assert.NotNull(actualResponseMetadata); + Assert.True(actualResponseMetadata.Value.IsDefaultResponse); + } + + private async Task RunInspectReturnStatementSyntax([CallerMemberName]string test = null) + { + // Arrange + var testSource = MvcTestSource.Read(GetType().Name, test); + return await RunInspectReturnStatementSyntax(testSource.Source, test); + } + + private async Task RunInspectReturnStatementSyntax(string source, string test) + { + var project = DiagnosticProject.Create(GetType().Assembly, new[] { source }); + var compilation = await project.GetCompilationAsync(); + var symbolCache = new ApiControllerSymbolCache(compilation); + + var returnType = compilation.GetTypeByMetadataName($"{Namespace}.{test}"); + var syntaxTree = returnType.DeclaringSyntaxReferences[0].SyntaxTree; + + var method = (IMethodSymbol)returnType.GetMembers().First(); + var methodSyntax = syntaxTree.GetRoot().FindNode(method.Locations[0].SourceSpan); + var returnStatement = methodSyntax.DescendantNodes().OfType().First(); + + return SymbolApiResponseMetadataProvider.InspectReturnStatementSyntax( + symbolCache, + compilation.GetSemanticModel(syntaxTree), + returnStatement, + CancellationToken.None); + } + private Task GetResponseMetadataCompilation() => GetCompilation("GetResponseMetadataTests"); private Task GetCompilation(string test) diff --git a/test/Mvc.Analyzers.Test/TestFiles/ApiConventionAnalyzerIntegrationTest/DiagnosticsAreReturned_IfMethodWithProducesResponseTypeAttribute_DoesNotDocumentSuccessStatusCode.cs b/test/Mvc.Analyzers.Test/TestFiles/ApiConventionAnalyzerIntegrationTest/DiagnosticsAreReturned_IfMethodWithProducesResponseTypeAttribute_DoesNotDocumentSuccessStatusCode.cs new file mode 100644 index 0000000000..bdbc7bf659 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/ApiConventionAnalyzerIntegrationTest/DiagnosticsAreReturned_IfMethodWithProducesResponseTypeAttribute_DoesNotDocumentSuccessStatusCode.cs @@ -0,0 +1,20 @@ +using System; + +namespace Microsoft.AspNetCore.Mvc.Analyzers +{ + [ApiController] + public class DiagnosticsAreReturned_IfMethodWithProducesResponseTypeAttribute_DoesNotDocumentSuccessStatusCode : ControllerBase + { + [ProducesResponseType(200)] + [ProducesResponseType(404)] + public ActionResult /*MM*/Method(int id) + { + if (id == 0) + { + return NotFound(); + } + + throw new NotImplementedException(); + } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/ApiConventionAnalyzerTest/ApiConventionAnalyzerTestFile.cs b/test/Mvc.Analyzers.Test/TestFiles/ApiConventionAnalyzerTest/ApiConventionAnalyzerTestFile.cs index c4d6a646a9..397c9b86b1 100644 --- a/test/Mvc.Analyzers.Test/TestFiles/ApiConventionAnalyzerTest/ApiConventionAnalyzerTestFile.cs +++ b/test/Mvc.Analyzers.Test/TestFiles/ApiConventionAnalyzerTest/ApiConventionAnalyzerTestFile.cs @@ -1,37 +1,7 @@ -using System.Collections.Generic; -using System.Net; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Http; -using Microsoft.AspNetCore.Mvc.Infrastructure; -using Microsoft.AspNetCore.Mvc.RazorPages; +using Microsoft.AspNetCore.Mvc.RazorPages; namespace Microsoft.AspNetCore.Mvc.Analyzers { - public class UnwrapMethodReturnType - { - public ApiConventionAnalyzerBaseModel ReturnsBaseModel() => null; - - public ActionResult ReturnsActionResultOfBaseModel() => null; - - public Task> ReturnsTaskOfActionResultOfBaseModel() => null; - - public ValueTask> ReturnsValueTaskOfActionResultOfBaseModel() => default(ValueTask>); - - public ActionResult> ReturnsActionResultOfIEnumerableOfBaseModel() => null; - - public IEnumerable ReturnsIEnumerableOfBaseModel() => null; - } - - [DefaultStatusCode(StatusCodes.Status412PreconditionFailed)] - public class TestActionResultUsingStatusCodesConstants { } - - [DefaultStatusCode((int)HttpStatusCode.Found)] - public class TestActionResultUsingHttpStatusCodeCast { } - - public class ApiConventionAnalyzerBaseModel { } - - public class ApiConventionAnalyzerDerivedModel : ApiConventionAnalyzerBaseModel { } - public class ApiConventionAnalyzerTest_IndexModel : PageModel { public IActionResult OnGet() => null; diff --git a/test/Mvc.Analyzers.Test/TestFiles/SymbolApiResponseMetadataProviderTest/GetDefaultStatusCodeTest.cs b/test/Mvc.Analyzers.Test/TestFiles/SymbolApiResponseMetadataProviderTest/GetDefaultStatusCodeTest.cs new file mode 100644 index 0000000000..6d9412a840 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/SymbolApiResponseMetadataProviderTest/GetDefaultStatusCodeTest.cs @@ -0,0 +1,12 @@ +using System.Net; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc.Infrastructure; + +namespace Microsoft.AspNetCore.Mvc.Analyzers +{ + [DefaultStatusCode(StatusCodes.Status412PreconditionFailed)] + public class TestActionResultUsingStatusCodesConstants { } + + [DefaultStatusCode((int)HttpStatusCode.Redirect)] + public class TestActionResultUsingHttpStatusCodeCast { } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/SymbolApiResponseMetadataProviderTest/InspectReturnExpression_ReturnsDefaultResponseMetadata_IfReturnedTypeIsNotActionResult.cs b/test/Mvc.Analyzers.Test/TestFiles/SymbolApiResponseMetadataProviderTest/InspectReturnExpression_ReturnsDefaultResponseMetadata_IfReturnedTypeIsNotActionResult.cs new file mode 100644 index 0000000000..ce2dbbb39b --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/SymbolApiResponseMetadataProviderTest/InspectReturnExpression_ReturnsDefaultResponseMetadata_IfReturnedTypeIsNotActionResult.cs @@ -0,0 +1,12 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers +{ + public class InspectReturnExpression_ReturnsDefaultResponseMetadata_IfReturnedTypeIsNotActionResult : ControllerBase + { + public object Get() + { + return new InspectReturnExpression_ReturnsDefaultResponseMetadata_IfReturnedTypeIsNotActionResultModel(); + } + } + + public class InspectReturnExpression_ReturnsDefaultResponseMetadata_IfReturnedTypeIsNotActionResultModel { } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/SymbolApiResponseMetadataProviderTest/InspectReturnExpression_ReturnsStatusCodeFromDefaultStatusCodeAttributeOnActionResult.cs b/test/Mvc.Analyzers.Test/TestFiles/SymbolApiResponseMetadataProviderTest/InspectReturnExpression_ReturnsStatusCodeFromDefaultStatusCodeAttributeOnActionResult.cs new file mode 100644 index 0000000000..4d5d0d9849 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/SymbolApiResponseMetadataProviderTest/InspectReturnExpression_ReturnsStatusCodeFromDefaultStatusCodeAttributeOnActionResult.cs @@ -0,0 +1,10 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers +{ + public class InspectReturnExpression_ReturnsStatusCodeFromDefaultStatusCodeAttributeOnActionResult : ControllerBase + { + public IActionResult Get() + { + return Unauthorized(); + } + } +} From f2608c2ff4084eb617a63ca6056ca1955edafd86 Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Thu, 12 Jul 2018 11:21:21 -0700 Subject: [PATCH 2/2] Do not suppress `ModelValidationState.Invalid` entries - #7992, #7963 --- .../Validation/ValidationVisitor.cs | 5 +- .../Internal/DefaultObjectValidatorTests.cs | 63 ++++++++ .../ModelBinding/ParameterBinderTest.cs | 148 +++++++++++++++--- 3 files changed, 193 insertions(+), 23 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs index f8c4c02dd7..ecf44973cc 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs @@ -296,7 +296,10 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation var entries = ModelState.FindKeysWithPrefix(key); foreach (var entry in entries) { - entry.Value.ValidationState = ModelValidationState.Skipped; + if (entry.Value.ValidationState != ModelValidationState.Invalid) + { + entry.Value.ValidationState = ModelValidationState.Skipped; + } } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs index 5371633029..a9ee38cfc6 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs @@ -133,6 +133,27 @@ namespace Microsoft.AspNetCore.Mvc.Internal Assert.Empty(entry.Errors); } + // More like how product code does suppressions than Validate_SimpleType_SuppressValidation() + [Fact] + public void Validate_SimpleType_SuppressValidationWithNullKey() + { + // Arrange + var actionContext = new ActionContext(); + var modelState = actionContext.ModelState; + var validator = CreateValidator(); + var model = "test"; + var validationState = new ValidationStateDictionary + { + { model, new ValidationStateEntry { SuppressValidation = true } } + }; + + // Act + validator.Validate(actionContext, validationState, "parameter", model); + + // Assert + Assert.True(modelState.IsValid); + Assert.Empty(modelState); + } [Fact] public void Validate_ComplexValueType_Valid() @@ -1195,6 +1216,48 @@ namespace Microsoft.AspNetCore.Mvc.Internal Assert.Empty(entry.Value.Errors); } + // Regression test for aspnet/Mvc#7992 + [Fact] + public void Validate_SuppressValidation_AfterHasReachedMaxErrors_Invalid() + { + // Arrange + var actionContext = new ActionContext(); + var modelState = actionContext.ModelState; + modelState.MaxAllowedErrors = 2; + modelState.AddModelError(key: "one", errorMessage: "1"); + modelState.AddModelError(key: "two", errorMessage: "2"); + + var validator = CreateValidator(); + var model = (object)23; // Box ASAP + var validationState = new ValidationStateDictionary + { + { model, new ValidationStateEntry { SuppressValidation = true } } + }; + + // Act + validator.Validate(actionContext, validationState, prefix: string.Empty, model); + + // Assert + Assert.False(modelState.IsValid); + Assert.True(modelState.HasReachedMaxErrors); + Assert.Collection( + modelState, + kvp => + { + Assert.Empty(kvp.Key); + Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState); + var error = Assert.Single(kvp.Value.Errors); + Assert.IsType(error.Exception); + }, + kvp => + { + Assert.Equal("one", kvp.Key); + Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState); + var error = Assert.Single(kvp.Value.Errors); + Assert.Equal("1", error.ErrorMessage); + }); + } + private static DefaultObjectValidator CreateValidator(Type excludedType) { var excludeFilters = new List(); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs index 7a605c4640..8c0ea54528 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs @@ -8,6 +8,7 @@ using System.Linq; using System.Reflection; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.JsonPatch; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.AspNetCore.Mvc.Controllers; using Microsoft.AspNetCore.Mvc.DataAnnotations; @@ -706,6 +707,131 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding }); } + // Regression test 1 for aspnet/Mvc#7963. ModelState should never be valid. + [Fact] + public async Task BindModelAsync_ForOverlappingParametersWithSuppressions_InValid_WithValidSecondParameter() + { + // Arrange + var parameterDescriptor = new ParameterDescriptor + { + Name = "patchDocument", + ParameterType = typeof(IJsonPatchDocument), + }; + + var actionContext = GetControllerContext(); + var modelState = actionContext.ModelState; + + // First ModelState key is not empty to match SimpleTypeModelBinder. + modelState.SetModelValue("id", "notAGuid", "notAGuid"); + modelState.AddModelError("id", "This is not valid."); + + var modelMetadataProvider = new TestModelMetadataProvider(); + modelMetadataProvider.ForType().ValidationDetails(v => v.ValidateChildren = false); + var modelMetadata = modelMetadataProvider.GetMetadataForType(typeof(IJsonPatchDocument)); + + var parameterBinder = new ParameterBinder( + modelMetadataProvider, + Mock.Of(), + new DefaultObjectValidator( + modelMetadataProvider, + new[] { TestModelValidatorProvider.CreateDefaultProvider() }), + _optionsAccessor, + NullLoggerFactory.Instance); + + // BodyModelBinder does not update ModelState in success case. + var modelBindingResult = ModelBindingResult.Success(new JsonPatchDocument()); + var modelBinder = CreateMockModelBinder(modelBindingResult); + + // Act + var result = await parameterBinder.BindModelAsync( + actionContext, + modelBinder, + new SimpleValueProvider(), + parameterDescriptor, + modelMetadata, + value: null); + + // Assert + Assert.True(result.IsModelSet); + Assert.False(modelState.IsValid); + Assert.Collection( + modelState, + kvp => + { + Assert.Equal("id", kvp.Key); + Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState); + var error = Assert.Single(kvp.Value.Errors); + Assert.Equal("This is not valid.", error.ErrorMessage); + }); + } + + // Regression test 2 for aspnet/Mvc#7963. ModelState should never be valid. + [Fact] + public async Task BindModelAsync_ForOverlappingParametersWithSuppressions_InValid_WithInValidSecondParameter() + { + // Arrange + var parameterDescriptor = new ParameterDescriptor + { + Name = "patchDocument", + ParameterType = typeof(IJsonPatchDocument), + }; + + var actionContext = GetControllerContext(); + var modelState = actionContext.ModelState; + + // First ModelState key is not empty to match SimpleTypeModelBinder. + modelState.SetModelValue("id", "notAGuid", "notAGuid"); + modelState.AddModelError("id", "This is not valid."); + + // Second ModelState key is empty to match BodyModelBinder. + modelState.AddModelError(string.Empty, "This is also not valid."); + + var modelMetadataProvider = new TestModelMetadataProvider(); + modelMetadataProvider.ForType().ValidationDetails(v => v.ValidateChildren = false); + var modelMetadata = modelMetadataProvider.GetMetadataForType(typeof(IJsonPatchDocument)); + + var parameterBinder = new ParameterBinder( + modelMetadataProvider, + Mock.Of(), + new DefaultObjectValidator( + modelMetadataProvider, + new[] { TestModelValidatorProvider.CreateDefaultProvider() }), + _optionsAccessor, + NullLoggerFactory.Instance); + + var modelBindingResult = ModelBindingResult.Failed(); + var modelBinder = CreateMockModelBinder(modelBindingResult); + + // Act + var result = await parameterBinder.BindModelAsync( + actionContext, + modelBinder, + new SimpleValueProvider(), + parameterDescriptor, + modelMetadata, + value: null); + + // Assert + Assert.False(result.IsModelSet); + Assert.False(modelState.IsValid); + Assert.Collection( + modelState, + kvp => + { + Assert.Empty(kvp.Key); + Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState); + var error = Assert.Single(kvp.Value.Errors); + Assert.Equal("This is also not valid.", error.ErrorMessage); + }, + kvp => + { + Assert.Equal("id", kvp.Key); + Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState); + var error = Assert.Single(kvp.Value.Errors); + Assert.Equal("This is not valid.", error.ErrorMessage); + }); + } + private static ControllerContext GetControllerContext() { var services = new ServiceCollection(); @@ -813,25 +939,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding return mockValueProvider.Object; } - private static IModelValidatorProvider CreateMockValidatorProvider(IModelValidator validator = null) - { - var mockValidator = new Mock(); - mockValidator - .Setup(o => o.CreateValidators( - It.IsAny())) - .Callback(context => - { - if (validator != null) - { - foreach (var result in context.Results) - { - result.Validator = validator; - } - } - }); - return mockValidator.Object; - } - private class Person : IEquatable, IEquatable { public string Name { get; set; } @@ -862,9 +969,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding public string DerivedProperty { get; set; } } - [Required] - private Person PersonProperty { get; set; } - public abstract class FakeModelMetadata : ModelMetadata { public FakeModelMetadata()