Incorporating PR comments - 1/19

samples/MvcSample.Web/Startup.cs

@@ -98,6 +98,7 @@ namespace MvcSample.Web
                     services.AddSingleton<PassThroughAttribute>();
                     services.AddSingleton<UserNameService>();
                     services.AddTransient<ITestService, TestService>();
+                    
                     // 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.

src/Microsoft.AspNet.Mvc.Core/Filters/FormatFilterAttribute.cs

@@ -13,8 +13,8 @@ using System.Collections.Generic;
 namespace Microsoft.AspNet.Mvc
 {
     /// <summary>
-    /// 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.
     /// </summary>
     [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
     public class FormatFilterAttribute : Attribute, IFormatFilter, IResourceFilter, IResultFilter
@@ -22,7 +22,7 @@ namespace Microsoft.AspNet.Mvc
         /// <summary>
         /// 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.
         /// </summary>
         /// <param name="context"></param>
@@ -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<IApiResponseMetadataProvider>();                    
+                    var responseTypeFilters = context.Filters.OfType<IApiResponseMetadataProvider>();
                     if (responseTypeFilters.Count() != 0)
                     {
                         var contentTypes = new List<MediaTypeHeaderValue>();
+
                         foreach (var filter in responseTypeFilters)
                         {
                             filter.SetContentTypes(contentTypes);
@@ -53,16 +54,20 @@ namespace Microsoft.AspNet.Mvc
                         {
                             context.Result = new HttpNotFoundResult();
                         }
-                    }
+                    }    
                 }
             }
         }
 
+        /// <inheritdoc />
         public void OnResourceExecuted([NotNull] ResourceExecutedContext context)
         {
 
         }
 
+        /// <summary>
+        /// This method looks at the format that request has and sets it in the context
+        /// </summary>
         public void OnResultExecuting([NotNull] ResultExecutingContext context)
         {
             var format = GetFormat(context);
@@ -79,12 +84,19 @@ namespace Microsoft.AspNet.Mvc
                 }
             }
         }
-
+        
+        /// <inheritdoc />     
         public void OnResultExecuted([NotNull] ResultExecutedContext context)
         {
 
         }
 
+        /// <summary>
+        /// Looks at the current request for the format parameter. If it contains format, it returns the content type 
+        /// for it.
+        /// </summary>
+        /// <param name="context"></param>
+        /// <returns></returns>
         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<IOptions<MvcOptions>>();
-            var contentType = options.Options.FormatterMappings.GetContentTypeForFormat(format.ToString());
+            var options = context.HttpContext.RequestServices.GetRequiredService<IOptions<MvcOptions>>();
+            var contentType = options.Options.FormatterMappings.GetMediaTypeForFormat(format.ToString());
             return contentType;
         }
     }

src/Microsoft.AspNet.Mvc.Core/Filters/IFormatFilter.cs

@@ -7,10 +7,14 @@ using System;
 namespace Microsoft.AspNet.Mvc
 {
     /// <summary>
-    /// 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. 
     /// </summary>
     public interface IFormatFilter : IFilter
     {
+        /// <summary>
+        /// Get the <see cref="MediaTypeHeaderValue"/> registered fot the format in the request.
+        /// </summary>
         MediaTypeHeaderValue GetContentTypeForCurrentRequest(FilterContext context);
     }
 }

src/Microsoft.AspNet.Mvc.Core/FormatterMappings.cs

@@ -11,15 +11,24 @@ using Microsoft.AspNet.Mvc.Core;
 namespace Microsoft.AspNet.Mvc
 {
     /// <summary>
-    /// These options are used to specify mapping between the Url Format and corresponding ContentType.
+    /// Used to specify mapping between the Url Format and corresponding <see cref="MediaTypeHeaderValue"/>.
     /// </summary>
     public class FormatterMappings
     {
         private readonly Dictionary<string, MediaTypeHeaderValue> _map =
             new Dictionary<string, MediaTypeHeaderValue>(StringComparer.OrdinalIgnoreCase);
 
-        public void SetFormatMapping([NotNull] string format, [NotNull] MediaTypeHeaderValue contentType)
+        /// <summary>
+        /// This will set mapping for the format to specified <see cref="MediaTypeHeaderValue"/>. 
+        /// If the format already exists, the <see cref="MediaTypeHeaderValue"/> will be overwritten with the new value.
+        /// </summary>
+        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)
+        /// <summary>
+        /// Gets <see cref="MediaTypeHeaderValue"/> for the specified format.
+        /// </summary>
+        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);

src/Microsoft.AspNet.Mvc.Core/MvcOptions.cs

@@ -54,6 +54,9 @@ namespace Microsoft.AspNet.Mvc
             }
         }
 
+        /// <summary>
+        /// Used to specify mapping between the Url Format and corresponding <see cref="MediaTypeHeaderValue"/>.
+        /// </summary>
         public FormatterMappings FormatterMappings { get; }
 
         /// <summary>

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());

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<IOptions<MvcOptions>>();
-            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<IOptions<MvcOptions>>();
-            options.Options.FormatterMappings.SetFormatMapping("star", MediaTypeHeaderValue.Parse("application/*"));
+            options.Options.FormatterMappings.SetMediaTypeMappingForFormat("star", MediaTypeHeaderValue.Parse("application/*"));
 
             var filter = new FormatFilterAttribute();
 

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()

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<ArgumentException>(() => options.SetFormatMapping(format, mediaType));
+            Assert.Throws<ArgumentException>(() => options.SetMediaTypeMappingForFormat(format, mediaType));
         }
     }
 }

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");
         }
     }
 }

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<IApplicationBuilder> _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()
         {

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";
         }
     }
 }

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 };
         }

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" });
             });            
         }
     }