Fix Create Property Model Name for Complex Objects #62459
Conversation
github-actions
bot
added
the
area-mvc
dotnet-policy-service
bot
added
the
community-contribution
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.
|
Okay, let's clarify a few things. The first is that I wasn't exactly sure where to write the tests. Internally, I needed a place where I could test the entire flow of the ComplexObjectModelBinder, without Mocks and proxies in between. The place I found was. The perfect place where I could find this was in CollectionModelBinderIntegrationTest, but it doesn't seem like the right place for that. Anyway, if there is a more appropriate place I can move it somewhere else. |
The second problem is that during development I noticed that: If complex objects are organized in the same way as in the original issue, the problem occurs as well, but it is not so simple to resolve due to recursion. Let's say we have the following scenario: class City
{
public Address Parameter { get; set; }
}
class Address
{
public string Parameter { get; set; }
}
public IActionResult SomeEndpoint(City parameter) { /* elided */ }The current behavior of the ModelBinder will incorrectly expect something like:
Unfortunately, the changes I made are not able to solve this problem, since the ComplexObjectModelBinder uses other binders internally to bind the properties with the values. In this case, it will use another ComplexObjectModelBinder to bind the property, but since the model was optimized earlier (it no longer has prefix.property.property and now only has prefix.property), it fails to match the key values to be bound, which is why the written test was marked as Skip. And that’s why I added this separate comment. I believe this could be the subject of a new issue in the future. |
Fix Create Property Model Name for Complex Objects
Description
Added an option to 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.
Now the optimization option allows to treat the modelName and the propertyName with only one if
both are equal.
I opened a PR trying to solve this almost 1 year ago, but I didn't have enough knowledge and ended up abandoning ship. Halfway through, I also deleted my fork of the old repository, so I didn't update the old PR, but I decided to finish this work in another one after having learned a little more. I'm sorry about that and I apologize to everyone who was counting on this.
Fixes #57637