From 6a0f471a420fdde6152abd282b00dd2a0e99e839 Mon Sep 17 00:00:00 2001 From: Mugdha Kulkarni Date: Thu, 22 Jan 2015 17:05:15 -0800 Subject: [PATCH] Adding ClearMediaTypeMappingForFormat to FormatterMappings and other PR comments --- samples/MvcSample.Web/Models/Product.cs | 4 +- .../ActionConstraints/ConsumesAttribute.cs | 2 +- .../Filters/FormatFilterAttribute.cs | 16 +++-- .../Filters/IFormatFilter.cs | 11 ++- .../FormatterMappings.cs | 35 +++++++-- .../Properties/Resources.Designer.cs | 13 ++++ src/Microsoft.AspNet.Mvc.Core/Resources.resx | 5 +- src/Microsoft.AspNet.Mvc/MvcOptionsSetup.cs | 2 +- .../Filters/FormatFilterTest.cs | 31 +++++++- .../FormatterMappingsTest.cs | 72 ++++++++++++++++++- .../Controllers/FormatFilterController.cs | 3 +- .../Controllers/ProducesOverrideClass.cs | 1 - .../FormatFilterWebSite/CustomFormatter.cs | 5 +- 13 files changed, 166 insertions(+), 34 deletions(-) diff --git a/samples/MvcSample.Web/Models/Product.cs b/samples/MvcSample.Web/Models/Product.cs index b1eb2434fc..53b0371251 100644 --- a/samples/MvcSample.Web/Models/Product.cs +++ b/samples/MvcSample.Web/Models/Product.cs @@ -1,4 +1,6 @@ -using System; +// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; namespace MvcSample.Web { diff --git a/src/Microsoft.AspNet.Mvc.Core/ActionConstraints/ConsumesAttribute.cs b/src/Microsoft.AspNet.Mvc.Core/ActionConstraints/ConsumesAttribute.cs index b118a51cc3..3d9edd6ee1 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ActionConstraints/ConsumesAttribute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ActionConstraints/ConsumesAttribute.cs @@ -118,7 +118,7 @@ namespace Microsoft.AspNet.Mvc CurrentCandidate = candidate }; - if (candidate.Constraints == null || candidate.Constraints.Count() == 0 || + if (candidate.Constraints == null || candidate.Constraints.Count == 0 || candidate.Constraints.Any(constraint => constraint is IConsumesActionConstraint && constraint.Accept(tempContext))) { diff --git a/src/Microsoft.AspNet.Mvc.Core/Filters/FormatFilterAttribute.cs b/src/Microsoft.AspNet.Mvc.Core/Filters/FormatFilterAttribute.cs index bca50549e2..1950971929 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Filters/FormatFilterAttribute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Filters/FormatFilterAttribute.cs @@ -2,13 +2,13 @@ // 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.Diagnostics; using System.Linq; using Microsoft.AspNet.Mvc.Description; using Microsoft.Framework.DependencyInjection; using Microsoft.Framework.OptionsModel; using Microsoft.Net.Http.Headers; -using System.Collections.Generic; namespace Microsoft.AspNet.Mvc { @@ -47,7 +47,7 @@ namespace Microsoft.AspNet.Mvc filter.SetContentTypes(contentTypes); } - if (contentTypes.Count() != 0) + if (contentTypes.Count != 0) { // If formatfilterContentType is not subset of any of the content types produced by // IApiResponseMetadataProviders, return 404 @@ -93,10 +93,11 @@ namespace Microsoft.AspNet.Mvc } /// - /// If the current request contains format value, returns true. It means the format filter is going to execute. + /// Returns true if the filter is active and will execute; otherwise, false. The filter is active + /// if the current request contains format value. /// /// The - /// If the filter is active and will execute. + /// true if the current request contains format value; otherwise, false. public bool IsActive(FilterContext context) { var format = GetFormat(context); @@ -113,7 +114,12 @@ namespace Microsoft.AspNet.Mvc format = context.HttpContext.Request.Query["format"]; } - return (string)format; + if (format != null) + { + return format.ToString(); + } + + return null; } private MediaTypeHeaderValue GetContentType(string format, FilterContext context) diff --git a/src/Microsoft.AspNet.Mvc.Core/Filters/IFormatFilter.cs b/src/Microsoft.AspNet.Mvc.Core/Filters/IFormatFilter.cs index 1107dd760f..09ebc7e262 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Filters/IFormatFilter.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Filters/IFormatFilter.cs @@ -1,23 +1,20 @@ // Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using Microsoft.Net.Http.Headers; -using System; - namespace Microsoft.AspNet.Mvc { /// /// A filter which produces a desired content type for the current request. /// - /// A FormatFilter decides what content type to use if a format value is present in the URL. - /// public interface IFormatFilter : IFilter { /// - /// Returns true if the filter is going to be executed for the current request. + /// Returns true if the filter will produce a content type for the current request, otherwise + /// false. /// /// The - /// If the filter will execute + /// true if the filter will produce a content type for the current request; otherwise, + /// false. bool IsActive(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 09c1051253..b759c9dadb 100644 --- a/src/Microsoft.AspNet.Mvc.Core/FormatterMappings.cs +++ b/src/Microsoft.AspNet.Mvc.Core/FormatterMappings.cs @@ -22,8 +22,8 @@ namespace Microsoft.AspNet.Mvc /// Sets mapping for the format to specified . /// If the format already exists, the will be overwritten with the new value. /// - /// format value - /// The for the format value + /// The format value. + /// The for the format value. public void SetMediaTypeMappingForFormat([NotNull] string format, [NotNull] MediaTypeHeaderValue contentType) { ValidateContentType(contentType); @@ -34,8 +34,8 @@ namespace Microsoft.AspNet.Mvc /// /// Gets for the specified format. /// - /// format value. - /// The for input format + /// The format value. + /// The for input format. public MediaTypeHeaderValue GetMediaTypeMappingForFormat([NotNull] string format) { format = RemovePeriodIfPresent(format); @@ -46,9 +46,20 @@ namespace Microsoft.AspNet.Mvc return value; } + /// + /// Clears the mapping for the format. + /// + /// The format value. + /// true if the format is successfully found and cleared; otherwise, false. + public bool ClearMediaTypeMappingForFormat([NotNull] string format) + { + format = RemovePeriodIfPresent(format); + return _map.Remove(format); + } + private void ValidateContentType(MediaTypeHeaderValue contentType) { - if(contentType.Type == "*" || contentType.SubType == "*") + if (contentType.Type == "*" || contentType.SubType == "*") { throw new ArgumentException(string.Format(Resources.FormatterMappings_NotValidMediaType, contentType)); } @@ -56,10 +67,20 @@ namespace Microsoft.AspNet.Mvc private string RemovePeriodIfPresent(string format) { + if (format == "") + { + throw new ArgumentException(Resources.ArgumentCannotBeNullOrEmpty, "format"); + } + if (format.StartsWith(".")) { - format = format.Substring(1); - } + if (format == ".") + { + throw new ArgumentException(string.Format(Resources.Format_NotValid, format)); + } + + format = format.Substring(1); + } return format; } diff --git a/src/Microsoft.AspNet.Mvc.Core/Properties/Resources.Designer.cs b/src/Microsoft.AspNet.Mvc.Core/Properties/Resources.Designer.cs index 96ae8d636e..8ae8d6c081 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Properties/Resources.Designer.cs @@ -562,11 +562,24 @@ namespace Microsoft.AspNet.Mvc.Core get { return GetString("Common_ValueNotValidForProperty"); } } + /// + /// The media type "{0}" is not valid. MediaTypes containing wildcards (*) are not allowed in formatter + /// mappings. + /// internal static string FormatterMappings_NotValidMediaType { get { return GetString("FormatterMappings_NotValidMediaType"); } } + /// + /// The format provided is invalid '{0}'. A format must be a non-empty file-extension, optionally prefixed + /// with a '.' character. + /// + internal static string Format_NotValid + { + get { return GetString("Format_NotValid"); } + } + /// /// The value '{0}' is invalid. /// diff --git a/src/Microsoft.AspNet.Mvc.Core/Resources.resx b/src/Microsoft.AspNet.Mvc.Core/Resources.resx index 27c11d8224..66e98854e1 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Resources.resx +++ b/src/Microsoft.AspNet.Mvc.Core/Resources.resx @@ -452,7 +452,10 @@ The action '{0}' has ApiExplorer enabled, but is using conventional routing. Only actions which use attribute routing support ApiExplorer. - The media type {0} is not valid. The media type containing "<mediatype>/*" are not supported. + The media type "{0}" is not valid. MediaTypes containing wildcards (*) are not allowed in formatter mappings. + + + The format provided is invalid '{0}'. A format must be a non-empty file-extension, optionally prefixed with a '.' character. No URL for remote validation could be found. diff --git a/src/Microsoft.AspNet.Mvc/MvcOptionsSetup.cs b/src/Microsoft.AspNet.Mvc/MvcOptionsSetup.cs index 615faedb6e..85d97c6d03 100644 --- a/src/Microsoft.AspNet.Mvc/MvcOptionsSetup.cs +++ b/src/Microsoft.AspNet.Mvc/MvcOptionsSetup.cs @@ -6,8 +6,8 @@ using System.Xml.Linq; using Microsoft.AspNet.Mvc.ModelBinding; using Microsoft.AspNet.Mvc.Razor; using Microsoft.Framework.OptionsModel; -using Newtonsoft.Json.Linq; using Microsoft.Net.Http.Headers; +using Newtonsoft.Json.Linq; namespace Microsoft.AspNet.Mvc { diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Filters/FormatFilterTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Filters/FormatFilterTest.cs index 1ae32112b5..25e581643b 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Filters/FormatFilterTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Filters/FormatFilterTest.cs @@ -118,7 +118,9 @@ 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.SetMediaTypeMappingForFormat(format, MediaTypeHeaderValue.Parse(contentType)); + options.Options.FormatterMappings.SetMediaTypeMappingForFormat( + format, + MediaTypeHeaderValue.Parse(contentType)); var filter = new FormatFilterAttribute(); @@ -194,7 +196,28 @@ namespace Microsoft.AspNet.Mvc var produces = new ProducesAttribute("application/xml;version=1", new string [] { }); var context = CreateResourceExecutingContext(new IFilter[] { produces }, "xml", FormatSource.RouteData); var options = context.HttpContext.RequestServices.GetService>(); - options.Options.FormatterMappings.SetMediaTypeMappingForFormat("xml", MediaTypeHeaderValue.Parse("application/xml")); + options.Options.FormatterMappings.SetMediaTypeMappingForFormat( + "xml", + MediaTypeHeaderValue.Parse("application/xml")); + var filter = new FormatFilterAttribute(); + + // Act + filter.OnResourceExecuting(context); + + // Assert + Assert.Null(context.Result); + } + + [Fact] + public void FormatFilter_LessSpecificThan_Produces_Wildcard() + { + // Arrange + var produces = new ProducesAttribute("application/*", new string[] { }); + var context = CreateResourceExecutingContext(new IFilter[] { produces }, "xml", FormatSource.RouteData); + var options = context.HttpContext.RequestServices.GetService>(); + options.Options.FormatterMappings.SetMediaTypeMappingForFormat( + "xml", + MediaTypeHeaderValue.Parse("application/xml")); var filter = new FormatFilterAttribute(); // Act @@ -211,7 +234,9 @@ namespace Microsoft.AspNet.Mvc var produces = new ProducesAttribute("application/xml", new string[] { }); var context = CreateResourceExecutingContext(new IFilter[] { produces }, "xml", FormatSource.RouteData); var options = context.HttpContext.RequestServices.GetService>(); - options.Options.FormatterMappings.SetMediaTypeMappingForFormat("xml", MediaTypeHeaderValue.Parse("application/xml;version=1")); + options.Options.FormatterMappings.SetMediaTypeMappingForFormat( + "xml", + MediaTypeHeaderValue.Parse("application/xml;version=1")); var filter = new FormatFilterAttribute(); // Act diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/FormatterMappingsTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/FormatterMappingsTest.cs index 080af73dd2..8e1ff8bc88 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/FormatterMappingsTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/FormatterMappingsTest.cs @@ -15,7 +15,7 @@ namespace Microsoft.AspNet.Mvc [InlineData("json", "application/json", "JSON")] [InlineData(".foo", "text/foo", "Foo")] [InlineData(".Json", "application/json", "json")] - [InlineData("FOo", "text/foo", "FOO")] + [InlineData("FOo", "text/foo", "FOO")] public void FormatterMappings_SetFormatMapping_DiffSetGetFormat(string setFormat, string contentType, string getFormat) { // Arrange @@ -30,6 +30,38 @@ namespace Microsoft.AspNet.Mvc Assert.Equal(mediaType, returnMediaType); } + [Fact] + public void FormatterMappings_Invalid_Period() + { + // Arrange + var options = new FormatterMappings(); + var format = "."; + var expected = string.Format(@"The format provided is invalid '{0}'. A format must be a non-empty file-" + + "extension, optionally prefixed with a '.' character.", format); + + // Act and assert + var exception = Assert.Throws(() => options.SetMediaTypeMappingForFormat( + format, + MediaTypeHeaderValue.Parse("application/xml"))); + Assert.Equal(expected, exception.Message); + } + + [Fact] + public void FormatterMappings_SetFormatMapping_FormatEmpty() + { + // Arrange + var options = new FormatterMappings(); + var format = ""; + var expected = "Value cannot be null or empty." + Environment.NewLine + "Parameter name: format"; + + // Act and assert + var exception = Assert.Throws(() => options.SetMediaTypeMappingForFormat( + format, + MediaTypeHeaderValue.Parse("application/xml"))); + Assert.Equal(expected, exception.Message); + } + + [Theory] [InlineData("application/*")] [InlineData("*/json")] @@ -38,11 +70,45 @@ namespace Microsoft.AspNet.Mvc { // Arrange var options = new FormatterMappings(); - var expected = string.Format(@"The media type {0} is not valid. The media type containing ""/*"" are not supported.", format); + var expected = string.Format(@"The media type ""{0}"" is not valid. MediaTypes containing wildcards (*) " + + "are not allowed in formatter mappings.", format); // Act and assert - var exception = Assert.Throws(() => options.SetMediaTypeMappingForFormat("star", MediaTypeHeaderValue.Parse(format))); + var exception = Assert.Throws(() => options.SetMediaTypeMappingForFormat( + "star", + MediaTypeHeaderValue.Parse(format))); Assert.Equal(expected, exception.Message); } + + [Theory] + [InlineData(".xml", true)] + [InlineData("json", true)] + [InlineData(".foo", true)] + [InlineData(".Json", true)] + [InlineData("FOo", true)] + [InlineData("bar", true)] + [InlineData("baz", false)] + [InlineData(".baz", false)] + [InlineData("BAZ", false)] + public void FormatterMappings_ClearFormatMapping(string format, bool expected) + { + // Arrange + var options = new FormatterMappings(); + var mediaType = MediaTypeHeaderValue.Parse("application/xml"); + options.SetMediaTypeMappingForFormat("xml", mediaType); + mediaType = MediaTypeHeaderValue.Parse("application/json"); + options.SetMediaTypeMappingForFormat("json", mediaType); + mediaType = MediaTypeHeaderValue.Parse("application/foo"); + options.SetMediaTypeMappingForFormat("foo", mediaType); + mediaType = MediaTypeHeaderValue.Parse("application/bar"); + options.SetMediaTypeMappingForFormat("bar", mediaType); + + // Act + var cleared = options.ClearMediaTypeMappingForFormat(format); + + // Assert + Assert.Equal(expected, cleared); + Assert.Null(options.GetMediaTypeMappingForFormat(format)); + } } } \ No newline at end of file diff --git a/test/WebSites/ConnegWebSite/Controllers/FormatFilterController.cs b/test/WebSites/ConnegWebSite/Controllers/FormatFilterController.cs index 85f4951245..8d49b9a97c 100644 --- a/test/WebSites/ConnegWebSite/Controllers/FormatFilterController.cs +++ b/test/WebSites/ConnegWebSite/Controllers/FormatFilterController.cs @@ -1,10 +1,9 @@ // Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using ConnegWebSite; using Microsoft.AspNet.Mvc; -namespace ConnegWebsite +namespace ConnegWebSite { [Produces("application/FormatFilterController")] public class FormatFilterController : Controller diff --git a/test/WebSites/FormatFilterWebSite/Controllers/ProducesOverrideClass.cs b/test/WebSites/FormatFilterWebSite/Controllers/ProducesOverrideClass.cs index 23b736d22e..ce4d3aee4a 100644 --- a/test/WebSites/FormatFilterWebSite/Controllers/ProducesOverrideClass.cs +++ b/test/WebSites/FormatFilterWebSite/Controllers/ProducesOverrideClass.cs @@ -11,7 +11,6 @@ namespace FormatFilterWebSite [Produces("application/ProducesMethod")] public string ReturnClassName() { - // should be written using the content defined at base class's action. return "ProducesOverrideController"; } } diff --git a/test/WebSites/FormatFilterWebSite/CustomFormatter.cs b/test/WebSites/FormatFilterWebSite/CustomFormatter.cs index 08d0df4045..45ae8cc2fa 100644 --- a/test/WebSites/FormatFilterWebSite/CustomFormatter.cs +++ b/test/WebSites/FormatFilterWebSite/CustomFormatter.cs @@ -27,6 +27,8 @@ namespace FormatFilterWebSite var actionReturn = context.Object as Product; if (actionReturn != null) { + var response = context.ActionContext.HttpContext.Response; + context.SelectedContentType = contentType; return true; } } @@ -35,8 +37,7 @@ namespace FormatFilterWebSite public override async Task WriteResponseBodyAsync(OutputFormatterContext context) { - var response = context.ActionContext.HttpContext.Response; - response.ContentType = ContentType + ";charset=utf-8"; + var response = context.ActionContext.HttpContext.Response; await response.WriteAsync(context.Object.ToString()); } }