aspnetcore
aspnetcore copied to clipboard
dotnet
Create an analyzer that warns when a parameter on a route handler delegate has a BindAsync or TryParse method that doesn't comply with RequestDelegateFactory requirements
Background and Motivation
NOTE: This issue has been reworked, @DamianEdwards initial issue is included below the standard template.
When using the ASP.NET minimal APIs the delegate that is mapped to the route can contain various custom types defined by the developer. In order to create these objects prior to calling the delegate the framework relies on the types in question implementing either BindAsync method or the TryParse method. Unfortunately, developers often define these methods incorrectly.
In .NET 6 a runtime behavior was introduced that resulted in an exception being thrown if these types were listed in the delegate, but their types did not implement these methods. In .NET 8 we want to shift this error left and use an analyzer to tell developers that these methods are missing. An analyzer could help alert one to the fact that they have a TryParse or BindAsync method defined on a type used as a route handler parameter that doesn't meet the requirements for discovery/invocation by RequestDelegateFactory.
Proposed Analyzer
Analyzer Behavior and Message
The analyzer should highlight the type and name declarations of the parameter, i.e. the Todo todo in the example below. If possible, it could also highlight the method name declaration on the type itself, i.e. the BindAsync in the example below, when it's determined that the type is used on a parameter of a route handler.
Category
- [ ] Design
- [ ] Documentation
- [ ] Globalization
- [ ] Interoperability
- [ ] Maintainability
- [ ] Naming
- [ ] Performance
- [ ] Reliability
- [ ] Security
- [ ] Style
- [x] Usage
Severity Level
- [x] Error
- [ ] Warning
- [ ] Info
- [ ] Hidden
Usage Scenarios
using System.Text.Json;
var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();
// ERROR! On both arguments because they don't implement TryParse and BindAsync respectively.
app.MapPost("/organizations/{organization}/contacts", (Organization organization, Contact contact) =>
{
return Results.Ok(contact);
});
app.Run();
// ERROR! Need so implement static bool TryParse(string value, IFormatProvider format, out Organization organization)
public class Organization
{
public static bool TryParse(string value, IFormatProvider format, out Organization organization)
{
organization = new Organization();
return true;
}
}
// ERROR! Need so implement static ValueTask<Contact> BindAsync(HttpContext context)
public class Contact
{
public async ValueTask<Contact> BindAsync(HttpContext context)
{
var contact = new Contact();
return contact;
}
}
Risks
Original Issue
It can be tricky to get the signature for TryParse and BindAsync correct as they're not enforced with any language features (until abstract static members on interfaces are in non-preview).
An analyzer could help alert one to the fact that they have a TryParse or BindAsync method defined on a type used as a route handler parameter that doesn't meet the requirements for discovery/invocation by RequestDelegateFactory.
The analyzer should highlight the type and name declarations of the parameter, i.e. the Todo todo in the example below. If possible it could also highlight the method name declaration on the type itself, i.e. the BindAsync in the example below, when it's determined that the type is used on a parameter of a route handler.
Example
app.MapPost("/todo", (Todo todo) =>
{
// Save the todo...
return Results.Created($"/todo/{todo.Id}", todo);
});
class Todo
{
public int? Id { get; set; }
public string? Title { get; set; }
public IList<string> Tags { get; set; } = new List<string>();
// This won't bind as it isn't static
public async ValueTask<Todo> BindAsync(HttpContext context)
{
if (int.TryParse(context.Request.Query["id"], out var id) Id = id;
Title = context.Request.Query["title"];
var tags = context.Request.Query["tags"];
return ValueTask.FromResult(this);
}
}
We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.
Actually I think we might just throw for this case now, yes @BrennanConroy @halter73?
@DamianEdwards There is indeed a throw
https://github.com/dotnet/aspnetcore/blob/6a172e3080877c506aedc325d03c25df28218afa/src/Shared/ParameterBindingMethodCache.cs#L241
will that be enough?
The analyzer would still be nice so you find out at design/build time before it throws at runtime.
Thanks for contacting us.
We're moving this issue to the .NET 7 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.
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.
Things that we warn on:
BindAsynchas to bestaticand return a ValueTask of X- This is probably an "Error" level analyzer because request delegate creation will fail if this is not set correctly
I noticed that we now have abstract static methods on interfaces in C#11 (non-preview). I'm wondering whether that means we don't need this analyzer anymore, or perhaps there is more guidance that this analyzer can provide. I'd imagine that we need to continue to support folks that have implemented TryParse and BindAsync properly already, but does the guidance change here from merely implementing these methods to implementing abstracting interfaces -- should we be adding interfaces like this to the model binding stack?
public interface IParsableModel<T>
{
public static abstract ValueTask<T> TryParse(string value, IFormatProvider formatter, out T model);
}
public interface IBindableModel<T>
{
public static abstract ValueTask<T> BindAsync(HttpContext context);
}
Hrm, it looks like we have IParsable<TSelf> in System.Runtime with an almost identical signature:
public interface IParsable<TSelf>
{
TSelf Parse(string s, IFormatProvider? provider);
bool TryParse(string? s, IFormatProvider? provider, out TSelf result);
}
https://github.com/dotnet/aspnetcore/issues/42286 and https://github.com/dotnet/aspnetcore/issues/43546
There is a possible ambiguity here that we need to deal with. Consider the following code:
var builder = WebApplication.CreateBuilder(args);
builder.Services.AddScoped<MyService>();
var app = builder.Build();
app.MapPost("/", (MyService myService) => myService.SayHello("World"));
app.Run();
public class MyService
{
public string SayHello(string name)
{
return $"Hello, {name}";
}
}
If we are just analyzing the delegate passed into MapPost there is no way to tell whether we expect MyService to have to implement BindAsync or whether it should be injected as a service. In this simplistic scenario we could probably look for the service injection - but it will be common for service injection to be buried pretty deeply in ASP.NET Core apps which would make an exhaustive search very difficult - but if we aren't exhaustive then throwing an analyzer error on lacking BindAsync would probably be too noisy.
@mitchdenny right, but this issue is about creating an analyzer that only warns when an argument type has a BindAsync or TryParse method but its signature is incorrect (which will lead to a runtime error), at least it was originally :smile:
I'm picking up on the nuance of what you are saying ;)
My PR #45286 achieves this for the parsing scenario because it checks the signature on the TryParse method (or that it implements IParsable<TSelf).
One thing I only just realized is that we get a runtime exception even for arguments on the handler delegate that exposes a BindAsync method that is incorrect but is not marked with the [FromServices] attribute. I'll continue working on the PR to cover the BindAsync scenario since there is no ambiguity at runtime anyway.