RequestDelegateFactory should produce clear errors for invalid Delegates

[davidfowl]

Is your feature request related to a problem? Please describe.

We should make sure its clear what is supported in MapAction instead of it failing with hard to grok errors. For example, #31658 (comment)

Describe the solution you'd like

I think we should have both runtime and compile time validation (an analyzer where possible).

Scenarios for MapActions

1. Forgetting to register a service with the DI container generates an incorrect error as the parameter is viewed as second [FromBody] attribute. Although from the user standpoint the second [FromBody] attribute is not explicit - #35086

app.MapPost("/createUser", ([FromBody] UserDTO userDTO, UserRepository userRepo) => {
    return $"user created  - {userDTO.LastName} - {userRepo.CreateUser(userDTO).LastName}";
 });

The user's intention was to inject the UserRepository as service to use.

Error message:

System.InvalidOperationException: 'Action cannot have more than one FromBody attribute.'

2. Allowing query strings on Post is not semantically correct and might be viewed as a security issue. The user would have expected the [FromQuery] attribute to be ignored/should not bind or should throw an exception at the startup time. Nothing to do.

 app.MapPost("/createUser", ([FromQuery] string userName, [FromBody] User user,) => $"user created- {userName} - {user.LastName} ");

3. FromBody attribute on MapGet does not throw an exception. (User could have copied the method and forget to remove the frombody). Postman allows you to add a body to a get method. However, without a body added to the request, we get 400 which is the correct behavior. If the body is present, binding happens. - Nothing to do - this is by design

builder.Services.AddSingleton<IUserRepository, UserRepository>()

app.MapGet("/findUser/{id}", (int id, [FromBody] UserDTO userDTO, IUserRepository userRepo) => {
    return $"user found - {userDTO.LastName} - {userRepo.FindUserById(id).LastName}";
})

4. In MapGet an implicit injection of any service returns a 400 if the user forgot to register the service in the DI container. However, an explicit use of [FromServices] returns the correct error. This is inconsistent - #35086

  app.MapGet("/findUser/{id}", (int id, UserRepository userRepo) => {
    return $"user found - {id} - {userRepo.FindUserById(id).LastName}";
  });

Error for explicit use of [FromServices]

  app.MapGet("/findUser/{id}", (int id, [FromServices] IUserRepository userRepo) => {
    return $"user found - {id} - {userRepo.FindUserById(id).LastName}";
  });

  System.InvalidOperationException: No service for type 'IUserRepository' has been registered.
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, 
  Type serviceType)
....

5. Improve the error message when routes parameters do match or just ignore casing(lower/upper) - This is a common mistake that users may make - #35087

   app.MapGet("/user/{FirstName}/{lastName}", ([FromRoute()]string? firstName, [FromRoute()]string lastName) => 
    UserService.GetUser(firstName, lastName, "aa"));

Error message (Notice FirstName != firstName):

System.InvalidOperationException: 'firstName is not a route paramter.'

6. What should we do when someone uses attributes that should not be allowed on certain methods ? see example below: -#35088

  app.MapGet("/user", ([Bind("FirstName,LastName")] User user) => $"username {user.FirstName}");

7. Adding an optional/nullability (?) in the route pattern, messes up the Swagger UI and the json file still recognizes as required field. - #35081

   app.MapGet("/user/{id?}", (int? id) => $"userId {id}");

Notice the missing closing bracket

[ghost]

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[BrennanConroy]

This might be done, all bullets either have a linked issue/PR that is closed or are marked "nothing to do".

[BrennanConroy]

Lets verify first before closing

[BrennanConroy]

All 7 scenarios validated against RTM build and before/after for each shown below.

Scenario 1:

// Forgot to register UserRepository as service
app.MapPost("/createUser", ([FromBody] UserDTO userDTO, UserRepository userRepo) => {
    return $"user created  - {userDTO.LastName} - {userRepo.CreateUser(userDTO).LastName}";
 });

Before:
System.InvalidOperationException: 'Action cannot have more than one FromBody attribute.'

After:

Unhandled exception. System.InvalidOperationException: Failure to infer one or more parameters.
Below is the list of parameters that we found:

Parameter           | Source
---------------------------------------------------------------------------------
userDTO             | Body (Attribute)
userRepo            | UNKNOWN


Did you mean to register the "UNKNOWN" parameters as a Service?

Scenario 2:

app.MapPost("/createUser", ([FromQuery] string userName, [FromBody] User user,) => $"user created- {userName} - {user.LastName} ");
Query on POST might be viewed as a security issue - we decided to do nothing here

Scenario 3:

builder.Services.AddSingleton<IUserRepository, UserRepository>();

app.MapGet("/findUser/{id}", (int id, [FromBody] UserDTO userDTO, IUserRepository userRepo) => {
    return $"user found - {userDTO.LastName} - {userRepo.FindUserById(id).LastName}";
});

[FromBody] on GET isn't common and generally a mistake on the users part. We decided to do nothing here.

Scenario 4

Part 1:

// No UserRepository registered
app.MapGet("/findUser/{id}", (int id, UserRepository userRepo) => {
    return $"user found - {id} - {userRepo.FindUserById(id).LastName}";
});

Part 2:

app.MapGet("/findUser/{id}", (int id, [FromServices] IUserRepository userRepo) => {
    return $"user found - {id} - {userRepo.FindUserById(id).LastName}";
});

Before:
Part 1: Returns 400
Pat 2: 500 and exception logged

An unhandled exception has occurred while executing the request.
      System.InvalidOperationException: No service for type 'IUserRepository' has been registered.

After:
Part 1: On startup:

Unhandled exception. System.InvalidOperationException: Body was inferred but the method does not allow inferred body parameters.
Below is the list of parameters that we found:

Parameter           | Source
---------------------------------------------------------------------------------
id                  | Route (Inferred)
userRepo            | Body (Inferred)


Did you mean to register the "Body (Inferred)" parameter(s) as a Service or apply the [FromService] or [FromBody] attribute?

Part 2: On Request
500 and exception logged

An unhandled exception has occurred while executing the request.
      System.InvalidOperationException: No service for type 'IUserRepository' has been registered.

Scenario 5

Improve the error message when routes parameters do not match or just ignore casing(lower/upper)

app.MapGet("/user/{FirstName}/{lastName}", ([FromRoute]string? firstName, [FromRoute]string lastName) => 
    $"{firstName} {lastName});

Before:
System.InvalidOperationException: 'firstName is not a route paramter.'

After:
FirstName matches firstName

Scenario 6

What should we do when someone uses attributes that should not be allowed on certain methods?

app.MapGet("/user", ([Bind("FirstName,LastName")] User user) => $"username {user.FirstName}");

Before:
Nothing

After:
Analyzer warning:
Warning ASP0003 BindAttribute should not be specified for a MapGet Delegate parameter

Scenario 7

Adding an optional/nullability (?) in the route pattern, messes up the Swagger UI and the json file still recognizes as required field.
app.MapGet("/user/{id?}", (int? id) => $"userId {id}");

Before:

After: