diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/ParameterModel.cs b/src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/ParameterModel.cs index 970a026a8a..f473ab94f4 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/ParameterModel.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/ParameterModel.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.Diagnostics; using System.Reflection; +using Microsoft.Extensions.Internal; namespace Microsoft.AspNetCore.Mvc.ApplicationModels { @@ -46,5 +47,14 @@ namespace Microsoft.AspNetCore.Mvc.ApplicationModels get => Name; set => Name = value; } + + public string DisplayName + { + get + { + var parameterTypeName = TypeNameHelper.GetTypeDisplayName(ParameterInfo.ParameterType, fullName: false); + return $"{parameterTypeName} {ParameterName}"; + } + } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs index adc39963a7..57c65a6d32 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs @@ -147,44 +147,34 @@ namespace Microsoft.AspNetCore.Mvc.Internal return; } var inferredBindingSources = new BindingSource[actionModel.Parameters.Count]; - var foundFromBodyParameter = false; - for (var i = 0; i < inferredBindingSources.Length; i++) + for (var i = 0; i < actionModel.Parameters.Count; i++) { var parameter = actionModel.Parameters[i]; var bindingSource = parameter.BindingInfo?.BindingSource; if (bindingSource == null) { bindingSource = InferBindingSourceForParameter(parameter); - } - - if (bindingSource == BindingSource.Body) - { - if (foundFromBodyParameter) - { - // More than one parameter is inferred as FromBody. Log a warning and skip this action. - _logger.UnableToInferBindingSource(actionModel); - } - else - { - foundFromBodyParameter = true; - } - } - - inferredBindingSources[i] = bindingSource; - } - - for (var i = 0; i < inferredBindingSources.Length; i++) - { - var bindingSource = inferredBindingSources[i]; - if (bindingSource != null) - { - actionModel.Parameters[i].BindingInfo = new BindingInfo + parameter.BindingInfo = new BindingInfo { BindingSource = bindingSource, }; } } + + var fromBodyParameters = actionModel.Parameters.Where(p => p.BindingInfo.BindingSource == BindingSource.Body).ToList(); + if (fromBodyParameters.Count > 1) + { + var parameters = string.Join(Environment.NewLine, fromBodyParameters.Select(p => p.DisplayName)); + var message = Resources.FormatApiController_MultipleBodyParametersFound( + actionModel.DisplayName, + nameof(FromQueryAttribute), + nameof(FromRouteAttribute), + nameof(FromBodyAttribute)); + + message += Environment.NewLine + parameters; + throw new InvalidOperationException(message); + } } // For any complex types that are bound from value providers, set the prefix diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreLoggerExtensions.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreLoggerExtensions.cs index 1dd72c189f..c840f785b1 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreLoggerExtensions.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreLoggerExtensions.cs @@ -102,7 +102,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal private static readonly Action _modelStateInvalidFilterExecuting; private static readonly Action _inferredParameterSource; - private static readonly Action _unableToInferParameterSources; private static readonly Action _registeredModelBinderProviders; private static readonly Action _foundNoValueForPropertyInRequest; private static readonly Action _foundNoValueForParameterInRequest; @@ -394,11 +393,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal 1, "Inferred binding source for '{ParameterName}` on `{ActionName}` as {BindingSource}."); - _unableToInferParameterSources = LoggerMessage.Define( - LogLevel.Warning, - 2, - "Unable to unambiguously infer binding sources for parameters on '{ActionName}'. More than one parameter may be inferred to bound from body."); - _unsupportedFormatFilterContentType = LoggerMessage.Define( LogLevel.Debug, 1, @@ -1124,16 +1118,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal } } - public static void UnableToInferBindingSource( - this ILogger logger, - ActionModel actionModel) - { - if (logger.IsEnabled(LogLevel.Warning)) - { - _unableToInferParameterSources(logger, actionModel.ActionMethod, null); - } - } - public static void IfMatchPreconditionFailed(this ILogger logger, EntityTagHeaderValue etag) { _ifMatchPreconditionFailed(logger, etag, null); diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs b/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs index ec92b11640..d60a46fe66 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs @@ -1452,6 +1452,20 @@ namespace Microsoft.AspNetCore.Mvc.Core internal static string FormatComplexTypeModelBinder_NoParameterlessConstructor_ForParameter(object p0, object p1) => string.Format(CultureInfo.CurrentCulture, GetString("ComplexTypeModelBinder_NoParameterlessConstructor_ForParameter"), p0, p1); + /// + /// Action '{0}' has more than one parameter that were specified or inferred as bound from request body. Only one parameter per action may be bound from body. Inspect the following parameters, and use '{1}' to specify query string bound, '{2}' to specify route bound, and '{3}' for parameters to be bound from body: + /// + internal static string ApiController_MultipleBodyParametersFound + { + get => GetString("ApiController_MultipleBodyParametersFound"); + } + + /// + /// Action '{0}' has more than one parameter that were specified or inferred as bound from request body. Only one parameter per action may be bound from body. Inspect the following parameters, and use '{1}' to specify query string bound, '{2}' to specify route bound, and '{3}' for parameters to be bound from body: + /// + internal static string FormatApiController_MultipleBodyParametersFound(object p0, object p1, object p2, object p3) + => string.Format(CultureInfo.CurrentCulture, GetString("ApiController_MultipleBodyParametersFound"), p0, p1, p2, p3); + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx b/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx index 671190495e..909febc6b4 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx +++ b/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx @@ -439,4 +439,7 @@ Could not create an instance of type '{0}'. Model bound complex types must not be abstract or value types and must have a parameterless constructor. Alternatively, give the '{1}' parameter a non-null default value. + + Action '{0}' has more than one parameter that was specified or inferred as bound from request body. Only one parameter per action may be bound from body. Inspect the following parameters, and use '{1}' to specify bound from query, '{2}' to specify bound from route, and '{3}' for parameters to be bound from body: + \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ApplicationModel/ParameterModelTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ApplicationModel/ParameterModelTest.cs index 650e1c068b..f89323a4ef 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ApplicationModel/ParameterModelTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ApplicationModel/ParameterModelTest.cs @@ -64,6 +64,11 @@ namespace Microsoft.AspNetCore.Mvc.ApplicationModels // Ensure non-default value Assert.NotEqual(value1, Activator.CreateInstance(property.PropertyType)); } + else if (property.Name.Equals(nameof(ActionModel.DisplayName))) + { + // DisplayName is re-calculated, hence reference equality wouldn't work. + Assert.Equal(value1, value2); + } else { Assert.Same(value1, value2); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs index 1fae855ed9..5ce29079a1 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs @@ -384,6 +384,101 @@ namespace Microsoft.AspNetCore.Mvc.Internal Assert.Same(BindingSource.Body, result); } + [Fact] + public void OnProvidersExecuting_DoesNotInferBindingSourceForParametersWithBindingInfo() + { + // Arrange + var actionName = nameof(ParameterWithBindingInfo.Action); + var provider = GetProvider(); + var context = GetContext(typeof(ParameterWithBindingInfo)); + + // Act + provider.OnProvidersExecuting(context); + + // Assert + var controllerModel = Assert.Single(context.Result.Controllers); + var actionModel = Assert.Single(controllerModel.Actions, a => a.ActionName == actionName); + var parameterModel = Assert.Single(actionModel.Parameters); + Assert.NotNull(parameterModel.BindingInfo); + Assert.Same(BindingSource.Custom, parameterModel.BindingInfo.BindingSource); + } + + [Fact] + public void OnProvidersExecuting_Throws_IfMultipleParametersAreInferredAsBodyBound() + { + // Arrange + var expected = +$@"Action '{typeof(ControllerWithMultipleInferredFromBodyParameters).FullName}.{nameof(ControllerWithMultipleInferredFromBodyParameters.Action)} ({typeof(ControllerWithMultipleInferredFromBodyParameters).Assembly.GetName().Name})' " + +"has more than one parameter that was specified or inferred as bound from request body. Only one parameter per action may be bound from body. Inspect the following parameters, and use 'FromQueryAttribute' to specify bound from query, 'FromRouteAttribute' to specify bound from route, and 'FromBodyAttribute' for parameters to be bound from body:" + +Environment.NewLine + "TestModel a" + +Environment.NewLine + "Car b"; + + var context = GetContext(typeof(ControllerWithMultipleInferredFromBodyParameters)); + var provider = GetProvider(); + + // Act & Assert + var ex = Assert.Throws(() => provider.OnProvidersExecuting(context)); + Assert.Equal(expected, ex.Message); + } + + [Fact] + public void OnProvidersExecuting_Throws_IfMultipleParametersAreInferredOrSpecifiedAsBodyBound() + { + // Arrange + var expected = +$@"Action '{typeof(ControllerWithMultipleInferredOrSpecifiedFromBodyParameters).FullName}.{nameof(ControllerWithMultipleInferredOrSpecifiedFromBodyParameters.Action)} ({typeof(ControllerWithMultipleInferredOrSpecifiedFromBodyParameters).Assembly.GetName().Name})' " + +"has more than one parameter that was specified or inferred as bound from request body. Only one parameter per action may be bound from body. Inspect the following parameters, and use 'FromQueryAttribute' to specify bound from query, 'FromRouteAttribute' to specify bound from route, and 'FromBodyAttribute' for parameters to be bound from body:" + +Environment.NewLine + "TestModel a" + +Environment.NewLine + "int b"; + + var context = GetContext(typeof(ControllerWithMultipleInferredOrSpecifiedFromBodyParameters)); + var provider = GetProvider(); + + // Act & Assert + var ex = Assert.Throws(() => provider.OnProvidersExecuting(context)); + Assert.Equal(expected, ex.Message); + } + + [Fact] + public void OnProvidersExecuting_Throws_IfMultipleParametersAreFromBody() + { + // Arrange + var expected = +$@"Action '{typeof(ControllerWithMultipleFromBodyParameters).FullName}.{nameof(ControllerWithMultipleFromBodyParameters.Action)} ({typeof(ControllerWithMultipleFromBodyParameters).Assembly.GetName().Name})' " + +"has more than one parameter that was specified or inferred as bound from request body. Only one parameter per action may be bound from body. Inspect the following parameters, and use 'FromQueryAttribute' to specify bound from query, 'FromRouteAttribute' to specify bound from route, and 'FromBodyAttribute' for parameters to be bound from body:" + +Environment.NewLine + "decimal a" + +Environment.NewLine + "int b"; + + var context = GetContext(typeof(ControllerWithMultipleFromBodyParameters)); + var provider = GetProvider(); + + // Act & Assert + var ex = Assert.Throws(() => provider.OnProvidersExecuting(context)); + Assert.Equal(expected, ex.Message); + } + + [Fact(Skip = "https://github.com/aspnet/Mvc/issues/7609")] + public void OnProvidersExecuting_PreservesBindingInfo_WhenInferringBindingSource() + { + // Arrange + var actionName = nameof(ParameterBindingController.ModelBinderAttribute); + var context = GetContext(typeof(ParameterBindingController)); + var provider = GetProvider(); + + // Act + provider.OnProvidersExecuting(context); + + // Assert + var controller = Assert.Single(context.Result.Controllers); + var action = Assert.Single(controller.Actions, a => a.ActionName == actionName); + var parameter = Assert.Single(action.Parameters); + + var bindingInfo = parameter.BindingInfo; + Assert.NotNull(bindingInfo); + Assert.Same(BindingSource.Query, bindingInfo.BindingSource); + Assert.Equal("top", bindingInfo.BinderModelName); + } + [Fact] public void InferParameterBindingSources_SetsCorrectBindingSourceForComplexTypesWithCancellationToken() { @@ -651,6 +746,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal [HttpPut("cancellation")] public IActionResult ComplexTypeModelWithCancellationToken(TestModel model, CancellationToken cancellationToken) => null; + + [HttpGet("parameter-with-model-binder-attribute")] + public IActionResult ModelBinderAttribute([ModelBinder(Name = "top")] int value) => null; } [ApiController] @@ -709,5 +807,35 @@ namespace Microsoft.AspNetCore.Mvc.Internal public IActionResult SomeAction([FromQuery] TestModel test) => null; } + + private class Car { } + + [ApiController] + private class ControllerWithMultipleInferredFromBodyParameters + { + [HttpGet("test")] + public IActionResult Action(TestModel a, Car b) => null; + } + + [ApiController] + private class ControllerWithMultipleInferredOrSpecifiedFromBodyParameters + { + [HttpGet("test")] + public IActionResult Action(TestModel a, [FromBody] int b) => null; + } + + [ApiController] + private class ControllerWithMultipleFromBodyParameters + { + [HttpGet("test")] + public IActionResult Action([FromBody] decimal a, [FromBody] int b) => null; + } + + [ApiController] + private class ParameterWithBindingInfo + { + [HttpGet("test")] + public IActionResult Action([ModelBinder(typeof(object))] Car car) => null; + } } } diff --git a/test/WebSites/BasicWebSite/Startup.cs b/test/WebSites/BasicWebSite/Startup.cs index 784a29f3e5..9d4db8b9da 100644 --- a/test/WebSites/BasicWebSite/Startup.cs +++ b/test/WebSites/BasicWebSite/Startup.cs @@ -28,6 +28,7 @@ namespace BasicWebSite // Filter that records a value in HttpContext.Items options.Filters.Add(new TraceResourceFilter()); }) + .SetCompatibilityVersion(CompatibilityVersion.Latest) .AddXmlDataContractSerializerFormatters(); services.Configure(options =>