From 6ba8bbd9273cfccc4bca45c291310ae4a4c442f3 Mon Sep 17 00:00:00 2001 From: Benjamin Petit Date: Thu, 17 Aug 2023 16:25:50 +0200 Subject: [PATCH 01/10] Add FromKeyedServicesAttribute support to MVC --- .../src/ModelBinding/BindingInfo.cs | 8 +++ .../src/ModelBinding/BindingSource.cs | 9 ++++ src/Mvc/Mvc.Abstractions/src/Resources.resx | 3 ++ .../test/ModelBinding/BindingInfoTest.cs | 21 ++++++++ .../Infrastructure/MvcCoreMvcOptionsSetup.cs | 1 + .../Binders/KeyedServicesModelBinder.cs | 45 ++++++++++++++++ .../KeyedServicesModelBinderProvider.cs | 43 ++++++++++++++++ .../Mvc.FunctionalTests/KeyedServicesTests.cs | 51 +++++++++++++++++++ .../Controllers/CustomServiceApiController.cs | 31 +++++++++++ .../WebSites/BasicWebSite/CustomService.cs | 25 +++++++++ .../StartupWithoutEndpointRouting.cs | 2 + 11 files changed, 239 insertions(+) create mode 100644 src/Mvc/Mvc.Core/src/ModelBinding/Binders/KeyedServicesModelBinder.cs create mode 100644 src/Mvc/Mvc.Core/src/ModelBinding/Binders/KeyedServicesModelBinderProvider.cs create mode 100644 src/Mvc/test/Mvc.FunctionalTests/KeyedServicesTests.cs create mode 100644 src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomServiceApiController.cs create mode 100644 src/Mvc/test/WebSites/BasicWebSite/CustomService.cs diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs index ad294bf6a52d..8246393939ae 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs @@ -4,6 +4,7 @@ using System.Linq; using System.Reflection; using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.Extensions.DependencyInjection; namespace Microsoft.AspNetCore.Mvc.ModelBinding; @@ -169,6 +170,13 @@ public Type? BinderType break; } + // Keyed services + foreach (var fromKeyedServicesAttribute in attributes.OfType()) + { + isBindingInfoPresent = true; + bindingInfo.BindingSource = BindingSource.KeyedServices; + } + return isBindingInfoPresent ? bindingInfo : null; } diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingSource.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingSource.cs index 45dafe05778d..1adf6b6b991b 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingSource.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingSource.cs @@ -86,6 +86,15 @@ public class BindingSource : IEquatable isGreedy: true, isFromRequest: false); + /// + /// A for request keyed services. + /// + public static readonly BindingSource KeyedServices = new BindingSource( + "KeyedServices", + Resources.BindingSource_KeyedServices, + isGreedy: true, + isFromRequest: false); + /// /// A for special parameter types that are not user input. /// diff --git a/src/Mvc/Mvc.Abstractions/src/Resources.resx b/src/Mvc/Mvc.Abstractions/src/Resources.resx index 5d38b2bdae14..9b699cae19a8 100644 --- a/src/Mvc/Mvc.Abstractions/src/Resources.resx +++ b/src/Mvc/Mvc.Abstractions/src/Resources.resx @@ -183,4 +183,7 @@ The specified key exceeded the maximum ModelState depth: {0} + + KeyedServices + \ No newline at end of file diff --git a/src/Mvc/Mvc.Abstractions/test/ModelBinding/BindingInfoTest.cs b/src/Mvc/Mvc.Abstractions/test/ModelBinding/BindingInfoTest.cs index a9680cd9e78c..c18d63dd1649 100644 --- a/src/Mvc/Mvc.Abstractions/test/ModelBinding/BindingInfoTest.cs +++ b/src/Mvc/Mvc.Abstractions/test/ModelBinding/BindingInfoTest.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using Microsoft.AspNetCore.Mvc.ModelBinding.Binders; +using Microsoft.Extensions.DependencyInjection; using Moq; namespace Microsoft.AspNetCore.Mvc.ModelBinding; @@ -286,4 +287,24 @@ public void GetBindingInfo_WithAttributesAndModelMetadata_PreserveEmptyBodyDefau Assert.NotNull(bindingInfo); Assert.Equal(EmptyBodyBehavior.Default, bindingInfo.EmptyBodyBehavior); } + + [Fact] + public void GetBindingInfo_WithFromKeyedServicesAttribute() + { + // Arrange + var attributes = new object[] + { + new FromKeyedServicesAttribute(new object()), + }; + var modelType = typeof(Guid); + var provider = new TestModelMetadataProvider(); + var modelMetadata = provider.GetMetadataForType(modelType); + + // Act + var bindingInfo = BindingInfo.GetBindingInfo(attributes, modelMetadata); + + // Assert + Assert.NotNull(bindingInfo); + Assert.Same(BindingSource.KeyedServices, bindingInfo.BindingSource); + } } diff --git a/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs b/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs index 908a33ef721c..3a849264fc33 100644 --- a/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs +++ b/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs @@ -47,6 +47,7 @@ public void Configure(MvcOptions options) // Set up ModelBinding options.ModelBinderProviders.Add(new BinderTypeModelBinderProvider()); options.ModelBinderProviders.Add(new ServicesModelBinderProvider()); + options.ModelBinderProviders.Add(new KeyedServicesModelBinderProvider()); options.ModelBinderProviders.Add(new BodyModelBinderProvider(options.InputFormatters, _readerFactory, _loggerFactory, options)); options.ModelBinderProviders.Add(new HeaderModelBinderProvider()); options.ModelBinderProviders.Add(new FloatingPointTypeModelBinderProvider()); diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/KeyedServicesModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/KeyedServicesModelBinder.cs new file mode 100644 index 000000000000..c231e39dbe8b --- /dev/null +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/KeyedServicesModelBinder.cs @@ -0,0 +1,45 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#nullable enable + +using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; +using Microsoft.Extensions.DependencyInjection; + +namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders; + +/// +/// An which binds models from the request services when a model +/// has the binding source . +/// +public class KeyedServicesModelBinder : IModelBinder +{ + internal bool IsOptional { get; set; } + + internal object? Key { get; set; } + + /// + public Task BindModelAsync(ModelBindingContext bindingContext) + { + ArgumentNullException.ThrowIfNull(bindingContext); + + var requestServices = bindingContext.HttpContext.RequestServices as IKeyedServiceProvider; + if (requestServices == null) + { + bindingContext.Result = ModelBindingResult.Failed(); + return Task.CompletedTask; + } + + var model = IsOptional ? + requestServices.GetKeyedService(bindingContext.ModelType, Key) : + requestServices.GetRequiredKeyedService(bindingContext.ModelType, Key); + + if (model != null) + { + bindingContext.ValidationState.Add(model, new ValidationStateEntry() { SuppressValidation = true }); + } + + bindingContext.Result = ModelBindingResult.Success(model); + return Task.CompletedTask; + } +} diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/KeyedServicesModelBinderProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/KeyedServicesModelBinderProvider.cs new file mode 100644 index 000000000000..9bd8fbf47fe0 --- /dev/null +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/KeyedServicesModelBinderProvider.cs @@ -0,0 +1,43 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#nullable enable + +using System.Reflection; +using Microsoft.Extensions.DependencyInjection; + +namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders; + +/// +/// An for binding from the . +/// +public class KeyedServicesModelBinderProvider : IModelBinderProvider +{ + /// + public IModelBinder? GetBinder(ModelBinderProviderContext context) + { + ArgumentNullException.ThrowIfNull(context); + + if (context.BindingInfo.BindingSource != null && + context.BindingInfo.BindingSource.CanAcceptDataFrom(BindingSource.KeyedServices)) + { + // IsRequired will be false for a Reference Type + // without a default value in a oblivious nullability context + // however, for services we should treat them as required + var isRequired = context.Metadata.IsRequired || + (context.Metadata.Identity.ParameterInfo?.HasDefaultValue != true && + !context.Metadata.ModelType.IsValueType && + context.Metadata.NullabilityState == NullabilityState.Unknown); + + var attribute = context.Metadata.Identity.ParameterInfo?.GetCustomAttribute(); + + return new KeyedServicesModelBinder + { + IsOptional = !isRequired, + Key = attribute?.Key + }; + } + + return null; + } +} diff --git a/src/Mvc/test/Mvc.FunctionalTests/KeyedServicesTests.cs b/src/Mvc/test/Mvc.FunctionalTests/KeyedServicesTests.cs new file mode 100644 index 000000000000..3b12938fc4c0 --- /dev/null +++ b/src/Mvc/test/Mvc.FunctionalTests/KeyedServicesTests.cs @@ -0,0 +1,51 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Net.Http; + +namespace Microsoft.AspNetCore.Mvc.FunctionalTests; + +public class KeyedServicesTests : IClassFixture> +{ + public KeyedServicesTests(MvcTestFixture fixture) + { + Client = fixture.CreateDefaultClient(); + } + + public HttpClient Client { get; } + + [Fact] + public async Task ExplicitSingleFromKeyedServiceAttribute() + { + // Arrange + var okRequest = new HttpRequestMessage(HttpMethod.Get, "/services/GetOk"); + var notokRequest = new HttpRequestMessage(HttpMethod.Get, "/services/GetNotOk"); + + // Act + var okResponse = await Client.SendAsync(okRequest); + var notokResponse = await Client.SendAsync(notokRequest); + + // Assert + Assert.True(okResponse.IsSuccessStatusCode); + Assert.True(notokResponse.IsSuccessStatusCode); + Assert.Equal("OK", await okResponse.Content.ReadAsStringAsync()); + Assert.Equal("NOT OK", await notokResponse.Content.ReadAsStringAsync()); + } + + [Fact] + public async Task ExplicitMultipleFromKeyedServiceAttribute() + { + // Arrange + var request = new HttpRequestMessage(HttpMethod.Get, "/services/GetBoth"); + + // Act + var response = await Client.SendAsync(request); + + // Assert + Assert.True(response.IsSuccessStatusCode); + Assert.Equal("OK,NOT OK", await response.Content.ReadAsStringAsync()); + + var response2 = await Client.SendAsync(new HttpRequestMessage(HttpMethod.Get, "/services/GetBoth")); + Assert.True(response2.IsSuccessStatusCode); + } +} diff --git a/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomServiceApiController.cs b/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomServiceApiController.cs new file mode 100644 index 000000000000..fe87492a8fb1 --- /dev/null +++ b/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomServiceApiController.cs @@ -0,0 +1,31 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.AspNetCore.Mvc; + +namespace BasicWebSite; + +[ApiController] +[Route("/services")] +public class CustomServicesApiController : Controller +{ + [HttpGet("GetOk")] + public ActionResult GetOk([FromKeyedServices("ok_service")] ICustomService service) + { + return service.Process(); + } + + [HttpGet("GetNotOk")] + public ActionResult GetNotOk([FromKeyedServices("not_ok_service")] ICustomService service) + { + return service.Process(); + } + + [HttpGet("GetBoth")] + public ActionResult GetBoth( + [FromKeyedServices("ok_service")] ICustomService s1, + [FromKeyedServices("not_ok_service")] ICustomService s2) + { + return $"{s1.Process()},{s2.Process()}"; + } +} diff --git a/src/Mvc/test/WebSites/BasicWebSite/CustomService.cs b/src/Mvc/test/WebSites/BasicWebSite/CustomService.cs new file mode 100644 index 000000000000..30b2e2d1e9e1 --- /dev/null +++ b/src/Mvc/test/WebSites/BasicWebSite/CustomService.cs @@ -0,0 +1,25 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using BasicWebSite.Models; +using Microsoft.AspNetCore.Http.HttpResults; +using Microsoft.AspNetCore.Mvc; + +namespace BasicWebSite; + +public interface ICustomService +{ + string Process(); +} + +public class OkCustomService : ICustomService +{ + public string Process() => "OK"; + public override string ToString() => Process(); +} + +public class BadCustomService : ICustomService +{ + public string Process() => "NOT OK"; + public override string ToString() => Process(); +} diff --git a/src/Mvc/test/WebSites/BasicWebSite/StartupWithoutEndpointRouting.cs b/src/Mvc/test/WebSites/BasicWebSite/StartupWithoutEndpointRouting.cs index f7b1b81e0a86..6ff3aafc0f91 100644 --- a/src/Mvc/test/WebSites/BasicWebSite/StartupWithoutEndpointRouting.cs +++ b/src/Mvc/test/WebSites/BasicWebSite/StartupWithoutEndpointRouting.cs @@ -43,6 +43,8 @@ public void ConfigureServices(IServiceCollection services) services.AddSingleton(); services.AddHttpContextAccessor(); services.AddSingleton(); + services.AddKeyedSingleton("ok_service"); + services.AddKeyedSingleton("not_ok_service"); services.AddScoped(); services.AddTransient(); services.AddScoped(); From faaff428bf4bc7e94f6387cd9e35353a3523044a Mon Sep 17 00:00:00 2001 From: Benjamin Petit Date: Thu, 17 Aug 2023 18:20:10 +0200 Subject: [PATCH 02/10] Do not allow mixing [FromServices] and [FromKeyedServices] on same parameter --- .../src/ModelBinding/BindingInfo.cs | 12 +++++++++++- .../test/ModelBinding/BindingInfoTest.cs | 17 +++++++++++++++++ .../Mvc.FunctionalTests/KeyedServicesTests.cs | 3 --- 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs index 8246393939ae..d221242f01b7 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs @@ -3,6 +3,8 @@ using System.Linq; using System.Reflection; +using Microsoft.AspNetCore.Http.Metadata; +using System.Reflection.Metadata; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.Extensions.DependencyInjection; @@ -171,7 +173,15 @@ public Type? BinderType } // Keyed services - foreach (var fromKeyedServicesAttribute in attributes.OfType()) + if (attributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.GetType()))) + { + if (attributes.OfType().FirstOrDefault() is not null) + { + throw new NotSupportedException( + $"The {nameof(FromKeyedServicesAttribute)} is not supported on parameters that are also annotated with {nameof(IFromServiceMetadata)}."); + } + } + if (attributes.OfType().Any()) { isBindingInfoPresent = true; bindingInfo.BindingSource = BindingSource.KeyedServices; diff --git a/src/Mvc/Mvc.Abstractions/test/ModelBinding/BindingInfoTest.cs b/src/Mvc/Mvc.Abstractions/test/ModelBinding/BindingInfoTest.cs index c18d63dd1649..4a7e60e73ca7 100644 --- a/src/Mvc/Mvc.Abstractions/test/ModelBinding/BindingInfoTest.cs +++ b/src/Mvc/Mvc.Abstractions/test/ModelBinding/BindingInfoTest.cs @@ -307,4 +307,21 @@ public void GetBindingInfo_WithFromKeyedServicesAttribute() Assert.NotNull(bindingInfo); Assert.Same(BindingSource.KeyedServices, bindingInfo.BindingSource); } + + [Fact] + public void GetBindingInfo_ThrowsWhenWithFromKeyedServicesAttributeAndIFromServiceMetadata() + { + // Arrange + var attributes = new object[] + { + new FromKeyedServicesAttribute(new object()), + new FromServicesAttribute() + }; + var modelType = typeof(Guid); + var provider = new TestModelMetadataProvider(); + var modelMetadata = provider.GetMetadataForType(modelType); + + // Act and Assert + Assert.Throws(() => BindingInfo.GetBindingInfo(attributes, modelMetadata)); + } } diff --git a/src/Mvc/test/Mvc.FunctionalTests/KeyedServicesTests.cs b/src/Mvc/test/Mvc.FunctionalTests/KeyedServicesTests.cs index 3b12938fc4c0..c02b3868c3ca 100644 --- a/src/Mvc/test/Mvc.FunctionalTests/KeyedServicesTests.cs +++ b/src/Mvc/test/Mvc.FunctionalTests/KeyedServicesTests.cs @@ -44,8 +44,5 @@ public async Task ExplicitMultipleFromKeyedServiceAttribute() // Assert Assert.True(response.IsSuccessStatusCode); Assert.Equal("OK,NOT OK", await response.Content.ReadAsStringAsync()); - - var response2 = await Client.SendAsync(new HttpRequestMessage(HttpMethod.Get, "/services/GetBoth")); - Assert.True(response2.IsSuccessStatusCode); } } From ff48ead5079b0af630f3e77c37f881c5de14dfe8 Mon Sep 17 00:00:00 2001 From: Benjamin Petit Date: Thu, 17 Aug 2023 20:42:36 +0200 Subject: [PATCH 03/10] Move key extraction to BindingInfo.GetBindingInfo to simplify a bit --- .../src/ModelBinding/BindingInfo.cs | 11 ++++- .../src/ModelBinding/BindingSource.cs | 9 ---- src/Mvc/Mvc.Abstractions/src/Resources.resx | 3 -- .../test/ModelBinding/BindingInfoTest.cs | 6 ++- .../Infrastructure/MvcCoreMvcOptionsSetup.cs | 1 - .../Binders/KeyedServicesModelBinder.cs | 30 ++++++------- .../KeyedServicesModelBinderProvider.cs | 43 ------------------- .../Binders/ServicesModelBinder.cs | 1 + .../Binders/ServicesModelBinderProvider.cs | 7 ++- 9 files changed, 34 insertions(+), 77 deletions(-) delete mode 100644 src/Mvc/Mvc.Core/src/ModelBinding/Binders/KeyedServicesModelBinderProvider.cs diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs index d221242f01b7..5272747047d5 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs @@ -38,6 +38,7 @@ public BindingInfo(BindingInfo other) PropertyFilterProvider = other.PropertyFilterProvider; RequestPredicate = other.RequestPredicate; EmptyBodyBehavior = other.EmptyBodyBehavior; + ServiceKey = other.ServiceKey; } /// @@ -92,6 +93,11 @@ public Type? BinderType /// public EmptyBodyBehavior EmptyBodyBehavior { get; set; } + /// + /// Get or sets the value used as the key when looking for a keyed service + /// + public object? ServiceKey { get; set; } + /// /// Constructs a new instance of from the given . /// @@ -181,10 +187,11 @@ public Type? BinderType $"The {nameof(FromKeyedServicesAttribute)} is not supported on parameters that are also annotated with {nameof(IFromServiceMetadata)}."); } } - if (attributes.OfType().Any()) + if (attributes.OfType().FirstOrDefault() is { } fromKeydServicesAttribute) { isBindingInfoPresent = true; - bindingInfo.BindingSource = BindingSource.KeyedServices; + bindingInfo.BindingSource = BindingSource.Services; + bindingInfo.ServiceKey = fromKeydServicesAttribute.Key; } return isBindingInfoPresent ? bindingInfo : null; diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingSource.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingSource.cs index 1adf6b6b991b..45dafe05778d 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingSource.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingSource.cs @@ -86,15 +86,6 @@ public class BindingSource : IEquatable isGreedy: true, isFromRequest: false); - /// - /// A for request keyed services. - /// - public static readonly BindingSource KeyedServices = new BindingSource( - "KeyedServices", - Resources.BindingSource_KeyedServices, - isGreedy: true, - isFromRequest: false); - /// /// A for special parameter types that are not user input. /// diff --git a/src/Mvc/Mvc.Abstractions/src/Resources.resx b/src/Mvc/Mvc.Abstractions/src/Resources.resx index 9b699cae19a8..5d38b2bdae14 100644 --- a/src/Mvc/Mvc.Abstractions/src/Resources.resx +++ b/src/Mvc/Mvc.Abstractions/src/Resources.resx @@ -183,7 +183,4 @@ The specified key exceeded the maximum ModelState depth: {0} - - KeyedServices - \ No newline at end of file diff --git a/src/Mvc/Mvc.Abstractions/test/ModelBinding/BindingInfoTest.cs b/src/Mvc/Mvc.Abstractions/test/ModelBinding/BindingInfoTest.cs index 4a7e60e73ca7..00d42c3b12c4 100644 --- a/src/Mvc/Mvc.Abstractions/test/ModelBinding/BindingInfoTest.cs +++ b/src/Mvc/Mvc.Abstractions/test/ModelBinding/BindingInfoTest.cs @@ -292,9 +292,10 @@ public void GetBindingInfo_WithAttributesAndModelMetadata_PreserveEmptyBodyDefau public void GetBindingInfo_WithFromKeyedServicesAttribute() { // Arrange + var key = new object(); var attributes = new object[] { - new FromKeyedServicesAttribute(new object()), + new FromKeyedServicesAttribute(key), }; var modelType = typeof(Guid); var provider = new TestModelMetadataProvider(); @@ -305,7 +306,8 @@ public void GetBindingInfo_WithFromKeyedServicesAttribute() // Assert Assert.NotNull(bindingInfo); - Assert.Same(BindingSource.KeyedServices, bindingInfo.BindingSource); + Assert.Same(BindingSource.Services, bindingInfo.BindingSource); + Assert.Same(key, bindingInfo.ServiceKey); } [Fact] diff --git a/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs b/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs index 3a849264fc33..908a33ef721c 100644 --- a/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs +++ b/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs @@ -47,7 +47,6 @@ public void Configure(MvcOptions options) // Set up ModelBinding options.ModelBinderProviders.Add(new BinderTypeModelBinderProvider()); options.ModelBinderProviders.Add(new ServicesModelBinderProvider()); - options.ModelBinderProviders.Add(new KeyedServicesModelBinderProvider()); options.ModelBinderProviders.Add(new BodyModelBinderProvider(options.InputFormatters, _readerFactory, _loggerFactory, options)); options.ModelBinderProviders.Add(new HeaderModelBinderProvider()); options.ModelBinderProviders.Add(new FloatingPointTypeModelBinderProvider()); diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/KeyedServicesModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/KeyedServicesModelBinder.cs index c231e39dbe8b..f148986930b0 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/KeyedServicesModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/KeyedServicesModelBinder.cs @@ -1,4 +1,4 @@ -// Licensed to the .NET Foundation under one or more agreements. +// Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. #nullable enable @@ -8,31 +8,29 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders; -/// -/// An which binds models from the request services when a model -/// has the binding source . -/// -public class KeyedServicesModelBinder : IModelBinder +internal class KeyedServicesModelBinder : IModelBinder { - internal bool IsOptional { get; set; } + private readonly object _key; + private readonly bool _isOptional; - internal object? Key { get; set; } + public KeyedServicesModelBinder(object key, bool isOptional) + { + _key = key ?? throw new ArgumentNullException(nameof(key)); + _isOptional = isOptional; + } - /// public Task BindModelAsync(ModelBindingContext bindingContext) { - ArgumentNullException.ThrowIfNull(bindingContext); - - var requestServices = bindingContext.HttpContext.RequestServices as IKeyedServiceProvider; - if (requestServices == null) + var keyedServices = bindingContext.HttpContext.RequestServices as IKeyedServiceProvider; + if (keyedServices == null) { bindingContext.Result = ModelBindingResult.Failed(); return Task.CompletedTask; } - var model = IsOptional ? - requestServices.GetKeyedService(bindingContext.ModelType, Key) : - requestServices.GetRequiredKeyedService(bindingContext.ModelType, Key); + var model = _isOptional ? + keyedServices.GetKeyedService(bindingContext.ModelType, _key) : + keyedServices.GetRequiredKeyedService(bindingContext.ModelType, _key); if (model != null) { diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/KeyedServicesModelBinderProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/KeyedServicesModelBinderProvider.cs deleted file mode 100644 index 9bd8fbf47fe0..000000000000 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/KeyedServicesModelBinderProvider.cs +++ /dev/null @@ -1,43 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -#nullable enable - -using System.Reflection; -using Microsoft.Extensions.DependencyInjection; - -namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders; - -/// -/// An for binding from the . -/// -public class KeyedServicesModelBinderProvider : IModelBinderProvider -{ - /// - public IModelBinder? GetBinder(ModelBinderProviderContext context) - { - ArgumentNullException.ThrowIfNull(context); - - if (context.BindingInfo.BindingSource != null && - context.BindingInfo.BindingSource.CanAcceptDataFrom(BindingSource.KeyedServices)) - { - // IsRequired will be false for a Reference Type - // without a default value in a oblivious nullability context - // however, for services we should treat them as required - var isRequired = context.Metadata.IsRequired || - (context.Metadata.Identity.ParameterInfo?.HasDefaultValue != true && - !context.Metadata.ModelType.IsValueType && - context.Metadata.NullabilityState == NullabilityState.Unknown); - - var attribute = context.Metadata.Identity.ParameterInfo?.GetCustomAttribute(); - - return new KeyedServicesModelBinder - { - IsOptional = !isRequired, - Key = attribute?.Key - }; - } - - return null; - } -} diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinder.cs index f40627bd1226..587736de7190 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinder.cs @@ -3,6 +3,7 @@ #nullable enable +using System.Reflection; using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Microsoft.Extensions.DependencyInjection; diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinderProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinderProvider.cs index 6dc30fb4952c..2ac35d51319f 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinderProvider.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinderProvider.cs @@ -25,12 +25,17 @@ public class ServicesModelBinderProvider : IModelBinderProvider { // IsRequired will be false for a Reference Type // without a default value in a oblivious nullability context - // however, for services we shoud treat them as required + // however, for services we should treat them as required var isRequired = context.Metadata.IsRequired || (context.Metadata.Identity.ParameterInfo?.HasDefaultValue != true && !context.Metadata.ModelType.IsValueType && context.Metadata.NullabilityState == NullabilityState.Unknown); + if (context.BindingInfo.ServiceKey != null) + { + return new KeyedServicesModelBinder(context.BindingInfo.ServiceKey, !isRequired); + } + return isRequired ? _servicesBinder : _optionalServicesBinder; } From 3092e3e959174110dbabf8789e941dbce3177b65 Mon Sep 17 00:00:00 2001 From: Benjamin Petit Date: Thu, 17 Aug 2023 21:16:13 +0200 Subject: [PATCH 04/10] Add ServiceKey getter and setter to the unshipped api list --- src/Mvc/Mvc.Abstractions/src/PublicAPI.Unshipped.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Mvc/Mvc.Abstractions/src/PublicAPI.Unshipped.txt b/src/Mvc/Mvc.Abstractions/src/PublicAPI.Unshipped.txt index 7dc5c58110bf..b6cfbc5fdfa4 100644 --- a/src/Mvc/Mvc.Abstractions/src/PublicAPI.Unshipped.txt +++ b/src/Mvc/Mvc.Abstractions/src/PublicAPI.Unshipped.txt @@ -1 +1,3 @@ #nullable enable +Microsoft.AspNetCore.Mvc.ModelBinding.BindingInfo.ServiceKey.get -> object? +Microsoft.AspNetCore.Mvc.ModelBinding.BindingInfo.ServiceKey.set -> void From af49ecd682a9240b3b3a1a1106a329db9563525d Mon Sep 17 00:00:00 2001 From: Benjamin Petit Date: Thu, 17 Aug 2023 22:01:18 +0200 Subject: [PATCH 05/10] Remove useless using directive --- src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs index 5272747047d5..8f2571c46907 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs @@ -4,7 +4,6 @@ using System.Linq; using System.Reflection; using Microsoft.AspNetCore.Http.Metadata; -using System.Reflection.Metadata; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.Extensions.DependencyInjection; From c45ac39f0a7982bff4c53e0eb7f554d498d90d2b Mon Sep 17 00:00:00 2001 From: Benjamin Petit Date: Thu, 17 Aug 2023 22:22:37 +0200 Subject: [PATCH 06/10] Remove another useless using directive --- src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinder.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinder.cs index 587736de7190..f40627bd1226 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinder.cs @@ -3,7 +3,6 @@ #nullable enable -using System.Reflection; using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Microsoft.Extensions.DependencyInjection; From 29a9509fcbf63972514f58bf36e09e8a7df5c744 Mon Sep 17 00:00:00 2001 From: Benjamin Petit Date: Fri, 18 Aug 2023 16:04:21 +0200 Subject: [PATCH 07/10] Add tests --- .../Mvc.FunctionalTests/KeyedServicesTests.cs | 41 +++++++++++++++++++ .../Controllers/CustomServiceApiController.cs | 22 ++++++++++ .../WebSites/BasicWebSite/CustomService.cs | 7 ++++ .../StartupWithoutEndpointRouting.cs | 1 + 4 files changed, 71 insertions(+) diff --git a/src/Mvc/test/Mvc.FunctionalTests/KeyedServicesTests.cs b/src/Mvc/test/Mvc.FunctionalTests/KeyedServicesTests.cs index c02b3868c3ca..87f304163e7d 100644 --- a/src/Mvc/test/Mvc.FunctionalTests/KeyedServicesTests.cs +++ b/src/Mvc/test/Mvc.FunctionalTests/KeyedServicesTests.cs @@ -45,4 +45,45 @@ public async Task ExplicitMultipleFromKeyedServiceAttribute() Assert.True(response.IsSuccessStatusCode); Assert.Equal("OK,NOT OK", await response.Content.ReadAsStringAsync()); } + + [Fact] + public async Task ExplicitSingleFromKeyedServiceAttributeWithNullKey() + { + // Arrange + var request = new HttpRequestMessage(HttpMethod.Get, "/services/GetKeyNull"); + + // Act + var response = await Client.SendAsync(request); + + // Assert + Assert.True(response.IsSuccessStatusCode); + Assert.Equal("DEFAULT", await response.Content.ReadAsStringAsync()); + } + + [Fact] + public async Task ExplicitSingleFromKeyedServiceAttributeOptionalNotRegistered() + { + // Arrange + var request = new HttpRequestMessage(HttpMethod.Get, "/services/GetOptionalNotRegistered"); + + // Act + var response = await Client.SendAsync(request); + + // Assert + Assert.True(response.IsSuccessStatusCode); + Assert.Equal(string.Empty, await response.Content.ReadAsStringAsync()); + } + + [Fact] + public async Task ExplicitSingleFromKeyedServiceAttributeRequiredNotRegistered() + { + // Arrange + var request = new HttpRequestMessage(HttpMethod.Get, "/services/GetRequiredNotRegistered"); + + // Act + var response = await Client.SendAsync(request); + + // Assert + Assert.False(response.IsSuccessStatusCode); + } } diff --git a/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomServiceApiController.cs b/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomServiceApiController.cs index fe87492a8fb1..1aa59aa0487d 100644 --- a/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomServiceApiController.cs +++ b/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomServiceApiController.cs @@ -28,4 +28,26 @@ public ActionResult GetBoth( { return $"{s1.Process()},{s2.Process()}"; } + + [HttpGet("GetKeyNull")] + public ActionResult GetKeyNull([FromKeyedServices(null)] ICustomService service) + { + return service.Process(); + } + + [HttpGet("GetOptionalNotRegistered")] + public ActionResult GetOptionalNotRegistered([FromKeyedServices("no_existing_key")] ICustomService? service) + { + if (service != null) + { + throw new Exception("Service should not have been resolved"); + } + return string.Empty; + } + + [HttpGet("GetRequiredNotRegistered")] + public ActionResult GetRequiredNotRegistered([FromKeyedServices("no_existing_key")] ICustomService service) + { + return service.Process(); + } } diff --git a/src/Mvc/test/WebSites/BasicWebSite/CustomService.cs b/src/Mvc/test/WebSites/BasicWebSite/CustomService.cs index 30b2e2d1e9e1..6f7b0aaaa8b1 100644 --- a/src/Mvc/test/WebSites/BasicWebSite/CustomService.cs +++ b/src/Mvc/test/WebSites/BasicWebSite/CustomService.cs @@ -23,3 +23,10 @@ public class BadCustomService : ICustomService public string Process() => "NOT OK"; public override string ToString() => Process(); } + +public class DefaultCustomService : ICustomService +{ + public string Process() => "DEFAULT"; + public override string ToString() => Process(); + public static DefaultCustomService Instance => new DefaultCustomService(); +} diff --git a/src/Mvc/test/WebSites/BasicWebSite/StartupWithoutEndpointRouting.cs b/src/Mvc/test/WebSites/BasicWebSite/StartupWithoutEndpointRouting.cs index 6ff3aafc0f91..94a467baa003 100644 --- a/src/Mvc/test/WebSites/BasicWebSite/StartupWithoutEndpointRouting.cs +++ b/src/Mvc/test/WebSites/BasicWebSite/StartupWithoutEndpointRouting.cs @@ -45,6 +45,7 @@ public void ConfigureServices(IServiceCollection services) services.AddSingleton(); services.AddKeyedSingleton("ok_service"); services.AddKeyedSingleton("not_ok_service"); + services.AddSingleton(); services.AddScoped(); services.AddTransient(); services.AddScoped(); From 7d20273fce4e1dd235bc9e9a01ddc6bb9ecbf47c Mon Sep 17 00:00:00 2001 From: Benjamin Petit Date: Fri, 18 Aug 2023 16:09:23 +0200 Subject: [PATCH 08/10] Addressing comments --- .../Mvc.Abstractions/src/ModelBinding/BindingInfo.cs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs index 8f2571c46907..7934a79fbfb1 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs @@ -178,19 +178,16 @@ public Type? BinderType } // Keyed services - if (attributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.GetType()))) + if (attributes.OfType().FirstOrDefault() is { } fromKeyedServicesAttribute) { - if (attributes.OfType().FirstOrDefault() is not null) + if (bindingInfo.BindingSource != null) { throw new NotSupportedException( - $"The {nameof(FromKeyedServicesAttribute)} is not supported on parameters that are also annotated with {nameof(IFromServiceMetadata)}."); + $"The {nameof(FromKeyedServicesAttribute)} is not supported on parameters that are also annotated with {nameof(IBindingSourceMetadata)}."); } - } - if (attributes.OfType().FirstOrDefault() is { } fromKeydServicesAttribute) - { isBindingInfoPresent = true; bindingInfo.BindingSource = BindingSource.Services; - bindingInfo.ServiceKey = fromKeydServicesAttribute.Key; + bindingInfo.ServiceKey = fromKeyedServicesAttribute.Key; } return isBindingInfoPresent ? bindingInfo : null; From b93fe29901bbb2216945ebe535544e8ff0b93ad7 Mon Sep 17 00:00:00 2001 From: Benjamin Petit Date: Mon, 21 Aug 2023 10:20:48 +0200 Subject: [PATCH 09/10] Remove again another useless using directive --- src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs index 7934a79fbfb1..1e4f8f3ca836 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs @@ -3,7 +3,6 @@ using System.Linq; using System.Reflection; -using Microsoft.AspNetCore.Http.Metadata; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.Extensions.DependencyInjection; From 397abeb35ffe5d010af88cb9b3f18aefa1699d1a Mon Sep 17 00:00:00 2001 From: Benjamin Petit Date: Mon, 21 Aug 2023 10:55:55 +0200 Subject: [PATCH 10/10] Add missing #nullable in test file --- .../BasicWebSite/Controllers/CustomServiceApiController.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomServiceApiController.cs b/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomServiceApiController.cs index 1aa59aa0487d..f4a2d432443d 100644 --- a/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomServiceApiController.cs +++ b/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomServiceApiController.cs @@ -35,6 +35,8 @@ public ActionResult GetKeyNull([FromKeyedServices(null)] ICustomService return service.Process(); } +# nullable enable + [HttpGet("GetOptionalNotRegistered")] public ActionResult GetOptionalNotRegistered([FromKeyedServices("no_existing_key")] ICustomService? service) {