From a14fdd863737d6c2b15493c6a1c2393dfff5a797 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Wed, 18 Nov 2015 18:31:50 -0800 Subject: [PATCH] Remove FormatFilter dependency on IActionContextAccessor This change simplifies IFormatFilter's API and removes the dependency on IActionContext accessor. The old API for IFormatFilter required computing state based on the current request as part of the constructor, which in turn implied the use of a context accessor. This isn't really needed. I didn't preserve caching of the 'format' as that seems like an early optimization. --- .../MvcCoreMvcCoreBuilderExtensions.cs | 2 +- .../Formatters/FormatFilter.cs | 84 +++++++------------ .../Formatters/IFormatFilter.cs | 17 +--- .../ProducesAttribute.cs | 2 +- .../Formatters/FormatFilterTest.cs | 52 ++++++------ .../ProducesAttributeTests.cs | 10 ++- 6 files changed, 69 insertions(+), 98 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/DependencyInjection/MvcCoreMvcCoreBuilderExtensions.cs b/src/Microsoft.AspNet.Mvc.Core/DependencyInjection/MvcCoreMvcCoreBuilderExtensions.cs index e4222c898b..b8359abbb5 100644 --- a/src/Microsoft.AspNet.Mvc.Core/DependencyInjection/MvcCoreMvcCoreBuilderExtensions.cs +++ b/src/Microsoft.AspNet.Mvc.Core/DependencyInjection/MvcCoreMvcCoreBuilderExtensions.cs @@ -62,7 +62,7 @@ namespace Microsoft.Extensions.DependencyInjection // Internal for testing. internal static void AddFormatterMappingsServices(IServiceCollection services) { - services.TryAddTransient(); + services.TryAddSingleton(); } public static IMvcCoreBuilder AddAuthorization(this IMvcCoreBuilder builder) diff --git a/src/Microsoft.AspNet.Mvc.Core/Formatters/FormatFilter.cs b/src/Microsoft.AspNet.Mvc.Core/Formatters/FormatFilter.cs index 8dd2dd8f03..d72216124b 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Formatters/FormatFilter.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Formatters/FormatFilter.cs @@ -6,7 +6,6 @@ using System.Collections.Generic; using System.Linq; using Microsoft.AspNet.Mvc.ApiExplorer; using Microsoft.AspNet.Mvc.Filters; -using Microsoft.AspNet.Mvc.Infrastructure; using Microsoft.Extensions.OptionsModel; using Microsoft.Net.Http.Headers; @@ -18,39 +17,36 @@ namespace Microsoft.AspNet.Mvc.Formatters /// public class FormatFilter : IFormatFilter, IResourceFilter, IResultFilter { + private readonly MvcOptions _options; + /// /// Initializes an instance of . /// /// The - /// The - public FormatFilter(IOptions options, IActionContextAccessor actionContextAccessor) + public FormatFilter(IOptions options) { - IsActive = true; - Format = GetFormat(actionContextAccessor.ActionContext); - - if (string.IsNullOrEmpty(Format)) - { - IsActive = false; - return; - } - - ContentType = options.Value.FormatterMappings.GetMediaTypeMappingForFormat(Format); + _options = options.Value; } - /// - /// Format value in the current request. null if format not present in the current request. - /// - public string Format { get; } + /// + public string GetFormat(ActionContext context) + { + object obj; + if (context.RouteData.Values.TryGetValue("format", out obj)) + { + // null and string.Empty are equivalent for route values. + var routeValue = obj?.ToString(); + return string.IsNullOrEmpty(routeValue) ? null : routeValue; + } - /// - /// for the format value in the current request. - /// - public MediaTypeHeaderValue ContentType { get; } + var query = context.HttpContext.Request.Query["format"]; + if (query.Count > 0) + { + return query.ToString(); + } - /// - /// true if the current is active and will execute. - /// - public bool IsActive { get; } + return null; + } /// /// As a , this filter looks at the request and rejects it before going ahead if @@ -65,13 +61,15 @@ namespace Microsoft.AspNet.Mvc.Formatters throw new ArgumentNullException(nameof(context)); } - if (!IsActive) + var format = GetFormat(context); + if (format == null) { // no format specified by user, so the filter is muted return; } - if (ContentType == null) + var contentType = _options.FormatterMappings.GetMediaTypeMappingForFormat(format); + if (contentType == null) { // no contentType exists for the format, return 404 context.Result = new HttpNotFoundResult(); @@ -93,7 +91,7 @@ namespace Microsoft.AspNet.Mvc.Formatters // request's format and IApiResponseMetadataProvider-provided content types similarly to an Accept // header and an output formatter's SupportedMediaTypes: Confirm action supports a more specific media // type than requested e.g. OK if "text/*" requested and action supports "text/plain". - if (!supportedMediaTypes.Any(contentType => contentType.IsSubsetOf(ContentType))) + if (!supportedMediaTypes.Any(c => c.IsSubsetOf(contentType))) { context.Result = new HttpNotFoundResult(); } @@ -116,43 +114,25 @@ namespace Microsoft.AspNet.Mvc.Formatters throw new ArgumentNullException(nameof(context)); } - if (!IsActive) + var format = GetFormat(context); + if (format == null) { - return; // no format specified by user, so the filter is muted + // no format specified by user, so the filter is muted + return; } var objectResult = context.Result as ObjectResult; if (objectResult != null) { + var contentType = _options.FormatterMappings.GetMediaTypeMappingForFormat(format); objectResult.ContentTypes.Clear(); - objectResult.ContentTypes.Add(ContentType); + objectResult.ContentTypes.Add(contentType); } } /// public void OnResultExecuted(ResultExecutedContext context) { - if (context == null) - { - throw new ArgumentNullException(nameof(context)); - } - } - - private string GetFormat(ActionContext context) - { - object format = null; - - if (!context.RouteData.Values.TryGetValue("format", out format)) - { - format = context.HttpContext.Request.Query["format"]; - } - - if (format != null) - { - return format.ToString(); - } - - return null; } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/Formatters/IFormatFilter.cs b/src/Microsoft.AspNet.Mvc.Core/Formatters/IFormatFilter.cs index 78377cde4a..301b7ea77b 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Formatters/IFormatFilter.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Formatters/IFormatFilter.cs @@ -2,7 +2,6 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using Microsoft.AspNet.Mvc.Filters; -using Microsoft.Net.Http.Headers; namespace Microsoft.AspNet.Mvc.Formatters { @@ -12,18 +11,10 @@ namespace Microsoft.AspNet.Mvc.Formatters public interface IFormatFilter : IFilterMetadata { /// - /// format value in the current request. null if format not present in the current request. + /// Gets the format value for the request associated with the provided . /// - string Format { get; } - - /// - /// for the format value in the current request. - /// - MediaTypeHeaderValue ContentType { get; } - - /// - /// true if the current is active and will execute. - /// - bool IsActive { get; } + /// The associated with the current request. + /// A format value, or null if a format cannot be determined for the request. + string GetFormat(ActionContext context); } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/ProducesAttribute.cs b/src/Microsoft.AspNet.Mvc.Core/ProducesAttribute.cs index 1f06e711c4..a969c3fbb8 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ProducesAttribute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ProducesAttribute.cs @@ -67,7 +67,7 @@ namespace Microsoft.AspNet.Mvc { // Check if there are any IFormatFilter in the pipeline, and if any of them is active. If there is one, // do not override the content type value. - if (context.Filters.OfType().All(f => !f.IsActive)) + if (context.Filters.OfType().All(f => f.GetFormat(context) == null)) { SetContentTypes(objectResult.ContentTypes); } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Formatters/FormatFilterTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Formatters/FormatFilterTest.cs index d4fd11e176..b95aafdbce 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Formatters/FormatFilterTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Formatters/FormatFilterTest.cs @@ -38,7 +38,7 @@ namespace Microsoft.AspNet.Mvc.Formatters var resultExecutingContext = mockObjects.CreateResultExecutingContext(); var resourceExecutingContext = mockObjects.CreateResourceExecutingContext(new IFilterMetadata[] { }); - var filter = new FormatFilter(mockObjects.OptionsManager, mockObjects.ActionContextAccessor); + var filter = new FormatFilter(mockObjects.OptionsManager); // Act filter.OnResourceExecuting(resourceExecutingContext); @@ -85,7 +85,7 @@ namespace Microsoft.AspNet.Mvc.Formatters ac, new IFilterMetadata[] { }); - var filter = new FormatFilter(mockObjects.OptionsManager, mockObjects.ActionContextAccessor); + var filter = new FormatFilter(mockObjects.OptionsManager); // Act filter.OnResourceExecuting(resourceExecutingContext); @@ -117,7 +117,7 @@ namespace Microsoft.AspNet.Mvc.Formatters format, MediaTypeHeaderValue.Parse(contentType)); - var filter = new FormatFilter(mockObjects.OptionsManager, mockObjects.ActionContextAccessor); + var filter = new FormatFilter(mockObjects.OptionsManager); // Act filter.OnResourceExecuting(resourceExecutingContext); @@ -140,7 +140,7 @@ namespace Microsoft.AspNet.Mvc.Formatters var mockObjects = new MockObjects(format, place); var resourceExecutingContext = mockObjects.CreateResourceExecutingContext(new IFilterMetadata[] { }); - var filter = new FormatFilter(mockObjects.OptionsManager, mockObjects.ActionContextAccessor); + var filter = new FormatFilter(mockObjects.OptionsManager); // Act filter.OnResourceExecuting(resourceExecutingContext); @@ -157,7 +157,7 @@ namespace Microsoft.AspNet.Mvc.Formatters var mockObjects = new MockObjects(); var resourceExecutingContext = mockObjects.CreateResourceExecutingContext(new IFilterMetadata[] { }); - var filter = new FormatFilter(mockObjects.OptionsManager, mockObjects.ActionContextAccessor); + var filter = new FormatFilter(mockObjects.OptionsManager); // Act filter.OnResourceExecuting(resourceExecutingContext); @@ -179,7 +179,7 @@ namespace Microsoft.AspNet.Mvc.Formatters var mockObjects = new MockObjects(format, place); var resourceExecutingContext = mockObjects.CreateResourceExecutingContext(new IFilterMetadata[] { produces }); - var filter = new FormatFilter(mockObjects.OptionsManager, mockObjects.ActionContextAccessor); + var filter = new FormatFilter(mockObjects.OptionsManager); // Act filter.OnResourceExecuting(resourceExecutingContext); @@ -200,7 +200,7 @@ namespace Microsoft.AspNet.Mvc.Formatters "xml", MediaTypeHeaderValue.Parse("application/xml")); - var filter = new FormatFilter(mockObjects.OptionsManager, mockObjects.ActionContextAccessor); + var filter = new FormatFilter(mockObjects.OptionsManager); // Act filter.OnResourceExecuting(resourceExecutingContext); @@ -221,7 +221,7 @@ namespace Microsoft.AspNet.Mvc.Formatters "xml", MediaTypeHeaderValue.Parse("application/xml;version=1")); - var filter = new FormatFilter(mockObjects.OptionsManager, mockObjects.ActionContextAccessor); + var filter = new FormatFilter(mockObjects.OptionsManager); // Act filter.OnResourceExecuting(resourceExecutingContext); @@ -247,7 +247,7 @@ namespace Microsoft.AspNet.Mvc.Formatters "xml", MediaTypeHeaderValue.Parse("application/xml")); - var filter = new FormatFilter(mockObjects.OptionsManager, mockObjects.ActionContextAccessor); + var filter = new FormatFilter(mockObjects.OptionsManager); // Act filter.OnResourceExecuting(resourceExecutingContext); @@ -266,7 +266,7 @@ namespace Microsoft.AspNet.Mvc.Formatters // Arrange var mockObjects = new MockObjects(format, place); var resourceExecutingContext = mockObjects.CreateResourceExecutingContext(new IFilterMetadata[] { }); - var filter = new FormatFilter(mockObjects.OptionsManager, mockObjects.ActionContextAccessor); + var filter = new FormatFilter(mockObjects.OptionsManager); // Act filter.OnResourceExecuting(resourceExecutingContext); @@ -276,23 +276,26 @@ namespace Microsoft.AspNet.Mvc.Formatters } [Theory] - [InlineData("json", FormatSource.RouteData, true)] - [InlineData("json", FormatSource.QueryData, true)] - [InlineData("", FormatSource.RouteAndQueryData, false)] - [InlineData(null, FormatSource.RouteAndQueryData, false)] - public void FormatFilter_IsActive( - string format, + [InlineData("json", FormatSource.RouteData, "json")] + [InlineData("json", FormatSource.QueryData, "json")] + [InlineData("", FormatSource.RouteAndQueryData, null)] + [InlineData(null, FormatSource.RouteAndQueryData, null)] + public void FormatFilter_GetFormat( + string input, FormatSource place, - bool expected) + string expected) { // Arrange - var mockObjects = new MockObjects(format, place); - var resultExecutingContext = mockObjects.CreateResultExecutingContext(); + var mockObjects = new MockObjects(input, place); + var context = mockObjects.CreateResultExecutingContext(); var filterAttribute = new FormatFilterAttribute(); - var filter = new FormatFilter(mockObjects.OptionsManager, mockObjects.ActionContextAccessor); + var filter = new FormatFilter(mockObjects.OptionsManager); - // Act and Assert - Assert.Equal(expected, filter.IsActive); + // Act + var format = filter.GetFormat(context); + + // Assert + Assert.Equal(expected, filter.GetFormat(context)); } private static void AssertMediaTypesEqual( @@ -317,7 +320,6 @@ namespace Microsoft.AspNet.Mvc.Formatters public HttpContext MockHttpContext { get; private set; } public ActionContext MockActionContext { get; private set; } - public IActionContextAccessor ActionContextAccessor { get; private set; } public IOptions OptionsManager { get; private set; } public MockObjects(string format = null, FormatSource? place = null) @@ -394,10 +396,6 @@ namespace Microsoft.AspNet.Mvc.Formatters // Setup MVC services on mock service provider MockActionContext = CreateMockActionContext(httpContext, format, place); - ActionContextAccessor = new ActionContextAccessor() - { - ActionContext = MockActionContext, - }; } } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ProducesAttributeTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ProducesAttributeTests.cs index 8f0834da7c..d70fa2cae1 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ProducesAttributeTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ProducesAttributeTests.cs @@ -45,8 +45,9 @@ namespace Microsoft.AspNet.Mvc.Test var producesContentAttribute = new ProducesAttribute("application/xml"); var formatFilter = new Mock(); - formatFilter.Setup(f => f.IsActive) - .Returns(false); + formatFilter + .Setup(f => f.GetFormat(It.IsAny())) + .Returns((string)null); var filters = new IFilterMetadata[] { producesContentAttribute, formatFilter.Object }; var resultExecutingContext = CreateResultExecutingContext(filters); @@ -69,8 +70,9 @@ namespace Microsoft.AspNet.Mvc.Test var producesContentAttribute = new ProducesAttribute("application/xml"); var formatFilter = new Mock(); - formatFilter.Setup(f => f.IsActive) - .Returns(true); + formatFilter + .Setup(f => f.GetFormat(It.IsAny())) + .Returns("xml"); var filters = new IFilterMetadata[] { producesContentAttribute, formatFilter.Object }; var resultExecutingContext = CreateResultExecutingContext(filters);