Dependency Injection method that takes an action with an IServiceProvider parameter

[erichiller]

Continuation from #4802 -- I think it would be consistent to have a dependency injection extension method that takes IServiceCollection. This would be consistent how existing AddX methods on IServiceCollection operate, and would only make use of existing imports (Microsoft.Extensions.DependencyInjection.Extensions).

My use case is for getting the connection string from IConfiguration, but I would think others have other configuration needs that could use this method.

Proposed method:

    /// <summary>
    /// Registers an <see cref="NpgsqlDataSource" /> and an <see cref="NpgsqlConnection" /> in the <see cref="IServiceCollection" />.
    /// </summary>
    /// <param name="serviceCollection">The <see cref="IServiceCollection" /> to add services to.</param>
    /// <param name="connectionString">An Npgsql connection string. Pass <c>null</c> if the connection string will be set within <paramref name="dataSourceBuilderAction"/></param>
    /// <param name="dataSourceBuilderAction">
    /// An action to configure the <see cref="NpgsqlDataSourceBuilder" /> for further customizations of the <see cref="NpgsqlDataSource" />.
    /// </param>
    /// <param name="connectionLifetime">
    /// The lifetime with which to register the <see cref="NpgsqlConnection" /> in the container.
    /// Defaults to <see cref="ServiceLifetime.Scoped" />.
    /// </param>
    /// <param name="dataSourceLifetime">
    /// The lifetime with which to register the <see cref="NpgsqlDataSource" /> service in the container.
    /// Defaults to <see cref="ServiceLifetime.Singleton" />.
    /// </param>
    /// <returns>The same service collection so that multiple calls can be chained.</returns>
    public static IServiceCollection AddNpgsqlDataSource(
        this IServiceCollection                            serviceCollection,
        string?                                            connectionString,
        Action<IServiceProvider, NpgsqlDataSourceBuilder>? dataSourceBuilderAction,
        ServiceLifetime                                    connectionLifetime = ServiceLifetime.Transient,
        ServiceLifetime                                    dataSourceLifetime = ServiceLifetime.Singleton
    ) {
        serviceCollection.TryAdd(
            new ServiceDescriptor(
                typeof(NpgsqlDataSource),
                sp => {
                    var dataSourceBuilder = new NpgsqlDataSourceBuilder( connectionString );
                    dataSourceBuilder.UseLoggerFactory( sp.GetService<ILoggerFactory>() );
                    dataSourceBuilderAction?.Invoke( sp, dataSourceBuilder );
                    return dataSourceBuilder.Build();
                },
                dataSourceLifetime ) );

        AddCommonServices( serviceCollection, connectionLifetime, dataSourceLifetime );

        return serviceCollection;
    }

My usage would be like:

  services.AddNpgsqlDataSource( null, ( serviceProvider, npgsqlDataSourceBuilder ) => {
      npgsqlDataSourceBuilder.ConnectionStringBuilder.ConnectionString = serviceProvider.GetRequiredService<IConfiguration>().GetConnectionString( "myConnectionString" );
      npgsqlDataSourceBuilder.UseNodaTime();
  } );

Let me know if you're interested in a PR.

[roji]

I'm a bit confused: why would you want to do that instead of the shorter and more standard:

services.AddNpgsqlDataSource(
    builder.Configuration.GetConnectionString("SchoolContext"),
    npgsqlDataSourceBuilder => npgsqlDataSourceBuilder.UseNodaTime());

[erichiller]

I have post service container build initialization that takes place and can change which connection string is used, so I'd need to gather the connection string at that later stage.

[roji]

That sounds like a pretty niche scenario, that IMO doesn't justify adding an extension to Npgsql... After all, users aren't blocked in this scenario - we're just talking about adding a little sugar for it.

[kpko]

Hi,

I needed that functionality too and was confused, that this wasn’t implemented yet. The method internally uses ServiceProvider anyways.

The reason we need access to the SP is, that we can’t depend on having an ASP.NET host around where we can grab the configuration from.

An overload with a service provider allows to have a dependency on DI.Abstractions but your solution requires to use a WebApplicationBuilder that has the IConfiguration ready to use.

So the overload would make it possible to add NpgsqlDataSource to any service collection using IConfiguration, the solution with WebApplicationBuilder depends on ASP.NET.

Would you accept a pull request for this?

[roji]

Can you provide a bit of context? I'm interested in understanding scenarios where the connection string (configuration) isn't available when calling AddNpgsqlDataSource, but is available later within the service provider.

One main reason I think this could be a bad idea, is that it would add an overload that is the same as the existing one, except in the parameters that the Action accepts (service provider+data source builder, instead of just the data source builder, following the proposal above). Such overloads mess up Intellisense, meaning that for the standard user who doesn't require the service provider (the 99% case), things become less easy.

[kpko]

Hi @roji, thanks for your quick anwer.

Can you provide a bit of context? I'm interested in understanding scenarios where the connection string (configuration) isn't available when calling AdDNpgsqlDataSource, but is available later within the service provider.

Of course, I would be happy to give you some context.

Currently a call to AddNpgsqlDataSource requires us to have the ConnectionString ready at hand, early in time. This only works in the proposed solution because it is using WebApplicationBuilder, which is only available in ASP.NET Core web applications. WAB is using a hybrid between IConfiguration and ConfigurationBuilder, where you can grab config entries at the time of building the WebApplication. This has two big disadvantages.

First, configuration sources could be added later in time through other DI services, while still building the WebApplication, which makes the order of services.Add-calls important for AddNpgsqlDataSource. This differs from AddX-calls like EntityFrameworks AddDbContext (that uses a Action<IServiceProvider,DbContextOptionsBuilder> for example) and other services in ASP.NET and could lead to some errors. More importantly for us, it also breaks the use of WebApplicationFactory for integration tests (https://learn.microsoft.com/en-us/aspnet/core/test/integration-tests?view=aspnetcore-7.0). WAF makes it possible to use any ASP.NET app and inject further configuration or services for integration test scenarios. In our scenario we wanted to add an in-memory configuration source that adds a ConnectionString for testing.

Second, it depends on using a WebApplicationBuilder which is not necessary for worker services, background jobs, usage in class libraries etc.

The proposed overload makes the grabbing of the ConnectionString lazy and allows it to be used everywhere we have a ServiceCollection, decouples the order of service registrations and makes it possible to be used in integration test scenarios.

One main reason I think this could be a bad idea, is that it would add an overload that is the same as the existing one, except in the parameters that the Action accepts (service provider+data source builder, instead of just the data source builder, following the proposal above). Such overloads mess up Intellisense, meaning that for the standard user who doesn't require the service provider (the 99% case), things become less easy.

I totally get that; this adds more surface area to the extension methods. In my experience this is not a 1% use case though, but instead the use case for everyone that doesn't use WebApplicationBuilder, which for me justifies adding another overload to fix the issues that I pointed out above.

[roji]

@kpko note that there's nothing about the specific API that's specific to ASP.NET or to WebApplicationBuilder; the only thing that's needed is to know the connection string at the point where you're configuring your services (i.e. when you call AddNpgsqlDataSource).

First, configuration sources could be added later in time through other DI services, while still building the WebApplication, which makes the order of services.Add-calls important for AddNpgsqlDataSource.

I don't think that's true - the ordering of Add calls should never be important here. The question is whether you know the connection string before the service provider is built or not; the current method assumes that you do (meaning you can pass it to AddNpgsqlDataSource), whereas you're saying there are scenarios where you don't.

I totally get that; this adds more surface area to the extension methods. In my experience this is not a 1% use case though, but instead the use case for everyone that doesn't use WebApplicationBuilder, which for me justifies adding another overload to fix the issues that I pointed out above.

The problem isn't really adding another overload to the API surface; it's adding an overload that interferes with the existing one (from an Intellisense perspective). I'm especially hesitant since we're only talking about a sugar method - you can easily bypass this and register your service directly without these methods.

But despite the above, I can imagine possible cases here, and EF indeed does have an AddDbContext overload with an Action that accepts an IServiceProvider. I'm reopening this to consider it.

/cc @ajcvickers @NinoFloris

[kpko]

Hi!

@kpko note that there's nothing about the specific API that's specific to ASP.NET or to WebApplicationBuilder; the only thing that's needed is to know the connection string at the point where you're configuring your services (i.e. when you call AddNpgsqlDataSource).

You are right, Npgsql.DI is not tied to WebApplicationBuilder. I just pointed out the example of a workaround was using WebApplicationBuilder:

services.AddNpgsqlDataSource(
    builder.Configuration.GetConnectionString("SchoolContext"),
    npgsqlDataSourceBuilder => npgsqlDataSourceBuilder.UseNodaTime());

This works because we have the ConfigurationManager from WAB and we can retrieve a connection string while still setting up services. Thats what I meant with the order of Add*-calls as well. In the WebApplicationFactory (integration test) scenario, we can AddNpgsqlDataSource before adding another configration source.

The problem isn't really adding another overload to the API surface; it's adding an overload that interferes with the existing one (from an Intellisense perspective). I'm especially hesitant since we're only talking about a sugar method - you can easily bypass this and register your service directly without these methods.

Could you elaborate on that? As far as I understood, there is no method to lazily add the connection string later or did I miss something?

For example, when using the ASP.NET worker template via dotnet new worker, I have the following contents:

IHost host = Host.CreateDefaultBuilder(args)
    .ConfigureServices(services =>
    {
        services.AddHostedService<Worker>();
    })
    .Build();

host.Run();

In this example I can setup my configuration and add the NpgsqlDataSource like this:

IHost host = Host.CreateDefaultBuilder(args)
    .ConfigureServices(services =>
    {
        services.AddHostedService<Worker>();
        services.AddOptions<MyOptions>()
            .ValidateOnStart()
            .ValidateDataAnnotations()
            .BindConfiguration("MySettings");

        services.AddNpgsqlDataSource("???", builder =>
        {
            if (???)
            {
                builder.EnableParameterLogging();
            }
        });
    })
    .Build();

host.Run();

public class MyOptions
{
    [Required]
    public string ConnectionString { get; set; } = string.Empty;
    
    public bool LogSensitiveInformation { get; set; }
}

In this case I'm using the Options pattern, but I could be using host.ConfigureAppConfiguration and adding more configuration sources as well. There is no way to retrieve an instance of my options object (or IConfiguration instance) without building a temporary ServiceProvider out of my ServiceCollection, which is a big anti-pattern.

This example also shows that this is not just happening for the connection string but also other related options like EnableParameterLogging, which could be configured from the HostEnvironment (e.g. only enable parameter logging on dev environments), which is also only available from a service provider in this case.

The proposed overload solves this issue by providing an Action<IServiceProvider, NpgsqlDataSourceBuilder>. This makes it possible to retrieve the service at a time where the ServiceProvider is already built:

services.AddNpgsqlDataSource((provider, builder) =>
{
    var options = provider.GetRequiredService<IOptions<MyOptions>>().Value;
    builder.ConnectionStringBuilder.ConnectionString = options.ConnectionString;
    
    var env = provider.GetRequiredService<IHostEnvironment>();
    if (env.IsDevelopment())
    {
        builder.EnableParameterLogging();
    }
});

I would consider this not just sugar, but adding a method to be able to configure NpgsqlDataSource when a service provider is available v.s. at service collection build time.

But despite the above, I can imagine possible cases here, and EF indeed does have an AddDbContext overload with an Action that accepts an IServiceProvider. I'm reopening this to consider it.

Thank you very much for considering this and reading through my wall of text. 😊

I would add another proposal to give an alternative to messing with the overloads and intellisense that much. I'm not exactly sure how the builder is invoked internally, but would it be possible to just add a ServiceProvider property to the NpgsqlDataSourceBuilder? This way we don't need an overload with an Action<IServiceProvider,NpgsqlDataSourceBuilder> but instead an overload that does not need a connection string, like AddNpgsqlDataSource(Action<NpgsqlDataSourceBuilder>,ServiceLifetime,ServiceLifetime)

Thanks again, looking forward to hear your thoughts on this.

[roji]

I would consider this not just sugar

I just meant in the sense that you can easily implement it yourself. All of Npgsql.DependencyInjection is pretty thin sugar which certainly improves life, but there's nothing here you can't just do yourself directly.

would it be possible to just add a ServiceProvider property to the NpgsqlDataSourceBuilder

NpgsqlDataSourceBuilder is part of Npgsql, which has no knowledge whatsoever of DI. It's true that IServiceProvider is part of .NET and not of Microsoft.Extensions.DependencyInjection, but we generally don't want to start inserting DI-related stuff in there.

Besides, remember that NpgsqlDataSourceBuilder needs to work without an IServiceProvider as well (and I don't think optional service works in constructor injection), and we also don't want to pollute the surface of that type with an IServiceProvider that isn't useful to anyone who isn't using DI etc.

[kpko]

NpgsqlDataSourceBuilder is part of Npgsql, which has no knowledge whatsoever of DI. It's true that IServiceProvider is part of .NET and not of Microsoft.Extensions.DependencyInjection, but we generally don't want to start inserting DI-related stuff in there.

That totally makes sense, I didn't realize NpgsqlDataSourceBuilder was part of Npgsql and not only the DI package. So the initial proposal with another overload (minus ConnectionString, plus Action<IServiceProvider,NpgsqlDataSourceBuilder>) would still be the way to go imho.

Thanks again for looking into this!

[steveoh]

That sounds like a pretty niche scenario, that IMO doesn't justify adding an extension to Npgsql... After all, users aren't blocked in this scenario - we're just talking about adding a little sugar for it.

This is not a niche scenario. People should not be encouraged to hard code database connection strings into code. This is what the configuration system was built for.

Work around (don't use the package)

The DI package is pretty basic so instead of using the extension method, this is equivalent...

services.AddSingleton((provider) => {
    var options = provider.GetService<IOptions<DatabaseConfiguration>>();

    var builder = new NpgsqlDataSourceBuilder(options.Value.ConnectionString);
    builder.UseLoggerFactory(provider.GetService<ILoggerFactory>());
    builder.UseNetTopologySuite();

    return builder.Build();
});

If you want to add the other fancy options it has, then take a look at the AddCommonServices

[roji]

This is not a niche scenario. People should not be encouraged to hard code database connection strings into code.

This is not at all what's being encouraged.

[roji]

I'm going to keep this open in the backlog for now, let's see how many users vote for it. I think adding this could make sense, but since it also hurts Intellisense and can very easily be worked around (without the DI package), I think it's better to get a bit of feedback first.

[PSanetra]

@roji we have another use case for accessing the serviceProvider in the AddNpgsqlDataSource dataSourceBuilderAction parameter.

We need to use the NpgsqlDataSourceBuilder.UsePeriodicPasswordProvider() method to load an AAD access token periodically and set it as password. We provide the provider for that token as a singleton service.

So our usage of AddNpgsqlDataSource with serviceProvider support would look something like this:

serviceCollection.AddNpgsqlDataSource(
    connectionString,
    (serviceProvider, builder) =>
    {
        var tokenProvider = serviceProvider.GetRequiredService<IAzurePostgresTokenProvider>();
        builder.UsePeriodicPasswordProvider(
            async (connectionStringBuilder, cancellationToken) => await tokenProvider.LoadToken(),
            TimeSpan.FromMinutes(15),
            TimeSpan.FromMinutes(1));
    }
);

[myzamaraev]

In my particular scenario I have separate data access library for each type of storage, and I find it very convenient to provide configuration to such libraries through Options pattern and resolve particular IOptions inside the library, e.x. to configure dataSourceBuilder through dataSourceBuilderAction. But in case of dataSourceBuilderAction without service provider available I have to either build entire service provider to just resolve IOptions, or pass IOptions as an additional parameter through the chain of service registration methods.

Nevrtheless I agree with the fact that it's easy to get by without this extension method and register NpgsqlDataSource myself.

[EraYaN]

We have essentially a bunch of IOptions with a lot of input validation (that only gets run when the Option is injected) and we use that to set the database name in an incoming connection string template. Having access to the service provider would make it a lot easier. And we wouldn't have to duplicate the DI package.

[akhakpouri]

@roji pull request 5572 that is supposed to resolve this issue is currently in the open state. Is there any chance your team could vet the PR and merge these changes into the upcoming release?

[roji]

@akhakpouri it's in progress - it's already approved and will indeed go in.