Add ConfigureDataSource() under UseNpgsql()

[roji]

#2400 added a UseNpgsql overload which accepts a built DbDataSource, but that requires users to do the building outside the EF call, get a data source, and pass that in. This isn't a great experience.

Experiment with having a UseNpgsql overload which accepts an NpgsqlDataSourceBuilder lambda, allowing configuring the data source in-place. The potential problem is the Intellisense ambiguity with the overload accepting a DbContextOptionsBuilder lambda.

Another advantage of a NpgsqlDataSourceBuilder lambda is that EF plugins (NodaTime, NTS) would be able to apply ADO-level mapping plugins to the data source builder before the data source is built, at least in theory. That would provide a seamless experience where you just add NTS at the EF level, and don't have to worry about the ADO.NET/data source level.

[roji]

Some design thoughts:

• One option is to have UseNpgsql accept an Action<NpgsqlDataSourceBuilder>.
  • That creates some Intellisense issues since we don't know which overload is being used - with NpgsqlDataSourceBuilder or with NpgsqlDbContextOptionsBuilder. Not an amazing experience.
  • Also, we'd need an overload with both Action<NpgsqlDataSourceBuilder> and Action<NpgsqlDbContextOptionsBuilder>, which requires the user to understand the different layers (config for ADO.NET, config for EF). Not amazing.
• Instead, we can just make the ADO.NET config accessible via NpgsqlDbContextOptionsBuilder; either by exposing NpgsqlDataSourceBuilder on it, or better yet, flattening the data source builder options on it (duplicating the NpgsqlDataSourceBuilder API).
• We could theoretically even make NpgsqlDbContextOptionsBuilder extend NpgsqlDataSourceBuilder, but then we can't extend RelationalDbContextOptionsBuilder. It would also mean that NpgsqlDbContextOptionsBuilder would have Build() and BuildMultiHost(), not amazing.
• Assuming we duplicate NpgsqlDataSourceBuilder APIs on NpgsqlDbContextOptionsBuilder... note that the latter still doesn't implement INpgsqlTypeMapper, so the ADO.NET UseNodaTime wouldn't light up on it - only the EF one, which is good. This could be a theoretical problem if someone needs to add an ADO.NET plugin on the data source, rather than an EF plugin (that's totally theoretical).
• There's the question of whether we should use NpgsqlDataSourceBuilder even for the regular, connection-string UseNpgsql. That would allow us to make the EF plugin APIs just work in that case as well; it's a behavioral change though, since every options now creates a new data source (pool), as opposed to reusing the same pool before. It does seem more correct though, and if someone has an issue and wants to share the same pool across different options etc., they can provide an NpgsqlDataSource directly.
• If an NpgsqlConnection is given, though, we obviously can't do NpgsqlDataSourceBuilder; same with an externally-provided NpgsqlDataSource. In those cases, if EF plugins are used, we maybe should warn that it's the users responsibility to set up the ADO.NET plugins beforehand.
• This all paves the way for saner enum mappings too. The user would indicate the enum mapping on NpgsqlDbContextOptionsBuilder (just like AddNodaTime), and that would affect both the EF level and the ADO.NET level.

[roji]

Blocked on dotnet/efcore#29489.

[roji]

Discussion with EF team: we could count the number of NpgsqlDataSources instantiated via the NpgsqlDataSourceBuilder overload, and warn/throw when that crosses a certain threshold. This is similar to the service provider error EF itself has.

This works well, until we get to using NpgsqlDataSourceBuilder internally even for the regular, connection-string UseNpgsql (this is something we want to do since it allows us to configure type mapping plugins properly). To make this work, we'd need to internally track all the NpgsqlDataSources we've instantiated, keyed by their connection string (this effectively redoes Npgsql's internal pool manager (conceptually a dictionary of connection strings to pools/data sources) outside of Npgsql. It would also not support varying type mapping plugins on the same connection strings (it would be hard to even throw when such a config discrepancy is done).

So it may be better to leave the connection string UseNpgsql alone, requiring users to continue doing global type mapping as today (that would also mean un-obsoleting GlobalTypeMapper). But the recommended way would be via NpgsqlDataSourceBuilder.

/cc @ajcvickers

[pinkfloydx33]

Regardless of the decision here... I wonder if an analyzer would be appropriate in order to steer users away from constructing the datasource/builder inside of the AddDbContext callback. A fixer (or the diagnostic message) could suggest the appropriate API or pattern depending on where this issue lands. I'm not sure if there's currently any analyzers for efcore.pg (a quick search yielded none) but this seems like it would be a good candidate...

[roji]

@pinkfloydx33 that's not a bad idea - opened #2588 to track that.

[Khitiara]

worth noting that all the efcore AddDbContextWhatever methods have an overload where the configure delegate is permitted to take an IServiceProvider, allowing for example

builder.Services.AddDbContext<ApplicationDbContext>((provider, options) =>
        options.UseNpgsql(provider.GetRequiredService<NpgsqlDataSource>(), o => {  }));

which is easily usable alongside Npgsql.DependencyInjection

[bkoelman]

Possibly related: I found that replacing:

string? connectionString = builder.Configuration.GetConnectionString("Default");
builder.Services.AddDbContext<AppDbContext>(options => options.UseNpgsql(connectionString));

with:

    string? connectionString = builder.Configuration.GetConnectionString("Default");
    builder.Services.AddNpgsqlDataSource(connectionString);
    builder.Services.AddDbContext<AppDbContext>(options => options.UseNpgsql());

works in a regular application.

But not in our Xunit integration tests (thousands) that run in parallel. Running a single test succeeds. But when running them all, about 45% of them fail with errors like:

Npgsql.PostgresException (0x80004005): 3D000: database "JsonApiTest-e2efce49343f412ebf394cddc447e688" does not exist",
          "",
          "DETAIL: It seems to have just been dropped or renamed.

and:

Npgsql.NpgsqlException: No password has been provided but the backend requires one (in SASL/SCRAM-SHA-256)

All tests share a single docker postgres server instance, but each test uses WebApplicationFactory, which has its own ServiceCollection where we create a database with a randomized name. It seems there's some shared state that lives outside of the lifetime of the service container.

[roji]

@bkoelman this shouldn't be the case... using NpgsqlDataSource with may cause EF to complain that too many (internal) service providers are being created in the tests, since different NpgsqlDataSources (currently) require different service providers (this is different from using connection strings directly, and it may change in the future). However, just using different data sources should trigger errors such as the above.

For the "database seems to have just been dropped or renamed", that looks like a simple case of trying to use the same PG database concurrently from multiple tests - you'll need to check how you manage your database names etc. As to how you're ending up with a connection string lacking a password (second error), that also is likely related to your specific test setup.

If you can't figure it out, try putting together some sort of minimal code repro and I'll try to help.

[Millarex]

I’m not entirely sure about this idea, but I think it might work.
Use NpgsqlDbContextOptionsBuilder for side effect add Extension mapper (as sample NpgsqlNodaTimeDbContextOptionsBuilderExtensions class) and build dataSource in UseNpgsql Method (NpgsqlDbContextOptionsBuilderExtensions class).
Such an approach could help to make logic more compact and avoid double extension registration.

Sorry my English isn’t as good as I’d like it to be.

Di Ef core Context Init:

private static void AddDbContextEfCore (IServiceCollection services, IConfiguration configuration)
{
        var dsRaw = new NpgsqlDataSourceBuilder(configuration.GetConnectionString("DB_connection"));
        services.AddDbContext<DbContext>((_, options) =>
        {
            options.UseNpgsqlDsBuilder(dsRaw, x =>
            {
                x.UseMyNodaTime(dsRaw);
                x.UseMyNetTopologySuite(dsRaw);
                x.MigrationsHistoryTable("__EFMigrationsHistory", "public");
                x.MigrationsAssembly("Migrations");
            });
        });
}

Logic in NpgsqlDbContextOptionsBuilderExtensions:

public static DbContextOptionsBuilder UseNpgsqlDsBuilder(
       this DbContextOptionsBuilder optionsBuilder,
       NpgsqlDataSourceBuilder builder,
       Action<NpgsqlDbContextOptionsBuilder>? npgsqlOptionsAction = null)
   {
       Check.NotNull(optionsBuilder, nameof(optionsBuilder));
       Check.NotNull(dataSource, nameof(dataSource));

       //side effect call builder.UseNetTopologySuite() and builder.UseNodaTime() and etc. 
       npgsqlOptionsAction?.Invoke(new NpgsqlDbContextOptionsBuilder(optionsBuilder));
       var dataSource = builder.Build();

       var extension = (NpgsqlOptionsExtension)GetOrCreateExtension(optionsBuilder).WithDataSource(dataSource);
       ((IDbContextOptionsBuilderInfrastructure)optionsBuilder).AddOrUpdateExtension(extension);

       ConfigureWarnings(optionsBuilder);

       //Old Invoke method position
       //npgsqlOptionsAction?.Invoke(new NpgsqlDbContextOptionsBuilder(optionsBuilder));

       return optionsBuilder;
   }

Possible NodaTime or etc Extensions logic:

public static NpgsqlDbContextOptionsBuilder UseNodaTime(
        this NpgsqlDbContextOptionsBuilder optionsBuilder,
        NpgsqlDataSourceBuilder? builder = null)
    {
        Check.NotNull(optionsBuilder, nameof(optionsBuilder));
	
	//Register Mapper in NpgsqlDataSourceBuilder
	builder?.UseNodaTime();
 
        NpgsqlConnection.GlobalTypeMapper.UseNodaTime();
        var coreOptionsBuilder = ((IRelationalDbContextOptionsBuilderInfrastructure)optionsBuilder).OptionsBuilder;
        var extension = coreOptionsBuilder.Options.FindExtension<NpgsqlNodaTimeOptionsExtension>()
                        ?? new NpgsqlNodaTimeOptionsExtension();
        ((IDbContextOptionsBuilderInfrastructure)coreOptionsBuilder).AddOrUpdateExtension(extension);
       
        return optionsBuilder;
    }

[roji]

Discussed this with @NinoFloris recently, the API proposed here is:

UseNpgsql("<connection string>", o => o
    .MapEnum<Foo>() // This is EF config on NpgsqlDbContextOptionsBuilder
    .DataSourceBuilder
        .EnableDynamicJson()
        .UseClientCertificate(...));

In other words, we'd add an NpgsqlDataSourceBuilder property on NpgsqlDbContextOptionsBuilder, which the user can use to configure all non-EF Npgsql things.

Note that in the above code, MapEnum is an EFCore.PG API (introduced in #3167); since EF has access here to the NpgsqlDataSourceBuilder, this EF MapEnum can cause MapEnum on the lower-level NpgsqlDataSourceBuilder to be called as well, allowing the user to configure the enum at both EFCore.PG and Npgsql levels.

One issue with this model, is that EF relies on being able to compare context options as a way to decide whether a new internal service provider is needed ("singleton options"). This works well with ints and strings, but an NpgsqlDataSourceBuilder property cannot be compared, and therefore this API is incompatible with differing NpgsqlDataSourceBuilder configurations (in the DbContext's OnConfiguring).

The API is safe in "factory-style" configuration (which is what happens e.g. with DI), and is also safe in OnConfiguring as long as the NpgsqlDataSourceBuilder config doesn't change. I think it's reasonable to introduce this with a documentation warning, and to work on the EF side to allow us to detect that UseNpgsql() is being called from OnConfiguring, triggering a warning.

/cc @NinoFloris @ajcvickers

[roji]

Implemented this via #3277, but going with a method+lambda approach:

UseNpgsql("<connection string>", o => o
    .MapEnum<Foo>() // This is EF config on NpgsqlDbContextOptionsBuilder
    .ConfigureDataSource(b => b
        .EnableDynamicJson()
        .UseClientCertificate(...)));

(this allows for easily using statements when configuring the data source etc.)