diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ApiBehaviorOptions.cs b/src/Microsoft.AspNetCore.Mvc.Core/ApiBehaviorOptions.cs index b2b2c2cd6f..b597eaba40 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ApiBehaviorOptions.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ApiBehaviorOptions.cs @@ -17,6 +17,7 @@ public class ApiBehaviorOptions : IEnumerable { private readonly CompatibilitySwitch _suppressMapClientErrors; private readonly CompatibilitySwitch _suppressUseValidationProblemDetailsForInvalidModelStateResponses; + private readonly CompatibilitySwitch _allowInferringBinderModelNameWhenBindingInfoIsSpecified; private readonly ICompatibilitySwitch[] _switches; private Func _invalidModelStateResponseFactory; @@ -28,10 +29,12 @@ public ApiBehaviorOptions() { _suppressMapClientErrors = new CompatibilitySwitch(nameof(SuppressMapClientErrors)); _suppressUseValidationProblemDetailsForInvalidModelStateResponses = new CompatibilitySwitch(nameof(SuppressUseValidationProblemDetailsForInvalidModelStateResponses)); + _allowInferringBinderModelNameWhenBindingInfoIsSpecified = new CompatibilitySwitch(nameof(AllowInferringBinderModelNameWhenBindingInfoIsSpecified)); _switches = new[] { _suppressMapClientErrors, _suppressUseValidationProblemDetailsForInvalidModelStateResponses, + _allowInferringBinderModelNameWhenBindingInfoIsSpecified }; } @@ -151,6 +154,49 @@ public bool SuppressUseValidationProblemDetailsForInvalidModelStateResponses set => _suppressUseValidationProblemDetailsForInvalidModelStateResponses.Value = value; } + /// + /// Gets or sets a value that determines if is inferred when the parameter or property + /// specifies any binding-specific value. + /// + /// is inferred for for bound properties and parameters on controllers + /// with for complex () types with greedy + /// () binding sources. + /// + /// When the is not inferred, + /// if the parameter or property is annotated with any model binding specific attribute + /// such as , or etc., or the associated + /// specifies a value for the . + /// + /// + /// + /// The default value is if the version is + /// or later; otherwise. + /// + /// + /// + /// This property is associated with a compatibility switch and can provide a different behavior depending on + /// the configured compatibility version for the application. See for + /// guidance and examples of setting the application's compatibility version. + /// + /// + /// Configuring the desired value of the compatibility switch by calling this property's setter will take + /// precedence over the value implied by the application's . + /// + /// + /// If the application's compatibility version is set to or + /// lower then this setting will have the value unless explicitly configured. + /// + /// + /// If the application's compatibility version is set to or + /// higher then this setting will have the value unless explicitly configured. + /// + /// + public bool AllowInferringBinderModelNameWhenBindingInfoIsSpecified + { + get => _allowInferringBinderModelNameWhenBindingInfoIsSpecified.Value; + set => _allowInferringBinderModelNameWhenBindingInfoIsSpecified.Value = value; + } + /// /// Gets a map of HTTP status codes to . Configured values /// are used to transform to an diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/ApiBehaviorApplicationModelProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/ApiBehaviorApplicationModelProvider.cs index 62831919de..5c112bf1d0 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/ApiBehaviorApplicationModelProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/ApiBehaviorApplicationModelProvider.cs @@ -50,7 +50,8 @@ internal class ApiBehaviorApplicationModelProvider : IApplicationModelProvider var inferParameterBindingInfoConvention = new InferParameterBindingInfoConvention(modelMetadataProvider) { - SuppressInferBindingSourcesForParameters = options.SuppressInferBindingSourcesForParameters + SuppressInferBindingSourcesForParameters = options.SuppressInferBindingSourcesForParameters, + AllowInferringBinderModelNameWhenBindingInfoIsSpecified = options.AllowInferringBinderModelNameWhenBindingInfoIsSpecified, }; ControllerModelConventions = new List { diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/InferParameterBindingInfoConvention.cs b/src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/InferParameterBindingInfoConvention.cs index 8861263faf..5163f14319 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/InferParameterBindingInfoConvention.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/InferParameterBindingInfoConvention.cs @@ -34,6 +34,8 @@ public class InferParameterBindingInfoConvention : IControllerModelConvention /// public bool SuppressInferBindingSourcesForParameters { get; set; } + internal bool AllowInferringBinderModelNameWhenBindingInfoIsSpecified { get; set; } + protected virtual bool ShouldApply(ControllerModel controller) => true; public void Apply(ControllerModel controller) @@ -49,7 +51,6 @@ public void Apply(ControllerModel controller) } InferBoundPropertyModelPrefixes(controller); - foreach (var action in controller.Actions) { InferParameterBindingSources(action); @@ -59,18 +60,56 @@ public void Apply(ControllerModel controller) internal void InferParameterBindingSources(ActionModel action) { + if (SuppressInferBindingSourcesForParameters) + { + // Do nothing. + return; + } + var inferredBindingSources = new BindingSource[action.Parameters.Count]; for (var i = 0; i < action.Parameters.Count; i++) { var parameter = action.Parameters[i]; - var bindingSource = parameter.BindingInfo?.BindingSource; + + var userSpecifiedBindingInfo = parameter.BindingInfo; + if (userSpecifiedBindingInfo?.BindingSource != null) + { + continue; + } + + var bindingSource = InferBindingSourceForParameter(parameter); + if (bindingSource == null) { - bindingSource = InferBindingSourceForParameter(parameter); + continue; + } - parameter.BindingInfo = parameter.BindingInfo ?? new BindingInfo(); - parameter.BindingInfo.BindingSource = bindingSource; + parameter.BindingInfo = parameter.BindingInfo ?? new BindingInfo(); + parameter.BindingInfo.BindingSource = bindingSource; + + if (AllowInferringBinderModelNameWhenBindingInfoIsSpecified) + { + // Using legacy compat behavior. BinderModelName will be inferred eventually. + return; + } + + // For any complex types that are bound from value providers, set the BinderModelName + // to an empty string. This makes binding much more predictable + // and documentable via ApiExplorer. + // Only do this if the user hasn't explicitly specified any binding information for the parameter. + if (userSpecifiedBindingInfo != null) + { + continue; + } + + var bindingInfo = parameter.BindingInfo; + if (bindingInfo.BinderModelName == null && + bindingSource.IsGreedy && + !IsFormFile(parameter.ParameterType) && + IsComplexTypeParameter(parameter)) + { + parameter.BindingInfo.BinderModelName = string.Empty; } } @@ -109,6 +148,11 @@ internal BindingSource InferBindingSourceForParameter(ParameterModel parameter) // and describable via ApiExplorer internal void InferBoundPropertyModelPrefixes(ControllerModel controllerModel) { + if (!AllowInferringBinderModelNameWhenBindingInfoIsSpecified) + { + return; + } + foreach (var property in controllerModel.ControllerProperties) { if (property.BindingInfo != null && @@ -130,6 +174,11 @@ internal void InferBoundPropertyModelPrefixes(ControllerModel controllerModel) internal void InferParameterModelPrefixes(ActionModel action) { + if (!AllowInferringBinderModelNameWhenBindingInfoIsSpecified) + { + return; + } + foreach (var parameter in action.Parameters) { var bindingInfo = parameter.BindingInfo; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorOptionsSetup.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorOptionsSetup.cs index bddaaa2478..39ade20156 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorOptionsSetup.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorOptionsSetup.cs @@ -35,6 +35,7 @@ public class ApiBehaviorOptionsSetup : { dictionary[nameof(ApiBehaviorOptions.SuppressMapClientErrors)] = true; dictionary[nameof(ApiBehaviorOptions.SuppressUseValidationProblemDetailsForInvalidModelStateResponses)] = true; + dictionary[nameof(ApiBehaviorOptions.AllowInferringBinderModelNameWhenBindingInfoIsSpecified)] = true; } return dictionary; diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ApplicationModels/InferParameterBindingInfoConventionTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ApplicationModels/InferParameterBindingInfoConventionTest.cs index 26da271e36..2add972c0c 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ApplicationModels/InferParameterBindingInfoConventionTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ApplicationModels/InferParameterBindingInfoConventionTest.cs @@ -156,6 +156,150 @@ public void OnProvidersExecuting_PreservesBindingInfo_WhenInferringFor_Parameter Assert.Equal("foo", bindingInfo.BinderModelName); } + [Fact] + public void Apply_InfersBindingSourceAndBinderModelName() + { + // Arrange + var actionName = nameof(TestController.PutModel); + var convention = GetConvention(); + var controller = GetControllerModel(typeof(TestController)); + + // Act + convention.Apply(controller); + + // Assert + var action = Assert.Single(controller.Actions, a => a.ActionName == actionName); + + Assert.Collection( + action.Parameters, + parameter => + { + Assert.Equal("id", parameter.Name); + var bindingInfo = parameter.BindingInfo; + Assert.NotNull(bindingInfo); + Assert.Same(BindingSource.Path, bindingInfo.BindingSource); + + Assert.Null(bindingInfo.BinderModelName); + }, + parameter => + { + Assert.Equal("model", parameter.Name); + var bindingInfo = parameter.BindingInfo; + Assert.NotNull(bindingInfo); + Assert.Same(BindingSource.Body, bindingInfo.BindingSource); + + Assert.Empty(bindingInfo.BinderModelName); + }); + } + + [Fact] + public void Apply_DoesNotInferBindingSource_IfSuppressInferBindingSourcesForParametersIsSet() + { + // Arrange + var actionName = nameof(TestController.PutModel); + var convention = GetConvention(); + convention.SuppressInferBindingSourcesForParameters = true; + var controller = GetControllerModel(typeof(TestController)); + + // Act + convention.Apply(controller); + + // Assert + var action = Assert.Single(controller.Actions, a => a.ActionName == actionName); + + Assert.Collection( + action.Parameters, + parameter => + { + Assert.Equal("id", parameter.Name); + var bindingInfo = parameter.BindingInfo; + Assert.Null(bindingInfo); + }, + parameter => + { + Assert.Equal("model", parameter.Name); + var bindingInfo = parameter.BindingInfo; + Assert.Null(bindingInfo); + }); + } + + [Fact] + public void Apply_DoesNotInferParameterPrefix_IfSuppressInferBindingSourcesForParametersIsSet() + { + // Arrange + var actionName = nameof(TestController.GetByCoordinates); + var convention = GetConvention(); + convention.SuppressInferBindingSourcesForParameters = true; + var controller = GetControllerModel(typeof(TestController)); + + // Act + convention.Apply(controller); + + // Assert + var action = Assert.Single(controller.Actions, a => a.ActionName == actionName); + + var parameter = Assert.Single(action.Parameters); + var bindingInfo = parameter.BindingInfo; + Assert.NotNull(bindingInfo); + Assert.Equal(BindingSource.Query, bindingInfo.BindingSource); + Assert.Null(bindingInfo.BinderModelName); + } + + [Fact] + public void Apply_InferParameterPrefix_IfAllowInferringBinderModelNameWhenBindingInfoIsSpecifiedIsSet() + { + // Arrange + var actionName = nameof(TestController.GetByCoordinates); + var convention = GetConvention(); + convention.SuppressInferBindingSourcesForParameters = true; + convention.AllowInferringBinderModelNameWhenBindingInfoIsSpecified = true; + var controller = GetControllerModel(typeof(TestController)); + + // Act + convention.Apply(controller); + + // Assert + var action = Assert.Single(controller.Actions, a => a.ActionName == actionName); + + var parameter = Assert.Single(action.Parameters); + var bindingInfo = parameter.BindingInfo; + Assert.NotNull(bindingInfo); + Assert.Equal(BindingSource.Query, bindingInfo.BindingSource); + Assert.Empty(bindingInfo.BinderModelName); + } + + [Fact] + public void Apply_InfersPropertyPrefix_IfAllowInferringBinderModelNameWhenBindingInfoIsSpecifiedIsSet() + { + // Arrange + var convention = GetConvention(); + convention.AllowInferringBinderModelNameWhenBindingInfoIsSpecified = true; + var controller = GetControllerModel(typeof(ControllerWithBoundProperty)); + + // Act + convention.Apply(controller); + + // Assert + Assert.Collection( + controller.ControllerProperties.OrderBy(p => p.Name), + property => + { + Assert.Equal(nameof(ControllerWithBoundProperty.Files), property.Name); + var bindingInfo = property.BindingInfo; + Assert.NotNull(bindingInfo); + Assert.Equal(BindingSource.Form, bindingInfo.BindingSource); + Assert.Null(bindingInfo.BinderModelName); // BindingSource is not greedy, BinderModelName should not be inferred. + }, + property => + { + Assert.Equal(nameof(ControllerWithBoundProperty.TestProperty), property.Name); + var bindingInfo = property.BindingInfo; + Assert.NotNull(bindingInfo); + Assert.Equal(BindingSource.Query, bindingInfo.BindingSource); + Assert.Empty(bindingInfo.BinderModelName); // BindingSource is greedy, BinderModelName should be inferred. + }); + } + [Fact] public void InferBindingSourceForParameter_ReturnsPath_IfParameterNameExistsInRouteAsSimpleToken() { @@ -681,6 +825,7 @@ public void InferBoundPropertyModelPrefixes_SetsModelPrefix_ForComplexTypeFromVa // Arrange var controller = GetControllerModel(typeof(ControllerWithBoundProperty)); var convention = GetConvention(); + convention.AllowInferringBinderModelNameWhenBindingInfoIsSpecified = true; // Act convention.InferBoundPropertyModelPrefixes(controller); @@ -711,6 +856,7 @@ public void InferParameterModelPrefixes_SetsModelPrefix_ForComplexTypeFromValueP // Arrange var action = GetActionModel(typeof(ControllerWithBoundProperty), nameof(ControllerWithBoundProperty.SomeAction)); var convention = GetConvention(); + convention.AllowInferringBinderModelNameWhenBindingInfoIsSpecified = true; // Act convention.InferParameterModelPrefixes(action); @@ -725,7 +871,7 @@ public void InferParameterModelPrefixes_DoesNotSetModelPrefix_ForFormFileParamet { // Arrange var action = GetActionModel( - typeof(ParameterBindingController), + typeof(ParameterBindingController), nameof(ParameterBindingController.FromFormFormFileParameters), TestModelMetadataProvider.CreateDefaultProvider()); var convention = GetConvention(); @@ -758,8 +904,8 @@ public void InferParameterModelPrefixes_DoesNotSetModelPrefix_ForFormFileParamet { // Arrange var action = GetActionModel( - typeof(ParameterBindingController), - nameof(ParameterBindingController.FormFileParameters), + typeof(ParameterBindingController), + nameof(ParameterBindingController.FormFileParameters), TestModelMetadataProvider.CreateDefaultProvider()); var convention = GetConvention(); @@ -1070,5 +1216,14 @@ private class ParameterWithBindingInfo [HttpGet("test")] public IActionResult Action([ModelBinder(typeof(object))] Car car) => null; } + + private class TestController + { + [HttpPut("{id}")] + public IActionResult PutModel(int id, TestModel model) => null; + + [HttpPut("{id}")] + public IActionResult GetByCoordinates([FromQuery] GpsCoordinates coordinates) => null; + } } } diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiBehaviorTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiBehaviorTest.cs index c30dd30750..920362dd57 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiBehaviorTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiBehaviorTest.cs @@ -8,8 +8,10 @@ using System.Net.Http; using System.Text; using System.Threading.Tasks; +using BasicWebSite; using BasicWebSite.Models; using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Mvc.Testing; using Newtonsoft.Json; using Newtonsoft.Json.Linq; using Xunit; @@ -20,14 +22,12 @@ public class ApiBehaviorTest : IClassFixture fixture) { - Client = fixture.CreateDefaultClient(); - - var factory = fixture.WithWebHostBuilder(ConfigureWebHostBuilder); - CustomInvalidModelStateClient = factory.CreateDefaultClient(); + Factory = fixture.Factories.FirstOrDefault() ?? fixture.WithWebHostBuilder(builder => builder.UseStartup()); + Client = Factory.CreateDefaultClient(); + CustomInvalidModelStateClient = Factory.WithWebHostBuilder(builder => builder.UseStartup()).CreateDefaultClient(); } - private static void ConfigureWebHostBuilder(IWebHostBuilder builder) => - builder.UseStartup(); + public WebApplicationFactory Factory { get; } public HttpClient Client { get; } public HttpClient CustomInvalidModelStateClient { get; } @@ -212,7 +212,7 @@ public async Task ActionsWithApiBehavior_InferEmptyPrefixForComplexValueProvider } [Fact] - public async Task ActionsWithApiBehavior_InferEmptyPrefixForComplexValueProviderModel_Ignored() + public async Task ActionsWithApiBehavior_DoNotInferPrefix_WhenExplicitBindingInfoIsSpecified() { // Arrange var id = 31; @@ -226,6 +226,28 @@ public async Task ActionsWithApiBehavior_InferEmptyPrefixForComplexValueProvider // Assert Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var result = await response.Content.ReadAsAsync(); + Assert.Equal(31, result.ContactId); + Assert.Equal(name, result.Name); + Assert.Equal(email, result.Email); + } + + [Fact] + public async Task ActionsWithApiBehavior_With21CompatibilityBehavior_InferEmptyPrefixForComplexValueProviderModel_Ignored() + { + // Arrange + var id = 31; + var name = "test_user"; + var email = "email@test.com"; + var url = $"/contact/ActionWithInferredEmptyPrefix?contact.name={name}&contact.contactid={id}&contact.email={email}"; + var client = Factory.WithWebHostBuilder(builder => builder.UseStartup()).CreateClient(); + + // Act + var response = await client.GetAsync(url); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var result = await response.Content.ReadAsAsync(); Assert.Equal(0, result.ContactId); Assert.Null(result.Name); diff --git a/test/WebSites/BasicWebSite/Startup.cs b/test/WebSites/BasicWebSite/Startup.cs index 4cd7646bfb..86dddf830c 100644 --- a/test/WebSites/BasicWebSite/Startup.cs +++ b/test/WebSites/BasicWebSite/Startup.cs @@ -13,6 +13,8 @@ namespace BasicWebSite { public class Startup { + public virtual CompatibilityVersion CompatibilityVersion => CompatibilityVersion.Latest; + // Set up application services public void ConfigureServices(IServiceCollection services) { @@ -30,7 +32,7 @@ public void ConfigureServices(IServiceCollection services) // Remove when all URL generation tests are passing - https://github.com/aspnet/Routing/issues/590 options.EnableEndpointRouting = false; }) - .SetCompatibilityVersion(CompatibilityVersion.Latest) + .SetCompatibilityVersion(CompatibilityVersion) .AddXmlDataContractSerializerFormatters(); services.ConfigureBaseWebSiteAuthPolicies(); diff --git a/test/WebSites/BasicWebSite/StartupWith21CompatibilityBehavior.cs b/test/WebSites/BasicWebSite/StartupWith21CompatibilityBehavior.cs new file mode 100644 index 0000000000..5d7db6f32b --- /dev/null +++ b/test/WebSites/BasicWebSite/StartupWith21CompatibilityBehavior.cs @@ -0,0 +1,12 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.AspNetCore.Mvc; + +namespace BasicWebSite +{ + public class StartupWith21CompatibilityBehavior : Startup + { + public override CompatibilityVersion CompatibilityVersion => CompatibilityVersion.Version_2_1; + } +}