From deed6c157fbfbb38350a1007d158a96c83203113 Mon Sep 17 00:00:00 2001 From: Sami Daniel Date: Tue, 24 Jun 2025 17:37:27 -0300 Subject: [PATCH] Optimize prefixed parameters when assembling model name. Added an method to allow optimize model names where the model name is the same as one of its properties when binding the model name. This should only be used when this is the desired behavior. Tests have been added to assert this behavior when passing parameters whose model name is the same as the property to be bound. This was introduced to allow circumventing the bug where the parameter name is the same as the name of one of its properties, causing the model to trick the PrefixContainer into considering that parameter as prefixed (model.property), even when it is not. ```C# class SomeClass { public string Parameter { get; set; } = ""; } ``` and later we define the model name as parameter (e.g in a ActionMethod for example) ```C# public IActionResult SomeEndpoint([FromQuery] SomeClass parameter) { /* elided */ } ``` internally, when we pass the query "?parameter=somecoolparametervalue", it will classify it as prefixed (parameter.Parameter instead of only parameter), because of the internal logic of PrefixContainer. However, it is not prefixed, so later on when it try to bind the key (the keys from the query, in this case parameter) with the internal keys (wrong classified as parameter.Parameter) the ModelBinder would not find an valid candidate for matching with the key. Now the optimization option allows to treat the modelName and the propertyName with only one if both are equal. --- .../Binders/ComplexObjectModelBinder.cs | 2 +- .../Mvc.Core/src/ModelBinding/ModelNames.cs | 40 +++++++++++ src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt | 1 + .../CollectionModelBinderIntegrationTest.cs | 68 +++++++++++++++++++ 4 files changed, 110 insertions(+), 1 deletion(-) diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexObjectModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexObjectModelBinder.cs index eadaa965ded5..e450f69eb275 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexObjectModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexObjectModelBinder.cs @@ -364,7 +364,7 @@ internal void CreateModel(ModelBindingContext bindingContext) } var fieldName = property.BinderModelName ?? property.PropertyName!; - var modelName = ModelNames.CreatePropertyModelName(bindingContext.ModelName, fieldName); + var modelName = ModelNames.CreatePropertyModelNameOptimized(bindingContext.ModelName, fieldName); var result = await BindPropertyAsync(bindingContext, property, propertyBinder, fieldName, modelName); if (result.IsModelSet) diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs b/src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs index 8e6afac46e68..c9d2f3cdffd1 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs @@ -61,4 +61,44 @@ public static string CreatePropertyModelName(string? prefix, string? propertyNam return prefix + "." + propertyName; } + + /// + /// Create a model property name using a prefix and a property name, + /// with a small optimization to avoid redundancy. + /// + /// For example, if both and are "parameter" + /// (ignoring case), the result will be just "parameter" instead of "parameter.Parameter". + /// + /// The prefix to use. + /// The property name. + /// The property model name. + public static string CreatePropertyModelNameOptimized(string? prefix, string? propertyName) + { + if (string.IsNullOrEmpty(prefix)) + { + return propertyName ?? string.Empty; + } + + if (string.IsNullOrEmpty(propertyName)) + { + return prefix ?? string.Empty; + } + + if (propertyName.StartsWith('[')) + { + // The propertyName might represent an indexer access, in which case combining + // with a 'dot' would be invalid. This case occurs only when called from ValidationVisitor. + return prefix + propertyName; + } + + if (string.Equals(prefix, propertyName, StringComparison.OrdinalIgnoreCase)) + { + // if we are dealing with with something like: + // prefix = parameter and propertyName = parameter + // it should fallback to the property name. + return propertyName; + } + + return prefix + "." + propertyName; + } } diff --git a/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt b/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt index 08c1f5142578..d8a202f54030 100644 --- a/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt +++ b/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt @@ -6,3 +6,4 @@ Microsoft.AspNetCore.Mvc.ProducesDefaultResponseTypeAttribute.Description.get -> Microsoft.AspNetCore.Mvc.ProducesDefaultResponseTypeAttribute.Description.set -> void Microsoft.AspNetCore.Mvc.ProducesResponseTypeAttribute.Description.get -> string? Microsoft.AspNetCore.Mvc.ProducesResponseTypeAttribute.Description.set -> void +static Microsoft.AspNetCore.Mvc.ModelBinding.ModelNames.CreatePropertyModelNameOptimized(string? prefix, string? propertyName) -> string! diff --git a/src/Mvc/test/Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs b/src/Mvc/test/Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs index 8bb72ff67055..591618788c9e 100644 --- a/src/Mvc/test/Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs +++ b/src/Mvc/test/Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs @@ -338,6 +338,74 @@ public async Task CollectionModelBinder_BindsListOfComplexType_WithRequiredPrope Assert.Equal("A value for the 'Name' parameter or property was not provided.", error.ErrorMessage); } + class TestClass + { + public TestClass2 Parameter { get; set; } + } + + class TestClass2 + { + public string Parameter { get; set; } = ""; + } + + [Fact(Skip = "Nested properties are more complex to deal with it. See https://github.com/dotnet/aspnetcore/pull/62459 for more info.")] + public async Task CollectionModelBinder_CanBind_NestedProperties_WithSameNameOfParameter() + { + // Arrange + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(); + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(TestClass) + }; + + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + request.QueryString = new QueryString("?parameter.parameter=testing"); + }); + + var modelState = testContext.ModelState; + + // Act + var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(modelBindingResult.IsModelSet); + + var model = Assert.IsType(modelBindingResult.Model); + Assert.Equal("testing", model.Parameter.Parameter); + Assert.True(modelState.IsValid); + } + + [Fact] + public async Task CollectionModelBinder_CanBind_SimpleProperties_WithSameNameOfParameter() + { + // Arrange + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(); + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(TestClass2) + }; + + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + request.QueryString = new QueryString("?parameter=testing"); + }); + + var modelState = testContext.ModelState; + + // Act + var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(modelBindingResult.IsModelSet); + + var model = Assert.IsType(modelBindingResult.Model); + Assert.Equal("testing", model.Parameter); + Assert.True(modelState.IsValid); + } + [Fact] public async Task CollectionModelBinder_BindsListOfComplexType_WithRequiredProperty_WithExplicitPrefix_PartialData() {