From 96b77c866349a6d31508ce375ab98345a59604f1 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Mon, 27 Aug 2018 17:35:38 -0700 Subject: [PATCH 01/21] Fix aspnet/Routing#721 --- .../Internal/MvcEndpointDataSource.cs | 27 ++++++++++++------- .../ApplicationModelTest.cs | 2 +- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs index 560016a4c4..2d3f5e4ba2 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs @@ -174,7 +174,8 @@ namespace Microsoft.AspNetCore.Mvc.Internal endpointInfo.Defaults, ++conventionalRouteOrder, endpointInfo, - suppressLinkGeneration: false); + suppressLinkGeneration: false, + suppressPathMatching: false); endpoints.Add(subEndpoint); } @@ -213,7 +214,8 @@ namespace Microsoft.AspNetCore.Mvc.Internal endpointInfo.Defaults, ++conventionalRouteOrder, endpointInfo, - suppressLinkGeneration: false); + suppressLinkGeneration: false, + suppressPathMatching: false); endpoints.Add(endpoint); } } @@ -227,7 +229,8 @@ namespace Microsoft.AspNetCore.Mvc.Internal nonInlineDefaults: null, action.AttributeRouteInfo.Order, action.AttributeRouteInfo, - suppressLinkGeneration: action.AttributeRouteInfo.SuppressLinkGeneration); + suppressLinkGeneration: action.AttributeRouteInfo.SuppressLinkGeneration, + suppressPathMatching: action.AttributeRouteInfo.SuppressPathMatching); endpoints.Add(endpoint); } } @@ -375,7 +378,8 @@ namespace Microsoft.AspNetCore.Mvc.Internal object nonInlineDefaults, int order, object source, - bool suppressLinkGeneration) + bool suppressLinkGeneration, + bool suppressPathMatching) { RequestDelegate requestDelegate = (context) => { @@ -403,7 +407,8 @@ namespace Microsoft.AspNetCore.Mvc.Internal routeName, new RouteValueDictionary(action.RouteValues), source, - suppressLinkGeneration); + suppressLinkGeneration, + suppressPathMatching); var endpoint = new RouteEndpoint( requestDelegate, @@ -420,12 +425,11 @@ namespace Microsoft.AspNetCore.Mvc.Internal string routeName, RouteValueDictionary requiredValues, object source, - bool suppressLinkGeneration) + bool suppressLinkGeneration, + bool suppressPathMatching) { var metadata = new List { - // REVIEW: Used for debugging. Consider removing before release - source, action }; @@ -475,6 +479,11 @@ namespace Microsoft.AspNetCore.Mvc.Internal metadata.Add(new SuppressLinkGenerationMetadata()); } + if (suppressPathMatching) + { + metadata.Add(new SuppressMatchingMetadata()); + } + var metadataCollection = new EndpointMetadataCollection(metadata); return metadataCollection; } @@ -500,7 +509,5 @@ namespace Microsoft.AspNetCore.Mvc.Internal defaults[kvp.Key] = kvp.Value; } } - - private class SuppressLinkGenerationMetadata : ISuppressLinkGenerationMetadata { } } } \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApplicationModelTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApplicationModelTest.cs index dcb50e1d8e..a698af7ba4 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApplicationModelTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApplicationModelTest.cs @@ -128,7 +128,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests Assert.Equal("From Header - HelloWorld", body); } - [Fact(Skip = "https://github.com/aspnet/Routing/issues/721")] + [Fact] public async Task ActionModelSuppressedForPathMatching_CannotBeRouted() { // Arrange & Act From 17d72c2b94679ca9da9f257200b61e201a5ec42e Mon Sep 17 00:00:00 2001 From: Massimiliano Donini Date: Fri, 24 Aug 2018 09:09:27 +0200 Subject: [PATCH 02/21] Fix msbuild targets to correctly copy deps.json --- .../netstandard2.0/Microsoft.AspNetCore.Mvc.Testing.targets | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Testing/build/netstandard2.0/Microsoft.AspNetCore.Mvc.Testing.targets b/src/Microsoft.AspNetCore.Mvc.Testing/build/netstandard2.0/Microsoft.AspNetCore.Mvc.Testing.targets index 6cd039b00b..452fc9909d 100644 --- a/src/Microsoft.AspNetCore.Mvc.Testing/build/netstandard2.0/Microsoft.AspNetCore.Mvc.Testing.targets +++ b/src/Microsoft.AspNetCore.Mvc.Testing/build/netstandard2.0/Microsoft.AspNetCore.Mvc.Testing.targets @@ -53,7 +53,7 @@ Include="$([System.IO.Path]::ChangeExtension('%(_ContentRootProjectReferences.ResolvedFrom)', '.deps.json'))" /> - + \ No newline at end of file From 667ad4daff242d4a09f4ca68836e8aa3e10b5a02 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Thu, 23 Aug 2018 10:37:58 -0700 Subject: [PATCH 03/21] Use ClientErrorData to configure ClientErrorResultFilter Fixes #8289 --- .../ApiBehaviorOptions.cs | 38 +++++---- .../ClientErrorData.cs | 29 +++++++ .../MvcCoreServiceCollectionExtensions.cs | 1 + .../Infrastructure/ClientErrorResultFilter.cs | 26 +++--- .../Infrastructure/IClientErrorFactory.cs | 20 +++++ .../ProblemDetailsClientErrorFactory.cs | 44 ++++++++++ .../ApiBehaviorApplicationModelProvider.cs | 5 +- .../Internal/ApiBehaviorOptionsSetup.cs | 80 +++++++------------ .../Internal/MvcCoreLoggerExtensions.cs | 10 +-- .../ProblemDetails.cs | 4 +- .../ClientErrorResultFilterTest.cs | 42 +++------- .../ProblemDetalsClientErrorFactoryTest.cs | 65 +++++++++++++++ ...ApiBehaviorApplicationModelProviderTest.cs | 9 ++- .../Internal/ApiBehaviorOptionsSetupTest.cs | 4 +- .../ApiBehaviorTest.cs | 4 + .../CompatibilitySwitchIntegrationTest.cs | 8 +- 16 files changed, 265 insertions(+), 124 deletions(-) create mode 100644 src/Microsoft.AspNetCore.Mvc.Core/ClientErrorData.cs create mode 100644 src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IClientErrorFactory.cs create mode 100644 src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ProblemDetailsClientErrorFactory.cs create mode 100644 test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/ProblemDetalsClientErrorFactoryTest.cs diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ApiBehaviorOptions.cs b/src/Microsoft.AspNetCore.Mvc.Core/ApiBehaviorOptions.cs index 54614a2465..b2b2c2cd6f 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ApiBehaviorOptions.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ApiBehaviorOptions.cs @@ -15,7 +15,7 @@ namespace Microsoft.AspNetCore.Mvc /// public class ApiBehaviorOptions : IEnumerable { - private readonly CompatibilitySwitch _suppressUseClientErrorFactory; + private readonly CompatibilitySwitch _suppressMapClientErrors; private readonly CompatibilitySwitch _suppressUseValidationProblemDetailsForInvalidModelStateResponses; private readonly ICompatibilitySwitch[] _switches; @@ -26,11 +26,11 @@ namespace Microsoft.AspNetCore.Mvc /// public ApiBehaviorOptions() { - _suppressUseClientErrorFactory = new CompatibilitySwitch(nameof(SuppressUseClientErrorFactory)); + _suppressMapClientErrors = new CompatibilitySwitch(nameof(SuppressMapClientErrors)); _suppressUseValidationProblemDetailsForInvalidModelStateResponses = new CompatibilitySwitch(nameof(SuppressUseValidationProblemDetailsForInvalidModelStateResponses)); _switches = new[] { - _suppressUseClientErrorFactory, + _suppressMapClientErrors, _suppressUseValidationProblemDetailsForInvalidModelStateResponses, }; } @@ -71,12 +71,16 @@ namespace Microsoft.AspNetCore.Mvc public bool SuppressConsumesConstraintForFormFileParameters { get; set; } /// - /// Gets or sets a value that determines if controllers with use - /// to transform certain certain client errors. + /// Gets or sets a value that determines if controllers with + /// transform certain certain client errors. /// - /// When false, is used to transform to the value - /// specified by the factory. In the default case, this converts instances to an - /// with . + /// When false, a result filter is added to API controller actions that transforms . + /// By default, is used to map to a + /// instance (returned as the value for ). + /// + /// + /// To customize the output of the filter (for e.g. to return a different error type), register a custom + /// implementation of of in the service collection. /// /// /// @@ -102,11 +106,11 @@ namespace Microsoft.AspNetCore.Mvc /// higher then this setting will have the value unless explicitly configured. /// /// - public bool SuppressUseClientErrorFactory + public bool SuppressMapClientErrors { // Note: When compatibility switches are removed in 3.0, this property should be retained as a regular boolean property. - get => _suppressUseClientErrorFactory.Value; - set => _suppressUseClientErrorFactory.Value = value; + get => _suppressMapClientErrors.Value; + set => _suppressMapClientErrors.Value = value; } /// @@ -148,11 +152,15 @@ namespace Microsoft.AspNetCore.Mvc } /// - /// Gets a map of HTTP status codes to factories. - /// Configured factories are used when is . + /// Gets a map of HTTP status codes to . Configured values + /// are used to transform to an + /// instance where the is . + /// + /// Use of this feature can be disabled by resetting . + /// /// - public IDictionary> ClientErrorFactory { get; } = - new Dictionary>(); + public IDictionary ClientErrorMapping { get; } = + new Dictionary(); IEnumerator IEnumerable.GetEnumerator() { diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ClientErrorData.cs b/src/Microsoft.AspNetCore.Mvc.Core/ClientErrorData.cs new file mode 100644 index 0000000000..38b3448ece --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Core/ClientErrorData.cs @@ -0,0 +1,29 @@ +// 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. + +namespace Microsoft.AspNetCore.Mvc +{ + /// + /// Information for producing client errors. This type is used to configure client errors + /// produced by consumers of . + /// + public class ClientErrorData + { + /// + /// Gets or sets a link (URI) that describes the client error. + /// + /// + /// By default, this maps to . + /// + public string Link { get; set; } + + /// + /// Gets or sets the summary of the client error. + /// + /// + /// By default, this maps to and should not change + /// between multiple occurences of the same error. + /// + public string Title { get; set; } + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs b/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs index 767aa0c33f..a0c082fdf6 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs @@ -257,6 +257,7 @@ namespace Microsoft.Extensions.DependencyInjection services.TryAddSingleton, RedirectToRouteResultExecutor>(); services.TryAddSingleton, RedirectToPageResultExecutor>(); services.TryAddSingleton, ContentResultExecutor>(); + services.TryAddSingleton(); // // Route Handlers diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ClientErrorResultFilter.cs b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ClientErrorResultFilter.cs index a6482dfe09..a212c50ea4 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ClientErrorResultFilter.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ClientErrorResultFilter.cs @@ -2,7 +2,6 @@ // 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 Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Mvc.Internal; using Microsoft.Extensions.Logging; @@ -11,7 +10,7 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure { internal class ClientErrorResultFilter : IAlwaysRunResultFilter, IOrderedFilter { - private readonly IDictionary> _clientErrorFactory; + private readonly IClientErrorFactory _clientErrorFactory; private readonly ILogger _logger; /// @@ -20,10 +19,10 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure public int Order => -2000; public ClientErrorResultFilter( - ApiBehaviorOptions apiBehaviorOptions, + IClientErrorFactory clientErrorFactory, ILogger logger) { - _clientErrorFactory = apiBehaviorOptions?.ClientErrorFactory ?? throw new ArgumentNullException(nameof(apiBehaviorOptions)); + _clientErrorFactory = clientErrorFactory ?? throw new ArgumentNullException(nameof(clientErrorFactory)); _logger = logger ?? throw new ArgumentNullException(nameof(logger)); } @@ -38,16 +37,19 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure throw new ArgumentNullException(nameof(context)); } - if (context.Result is IClientErrorActionResult clientErrorActionResult && - clientErrorActionResult.StatusCode is int statusCode && - _clientErrorFactory.TryGetValue(statusCode, out var factory)) + if (!(context.Result is IClientErrorActionResult clientError)) { - var result = factory(context); - - _logger.TransformingClientError(context.Result.GetType(), result?.GetType(), statusCode); - - context.Result = factory(context); + return; } + + var result = _clientErrorFactory.GetClientError(context, clientError); + if (result == null) + { + return; + } + + _logger.TransformingClientError(context.Result.GetType(), result?.GetType(), clientError.StatusCode); + context.Result = result; } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IClientErrorFactory.cs b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IClientErrorFactory.cs new file mode 100644 index 0000000000..b592c52a9b --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IClientErrorFactory.cs @@ -0,0 +1,20 @@ +// 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. + +namespace Microsoft.AspNetCore.Mvc.Infrastructure +{ + /// + /// A factory for producing client errors. This contract is used by controllers annotated + /// with to transform . + /// + public interface IClientErrorFactory + { + /// + /// Transforms for the specified . + /// + /// The . + /// The . + /// THe that would be returned to the client. + IActionResult GetClientError(ActionContext actionContext, IClientErrorActionResult clientError); + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ProblemDetailsClientErrorFactory.cs b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ProblemDetailsClientErrorFactory.cs new file mode 100644 index 0000000000..1bf1e3ac43 --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ProblemDetailsClientErrorFactory.cs @@ -0,0 +1,44 @@ +// 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 Microsoft.Extensions.Options; + +namespace Microsoft.AspNetCore.Mvc.Infrastructure +{ + internal class ProblemDetailsClientErrorFactory : IClientErrorFactory + { + private readonly ApiBehaviorOptions _options; + + public ProblemDetailsClientErrorFactory(IOptions options) + { + _options = options?.Value ?? throw new ArgumentNullException(nameof(options)); + } + + public IActionResult GetClientError(ActionContext actionContext, IClientErrorActionResult clientError) + { + var problemDetails = new ProblemDetails + { + Status = clientError.StatusCode, + Type = "about:blank", + }; + + if (clientError.StatusCode is int statusCode && + _options.ClientErrorMapping.TryGetValue(statusCode, out var errorData)) + { + problemDetails.Title = errorData.Title; + problemDetails.Type = errorData.Link; + } + + return new ObjectResult(problemDetails) + { + StatusCode = problemDetails.Status, + ContentTypes = + { + "application/problem+json", + "application/problem+xml", + }, + }; + } + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs index bc1ac18c79..5b21f55378 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs @@ -27,6 +27,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal public ApiBehaviorApplicationModelProvider( IOptions apiBehaviorOptions, IModelMetadataProvider modelMetadataProvider, + IClientErrorFactory clientErrorFactory, ILoggerFactory loggerFactory) { _apiBehaviorOptions = apiBehaviorOptions.Value; @@ -45,7 +46,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal loggerFactory.CreateLogger()); _clientErrorResultFilter = new ClientErrorResultFilter( - _apiBehaviorOptions, + clientErrorFactory, loggerFactory.CreateLogger()); } @@ -158,7 +159,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal private void AddClientErrorFilter(ActionModel actionModel) { - if (_apiBehaviorOptions.SuppressUseClientErrorFactory) + if (_apiBehaviorOptions.SuppressMapClientErrors) { return; } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorOptionsSetup.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorOptionsSetup.cs index 15c4ef8892..4d64d9bb84 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorOptionsSetup.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorOptionsSetup.cs @@ -32,7 +32,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal if (Version < CompatibilityVersion.Version_2_2) { - dictionary[nameof(ApiBehaviorOptions.SuppressUseClientErrorFactory)] = true; + dictionary[nameof(ApiBehaviorOptions.SuppressMapClientErrors)] = true; dictionary[nameof(ApiBehaviorOptions.SuppressUseValidationProblemDetailsForInvalidModelStateResponses)] = true; } @@ -48,7 +48,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal } options.InvalidModelStateResponseFactory = DefaultFactory; - ConfigureClientErrorFactories(options); + ConfigureClientErrorMapping(options); } public override void PostConfigure(string name, ApiBehaviorOptions options) @@ -57,9 +57,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal base.PostConfigure(name, options); // We want to use problem details factory only if - // (a) it has not been opted out of (SuppressUseClientErrorFactory = true) + // (a) it has not been opted out of (SuppressMapClientErrors = true) // (b) a different factory was configured - if (!options.SuppressUseClientErrorFactory && + if (!options.SuppressMapClientErrors && object.ReferenceEquals(options.InvalidModelStateResponseFactory, DefaultFactory)) { options.InvalidModelStateResponseFactory = ProblemDetailsFactory; @@ -67,77 +67,55 @@ namespace Microsoft.AspNetCore.Mvc.Internal } // Internal for unit testing - internal static void ConfigureClientErrorFactories(ApiBehaviorOptions options) + internal static void ConfigureClientErrorMapping(ApiBehaviorOptions options) { - AddClientErrorFactory(new ProblemDetails + options.ClientErrorMapping[400] = new ClientErrorData { - Status = 400, - Type = "https://tools.ietf.org/html/rfc7231#section-6.5.1", + Link = "https://tools.ietf.org/html/rfc7231#section-6.5.1", Title = Resources.ApiConventions_Title_400, - }); + }; - AddClientErrorFactory(new ProblemDetails + options.ClientErrorMapping[401] = new ClientErrorData { - Status = 401, - Type = "https://tools.ietf.org/html/rfc7235#section-3.1", + Link = "https://tools.ietf.org/html/rfc7235#section-3.1", Title = Resources.ApiConventions_Title_401, - }); + }; - AddClientErrorFactory(new ProblemDetails + options.ClientErrorMapping[403] = new ClientErrorData { - Status = 403, - Type = "https://tools.ietf.org/html/rfc7231#section-6.5.3", + Link = "https://tools.ietf.org/html/rfc7231#section-6.5.3", Title = Resources.ApiConventions_Title_403, - }); + }; - AddClientErrorFactory(new ProblemDetails + options.ClientErrorMapping[404] = new ClientErrorData { - Status = 404, - Type = "https://tools.ietf.org/html/rfc7231#section-6.5.4", + Link = "https://tools.ietf.org/html/rfc7231#section-6.5.4", Title = Resources.ApiConventions_Title_404, - }); + }; - AddClientErrorFactory(new ProblemDetails + options.ClientErrorMapping[406] = new ClientErrorData { - Status = 406, - Type = "https://tools.ietf.org/html/rfc7231#section-6.5.6", + Link = "https://tools.ietf.org/html/rfc7231#section-6.5.6", Title = Resources.ApiConventions_Title_406, - }); + }; - AddClientErrorFactory(new ProblemDetails + options.ClientErrorMapping[409] = new ClientErrorData { - Status = 409, - Type = "https://tools.ietf.org/html/rfc7231#section-6.5.8", + Link = "https://tools.ietf.org/html/rfc7231#section-6.5.8", Title = Resources.ApiConventions_Title_409, - }); + }; - AddClientErrorFactory(new ProblemDetails + options.ClientErrorMapping[415] = new ClientErrorData { - Status = 415, - Type = "https://tools.ietf.org/html/rfc7231#section-6.5.13", + Link = "https://tools.ietf.org/html/rfc7231#section-6.5.13", Title = Resources.ApiConventions_Title_415, - }); + }; - AddClientErrorFactory(new ProblemDetails + options.ClientErrorMapping[422] = new ClientErrorData { - Status = 422, - Type = "https://tools.ietf.org/html/rfc4918#section-11.2", + Link = "https://tools.ietf.org/html/rfc4918#section-11.2", Title = Resources.ApiConventions_Title_422, - }); - - void AddClientErrorFactory(ProblemDetails problemDetails) - { - var statusCode = problemDetails.Status.Value; - options.ClientErrorFactory[statusCode] = _ => new ObjectResult(problemDetails) - { - StatusCode = statusCode, - ContentTypes = - { - "application/problem+json", - "application/problem+xml", - }, - }; - } + }; } private static IActionResult DefaultInvalidModelStateResponse(ActionContext context) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreLoggerExtensions.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreLoggerExtensions.cs index 4c2d80972e..b4a223b8b2 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreLoggerExtensions.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreLoggerExtensions.cs @@ -151,7 +151,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal private static readonly Action _notMostEffectiveFilter; private static readonly Action, Exception> _registeredOutputFormatters; - private static readonly Action _transformingClientError; + private static readonly Action _transformingClientError; static MvcCoreLoggerExtensions() { @@ -651,10 +651,10 @@ namespace Microsoft.AspNetCore.Mvc.Internal 48, "Skipped binding parameter '{ParameterName}' since its binding information disallowed it for the current request."); - _transformingClientError = LoggerMessage.Define( + _transformingClientError = LoggerMessage.Define( LogLevel.Trace, new EventId(49, nameof(Infrastructure.ClientErrorResultFilter)), - "Replacing {InitialActionResultType} with status code {StatusCode} with {ReplacedActionResultType} produced from ClientErrorFactory'."); + "Replacing {InitialActionResultType} with status code {StatusCode} with {ReplacedActionResultType}."); } public static void RegisteredOutputFormatters(this ILogger logger, IEnumerable outputFormatters) @@ -1585,9 +1585,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal } } - public static void TransformingClientError(this ILogger logger, Type initialType, Type replacedType, int statusCode) + public static void TransformingClientError(this ILogger logger, Type initialType, Type replacedType, int? statusCode) { - _transformingClientError(logger, initialType, replacedType, statusCode, null); + _transformingClientError(logger, initialType, statusCode, replacedType, null); } private static void LogFilterExecutionPlan( diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ProblemDetails.cs b/src/Microsoft.AspNetCore.Mvc.Core/ProblemDetails.cs index 35ac215562..573419b0bc 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ProblemDetails.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ProblemDetails.cs @@ -20,7 +20,7 @@ namespace Microsoft.AspNetCore.Mvc public string Type { get; set; } /// - /// A short, human-readable summary of the problem type.It SHOULD NOT change from occurrence to occurrence + /// A short, human-readable summary of the problem type. It SHOULD NOT change from occurrence to occurrence /// of the problem, except for purposes of localization(e.g., using proactive content negotiation; /// see[RFC7231], Section 3.4). /// @@ -37,7 +37,7 @@ namespace Microsoft.AspNetCore.Mvc public string Detail { get; set; } /// - /// A URI reference that identifies the specific occurrence of the problem.It may or may not yield further information if dereferenced. + /// A URI reference that identifies the specific occurrence of the problem. It may or may not yield further information if dereferenced. /// public string Instance { get; set; } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/ClientErrorResultFilterTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/ClientErrorResultFilterTest.cs index 7c3f55858d..83098744ff 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/ClientErrorResultFilterTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/ClientErrorResultFilterTest.cs @@ -32,35 +32,25 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure } [Fact] - public void OnResultExecuting_DoesNothing_IfStatusCodeDoesNotExistInApiBehaviorOptions() + public void OnResultExecuting_DoesNothing_IfTransformedValueIsNull() { // Arrange var actionResult = new NotFoundResult(); var context = GetContext(actionResult); - var filter = GetFilter(new ApiBehaviorOptions()); - - // Act - filter.OnResultExecuting(context); - - // Assert - Assert.Same(actionResult, context.Result); - } - - [Fact] - public void OnResultExecuting_DoesNothing_IfResultDoesNotHaveStatusCode() - { - // Arrange - var actionResult = new Mock() - .As() - .Object; - var context = GetContext(actionResult); - var filter = GetFilter(new ApiBehaviorOptions()); + var factory = new Mock(); + factory + .Setup(f => f.GetClientError(It.IsAny(), It.IsAny())) + .Returns((IActionResult)null) + .Verifiable(); + + var filter = new ClientErrorResultFilter(factory.Object, NullLogger.Instance); // Act filter.OnResultExecuting(context); // Assert Assert.Same(actionResult, context.Result); + factory.Verify(); } [Fact] @@ -78,18 +68,12 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure Assert.Same(Result, context.Result); } - private static ClientErrorResultFilter GetFilter(ApiBehaviorOptions options = null) + private static ClientErrorResultFilter GetFilter() { - var apiBehaviorOptions = options ?? GetOptions(); - var filter = new ClientErrorResultFilter(apiBehaviorOptions, NullLogger.Instance); - return filter; - } + var factory = Mock.Of( + f => f.GetClientError(It.IsAny(), It.IsAny()) == Result); - private static ApiBehaviorOptions GetOptions() - { - var apiBehaviorOptions = new ApiBehaviorOptions(); - apiBehaviorOptions.ClientErrorFactory[404] = _ => Result; - return apiBehaviorOptions; + return new ClientErrorResultFilter(factory, NullLogger.Instance); } private static ResultExecutingContext GetContext(IActionResult actionResult) diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/ProblemDetalsClientErrorFactoryTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/ProblemDetalsClientErrorFactoryTest.cs new file mode 100644 index 0000000000..65a53a8158 --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/ProblemDetalsClientErrorFactoryTest.cs @@ -0,0 +1,65 @@ +// 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.Extensions.Options; +using Xunit; + +namespace Microsoft.AspNetCore.Mvc.Infrastructure +{ + public class ProblemDetalsClientErrorFactoryTest + { + [Fact] + public void GetClientError_ReturnsProblemDetails_IfNoMappingWasFound() + { + // Arrange + var clientError = new UnsupportedMediaTypeResult(); + var factory = new ProblemDetailsClientErrorFactory(Options.Create(new ApiBehaviorOptions + { + ClientErrorMapping = + { + [405] = new ClientErrorData { Link = "Some link", Title = "Summary" }, + }, + })); + + // Act + var result = factory.GetClientError(new ActionContext(), clientError); + + // Assert + var objectResult = Assert.IsType(result); + Assert.Equal(new[] { "application/problem+json", "application/problem+xml" }, objectResult.ContentTypes); + var problemDetails = Assert.IsType(objectResult.Value); + Assert.Equal(415, problemDetails.Status); + Assert.Equal("about:blank", problemDetails.Type); + Assert.Null(problemDetails.Title); + Assert.Null(problemDetails.Detail); + Assert.Null(problemDetails.Instance); + } + + [Fact] + public void GetClientError_ReturnsProblemDetails() + { + // Arrange + var clientError = new UnsupportedMediaTypeResult(); + var factory = new ProblemDetailsClientErrorFactory(Options.Create(new ApiBehaviorOptions + { + ClientErrorMapping = + { + [415] = new ClientErrorData { Link = "Some link", Title = "Summary" }, + }, + })); + + // Act + var result = factory.GetClientError(new ActionContext(), clientError); + + // Assert + var objectResult = Assert.IsType(result); + Assert.Equal(new[] { "application/problem+json", "application/problem+xml" }, objectResult.ContentTypes); + var problemDetails = Assert.IsType(objectResult.Value); + Assert.Equal(415, problemDetails.Status); + Assert.Equal("Some link", problemDetails.Type); + Assert.Equal("Summary", problemDetails.Title); + Assert.Null(problemDetails.Detail); + Assert.Null(problemDetails.Instance); + } + } +} diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs index 3b5d4415de..c0c415573f 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs @@ -17,6 +17,7 @@ using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.ModelBinding; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; +using Moq; using Xunit; namespace Microsoft.AspNetCore.Mvc.Internal @@ -1081,7 +1082,7 @@ Environment.NewLine + "int b"; var context = GetContext(typeof(TestApiController)); var options = new ApiBehaviorOptions { - SuppressUseClientErrorFactory = true, + SuppressMapClientErrors = true, InvalidModelStateResponseFactory = _ => null, }; var provider = GetProvider(options); @@ -1122,7 +1123,11 @@ Environment.NewLine + "int b"; var loggerFactory = NullLoggerFactory.Instance; modelMetadataProvider = modelMetadataProvider ?? new EmptyModelMetadataProvider(); - return new ApiBehaviorApplicationModelProvider(optionsAccessor, modelMetadataProvider, loggerFactory); + return new ApiBehaviorApplicationModelProvider( + optionsAccessor, + modelMetadataProvider, + Mock.Of(), + loggerFactory); } private static ApplicationModelProviderContext GetContext( diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorOptionsSetupTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorOptionsSetupTest.cs index 66f4f7933b..622a36e5ff 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorOptionsSetupTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorOptionsSetupTest.cs @@ -28,7 +28,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal } [Fact] - public void Configure_AddsClientErrorFactories() + public void Configure_AddsClientErrorMappings() { // Arrange var expected = new[] { 400, 401, 403, 404, 406, 409, 415, 422, }; @@ -41,7 +41,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal optionsSetup.Configure(options); // Assert - Assert.Equal(expected, options.ClientErrorFactory.Keys); + Assert.Equal(expected, options.ClientErrorMapping.Keys); } [Fact] diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiBehaviorTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiBehaviorTest.cs index 0d0a85999f..d3ec4d9e29 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiBehaviorTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiBehaviorTest.cs @@ -97,6 +97,10 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests // Assert await response.AssertStatusCodeAsync(HttpStatusCode.UnsupportedMediaType); + var content = await response.Content.ReadAsStringAsync(); + var problemDetails = JsonConvert.DeserializeObject(content); + Assert.Equal((int)HttpStatusCode.UnsupportedMediaType, problemDetails.Status); + Assert.Equal("Unsupported Media Type", problemDetails.Title); } [Fact] diff --git a/test/Microsoft.AspNetCore.Mvc.Test/IntegrationTest/CompatibilitySwitchIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.Test/IntegrationTest/CompatibilitySwitchIntegrationTest.cs index 813c148bfe..2413eb34bf 100644 --- a/test/Microsoft.AspNetCore.Mvc.Test/IntegrationTest/CompatibilitySwitchIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Test/IntegrationTest/CompatibilitySwitchIntegrationTest.cs @@ -43,7 +43,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTest Assert.False(mvcOptions.EnableEndpointRouting); Assert.Null(mvcOptions.MaxValidationDepth); Assert.True(apiBehaviorOptions.SuppressUseValidationProblemDetailsForInvalidModelStateResponses); - Assert.True(apiBehaviorOptions.SuppressUseClientErrorFactory); + Assert.True(apiBehaviorOptions.SuppressMapClientErrors); } [Fact] @@ -72,7 +72,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTest Assert.False(mvcOptions.EnableEndpointRouting); Assert.Null(mvcOptions.MaxValidationDepth); Assert.True(apiBehaviorOptions.SuppressUseValidationProblemDetailsForInvalidModelStateResponses); - Assert.True(apiBehaviorOptions.SuppressUseClientErrorFactory); + Assert.True(apiBehaviorOptions.SuppressMapClientErrors); } [Fact] @@ -101,7 +101,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTest Assert.True(mvcOptions.EnableEndpointRouting); Assert.Equal(32, mvcOptions.MaxValidationDepth); Assert.False(apiBehaviorOptions.SuppressUseValidationProblemDetailsForInvalidModelStateResponses); - Assert.False(apiBehaviorOptions.SuppressUseClientErrorFactory); + Assert.False(apiBehaviorOptions.SuppressMapClientErrors); } [Fact] @@ -130,7 +130,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTest Assert.True(mvcOptions.EnableEndpointRouting); Assert.Equal(32, mvcOptions.MaxValidationDepth); Assert.False(apiBehaviorOptions.SuppressUseValidationProblemDetailsForInvalidModelStateResponses); - Assert.False(apiBehaviorOptions.SuppressUseClientErrorFactory); + Assert.False(apiBehaviorOptions.SuppressMapClientErrors); } // This just does the minimum needed to be able to resolve these options. From d09c3c9e2889e9e1c41cdcf3b45f3d724f98fc9f Mon Sep 17 00:00:00 2001 From: Pranav K Date: Fri, 24 Aug 2018 11:12:28 -0700 Subject: [PATCH 04/21] Polish ProblemDetails * Add ability to set extended members on ProblemDetails * Skip empty valued properties when serializing ProblemDetails Fixes #8296 Fixes #8317 --- .../ProblemDetails.cs | 24 +- .../ProblemDetailsWrapper.cs | 187 +++++++++++++++ .../Properties/AssemblyInfo.cs | 6 + .../SerializableErrorWrapper.cs | 2 +- .../ValidationProblemDetailsWrapper.cs | 126 ++++++++++ .../WrapperProviderFactoriesExtensions.cs | 19 ++ .../WrapperProviderFactory.cs | 50 ++++ ...XmlDataContractSerializerInputFormatter.cs | 3 +- ...mlDataContractSerializerOutputFormatter.cs | 3 +- .../XmlSerializerInputFormatter.cs | 3 +- .../XmlSerializerOutputFormatter.cs | 3 +- .../ProblemDetailsWrapperTest.cs | 100 ++++++++ .../ValidationProblemDetailsWrapperTest.cs | 226 ++++++++++++++++++ .../WrapperProviderFactoryExtensionsTest.cs | 34 +++ .../WrapperProviderFactoryTest.cs | 63 +++++ .../ApiBehaviorTest.cs | 62 ++++- .../Infrastructure/HttpClientExtensions.cs | 4 +- ...ontractSerializerFormattersWrappingTest.cs | 65 ++++- .../XmlSerializerFormattersWrappingTest.cs | 63 +++++ .../Controllers/ContactApiController.cs | 35 +++ .../Controllers/XmlApiControllerBase.cs | 55 +++++ .../XmlDataContractApiController.cs | 29 +++ .../Controllers/XmlSerializedApiController.cs | 29 +++ 23 files changed, 1175 insertions(+), 16 deletions(-) create mode 100644 src/Microsoft.AspNetCore.Mvc.Formatters.Xml/ProblemDetailsWrapper.cs create mode 100644 src/Microsoft.AspNetCore.Mvc.Formatters.Xml/Properties/AssemblyInfo.cs create mode 100644 src/Microsoft.AspNetCore.Mvc.Formatters.Xml/ValidationProblemDetailsWrapper.cs create mode 100644 src/Microsoft.AspNetCore.Mvc.Formatters.Xml/WrapperProviderFactory.cs create mode 100644 test/Microsoft.AspNetCore.Mvc.Formatters.Xml.Test/ProblemDetailsWrapperTest.cs create mode 100644 test/Microsoft.AspNetCore.Mvc.Formatters.Xml.Test/ValidationProblemDetailsWrapperTest.cs create mode 100644 test/Microsoft.AspNetCore.Mvc.Formatters.Xml.Test/WrapperProviderFactoryExtensionsTest.cs create mode 100644 test/Microsoft.AspNetCore.Mvc.Formatters.Xml.Test/WrapperProviderFactoryTest.cs create mode 100644 test/WebSites/XmlFormattersWebSite/Controllers/XmlApiControllerBase.cs create mode 100644 test/WebSites/XmlFormattersWebSite/Controllers/XmlDataContractApiController.cs create mode 100644 test/WebSites/XmlFormattersWebSite/Controllers/XmlSerializedApiController.cs diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ProblemDetails.cs b/src/Microsoft.AspNetCore.Mvc.Core/ProblemDetails.cs index 573419b0bc..44b816aa05 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ProblemDetails.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ProblemDetails.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using Newtonsoft.Json; namespace Microsoft.AspNetCore.Mvc { @@ -17,28 +18,47 @@ namespace Microsoft.AspNetCore.Mvc /// (e.g., using HTML [W3C.REC-html5-20141028]). When this member is not present, its value is assumed to be /// "about:blank". /// + [JsonProperty(NullValueHandling = NullValueHandling.Ignore)] public string Type { get; set; } /// - /// A short, human-readable summary of the problem type. It SHOULD NOT change from occurrence to occurrence + /// A short, human-readable summary of the problem type.It SHOULD NOT change from occurrence to occurrence /// of the problem, except for purposes of localization(e.g., using proactive content negotiation; /// see[RFC7231], Section 3.4). /// + [JsonProperty(NullValueHandling = NullValueHandling.Ignore)] public string Title { get; set; } /// /// The HTTP status code([RFC7231], Section 6) generated by the origin server for this occurrence of the problem. /// + [JsonProperty(NullValueHandling = NullValueHandling.Ignore)] public int? Status { get; set; } /// /// A human-readable explanation specific to this occurrence of the problem. /// + [JsonProperty(NullValueHandling = NullValueHandling.Ignore)] public string Detail { get; set; } /// - /// A URI reference that identifies the specific occurrence of the problem. It may or may not yield further information if dereferenced. + /// A URI reference that identifies the specific occurrence of the problem.It may or may not yield further information if dereferenced. /// + [JsonProperty(NullValueHandling = NullValueHandling.Ignore)] public string Instance { get; set; } + + /// + /// Gets the for extension members. + /// + /// Problem type definitions MAY extend the problem details object with additional members. Extension members appear in the same namespace as + /// other members of a problem type. + /// + /// + /// + /// The round-tripping behavior for is determined by the implementation of the Input \ Output formatters. + /// In particular, complex types or collection types may not round-trip to the original type when using the built-in JSON or XML formatters. + /// + [JsonExtensionData] + public IDictionary Extensions { get; } = new Dictionary(StringComparer.Ordinal); } } diff --git a/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/ProblemDetailsWrapper.cs b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/ProblemDetailsWrapper.cs new file mode 100644 index 0000000000..30775bd1b1 --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/ProblemDetailsWrapper.cs @@ -0,0 +1,187 @@ +// 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.Globalization; +using System.Xml; +using System.Xml.Schema; +using System.Xml.Serialization; + +namespace Microsoft.AspNetCore.Mvc.Formatters.Xml +{ + /// + /// Wrapper class for to enable it to be serialized by the xml formatters. + /// + [XmlRoot(nameof(ProblemDetails))] + public class ProblemDetailsWrapper : IXmlSerializable, IUnwrappable + { + /// + /// Key used to represent dictionary elements with empty keys + /// + protected static readonly string EmptyKey = SerializableErrorWrapper.EmptyKey; + + /// + /// Initializes a new instance of . + /// + public ProblemDetailsWrapper() + : this(new ProblemDetails()) + { + } + + /// + /// Initializes a new instance of . + /// + public ProblemDetailsWrapper(ProblemDetails problemDetails) + { + ProblemDetails = problemDetails; + } + + internal ProblemDetails ProblemDetails { get; } + + /// + public XmlSchema GetSchema() => null; + + /// + public virtual void ReadXml(XmlReader reader) + { + if (reader == null) + { + throw new ArgumentNullException(nameof(reader)); + } + + if (reader.IsEmptyElement) + { + reader.Read(); + return; + } + + reader.ReadStartElement(); + while (reader.NodeType != XmlNodeType.EndElement) + { + var key = XmlConvert.DecodeName(reader.LocalName); + ReadValue(reader, key); + + reader.MoveToContent(); + } + + reader.ReadEndElement(); + } + + /// + /// Reads the value for the specified from the . + /// + /// The . + /// The name of the node. + protected virtual void ReadValue(XmlReader reader, string name) + { + if (reader == null) + { + throw new ArgumentNullException(nameof(reader)); + } + + var value = reader.ReadInnerXml(); + + switch (name) + { + case nameof(ProblemDetails.Detail): + ProblemDetails.Detail = value; + break; + + case nameof(ProblemDetails.Instance): + ProblemDetails.Instance = value; + break; + + case nameof(ProblemDetails.Status): + ProblemDetails.Status = string.IsNullOrEmpty(value) ? + (int?)null : + int.Parse(value, CultureInfo.InvariantCulture); + break; + + case nameof(ProblemDetails.Title): + ProblemDetails.Title = value; + break; + + case nameof(ProblemDetails.Type): + ProblemDetails.Type = value; + break; + + default: + if (string.Equals(name, EmptyKey, StringComparison.Ordinal)) + { + name = string.Empty; + } + + ProblemDetails.Extensions.Add(name, value); + break; + } + } + + /// + public virtual void WriteXml(XmlWriter writer) + { + if (!string.IsNullOrEmpty(ProblemDetails.Detail)) + { + writer.WriteElementString( + XmlConvert.EncodeLocalName(nameof(ProblemDetails.Detail)), + ProblemDetails.Detail); + } + + if (!string.IsNullOrEmpty(ProblemDetails.Instance)) + { + writer.WriteElementString( + XmlConvert.EncodeLocalName(nameof(ProblemDetails.Instance)), + ProblemDetails.Instance); + } + + if (ProblemDetails.Status.HasValue) + { + writer.WriteStartElement(XmlConvert.EncodeLocalName(nameof(ProblemDetails.Status))); + writer.WriteValue(ProblemDetails.Status.Value); + writer.WriteEndElement(); + } + + if (!string.IsNullOrEmpty(ProblemDetails.Title)) + { + writer.WriteElementString( + XmlConvert.EncodeLocalName(nameof(ProblemDetails.Title)), + ProblemDetails.Title); + } + + if (!string.IsNullOrEmpty(ProblemDetails.Type)) + { + writer.WriteElementString( + XmlConvert.EncodeLocalName(nameof(ProblemDetails.Type)), + ProblemDetails.Type); + } + + foreach (var keyValuePair in ProblemDetails.Extensions) + { + var key = keyValuePair.Key; + var value = keyValuePair.Value; + + if (string.IsNullOrEmpty(key)) + { + key = EmptyKey; + } + + writer.WriteStartElement(XmlConvert.EncodeLocalName(key)); + if (value != null) + { + writer.WriteValue(value); + } + + writer.WriteEndElement(); + } + } + + object IUnwrappable.Unwrap(Type declaredType) + { + if (declaredType == null) + { + throw new ArgumentNullException(nameof(declaredType)); + } + + return ProblemDetails; + } + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/Properties/AssemblyInfo.cs b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/Properties/AssemblyInfo.cs new file mode 100644 index 0000000000..61797e230b --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/Properties/AssemblyInfo.cs @@ -0,0 +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.Runtime.CompilerServices; + +[assembly: InternalsVisibleTo("Microsoft.AspNetCore.Mvc.Formatters.Xml.Test, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")] diff --git a/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/SerializableErrorWrapper.cs b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/SerializableErrorWrapper.cs index e8a28f576f..8a72825a80 100644 --- a/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/SerializableErrorWrapper.cs +++ b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/SerializableErrorWrapper.cs @@ -16,7 +16,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Xml { // Element name used when ModelStateEntry's Key is empty. Dash in element name should avoid collisions with // other ModelState entries because the character is not legal in an expression name. - private static readonly string EmptyKey = "MVC-Empty"; + internal static readonly string EmptyKey = "MVC-Empty"; // Note: XmlSerializer requires to have default constructor public SerializableErrorWrapper() diff --git a/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/ValidationProblemDetailsWrapper.cs b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/ValidationProblemDetailsWrapper.cs new file mode 100644 index 0000000000..b8787ee0e0 --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/ValidationProblemDetailsWrapper.cs @@ -0,0 +1,126 @@ +// 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.Xml; +using System.Xml.Serialization; + +namespace Microsoft.AspNetCore.Mvc.Formatters.Xml +{ + /// + /// Wrapper class for to enable it to be serialized by the xml formatters. + /// + [XmlRoot(nameof(ValidationProblemDetails))] + public class ValidationProblemDetailsWrapper : ProblemDetailsWrapper, IUnwrappable + { + private static readonly string ErrorKey = "MVC-Errors"; + + /// + /// Initializes a new instance of . + /// + public ValidationProblemDetailsWrapper() + : this(new ValidationProblemDetails()) + { + } + + /// + /// Initializes a new instance of for the specified + /// . + /// + /// The . + public ValidationProblemDetailsWrapper(ValidationProblemDetails problemDetails) + : base(problemDetails) + { + ProblemDetails = problemDetails; + } + + internal new ValidationProblemDetails ProblemDetails { get; } + + /// + protected override void ReadValue(XmlReader reader, string name) + { + if (reader == null) + { + throw new ArgumentNullException(nameof(reader)); + } + + if (string.Equals(name, ErrorKey, StringComparison.Ordinal)) + { + reader.Read(); + ReadErrorProperty(reader); + } + else + { + base.ReadValue(reader, name); + } + } + + private void ReadErrorProperty(XmlReader reader) + { + if (reader.IsEmptyElement) + { + return; + } + + while (reader.NodeType != XmlNodeType.EndElement) + { + var key = XmlConvert.DecodeName(reader.LocalName); + var value = reader.ReadInnerXml(); + if (string.Equals(EmptyKey, key, StringComparison.Ordinal)) + { + key = string.Empty; + } + + ProblemDetails.Errors.Add(key, new[] { value }); + reader.MoveToContent(); + } + } + + /// + public override void WriteXml(XmlWriter writer) + { + if (writer == null) + { + throw new ArgumentNullException(nameof(writer)); + } + + base.WriteXml(writer); + + if (ProblemDetails.Errors.Count == 0) + { + return; + } + + writer.WriteStartElement(XmlConvert.EncodeLocalName(ErrorKey)); + + foreach (var keyValuePair in ProblemDetails.Errors) + { + var key = keyValuePair.Key; + var value = keyValuePair.Value; + if (string.IsNullOrEmpty(key)) + { + key = EmptyKey; + } + + writer.WriteStartElement(XmlConvert.EncodeLocalName(key)); + if (value != null) + { + writer.WriteValue(value); + } + + writer.WriteEndElement(); + } + writer.WriteEndElement(); + } + + object IUnwrappable.Unwrap(Type declaredType) + { + if (declaredType == null) + { + throw new ArgumentNullException(nameof(declaredType)); + } + + return ProblemDetails; + } + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/WrapperProviderFactoriesExtensions.cs b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/WrapperProviderFactoriesExtensions.cs index 1ccea1645d..6ff62a8ec0 100644 --- a/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/WrapperProviderFactoriesExtensions.cs +++ b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/WrapperProviderFactoriesExtensions.cs @@ -44,5 +44,24 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Xml return null; } + + internal static IList GetDefaultProviderFactories() + { + var wrapperProviderFactories = new List(); + + wrapperProviderFactories.Add(new SerializableErrorWrapperProviderFactory()); + + wrapperProviderFactories.Add(new WrapperProviderFactory( + typeof(ProblemDetails), + typeof(ProblemDetailsWrapper), + value => new ProblemDetailsWrapper((ProblemDetails)value))); + + wrapperProviderFactories.Add(new WrapperProviderFactory( + typeof(ValidationProblemDetails), + typeof(ValidationProblemDetailsWrapper), + value => new ValidationProblemDetailsWrapper((ValidationProblemDetails)value))); + + return wrapperProviderFactories; + } } } \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/WrapperProviderFactory.cs b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/WrapperProviderFactory.cs new file mode 100644 index 0000000000..3f7c4a48af --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/WrapperProviderFactory.cs @@ -0,0 +1,50 @@ +// 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; + +namespace Microsoft.AspNetCore.Mvc.Formatters.Xml +{ + internal class WrapperProviderFactory : IWrapperProviderFactory + { + public WrapperProviderFactory(Type declaredType, Type wrappingType, Func wrapper) + { + DeclaredType = declaredType; + WrappingType = wrappingType; + Wrapper = wrapper; + } + + public Type DeclaredType { get; } + + public Type WrappingType { get; } + + public Func Wrapper { get; } + + public IWrapperProvider GetProvider(WrapperProviderContext context) + { + if (context.DeclaredType == DeclaredType) + { + return new WrapperProvider(this); + } + + return null; + } + + private class WrapperProvider : IWrapperProvider + { + private readonly WrapperProviderFactory _wrapperFactory; + + public WrapperProvider(WrapperProviderFactory wrapperFactory) + { + _wrapperFactory = wrapperFactory; + } + + public Type WrappingType => _wrapperFactory.WrappingType; + + public object Wrap(object original) + { + return _wrapperFactory.Wrapper(original); + } + } + } +} \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlDataContractSerializerInputFormatter.cs b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlDataContractSerializerInputFormatter.cs index 7916715002..b5d6d74a22 100644 --- a/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlDataContractSerializerInputFormatter.cs +++ b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlDataContractSerializerInputFormatter.cs @@ -46,8 +46,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters _serializerSettings = new DataContractSerializerSettings(); - WrapperProviderFactories = new List(); - WrapperProviderFactories.Add(new SerializableErrorWrapperProviderFactory()); + WrapperProviderFactories = WrapperProviderFactoriesExtensions.GetDefaultProviderFactories(); } /// diff --git a/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlDataContractSerializerOutputFormatter.cs b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlDataContractSerializerOutputFormatter.cs index a0f374cc27..6a312d903d 100644 --- a/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlDataContractSerializerOutputFormatter.cs +++ b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlDataContractSerializerOutputFormatter.cs @@ -76,9 +76,8 @@ namespace Microsoft.AspNetCore.Mvc.Formatters _serializerSettings = new DataContractSerializerSettings(); - WrapperProviderFactories = new List(); + WrapperProviderFactories = WrapperProviderFactoriesExtensions.GetDefaultProviderFactories(); WrapperProviderFactories.Add(new EnumerableWrapperProviderFactory(WrapperProviderFactories)); - WrapperProviderFactories.Add(new SerializableErrorWrapperProviderFactory()); _logger = loggerFactory?.CreateLogger(GetType()); } diff --git a/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlSerializerInputFormatter.cs b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlSerializerInputFormatter.cs index 6708c81257..2c1ea40bbd 100644 --- a/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlSerializerInputFormatter.cs +++ b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlSerializerInputFormatter.cs @@ -43,8 +43,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters SupportedMediaTypes.Add(MediaTypeHeaderValues.TextXml); SupportedMediaTypes.Add(MediaTypeHeaderValues.ApplicationAnyXmlSyntax); - WrapperProviderFactories = new List(); - WrapperProviderFactories.Add(new SerializableErrorWrapperProviderFactory()); + WrapperProviderFactories = WrapperProviderFactoriesExtensions.GetDefaultProviderFactories(); } /// diff --git a/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlSerializerOutputFormatter.cs b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlSerializerOutputFormatter.cs index c71be18264..e24b2c9d20 100644 --- a/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlSerializerOutputFormatter.cs +++ b/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlSerializerOutputFormatter.cs @@ -73,9 +73,8 @@ namespace Microsoft.AspNetCore.Mvc.Formatters WriterSettings = writerSettings; - WrapperProviderFactories = new List(); + WrapperProviderFactories = WrapperProviderFactoriesExtensions.GetDefaultProviderFactories(); WrapperProviderFactories.Add(new EnumerableWrapperProviderFactory(WrapperProviderFactories)); - WrapperProviderFactories.Add(new SerializableErrorWrapperProviderFactory()); _logger = loggerFactory?.CreateLogger(GetType()); } diff --git a/test/Microsoft.AspNetCore.Mvc.Formatters.Xml.Test/ProblemDetailsWrapperTest.cs b/test/Microsoft.AspNetCore.Mvc.Formatters.Xml.Test/ProblemDetailsWrapperTest.cs new file mode 100644 index 0000000000..b6760f0f15 --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.Formatters.Xml.Test/ProblemDetailsWrapperTest.cs @@ -0,0 +1,100 @@ +// 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.IO; +using System.Linq; +using System.Runtime.Serialization; +using System.Text; +using System.Xml; +using Xunit; + +namespace Microsoft.AspNetCore.Mvc.Formatters.Xml +{ + public class ProblemDetailsWrapperTest + { + [Fact] + public void ReadXml_ReadsProblemDetailsXml() + { + // Arrange + var xml = "" + + "" + + "Some title" + + "403" + + "Some instance" + + "Test Value 1" + + "<_x005B_key2_x005D_>Test Value 2" + + "Test Value 3" + + ""; + var serializer = new DataContractSerializer(typeof(ProblemDetailsWrapper)); + + // Act + var value = serializer.ReadObject( + new MemoryStream(Encoding.UTF8.GetBytes(xml))); + + // Assert + var problemDetails = Assert.IsType(value).ProblemDetails; + Assert.Equal("Some title", problemDetails.Title); + Assert.Equal("Some instance", problemDetails.Instance); + Assert.Equal(403, problemDetails.Status); + + Assert.Collection( + problemDetails.Extensions.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Empty(kvp.Key); + Assert.Equal("Test Value 3", kvp.Value); + }, + kvp => + { + Assert.Equal("[key2]", kvp.Key); + Assert.Equal("Test Value 2", kvp.Value); + }, + kvp => + { + Assert.Equal("key1", kvp.Key); + Assert.Equal("Test Value 1", kvp.Value); + }); + } + + + [Fact] + public void WriteXml_WritesValidXml() + { + // Arrange + var problemDetails = new ProblemDetails + { + Title = "Some title", + Detail = "Some detail", + Extensions = + { + ["key1"] = "Test Value 1", + ["[Key2]"] = "Test Value 2", + [""] = "Test Value 3", + }, + }; + + var wrapper = new ProblemDetailsWrapper(problemDetails); + var outputStream = new MemoryStream(); + var expectedContent = "" + + "" + + "Some detail" + + "Some title" + + "Test Value 1" + + "<_x005B_Key2_x005D_>Test Value 2" + + "Test Value 3" + + ""; + + // Act + using (var xmlWriter = XmlWriter.Create(outputStream)) + { + var dataContractSerializer = new DataContractSerializer(wrapper.GetType()); + dataContractSerializer.WriteObject(xmlWriter, wrapper); + } + outputStream.Position = 0; + var res = new StreamReader(outputStream, Encoding.UTF8).ReadToEnd(); + + // Assert + Assert.Equal(expectedContent, res); + } + } +} diff --git a/test/Microsoft.AspNetCore.Mvc.Formatters.Xml.Test/ValidationProblemDetailsWrapperTest.cs b/test/Microsoft.AspNetCore.Mvc.Formatters.Xml.Test/ValidationProblemDetailsWrapperTest.cs new file mode 100644 index 0000000000..50630a0082 --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.Formatters.Xml.Test/ValidationProblemDetailsWrapperTest.cs @@ -0,0 +1,226 @@ +// 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.IO; +using System.Linq; +using System.Runtime.Serialization; +using System.Text; +using System.Xml; +using Xunit; + +namespace Microsoft.AspNetCore.Mvc.Formatters.Xml +{ + public class ValidationProblemDetailsWrapperTest + { + [Fact] + public void ReadXml_ReadsValidationProblemDetailsXml() + { + // Arrange + var xml = "" + + "" + + "Some title" + + "400" + + "Some instance" + + "Test Value 1" + + "<_x005B_key2_x005D_>Test Value 2" + + "" + + "Test error 1 Test error 2" + + "<_x005B_error2_x005D_>Test error 3" + + "Test error 4" + + "" + + ""; + var serializer = new DataContractSerializer(typeof(ValidationProblemDetailsWrapper)); + + // Act + var value = serializer.ReadObject( + new MemoryStream(Encoding.UTF8.GetBytes(xml))); + + // Assert + var problemDetails = Assert.IsType(value).ProblemDetails; + Assert.Equal("Some title", problemDetails.Title); + Assert.Equal("Some instance", problemDetails.Instance); + Assert.Equal(400, problemDetails.Status); + + Assert.Collection( + problemDetails.Extensions.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("[key2]", kvp.Key); + Assert.Equal("Test Value 2", kvp.Value); + }, + kvp => + { + Assert.Equal("key1", kvp.Key); + Assert.Equal("Test Value 1", kvp.Value); + }); + + Assert.Collection( + problemDetails.Errors.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Empty(kvp.Key); + Assert.Equal(new[] { "Test error 4" }, kvp.Value); + }, + kvp => + { + Assert.Equal("[error2]", kvp.Key); + Assert.Equal(new[] { "Test error 3" }, kvp.Value); + }, + kvp => + { + Assert.Equal("error1", kvp.Key); + Assert.Equal(new[] { "Test error 1 Test error 2" }, kvp.Value); + }); + } + + [Fact] + public void ReadXml_ReadsValidationProblemDetailsXml_WithNoErrors() + { + // Arrange + var xml = "" + + "" + + "Some title" + + "400" + + "Some instance" + + "Test Value 1" + + "<_x005B_key2_x005D_>Test Value 2" + + ""; + var serializer = new DataContractSerializer(typeof(ValidationProblemDetailsWrapper)); + + // Act + var value = serializer.ReadObject( + new MemoryStream(Encoding.UTF8.GetBytes(xml))); + + // Assert + var problemDetails = Assert.IsType(value).ProblemDetails; + Assert.Equal("Some title", problemDetails.Title); + Assert.Equal("Some instance", problemDetails.Instance); + Assert.Equal(400, problemDetails.Status); + + Assert.Collection( + problemDetails.Extensions, + kvp => + { + Assert.Equal("key1", kvp.Key); + Assert.Equal("Test Value 1", kvp.Value); + }, + kvp => + { + Assert.Equal("[key2]", kvp.Key); + Assert.Equal("Test Value 2", kvp.Value); + }); + + Assert.Empty(problemDetails.Errors); + } + + [Fact] + public void ReadXml_ReadsValidationProblemDetailsXml_WithEmptyErrorsElement() + { + // Arrange + var xml = "" + + "" + + "Some title" + + "400" + + "" + + ""; + var serializer = new DataContractSerializer(typeof(ValidationProblemDetailsWrapper)); + + // Act + var value = serializer.ReadObject( + new MemoryStream(Encoding.UTF8.GetBytes(xml))); + + // Assert + var problemDetails = Assert.IsType(value).ProblemDetails; + Assert.Equal("Some title", problemDetails.Title); + Assert.Equal(400, problemDetails.Status); + Assert.Empty(problemDetails.Errors); + } + + [Fact] + public void WriteXml_WritesValidXml() + { + // Arrange + var problemDetails = new ValidationProblemDetails + { + Title = "Some title", + Detail = "Some detail", + Extensions = + { + ["key1"] = "Test Value 1", + ["[Key2]"] = "Test Value 2" + }, + Errors = + { + { "error1", new[] {"Test error 1", "Test error 2" } }, + { "[error2]", new[] {"Test error 3" } }, + { "", new[] { "Test error 4" } }, + } + }; + + var wrapper = new ValidationProblemDetailsWrapper(problemDetails); + var outputStream = new MemoryStream(); + var expectedContent = "" + + "" + + "Some detail" + + "Some title" + + "Test Value 1" + + "<_x005B_Key2_x005D_>Test Value 2" + + "" + + "Test error 1 Test error 2" + + "<_x005B_error2_x005D_>Test error 3" + + "Test error 4" + + "" + + ""; + + // Act + using (var xmlWriter = XmlWriter.Create(outputStream)) + { + var dataContractSerializer = new DataContractSerializer(wrapper.GetType()); + dataContractSerializer.WriteObject(xmlWriter, wrapper); + } + outputStream.Position = 0; + var res = new StreamReader(outputStream, Encoding.UTF8).ReadToEnd(); + + // Assert + Assert.Equal(expectedContent, res); + } + + [Fact] + public void WriteXml_WithNoValidationErrors() + { + // Arrange + var problemDetails = new ValidationProblemDetails + { + Title = "Some title", + Detail = "Some detail", + Extensions = + { + ["key1"] = "Test Value 1", + ["[Key2]"] = "Test Value 2" + }, + }; + + var wrapper = new ValidationProblemDetailsWrapper(problemDetails); + var outputStream = new MemoryStream(); + var expectedContent = "" + + "" + + "Some detail" + + "Some title" + + "Test Value 1" + + "<_x005B_Key2_x005D_>Test Value 2" + + ""; + + // Act + using (var xmlWriter = XmlWriter.Create(outputStream)) + { + var dataContractSerializer = new DataContractSerializer(wrapper.GetType()); + dataContractSerializer.WriteObject(xmlWriter, wrapper); + } + outputStream.Position = 0; + var res = new StreamReader(outputStream, Encoding.UTF8).ReadToEnd(); + + // Assert + Assert.Equal(expectedContent, res); + } + } +} diff --git a/test/Microsoft.AspNetCore.Mvc.Formatters.Xml.Test/WrapperProviderFactoryExtensionsTest.cs b/test/Microsoft.AspNetCore.Mvc.Formatters.Xml.Test/WrapperProviderFactoryExtensionsTest.cs new file mode 100644 index 0000000000..ca981f679a --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.Formatters.Xml.Test/WrapperProviderFactoryExtensionsTest.cs @@ -0,0 +1,34 @@ +// 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 Xunit; + +namespace Microsoft.AspNetCore.Mvc.Formatters.Xml +{ + public class WrapperProviderFactoryExtensionsTest + { + [Fact] + public void GetDefaultProviderFactories_GetsFactoriesUsedByInputAndOutputFormatters() + { + // Act + var factoryProviders = WrapperProviderFactoriesExtensions.GetDefaultProviderFactories(); + + // Assert + Assert.Collection( + factoryProviders, + factory => Assert.IsType(factory), + factory => + { + var wrapperProviderFactory = Assert.IsType(factory); + Assert.Equal(typeof(ProblemDetails), wrapperProviderFactory.DeclaredType); + Assert.Equal(typeof(ProblemDetailsWrapper), wrapperProviderFactory.WrappingType); + }, + factory => + { + var wrapperProviderFactory = Assert.IsType(factory); + Assert.Equal(typeof(ValidationProblemDetails), wrapperProviderFactory.DeclaredType); + Assert.Equal(typeof(ValidationProblemDetailsWrapper), wrapperProviderFactory.WrappingType); + }); + } + } +} diff --git a/test/Microsoft.AspNetCore.Mvc.Formatters.Xml.Test/WrapperProviderFactoryTest.cs b/test/Microsoft.AspNetCore.Mvc.Formatters.Xml.Test/WrapperProviderFactoryTest.cs new file mode 100644 index 0000000000..c067ed4ab9 --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.Formatters.Xml.Test/WrapperProviderFactoryTest.cs @@ -0,0 +1,63 @@ +// 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 Xunit; + +namespace Microsoft.AspNetCore.Mvc.Formatters.Xml +{ + public class WrapperProviderFactoryTest + { + [Fact] + public void GetProvider_ReturnsNull_IfTypeDoesNotMatch() + { + // Arrange + var provider = new WrapperProviderFactory( + typeof(ProblemDetails), + typeof(ProblemDetailsWrapper), + _ => null); + var context = new WrapperProviderContext(typeof(SerializableError), isSerialization: true); + + // Act + var result = provider.GetProvider(context); + + // Assert + Assert.Null(result); + } + + [Fact] + public void GetProvider_ReturnsNull_IfTypeIsSubtype() + { + // Arrange + var provider = new WrapperProviderFactory( + typeof(ProblemDetails), + typeof(ProblemDetailsWrapper), + _ => null); + var context = new WrapperProviderContext(typeof(ValidationProblemDetails), isSerialization: true); + + // Act + var result = provider.GetProvider(context); + + // Assert + Assert.Null(result); + } + + [Fact] + public void GetProvider_ReturnsValue_IfTypeMatches() + { + // Arrange + var expected = new object(); + var providerFactory = new WrapperProviderFactory( + typeof(ProblemDetails), + typeof(ProblemDetailsWrapper), + _ => expected); + var context = new WrapperProviderContext(typeof(ProblemDetails), isSerialization: true); + + // Act + var provider = providerFactory.GetProvider(context); + var result = provider.Wrap(new ProblemDetails()); + + // Assert + Assert.Same(expected, result); + } + } +} diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiBehaviorTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiBehaviorTest.cs index d3ec4d9e29..127d612fdd 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiBehaviorTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiBehaviorTest.cs @@ -10,6 +10,7 @@ using System.Threading.Tasks; using BasicWebSite.Models; using Microsoft.AspNetCore.Hosting; using Newtonsoft.Json; +using Newtonsoft.Json.Linq; using Xunit; namespace Microsoft.AspNetCore.Mvc.FunctionalTests @@ -116,8 +117,8 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests }; var expected = new Dictionary { - {"Name", new string[] {"The field Name must be a string with a minimum length of 5 and a maximum length of 30."}}, - {"Zip", new string[]{ @"The field Zip must match the regular expression '\d{5}'."}} + {"Name", new[] {"The field Name must be a string with a minimum length of 5 and a maximum length of 30."}}, + {"Zip", new[] { @"The field Zip must match the regular expression '\d{5}'."}} }; var contactString = JsonConvert.SerializeObject(contactModel); @@ -261,5 +262,62 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests var problemDetails = JsonConvert.DeserializeObject(content); Assert.Equal(404, problemDetails.Status); } + + [Fact] + public async Task SerializingProblemDetails_IgnoresNullValuedProperties() + { + // Arrange + var expected = new[] { "status", "title", "type" }; + + // Act + var response = await Client.GetAsync("/contact/ActionReturningStatusCodeResult"); + + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.NotFound); + var content = await response.Content.ReadAsStringAsync(); + + // Verify that null-valued properties on ProblemDetails are not serialized. + var json = JObject.Parse(content); + Assert.Equal(expected, json.Properties().OrderBy(p => p.Name).Select(p => p.Name)); + } + + [Fact] + public async Task SerializingProblemDetails_WithAllValuesSpecified() + { + // Arrange + var expected = new[] { "detail", "instance", "status", "title", "tracking-id", "type" }; + + // Act + var response = await Client.GetAsync("/contact/ActionReturningProblemDetails"); + + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.NotFound); + var content = await response.Content.ReadAsStringAsync(); + var json = JObject.Parse(content); + Assert.Equal(expected, json.Properties().OrderBy(p => p.Name).Select(p => p.Name)); + } + + [Fact] + public async Task SerializingValidationProblemDetails_WithExtensionData() + { + // Act + var response = await Client.GetAsync("/contact/ActionReturningValidationProblemDetails"); + + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.BadRequest); + var content = await response.Content.ReadAsStringAsync(); + var validationProblemDetails = JsonConvert.DeserializeObject(content); + + Assert.Equal("Error", validationProblemDetails.Title); + Assert.Equal(400, validationProblemDetails.Status); + Assert.Equal("27", validationProblemDetails.Extensions["tracking-id"]); + Assert.Collection( + validationProblemDetails.Errors, + kvp => + { + Assert.Equal("Error1", kvp.Key); + Assert.Equal(new[] { "Error Message" }, kvp.Value); + }); + } } } diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/Infrastructure/HttpClientExtensions.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/Infrastructure/HttpClientExtensions.cs index 94e52c7e14..184e83cd31 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/Infrastructure/HttpClientExtensions.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/Infrastructure/HttpClientExtensions.cs @@ -53,7 +53,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests throw new StatusCodeMismatchException { - ExpectedStatusCode = HttpStatusCode.OK, + ExpectedStatusCode = expectedStatusCode, ActualStatusCode = response.StatusCode, ResponseContent = responseContent, }; @@ -71,7 +71,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests { get { - return $"Excepted status code 200. Actual {ActualStatusCode}. Response Content:" + Environment.NewLine + ResponseContent; + return $"Excepted status code {ExpectedStatusCode}. Actual {ActualStatusCode}. Response Content:" + Environment.NewLine + ResponseContent; } } } diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/XmlDataContractSerializerFormattersWrappingTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/XmlDataContractSerializerFormattersWrappingTest.cs index 5e6443a862..9ee16921a6 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/XmlDataContractSerializerFormattersWrappingTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/XmlDataContractSerializerFormattersWrappingTest.cs @@ -208,5 +208,68 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests "", result); } + + [Fact] + public async Task ProblemDetails_IsSerialized() + { + // Arrange + var expected = @"404Not Foundhttps://tools.ietf.org/html/rfc7231#section-6.5.4"; + + // Act + var response = await Client.GetAsync("/api/XmlDataContractApi/ActionReturningClientErrorStatusCodeResult"); + + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.NotFound); + var content = await response.Content.ReadAsStringAsync(); + XmlAssert.Equal(expected, content); + } + + [Fact] + public async Task ProblemDetails_WithExtensionMembers_IsSerialized() + { + // Arrange + var expected = @"instance404title +correlationAccount1 Account2"; + + // Act + var response = await Client.GetAsync("/api/XmlDataContractApi/ActionReturningProblemDetails"); + + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.NotFound); + var content = await response.Content.ReadAsStringAsync(); + XmlAssert.Equal(expected, content); + } + + [Fact] + public async Task ValidationProblemDetails_IsSerialized() + { + // Arrange + var expected = @"400One or more validation errors occurred. +The State field is required."; + + // Act + var response = await Client.GetAsync("/api/XmlDataContractApi/ActionReturningValidationProblem"); + + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.BadRequest); + var content = await response.Content.ReadAsStringAsync(); + XmlAssert.Equal(expected, content); + } + + [Fact] + public async Task ValidationProblemDetails_WithExtensionMembers_IsSerialized() + { + // Arrange + var expected = @"some detail400One or more validation errors occurred. +some typecorrelationErrorValue"; + + // Act + var response = await Client.GetAsync("/api/XmlDataContractApi/ActionReturningValidationDetailsWithMetadata"); + + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.BadRequest); + var content = await response.Content.ReadAsStringAsync(); + XmlAssert.Equal(expected, content); + } } -} \ No newline at end of file +} diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/XmlSerializerFormattersWrappingTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/XmlSerializerFormattersWrappingTest.cs index 2b8ddd9a06..d54cd3880c 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/XmlSerializerFormattersWrappingTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/XmlSerializerFormattersWrappingTest.cs @@ -183,5 +183,68 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests "key2-error", result); } + + [Fact] + public async Task ProblemDetails_IsSerialized() + { + // Arrange + var expected = @"404Not Foundhttps://tools.ietf.org/html/rfc7231#section-6.5.4"; + + // Act + var response = await Client.GetAsync("/api/XmlSerializerApi/ActionReturningClientErrorStatusCodeResult"); + + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.NotFound); + var content = await response.Content.ReadAsStringAsync(); + XmlAssert.Equal(expected, content); + } + + [Fact] + public async Task ProblemDetails_WithExtensionMembers_IsSerialized() + { + // Arrange + var expected = @"instance404title +correlationAccount1 Account2"; + + // Act + var response = await Client.GetAsync("/api/XmlSerializerApi/ActionReturningProblemDetails"); + + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.NotFound); + var content = await response.Content.ReadAsStringAsync(); + XmlAssert.Equal(expected, content); + } + + [Fact] + public async Task ValidationProblemDetails_IsSerialized() + { + // Arrange + var expected = @"400One or more validation errors occurred. +The State field is required."; + + // Act + var response = await Client.GetAsync("/api/XmlSerializerApi/ActionReturningValidationProblem"); + + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.BadRequest); + var content = await response.Content.ReadAsStringAsync(); + XmlAssert.Equal(expected, content); + } + + [Fact] + public async Task ValidationProblemDetails_WithExtensionMembers_IsSerialized() + { + // Arrange + var expected = @"some detail400One or more validation errors occurred. +some typecorrelationErrorValue"; + + // Act + var response = await Client.GetAsync("/api/XmlSerializerApi/ActionReturningValidationDetailsWithMetadata"); + + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.BadRequest); + var content = await response.Content.ReadAsStringAsync(); + XmlAssert.Equal(expected, content); + } } } diff --git a/test/WebSites/BasicWebSite/Controllers/ContactApiController.cs b/test/WebSites/BasicWebSite/Controllers/ContactApiController.cs index a2cd22afc5..c2d3400e1f 100644 --- a/test/WebSites/BasicWebSite/Controllers/ContactApiController.cs +++ b/test/WebSites/BasicWebSite/Controllers/ContactApiController.cs @@ -91,6 +91,41 @@ namespace BasicWebSite return NotFound(); } + [HttpGet("[action]")] + public ActionResult ActionReturningProblemDetails() + { + return NotFound(new ProblemDetails + { + Title = "Not Found", + Type = "Type", + Detail = "Detail", + Status = 404, + Instance = "Instance", + Extensions = + { + ["tracking-id"] = 27, + }, + }); + } + + [HttpGet("[action]")] + public ActionResult ActionReturningValidationProblemDetails() + { + return BadRequest(new ValidationProblemDetails + { + Title = "Error", + Status = 400, + Extensions = + { + ["tracking-id"] = "27", + }, + Errors = + { + { "Error1", new[] { "Error Message" } }, + }, + }); + } + private class TestModelBinder : IModelBinder { public Task BindModelAsync(ModelBindingContext bindingContext) diff --git a/test/WebSites/XmlFormattersWebSite/Controllers/XmlApiControllerBase.cs b/test/WebSites/XmlFormattersWebSite/Controllers/XmlApiControllerBase.cs new file mode 100644 index 0000000000..5296428144 --- /dev/null +++ b/test/WebSites/XmlFormattersWebSite/Controllers/XmlApiControllerBase.cs @@ -0,0 +1,55 @@ +// 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 Microsoft.AspNetCore.Mvc; +using XmlFormattersWebSite.Models; + +namespace XmlFormattersWebSite +{ + [ApiController] + [Route("api/[controller]/[action]")] + public abstract class XmlApiControllerBase : ControllerBase + { + [HttpGet] + public ActionResult ActionReturningClientErrorStatusCodeResult() + => NotFound(); + + [HttpGet] + public ActionResult ActionReturningProblemDetails() + { + return NotFound(new ProblemDetails + { + Instance = "instance", + Title = "title", + Extensions = + { + ["Correlation"] = "correlation", + ["Accounts"] = new[] { "Account1", "Account2" }, + }, + }); + } + + [HttpGet] + public ActionResult ActionReturningValidationProblem([FromQuery] Address address) + => throw new NotImplementedException(); + + [HttpGet] + public ActionResult ActionReturningValidationDetailsWithMetadata() + { + return new BadRequestObjectResult(new ValidationProblemDetails + { + Detail = "some detail", + Type = "some type", + Extensions = + { + ["CorrelationId"] = "correlation", + }, + Errors = + { + ["Error1"] = new[] { "ErrorValue"}, + }, + }); + } + } +} diff --git a/test/WebSites/XmlFormattersWebSite/Controllers/XmlDataContractApiController.cs b/test/WebSites/XmlFormattersWebSite/Controllers/XmlDataContractApiController.cs new file mode 100644 index 0000000000..dd8f228caa --- /dev/null +++ b/test/WebSites/XmlFormattersWebSite/Controllers/XmlDataContractApiController.cs @@ -0,0 +1,29 @@ +// 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; +using Microsoft.AspNetCore.Mvc.Filters; +using Microsoft.AspNetCore.Mvc.Formatters; + +namespace XmlFormattersWebSite +{ + [SetupOutputFormatters] + public class XmlDataContractApiController : XmlApiControllerBase + { + private class SetupOutputFormattersAttribute : ResultFilterAttribute + { + public override void OnResultExecuting(ResultExecutingContext context) + { + if (!(context.Result is ObjectResult objectResult)) + { + return; + } + + // Both kinds of Xml serializers are configured for this application and use custom content-types to do formatter + // selection. The globally configured formatters rely on custom content-type to perform conneg which does not play + // well the ProblemDetails returning filters that defaults to using application/xml. We'll explicitly select the formatter for this controller. + objectResult.Formatters.Add(new XmlDataContractSerializerOutputFormatter()); + } + } + } +} \ No newline at end of file diff --git a/test/WebSites/XmlFormattersWebSite/Controllers/XmlSerializedApiController.cs b/test/WebSites/XmlFormattersWebSite/Controllers/XmlSerializedApiController.cs new file mode 100644 index 0000000000..6ee3ec4708 --- /dev/null +++ b/test/WebSites/XmlFormattersWebSite/Controllers/XmlSerializedApiController.cs @@ -0,0 +1,29 @@ +// 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; +using Microsoft.AspNetCore.Mvc.Filters; +using Microsoft.AspNetCore.Mvc.Formatters; + +namespace XmlFormattersWebSite +{ + [SetupOutputFormatters] + public class XmlSerializerApiController : XmlApiControllerBase + { + private class SetupOutputFormattersAttribute : ResultFilterAttribute + { + public override void OnResultExecuting(ResultExecutingContext context) + { + if (!(context.Result is ObjectResult objectResult)) + { + return; + } + + // Both kinds of Xml serializers are configured for this application and use custom content-types to do formatter + // selection. The globally configured formatters rely on custom content-type to perform conneg which does not play + // well the ProblemDetails returning filters that defaults to using application/xml. We'll explicitly select the formatter for this controller. + objectResult.Formatters.Add(new XmlSerializerOutputFormatter()); + } + } + } +} \ No newline at end of file From 234b003b313c4f637d46c356a2badbd92d678a10 Mon Sep 17 00:00:00 2001 From: Ryan Brandenburg Date: Tue, 28 Aug 2018 10:39:00 -0700 Subject: [PATCH 05/21] Set longRunningTestSeconds for Functional tests --- .../Microsoft.AspNetCore.Mvc.FunctionalTests/xunit.runner.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/xunit.runner.json b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/xunit.runner.json index 1c72a421ad..0d8a1f6a45 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/xunit.runner.json +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/xunit.runner.json @@ -1,3 +1,4 @@ { - "shadowCopy": false + "shadowCopy": false, + "longRunningTestSeconds": 60 } From 28f96bf83293d2947bc5e6d4244dbae7ea496433 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Thu, 30 Aug 2018 08:16:57 +1200 Subject: [PATCH 06/21] Fix obsolete build warning (#8358) --- .../Internal/AttributeRoutingTest.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/AttributeRoutingTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/AttributeRoutingTest.cs index c9d281b80d..4c475cadad 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/AttributeRoutingTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/AttributeRoutingTest.cs @@ -175,8 +175,12 @@ namespace Microsoft.AspNetCore.Mvc.Internal .SetupGet(o => o.Value) .Returns(new RouteOptions()); +#pragma warning disable CS0618 // Type or member is obsolete + var inlineConstraintResolver = new DefaultInlineConstraintResolver(routeOptions.Object); +#pragma warning restore CS0618 // Type or member is obsolete + var services = new ServiceCollection() - .AddSingleton(new DefaultInlineConstraintResolver(routeOptions.Object)) + .AddSingleton(inlineConstraintResolver) .AddSingleton(); services.AddSingleton(); From b649133eec344e733d1724964a3255b972ed5ae5 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Thu, 30 Aug 2018 08:57:53 +1200 Subject: [PATCH 07/21] Refactor KnownRouteValueConstraint to not require HttpContext (#8352) --- build/dependencies.props | 4 +- .../Routing/KnownRouteValueConstraint.cs | 82 ++++++++----- .../Internal/MvcEndpointDataSourceTests.cs | 32 +++-- .../Routing/KnownRouteValueConstraintTests.cs | 109 ++++++++++++++++-- 4 files changed, 172 insertions(+), 55 deletions(-) diff --git a/build/dependencies.props b/build/dependencies.props index b0c3540479..d718620eed 100644 --- a/build/dependencies.props +++ b/build/dependencies.props @@ -48,8 +48,8 @@ 2.2.0-preview2-35090 2.2.0-preview2-35090 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 + 2.2.0-a-preview2-routeconstraint-httpcontext-16912 + 2.2.0-a-preview2-routeconstraint-httpcontext-16912 2.2.0-preview2-35090 2.2.0-preview2-35090 2.2.0-preview2-35090 diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Routing/KnownRouteValueConstraint.cs b/src/Microsoft.AspNetCore.Mvc.Core/Routing/KnownRouteValueConstraint.cs index a80b3b510c..6989c22695 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Routing/KnownRouteValueConstraint.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Routing/KnownRouteValueConstraint.cs @@ -14,8 +14,26 @@ namespace Microsoft.AspNetCore.Mvc.Routing { public class KnownRouteValueConstraint : IRouteConstraint { + private readonly IActionDescriptorCollectionProvider _actionDescriptorCollectionProvider; private RouteValuesCollection _cachedValuesCollection; + [Obsolete("This constructor is obsolete. Use KnownRouteValueConstraint.ctor(IActionDescriptorCollectionProvider) instead.")] + public KnownRouteValueConstraint() + { + // Empty constructor for backwards compatibility + // Services will need to be resolved from HttpContext when this ctor is used + } + + public KnownRouteValueConstraint(IActionDescriptorCollectionProvider actionDescriptorCollectionProvider) + { + if (actionDescriptorCollectionProvider == null) + { + throw new ArgumentNullException(nameof(actionDescriptorCollectionProvider)); + } + + _actionDescriptorCollectionProvider = actionDescriptorCollectionProvider; + } + public bool Match( HttpContext httpContext, IRouter route, @@ -23,16 +41,6 @@ namespace Microsoft.AspNetCore.Mvc.Routing RouteValueDictionary values, RouteDirection routeDirection) { - if (httpContext == null) - { - throw new ArgumentNullException(nameof(httpContext)); - } - - if (route == null) - { - throw new ArgumentNullException(nameof(route)); - } - if (routeKey == null) { throw new ArgumentNullException(nameof(routeKey)); @@ -49,7 +57,9 @@ namespace Microsoft.AspNetCore.Mvc.Routing var value = obj as string; if (value != null) { - var allValues = GetAndCacheAllMatchingValues(routeKey, httpContext); + var actionDescriptors = GetAndValidateActionDescriptors(httpContext); + + var allValues = GetAndCacheAllMatchingValues(routeKey, actionDescriptors); foreach (var existingValue in allValues) { if (string.Equals(value, existingValue, StringComparison.OrdinalIgnoreCase)) @@ -63,9 +73,36 @@ namespace Microsoft.AspNetCore.Mvc.Routing return false; } - private string[] GetAndCacheAllMatchingValues(string routeKey, HttpContext httpContext) + private ActionDescriptorCollection GetAndValidateActionDescriptors(HttpContext httpContext) + { + var actionDescriptorsProvider = _actionDescriptorCollectionProvider; + + if (actionDescriptorsProvider == null) + { + // Only validate that HttpContext was passed to constraint if it is needed + if (httpContext == null) + { + throw new ArgumentNullException(nameof(httpContext)); + } + + var services = httpContext.RequestServices; + actionDescriptorsProvider = services.GetRequiredService(); + } + + var actionDescriptors = actionDescriptorsProvider.ActionDescriptors; + if (actionDescriptors == null) + { + throw new InvalidOperationException( + Resources.FormatPropertyOfTypeCannotBeNull( + nameof(IActionDescriptorCollectionProvider.ActionDescriptors), + actionDescriptorsProvider.GetType())); + } + + return actionDescriptors; + } + + private string[] GetAndCacheAllMatchingValues(string routeKey, ActionDescriptorCollection actionDescriptors) { - var actionDescriptors = GetAndValidateActionDescriptorCollection(httpContext); var version = actionDescriptors.Version; var valuesCollection = _cachedValuesCollection; @@ -77,8 +114,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing { var action = actionDescriptors.Items[i]; - string value; - if (action.RouteValues.TryGetValue(routeKey, out value) && + if (action.RouteValues.TryGetValue(routeKey, out var value) && !string.IsNullOrEmpty(value)) { values.Add(value); @@ -92,22 +128,6 @@ namespace Microsoft.AspNetCore.Mvc.Routing return _cachedValuesCollection.Items; } - private static ActionDescriptorCollection GetAndValidateActionDescriptorCollection(HttpContext httpContext) - { - var services = httpContext.RequestServices; - var provider = services.GetRequiredService(); - var descriptors = provider.ActionDescriptors; - - if (descriptors == null) - { - throw new InvalidOperationException( - Resources.FormatPropertyOfTypeCannotBeNull("ActionDescriptors", - provider.GetType())); - } - - return descriptors; - } - private class RouteValuesCollection { public RouteValuesCollection(int version, string[] items) diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs index be31acba0b..fe5d8dbbf7 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs @@ -180,7 +180,15 @@ namespace Microsoft.AspNetCore.Mvc.Internal var actionDescriptorCollection = GetActionDescriptorCollection( new { controller = "TestController", action = "TestAction", area = "TestArea" }); var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); - dataSource.ConventionalEndpointInfos.Add(CreateEndpointInfo(string.Empty, endpointInfoRoute)); + + var services = new ServiceCollection(); + services.AddRouting(); + services.AddSingleton(actionDescriptorCollection); + + var routeOptionsSetup = new MvcCoreRouteOptionsSetup(); + services.Configure(routeOptionsSetup.Configure); + + dataSource.ConventionalEndpointInfos.Add(CreateEndpointInfo(string.Empty, endpointInfoRoute, serviceProvider: services.BuildServiceProvider())); // Act var endpoints = dataSource.Endpoints; @@ -719,13 +727,13 @@ namespace Microsoft.AspNetCore.Mvc.Internal Array.Empty()); } - var serviceProviderMock = new Mock(); - serviceProviderMock.Setup(m => m.GetService(typeof(IActionDescriptorCollectionProvider))).Returns(actionDescriptorCollectionProvider); + var services = new ServiceCollection(); + services.AddSingleton(actionDescriptorCollectionProvider); var dataSource = new MvcEndpointDataSource( actionDescriptorCollectionProvider, mvcEndpointInvokerFactory ?? new MvcEndpointInvokerFactory(new ActionInvokerFactory(Array.Empty())), - serviceProviderMock.Object); + services.BuildServiceProvider()); return dataSource; } @@ -735,15 +743,19 @@ namespace Microsoft.AspNetCore.Mvc.Internal string template, RouteValueDictionary defaults = null, IDictionary constraints = null, - RouteValueDictionary dataTokens = null) + RouteValueDictionary dataTokens = null, + IServiceProvider serviceProvider = null) { - var serviceCollection = new ServiceCollection(); - serviceCollection.AddRouting(); + if (serviceProvider == null) + { + var serviceCollection = new ServiceCollection(); + serviceCollection.AddRouting(); - var routeOptionsSetup = new MvcCoreRouteOptionsSetup(); - serviceCollection.Configure(routeOptionsSetup.Configure); + var routeOptionsSetup = new MvcCoreRouteOptionsSetup(); + serviceCollection.Configure(routeOptionsSetup.Configure); - var serviceProvider = serviceCollection.BuildServiceProvider(); + serviceProvider = serviceCollection.BuildServiceProvider(); + } var parameterPolicyFactory = serviceProvider.GetRequiredService(); return new MvcEndpointInfo(name, template, defaults, constraints, dataTokens, parameterPolicyFactory); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/KnownRouteValueConstraintTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/KnownRouteValueConstraintTests.cs index 1e1d1f8951..5aecf45a77 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/KnownRouteValueConstraintTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/KnownRouteValueConstraintTests.cs @@ -2,7 +2,6 @@ // 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 Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; @@ -10,6 +9,7 @@ using Microsoft.AspNetCore.Mvc.Controllers; using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.Internal; using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.DependencyInjection; using Moq; using Xunit; @@ -17,7 +17,47 @@ namespace Microsoft.AspNetCore.Mvc.Routing { public class KnownRouteValueConstraintTests { +#pragma warning disable CS0618 // Type or member is obsolete private readonly IRouteConstraint _constraint = new KnownRouteValueConstraint(); +#pragma warning restore CS0618 // Type or member is obsolete + + [Fact] + public void ResolveFromServices_InjectsServiceProvider_HttpContextNotNeeded() + { + // Arrange + var actionDescriptor = CreateActionDescriptor("testArea", + "testController", + "testAction"); + actionDescriptor.RouteValues.Add("randomKey", "testRandom"); + var descriptorCollectionProvider = CreateActionDesciprtorCollectionProvider(actionDescriptor); + + var services = new ServiceCollection(); + services.AddRouting(); + services.AddSingleton(descriptorCollectionProvider); + + var routeOptionsSetup = new MvcCoreRouteOptionsSetup(); + services.Configure(routeOptionsSetup.Configure); + + var serviceProvider = services.BuildServiceProvider(); + + var inlineConstraintResolver = serviceProvider.GetRequiredService(); + var constraint = inlineConstraintResolver.ResolveConstraint("exists"); + + var values = new RouteValueDictionary() + { + { "area", "testArea" }, + { "controller", "testController" }, + { "action", "testAction" }, + { "randomKey", "testRandom" } + }; + + // Act + var knownRouteValueConstraint = Assert.IsType(constraint); + var match = knownRouteValueConstraint.Match(httpContext: null, route: null, "area", values, RouteDirection.IncomingRequest); + + // Assert + Assert.True(match); + } [Theory] [InlineData("area", RouteDirection.IncomingRequest)] @@ -55,8 +95,8 @@ namespace Microsoft.AspNetCore.Mvc.Routing { // Arrange var actionDescriptor = CreateActionDescriptor("testArea", - "testController", - "testAction"); + "testController", + "testAction"); actionDescriptor.RouteValues.Add("randomKey", "testRandom"); var httpContext = GetHttpContext(actionDescriptor); var route = Mock.Of(); @@ -115,8 +155,8 @@ namespace Microsoft.AspNetCore.Mvc.Routing public void RouteValue_IsNotAString_MatchFails(RouteDirection direction) { var actionDescriptor = CreateActionDescriptor("testArea", - controller: null, - action: null); + controller: null, + action: null); var httpContext = GetHttpContext(actionDescriptor); var route = Mock.Of(); var values = new RouteValueDictionary() @@ -157,7 +197,57 @@ namespace Microsoft.AspNetCore.Mvc.Routing ex.Message); } - private static HttpContext GetHttpContext(ActionDescriptor actionDescriptor) + [Theory] + [InlineData("area", RouteDirection.IncomingRequest)] + [InlineData("controller", RouteDirection.IncomingRequest)] + [InlineData("action", RouteDirection.IncomingRequest)] + [InlineData("randomKey", RouteDirection.IncomingRequest)] + [InlineData("area", RouteDirection.UrlGeneration)] + [InlineData("controller", RouteDirection.UrlGeneration)] + [InlineData("action", RouteDirection.UrlGeneration)] + [InlineData("randomKey", RouteDirection.UrlGeneration)] + public void ServiceInjected_RouteKey_Exists_MatchSucceeds(string keyName, RouteDirection direction) + { + // Arrange + var actionDescriptor = CreateActionDescriptor("testArea", + "testController", + "testAction"); + actionDescriptor.RouteValues.Add("randomKey", "testRandom"); + + var provider = CreateActionDesciprtorCollectionProvider(actionDescriptor); + + var constraint = new KnownRouteValueConstraint(provider); + + var values = new RouteValueDictionary() + { + { "area", "testArea" }, + { "controller", "testController" }, + { "action", "testAction" }, + { "randomKey", "testRandom" } + }; + + // Act + var match = constraint.Match(httpContext: null, route: null, keyName, values, direction); + + // Assert + Assert.True(match); + } + + private static HttpContext GetHttpContext(ActionDescriptor actionDescriptor, bool setupRequestServices = true) + { + var descriptorCollectionProvider = CreateActionDesciprtorCollectionProvider(actionDescriptor); + + var context = new Mock(); + if (setupRequestServices) + { + context.Setup(o => o.RequestServices + .GetService(typeof(IActionDescriptorCollectionProvider))) + .Returns(descriptorCollectionProvider); + } + return context.Object; + } + + private static IActionDescriptorCollectionProvider CreateActionDesciprtorCollectionProvider(ActionDescriptor actionDescriptor) { var actionProvider = new Mock(MockBehavior.Strict); @@ -176,12 +266,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing var descriptorCollectionProvider = new DefaultActionDescriptorCollectionProvider( new[] { actionProvider.Object }, Enumerable.Empty()); - - var context = new Mock(); - context.Setup(o => o.RequestServices - .GetService(typeof(IActionDescriptorCollectionProvider))) - .Returns(descriptorCollectionProvider); - return context.Object; + return descriptorCollectionProvider; } private static ActionDescriptor CreateActionDescriptor(string area, string controller, string action) From 82a01a414dfd69de6c407951c917ba397b210be8 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Tue, 28 Aug 2018 16:48:45 -0700 Subject: [PATCH 08/21] Set trace id in ProblemDetalsClientErrorFactory --- .../ProblemDetailsClientErrorFactory.cs | 10 ++ .../Internal/ApiBehaviorOptionsSetup.cs | 12 +- .../ProblemDetalsClientErrorFactoryTest.cs | 68 +++++++++++- .../Internal/ApiBehaviorOptionsSetupTest.cs | 62 +++++++++++ .../ActivityReplacer.cs | 25 +++++ .../ApiBehaviorTest.cs | 105 +++++++++++------- ...soft.AspNetCore.Mvc.FunctionalTests.csproj | 1 + ...ontractSerializerFormattersWrappingTest.cs | 48 +++++--- .../XmlSerializerFormattersWrappingTest.cs | 48 +++++--- 9 files changed, 307 insertions(+), 72 deletions(-) create mode 100644 test/Microsoft.AspNetCore.Mvc.Core.TestCommon/ActivityReplacer.cs diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ProblemDetailsClientErrorFactory.cs b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ProblemDetailsClientErrorFactory.cs index 1bf1e3ac43..ff47a18fa6 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ProblemDetailsClientErrorFactory.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ProblemDetailsClientErrorFactory.cs @@ -2,12 +2,14 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Diagnostics; using Microsoft.Extensions.Options; namespace Microsoft.AspNetCore.Mvc.Infrastructure { internal class ProblemDetailsClientErrorFactory : IClientErrorFactory { + private static readonly string TraceIdentifierKey = "traceId"; private readonly ApiBehaviorOptions _options; public ProblemDetailsClientErrorFactory(IOptions options) @@ -28,6 +30,8 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure { problemDetails.Title = errorData.Title; problemDetails.Type = errorData.Link; + + SetTraceId(actionContext, problemDetails); } return new ObjectResult(problemDetails) @@ -40,5 +44,11 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure }, }; } + + internal static void SetTraceId(ActionContext actionContext, ProblemDetails problemDetails) + { + var traceId = Activity.Current?.Id ?? actionContext.HttpContext.TraceIdentifier; + problemDetails.Extensions[TraceIdentifierKey] = traceId; + } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorOptionsSetup.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorOptionsSetup.cs index 4d64d9bb84..bddaaa2478 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorOptionsSetup.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorOptionsSetup.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Core; using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.Extensions.Logging; @@ -128,9 +129,16 @@ namespace Microsoft.AspNetCore.Mvc.Internal return result; } - private static IActionResult ProblemDetailsInvalidModelStateResponse(ActionContext context) + internal static IActionResult ProblemDetailsInvalidModelStateResponse(ActionContext context) { - var result = new BadRequestObjectResult(new ValidationProblemDetails(context.ModelState)); + var problemDetails = new ValidationProblemDetails(context.ModelState) + { + Status = StatusCodes.Status400BadRequest, + }; + + ProblemDetailsClientErrorFactory.SetTraceId(context, problemDetails); + + var result = new BadRequestObjectResult(problemDetails); result.ContentTypes.Add("application/problem+json"); result.ContentTypes.Add("application/problem+xml"); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/ProblemDetalsClientErrorFactoryTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/ProblemDetalsClientErrorFactoryTest.cs index 65a53a8158..603b60e381 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/ProblemDetalsClientErrorFactoryTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/ProblemDetalsClientErrorFactoryTest.cs @@ -1,6 +1,8 @@ // 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.Diagnostics; +using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Options; using Xunit; @@ -22,7 +24,7 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure })); // Act - var result = factory.GetClientError(new ActionContext(), clientError); + var result = factory.GetClientError(GetActionContext(), clientError); // Assert var objectResult = Assert.IsType(result); @@ -49,7 +51,7 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure })); // Act - var result = factory.GetClientError(new ActionContext(), clientError); + var result = factory.GetClientError(GetActionContext(), clientError); // Assert var objectResult = Assert.IsType(result); @@ -61,5 +63,67 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure Assert.Null(problemDetails.Detail); Assert.Null(problemDetails.Instance); } + + [Fact] + public void GetClientError_UsesActivityId_ToSetTraceId() + { + // Arrange + using (new ActivityReplacer()) + { + var clientError = new UnsupportedMediaTypeResult(); + var factory = new ProblemDetailsClientErrorFactory(Options.Create(new ApiBehaviorOptions + { + ClientErrorMapping = + { + [415] = new ClientErrorData { Link = "Some link", Title = "Summary" }, + }, + })); + + // Act + var result = factory.GetClientError(GetActionContext(), clientError); + + // Assert + var objectResult = Assert.IsType(result); + Assert.Equal(new[] { "application/problem+json", "application/problem+xml" }, objectResult.ContentTypes); + var problemDetails = Assert.IsType(objectResult.Value); + + Assert.Equal(Activity.Current.Id, problemDetails.Extensions["traceId"]); + } + } + + [Fact] + public void GetClientError_UsesHttpContext_ToSetTraceIdIfActivityIdIsNotSet() + { + // Arrange + var clientError = new UnsupportedMediaTypeResult(); + var factory = new ProblemDetailsClientErrorFactory(Options.Create(new ApiBehaviorOptions + { + ClientErrorMapping = + { + [415] = new ClientErrorData { Link = "Some link", Title = "Summary" }, + }, + })); + + // Act + var result = factory.GetClientError(GetActionContext(), clientError); + + // Assert + var objectResult = Assert.IsType(result); + Assert.Equal(new[] { "application/problem+json", "application/problem+xml" }, objectResult.ContentTypes); + var problemDetails = Assert.IsType(objectResult.Value); + + Assert.Equal("42", problemDetails.Extensions["traceId"]); + } + + private static ActionContext GetActionContext() + { + return new ActionContext + { + HttpContext = new DefaultHttpContext + { + TraceIdentifier = "42", + } + }; + } } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorOptionsSetupTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorOptionsSetupTest.cs index 622a36e5ff..4ae53cd4d7 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorOptionsSetupTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorOptionsSetupTest.cs @@ -2,6 +2,9 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Diagnostics; +using System.Linq; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; @@ -97,5 +100,64 @@ namespace Microsoft.AspNetCore.Mvc.Internal // Assert Assert.Same(expected, options.InvalidModelStateResponseFactory); } + + [Fact] + public void ProblemDetailsInvalidModelStateResponse_ReturnsBadRequestWithProblemDetails() + { + // Arrange + var actionContext = new ActionContext + { + HttpContext = new DefaultHttpContext { TraceIdentifier = "42" }, + }; + + // Act + var result = ApiBehaviorOptionsSetup.ProblemDetailsInvalidModelStateResponse(actionContext); + + // Assert + var badRequest = Assert.IsType(result); + Assert.Equal(new[] { "application/problem+json", "application/problem+xml" }, badRequest.ContentTypes.OrderBy(c => c)); + + var problemDetails = Assert.IsType(badRequest.Value); + Assert.Equal(400, problemDetails.Status); + } + + [Fact] + public void ProblemDetailsInvalidModelStateResponse_SetsTraceId() + { + // Arrange + using (new ActivityReplacer()) + { + var actionContext = new ActionContext + { + HttpContext = new DefaultHttpContext { TraceIdentifier = "42" }, + }; + + // Act + var result = ApiBehaviorOptionsSetup.ProblemDetailsInvalidModelStateResponse(actionContext); + + // Assert + var badRequest = Assert.IsType(result); + var problemDetails = Assert.IsType(badRequest.Value); + Assert.Equal(Activity.Current.Id, problemDetails.Extensions["traceId"]); + } + } + + [Fact] + public void ProblemDetailsInvalidModelStateResponse_SetsTraceIdFromRequest_IfActivityIsNull() + { + // Arrange + var actionContext = new ActionContext + { + HttpContext = new DefaultHttpContext { TraceIdentifier = "42" }, + }; + + // Act + var result = ApiBehaviorOptionsSetup.ProblemDetailsInvalidModelStateResponse(actionContext); + + // Assert + var badRequest = Assert.IsType(result); + var problemDetails = Assert.IsType(badRequest.Value); + Assert.Equal("42", problemDetails.Extensions["traceId"]); + } } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.TestCommon/ActivityReplacer.cs b/test/Microsoft.AspNetCore.Mvc.Core.TestCommon/ActivityReplacer.cs new file mode 100644 index 0000000000..f7bc9d8193 --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.Core.TestCommon/ActivityReplacer.cs @@ -0,0 +1,25 @@ +// 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.Diagnostics; + +namespace Microsoft.AspNetCore.Mvc +{ + public class ActivityReplacer : IDisposable + { + private readonly Activity _activity; + + public ActivityReplacer() + { + _activity = new Activity("Test"); + _activity.Start(); + } + + public void Dispose() + { + Debug.Assert(Activity.Current == _activity); + _activity.Stop(); + } + } +} diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiBehaviorTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiBehaviorTest.cs index 127d612fdd..c30dd30750 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiBehaviorTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiBehaviorTest.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Net; using System.Net.Http; @@ -35,37 +36,48 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests public async Task ActionsReturnBadRequest_WhenModelStateIsInvalid() { // Arrange - var contactModel = new Contact + using (new ActivityReplacer()) { - Name = "Abc", - City = "Redmond", - State = "WA", - Zip = "Invalid", - }; - var contactString = JsonConvert.SerializeObject(contactModel); - - // Act - var response = await Client.PostAsJsonAsync("/contact", contactModel); - - // Assert - await response.AssertStatusCodeAsync(HttpStatusCode.BadRequest); - Assert.Equal("application/problem+json", response.Content.Headers.ContentType.MediaType); - var problemDetails = JsonConvert.DeserializeObject(await response.Content.ReadAsStringAsync()); - Assert.Collection( - problemDetails.Errors.OrderBy(kvp => kvp.Key), - kvp => + var contactModel = new Contact { - Assert.Equal("Name", kvp.Key); - var error = Assert.Single(kvp.Value); - Assert.Equal("The field Name must be a string with a minimum length of 5 and a maximum length of 30.", error); - }, - kvp => - { - Assert.Equal("Zip", kvp.Key); - var error = Assert.Single(kvp.Value); - Assert.Equal("The field Zip must match the regular expression '\\d{5}'.", error); - } - ); + Name = "Abc", + City = "Redmond", + State = "WA", + Zip = "Invalid", + }; + var contactString = JsonConvert.SerializeObject(contactModel); + + // Act + var response = await Client.PostAsJsonAsync("/contact", contactModel); + + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.BadRequest); + Assert.Equal("application/problem+json", response.Content.Headers.ContentType.MediaType); + var problemDetails = JsonConvert.DeserializeObject(await response.Content.ReadAsStringAsync()); + Assert.Collection( + problemDetails.Errors.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("Name", kvp.Key); + var error = Assert.Single(kvp.Value); + Assert.Equal("The field Name must be a string with a minimum length of 5 and a maximum length of 30.", error); + }, + kvp => + { + Assert.Equal("Zip", kvp.Key); + var error = Assert.Single(kvp.Value); + Assert.Equal("The field Zip must match the regular expression '\\d{5}'.", error); + } + ); + + Assert.Collection( + problemDetails.Extensions, + kvp => + { + Assert.Equal("traceId", kvp.Key); + Assert.Equal(Activity.Current.Id, kvp.Value); + }); + } } [Fact] @@ -253,21 +265,31 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests [Fact] public async Task ClientErrorResultFilterExecutesForStatusCodeResults() { - // Act - var response = await Client.GetAsync("/contact/ActionReturningStatusCodeResult"); + using (new ActivityReplacer()) + { + // Act + var response = await Client.GetAsync("/contact/ActionReturningStatusCodeResult"); - // Assert - await response.AssertStatusCodeAsync(HttpStatusCode.NotFound); - var content = await response.Content.ReadAsStringAsync(); - var problemDetails = JsonConvert.DeserializeObject(content); - Assert.Equal(404, problemDetails.Status); + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.NotFound); + var content = await response.Content.ReadAsStringAsync(); + var problemDetails = JsonConvert.DeserializeObject(content); + Assert.Equal(404, problemDetails.Status); + Assert.Collection( + problemDetails.Extensions, + kvp => + { + Assert.Equal("traceId", kvp.Key); + Assert.Equal(Activity.Current.Id, kvp.Value); + }); + } } [Fact] public async Task SerializingProblemDetails_IgnoresNullValuedProperties() { // Arrange - var expected = new[] { "status", "title", "type" }; + var expected = new[] { "status", "title", "traceId", "type" }; // Act var response = await Client.GetAsync("/contact/ActionReturningStatusCodeResult"); @@ -310,7 +332,14 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests Assert.Equal("Error", validationProblemDetails.Title); Assert.Equal(400, validationProblemDetails.Status); - Assert.Equal("27", validationProblemDetails.Extensions["tracking-id"]); + Assert.Collection( + validationProblemDetails.Extensions, + kvp => + { + Assert.Equal("tracking-id", kvp.Key); + Assert.Equal("27", kvp.Value); + }); + Assert.Collection( validationProblemDetails.Errors, kvp => diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/Microsoft.AspNetCore.Mvc.FunctionalTests.csproj b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/Microsoft.AspNetCore.Mvc.FunctionalTests.csproj index 1e7cb2f215..bce36312a4 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/Microsoft.AspNetCore.Mvc.FunctionalTests.csproj +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/Microsoft.AspNetCore.Mvc.FunctionalTests.csproj @@ -10,6 +10,7 @@ + diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/XmlDataContractSerializerFormattersWrappingTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/XmlDataContractSerializerFormattersWrappingTest.cs index 9ee16921a6..2e34fbbe27 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/XmlDataContractSerializerFormattersWrappingTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/XmlDataContractSerializerFormattersWrappingTest.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 System.Diagnostics; using System.Net; using System.Net.Http; using System.Net.Http.Headers; @@ -213,15 +214,23 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests public async Task ProblemDetails_IsSerialized() { // Arrange - var expected = @"404Not Foundhttps://tools.ietf.org/html/rfc7231#section-6.5.4"; + using (new ActivityReplacer()) + { + var expected = "" + + "404" + + "Not Found" + + "https://tools.ietf.org/html/rfc7231#section-6.5.4" + + $"{Activity.Current.Id}" + + ""; - // Act - var response = await Client.GetAsync("/api/XmlDataContractApi/ActionReturningClientErrorStatusCodeResult"); + // Act + var response = await Client.GetAsync("/api/XmlDataContractApi/ActionReturningClientErrorStatusCodeResult"); - // Assert - await response.AssertStatusCodeAsync(HttpStatusCode.NotFound); - var content = await response.Content.ReadAsStringAsync(); - XmlAssert.Equal(expected, content); + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.NotFound); + var content = await response.Content.ReadAsStringAsync(); + XmlAssert.Equal(expected, content); + } } [Fact] @@ -244,16 +253,25 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests public async Task ValidationProblemDetails_IsSerialized() { // Arrange - var expected = @"400One or more validation errors occurred. -The State field is required."; + using (new ActivityReplacer()) + { + var expected = "" + + "400" + + "One or more validation errors occurred." + + $"{Activity.Current.Id}" + + "" + + "The State field is required." + + "" + + ""; - // Act - var response = await Client.GetAsync("/api/XmlDataContractApi/ActionReturningValidationProblem"); + // Act + var response = await Client.GetAsync("/api/XmlDataContractApi/ActionReturningValidationProblem"); - // Assert - await response.AssertStatusCodeAsync(HttpStatusCode.BadRequest); - var content = await response.Content.ReadAsStringAsync(); - XmlAssert.Equal(expected, content); + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.BadRequest); + var content = await response.Content.ReadAsStringAsync(); + XmlAssert.Equal(expected, content); + } } [Fact] diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/XmlSerializerFormattersWrappingTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/XmlSerializerFormattersWrappingTest.cs index d54cd3880c..b8b41c5f0a 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/XmlSerializerFormattersWrappingTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/XmlSerializerFormattersWrappingTest.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 System.Diagnostics; using System.Net; using System.Net.Http; using System.Net.Http.Headers; @@ -188,15 +189,23 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests public async Task ProblemDetails_IsSerialized() { // Arrange - var expected = @"404Not Foundhttps://tools.ietf.org/html/rfc7231#section-6.5.4"; + using (new ActivityReplacer()) + { + var expected = "" + + "404" + + "Not Found" + + "https://tools.ietf.org/html/rfc7231#section-6.5.4" + + $"{Activity.Current.Id}" + + ""; - // Act - var response = await Client.GetAsync("/api/XmlSerializerApi/ActionReturningClientErrorStatusCodeResult"); + // Act + var response = await Client.GetAsync("/api/XmlSerializerApi/ActionReturningClientErrorStatusCodeResult"); - // Assert - await response.AssertStatusCodeAsync(HttpStatusCode.NotFound); - var content = await response.Content.ReadAsStringAsync(); - XmlAssert.Equal(expected, content); + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.NotFound); + var content = await response.Content.ReadAsStringAsync(); + XmlAssert.Equal(expected, content); + } } [Fact] @@ -219,16 +228,25 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests public async Task ValidationProblemDetails_IsSerialized() { // Arrange - var expected = @"400One or more validation errors occurred. -The State field is required."; + using (new ActivityReplacer()) + { + var expected = "" + + "400" + + "One or more validation errors occurred." + + $"{Activity.Current.Id}" + + "" + + "The State field is required." + + "" + + ""; - // Act - var response = await Client.GetAsync("/api/XmlSerializerApi/ActionReturningValidationProblem"); + // Act + var response = await Client.GetAsync("/api/XmlSerializerApi/ActionReturningValidationProblem"); - // Assert - await response.AssertStatusCodeAsync(HttpStatusCode.BadRequest); - var content = await response.Content.ReadAsStringAsync(); - XmlAssert.Equal(expected, content); + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.BadRequest); + var content = await response.Content.ReadAsStringAsync(); + XmlAssert.Equal(expected, content); + } } [Fact] From 7bd9f9cc3e11c0e9686453f4cad21ac6bf7ff04a Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Wed, 29 Aug 2018 03:32:20 +0100 Subject: [PATCH 09/21] Reduce IList interface calls --- .../Internal/PagedBufferedTextWriter.cs | 3 +- .../Internal/PagedCharBuffer.cs | 28 +++++++++++-------- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedBufferedTextWriter.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedBufferedTextWriter.cs index 5dce67153b..9148831b6e 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedBufferedTextWriter.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedBufferedTextWriter.cs @@ -37,7 +37,8 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal } var pages = _charBuffer.Pages; - for (var i = 0; i < pages.Count; i++) + var count = pages.Count; + for (var i = 0; i < count; i++) { var page = pages[i]; var pageLength = Math.Min(length, page.Length); diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedCharBuffer.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedCharBuffer.cs index f086daf67b..12dcce04ea 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedCharBuffer.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedCharBuffer.cs @@ -11,6 +11,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal { public const int PageSize = 1024; private int _charIndex; + private List _pages = new List(); public PagedCharBuffer(ICharBufferSource bufferSource) { @@ -19,16 +20,18 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal public ICharBufferSource BufferSource { get; } - public IList Pages { get; } = new List(); + public IList Pages => _pages; public int Length { get { var length = _charIndex; - for (var i = 0; i < Pages.Count - 1; i++) + var pages = _pages; + var fullPages = pages.Count - 1; + for (var i = 0; i < fullPages; i++) { - length += Pages[i].Length; + length += pages[i].Length; } return length; @@ -100,13 +103,14 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal /// public void Clear() { - for (var i = Pages.Count - 1; i > 0; i--) + var pages = _pages; + for (var i = pages.Count - 1; i > 0; i--) { - var page = Pages[i]; + var page = pages[i]; try { - Pages.RemoveAt(i); + pages.RemoveAt(i); } finally { @@ -115,7 +119,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal } _charIndex = 0; - CurrentPage = Pages.Count > 0 ? Pages[0] : null; + CurrentPage = pages.Count > 0 ? pages[0] : null; } private char[] GetCurrentPage() @@ -135,7 +139,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal try { page = BufferSource.Rent(PageSize); - Pages.Add(page); + _pages.Add(page); } catch when (page != null) { @@ -148,12 +152,14 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal public void Dispose() { - for (var i = 0; i < Pages.Count; i++) + var pages = _pages; + var count = pages.Count; + for (var i = 0; i < count; i++) { - BufferSource.Return(Pages[i]); + BufferSource.Return(pages[i]); } - Pages.Clear(); + pages.Clear(); } } } From 22a40b6f2bee8fd96874bf0fa0daa94fcf6a6710 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Wed, 29 Aug 2018 04:42:53 +0100 Subject: [PATCH 10/21] Use Pages as List --- .../Internal/PagedCharBuffer.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedCharBuffer.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedCharBuffer.cs index 12dcce04ea..0c4931c7b9 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedCharBuffer.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/PagedCharBuffer.cs @@ -11,7 +11,6 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal { public const int PageSize = 1024; private int _charIndex; - private List _pages = new List(); public PagedCharBuffer(ICharBufferSource bufferSource) { @@ -20,14 +19,15 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal public ICharBufferSource BufferSource { get; } - public IList Pages => _pages; + // Strongly typed rather than IList for performance + public List Pages { get; } = new List(); public int Length { get { var length = _charIndex; - var pages = _pages; + var pages = Pages; var fullPages = pages.Count - 1; for (var i = 0; i < fullPages; i++) { @@ -103,7 +103,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal /// public void Clear() { - var pages = _pages; + var pages = Pages; for (var i = pages.Count - 1; i > 0; i--) { var page = pages[i]; @@ -139,7 +139,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal try { page = BufferSource.Rent(PageSize); - _pages.Add(page); + Pages.Add(page); } catch when (page != null) { @@ -152,7 +152,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal public void Dispose() { - var pages = _pages; + var pages = Pages; var count = pages.Count; for (var i = 0; i < count; i++) { From c7f6e7ab2fe15affac25cceb0fdb0df127187082 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Wed, 29 Aug 2018 04:52:30 +0100 Subject: [PATCH 11/21] Grumpy XUnit --- .../Internal/PagedCharBufferTest.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/PagedCharBufferTest.cs b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/PagedCharBufferTest.cs index 28944150ff..8c05f896f4 100644 --- a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/PagedCharBufferTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/PagedCharBufferTest.cs @@ -469,14 +469,14 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal // Assert - 1 Assert.Equal(0, buffer.Length); - Assert.Equal(1, buffer.Pages.Count); + Assert.Single(buffer.Pages); // Act - 2 buffer.Append("efgh"); // Assert - 2 Assert.Equal(4, buffer.Length); - Assert.Equal(1, buffer.Pages.Count); + Assert.Single(buffer.Pages); Assert.Equal(new[] { 'e', 'f', 'g', 'h' }, buffer.Pages[0].Take(buffer.Length)); } } From 2a426dfea5982bf654a5dd0cedf138bdfc233afc Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Thu, 30 Aug 2018 01:07:45 +0100 Subject: [PATCH 12/21] Make ViewBuffer methods more inlinable (#8339) * Make ViewBuffer methods more inlinable --- .../Internal/ViewBuffer.cs | 45 +++++++++++++------ .../Internal/ViewBufferPage.cs | 4 ++ 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ViewBuffer.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ViewBuffer.cs index 5143e515d4..7a39276b7b 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ViewBuffer.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ViewBuffer.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.Diagnostics; using System.IO; +using System.Runtime.CompilerServices; using System.Text.Encodings.Web; using System.Threading.Tasks; using Microsoft.AspNetCore.Html; @@ -90,55 +91,73 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal } /// + // Very common trival method; nudge it to inline https://github.com/aspnet/Mvc/pull/8339 + [MethodImpl(MethodImplOptions.AggressiveInlining)] public IHtmlContentBuilder Append(string unencoded) { - if (unencoded == null) + if (unencoded != null) { - return this; + // Text that needs encoding is the uncommon case in views, which is why it + // creates a wrapper and pre-encoded text does not. + AppendValue(new ViewBufferValue(new EncodingWrapper(unencoded))); } - // Text that needs encoding is the uncommon case in views, which is why it - // creates a wrapper and pre-encoded text does not. - AppendValue(new ViewBufferValue(new EncodingWrapper(unencoded))); return this; } /// + // Very common trival method; nudge it to inline https://github.com/aspnet/Mvc/pull/8339 + [MethodImpl(MethodImplOptions.AggressiveInlining)] public IHtmlContentBuilder AppendHtml(IHtmlContent content) { - if (content == null) + if (content != null) { - return this; + AppendValue(new ViewBufferValue(content)); } - AppendValue(new ViewBufferValue(content)); return this; } /// + // Very common trival method; nudge it to inline https://github.com/aspnet/Mvc/pull/8339 + [MethodImpl(MethodImplOptions.AggressiveInlining)] public IHtmlContentBuilder AppendHtml(string encoded) { - if (encoded == null) + if (encoded != null) { - return this; + AppendValue(new ViewBufferValue(encoded)); } - AppendValue(new ViewBufferValue(encoded)); return this; } + // Very common trival method; nudge it to inline https://github.com/aspnet/Mvc/pull/8339 + [MethodImpl(MethodImplOptions.AggressiveInlining)] private void AppendValue(ViewBufferValue value) { var page = GetCurrentPage(); page.Append(value); } + // Very common trival method; nudge it to inline https://github.com/aspnet/Mvc/pull/8339 + [MethodImpl(MethodImplOptions.AggressiveInlining)] private ViewBufferPage GetCurrentPage() { - if (_currentPage == null || _currentPage.IsFull) + var currentPage = _currentPage; + if (currentPage == null || currentPage.IsFull) { - AddPage(new ViewBufferPage(_bufferScope.GetPage(_pageSize))); + // Uncommon slow-path + return AppendNewPage(); } + + return currentPage; + } + + // Slow path for above, don't inline + [MethodImpl(MethodImplOptions.NoInlining)] + private ViewBufferPage AppendNewPage() + { + AddPage(new ViewBufferPage(_bufferScope.GetPage(_pageSize))); return _currentPage; } diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ViewBufferPage.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ViewBufferPage.cs index b561e71825..416e329bd7 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ViewBufferPage.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ViewBufferPage.cs @@ -1,6 +1,8 @@ // 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.Runtime.CompilerServices; + namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal { public class ViewBufferPage @@ -18,6 +20,8 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal public bool IsFull => Count == Capacity; + // Very common trival method; nudge it to inline https://github.com/aspnet/Mvc/pull/8339 + [MethodImpl(MethodImplOptions.AggressiveInlining)] public void Append(ViewBufferValue value) => Buffer[Count++] = value; } } From ffdbea9dc1aa5888ddca3db24b15864e6eae4e01 Mon Sep 17 00:00:00 2001 From: Kristian Hellang Date: Wed, 8 Aug 2018 12:50:46 +0200 Subject: [PATCH 13/21] Add analyzer support for status code methods and constructors --- .../CodeAnalysisExtensions.cs | 2 +- .../MvcFacts.cs | 5 + .../ApiControllerSymbolCache.cs | 10 + .../ApiSymbolNames.cs | 4 + .../SymbolApiResponseMetadataProvider.cs | 173 +++++++++++++++++- .../ControllerBase.cs | 11 +- .../StatusCodeValueAttribute.cs | 12 ++ .../StatusCodeResult.cs | 2 +- ...AttributeCodeFixProviderIntegrationTest.cs | 12 ++ ...ForNonExistingStatusCodeConstants.Input.cs | 17 ++ ...orNonExistingStatusCodeConstants.Output.cs | 22 +++ ...tusCodesFromConstructorParameters.Input.cs | 38 ++++ ...usCodesFromConstructorParameters.Output.cs | 44 +++++ ...dsStatusCodesFromMethodParameters.Input.cs | 38 ++++ ...sStatusCodesFromMethodParameters.Output.cs | 44 +++++ ...sStatusCodesFromObjectInitializer.Input.cs | 51 ++++++ ...StatusCodesFromObjectInitializer.Output.cs | 57 ++++++ 17 files changed, 526 insertions(+), 16 deletions(-) create mode 100644 src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/StatusCodeValueAttribute.cs create mode 100644 test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsNumericLiteralForNonExistingStatusCodeConstants.Input.cs create mode 100644 test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsNumericLiteralForNonExistingStatusCodeConstants.Output.cs create mode 100644 test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromConstructorParameters.Input.cs create mode 100644 test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromConstructorParameters.Output.cs create mode 100644 test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromMethodParameters.Input.cs create mode 100644 test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromMethodParameters.Output.cs create mode 100644 test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromObjectInitializer.Input.cs create mode 100644 test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromObjectInitializer.Output.cs diff --git a/src/Microsoft.AspNetCore.Mvc.Analyzers/CodeAnalysisExtensions.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/CodeAnalysisExtensions.cs index 7856b27b02..72da1b1f4c 100644 --- a/src/Microsoft.AspNetCore.Mvc.Analyzers/CodeAnalysisExtensions.cs +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/CodeAnalysisExtensions.cs @@ -123,7 +123,7 @@ namespace Microsoft.CodeAnalysis return false; } - private static bool HasAttribute(this ISymbol symbol, ITypeSymbol attribute) + public static bool HasAttribute(this ISymbol symbol, ITypeSymbol attribute) { foreach (var declaredAttribute in symbol.GetAttributes()) { diff --git a/src/Microsoft.AspNetCore.Mvc.Analyzers/MvcFacts.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/MvcFacts.cs index 90651247c4..fc06eda6bb 100644 --- a/src/Microsoft.AspNetCore.Mvc.Analyzers/MvcFacts.cs +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/MvcFacts.cs @@ -116,6 +116,11 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers return false; } + if (!method.ReturnsVoid) + { + return false; + } + if (method.Parameters.Length != disposableDispose.Parameters.Length) { return false; diff --git a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiControllerSymbolCache.cs b/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiControllerSymbolCache.cs index c2a6f167d2..e92c0927ca 100644 --- a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiControllerSymbolCache.cs +++ b/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiControllerSymbolCache.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Diagnostics; using Microsoft.CodeAnalysis; namespace Microsoft.AspNetCore.Mvc.Api.Analyzers @@ -24,6 +25,11 @@ namespace Microsoft.AspNetCore.Mvc.Api.Analyzers ProducesDefaultResponseTypeAttribute = compilation.GetTypeByMetadataName(ApiSymbolNames.ProducesDefaultResponseTypeAttribute); ProducesResponseTypeAttribute = compilation.GetTypeByMetadataName(ApiSymbolNames.ProducesResponseTypeAttribute); + StatusCodeValueAttribute = compilation.GetTypeByMetadataName(ApiSymbolNames.StatusCodeValueAttribute); + + var statusCodeActionResult = compilation.GetTypeByMetadataName(ApiSymbolNames.IStatusCodeActionResult); + StatusCodeActionResultStatusProperty = (IPropertySymbol)statusCodeActionResult.GetMembers("StatusCode")[0]; + var disposable = compilation.GetSpecialType(SpecialType.System_IDisposable); var members = disposable.GetMembers(nameof(IDisposable.Dispose)); IDisposableDispose = members.Length == 1 ? (IMethodSymbol)members[0] : null; @@ -47,6 +53,8 @@ namespace Microsoft.AspNetCore.Mvc.Api.Analyzers public IMethodSymbol IDisposableDispose { get; } + public IPropertySymbol StatusCodeActionResultStatusProperty { get; } + public ITypeSymbol ModelStateDictionary { get; } public INamedTypeSymbol NonActionAttribute { get; } @@ -56,5 +64,7 @@ namespace Microsoft.AspNetCore.Mvc.Api.Analyzers public INamedTypeSymbol ProducesDefaultResponseTypeAttribute { get; } public INamedTypeSymbol ProducesResponseTypeAttribute { get; } + + public INamedTypeSymbol StatusCodeValueAttribute { get; } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiSymbolNames.cs b/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiSymbolNames.cs index 0771fafc22..563c3ad570 100644 --- a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiSymbolNames.cs +++ b/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiSymbolNames.cs @@ -23,6 +23,8 @@ namespace Microsoft.AspNetCore.Mvc.Api.Analyzers public const string IActionResult = "Microsoft.AspNetCore.Mvc.IActionResult"; + public const string IStatusCodeActionResult = "Microsoft.AspNetCore.Mvc.Infrastructure.IStatusCodeActionResult"; + public const string ModelStateDictionary = "Microsoft.AspNetCore.Mvc.ModelBinding.ModelStateDictionary"; public const string NonActionAttribute = "Microsoft.AspNetCore.Mvc.NonActionAttribute"; @@ -34,5 +36,7 @@ namespace Microsoft.AspNetCore.Mvc.Api.Analyzers public const string ProducesResponseTypeAttribute = "Microsoft.AspNetCore.Mvc.ProducesResponseTypeAttribute"; public const string HttpStatusCodes = "Microsoft.AspNetCore.Http.StatusCodes"; + + public const string StatusCodeValueAttribute = "Microsoft.AspNetCore.Mvc.Infrastructure.StatusCodeValueAttribute"; } } diff --git a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/SymbolApiResponseMetadataProvider.cs b/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/SymbolApiResponseMetadataProvider.cs index 5de70bae0a..7e3744757e 100644 --- a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/SymbolApiResponseMetadataProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/SymbolApiResponseMetadataProvider.cs @@ -61,7 +61,8 @@ namespace Microsoft.AspNetCore.Mvc.Api.Analyzers } return Array.Empty(); - } + } + private static IMethodSymbol GetMethodFromConventionMethodAttribute(ApiControllerSymbolCache symbolCache, IMethodSymbol method) { var attribute = method.GetAttributes(symbolCache.ApiConventionMethodAttribute, inherit: true) @@ -222,6 +223,12 @@ namespace Microsoft.AspNetCore.Mvc.Api.Analyzers foreach (var returnStatementSyntax in methodSyntax.DescendantNodes(_shouldDescendIntoChildren).OfType()) { + if (returnStatementSyntax.IsMissing || returnStatementSyntax.Expression.IsMissing) + { + // Ignore malformed return statements. + continue; + } + var responseMetadata = InspectReturnStatementSyntax( symbolCache, semanticModel, @@ -248,11 +255,6 @@ namespace Microsoft.AspNetCore.Mvc.Api.Analyzers CancellationToken cancellationToken) { var returnExpression = returnStatementSyntax.Expression; - if (returnExpression.IsMissing) - { - return null; - } - var typeInfo = semanticModel.GetTypeInfo(returnExpression, cancellationToken); if (typeInfo.Type.TypeKind == TypeKind.Error) { @@ -267,25 +269,176 @@ namespace Microsoft.AspNetCore.Mvc.Api.Analyzers if (defaultStatusCodeAttribute != null) { - var statusCode = GetDefaultStatusCode(defaultStatusCodeAttribute); - if (statusCode == null) + var defaultStatusCode = GetDefaultStatusCode(defaultStatusCodeAttribute); + if (defaultStatusCode == null) { // Unable to read the status code even though the attribute exists. return null; } - return new ActualApiResponseMetadata(returnStatementSyntax, statusCode.Value); + return new ActualApiResponseMetadata(returnStatementSyntax, defaultStatusCode.Value); } - else if (!symbolCache.IActionResult.IsAssignableFrom(statementReturnType)) + + 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); } + int statusCode; + switch (returnExpression) + { + case InvocationExpressionSyntax invocation: + // Covers the 'return StatusCode(200)' case. + if (TryGetParameterStatusCode(symbolCache, semanticModel, invocation.Expression, invocation.ArgumentList, cancellationToken, out statusCode)) + { + return new ActualApiResponseMetadata(returnStatementSyntax, statusCode); + } + break; + + case ObjectCreationExpressionSyntax creation: + // Covers the 'return new ObjectResult(...) { StatusCode = 200 }' case. + if (TryGetInitializerStatusCode(symbolCache, semanticModel, creation.Initializer, cancellationToken, out statusCode)) + { + return new ActualApiResponseMetadata(returnStatementSyntax, statusCode); + } + + // Covers the 'return new StatusCodeResult(200) case. + if (TryGetParameterStatusCode(symbolCache, semanticModel, creation, creation.ArgumentList, cancellationToken, out statusCode)) + { + return new ActualApiResponseMetadata(returnStatementSyntax, statusCode); + } + break; + } + return null; } + private static bool TryGetInitializerStatusCode( + in ApiControllerSymbolCache symbolCache, + SemanticModel semanticModel, + InitializerExpressionSyntax initializer, + CancellationToken cancellationToken, + out int statusCode) + { + if (initializer == null) + { + statusCode = default; + return false; + } + + for (var i = 0; i < initializer.Expressions.Count; i++) + { + if (!(initializer.Expressions[i] is AssignmentExpressionSyntax assignment)) + { + continue; + } + + if (assignment.Left is IdentifierNameSyntax identifier) + { + var symbolInfo = semanticModel.GetSymbolInfo(identifier, cancellationToken); + + if (symbolInfo.Symbol is IPropertySymbol property && IsInterfaceImplementation(property, symbolCache.StatusCodeActionResultStatusProperty)) + { + return TryGetExpressionStatusCode(semanticModel, assignment.Right, cancellationToken, out statusCode); + } + } + } + + statusCode = default; + return false; + } + + private static bool IsInterfaceImplementation(IPropertySymbol property, IPropertySymbol statusCodeActionResultStatusProperty) + { + if (property.Name != statusCodeActionResultStatusProperty.Name) + { + return false; + } + + for (var i = 0; i < property.ExplicitInterfaceImplementations.Length; i++) + { + if (property.ExplicitInterfaceImplementations[i] == statusCodeActionResultStatusProperty) + { + return true; + } + } + + var implementedProperty = property.ContainingType.FindImplementationForInterfaceMember(statusCodeActionResultStatusProperty); + return implementedProperty == property; + } + + private static bool TryGetParameterStatusCode( + in ApiControllerSymbolCache symbolCache, + SemanticModel semanticModel, + ExpressionSyntax expression, + BaseArgumentListSyntax argumentList, + CancellationToken cancellationToken, + out int statusCode) + { + var symbolInfo = semanticModel.GetSymbolInfo(expression, cancellationToken); + + if (!(symbolInfo.Symbol is IMethodSymbol method)) + { + statusCode = default; + return false; + } + + for (var i = 0; i < method.Parameters.Length; i++) + { + var parameter = method.Parameters[i]; + if (!parameter.HasAttribute(symbolCache.StatusCodeValueAttribute)) + { + continue; + } + + + var argument = argumentList.Arguments[parameter.Ordinal]; + return TryGetExpressionStatusCode(semanticModel, argument.Expression, cancellationToken, out statusCode); + } + + statusCode = default; + return false; + } + + private static bool TryGetExpressionStatusCode( + SemanticModel semanticModel, + ExpressionSyntax expression, + CancellationToken cancellationToken, + out int statusCode) + { + if (expression is LiteralExpressionSyntax literal && literal.Token.Value is int literalStatusCode) + { + // Covers the 'return StatusCode(200)' case. + statusCode = literalStatusCode; + return true; + } + + if (expression is IdentifierNameSyntax || expression is MemberAccessExpressionSyntax) + { + var symbolInfo = semanticModel.GetSymbolInfo(expression, cancellationToken); + + if (symbolInfo.Symbol is IFieldSymbol field && field.HasConstantValue && field.ConstantValue is int constantStatusCode) + { + // Covers the 'return StatusCode(StatusCodes.Status200OK)' case. + // It also covers the 'return StatusCode(StatusCode)' case, where 'StatusCode' is a constant field. + statusCode = constantStatusCode; + return true; + } + + if (symbolInfo.Symbol is ILocalSymbol local && local.HasConstantValue && local.ConstantValue is int localStatusCode) + { + // Covers the 'return StatusCode(statusCode)' case, where 'statusCode' is a local constant. + statusCode = localStatusCode; + return true; + } + } + + statusCode = default; + return false; + } + private static bool ShouldDescendIntoChildren(SyntaxNode syntaxNode) { return !syntaxNode.IsKind(SyntaxKind.LocalFunctionStatement) && diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs b/src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs index c51f809da2..520f8a5fbe 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs @@ -10,6 +10,7 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Core; +using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.ModelBinding; using Microsoft.AspNetCore.Mvc.ModelBinding.Internal; using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; @@ -200,7 +201,7 @@ namespace Microsoft.AspNetCore.Mvc /// The status code to set on the response. /// The created object for the response. [NonAction] - public virtual StatusCodeResult StatusCode(int statusCode) + public virtual StatusCodeResult StatusCode([StatusCodeValue] int statusCode) => new StatusCodeResult(statusCode); /// @@ -210,10 +211,12 @@ namespace Microsoft.AspNetCore.Mvc /// The value to set on the . /// The created object for the response. [NonAction] - public virtual ObjectResult StatusCode(int statusCode, object value) + public virtual ObjectResult StatusCode([StatusCodeValue] int statusCode, object value) { - var result = new ObjectResult(value); - result.StatusCode = statusCode; + var result = new ObjectResult(value) + { + StatusCode = statusCode + }; return result; } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/StatusCodeValueAttribute.cs b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/StatusCodeValueAttribute.cs new file mode 100644 index 0000000000..944f958d62 --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/StatusCodeValueAttribute.cs @@ -0,0 +1,12 @@ +// 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; + +namespace Microsoft.AspNetCore.Mvc.Infrastructure +{ + [AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)] + internal sealed class StatusCodeValueAttribute : Attribute + { + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.Core/StatusCodeResult.cs b/src/Microsoft.AspNetCore.Mvc.Core/StatusCodeResult.cs index c3ec5614c0..c95b69237e 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/StatusCodeResult.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/StatusCodeResult.cs @@ -20,7 +20,7 @@ namespace Microsoft.AspNetCore.Mvc /// with the given . /// /// The HTTP status code of the response. - public StatusCodeResult(int statusCode) + public StatusCodeResult([StatusCodeValue] int statusCode) { StatusCode = statusCode; } diff --git a/test/Mvc.Api.Analyzers.Test/AddResponseTypeAttributeCodeFixProviderIntegrationTest.cs b/test/Mvc.Api.Analyzers.Test/AddResponseTypeAttributeCodeFixProviderIntegrationTest.cs index beea4d68a7..755fe9b136 100644 --- a/test/Mvc.Api.Analyzers.Test/AddResponseTypeAttributeCodeFixProviderIntegrationTest.cs +++ b/test/Mvc.Api.Analyzers.Test/AddResponseTypeAttributeCodeFixProviderIntegrationTest.cs @@ -33,6 +33,18 @@ namespace Microsoft.AspNetCore.Mvc.Api.Analyzers [Fact] public Task CodeFixAddsFullyQualifiedProducesResponseType() => RunTest(); + [Fact] + public Task CodeFixAddsNumericLiteralForNonExistingStatusCodeConstants() => RunTest(); + + [Fact] + public Task CodeFixAddsStatusCodesFromMethodParameters() => RunTest(); + + [Fact] + public Task CodeFixAddsStatusCodesFromConstructorParameters() => RunTest(); + + [Fact] + public Task CodeFixAddsStatusCodesFromObjectInitializer() => RunTest(); + private async Task RunTest([CallerMemberName] string testMethod = "") { // Arrange diff --git a/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsNumericLiteralForNonExistingStatusCodeConstants.Input.cs b/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsNumericLiteralForNonExistingStatusCodeConstants.Input.cs new file mode 100644 index 0000000000..93668878b1 --- /dev/null +++ b/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsNumericLiteralForNonExistingStatusCodeConstants.Input.cs @@ -0,0 +1,17 @@ +namespace Microsoft.AspNetCore.Mvc.Api.Analyzers._INPUT_ +{ + [ApiController] + [Route("[controller]/[action]")] + public class CodeFixAddsNumericLiteralForNonExistingStatusCodeConstantsController : ControllerBase + { + public IActionResult GetItem(int id) + { + if (id == 0) + { + return StatusCode(345); + } + + return Ok(new object()); + } + } +} diff --git a/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsNumericLiteralForNonExistingStatusCodeConstants.Output.cs b/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsNumericLiteralForNonExistingStatusCodeConstants.Output.cs new file mode 100644 index 0000000000..82a8f44a3b --- /dev/null +++ b/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsNumericLiteralForNonExistingStatusCodeConstants.Output.cs @@ -0,0 +1,22 @@ +using Microsoft.AspNetCore.Http; + +namespace Microsoft.AspNetCore.Mvc.Api.Analyzers._OUTPUT_ +{ + [ApiController] + [Route("[controller]/[action]")] + public class CodeFixAddsNumericLiteralForNonExistingStatusCodeConstantsController : ControllerBase + { + [ProducesResponseType(StatusCodes.Status200OK)] + [ProducesResponseType(345)] + [ProducesDefaultResponseType] + public IActionResult GetItem(int id) + { + if (id == 0) + { + return StatusCode(345); + } + + return Ok(new object()); + } + } +} diff --git a/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromConstructorParameters.Input.cs b/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromConstructorParameters.Input.cs new file mode 100644 index 0000000000..0fe4637037 --- /dev/null +++ b/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromConstructorParameters.Input.cs @@ -0,0 +1,38 @@ +using Microsoft.AspNetCore.Http; + +namespace Microsoft.AspNetCore.Mvc.Api.Analyzers._INPUT_ +{ + [ApiController] + [Route("[controller]/[action]")] + public class CodeFixAddsStatusCodesFromConstructorParametersController : ControllerBase + { + private const int FieldStatusCode = 201; + + public IActionResult GetItem(int id) + { + if (id == 0) + { + return new StatusCodeResult(422); + } + + if (id == 1) + { + return new StatusCodeResult(StatusCodes.Status202Accepted); + } + + if (id == 2) + { + const int localStatusCode = 204; + + return new StatusCodeResult(localStatusCode); + } + + if (id == 3) + { + return new StatusCodeResult(FieldStatusCode); + } + + return Ok(new object()); + } + } +} diff --git a/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromConstructorParameters.Output.cs b/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromConstructorParameters.Output.cs new file mode 100644 index 0000000000..dbdf8d7fae --- /dev/null +++ b/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromConstructorParameters.Output.cs @@ -0,0 +1,44 @@ +using Microsoft.AspNetCore.Http; + +namespace Microsoft.AspNetCore.Mvc.Api.Analyzers._OUTPUT_ +{ + [ApiController] + [Route("[controller]/[action]")] + public class CodeFixAddsStatusCodesFromConstructorParametersController : ControllerBase + { + private const int FieldStatusCode = 201; + + [ProducesResponseType(StatusCodes.Status200OK)] + [ProducesResponseType(StatusCodes.Status201Created)] + [ProducesResponseType(StatusCodes.Status202Accepted)] + [ProducesResponseType(StatusCodes.Status204NoContent)] + [ProducesResponseType(StatusCodes.Status422UnprocessableEntity)] + [ProducesDefaultResponseType] + public IActionResult GetItem(int id) + { + if (id == 0) + { + return new StatusCodeResult(422); + } + + if (id == 1) + { + return new StatusCodeResult(StatusCodes.Status202Accepted); + } + + if (id == 2) + { + const int localStatusCode = 204; + + return new StatusCodeResult(localStatusCode); + } + + if (id == 3) + { + return new StatusCodeResult(FieldStatusCode); + } + + return Ok(new object()); + } + } +} diff --git a/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromMethodParameters.Input.cs b/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromMethodParameters.Input.cs new file mode 100644 index 0000000000..8bc4a372e1 --- /dev/null +++ b/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromMethodParameters.Input.cs @@ -0,0 +1,38 @@ +using Microsoft.AspNetCore.Http; + +namespace Microsoft.AspNetCore.Mvc.Api.Analyzers._INPUT_ +{ + [ApiController] + [Route("[controller]/[action]")] + public class CodeFixAddsStatusCodesFromMethodParametersController : ControllerBase + { + private const int FieldStatusCode = 201; + + public IActionResult GetItem(int id) + { + if (id == 0) + { + return StatusCode(422); + } + + if (id == 1) + { + return StatusCode(StatusCodes.Status202Accepted); + } + + if (id == 2) + { + const int localStatusCode = 204; + + return StatusCode(localStatusCode); + } + + if (id == 3) + { + return StatusCode(FieldStatusCode); + } + + return Ok(new object()); + } + } +} diff --git a/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromMethodParameters.Output.cs b/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromMethodParameters.Output.cs new file mode 100644 index 0000000000..d1ad5d182d --- /dev/null +++ b/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromMethodParameters.Output.cs @@ -0,0 +1,44 @@ +using Microsoft.AspNetCore.Http; + +namespace Microsoft.AspNetCore.Mvc.Api.Analyzers._OUTPUT_ +{ + [ApiController] + [Route("[controller]/[action]")] + public class CodeFixAddsStatusCodesFromMethodParametersController : ControllerBase + { + private const int FieldStatusCode = 201; + + [ProducesResponseType(StatusCodes.Status200OK)] + [ProducesResponseType(StatusCodes.Status201Created)] + [ProducesResponseType(StatusCodes.Status202Accepted)] + [ProducesResponseType(StatusCodes.Status204NoContent)] + [ProducesResponseType(StatusCodes.Status422UnprocessableEntity)] + [ProducesDefaultResponseType] + public IActionResult GetItem(int id) + { + if (id == 0) + { + return StatusCode(422); + } + + if (id == 1) + { + return StatusCode(StatusCodes.Status202Accepted); + } + + if (id == 2) + { + const int localStatusCode = 204; + + return StatusCode(localStatusCode); + } + + if (id == 3) + { + return StatusCode(FieldStatusCode); + } + + return Ok(new object()); + } + } +} diff --git a/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromObjectInitializer.Input.cs b/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromObjectInitializer.Input.cs new file mode 100644 index 0000000000..7041164c5e --- /dev/null +++ b/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromObjectInitializer.Input.cs @@ -0,0 +1,51 @@ +using Microsoft.AspNetCore.Http; + +namespace Microsoft.AspNetCore.Mvc.Api.Analyzers._INPUT_ +{ + [ApiController] + [Route("[controller]/[action]")] + public class CodeFixAddsStatusCodesFromObjectInitializerController : ControllerBase + { + private const int FieldStatusCode = 201; + + public IActionResult GetItem(int id) + { + if (id == 0) + { + return new ObjectResult(new object()) + { + StatusCode = 422 + }; + } + + if (id == 1) + { + return new ObjectResult(new object()) + { + StatusCode = StatusCodes.Status202Accepted + }; + } + + if (id == 2) + { + const int localStatusCode = 204; + + return new ObjectResult(new object()) + { + StatusCode = localStatusCode + }; + } + + if (id == 3) + { + return new ObjectResult(new object()) + { + ContentTypes = { "application/json" }, + StatusCode = FieldStatusCode + }; + } + + return Ok(new object()); + } + } +} diff --git a/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromObjectInitializer.Output.cs b/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromObjectInitializer.Output.cs new file mode 100644 index 0000000000..8f35baeee7 --- /dev/null +++ b/test/Mvc.Api.Analyzers.Test/TestFiles/AddResponseTypeAttributeCodeFixProviderIntegrationTest/CodeFixAddsStatusCodesFromObjectInitializer.Output.cs @@ -0,0 +1,57 @@ +using Microsoft.AspNetCore.Http; + +namespace Microsoft.AspNetCore.Mvc.Api.Analyzers._OUTPUT_ +{ + [ApiController] + [Route("[controller]/[action]")] + public class CodeFixAddsStatusCodesFromObjectInitializerController : ControllerBase + { + private const int FieldStatusCode = 201; + + [ProducesResponseType(StatusCodes.Status200OK)] + [ProducesResponseType(StatusCodes.Status201Created)] + [ProducesResponseType(StatusCodes.Status202Accepted)] + [ProducesResponseType(StatusCodes.Status204NoContent)] + [ProducesResponseType(StatusCodes.Status422UnprocessableEntity)] + [ProducesDefaultResponseType] + public IActionResult GetItem(int id) + { + if (id == 0) + { + return new ObjectResult(new object()) + { + StatusCode = 422 + }; + } + + if (id == 1) + { + return new ObjectResult(new object()) + { + StatusCode = StatusCodes.Status202Accepted + }; + } + + if (id == 2) + { + const int localStatusCode = 204; + + return new ObjectResult(new object()) + { + StatusCode = localStatusCode + }; + } + + if (id == 3) + { + return new ObjectResult(new object()) + { + ContentTypes = { "application/json" }, + StatusCode = FieldStatusCode + }; + } + + return Ok(new object()); + } + } +} From 5cdc172b17cd339c2a686b2191d3729613ae7441 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Thu, 30 Aug 2018 13:28:46 +1200 Subject: [PATCH 14/21] Fix obsolete constraint resolver usage (#8361) --- .../RemoteAttributeTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/RemoteAttributeTest.cs b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/RemoteAttributeTest.cs index 5c8da2b478..5742da183b 100644 --- a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/RemoteAttributeTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/RemoteAttributeTest.cs @@ -1045,7 +1045,7 @@ namespace Microsoft.AspNetCore.Mvc serviceCollection.AddRouting(); serviceCollection.AddSingleton( - provider => new DefaultInlineConstraintResolver(provider.GetRequiredService>())); + provider => new DefaultInlineConstraintResolver(provider.GetRequiredService>(), provider)); if (localizerFactory != null) { From 927e7c8bfc449513a25a2f6e796c684c293c2cde Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Thu, 30 Aug 2018 15:14:34 +1200 Subject: [PATCH 15/21] Support route data tokens with Endpoint Routing (#8360) --- .../Internal/MvcEndpointDataSource.cs | 26 ++++++++++++------- .../Internal/MvcEndpointDataSourceTests.cs | 23 +--------------- .../RoutingTestsBase.cs | 24 +++++++++++++++++ .../Controllers/DataTokensController.cs | 15 +++++++++++ test/WebSites/RoutingWebSite/Startup.cs | 17 ++++++++---- .../RoutingWebSite/StartupWith21Compat.cs | 17 ++++++++---- 6 files changed, 80 insertions(+), 42 deletions(-) create mode 100644 test/WebSites/RoutingWebSite/Controllers/DataTokensController.cs diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs index 2d3f5e4ba2..c99e3a216d 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs @@ -174,6 +174,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal endpointInfo.Defaults, ++conventionalRouteOrder, endpointInfo, + endpointInfo.DataTokens, suppressLinkGeneration: false, suppressPathMatching: false); endpoints.Add(subEndpoint); @@ -214,6 +215,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal endpointInfo.Defaults, ++conventionalRouteOrder, endpointInfo, + endpointInfo.DataTokens, suppressLinkGeneration: false, suppressPathMatching: false); endpoints.Add(endpoint); @@ -229,6 +231,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal nonInlineDefaults: null, action.AttributeRouteInfo.Order, action.AttributeRouteInfo, + dataTokens: null, suppressLinkGeneration: action.AttributeRouteInfo.SuppressLinkGeneration, suppressPathMatching: action.AttributeRouteInfo.SuppressPathMatching); endpoints.Add(endpoint); @@ -378,20 +381,13 @@ namespace Microsoft.AspNetCore.Mvc.Internal object nonInlineDefaults, int order, object source, + RouteValueDictionary dataTokens, bool suppressLinkGeneration, bool suppressPathMatching) { RequestDelegate requestDelegate = (context) => { - var values = context.Features.Get().RouteValues; - var routeData = new RouteData(); - foreach (var kvp in values) - { - if (kvp.Value != null) - { - routeData.Values.Add(kvp.Key, kvp.Value); - } - } + var routeData = context.GetRouteData(); var actionContext = new ActionContext(context, routeData, action); @@ -407,6 +403,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal routeName, new RouteValueDictionary(action.RouteValues), source, + dataTokens, suppressLinkGeneration, suppressPathMatching); @@ -425,6 +422,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal string routeName, RouteValueDictionary requiredValues, object source, + RouteValueDictionary dataTokens, bool suppressLinkGeneration, bool suppressPathMatching) { @@ -438,6 +436,11 @@ namespace Microsoft.AspNetCore.Mvc.Internal metadata.AddRange(action.EndpointMetadata); } + if (dataTokens != null) + { + metadata.Add(new DataTokensMetadata(dataTokens)); + } + metadata.Add(new RouteValuesAddressMetadata(routeName, requiredValues)); // Add filter descriptors to endpoint metadata @@ -506,7 +509,10 @@ namespace Microsoft.AspNetCore.Mvc.Internal { foreach (var kvp in requiredValues) { - defaults[kvp.Key] = kvp.Value; + if (kvp.Value != null) + { + defaults[kvp.Key] = kvp.Value; + } } } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs index fe5d8dbbf7..a6f92ddc2c 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs @@ -89,6 +89,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal var featureCollection = new FeatureCollection(); featureCollection.Set(endpointFeature); featureCollection.Set(endpointFeature); + featureCollection.Set(endpointFeature); var httpContextMock = new Mock(); httpContextMock.Setup(m => m.Features).Returns(featureCollection); @@ -694,28 +695,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal AssertIsSubset(expectedDefaults, matcherEndpoint.RoutePattern.Defaults); } - [Fact] - public void RequiredValues_HavingNull_AndNotPresentInDefaultValues_IsAddedToDefaultValues() - { - // Arrange - var requiredValues = new RouteValueDictionary( - new { area = (string)null, controller = "Foo", action = "Bar", page = (string)null }); - var expectedDefaults = requiredValues; - var actionDescriptorCollection = GetActionDescriptorCollection(requiredValues: requiredValues); - var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); - dataSource.ConventionalEndpointInfos.Add( - CreateEndpointInfo(string.Empty, "{controller=Home}/{action=Index}")); - - // Act - var endpoints = dataSource.Endpoints; - - // Assert - var endpoint = Assert.Single(endpoints); - var matcherEndpoint = Assert.IsType(endpoint); - Assert.Equal("Foo/Bar", matcherEndpoint.RoutePattern.RawText); - AssertIsSubset(expectedDefaults, matcherEndpoint.RoutePattern.Defaults); - } - private MvcEndpointDataSource CreateMvcEndpointDataSource( IActionDescriptorCollectionProvider actionDescriptorCollectionProvider = null, MvcEndpointInvokerFactory mvcEndpointInvokerFactory = null) diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RoutingTestsBase.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RoutingTestsBase.cs index 6430ff2005..464a36beb5 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RoutingTestsBase.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RoutingTestsBase.cs @@ -72,6 +72,30 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests public string[] Routers { get; set; } } + [Fact] + public async Task DataTokens_ReturnsDataTokensForRoute() + { + // Arrange & Act + var response = await Client.GetAsync("http://localhost/DataTokensRoute/DataTokens/Index"); + + // Assert + var body = await response.Content.ReadAsStringAsync(); + var result = JsonConvert.DeserializeObject>(body); + Assert.Single(result, kvp => kvp.Key == "hasDataTokens" && ((bool)kvp.Value) == true); + } + + [Fact] + public async Task DataTokens_ReturnsNoDataTokensForRoute() + { + // Arrange & Act + var response = await Client.GetAsync("http://localhost/DataTokens/Index"); + + // Assert + var body = await response.Content.ReadAsStringAsync(); + var result = JsonConvert.DeserializeObject>(body); + Assert.Empty(result); + } + [Fact] public virtual async Task ConventionalRoutedController_ActionIsReachable() { diff --git a/test/WebSites/RoutingWebSite/Controllers/DataTokensController.cs b/test/WebSites/RoutingWebSite/Controllers/DataTokensController.cs new file mode 100644 index 0000000000..b6ae438777 --- /dev/null +++ b/test/WebSites/RoutingWebSite/Controllers/DataTokensController.cs @@ -0,0 +1,15 @@ +// 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; + +namespace RoutingWebSite +{ + public class DataTokensController : Controller + { + public object Index() + { + return RouteData.DataTokens; + } + } +} \ No newline at end of file diff --git a/test/WebSites/RoutingWebSite/Startup.cs b/test/WebSites/RoutingWebSite/Startup.cs index 984e4cc5c2..cea64f826f 100644 --- a/test/WebSites/RoutingWebSite/Startup.cs +++ b/test/WebSites/RoutingWebSite/Startup.cs @@ -24,12 +24,19 @@ namespace RoutingWebSite { app.UseMvc(routes => { + routes.MapRoute( + "DataTokensRoute", + "DataTokensRoute/{controller}/{action}", + defaults: null, + constraints: new { controller = "DataTokens" }, + dataTokens: new { hasDataTokens = true }); + routes.MapAreaRoute( - "flightRoute", - "adminRoute", - "{area:exists}/{controller}/{action}", - new { controller = "Home", action = "Index" }, - new { area = "Travel" }); + "flightRoute", + "adminRoute", + "{area:exists}/{controller}/{action}", + defaults: new { controller = "Home", action = "Index" }, + constraints: new { area = "Travel" }); routes.MapRoute( "ActionAsMethod", diff --git a/test/WebSites/RoutingWebSite/StartupWith21Compat.cs b/test/WebSites/RoutingWebSite/StartupWith21Compat.cs index 33c10a0275..1b34b2cfb0 100644 --- a/test/WebSites/RoutingWebSite/StartupWith21Compat.cs +++ b/test/WebSites/RoutingWebSite/StartupWith21Compat.cs @@ -24,12 +24,19 @@ namespace RoutingWebSite { app.UseMvc(routes => { + routes.MapRoute( + "DataTokensRoute", + "DataTokensRoute/{controller}/{action}", + defaults: null, + constraints: new { controller = "DataTokens" }, + dataTokens: new { hasDataTokens = true }); + routes.MapAreaRoute( - "flightRoute", - "adminRoute", - "{area:exists}/{controller}/{action}", - new { controller = "Home", action = "Index" }, - new { area = "Travel" }); + "flightRoute", + "adminRoute", + "{area:exists}/{controller}/{action}", + defaults: new { controller = "Home", action = "Index" }, + constraints: new { area = "Travel" }); routes.MapRoute( "ActionAsMethod", From f90a47c5aff7c805b8102e3d07c71687afc6c061 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Wed, 22 Aug 2018 13:17:42 -0700 Subject: [PATCH 16/21] Introduce ProducesErrorResponseTypeAttribute Fixes https://github.com/aspnet/Mvc/issues/8288 --- .../ApiResponseTypeProvider.cs | 73 +++--- .../ApiBehaviorApplicationModelProvider.cs | 22 ++ .../ProducesErrorResponseTypeAttribute.cs | 38 +++ .../ApiResponseTypeProviderTest.cs | 241 ++++++++++++++++++ ...ApiBehaviorApplicationModelProviderTest.cs | 167 +++++++++++- .../ApiExplorerTest.cs | 46 ++-- 6 files changed, 531 insertions(+), 56 deletions(-) create mode 100644 src/Microsoft.AspNetCore.Mvc.Core/ProducesErrorResponseTypeAttribute.cs diff --git a/src/Microsoft.AspNetCore.Mvc.ApiExplorer/ApiResponseTypeProvider.cs b/src/Microsoft.AspNetCore.Mvc.ApiExplorer/ApiResponseTypeProvider.cs index ec791aaee0..d2eec34199 100644 --- a/src/Microsoft.AspNetCore.Mvc.ApiExplorer/ApiResponseTypeProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.ApiExplorer/ApiResponseTypeProvider.cs @@ -46,7 +46,13 @@ namespace Microsoft.AspNetCore.Mvc.ApiExplorer responseMetadataAttributes = apiConventionResult.ResponseMetadataProviders; } - var apiResponseTypes = GetApiResponseTypes(responseMetadataAttributes, runtimeReturnType); + var defaultErrorType = typeof(void); + if (action.Properties.TryGetValue(typeof(ProducesErrorResponseTypeAttribute), out result)) + { + defaultErrorType = ((ProducesErrorResponseTypeAttribute)result).Type; + } + + var apiResponseTypes = GetApiResponseTypes(responseMetadataAttributes, runtimeReturnType, defaultErrorType); return apiResponseTypes; } @@ -69,7 +75,8 @@ namespace Microsoft.AspNetCore.Mvc.ApiExplorer private ICollection GetApiResponseTypes( IReadOnlyList responseMetadataAttributes, - Type type) + Type type, + Type defaultErrorType) { var results = new Dictionary(); @@ -83,48 +90,39 @@ namespace Microsoft.AspNetCore.Mvc.ApiExplorer { metadataAttribute.SetContentTypes(contentTypes); - ApiResponseType apiResponseType; + var statusCode = metadataAttribute.StatusCode; - if (metadataAttribute is IApiDefaultResponseMetadataProvider) + var apiResponseType = new ApiResponseType { - apiResponseType = new ApiResponseType + Type = metadataAttribute.Type, + StatusCode = statusCode, + IsDefaultResponse = metadataAttribute is IApiDefaultResponseMetadataProvider, + }; + + if (apiResponseType.Type == typeof(void)) + { + if (type != null && (statusCode == StatusCodes.Status200OK || statusCode == StatusCodes.Status201Created)) { - IsDefaultResponse = true, - Type = metadataAttribute.Type, - }; - } - else if (metadataAttribute.Type == typeof(void) && - type != null && - (metadataAttribute.StatusCode == StatusCodes.Status200OK || metadataAttribute.StatusCode == StatusCodes.Status201Created)) - { - // ProducesResponseTypeAttribute's constructor defaults to setting "Type" to void when no value is specified. - // In this event, use the action's return type for 200 or 201 status codes. This lets you decorate an action with a - // [ProducesResponseType(201)] instead of [ProducesResponseType(201, typeof(Person)] when typeof(Person) can be inferred - // from the return type. - apiResponseType = new ApiResponseType + // ProducesResponseTypeAttribute's constructor defaults to setting "Type" to void when no value is specified. + // In this event, use the action's return type for 200 or 201 status codes. This lets you decorate an action with a + // [ProducesResponseType(201)] instead of [ProducesResponseType(201, typeof(Person)] when typeof(Person) can be inferred + // from the return type. + apiResponseType.Type = type; + } + else if (IsClientError(statusCode) || apiResponseType.IsDefaultResponse) { - StatusCode = metadataAttribute.StatusCode, - Type = type, - }; - } - else if (metadataAttribute.Type != null) - { - apiResponseType = new ApiResponseType - { - StatusCode = metadataAttribute.StatusCode, - Type = metadataAttribute.Type, - }; - } - else - { - continue; + // Use the default error type for "default" responses or 4xx client errors if no response type is specified. + apiResponseType.Type = defaultErrorType; + } } - results[apiResponseType.StatusCode] = apiResponseType; + if (apiResponseType.Type != null) + { + results[apiResponseType.StatusCode] = apiResponseType; + } } } - // Set the default status only when no status has already been set explicitly if (results.Count == 0 && type != null) { @@ -225,5 +223,10 @@ namespace Microsoft.AspNetCore.Mvc.ApiExplorer return declaredReturnType; } + + private static bool IsClientError(int statusCode) + { + return statusCode >= 400 && statusCode < 500; + } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs index 5b21f55378..9eeb83769a 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs @@ -18,6 +18,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal { public class ApiBehaviorApplicationModelProvider : IApplicationModelProvider { + private readonly ProducesErrorResponseTypeAttribute DefaultErrorType = new ProducesErrorResponseTypeAttribute(typeof(ProblemDetails)); private readonly ApiBehaviorOptions _apiBehaviorOptions; private readonly IModelMetadataProvider _modelMetadataProvider; private readonly ModelStateInvalidFilter _modelStateInvalidFilter; @@ -105,6 +106,8 @@ namespace Microsoft.AspNetCore.Mvc.Internal AddMultipartFormDataConsumesAttribute(actionModel); DiscoverApiConvention(actionModel, conventions); + + DiscoverErrorResponseType(actionModel); } } } @@ -274,6 +277,25 @@ namespace Microsoft.AspNetCore.Mvc.Internal } } + internal void DiscoverErrorResponseType(ActionModel actionModel) + { + var errorTypeAttribute = + actionModel.Attributes.OfType().FirstOrDefault() ?? + actionModel.Controller.Attributes.OfType().FirstOrDefault() ?? + actionModel.Controller.ControllerType.Assembly.GetCustomAttribute(); + + if (!_apiBehaviorOptions.SuppressMapClientErrors) + { + // If ClientErrorFactory is being used and the application does not supply a error response type, assume ProblemDetails. + errorTypeAttribute = errorTypeAttribute ?? DefaultErrorType; + } + + if (errorTypeAttribute != null) + { + actionModel.Properties[typeof(ProducesErrorResponseTypeAttribute)] = errorTypeAttribute; + } + } + private bool ParameterExistsInAnyRoute(ActionModel actionModel, string parameterName) { foreach (var (route, _, _) in ActionAttributeRouteModel.GetAttributeRoutes(actionModel)) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ProducesErrorResponseTypeAttribute.cs b/src/Microsoft.AspNetCore.Mvc.Core/ProducesErrorResponseTypeAttribute.cs new file mode 100644 index 0000000000..f74a826e93 --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Core/ProducesErrorResponseTypeAttribute.cs @@ -0,0 +1,38 @@ +// 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 Microsoft.AspNetCore.Mvc.Infrastructure; + +namespace Microsoft.AspNetCore.Mvc +{ + /// + /// Specifies the type returned by default by controllers annotated with . + /// + /// specifies the error model type associated with a + /// for a client error (HTTP Status Code 4xx) when no value is provided. When no value is specified, MVC assumes the + /// client error type to be , if mapping client errors () + /// is used. + /// + /// + /// Use this to configure the default error type if your application uses a custom error type to respond. + /// + /// + [AttributeUsage(AttributeTargets.Assembly | AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)] + public sealed class ProducesErrorResponseTypeAttribute : Attribute + { + /// + /// Initializes a new instance of . + /// + /// The error type. + public ProducesErrorResponseTypeAttribute(Type type) + { + Type = type ?? throw new ArgumentNullException(nameof(type)); + } + + /// + /// Gets the default error type. + /// + public Type Type { get; } + } +} diff --git a/test/Microsoft.AspNetCore.Mvc.ApiExplorer.Test/ApiResponseTypeProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.ApiExplorer.Test/ApiResponseTypeProviderTest.cs index 11c9bcb8e0..aee457d5a6 100644 --- a/test/Microsoft.AspNetCore.Mvc.ApiExplorer.Test/ApiResponseTypeProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.ApiExplorer.Test/ApiResponseTypeProviderTest.cs @@ -309,6 +309,247 @@ namespace Microsoft.AspNetCore.Mvc.ApiExplorer }); } + [Fact] + public void GetApiResponseTypes_UsesErrorType_ForClientErrors() + { + // Arrange + var errorType = typeof(InvalidTimeZoneException); + var actionDescriptor = GetControllerActionDescriptor( + typeof(GetApiResponseTypes_ReturnsResponseTypesFromDefaultConventionsController), + nameof(GetApiResponseTypes_ReturnsResponseTypesFromDefaultConventionsController.DeleteBase)); + actionDescriptor.Properties[typeof(ApiConventionResult)] = new ApiConventionResult(new IApiResponseMetadataProvider[] + { + new ProducesResponseTypeAttribute(200), + new ProducesResponseTypeAttribute(404), + new ProducesResponseTypeAttribute(415), + }); + + actionDescriptor.Properties[typeof(ProducesErrorResponseTypeAttribute)] = new ProducesErrorResponseTypeAttribute(errorType); + + var provider = GetProvider(); + + // Act + var result = provider.GetApiResponseTypes(actionDescriptor); + + // Assert + Assert.Collection( + result.OrderBy(r => r.StatusCode), + responseType => + { + Assert.Equal(200, responseType.StatusCode); + Assert.Equal(typeof(BaseModel), responseType.Type); + Assert.Collection( + responseType.ApiResponseFormats, + format => Assert.Equal("application/json", format.MediaType)); + }, + responseType => + { + Assert.Equal(404, responseType.StatusCode); + Assert.Equal(errorType, responseType.Type); + Assert.False(responseType.IsDefaultResponse); + Assert.Collection( + responseType.ApiResponseFormats, + format => Assert.Equal("application/json", format.MediaType)); + }, + responseType => + { + Assert.Equal(415, responseType.StatusCode); + Assert.Equal(errorType, responseType.Type); + Assert.False(responseType.IsDefaultResponse); + Assert.Collection( + responseType.ApiResponseFormats, + format => Assert.Equal("application/json", format.MediaType)); + }); + } + + [Fact] + public void GetApiResponseTypes_UsesErrorType_ForDefaultResponse() + { + // Arrange + var errorType = typeof(ProblemDetails); + var actionDescriptor = GetControllerActionDescriptor( + typeof(GetApiResponseTypes_ReturnsResponseTypesFromDefaultConventionsController), + nameof(GetApiResponseTypes_ReturnsResponseTypesFromDefaultConventionsController.DeleteBase)); + actionDescriptor.Properties[typeof(ApiConventionResult)] = new ApiConventionResult(new IApiResponseMetadataProvider[] + { + new ProducesResponseTypeAttribute(200), + new ProducesDefaultResponseTypeAttribute(), + }); + + actionDescriptor.Properties[typeof(ProducesErrorResponseTypeAttribute)] = new ProducesErrorResponseTypeAttribute(errorType); + + var provider = GetProvider(); + + // Act + var result = provider.GetApiResponseTypes(actionDescriptor); + + // Assert + Assert.Collection( + result.OrderBy(r => r.StatusCode), + responseType => + { + Assert.Equal(errorType, responseType.Type); + Assert.True(responseType.IsDefaultResponse); + Assert.Collection( + responseType.ApiResponseFormats, + format => Assert.Equal("application/json", format.MediaType)); + }, + responseType => + { + Assert.Equal(200, responseType.StatusCode); + Assert.Equal(typeof(BaseModel), responseType.Type); + Assert.Collection( + responseType.ApiResponseFormats, + format => Assert.Equal("application/json", format.MediaType)); + }); + } + + [Fact] + public void GetApiResponseTypes_DoesNotUseErrorType_IfSpecified() + { + // Arrange + var errorType = typeof(InvalidTimeZoneException); + var actionDescriptor = GetControllerActionDescriptor( + typeof(GetApiResponseTypes_ReturnsResponseTypesFromDefaultConventionsController), + nameof(GetApiResponseTypes_ReturnsResponseTypesFromDefaultConventionsController.DeleteBase)); + actionDescriptor.Properties[typeof(ApiConventionResult)] = new ApiConventionResult(new IApiResponseMetadataProvider[] + { + new ProducesResponseTypeAttribute(200), + new ProducesResponseTypeAttribute(typeof(DivideByZeroException), 415), + new ProducesDefaultResponseTypeAttribute(typeof(DivideByZeroException)), + }); + + actionDescriptor.Properties[typeof(ProducesErrorResponseTypeAttribute)] = new ProducesErrorResponseTypeAttribute(errorType); + + var provider = GetProvider(); + + // Act + var result = provider.GetApiResponseTypes(actionDescriptor); + + // Assert + Assert.Collection( + result.OrderBy(r => r.StatusCode), + responseType => + { + Assert.Equal(typeof(DivideByZeroException), responseType.Type); + Assert.True(responseType.IsDefaultResponse); + Assert.Collection( + responseType.ApiResponseFormats, + format => Assert.Equal("application/json", format.MediaType)); + }, + responseType => + { + Assert.Equal(200, responseType.StatusCode); + Assert.Equal(typeof(BaseModel), responseType.Type); + Assert.Collection( + responseType.ApiResponseFormats, + format => Assert.Equal("application/json", format.MediaType)); + }, + responseType => + { + Assert.Equal(415, responseType.StatusCode); + Assert.Equal(typeof(DivideByZeroException), responseType.Type); + Assert.False(responseType.IsDefaultResponse); + Assert.Collection( + responseType.ApiResponseFormats, + format => Assert.Equal("application/json", format.MediaType)); + }); + } + + [Fact] + public void GetApiResponseTypes_DoesNotUseErrorType_ForNonClientErrors() + { + // Arrange + var actionDescriptor = GetControllerActionDescriptor( + typeof(GetApiResponseTypes_ReturnsResponseTypesFromDefaultConventionsController), + nameof(GetApiResponseTypes_ReturnsResponseTypesFromDefaultConventionsController.DeleteBase)); + actionDescriptor.Properties[typeof(ApiConventionResult)] = new ApiConventionResult(new IApiResponseMetadataProvider[] + { + new ProducesResponseTypeAttribute(201), + new ProducesResponseTypeAttribute(300), + new ProducesResponseTypeAttribute(500), + }); + + actionDescriptor.Properties[typeof(ProducesErrorResponseTypeAttribute)] = new ProducesErrorResponseTypeAttribute(typeof(InvalidTimeZoneException)); + + var provider = GetProvider(); + + // Act + var result = provider.GetApiResponseTypes(actionDescriptor); + + // Assert + Assert.Collection( + result.OrderBy(r => r.StatusCode), + responseType => + { + Assert.Equal(201, responseType.StatusCode); + Assert.Equal(typeof(BaseModel), responseType.Type); + Assert.Collection( + responseType.ApiResponseFormats, + format => Assert.Equal("application/json", format.MediaType)); + }, + responseType => + { + Assert.Equal(300, responseType.StatusCode); + Assert.Equal(typeof(void), responseType.Type); + Assert.Empty(responseType.ApiResponseFormats); + }, + responseType => + { + Assert.Equal(500, responseType.StatusCode); + Assert.Equal(typeof(void), responseType.Type); + Assert.Empty(responseType.ApiResponseFormats); + }); + } + + [Fact] + public void GetApiResponseTypes_AllowsUsingVoid() + { + // Arrange + var actionDescriptor = GetControllerActionDescriptor( + typeof(GetApiResponseTypes_ReturnsResponseTypesFromDefaultConventionsController), + nameof(GetApiResponseTypes_ReturnsResponseTypesFromDefaultConventionsController.DeleteBase)); + actionDescriptor.Properties[typeof(ApiConventionResult)] = new ApiConventionResult(new IApiResponseMetadataProvider[] + { + new ProducesResponseTypeAttribute(typeof(InvalidCastException), 400), + new ProducesResponseTypeAttribute(415), + new ProducesDefaultResponseTypeAttribute(), + }); + + actionDescriptor.Properties[typeof(ProducesErrorResponseTypeAttribute)] = new ProducesErrorResponseTypeAttribute(typeof(void)); + + var provider = GetProvider(); + + // Act + var result = provider.GetApiResponseTypes(actionDescriptor); + + // Assert + Assert.Collection( + result.OrderBy(r => r.StatusCode), + responseType => + { + Assert.True(responseType.IsDefaultResponse); + Assert.Equal(typeof(void), responseType.Type); + Assert.Empty(responseType.ApiResponseFormats); + }, + responseType => + { + Assert.Equal(400, responseType.StatusCode); + Assert.Equal(typeof(InvalidCastException), responseType.Type); + Assert.False(responseType.IsDefaultResponse); + Assert.Collection( + responseType.ApiResponseFormats, + format => Assert.Equal("application/json", format.MediaType)); + }, + responseType => + { + Assert.Equal(415, responseType.StatusCode); + Assert.Equal(typeof(void), responseType.Type); + Assert.False(responseType.IsDefaultResponse); + Assert.Empty(responseType.ApiResponseFormats); + }); + } + private static ApiResponseTypeProvider GetProvider() { var mvcOptions = new MvcOptions diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs index c0c415573f..c4104b9027 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs @@ -12,7 +12,6 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.ApiExplorer; using Microsoft.AspNetCore.Mvc.ApplicationModels; -using Microsoft.AspNetCore.Mvc.Authorization; using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.ModelBinding; using Microsoft.Extensions.Logging.Abstractions; @@ -20,6 +19,8 @@ using Microsoft.Extensions.Options; using Moq; using Xunit; +[assembly: Microsoft.AspNetCore.Mvc.ProducesErrorResponseType(typeof(InvalidEnumArgumentException))] + namespace Microsoft.AspNetCore.Mvc.Internal { public class ApiBehaviorApplicationModelProviderTest @@ -1042,9 +1043,6 @@ Environment.NewLine + "int b"; var actionModel = new ActionModel( typeof(TestApiConventionController).GetMethod(nameof(TestApiConventionController.Delete)), Array.Empty()); - actionModel.Filters.Add(new AuthorizeFilter()); - actionModel.Filters.Add(new ServiceFilterAttribute(typeof(object))); - actionModel.Filters.Add(new ConsumesAttribute("application/xml")); var attributes = new[] { new ApiConventionTypeAttribute(typeof(DefaultApiConventions)) }; // Act @@ -1060,6 +1058,167 @@ Environment.NewLine + "int b"; }); } + [Fact] + public void DiscoverErrorResponseType_SetsProblemDetails_IfActionHasNoAttributes() + { + // Arrange + var expected = typeof(ProblemDetails); + var controllerModel = new ControllerModel(typeof(object).GetTypeInfo(), new[] { new object() }); + var actionModel = new ActionModel( + typeof(TestApiConventionController).GetMethod(nameof(TestApiConventionController.Delete)), + Array.Empty()) + { + Controller = controllerModel, + }; + var provider = GetProvider(); + + // Act + provider.DiscoverErrorResponseType(actionModel); + + // Assert + Assert.Collection( + actionModel.Properties, + kvp => + { + Assert.Equal(typeof(ProducesErrorResponseTypeAttribute), kvp.Key); + var value = Assert.IsType(kvp.Value); + Assert.Equal(expected, value.Type); + }); + } + + [Fact] + public void DiscoverErrorResponseType_DoesNotSetDefaultProblemDetailsResponse_IfSuppressMapClientErrorsIsSet() + { + // Arrange + var expected = typeof(ProblemDetails); + var controllerModel = new ControllerModel(typeof(object).GetTypeInfo(), new[] { new object() }); + var actionModel = new ActionModel( + typeof(TestApiConventionController).GetMethod(nameof(TestApiConventionController.Delete)), + Array.Empty()) + { + Controller = controllerModel, + }; + var provider = GetProvider(new ApiBehaviorOptions + { + InvalidModelStateResponseFactory = _ => null, + SuppressMapClientErrors = true, + }); + + // Act + provider.DiscoverErrorResponseType(actionModel); + + // Assert + Assert.Empty(actionModel.Properties); + } + + [Fact] + public void DiscoverErrorResponseType_UsesValueFromApiErrorTypeAttribute_SpecifiedOnControllerAsssembly() + { + // Arrange + var expected = typeof(InvalidEnumArgumentException); + var controllerModel = new ControllerModel(typeof(TestApiConventionController).GetTypeInfo(), new[] { new object() }); + var actionModel = new ActionModel( + typeof(TestApiConventionController).GetMethod(nameof(TestApiConventionController.Delete)), + Array.Empty()) + { + Controller = controllerModel, + }; + var provider = GetProvider(); + + // Act + provider.DiscoverErrorResponseType(actionModel); + + // Assert + Assert.Collection( + actionModel.Properties, + kvp => + { + Assert.Equal(typeof(ProducesErrorResponseTypeAttribute), kvp.Key); + var value = Assert.IsType(kvp.Value); + Assert.Equal(expected, value.Type); + }); + } + + [Fact] + public void DiscoverErrorResponseType_UsesValueFromApiErrorTypeAttribute_SpecifiedOnController() + { + // Arrange + var expected = typeof(InvalidTimeZoneException); + var controllerModel = new ControllerModel(typeof(TestApiConventionController).GetTypeInfo(), new[] { new ProducesErrorResponseTypeAttribute(expected) }); + var actionModel = new ActionModel( + typeof(TestApiConventionController).GetMethod(nameof(TestApiConventionController.Delete)), + Array.Empty()) + { + Controller = controllerModel, + }; + var provider = GetProvider(); + + // Act + provider.DiscoverErrorResponseType(actionModel); + + // Assert + Assert.Collection( + actionModel.Properties, + kvp => + { + Assert.Equal(typeof(ProducesErrorResponseTypeAttribute), kvp.Key); + var value = Assert.IsType(kvp.Value); + Assert.Equal(expected, value.Type); + }); + } + + [Fact] + public void DiscoverErrorResponseType_UsesValueFromApiErrorTypeAttribute_SpecifiedOnAction() + { + // Arrange + var expected = typeof(InvalidTimeZoneException); + var controllerModel = new ControllerModel(typeof(TestApiConventionController).GetTypeInfo(), new[] { new ProducesErrorResponseTypeAttribute(typeof(Guid)) }); + var actionModel = new ActionModel( + typeof(TestApiConventionController).GetMethod(nameof(TestApiConventionController.Delete)), + new[] { new ProducesErrorResponseTypeAttribute(expected) }) + { + Controller = controllerModel, + }; + var provider = GetProvider(); + + // Act + provider.DiscoverErrorResponseType(actionModel); + + // Assert + Assert.Collection( + actionModel.Properties, + kvp => + { + Assert.Equal(typeof(ProducesErrorResponseTypeAttribute), kvp.Key); + var value = Assert.IsType(kvp.Value); + Assert.Equal(expected, value.Type); + }); + } + + [Fact] + public void DiscoverErrorResponseType_AllowsVoidsType() + { + // Arrange + var expected = typeof(void); + var actionModel = new ActionModel( + typeof(TestApiConventionController).GetMethod(nameof(TestApiConventionController.Delete)), + new[] { new ProducesErrorResponseTypeAttribute(expected) }); + var provider = GetProvider(); + + // Act + provider.DiscoverErrorResponseType(actionModel); + + // Assert + Assert.Collection( + actionModel.Properties, + kvp => + { + Assert.Equal(typeof(ProducesErrorResponseTypeAttribute), kvp.Key); + var value = Assert.IsType(kvp.Value); + Assert.Equal(expected, value.Type); + }); + } + [Fact] public void OnProvidersExecuting_AddsClientErrorResultFilter() { diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiExplorerTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiExplorerTest.cs index d216ad4aff..6230542cb0 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiExplorerTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiExplorerTest.cs @@ -1180,9 +1180,9 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests }, responseType => { - Assert.Equal(typeof(void).FullName, responseType.ResponseType); + Assert.Equal(typeof(ProblemDetails).FullName, responseType.ResponseType); Assert.Equal(404, responseType.StatusCode); - Assert.Empty(responseType.ResponseFormats); + Assert.Equal(expectedMediaTypes, GetSortedMediaTypes(responseType)); }); } @@ -1215,7 +1215,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests public async Task ApiConvention_ForMethodWithResponseTypeAttributes() { // Arrange - var expectedMediaTypes = new[] { "application/json", "application/xml", "text/json", "text/xml" }; + var expectedMediaTypes = new[] { "application/json" }; // Act var response = await Client.PostAsync( @@ -1236,15 +1236,18 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests }, responseType => { - Assert.Equal(typeof(void).FullName, responseType.ResponseType); + Assert.Equal(typeof(ProblemDetails).FullName, responseType.ResponseType); Assert.Equal(403, responseType.StatusCode); - Assert.Empty(responseType.ResponseFormats); + Assert.Equal(expectedMediaTypes, GetSortedMediaTypes(responseType)); }); } [Fact] public async Task ApiConvention_ForPostMethodThatMatchesConvention() { + // Arrange + var expectedMediaTypes = new[] { "application/json", "application/xml", "text/json", "text/xml" }; + // Act var response = await Client.PostAsync( $"ApiExplorerResponseTypeWithApiConventionController/PostTaskOfProduct", @@ -1268,15 +1271,18 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests }, responseType => { - Assert.Equal(typeof(void).FullName, responseType.ResponseType); + Assert.Equal(typeof(ProblemDetails).FullName, responseType.ResponseType); Assert.Equal(400, responseType.StatusCode); - Assert.Empty(responseType.ResponseFormats); + Assert.Equal(expectedMediaTypes, GetSortedMediaTypes(responseType)); }); } [Fact] public async Task ApiConvention_ForPutActionThatMatchesConvention() { + // Arrange + var expectedMediaTypes = new[] { "application/json", "application/xml", "text/json", "text/xml" }; + // Act var response = await Client.PutAsync( $"ApiExplorerResponseTypeWithApiConventionController/Put", @@ -1300,21 +1306,24 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests }, responseType => { - Assert.Equal(typeof(void).FullName, responseType.ResponseType); + Assert.Equal(typeof(ProblemDetails).FullName, responseType.ResponseType); Assert.Equal(400, responseType.StatusCode); - Assert.Empty(responseType.ResponseFormats); + Assert.Equal(expectedMediaTypes, GetSortedMediaTypes(responseType)); }, responseType => { - Assert.Equal(typeof(void).FullName, responseType.ResponseType); + Assert.Equal(typeof(ProblemDetails).FullName, responseType.ResponseType); Assert.Equal(404, responseType.StatusCode); - Assert.Empty(responseType.ResponseFormats); + Assert.Equal(expectedMediaTypes, GetSortedMediaTypes(responseType)); }); } [Fact] public async Task ApiConvention_ForDeleteActionThatMatchesConvention() { + // Arrange + var expectedMediaTypes = new[] { "application/json", "application/xml", "text/json", "text/xml" }; + // Act var response = await Client.DeleteAsync( $"ApiExplorerResponseTypeWithApiConventionController/DeleteProductAsync"); @@ -1337,21 +1346,24 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests }, responseType => { - Assert.Equal(typeof(void).FullName, responseType.ResponseType); + Assert.Equal(typeof(ProblemDetails).FullName, responseType.ResponseType); Assert.Equal(400, responseType.StatusCode); - Assert.Empty(responseType.ResponseFormats); + Assert.Equal(expectedMediaTypes, GetSortedMediaTypes(responseType)); }, responseType => { - Assert.Equal(typeof(void).FullName, responseType.ResponseType); + Assert.Equal(typeof(ProblemDetails).FullName, responseType.ResponseType); Assert.Equal(404, responseType.StatusCode); - Assert.Empty(responseType.ResponseFormats); + Assert.Equal(expectedMediaTypes, GetSortedMediaTypes(responseType)); }); } [Fact] public async Task ApiConvention_ForActionWtihApiConventionMethod() { + // Arrange + var expectedMediaTypes = new[] { "application/json", "application/xml", "text/json", "text/xml" }; + // Act var response = await Client.PostAsync( "ApiExplorerResponseTypeWithApiConventionController/PostItem", @@ -1371,9 +1383,9 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests }, responseType => { - Assert.Equal(typeof(void).FullName, responseType.ResponseType); + Assert.Equal(typeof(ProblemDetails).FullName, responseType.ResponseType); Assert.Equal(409, responseType.StatusCode); - Assert.Empty(responseType.ResponseFormats); + Assert.Equal(expectedMediaTypes, GetSortedMediaTypes(responseType)); }); } From 3dfa26f7e364fcd913e972a1188579c808d04d52 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Tue, 28 Aug 2018 03:08:55 +0100 Subject: [PATCH 17/21] Resolve virtual ViewContext max once per method --- .../RazorPage.cs | 11 +++--- .../RazorPageBase.cs | 39 +++++++++++-------- 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Razor/RazorPage.cs b/src/Microsoft.AspNetCore.Mvc.Razor/RazorPage.cs index 3d13f977f7..53fd192c24 100644 --- a/src/Microsoft.AspNetCore.Mvc.Razor/RazorPage.cs +++ b/src/Microsoft.AspNetCore.Mvc.Razor/RazorPage.cs @@ -196,11 +196,12 @@ namespace Microsoft.AspNetCore.Mvc.Razor else if (required) { // If the section is not found, and it is not optional, throw an error. - var message = Resources.FormatSectionNotDefined( - ViewContext.ExecutingFilePath, - sectionName, - ViewContext.View.Path); - throw new InvalidOperationException(message); + var viewContext = ViewContext; + throw new InvalidOperationException( + Resources.FormatSectionNotDefined( + viewContext.ExecutingFilePath, + sectionName, + viewContext.View.Path)); } else { diff --git a/src/Microsoft.AspNetCore.Mvc.Razor/RazorPageBase.cs b/src/Microsoft.AspNetCore.Mvc.Razor/RazorPageBase.cs index 96ef957f60..016076520f 100644 --- a/src/Microsoft.AspNetCore.Mvc.Razor/RazorPageBase.cs +++ b/src/Microsoft.AspNetCore.Mvc.Razor/RazorPageBase.cs @@ -51,13 +51,13 @@ namespace Microsoft.AspNetCore.Mvc.Razor { get { - if (ViewContext == null) + var viewContext = ViewContext; + if (viewContext == null) { - var message = Resources.FormatViewContextMustBeSet("ViewContext", "Output"); - throw new InvalidOperationException(message); + throw new InvalidOperationException(Resources.FormatViewContextMustBeSet(nameof(ViewContext), nameof(Output))); } - return ViewContext.Writer; + return viewContext.Writer; } } @@ -183,8 +183,9 @@ namespace Microsoft.AspNetCore.Mvc.Razor /// public void StartTagHelperWritingScope(HtmlEncoder encoder) { + var viewContext = ViewContext; var buffer = new ViewBuffer(BufferScope, Path, ViewBuffer.TagHelperPageSize); - TagHelperScopes.Push(new TagHelperScopeInfo(buffer, HtmlEncoder, ViewContext.Writer)); + TagHelperScopes.Push(new TagHelperScopeInfo(buffer, HtmlEncoder, viewContext.Writer)); // If passed an HtmlEncoder, override the property. if (encoder != null) @@ -194,7 +195,7 @@ namespace Microsoft.AspNetCore.Mvc.Razor // We need to replace the ViewContext's Writer to ensure that all content (including content written // from HTML helpers) is redirected. - ViewContext.Writer = new ViewBufferTextWriter(buffer, ViewContext.Writer.Encoding); + viewContext.Writer = new ViewBufferTextWriter(buffer, viewContext.Writer.Encoding); } /// @@ -238,7 +239,8 @@ namespace Microsoft.AspNetCore.Mvc.Razor throw new InvalidOperationException(Resources.RazorPage_NestingAttributeWritingScopesNotSupported); } - _pageWriter = ViewContext.Writer; + var viewContext = ViewContext; + _pageWriter = viewContext.Writer; if (_valueBuffer == null) { @@ -247,7 +249,7 @@ namespace Microsoft.AspNetCore.Mvc.Razor // We need to replace the ViewContext's Writer to ensure that all content (including content written // from HTML helpers) is redirected. - ViewContext.Writer = _valueBuffer; + viewContext.Writer = _valueBuffer; } @@ -284,15 +286,18 @@ namespace Microsoft.AspNetCore.Mvc.Razor throw new ArgumentNullException(nameof(writer)); } - _textWriterStack.Push(ViewContext.Writer); - ViewContext.Writer = writer; + var viewContext = ViewContext; + _textWriterStack.Push(viewContext.Writer); + viewContext.Writer = writer; } // Internal for unit testing. protected internal virtual TextWriter PopWriter() { - ViewContext.Writer = _textWriterStack.Pop(); - return ViewContext.Writer; + var viewContext = ViewContext; + var writer = _textWriterStack.Pop(); + viewContext.Writer = writer; + return writer; } public virtual string Href(string contentPath) @@ -304,9 +309,10 @@ namespace Microsoft.AspNetCore.Mvc.Razor if (_urlHelper == null) { - var services = ViewContext?.HttpContext.RequestServices; + var viewContext = ViewContext; + var services = viewContext?.HttpContext.RequestServices; var factory = services.GetRequiredService(); - _urlHelper = factory.GetUrlHelper(ViewContext); + _urlHelper = factory.GetUrlHelper(viewContext); } return _urlHelper.Content(contentPath); @@ -637,8 +643,9 @@ namespace Microsoft.AspNetCore.Mvc.Razor /// before flushes the headers. public virtual HtmlString SetAntiforgeryCookieAndHeader() { - var antiforgery = ViewContext?.HttpContext.RequestServices.GetRequiredService(); - antiforgery.SetCookieTokenAndHeader(ViewContext?.HttpContext); + var viewContext = ViewContext; + var antiforgery = viewContext?.HttpContext.RequestServices.GetRequiredService(); + antiforgery.SetCookieTokenAndHeader(viewContext?.HttpContext); return HtmlString.Empty; } From 1128bd572cf46e782a71d9c4240b2c737d8b0a72 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Thu, 30 Aug 2018 13:08:07 -0700 Subject: [PATCH 18/21] Add a functional test for middleware after routing It came up during routing discussions that we don't have any tests for this scenario. --- .../RoutingTestsBase.cs | 13 +++++++++++++ test/WebSites/RoutingWebSite/Startup.cs | 6 ++++++ test/WebSites/RoutingWebSite/StartupWith21Compat.cs | 6 ++++++ 3 files changed, 25 insertions(+) diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RoutingTestsBase.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RoutingTestsBase.cs index 464a36beb5..175dfaef1a 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RoutingTestsBase.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RoutingTestsBase.cs @@ -1265,6 +1265,19 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests Assert.Equal(actionName, result.Action); } + [Fact] + public async Task CanRunMiddlewareAfterRouting() + { + // Act + var response = await Client.GetAsync("/afterrouting"); + + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.OK); + var content = await response.Content.ReadAsStringAsync(); + Assert.Equal("Hello from middleware after routing", content); + } + + protected static LinkBuilder LinkFrom(string url) { return new LinkBuilder(url); diff --git a/test/WebSites/RoutingWebSite/Startup.cs b/test/WebSites/RoutingWebSite/Startup.cs index cea64f826f..08a67d963f 100644 --- a/test/WebSites/RoutingWebSite/Startup.cs +++ b/test/WebSites/RoutingWebSite/Startup.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.Extensions.DependencyInjection; @@ -47,6 +48,11 @@ namespace RoutingWebSite "RouteWithOptionalSegment", "{controller}/{action}/{path?}"); }); + + app.Map("/afterrouting", b => b.Run(c => + { + return c.Response.WriteAsync("Hello from middleware after routing"); + })); } } } \ No newline at end of file diff --git a/test/WebSites/RoutingWebSite/StartupWith21Compat.cs b/test/WebSites/RoutingWebSite/StartupWith21Compat.cs index 1b34b2cfb0..eb891a53c5 100644 --- a/test/WebSites/RoutingWebSite/StartupWith21Compat.cs +++ b/test/WebSites/RoutingWebSite/StartupWith21Compat.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.Extensions.DependencyInjection; @@ -47,6 +48,11 @@ namespace RoutingWebSite "RouteWithOptionalSegment", "{controller}/{action}/{path?}"); }); + + app.Map("/afterrouting", b => b.Run(c => + { + return c.Response.WriteAsync("Hello from middleware after routing"); + })); } } } \ No newline at end of file From d8b7dbd1f3fef9789b12169de2bb25481ebad05c Mon Sep 17 00:00:00 2001 From: Pranav K Date: Thu, 30 Aug 2018 15:17:09 -0700 Subject: [PATCH 19/21] Avoid null refs when IStatusCodeActionResult cannot be discovered --- .../ApiControllerSymbolCache.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiControllerSymbolCache.cs b/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiControllerSymbolCache.cs index e92c0927ca..7cbec7eb36 100644 --- a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiControllerSymbolCache.cs +++ b/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiControllerSymbolCache.cs @@ -28,7 +28,7 @@ namespace Microsoft.AspNetCore.Mvc.Api.Analyzers StatusCodeValueAttribute = compilation.GetTypeByMetadataName(ApiSymbolNames.StatusCodeValueAttribute); var statusCodeActionResult = compilation.GetTypeByMetadataName(ApiSymbolNames.IStatusCodeActionResult); - StatusCodeActionResultStatusProperty = (IPropertySymbol)statusCodeActionResult.GetMembers("StatusCode")[0]; + StatusCodeActionResultStatusProperty = (IPropertySymbol)statusCodeActionResult?.GetMembers("StatusCode")[0]; var disposable = compilation.GetSpecialType(SpecialType.System_IDisposable); var members = disposable.GetMembers(nameof(IDisposable.Dispose)); From b9793f0a1d9a75940c50e31e956d09397a5d1876 Mon Sep 17 00:00:00 2001 From: "ASP.NET CI" Date: Sun, 2 Sep 2018 12:21:18 -0700 Subject: [PATCH 20/21] Update dependencies.props [auto-updated: dependencies] --- build/dependencies.props | 146 +++++++++++++++++++-------------------- korebuild-lock.txt | 4 +- 2 files changed, 75 insertions(+), 75 deletions(-) diff --git a/build/dependencies.props b/build/dependencies.props index d718620eed..712c6521e9 100644 --- a/build/dependencies.props +++ b/build/dependencies.props @@ -16,87 +16,87 @@ 0.43.0 2.1.1.1 2.1.1 - 2.2.0-preview2-35090 - 2.2.0-preview1-20180807.2 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-a-preview2-routeconstraint-httpcontext-16912 - 2.2.0-a-preview2-routeconstraint-httpcontext-16912 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 + 2.2.0-preview2-35143 + 2.2.0-preview1-20180821.1 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 5.2.6 2.8.0 2.8.0 - 2.2.0-preview2-35090 + 2.2.0-preview2-35143 1.7.0 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 2.1.0 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 2.0.9 2.1.2 2.2.0-preview1-26618-02 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 + 2.2.0-preview2-35143 + 2.2.0-preview2-35143 15.6.1 4.7.49 2.0.3 diff --git a/korebuild-lock.txt b/korebuild-lock.txt index 3fbcc80189..ad704918df 100644 --- a/korebuild-lock.txt +++ b/korebuild-lock.txt @@ -1,2 +1,2 @@ -version:2.2.0-preview1-20180807.2 -commithash:11495dbd236104434e08cb1152fcb58cf2a20923 +version:2.2.0-preview1-20180821.1 +commithash:c8d0cc52cd1abb697be24e288ffd54f8fae8bf17 From 6498c89f880d65681e61e1bdd86c755fb5136f7e Mon Sep 17 00:00:00 2001 From: Nate McMaster Date: Wed, 5 Sep 2018 16:28:46 -0700 Subject: [PATCH 21/21] Update branding to 2.2.0-preview3 --- version.props | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/version.props b/version.props index d623e08663..e01d0a1143 100644 --- a/version.props +++ b/version.props @@ -1,7 +1,7 @@  2.2.0 - preview2 + preview3 t000 a- @@ -11,7 +11,7 @@ $(VersionSuffix)-$(BuildNumber) 0.2.0 - preview2 + preview3 $(ExperimentalVersionPrefix) $(ExperimentalVersionPrefix)-$(ExperimentalVersionSuffix)-final