-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Fix for issue #54500: Middleware keyed dependency injection #55722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
33c9cf4
c4a279f
d28858e
420f3ed
393802a
2bc5913
e9b37eb
44a072c
d938d4b
56d998c
9b5025f
879b4fc
24a913a
e90de6e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ | |
using System.Runtime.CompilerServices; | ||
using Microsoft.AspNetCore.Http; | ||
using Microsoft.AspNetCore.Http.Abstractions; | ||
using Microsoft.Extensions.Internal; | ||
using Microsoft.Extensions.DependencyInjection; | ||
|
||
namespace Microsoft.AspNetCore.Builder; | ||
|
||
|
@@ -21,6 +21,7 @@ public static class UseMiddlewareExtensions | |
internal const string InvokeAsyncMethodName = "InvokeAsync"; | ||
|
||
private static readonly MethodInfo GetServiceInfo = typeof(UseMiddlewareExtensions).GetMethod(nameof(GetService), BindingFlags.NonPublic | BindingFlags.Static)!; | ||
private static readonly MethodInfo GetKeyedServiceInfo = typeof(UseMiddlewareExtensions).GetMethod(nameof(GetKeyedService), BindingFlags.NonPublic | BindingFlags.Static)!; | ||
|
||
// We're going to keep all public constructors and public methods on middleware | ||
private const DynamicallyAccessedMemberTypes MiddlewareAccessibility = | ||
|
@@ -215,13 +216,55 @@ private static Func<T, HttpContext, IServiceProvider, Task> ReflectionFallback<T | |
methodArguments[0] = context; | ||
for (var i = 1; i < parameters.Length; i++) | ||
{ | ||
methodArguments[i] = GetService(serviceProvider, parameters[i].ParameterType, methodInfo.DeclaringType!); | ||
var parameter = parameters[i]; | ||
|
||
var hasServiceKey = TryGetServiceKey(parameter, out object? key); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is going to happen on every single request, probably worth moving outside of the factory method since the data is the same for every single call. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi Brennan, Thanks for your valuable feedback. I've made some changes to reflect the feedback. I now precompute and cache the results for each parameter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @BrennanConroy, I could eliminate the condition check (hasServiceKey ? GetKeyedService(...) : GetService(...)) as wel by compiling the method calls into delegates, and calling to the delegate directly within the factory. We'd probably be talking about a microsecond though. The code might become slightly less readable though. What's your opinion on the matter? |
||
var parameterType = parameter.ParameterType; | ||
var declaringType = methodInfo.DeclaringType; | ||
|
||
methodArguments[i] = hasServiceKey ? GetKeyedService(serviceProvider, key!, parameterType, declaringType!) : GetService(serviceProvider, parameterType, declaringType!); | ||
} | ||
|
||
return (Task)methodInfo.Invoke(middleware, BindingFlags.DoNotWrapExceptions, binder: null, methodArguments, culture: null)!; | ||
}; | ||
} | ||
|
||
private static bool TryGetServiceKey(ParameterInfo parameterInfo, out object? key) | ||
BrennanConroy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
if (parameterInfo.CustomAttributes != null) | ||
{ | ||
foreach (var attribute in parameterInfo.GetCustomAttributes(true)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @captainsafia, I've taken your feedback into account and changed the method to be more straightforward. Also changed the Inherit parameter from true to false, to meet the ActivatorUtilities from the Microsoft.Extensions.DependencyInjections.Abstractions package. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @captainsafia, @BrennanConroy, the changes within this pull-request target main, and thus the .NET 9.0 version. If I'd want the changes to be available in the latest .NET 8.x.x version as well, what would the appropriate steps for me to take? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @captainsafia, @BrennanConroy, any follow-up on my question above? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is a backport candidate. This change is a feature, not fixing a major bug/regression introduced in 8.0. |
||
{ | ||
if (attribute is FromKeyedServicesAttribute keyed) | ||
{ | ||
key = keyed.Key; | ||
return true; | ||
} | ||
} | ||
} | ||
key = null; | ||
NicoBrabers marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return false; | ||
} | ||
|
||
private static UnaryExpression GetMethodArgument(ParameterInfo parameter, ParameterExpression providerArg, Type parameterType, Type? declaringType) | ||
{ | ||
var parameterTypeExpression = new List<Expression>() { providerArg }; | ||
var hasServiceKey = TryGetServiceKey(parameter, out object? key); | ||
|
||
if (hasServiceKey) | ||
{ | ||
parameterTypeExpression.Add(Expression.Constant(key, typeof(object))); | ||
} | ||
|
||
parameterTypeExpression.Add(Expression.Constant(parameterType, typeof(Type))); | ||
parameterTypeExpression.Add(Expression.Constant(declaringType, typeof(Type))); | ||
|
||
var getServiceCall = Expression.Call(hasServiceKey ? GetKeyedServiceInfo : GetServiceInfo, parameterTypeExpression); | ||
var methodArgument = Expression.Convert(getServiceCall, parameterType); | ||
|
||
return methodArgument; | ||
} | ||
|
||
private static Func<T, HttpContext, IServiceProvider, Task> CompileExpression<T>(MethodInfo methodInfo, ParameterInfo[] parameters) | ||
{ | ||
Debug.Assert(RuntimeFeature.IsDynamicCodeSupported, "Use compiled expression when dynamic code is supported."); | ||
|
@@ -262,21 +305,14 @@ private static Func<T, HttpContext, IServiceProvider, Task> CompileExpression<T> | |
methodArguments[0] = httpContextArg; | ||
for (var i = 1; i < parameters.Length; i++) | ||
{ | ||
var parameterType = parameters[i].ParameterType; | ||
var parameter = parameters[i]; | ||
var parameterType = parameter.ParameterType; | ||
if (parameterType.IsByRef) | ||
{ | ||
throw new NotSupportedException(Resources.FormatException_InvokeDoesNotSupportRefOrOutParams(InvokeMethodName)); | ||
} | ||
|
||
var parameterTypeExpression = new Expression[] | ||
{ | ||
providerArg, | ||
Expression.Constant(parameterType, typeof(Type)), | ||
Expression.Constant(methodInfo.DeclaringType, typeof(Type)) | ||
}; | ||
|
||
var getServiceCall = Expression.Call(GetServiceInfo, parameterTypeExpression); | ||
methodArguments[i] = Expression.Convert(getServiceCall, parameterType); | ||
methodArguments[i] = GetMethodArgument(parameter, providerArg, parameterType, methodInfo.DeclaringType); | ||
} | ||
|
||
Expression middlewareInstanceArg = instanceArg; | ||
|
@@ -294,12 +330,20 @@ private static Func<T, HttpContext, IServiceProvider, Task> CompileExpression<T> | |
|
||
private static object GetService(IServiceProvider sp, Type type, Type middleware) | ||
{ | ||
var service = sp.GetService(type); | ||
if (service == null) | ||
var service = sp.GetService(type) ?? throw new InvalidOperationException(Resources.FormatException_InvokeMiddlewareNoService(type, middleware)); | ||
|
||
return service; | ||
} | ||
|
||
private static object GetKeyedService(IServiceProvider sp, object key, Type type, Type middleware) | ||
{ | ||
if (sp is IKeyedServiceProvider ksp) | ||
{ | ||
throw new InvalidOperationException(Resources.FormatException_InvokeMiddlewareNoService(type, middleware)); | ||
var service = ksp.GetKeyedService(type, key) ?? throw new InvalidOperationException(Resources.FormatException_InvokeMiddlewareNoService(type, middleware)); | ||
|
||
return service; | ||
} | ||
|
||
return service; | ||
throw new InvalidOperationException(Resources.Exception_KeyedServicesNotSupported); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -162,4 +162,10 @@ | |
<data name="RouteValueDictionary_DuplicatePropertyName" xml:space="preserve"> | ||
<value>The type '{0}' defines properties '{1}' and '{2}' which differ only by casing. This is not supported by {3} which uses case-insensitive comparisons.</value> | ||
</data> | ||
<data name="Exception_KeyedServicesNotSupported" xml:space="preserve"> | ||
<value>This service provider doesn't support keyed services.</value> | ||
</data> | ||
<data name="Exception_NoServiceRegistered" xml:space="preserve"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, there is this one too 😢 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hehe, don't sweat it, should've checked it. Will run by all resources 👍 |
||
<value>No service for type '{0}' has been registered.</value> | ||
</data> | ||
</root> |
Uh oh!
There was an error while loading. Please reload this page.