Skip to content

Commit deed6c1

Browse files
committed
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.
1 parent f2353d2 commit deed6c1

File tree

4 files changed

+110
-1
lines changed

4 files changed

+110
-1
lines changed

src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexObjectModelBinder.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -364,7 +364,7 @@ internal void CreateModel(ModelBindingContext bindingContext)
364364
}
365365

366366
var fieldName = property.BinderModelName ?? property.PropertyName!;
367-
var modelName = ModelNames.CreatePropertyModelName(bindingContext.ModelName, fieldName);
367+
var modelName = ModelNames.CreatePropertyModelNameOptimized(bindingContext.ModelName, fieldName);
368368
var result = await BindPropertyAsync(bindingContext, property, propertyBinder, fieldName, modelName);
369369

370370
if (result.IsModelSet)

src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,4 +61,44 @@ public static string CreatePropertyModelName(string? prefix, string? propertyNam
6161

6262
return prefix + "." + propertyName;
6363
}
64+
65+
/// <summary>
66+
/// Create a model property name using a prefix and a property name,
67+
/// with a small optimization to avoid redundancy.
68+
///
69+
/// For example, if both <paramref name="prefix"/> and <paramref name="propertyName"/> are "parameter"
70+
/// (ignoring case), the result will be just "parameter" instead of "parameter.Parameter".
71+
/// </summary>
72+
/// <param name="prefix">The prefix to use.</param>
73+
/// <param name="propertyName">The property name.</param>
74+
/// <returns>The property model name.</returns>
75+
public static string CreatePropertyModelNameOptimized(string? prefix, string? propertyName)
76+
{
77+
if (string.IsNullOrEmpty(prefix))
78+
{
79+
return propertyName ?? string.Empty;
80+
}
81+
82+
if (string.IsNullOrEmpty(propertyName))
83+
{
84+
return prefix ?? string.Empty;
85+
}
86+
87+
if (propertyName.StartsWith('['))
88+
{
89+
// The propertyName might represent an indexer access, in which case combining
90+
// with a 'dot' would be invalid. This case occurs only when called from ValidationVisitor.
91+
return prefix + propertyName;
92+
}
93+
94+
if (string.Equals(prefix, propertyName, StringComparison.OrdinalIgnoreCase))
95+
{
96+
// if we are dealing with with something like:
97+
// prefix = parameter and propertyName = parameter
98+
// it should fallback to the property name.
99+
return propertyName;
100+
}
101+
102+
return prefix + "." + propertyName;
103+
}
64104
}

src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,4 @@ Microsoft.AspNetCore.Mvc.ProducesDefaultResponseTypeAttribute.Description.get ->
66
Microsoft.AspNetCore.Mvc.ProducesDefaultResponseTypeAttribute.Description.set -> void
77
Microsoft.AspNetCore.Mvc.ProducesResponseTypeAttribute.Description.get -> string?
88
Microsoft.AspNetCore.Mvc.ProducesResponseTypeAttribute.Description.set -> void
9+
static Microsoft.AspNetCore.Mvc.ModelBinding.ModelNames.CreatePropertyModelNameOptimized(string? prefix, string? propertyName) -> string!

src/Mvc/test/Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -338,6 +338,74 @@ public async Task CollectionModelBinder_BindsListOfComplexType_WithRequiredPrope
338338
Assert.Equal("A value for the 'Name' parameter or property was not provided.", error.ErrorMessage);
339339
}
340340

341+
class TestClass
342+
{
343+
public TestClass2 Parameter { get; set; }
344+
}
345+
346+
class TestClass2
347+
{
348+
public string Parameter { get; set; } = "";
349+
}
350+
351+
[Fact(Skip = "Nested properties are more complex to deal with it. See https://github.com/dotnet/aspnetcore/pull/62459 for more info.")]
352+
public async Task CollectionModelBinder_CanBind_NestedProperties_WithSameNameOfParameter()
353+
{
354+
// Arrange
355+
var parameterBinder = ModelBindingTestHelper.GetParameterBinder();
356+
var parameter = new ParameterDescriptor()
357+
{
358+
Name = "parameter",
359+
ParameterType = typeof(TestClass)
360+
};
361+
362+
var testContext = ModelBindingTestHelper.GetTestContext(request =>
363+
{
364+
request.QueryString = new QueryString("?parameter.parameter=testing");
365+
});
366+
367+
var modelState = testContext.ModelState;
368+
369+
// Act
370+
var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext);
371+
372+
// Assert
373+
Assert.True(modelBindingResult.IsModelSet);
374+
375+
var model = Assert.IsType<TestClass>(modelBindingResult.Model);
376+
Assert.Equal("testing", model.Parameter.Parameter);
377+
Assert.True(modelState.IsValid);
378+
}
379+
380+
[Fact]
381+
public async Task CollectionModelBinder_CanBind_SimpleProperties_WithSameNameOfParameter()
382+
{
383+
// Arrange
384+
var parameterBinder = ModelBindingTestHelper.GetParameterBinder();
385+
var parameter = new ParameterDescriptor()
386+
{
387+
Name = "parameter",
388+
ParameterType = typeof(TestClass2)
389+
};
390+
391+
var testContext = ModelBindingTestHelper.GetTestContext(request =>
392+
{
393+
request.QueryString = new QueryString("?parameter=testing");
394+
});
395+
396+
var modelState = testContext.ModelState;
397+
398+
// Act
399+
var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext);
400+
401+
// Assert
402+
Assert.True(modelBindingResult.IsModelSet);
403+
404+
var model = Assert.IsType<TestClass2>(modelBindingResult.Model);
405+
Assert.Equal("testing", model.Parameter);
406+
Assert.True(modelState.IsValid);
407+
}
408+
341409
[Fact]
342410
public async Task CollectionModelBinder_BindsListOfComplexType_WithRequiredProperty_WithExplicitPrefix_PartialData()
343411
{

0 commit comments

Comments
 (0)