Remove ParameterModel.IsOptional

The ParameterModel and ParameterDescriptor have had a notion of
optionality for a while now, even though all parameters are treated as
'optional' in MVC.

This change removes these settings. Optionality for overloading in webapi
compat shim is reimplemented via a new binder metadata.

src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultActionModelBuilder.cs

@@ -317,7 +317,6 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             parameterModel.BinderMetadata = attributes.OfType<IBinderMetadata>().FirstOrDefault();
 
             parameterModel.ParameterName = parameterInfo.Name;
-            parameterModel.IsOptional = parameterInfo.HasDefaultValue;
 
             return parameterModel;
         }

src/Microsoft.AspNet.Mvc.Core/ApplicationModels/ParameterModel.cs

@@ -22,7 +22,6 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             Action = other.Action;
             Attributes = new List<object>(other.Attributes);
             BinderMetadata = other.BinderMetadata;
-            IsOptional = other.IsOptional;
             ParameterInfo = other.ParameterInfo;
             ParameterName = other.ParameterName;
         }
@@ -33,8 +32,6 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
 
         public IBinderMetadata BinderMetadata { get; set; }
 
-        public bool IsOptional { get; set; }
-
         public ParameterInfo ParameterInfo { get; private set; }
 
         public string ParameterName { get; set; }

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

@@ -275,7 +275,6 @@ namespace Microsoft.AspNet.Mvc
             var parameterDescriptor = new ParameterDescriptor()
             {
                 BinderMetadata = parameter.BinderMetadata,
-                IsOptional = parameter.IsOptional,
                 Name = parameter.ParameterName,
                 ParameterType = parameter.ParameterInfo.ParameterType,
             };

src/Microsoft.AspNet.Mvc.Core/Description/DefaultApiDescriptionProvider.cs

@@ -263,7 +263,6 @@ namespace Microsoft.AspNet.Mvc.Description
         {
             var resourceParameter = new ApiParameterDescription
             {
-                IsOptional = parameter.IsOptional,
                 Name = parameter.Name,
                 ParameterDescriptor = parameter,
                 Type = parameter.ParameterType,
@@ -288,7 +287,7 @@ namespace Microsoft.AspNet.Mvc.Description
             var resourceParameter = new ApiParameterDescription
             {
                 Source = ApiParameterSource.Path,
-                IsOptional = parameter.IsOptional && IsOptionalParameter(templateParameter),
+                IsOptional = IsOptionalParameter(templateParameter),
                 Name = parameter.Name,
                 ParameterDescriptor = parameter,
                 Constraints = GetConstraints(_constraintResolver, templateParameter.InlineConstraints),

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

@@ -10,8 +10,6 @@ namespace Microsoft.AspNet.Mvc
     {
         public string Name { get; set; }
 
-        public bool IsOptional { get; set; }
-
         public Type ParameterType { get; set; }
 
         public IBinderMetadata BinderMetadata { get; set; }

src/Microsoft.AspNet.Mvc.WebApiCompatShim/Conventions/WebApiParameterConventionsApplicationModelConvention.cs

@@ -1,6 +1,7 @@
 // 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;
 using System.Linq;
 using System.Web.Http;
 using Microsoft.AspNet.Mvc.ApplicationModels;
@@ -46,6 +47,13 @@ namespace Microsoft.AspNet.Mvc.WebApiCompatShim
                         // Complex types are by-default from the body.
                         parameter.BinderMetadata = new FromBodyAttribute();
                     }
+
+                    // If the parameter has a default value, we want to consider it as optional parameter by default.
+                    var optionalMetadata = parameter.BinderMetadata as FromUriAttribute;
+                    if (parameter.ParameterInfo.HasDefaultValue && optionalMetadata != null)
+                    {
+                        optionalMetadata.IsOptional = true;
+                    }
                 }
             }
         }

src/Microsoft.AspNet.Mvc.WebApiCompatShim/OverloadActionConstraint.cs

@@ -93,9 +93,17 @@ namespace Microsoft.AspNet.Mvc.WebApiCompatShim
                 // We only consider parameters that are bound from the URL.
                 if ((parameter.BinderMetadata is IRouteDataValueProviderMetadata ||
                     parameter.BinderMetadata is IQueryValueProviderMetadata) &&
-                    !parameter.IsOptional &&
                     ValueProviderResult.CanConvertFromString(parameter.ParameterType))
                 {
+                    var optionalMetadata = parameter.BinderMetadata as IOptionalBinderMetadata;
+                    if (optionalMetadata == null || optionalMetadata.IsOptional)
+                    {
+                        // Optional parameters are ignored in overloading. If a parameter doesn't specify that it's
+                        // required then treat it as optional (MVC default). WebAPI parameters will all by-default
+                        // specify themselves as required unless they have a default value.
+                        continue;
+                    }
+
                     var nameProvider = parameter.BinderMetadata as IModelNameProvider;
                     var prefix = nameProvider?.Name ?? parameter.Name;
 

src/Microsoft.AspNet.Mvc.WebApiCompatShim/ParameterBinding/FromUriAttribute.cs

@@ -12,10 +12,13 @@ namespace System.Web.Http
     [AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = true)]
     public class FromUriAttribute :
         Attribute,
+        IOptionalBinderMetadata,
         IQueryValueProviderMetadata,
         IRouteDataValueProviderMetadata,
         IModelNameProvider
     {
+        public bool IsOptional { get; set; }
+
         /// <inheritdoc />
         public string Name { get; set; }
     }

src/Microsoft.AspNet.Mvc.WebApiCompatShim/ParameterBinding/IOptionalBinderMetadata.cs

@@ -0,0 +1,17 @@
+// 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.
+
+namespace Microsoft.AspNet.Mvc.ModelBinding
+{
+    /// <summary>
+    /// An <see cref="IBinderMetadata"/> that designates an optional parameter for the purposes
+    /// of WebAPI action overloading. Optional parameters do not participate in overloading, and 
+    /// do not have to have a value for the action to be selected.
+    /// 
+    /// This has no impact when used without WebAPI action overloading.
+    /// </summary>
+    public interface IOptionalBinderMetadata : IBinderMetadata
+    {
+        bool IsOptional { get; }
+    }
+}

test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/ParameterModelTest.cs

@@ -19,7 +19,6 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
 
             parameter.Action = new ActionModel(typeof(TestController).GetMethod("Edit"), new List<object>());
             parameter.BinderMetadata = (IBinderMetadata)parameter.Attributes[0];
-            parameter.IsOptional = true;
             parameter.ParameterName = "id";
 
             // Act

test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionDescriptorProviderTests.cs

@@ -109,7 +109,6 @@ namespace Microsoft.AspNet.Mvc.Test
             var id = Assert.Single(main.Parameters);
 
             Assert.Equal("id", id.Name);
-            Assert.False(id.IsOptional);
             Assert.Null(id.BinderMetadata);
             Assert.Equal(typeof(int), id.ParameterType);
         }
@@ -129,14 +128,12 @@ namespace Microsoft.AspNet.Mvc.Test
             var id = Assert.Single(main.Parameters, p => p.Name == "id");
 
             Assert.Equal("id", id.Name);
-            Assert.False(id.IsOptional);
             Assert.Null(id.BinderMetadata);
             Assert.Equal(typeof(int), id.ParameterType);
 
             var entity = Assert.Single(main.Parameters, p => p.Name == "entity");
 
             Assert.Equal("entity", entity.Name);
-            Assert.False(entity.IsOptional);
             Assert.IsType<FromBodyAttribute>(entity.BinderMetadata);
             Assert.Equal(typeof(TestActionParameter), entity.ParameterType);
         }
@@ -156,49 +153,22 @@ namespace Microsoft.AspNet.Mvc.Test
             var id = Assert.Single(main.Parameters, p => p.Name == "id");
 
             Assert.Equal("id", id.Name);
-            Assert.False(id.IsOptional);
             Assert.Null(id.BinderMetadata);
             Assert.Equal(typeof(int), id.ParameterType);
 
             var upperCaseId = Assert.Single(main.Parameters, p => p.Name == "ID");
 
             Assert.Equal("ID", upperCaseId.Name);
-            Assert.False(upperCaseId.IsOptional);
             Assert.Null(upperCaseId.BinderMetadata);
             Assert.Equal(typeof(int), upperCaseId.ParameterType);
 
             var pascalCaseId = Assert.Single(main.Parameters, p => p.Name == "Id");
 
             Assert.Equal("Id", pascalCaseId.Name);
-            Assert.False(pascalCaseId.IsOptional);
             Assert.Null(id.BinderMetadata);
             Assert.Equal(typeof(int), pascalCaseId.ParameterType);
         }
 
-        [Theory]
-        [InlineData(nameof(ActionParametersController.OptionalInt), typeof(Nullable<int>))]
-        [InlineData(nameof(ActionParametersController.OptionalChar), typeof(char))]
-        public void GetDescriptors_AddsParametersWithDefaultValues_AsOptionalParameters(
-            string actionName,
-            Type parameterType)
-        {
-            // Arrange & Act
-            var descriptors = GetDescriptors(
-                typeof(ActionParametersController).GetTypeInfo());
-
-            // Assert
-            var optional = Assert.Single(descriptors,
-                d => d.Name.Equals(actionName));
-
-            Assert.NotNull(optional.Parameters);
-            var id = Assert.Single(optional.Parameters);
-
-            Assert.Equal("id", id.Name);
-            Assert.True(id.IsOptional);
-            Assert.Null(id.BinderMetadata);
-            Assert.Equal(parameterType, id.ParameterType);
-        }
-
         [Fact]
         public void GetDescriptors_AddsParameters_DetectsFromBodyParameters()
         {
@@ -216,7 +186,6 @@ namespace Microsoft.AspNet.Mvc.Test
             var entity = Assert.Single(fromBody.Parameters);
 
             Assert.Equal("entity", entity.Name);
-            Assert.False(entity.IsOptional);
             Assert.IsType<FromBodyAttribute>(entity.BinderMetadata);
             Assert.Equal(typeof(TestActionParameter), entity.ParameterType);
         }
@@ -238,7 +207,6 @@ namespace Microsoft.AspNet.Mvc.Test
             var entity = Assert.Single(notFromBody.Parameters);
 
             Assert.Equal("entity", entity.Name);
-            Assert.False(entity.IsOptional);
             Assert.Null(entity.BinderMetadata);
             Assert.Equal(typeof(TestActionParameter), entity.ParameterType);
         }
@@ -1710,10 +1678,6 @@ namespace Microsoft.AspNet.Mvc.Test
         {
             public void RequiredInt(int id) { }
 
-            public void OptionalInt(int? id = 5) { }
-
-            public void OptionalChar(char id = 'c') { }
-
             public void FromBodyParameter([FromBody] TestActionParameter entity) { }
 
             public void NotFromBodyParameter(TestActionParameter entity) { }

test/Microsoft.AspNet.Mvc.Core.Test/Description/DefaultApiDescriptionProviderTest.cs

@@ -121,7 +121,6 @@ namespace Microsoft.AspNet.Mvc.Description
                 new ParameterDescriptor()
                 {
                     Name = "id",
-                    IsOptional = true,
                     ParameterType = typeof(int),
                 },
                 new ParameterDescriptor()
@@ -141,7 +140,7 @@ namespace Microsoft.AspNet.Mvc.Description
 
             var id = Assert.Single(description.ParameterDescriptions, p => p.Name == "id");
             Assert.NotNull(id.ModelMetadata);
-            Assert.True(id.IsOptional);
+            Assert.False(id.IsOptional);
             Assert.Same(action.Parameters[0], id.ParameterDescriptor);
             Assert.Equal(ApiParameterSource.Query, id.Source);
             Assert.Equal(typeof(int), id.Type);
@@ -224,7 +223,6 @@ namespace Microsoft.AspNet.Mvc.Description
             var parameterDescriptor = new ParameterDescriptor
             {
                 Name = "id",
-                IsOptional = true,
                 ParameterType = typeof(int),
             };
             action.Parameters = new List<ParameterDescriptor> { parameterDescriptor };
@@ -280,7 +278,6 @@ namespace Microsoft.AspNet.Mvc.Description
             {
                 BinderMetadata = new FromBodyAttribute(),
                 Name = "id",
-                IsOptional = false,
                 ParameterType = typeof(int),
             };
             action.Parameters = new List<ParameterDescriptor> { parameterDescriptor };
@@ -317,15 +314,11 @@ namespace Microsoft.AspNet.Mvc.Description
         }
 
         [Theory]
-        [InlineData("api/products/{id}", false, false)]
+        [InlineData("api/products/{id}", false)]
-        [InlineData("api/products/{id}", true, false)]
+        [InlineData("api/products/{id?}", true)]
-        [InlineData("api/products/{id?}", false, false)]
+        [InlineData("api/products/{id=5}", true)]
-        [InlineData("api/products/{id?}", true, true)]
+        public void GetApiDescription_ParameterFromPathAndDescriptor_IsOptionalIfRouteParameterIsOptional(
-        [InlineData("api/products/{id=5}", false, false)]
-        [InlineData("api/products/{id=5}", true, true)]
-        public void GetApiDescription_ParameterFromPathAndDescriptor_IsOptionalOnly_IfBothAreOptional(
             string template,
-            bool isDescriptorParameterOptional,
             bool expectedOptional)
         {
             // Arrange
@@ -335,7 +328,6 @@ namespace Microsoft.AspNet.Mvc.Description
             var parameterDescriptor = new ParameterDescriptor
             {
                 Name = "id",
-                IsOptional = isDescriptorParameterOptional,
                 ParameterType = typeof(int),
             };
             action.Parameters = new List<ParameterDescriptor> { parameterDescriptor };

test/Microsoft.AspNet.Mvc.FunctionalTests/ApiExplorerTest.cs

@@ -324,7 +324,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
             Assert.Equal("RangeRouteConstraint", Assert.Single(month.ConstraintTypes));
 
             var day = Assert.Single(description.ParameterDescriptions, p => p.Name == "day");
-            Assert.False(day.IsOptional);
+            Assert.True(day.IsOptional);
             Assert.Equal("Path", day.Source);
             Assert.Equal("IntRouteConstraint", Assert.Single(day.ConstraintTypes));
 

test/Microsoft.AspNet.Mvc.FunctionalTests/ApplicationModelTest.cs

@@ -57,13 +57,13 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
             var client = server.CreateClient();
 
             // Act
-            var response = await client.GetAsync("http://localhost/ParameterModel/GetParameterIsOptional");
+            var response = await client.GetAsync("http://localhost/ParameterModel/GetParameterMetadata");
 
             // Assert
             Assert.Equal(HttpStatusCode.OK, response.StatusCode);
 
             var body = await response.Content.ReadAsStringAsync();
-            Assert.Equal("True", body);
+            Assert.Equal("CoolMetadata", body);
 
         }
     }

test/Microsoft.AspNet.Mvc.WebApiCompatShimTest/ApiControllerActionDiscoveryTest.cs

@@ -277,7 +277,8 @@ namespace System.Web.Http
             foreach (var action in actions)
             {
                 var parameter = Assert.Single(action.Parameters);
-                Assert.IsType<FromUriAttribute>(parameter.BinderMetadata);
+                var metadata = Assert.IsType<FromUriAttribute>(parameter.BinderMetadata);
+                Assert.False(metadata.IsOptional);
             }
         }
 
@@ -335,6 +336,36 @@ namespace System.Web.Http
             }
         }
 
+        [Theory]
+        [InlineData(nameof(TestControllers.EventsController.GetWithId))]
+        [InlineData(nameof(TestControllers.EventsController.GetWithEmployee))]
+        public void GetActions_Parameters_ImplicitOptional(string name)
+        {
+            // Arrange
+            var provider = CreateProvider();
+
+            // Act
+            var context = new ActionDescriptorProviderContext();
+            provider.Invoke(context);
+
+            var results = context.Results.Cast<ControllerActionDescriptor>();
+
+            // Assert
+            var controllerType = typeof(TestControllers.EventsController).GetTypeInfo();
+            var actions = results
+                .Where(ad => ad.ControllerTypeInfo == controllerType)
+                .Where(ad => ad.Name == name)
+                .ToArray();
+
+            Assert.NotEmpty(actions);
+            foreach (var action in actions)
+            {
+                var parameter = Assert.Single(action.Parameters);
+                var metadata = Assert.IsType<FromUriAttribute>(parameter.BinderMetadata);
+                Assert.True(metadata.IsOptional);
+            }
+        }
+
         private INestedProviderManager<ActionDescriptorProviderContext> CreateProvider()
         {
             var assemblyProvider = new Mock<IAssemblyProvider>();
@@ -455,5 +486,18 @@ namespace System.Web.Http.TestControllers
     public class Employee
     {
     }
+
+    public class EventsController : ApiController
+    {
+        public IActionResult GetWithId(int id = 0)
+        {
+            return null;
+        }
+
+        public IActionResult GetWithEmployee([FromUri] Employee e = null)
+        {
+            return null;
+        }
+    }
 }
 #endif

test/Microsoft.AspNet.Mvc.WebApiCompatShimTest/OverloadActionConstraintTest.cs

@@ -165,9 +165,8 @@ namespace Microsoft.AspNet.Mvc.WebApiCompatShim
                 },
                 new ParameterDescriptor()
                 {
-                    BinderMetadata = new FromUriAttribute(),
+                    BinderMetadata = new FromUriAttribute() { IsOptional = true },
                     Name = "quantity",
-                    IsOptional = true,
                     ParameterType = typeof(int),
                 },
             };
@@ -307,9 +306,8 @@ namespace Microsoft.AspNet.Mvc.WebApiCompatShim
                 },
                 new ParameterDescriptor()
                 {
-                    BinderMetadata = new FromUriAttribute(),
+                    BinderMetadata = new FromUriAttribute() { IsOptional = true },
                     Name = "quantity",
-                    IsOptional = true,
                     ParameterType = typeof(int),
                 },
             };
@@ -430,8 +428,7 @@ namespace Microsoft.AspNet.Mvc.WebApiCompatShim
                 },
                 new ParameterDescriptor()
                 {
-                    BinderMetadata = new FromUriAttribute(),
+                    BinderMetadata = new FromUriAttribute() { IsOptional = true },
-                    IsOptional = true,
                     Name = "quantity",
                     ParameterType = typeof(int),
                 },

test/WebSites/ApplicationModelWebSite/Controllers/ParameterModelController.cs

@@ -4,25 +4,31 @@
 using System;
 using Microsoft.AspNet.Mvc;
 using Microsoft.AspNet.Mvc.ApplicationModels;
+using Microsoft.AspNet.Mvc.ModelBinding;
 
 namespace ApplicationModelWebSite
 {
-    // This controller uses an reflected model attribute to change a parameter to optional.
+    // This controller uses an reflected model attribute to change a parameter's binder metadata.
+    //
+    // This could be accomplished by simply making an attribute that implements IBinderMetadata, but
+    // this is part of a test for IParameterModelConvention.
     public class ParameterModelController : Controller
     {
-        public string GetParameterIsOptional([Optional] int? id)
+        public string GetParameterMetadata([Cool] int? id)
         {
-            var actionDescriptor = (ControllerActionDescriptor)ActionContext.ActionDescriptor;
+            return ActionContext.ActionDescriptor.Parameters[0].BinderMetadata.GetType().Name;
-
-            return actionDescriptor.Parameters[0].IsOptional.ToString();
         }
 
-        private class OptionalAttribute : Attribute, IParameterModelConvention
+        private class CoolAttribute : Attribute, IParameterModelConvention
         {
             public void Apply(ParameterModel model)
             {
-                model.IsOptional = true;
+                model.BinderMetadata = new CoolMetadata();
             }
         }
+
+        private class CoolMetadata : IBinderMetadata
+        {
+        }
     }
 }