Optimize IReportApiVersions DI resolution

commonsensesoftware · commonsensesoftware · commit d4927a57758a · 2022-12-01T18:35:06.000-08:00
diff --git a/src/AspNet/WebApi/src/Asp.Versioning.WebApi/DefaultApiVersionReporter.cs b/src/AspNet/WebApi/src/Asp.Versioning.WebApi/DefaultApiVersionReporter.cs
@@ -12,7 +12,8 @@ public partial class DefaultApiVersionReporter
 {
     private static DefaultApiVersionReporter? instance;
 
-    internal static IReportApiVersions Instance => instance ??= new();
+    internal static IReportApiVersions GetOrCreate( ISunsetPolicyManager sunsetPolicyManager ) =>
+        instance ??= new( sunsetPolicyManager );
 
     private static void AddApiVersionHeader( HttpResponseHeaders headers, string headerName, IReadOnlyList<ApiVersion> versions )
     {
diff --git a/src/AspNet/WebApi/src/Asp.Versioning.WebApi/DependencyResolverExtensions.cs b/src/AspNet/WebApi/src/Asp.Versioning.WebApi/DependencyResolverExtensions.cs
@@ -13,7 +13,7 @@ internal static IApiVersionParser GetApiVersionParser( this IDependencyResolver
         resolver.GetService<IApiVersionParser>() ?? ApiVersionParser.Default;
 
     internal static IReportApiVersions GetApiVersionReporter( this IDependencyResolver resolver ) =>
-        resolver.GetService<IReportApiVersions>() ?? DefaultApiVersionReporter.Instance;
+        resolver.GetService<IReportApiVersions>() ?? DefaultApiVersionReporter.GetOrCreate( resolver.GetSunsetPolicyManager() );
 
     internal static IControllerNameConvention GetControllerNameConvention( this IDependencyResolver resolver ) =>
         resolver.GetService<IControllerNameConvention>() ?? ControllerNameConvention.Default;
diff --git a/src/AspNet/WebApi/test/Asp.Versioning.WebApi.Tests/DefaultApiVersionReporterTest.cs b/src/AspNet/WebApi/test/Asp.Versioning.WebApi.Tests/DefaultApiVersionReporterTest.cs
@@ -5,7 +5,6 @@ namespace Asp.Versioning;
 using Asp.Versioning.Simulators;
 using System.Web.Http;
 using System.Web.Http.Controllers;
-using System.Web.Http.Dependencies;
 using static System.Net.HttpStatusCode;
 
 public class DefaultApiVersionReporterTest
@@ -14,9 +13,8 @@ public class DefaultApiVersionReporterTest
     public void report_should_add_expected_headers()
     {
         // arrange
-        var reporter = new DefaultApiVersionReporter();
         var sunsetDate = DateTimeOffset.Now;
-        var dependencyResolver = new Mock<IDependencyResolver>();
+        var reporter = new DefaultApiVersionReporter( new TestSunsetPolicyManager( sunsetDate ) );
         var configuration = new HttpConfiguration();
         var request = new HttpRequestMessage();
         var response = new HttpResponseMessage( OK ) { RequestMessage = request };
@@ -34,9 +32,6 @@ public void report_should_add_expected_headers()
             deprecatedAdvertisedVersions: Enumerable.Empty<ApiVersion>() );
         var metadata = new ApiVersionMetadata( apiModel, endpointModel, "Test" );
 
-        dependencyResolver.Setup( dr => dr.GetService( typeof( ISunsetPolicyManager ) ) )
-                          .Returns( new TestSunsetPolicyManager( sunsetDate ) );
-        configuration.DependencyResolver = dependencyResolver.Object;
         request.SetConfiguration( configuration );
         request.ApiVersionProperties().RequestedApiVersion = new ApiVersion( 1.0 );
         request.Properties["MS_HttpActionDescriptor"] =
diff --git a/src/AspNetCore/WebApi/src/Asp.Versioning.Http/Builder/EndpointBuilderFinalizer.cs b/src/AspNetCore/WebApi/src/Asp.Versioning.Http/Builder/EndpointBuilderFinalizer.cs
@@ -52,7 +52,10 @@ private static void Finialize( EndpointBuilder endpointBuilder, ApiVersionSet? v
         if ( reportApiVersions )
         {
             requestDelegate = EnsureRequestDelegate( requestDelegate, endpointBuilder.RequestDelegate );
-            requestDelegate = new ReportApiVersionsDecorator( requestDelegate, metadata );
+
+            var reporter = services.GetRequiredService<IReportApiVersions>();
+
+            requestDelegate = new ReportApiVersionsDecorator( requestDelegate, reporter, metadata );
             endpointBuilder.RequestDelegate = requestDelegate;
         }
 
diff --git a/src/AspNetCore/WebApi/src/Asp.Versioning.Http/Routing/ReportApiVersionsDecorator.cs b/src/AspNetCore/WebApi/src/Asp.Versioning.Http/Routing/ReportApiVersionsDecorator.cs
@@ -3,35 +3,32 @@
 namespace Asp.Versioning.Routing;
 
 using Microsoft.AspNetCore.Http;
-using Microsoft.Extensions.DependencyInjection;
 
 internal sealed class ReportApiVersionsDecorator
 {
     private readonly RequestDelegate decorated;
-    private readonly ApiVersionMetadata metadata;
+    private readonly IReportApiVersions reporter;
+    private readonly ApiVersionModel model;
 
-    public ReportApiVersionsDecorator( RequestDelegate decorated, ApiVersionMetadata metadata )
+    public ReportApiVersionsDecorator( RequestDelegate decorated, IReportApiVersions reporter, ApiVersionMetadata metadata )
     {
         this.decorated = decorated;
-        this.metadata = metadata;
+        this.reporter = reporter;
+        model = metadata.Map( reporter.Mapping );
     }
 
     public static implicit operator RequestDelegate( ReportApiVersionsDecorator decorator ) =>
         ( context ) =>
         {
-            var reporter = context.RequestServices.GetRequiredService<IReportApiVersions>();
-            var model = decorator.metadata.Map( reporter.Mapping );
             var response = context.Response;
-
-            response.OnStarting( ReportApiVersions, (reporter, response, model) );
-
+            response.OnStarting( ReportApiVersions, (decorator, response) );
             return decorator.decorated( context );
         };
 
     private static Task ReportApiVersions( object state )
     {
-        var (reporter, response, model) = ((IReportApiVersions, HttpResponse, ApiVersionModel)) state;
-        reporter.Report( response, model );
+        var (decorator, response) = ((ReportApiVersionsDecorator, HttpResponse)) state;
+        decorator.reporter.Report( response, decorator.model );
         return Task.CompletedTask;
     }
 }
diff --git a/src/AspNetCore/WebApi/test/Asp.Versioning.Http.Tests/DefaultApiVersionReporterTest.cs b/src/AspNetCore/WebApi/test/Asp.Versioning.Http.Tests/DefaultApiVersionReporterTest.cs
@@ -12,8 +12,8 @@ public class DefaultApiVersionReporterTest
     public void report_should_add_expected_headers()
     {
         // arrange
-        var reporter = new DefaultApiVersionReporter();
         var sunsetDate = DateTimeOffset.Now;
+        var reporter = new DefaultApiVersionReporter( new TestSunsetPolicyManager( sunsetDate ) );
         var httpContext = new Mock<HttpContext>();
         var features = new Mock<IFeatureCollection>();
         var query = new Mock<IQueryCollection>();
@@ -50,7 +50,6 @@ public void report_should_add_expected_headers()
         response.SetupGet( r => r.HttpContext ).Returns( () => httpContext.Object );
         serviceProvider.Setup( sp => sp.GetService( typeof( IApiVersionParser ) ) ).Returns( ApiVersionParser.Default );
         serviceProvider.Setup( sp => sp.GetService( typeof( IApiVersionReader ) ) ).Returns( new QueryStringApiVersionReader() );
-        serviceProvider.Setup( sp => sp.GetService( typeof( ISunsetPolicyManager ) ) ).Returns( new TestSunsetPolicyManager( sunsetDate ) );
         httpContext.SetupGet( c => c.Features ).Returns( features.Object );
         httpContext.SetupGet( c => c.Request ).Returns( request.Object );
         httpContext.SetupProperty( c => c.RequestServices, serviceProvider.Object );
diff --git a/src/AspNetCore/WebApi/test/Asp.Versioning.Mvc.Tests/ReportApiVersionsAttributeTest.cs b/src/AspNetCore/WebApi/test/Asp.Versioning.Mvc.Tests/ReportApiVersionsAttributeTest.cs
@@ -81,15 +81,14 @@ private static ActionExecutingContext CreateContext(
         var actionArguments = new Dictionary<string, object>();
         var controller = default( object );
         var endpoint = new Endpoint( c => Task.CompletedTask, new( new[] { metadata } ), "Test" );
+        var options = Options.Create( new ApiVersioningOptions() );
+        var reporter = new DefaultApiVersionReporter( new SunsetPolicyManager( options ) );
 
         endpointFeature.SetupProperty( f => f.Endpoint, endpoint );
         versioningFeature.SetupProperty( f => f.RequestedApiVersion, new ApiVersion( 1.0 ) );
         features.Set( endpointFeature.Object );
         features.Set( versioningFeature.Object );
-        serviceProvider.Setup( sp => sp.GetService( typeof( IReportApiVersions ) ) )
-                       .Returns( new DefaultApiVersionReporter() );
-        serviceProvider.Setup( sp => sp.GetService( typeof( ISunsetPolicyManager ) ) )
-                       .Returns( new SunsetPolicyManager( Options.Create( new ApiVersioningOptions() ) ) );
+        serviceProvider.Setup( sp => sp.GetService( typeof( IReportApiVersions ) ) ).Returns( reporter );
         response.SetupGet( r => r.Headers ).Returns( headers );
         response.SetupGet( r => r.HttpContext ).Returns( () => httpContext.Object );
         response.Setup( r => r.OnStarting( It.IsAny<Func<object, Task>>(), It.IsAny<object>() ) )
diff --git a/src/Common/src/Common.Mvc/ReportApiVersionsAttribute.cs b/src/Common/src/Common.Mvc/ReportApiVersionsAttribute.cs
@@ -27,7 +27,5 @@ public ReportApiVersionsAttribute() { }
     /// Initializes a new instance of the <see cref="ReportApiVersionsAttribute"/> class.
     /// </summary>
     /// <param name="reportApiVersions">The <see cref="IReportApiVersions">object</see> used to report API versions.</param>
-#pragma warning disable CA1019 // Define accessors for attribute arguments
     public ReportApiVersionsAttribute( IReportApiVersions reportApiVersions ) => this.reportApiVersions = reportApiVersions;
-#pragma warning restore CA1019 // Define accessors for attribute arguments
 }
diff --git a/src/Common/src/Common/DefaultApiVersionReporter.cs b/src/Common/src/Common/DefaultApiVersionReporter.cs
@@ -7,7 +7,6 @@ namespace Asp.Versioning;
 using HttpResponse = System.Net.Http.HttpResponseMessage;
 #else
 using Microsoft.AspNetCore.Http;
-using Microsoft.Extensions.DependencyInjection;
 #endif
 using static Asp.Versioning.ApiVersionMapping;
 
@@ -23,24 +22,27 @@ public sealed partial class DefaultApiVersionReporter : IReportApiVersions
     private const string ApiDeprecatedVersions = "api-deprecated-versions";
     private const string Sunset = nameof( Sunset );
     private const string Link = nameof( Link );
+    private readonly ISunsetPolicyManager sunsetPolicyManager;
     private readonly string apiSupportedVersionsName;
     private readonly string apiDeprecatedVersionsName;
 
     /// <summary>
     /// Initializes a new instance of the <see cref="DefaultApiVersionReporter"/> class.
     /// </summary>
+    /// <param name="sunsetPolicyManager">The <see cref="ISunsetPolicyManager">manager</see> used to resolve sunset policies.</param>
     /// <param name="supportedHeaderName">The HTTP header name used for supported API versions.
     /// The default value is "api-supported-versions".</param>
     /// <param name="deprecatedHeaderName">THe HTTP header name used for deprecated API versions.
     /// The default value is "api-deprecated-versions".</param>
     /// <param name="mapping">One or more of API versioning mappings. The default value is
     /// <see cref="ApiVersionMapping.Explicit"/> and <see cref="ApiVersionMapping.Implicit"/>.</param>
     public DefaultApiVersionReporter(
+        ISunsetPolicyManager sunsetPolicyManager,
         string supportedHeaderName = ApiSupportedVersions,
         string deprecatedHeaderName = ApiDeprecatedVersions,
         ApiVersionMapping mapping = Explicit | Implicit )
     {
-        Mapping = mapping;
+        this.sunsetPolicyManager = sunsetPolicyManager ?? throw new ArgumentNullException( nameof( sunsetPolicyManager ) );
 
         if ( string.IsNullOrEmpty( apiSupportedVersionsName = supportedHeaderName ) )
         {
@@ -51,6 +53,8 @@ public DefaultApiVersionReporter(
         {
             throw new ArgumentNullException( nameof( deprecatedHeaderName ) );
         }
+
+        Mapping = mapping;
     }
 
     /// <inheritdoc />
@@ -86,8 +90,6 @@ public void Report( HttpResponse response, ApiVersionModel apiVersionModel )
             return;
         }
 
-        var name = metadata.Name;
-        var policyManager = request.GetConfiguration().DependencyResolver.GetSunsetPolicyManager();
         var version = request.GetRequestedApiVersion();
 #else
         var context = response.HttpContext;
@@ -97,14 +99,13 @@ public void Report( HttpResponse response, ApiVersionModel apiVersionModel )
             return;
         }
 
-        var name = metadata.Name;
-        var policyManager = context.RequestServices.GetRequiredService<ISunsetPolicyManager>();
         var version = context.GetRequestedApiVersion();
 #endif
+        var name = metadata.Name;
 
-        if ( policyManager.TryGetPolicy( name, version, out var policy ) ||
-           ( !string.IsNullOrEmpty( name ) && policyManager.TryGetPolicy( name, out policy ) ) ||
-           ( version != null && policyManager.TryGetPolicy( version, out policy ) ) )
+        if ( sunsetPolicyManager.TryGetPolicy( name, version, out var policy ) ||
+           ( !string.IsNullOrEmpty( name ) && sunsetPolicyManager.TryGetPolicy( name, out policy ) ) ||
+           ( version != null && sunsetPolicyManager.TryGetPolicy( version, out policy ) ) )
         {
             response.WriteSunsetPolicy( policy );
         }

Original file line number	Diff line number	Diff line change
`@@ -12,7 +12,8 @@ public partial class DefaultApiVersionReporter`
`12`	`12`	`{`
`13`	`13`	`private static DefaultApiVersionReporter? instance;`
`14`	`14`
`15`		`- internal static IReportApiVersions Instance => instance ??= new();`
	`15`	`+ internal static IReportApiVersions GetOrCreate( ISunsetPolicyManager sunsetPolicyManager ) =>`
	`16`	`+ instance ??= new( sunsetPolicyManager );`
`16`	`17`
`17`	`18`	`private static void AddApiVersionHeader( HttpResponseHeaders headers, string headerName, IReadOnlyList<ApiVersion> versions )`
`18`	`19`	`{`
Original file line number	Diff line number	Diff line change
`@@ -52,7 +52,10 @@ private static void Finialize( EndpointBuilder endpointBuilder, ApiVersionSet? v`
`52`	`52`	`if ( reportApiVersions )`
`53`	`53`	`{`
`54`	`54`	`requestDelegate = EnsureRequestDelegate( requestDelegate, endpointBuilder.RequestDelegate );`
`55`		`- requestDelegate = new ReportApiVersionsDecorator( requestDelegate, metadata );`
	`55`	`+`
	`56`	`+ var reporter = services.GetRequiredService<IReportApiVersions>();`
	`57`	`+`
	`58`	`+ requestDelegate = new ReportApiVersionsDecorator( requestDelegate, reporter, metadata );`
`56`	`59`	`endpointBuilder.RequestDelegate = requestDelegate;`
`57`	`60`	`}`
`58`	`61`
Original file line number	Diff line number	Diff line change
`@@ -3,35 +3,32 @@`
`3`	`3`	`namespace Asp.Versioning.Routing;`
`4`	`4`
`5`	`5`	`using Microsoft.AspNetCore.Http;`
`6`		`-using Microsoft.Extensions.DependencyInjection;`
`7`	`6`
`8`	`7`	`internal sealed class ReportApiVersionsDecorator`
`9`	`8`	`{`
`10`	`9`	`private readonly RequestDelegate decorated;`
`11`		`- private readonly ApiVersionMetadata metadata;`
	`10`	`+ private readonly IReportApiVersions reporter;`
	`11`	`+ private readonly ApiVersionModel model;`
`12`	`12`
`13`		`- public ReportApiVersionsDecorator( RequestDelegate decorated, ApiVersionMetadata metadata )`
	`13`	`+ public ReportApiVersionsDecorator( RequestDelegate decorated, IReportApiVersions reporter, ApiVersionMetadata metadata )`
`14`	`14`	`{`
`15`	`15`	`this.decorated = decorated;`
`16`		`- this.metadata = metadata;`
	`16`	`+ this.reporter = reporter;`
	`17`	`+ model = metadata.Map( reporter.Mapping );`
`17`	`18`	`}`
`18`	`19`
`19`	`20`	`public static implicit operator RequestDelegate( ReportApiVersionsDecorator decorator ) =>`
`20`	`21`	`( context ) =>`
`21`	`22`	`{`
`22`		`- var reporter = context.RequestServices.GetRequiredService<IReportApiVersions>();`
`23`		`- var model = decorator.metadata.Map( reporter.Mapping );`
`24`	`23`	`var response = context.Response;`
`25`		`-`
`26`		`- response.OnStarting( ReportApiVersions, (reporter, response, model) );`
`27`		`-`
	`24`	`+ response.OnStarting( ReportApiVersions, (decorator, response) );`
`28`	`25`	`return decorator.decorated( context );`
`29`	`26`	`};`
`30`	`27`
`31`	`28`	`private static Task ReportApiVersions( object state )`
`32`	`29`	`{`
`33`		`- var (reporter, response, model) = ((IReportApiVersions, HttpResponse, ApiVersionModel)) state;`
`34`		`- reporter.Report( response, model );`
	`30`	`+ var (decorator, response) = ((ReportApiVersionsDecorator, HttpResponse)) state;`
	`31`	`+ decorator.reporter.Report( response, decorator.model );`
`35`	`32`	`return Task.CompletedTask;`
`36`	`33`	`}`
`37`	`34`	`}`
Original file line number	Diff line number	Diff line change
`@@ -27,7 +27,5 @@ public ReportApiVersionsAttribute() { }`
`27`	`27`	`/// Initializes a new instance of the <see cref="ReportApiVersionsAttribute"/> class.`
`28`	`28`	`/// </summary>`
`29`	`29`	`/// <param name="reportApiVersions">The <see cref="IReportApiVersions">object</see> used to report API versions.</param>`
`30`		`-#pragma warning disable CA1019 // Define accessors for attribute arguments`
`31`	`30`	`public ReportApiVersionsAttribute( IReportApiVersions reportApiVersions ) => this.reportApiVersions = reportApiVersions;`
`32`		`-#pragma warning restore CA1019 // Define accessors for attribute arguments`
`33`	`31`	`}`