From 7ee80020e1edcf4ffd243af1f0fe2afbe24aa8d7 Mon Sep 17 00:00:00 2001 From: Mugdha Kulkarni Date: Mon, 19 Jan 2015 13:41:35 -0800 Subject: [PATCH] Incorporating PR comments - 1/19 --- samples/MvcSample.Web/Startup.cs | 1 + .../Filters/FormatFilterAttribute.cs | 39 +++++++++++++----- .../Filters/IFormatFilter.cs | 6 ++- .../FormatterMappings.cs | 22 ++++++---- src/Microsoft.AspNet.Mvc.Core/MvcOptions.cs | 3 ++ src/Microsoft.AspNet.Mvc/MvcOptionsSetup.cs | 2 +- .../Filters/FormatFilterTest.cs | 6 ++- .../Filters/ProducesAttributeTests.cs | 3 +- .../FormatterMappingsTest.cs | 9 ++-- .../ConnegTests.cs | 3 +- .../FormatFilterTest.cs | 32 +++++++++++++- .../Controllers/FormatFilterController.cs | 17 ++++++-- .../Controllers/FormatFilterController.cs | 2 +- test/WebSites/FormatFilterWebSite/Startup.cs | 6 ++- .../FormatFilterWebSite/wwwroot/readme.md | Bin 0 -> 536 bytes 15 files changed, 117 insertions(+), 34 deletions(-) create mode 100644 test/WebSites/FormatFilterWebSite/wwwroot/readme.md diff --git a/samples/MvcSample.Web/Startup.cs b/samples/MvcSample.Web/Startup.cs index d2f766cc09..ca21bbcf01 100644 --- a/samples/MvcSample.Web/Startup.cs +++ b/samples/MvcSample.Web/Startup.cs @@ -98,6 +98,7 @@ namespace MvcSample.Web services.AddSingleton(); services.AddSingleton(); services.AddTransient(); + // Setup services with a test AssemblyProvider so that only the // sample's assemblies are loaded. This prevents loading controllers from other assemblies // when the sample is used in the Functional Tests. diff --git a/src/Microsoft.AspNet.Mvc.Core/Filters/FormatFilterAttribute.cs b/src/Microsoft.AspNet.Mvc.Core/Filters/FormatFilterAttribute.cs index d657aaf312..0fd28aa08e 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Filters/FormatFilterAttribute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Filters/FormatFilterAttribute.cs @@ -13,8 +13,8 @@ using System.Collections.Generic; namespace Microsoft.AspNet.Mvc { /// - /// This will look at the format parameter if present in the route data or query data and - /// sets the content type in ObjectResult corresponding to the format value. + /// This will look at the format parameter if present in the route data or query data and sets the content type in + /// ObjectResult corresponding to the format value. /// [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)] public class FormatFilterAttribute : Attribute, IFormatFilter, IResourceFilter, IResultFilter @@ -22,7 +22,7 @@ namespace Microsoft.AspNet.Mvc /// /// As a resourceFilter, this filter looks at the request and rejects it /// before going ahead if - /// 1. The Format in the request doesnt match any format in the map. + /// 1. The format in the request doesnt match any format in the map. /// 2. If there is a conflicting producesFilter. /// /// @@ -30,7 +30,7 @@ namespace Microsoft.AspNet.Mvc { var format = GetFormat(context); - if (format != null && !string.IsNullOrEmpty(format.ToString())) + if (!string.IsNullOrEmpty(format)) { var formatContentType = GetContentType(format, context); if (formatContentType == null) @@ -40,10 +40,11 @@ namespace Microsoft.AspNet.Mvc } else { - var responseTypeFilters = context.Filters.OfType(); + var responseTypeFilters = context.Filters.OfType(); if (responseTypeFilters.Count() != 0) { var contentTypes = new List(); + foreach (var filter in responseTypeFilters) { filter.SetContentTypes(contentTypes); @@ -53,16 +54,20 @@ namespace Microsoft.AspNet.Mvc { context.Result = new HttpNotFoundResult(); } - } + } } } } + /// public void OnResourceExecuted([NotNull] ResourceExecutedContext context) { } + /// + /// This method looks at the format that request has and sets it in the context + /// public void OnResultExecuting([NotNull] ResultExecutingContext context) { var format = GetFormat(context); @@ -79,12 +84,19 @@ namespace Microsoft.AspNet.Mvc } } } - + + /// public void OnResultExecuted([NotNull] ResultExecutedContext context) { } + /// + /// Looks at the current request for the format parameter. If it contains format, it returns the content type + /// for it. + /// + /// + /// public MediaTypeHeaderValue GetContentTypeForCurrentRequest(FilterContext context) { var format = GetFormat(context); @@ -96,7 +108,7 @@ namespace Microsoft.AspNet.Mvc return null; } - private object GetFormat(FilterContext context) + private string GetFormat(FilterContext context) { object format = null; @@ -105,17 +117,22 @@ namespace Microsoft.AspNet.Mvc if (context.HttpContext.Request.Query.ContainsKey("format")) { format = context.HttpContext.Request.Query.Get("format"); + return format.ToString(); } } + else + { + return format.ToString(); + } - return format; + return null; } private MediaTypeHeaderValue GetContentType(object format, FilterContext context) { Debug.Assert(format != null); - var options = context.HttpContext.RequestServices.GetService>(); - var contentType = options.Options.FormatterMappings.GetContentTypeForFormat(format.ToString()); + var options = context.HttpContext.RequestServices.GetRequiredService>(); + var contentType = options.Options.FormatterMappings.GetMediaTypeForFormat(format.ToString()); return contentType; } } diff --git a/src/Microsoft.AspNet.Mvc.Core/Filters/IFormatFilter.cs b/src/Microsoft.AspNet.Mvc.Core/Filters/IFormatFilter.cs index 3df67ed4f9..f16c14893d 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Filters/IFormatFilter.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Filters/IFormatFilter.cs @@ -7,10 +7,14 @@ using System; namespace Microsoft.AspNet.Mvc { /// - /// Implement this interface if you want to have your own implementation of FormatFilter + /// Implement this interface if you want to have your own implementation of FormatFilter. A FormatFilter decides + /// what content type to use if the format is present in the Url. /// public interface IFormatFilter : IFilter { + /// + /// Get the registered fot the format in the request. + /// MediaTypeHeaderValue GetContentTypeForCurrentRequest(FilterContext context); } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/FormatterMappings.cs b/src/Microsoft.AspNet.Mvc.Core/FormatterMappings.cs index 59e5cf7eab..f4e2f05b00 100644 --- a/src/Microsoft.AspNet.Mvc.Core/FormatterMappings.cs +++ b/src/Microsoft.AspNet.Mvc.Core/FormatterMappings.cs @@ -11,15 +11,24 @@ using Microsoft.AspNet.Mvc.Core; namespace Microsoft.AspNet.Mvc { /// - /// These options are used to specify mapping between the Url Format and corresponding ContentType. + /// Used to specify mapping between the Url Format and corresponding . /// public class FormatterMappings { private readonly Dictionary _map = new Dictionary(StringComparer.OrdinalIgnoreCase); - public void SetFormatMapping([NotNull] string format, [NotNull] MediaTypeHeaderValue contentType) + /// + /// This will set mapping for the format to specified . + /// If the format already exists, the will be overwritten with the new value. + /// + public void SetMediaTypeMappingForFormat([NotNull] string format, [NotNull] MediaTypeHeaderValue contentType) { + if (string.IsNullOrEmpty(format)) + { + throw new ArgumentException(Resources.ArgumentCannotBeNullOrEmpty, "format"); + } + if (contentType == null) { throw new ArgumentException((Resources.ArgumentCannotBeNullOrEmpty), "contentType"); @@ -29,7 +38,10 @@ namespace Microsoft.AspNet.Mvc _map[format] = contentType; } - public MediaTypeHeaderValue GetContentTypeForFormat(string format) + /// + /// Gets for the specified format. + /// + public MediaTypeHeaderValue GetMediaTypeForFormat(string format) { format = RemovePeriodIfPresent(format); MediaTypeHeaderValue value = null; @@ -39,10 +51,6 @@ namespace Microsoft.AspNet.Mvc private string RemovePeriodIfPresent(string format) { - if (string.IsNullOrEmpty(format)) - { - throw new ArgumentException(Resources.ArgumentCannotBeNullOrEmpty, "format"); - } if (format.StartsWith(".")) { format = format.Substring(1); diff --git a/src/Microsoft.AspNet.Mvc.Core/MvcOptions.cs b/src/Microsoft.AspNet.Mvc.Core/MvcOptions.cs index 6fb389ca7d..9a61898512 100644 --- a/src/Microsoft.AspNet.Mvc.Core/MvcOptions.cs +++ b/src/Microsoft.AspNet.Mvc.Core/MvcOptions.cs @@ -54,6 +54,9 @@ namespace Microsoft.AspNet.Mvc } } + /// + /// Used to specify mapping between the Url Format and corresponding . + /// public FormatterMappings FormatterMappings { get; } /// diff --git a/src/Microsoft.AspNet.Mvc/MvcOptionsSetup.cs b/src/Microsoft.AspNet.Mvc/MvcOptionsSetup.cs index 2a3538124e..615faedb6e 100644 --- a/src/Microsoft.AspNet.Mvc/MvcOptionsSetup.cs +++ b/src/Microsoft.AspNet.Mvc/MvcOptionsSetup.cs @@ -47,7 +47,7 @@ namespace Microsoft.AspNet.Mvc options.OutputFormatters.Add(new JsonOutputFormatter()); // Set up default mapping for json extensions to content type - options.FormatterMappings.SetFormatMapping("json", MediaTypeHeaderValue.Parse("application/json")); + options.FormatterMappings.SetMediaTypeMappingForFormat("json", MediaTypeHeaderValue.Parse("application/json")); // Set up default input formatters. options.InputFormatters.Add(new JsonInputFormatter()); diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Filters/FormatFilterTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Filters/FormatFilterTest.cs index 7ab55493d2..ba1698d88a 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Filters/FormatFilterTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Filters/FormatFilterTest.cs @@ -66,6 +66,8 @@ namespace Microsoft.AspNet.Mvc [Fact] public void FormatFilter_ContextContainsFormat_InRouteAndQueryData() { + // If the format is present in both route and query data, the one in route data wins + // Arrange var mediaType = MediaTypeHeaderValue.Parse("application/json"); @@ -116,7 +118,7 @@ namespace Microsoft.AspNet.Mvc var resultExecutingContext = CreateResultExecutingContext(format, place); var resourceExecutingContext = CreateResourceExecutingContext(new IFilter[] { }, format, place); var options = resultExecutingContext.HttpContext.RequestServices.GetService>(); - options.Options.FormatterMappings.SetFormatMapping(format, MediaTypeHeaderValue.Parse(contentType)); + options.Options.FormatterMappings.SetMediaTypeMappingForFormat(format, MediaTypeHeaderValue.Parse(contentType)); var filter = new FormatFilterAttribute(); @@ -194,7 +196,7 @@ namespace Microsoft.AspNet.Mvc new string[] { "application/foo", "text/bar" }); var context = CreateResourceExecutingContext(new IFilter[] { produces }, "star", FormatSource.RouteData); var options = context.HttpContext.RequestServices.GetService>(); - options.Options.FormatterMappings.SetFormatMapping("star", MediaTypeHeaderValue.Parse("application/*")); + options.Options.FormatterMappings.SetMediaTypeMappingForFormat("star", MediaTypeHeaderValue.Parse("application/*")); var filter = new FormatFilterAttribute(); diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Filters/ProducesAttributeTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Filters/ProducesAttributeTests.cs index 9f699627e9..8f2b4db5c2 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Filters/ProducesAttributeTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Filters/ProducesAttributeTests.cs @@ -131,7 +131,8 @@ namespace Microsoft.AspNet.Mvc.Test return new ResultExecutingContext( CreateActionContext(), filters, - new ObjectResult("Some Value")); + new ObjectResult("Some Value"), + controller: new object()); } private static ActionContext CreateActionContext() diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/FormatterMappingsTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/FormatterMappingsTest.cs index 38965622c7..9b583257e5 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/FormatterMappingsTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/FormatterMappingsTest.cs @@ -21,13 +21,13 @@ namespace Microsoft.AspNet.Mvc // Arrange var mediaType = MediaTypeHeaderValue.Parse(contentType); var options = new FormatterMappings(); - options.SetFormatMapping(setFormat, mediaType); + options.SetMediaTypeMappingForFormat(setFormat, mediaType); // Act - var returnmediaType = options.GetContentTypeForFormat(getFormat); + var returnMediaType = options.GetMediaTypeForFormat(getFormat); // Assert - Assert.Equal(mediaType, returnmediaType); + Assert.Equal(mediaType, returnMediaType); } [Theory] @@ -45,9 +45,10 @@ namespace Microsoft.AspNet.Mvc } var options = new FormatterMappings(); + var expectedError = "Value cannot be null or empty." + Environment.NewLine + "Parameter name: format"; // Act and Assert - Assert.Throws(() => options.SetFormatMapping(format, mediaType)); + Assert.Throws(() => options.SetMediaTypeMappingForFormat(format, mediaType)); } } } \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ConnegTests.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ConnegTests.cs index 709dea295e..2e560c4e18 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/ConnegTests.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/ConnegTests.cs @@ -432,8 +432,9 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests var response = await client.GetAsync("http://localhost/FormatFilter/MethodWithFormatFilter"); // Assert - var type = response.Content.Headers.ContentType; + Assert.Equal(HttpStatusCode.OK, response.StatusCode); var body = await response.Content.ReadAsStringAsync(); + Assert.Equal(body, "MethodWithFormatFilter"); } } } \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/FormatFilterTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/FormatFilterTest.cs index e5aee15f4d..7b198cd9b8 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/FormatFilterTest.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/FormatFilterTest.cs @@ -12,7 +12,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests { public class FormatFilterTest { - private readonly IServiceProvider _services = TestHelper.CreateServices("FormatFilterWebSite"); + private readonly IServiceProvider _services = TestHelper.CreateServices(nameof(FormatFilterWebSite)); private readonly Action _app = new FormatFilterWebSite.Startup().Configure; [Fact] @@ -45,6 +45,21 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests Assert.Equal(@"{""SampleInt"":5}", await response.Content.ReadAsStringAsync()); } + [Fact] + public async Task FormatFilter_ExtensionInRequest_Optional() + { + // Arrange + var server = TestServer.Create(_services, _app); + var client = server.CreateClient(); + + // Act + var response = await client.GetAsync("http://localhost/FormatFilter/GetProduct.json"); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal(@"{""SampleInt"":0}", await response.Content.ReadAsStringAsync()); + } + [Fact] public async Task FormatFilter_ExtensionInRequest_Custom() { @@ -60,6 +75,21 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests Assert.Equal(@"SampleInt:5", await response.Content.ReadAsStringAsync()); } + [Fact] + public async Task FormatFilter_ExtensionInRequest_CaseInsensitivity() + { + // Arrange + var server = TestServer.Create(_services, _app); + var client = server.CreateClient(); + + // Act + var response = await client.GetAsync("http://localhost/FormatFilter/GetProduct/5.Custom"); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal(@"SampleInt:5", await response.Content.ReadAsStringAsync()); + } + [Fact] public async Task FormatFilter_ExtensionInRequest_NonExistant() { diff --git a/test/WebSites/ConnegWebSite/Controllers/FormatFilterController.cs b/test/WebSites/ConnegWebSite/Controllers/FormatFilterController.cs index 75ee7711b2..85f4951245 100644 --- a/test/WebSites/ConnegWebSite/Controllers/FormatFilterController.cs +++ b/test/WebSites/ConnegWebSite/Controllers/FormatFilterController.cs @@ -9,10 +9,21 @@ namespace ConnegWebsite [Produces("application/FormatFilterController")] public class FormatFilterController : Controller { - [FormatFilter] - public User MethodWithFormatFilter() + public override void OnActionExecuted(ActionExecutedContext context) { - return new User() { Name = "Joe", Address = "1 abc way" }; + var result = context.Result as ObjectResult; + if (result != null) + { + result.Formatters.Add(new CustomFormatter("application/FormatFilterController")); + } + + base.OnActionExecuted(context); + } + + [FormatFilter] + public string MethodWithFormatFilter() + { + return "MethodWithFormatFilter"; } } } \ No newline at end of file diff --git a/test/WebSites/FormatFilterWebSite/Controllers/FormatFilterController.cs b/test/WebSites/FormatFilterWebSite/Controllers/FormatFilterController.cs index aafbaee7b7..e27aeeebb2 100644 --- a/test/WebSites/FormatFilterWebSite/Controllers/FormatFilterController.cs +++ b/test/WebSites/FormatFilterWebSite/Controllers/FormatFilterController.cs @@ -8,7 +8,7 @@ namespace FormatFilterWebSite [FormatFilter] public class FormatFilterController : Controller { - public Product GetProduct(int id) + public Product GetProduct(int id = 0) { return new Product() { SampleInt = id }; } diff --git a/test/WebSites/FormatFilterWebSite/Startup.cs b/test/WebSites/FormatFilterWebSite/Startup.cs index 5486deb46e..e181a891f5 100644 --- a/test/WebSites/FormatFilterWebSite/Startup.cs +++ b/test/WebSites/FormatFilterWebSite/Startup.cs @@ -26,7 +26,7 @@ namespace FormatFilterWebSite var customFormatter = new CustomFormatter("application/custom"); options.OutputFormatters.Add(customFormatter); - options.FormatterMappings.SetFormatMapping( + options.FormatterMappings.SetMediaTypeMappingForFormat( "custom", MediaTypeHeaderValue.Parse("application/custom")); }); @@ -37,6 +37,10 @@ namespace FormatFilterWebSite routes.MapRoute("formatroute", "{controller}/{action}/{id}.{format?}", new { controller = "Home", action = "Index" }); + + routes.MapRoute("optionalroute", + "{controller}/{action}.{format?}", + new { controller = "Home", action = "Index" }); }); } } diff --git a/test/WebSites/FormatFilterWebSite/wwwroot/readme.md b/test/WebSites/FormatFilterWebSite/wwwroot/readme.md new file mode 100644 index 0000000000000000000000000000000000000000..cc2f48a162eeb5fa7afb7604e808b58709084006 GIT binary patch literal 536 zcmZXROHRZ<3`A><#2s=1ZoqaV7OViVZt|Io$WQqwn#%*P+CoHwqI9_(yIih(f1NaH zRs)!!Kz+q+ zBG(f8tig%8QDv?(=ctC$4DLA-4}9Cf1~lVFhdbZE4t(ZTQJsj*nev>On|GUdf@#qM zxhEJxNzbg|pHg?5EoOV)-#N{}FDp&dRA8BTyy=j+S6!KDg}3wndXD>r6a7J5Vy^{O tJKKUg=-nI@*{_XB;jG>9{&v(IC3RMI&I&?vxE=rO*2U=B>7Dh7h9B@4VPpUR literal 0 HcmV?d00001