dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.58k stars 10.06k forks source link

ASP.NET Core Web API complex type binding from query does not work if parameter name equals property name #37360

Open yohny opened 3 years ago

yohny commented 3 years ago

I have a ASP.NET core Web API project (fresh ASP.NET Core Web API template from Visual Studio 2019, no customizations), with a controller action like this:

[HttpGet("search")]
public IEnumerable<string> Search([FromQuery] SearchDto dto)
{
  // implementation
}

the action parameter dto is a complex type bound from query and it looks like this:

public class SearchDto
{
  public string Term { get; set; }
  public bool CaseInsensitive { get; set; }
}

If I call the API using URL <host>/<controller>/search?term=abc&caseInsensitive=true it all works as expected - Term property contains "abc" and CaseInsensitive is true.

This makes sense, as according to https://docs.microsoft.com/en-us/aspnet/core/mvc/models/model-binding?view=aspnetcore-5.0#prefix--parameter-name, the binding first tries to find values prefixed with parameter name (aka. it searches for keys "dto.Term" and "dto.CaseInsensitive" in URL query string) and if that fails it tries the properties without prefix (aka. it searches for "Term" and "CaseInsensitive" keys in URL query string) which succeeds and corresponding values are bound.

However if I change the controller action slightly by changing the dto parameter name to term to match the Term property in DTO class:

[HttpGet("search")]
public IEnumerable<string> Search([FromQuery] SearchDto term) // changed from 'dto' to 'term'
{
  // implementation
}

then the binding stops working. Calling the API using the same URL as previously results in controller action being invoked, but the DTO properties are not filled with values from URL, instead they have default values (aka. Term property is null and CaseInsensitive is false).

Why is that? The binding should evaluate the same way as previously - keys "term.Term" and "term.CaseInsensitive" are not found in URL query string, so unprefixed names should be tried and matched as before, thus I expected the binding to work. It seems very weird that such innocent change completely breaks the binding. We encountered this behavior today and it was not easy to figure out why out of nowhere the binding does not work anymore...

Further technical details

Runtime Environment: OS Name: Windows OS Version: 10.0.18363 OS Platform: Windows RID: win10-x64 Base Path: C:\Program Files\dotnet\sdk\5.0.401\

Host (useful for support): Version: 5.0.10 Commit: e1825b4928

.NET SDKs installed: 5.0.301 [C:\Program Files\dotnet\sdk] 5.0.401 [C:\Program Files\dotnet\sdk]

.NET runtimes installed: Microsoft.AspNetCore.App 3.1.19 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 5.0.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 5.0.10 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.NETCore.App 3.1.19 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 5.0.7 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 5.0.10 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.WindowsDesktop.App 3.1.19 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 5.0.7 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 5.0.10 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

pranavkm commented 3 years ago

@captainsafia I wouldn't classify this as a bug. We document this behavior (maybe the docs could do with some helping) - https://docs.microsoft.com/en-us/aspnet/core/mvc/models/model-binding?view=aspnetcore-5.0#prefix--parameter-name and have an analyzer that tries and complains about it for non-api controllers - https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Analyzers/src/TopLevelParameterNameAnalyzer.cs.

captainsafia commented 3 years ago

@pranavkm Thanks for clarifying! We annotated this as a bug during triage because it seemed like buggy behavior.

So, it is intentional that if your parameter name is the same as one of the property names, the lookup will fail? What's responsible for this behavior?

pranavkm commented 3 years ago

So, it is intentional that if your parameter name is the same as one of the property names, the lookup will fail? What's responsible for this behavior?

For a model record Person(string Name, int Age), Mvc supports binding both person.Name / person.Age or Name / Age. However it only falls back to the prefix-less lookups if it cannot find any values that begin with the person prefix. Note that person is also a valid value for this thing since you could have a type converter that converts person=Test|10 and produces a Person instance out of it.

The decision tree looks like so:

The special case is if you have a parameter and a property with the same name: public IActionResult Post(Person name) with ?Name=SomeName&Age=10. In this case, binding decides to use the prefix and starts looking for name.Name and name.Age.

Usually changing model binding's behavior is tricky because user apps, custom model binders, or value providers may have built solutions around the current behavior and we have very limited ways to know outside of users telling us when we break them.

yohny commented 3 years ago

I agree that this can be tricky to change, so if not possible than at least it should be documented better. The current documentation does not describe the behavior as you put it in your comment and does not mention any potential issues/rules for parameter name. Also the mentioned analyzer could be part of default ASP.NET Core Web API projects.

pranavkm commented 3 years ago

The analyzer is on by default, but it skips ApiController instances https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Analyzers/src/TopLevelParameterNameAnalyzer.cs#L57-L62. We could update the code here and have it apply to complex parameters with FromQuery / FromForm / FromRoute etc attributes.

ghost commented 2 years ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

captainsafia commented 2 years ago

Removing the doc label since the behavior is documented here.

Adding the analyzer label since we want to apply the analyzer updates that were mentioned above.