Add support for implicit inference of FromServices for types that appear in DI

[brunolins16]

Current design

With the Minimal APIs, what the RequestDelegateFactory does is fall back to try check if the Parameter is a Service registered in the DI when:

1. No Metadata attributes set and
2. No special types (HttpContext, HttpRequest, HttpResponse, ClaimsPrincipal, CancellationToken, IFormFileCollection and IFormFile) and
3. Parameter does not have BindAsync method and
4. Parameter is not string and
5. Parameter does not have TryParse method

Currently MVC do not have support for items 3 and 4.

The way MVC implements the ModelBinding logic is through ModelBinders that will be assigned, by Provider (Eg. ComplexObjectModelBinderProvider) based on the Parameter metadata.

Also, the binding FromServices is already implemented (ServicesModelBinderProvider/ServicesModelBinder) that will be used when the BindingSource is set to Services.

For API Controllers, the BindingSource will be inferred, when the metadata is not set yet, based on with the following logic:

1. Complex Type => BindingSource = Body
2. Is part of any route => BindingSource = Path
3. Default => BindingSource = Query

In addition to that, Item 2 is similar in MVC since the Special or FormFile binding source is set by BindingSourceMetadataProvider, except for HttpContext, HttpRequest and HttpResponse that are available in the ControllerBase class

Eg.:

modelMetadataDetailsProviders.Add(new BindingSourceMetadataProvider(typeof(CancellationToken), BindingSource.Special));
modelMetadataDetailsProviders.Add(new BindingSourceMetadataProvider(typeof(IFormFile), BindingSource.FormFile));
modelMetadataDetailsProviders.Add(new BindingSourceMetadataProvider(typeof(IFormCollection), BindingSource.FormFile));
modelMetadataDetailsProviders.Add(new BindingSourceMetadataProvider(typeof(IFormFileCollection), BindingSource.FormFile));
modelMetadataDetailsProviders.Add(new BindingSourceMetadataProvider(typeof(IEnumerable<IFormFile>), BindingSource.FormFile));

Based on the previous logic, nothing will be inferred as Services and the only way to the BindingSource to be set to Services is using the [FromServices] attribute.

Proposed Change

Update the Infer mechanism to do the following:

aspnetcore/src/Mvc/Mvc.Core/src/ApplicationModels/InferParameterBindingInfoConvention.cs

Line 94 in 8bd8f58

internal BindingSource InferBindingSourceForParameter(ParameterModel parameter)

1. Complex Type:
   a. Is Registered in the DI =>BindingSource = Services
   b. No registered => BindingSource = Body
2. Simple Types:
   a. Is part of any route => BindingSource = Path
3. Default => BindingSource = Query

My proposal is to use the IServiceProviderIsService service, same used by the RequestDelegateFactory, injected in the constructor.

    public InferParameterBindingInfoConvention(
        IModelMetadataProvider modelMetadataProvider,
+         IServiceProviderIsService? serviceProviderIsService = null)

And update the InferBindingSourceForParameter method to verify if the parameter type is registered in the DI.

          if (_serviceProviderIsService.IsService(parameter.ParameterType))
          {
               return BindingSource.Services;
          }

That will cover the idea of implicit inference of FromService since the ServiceModelBinder will be activated when we set the BindingSource to Services.

Also, I prefer this to be the new default behavior, so my suggestion is to include allowing users to opt-out:

namespace Microsoft.AspNetCore.Mvc;
public class ApiBehaviorOptions
{
+    public bool SuppressInferBindingFromServicesForParameters { get; set; }
}

Usage Examples

services.Configure<ApiBehaviorOptions>(options => {
     options.SuppressInferBindingFromServicesForParameters = true;            
});

[brunolins16]

One aspect of this implementation, in comparison with Minimal, that we could consider as an additional enhancement is if the parameter is Optional we call the GetService operation instead of the GetRequiredService that is been used here:

aspnetcore/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinder.cs

Line 26 in 8bd8f58

var model = requestServices.GetRequiredService(bindingContext.ModelType);

[ghost]

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.

[brunolins16]

One aspect of this implementation, in comparison with Minimal, that we could consider as an additional enhancement is if the parameter is Optional we call the GetService operation instead of the GetRequiredService that is been used here:

aspnetcore/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinder.cs

Line 26 in 8bd8f58

var model = requestServices.GetRequiredService(bindingContext.ModelType);

I added a new issue for this #39757

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

We just approved DisableImplicitFromServiceParameters for the same option in SignalR. #34047 It's not too late to change the property name in SignalR, but I think we should use the same name.

[brunolins16]

We just approved DisableImplicitFromServiceParameters for the same option in SignalR. #34047 It's not too late to change the property name in SignalR, but I think we should use the same name.``

I agree with you, we definitely should use the same. I completely forgot about the API approved in SignalR and suggested the name with the wording SuppressInferBinding that have been used in ApiBehaviorOptions already.

[brunolins16]

In my draft implementation the following API change might be also needed:

namespace Microsoft.AspNetCore.Mvc.ApplicationModels;

public class InferParameterBindingInfoConvention : IActionModelConvention
{
+    public InferParameterBindingInfoConvention(
+       IModelMetadataProvider modelMetadataProvider,
+       IServiceProviderIsService? serviceProviderIsService){}
}

[pranavkm]

API review:

Approved including the new constructor (note that it's has a non-null 2nd parameter)

namespace Microsoft.AspNetCore.Mvc.ApplicationModels;

public class InferParameterBindingInfoConvention : IActionModelConvention
{
+    public InferParameterBindingInfoConvention(
+       IModelMetadataProvider modelMetadataProvider,
+       IServiceProviderIsService serviceProviderIsService){}
}

• We want to change the option to DisableImplicitFromServicesParameters in ApiBehaviorOptions.
• We also want to update SignalR HubOptions to use the new name (there's a small difference from the current name).
• Since this is a breaking behavior change, this should follow SignalR's announcement and also file a breaking change announcement.

[brunolins16]

Announcement Discussion: #40071