Open captainsafia opened 2 months ago
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:
Couple comments:
ILogger<ApiDescriptionGroupCollectionProvider>
instead of ILoggerFactory
?IServiceProvider
instead so any future additions wouldn't require API additions.I think I'm probably missing something obvious (and it's probably too late anyway), but why is ApiDescriptionGroupCollectionProvider
public? Relatedly, should it be sealed?
Should we use ILogger
instead of ILoggerFactory?
Good point -- we seem to use ILoggerFactory
more frequently in the MVC codepath although I think that's an probably an MVC artifact.
Should we use ILogger
instead of ILoggerFactory?
That's an option -- I dunno if we'd need something more in the future and I prefer the straightforwardness of taking an ILogger.
We could also make a new private implementation that adds the new ctor and is added to DI via our extension methods. This would require 0 new API
Yep, that's the alternative design that was proposed above. I'm fine with this approach if we're feelin particularly prickly about introducing new API.
I think I'm probably missing something obvious (and it's probably too late anyway), but why is ApiDescriptionGroupCollectionProvider public? Relatedly, should it be sealed?
It's an old API that was never reviewed so I suspect that there isn't a ton of reason for this. From my knowledge of the area, I don't see a reason to have it be public or extendable (so we can seal it).
One upside of ILoggerFactory
is that it's useful if we need to pass it to another subcomponent that logs to a different category. I'm not a big fan of taking an IServiceProvider
parameter if we have a good idea of what specific services we need and we don't expect it to change much.
I'm in favor of either approving the API as proposed or making no changes and doing the internal service "Impl" pattern as I like to call it. If this were new API, I think the service would be internal and sealed.
One thing to note about the "Impl" pattern is that AddGrpcSwagger()
calls the constructor on the public implementation type from another assembly.
Assuming we want to add logging in that case as well, we might have to add public constructor. I was going to suggest updating AddGrpcSwagger()
to call AddApiExplorer()
, but I don't think we want AddGrpcSwagger()
to add MVC's DefaultApiDescriptionProvider
.
Assuming we want to add logging in that case as well, we might have to add public constructor. I was going to suggest updating AddGrpcSwagger() to call AddApiExplorer(), but I don't think we want AddGrpcSwagger() to add MVC's DefaultApiDescriptionProvider.
Yep, I suspect this is the reason the gRPC implementation went the route of registering the ApiExplorer dependencies themselves. Given that this feature helps with diagnosability, I think we should make an effort to enable it for all scenarios where ApiExplorer
might be used.
We can't take back the fact that we made this a public API but supporting log for both in-framework and out-of-framework consumes seems like goodness.
I've slept on this. I still think making the ILoggerFactory
change a public API is the right way to go but I'd advocate for doing this as a .NET 10 change instead of for .NET 9 RC 1. We can ship the logging changes with an internal API for .NET 9 RC 1 and then introduce the public API after we branch.
FWIW, none of my concerns about the API relate to the timing - it's a pragmatic compromise whether it's added in 9 or 10.
API Review Notes:
ILogger<ApiDescriptionGroupCollectionProvider>
, ILoggerFactory
and IServiceProvider
, we choose ILoggerFactory
.API approved as propsed.
public class ApiDescriptionGroupCollectionProvider : IApiDescriptionGroupCollectionProvider
{
+ public ApiDescriptionGroupCollectionProvider(
+ IActionDescriptorCollectionProvider actionDescriptorCollectionProvider,
+ IEnumerable<IApiDescriptionProvider> apiDescriptionProviders,
+ ILoggerFactory loggerFactory)
}
Background and Motivation
To improve the diagnosability of the OpenAPI layer, we'd like to provide logs so that users can understand what components are contributing custom
IApiDescriptionProvider
implementations to theApiExplorer
layer. In order to support this scenario, we need to support injecting anILoggerFactory
instance to theApiDescriptionGroupCollectionProvider
class that invokes all the discoveredIApiDescriptionProvider
instances.Proposed API
Alternative Designs
ApiDescriptionGroupCollectionProvider
and supportILoggerFactory
injection in its constructors.