Test Results

    2 files  ±0      2 suites  ±0   41s ⏱️ -3s
5 564 tests ±0  5 232 ✅ ±0  332 💤 ±0  0 ❌ ±0 
6 082 runs  ±0  5 486 ✅ ±0  596 💤 ±0  0 ❌ ±0 

Results for commit 85cf9c8. ± Comparison against base commit 039466a.

♻️ This comment has been updated with latest results.

The main reason is to avoid constructor ambiguity in DI, but I agree that keeping the old ctor is better for backward compatibility and gives users a clear obsolete warning instead of a breaking change.

I can rework this so the existing IConnectionStringAccessor constructor is kept and marked obsolete, delegating to the connection-string based factory path. The new IMigrationConnectionFactory path will be used by the service registrations / internal construction path, while direct callers of the old constructor will get an obsolete warning.

Just to give some context: I need this functionality quite urgently in my current project, which is why I started the implementation. The ability to provide a fully configured connection through WithConnectionFactory(...) would solve an immediate issue for me, but I still want to make sure the design is aligned with FluentMigrator before going further.

I’m happy to update the PR based on your feedback.

The main thing I never settled is whether we should use the existing DbDataSource. dotnet/runtime#64812 If you look, someone contributed a related (still open) PR but I never pulled it in because I never spent the brain cycles to compare and contrast the two approaches, and there wasn't a ton of demands. But I absolutely am open to adding this, possibly this week, as I'm on a stay at home vacation right now.

See #2002

Good point. I had not fully considered DbDataSource.

This PR uses IMigrationConnectionFactory mainly because it works for all current provider targets and allows the runner to receive a ready IDbConnection regardless of whether it came from a connection string, token callback, NpgsqlDataSource, SqlConnection configuration, tenant resolution, etc.

But I agree DbDataSource is probably the more idiomatic ADO.NET direction where it is available. It represents a configured data source/pool and is intended to hand out ready connections, while DbProviderFactory is more of a provider object factory. The runtime proposal explicitly argues for keeping DbProviderFactory and DbDataSource separate because they represent different concepts.

Maybe the best approach is to support both:

• Keep IMigrationConnectionFactory as the internal abstraction used by processors.
• Add a WithConnectionFactory(...) overload for simple/custom connection creation.
• Add a WithDataSource(...) overload when callers already have a DbDataSource.
• The DbDataSource overload can adapt to IMigrationConnectionFactory internally by calling dataSource.CreateConnection().

Something like:

public static IMigrationRunnerBuilder WithDataSource(
    this IMigrationRunnerBuilder builder,
    Func<IServiceProvider, DbDataSource> dataSourceFactory)
{
    return builder.WithConnectionFactory((sp, _) =>
    {
        var dataSource = dataSourceFactory(sp);
        return dataSource.CreateConnection();
    });
}

Regarding #2002, I checked it. My understanding is that it solves the PostgreSQL dynamic password case by allowing a custom DbProviderFactory to be supplied for Postgres only. It looks intentionally minimal and delegates the dynamic password behavior to Npgsql. I also noticed the note there that DbProviderFactory may be reasonable internally but may not be the best public API surface.

That is why I was thinking the current PR could be a more general version of the same idea:

• Feature/custom db provider factory #2002: provider-specific, Postgres-only, custom DbProviderFactory
• this PR Custom Connection factory #2268: provider-neutral, caller-configured connection creation through IMigrationConnectionFactory / WithConnectionFactory(...)

I agree DbDataSource is worth considering, especially for NpgsqlDataSource and dynamic password scenarios. My preference would be to first support caller-configured/factory-created connections, because it solves the immediate need and also keeps the internal design flexible. Then DbDataSource could be added as a follow-up or additional overload that adapts into the same internal connection-factory abstraction.

Something like:
WithDataSource(Func<IServiceProvider, DbDataSource> dataSourceFactory)
could internally call:
dataSource.CreateConnection()
and reuse the same IMigrationConnectionFactory path.

My thinking is that the immediate need is support for caller-provided / externally-created connections, because that unblocks cases where the connection has to be configured at runtime before FluentMigrator opens it.

I will bring back the IConnectionStringAccessor' ctors first and mark them as Obsolete to give users a clear warning instead of a breaking change.

I addressed the backward-compatibility and DI ambiguity feedback and pushed the updates.

Main changes:

• restored the existing IConnectionStringAccessor constructors and marked them obsolete
• kept IMigrationConnectionFactory as the preferred/internal path
• resolved constructor ambiguity in DI
• aligned the tests with the existing integration test infrastructure

If you get a chance, could you please take another look?

@jzabroski I like the idea behind this PR, as it would fix the outstanding issue where you need to configure the - for example - NpgsqlConnection in ways that cannot be expressed in the connection string.

@fubar-coder does that mean you reviewed it and approve

@Sherif-Ahmed can you add this to the PR

public static IMigrationRunnerBuilder WithDataSource(
    this IMigrationRunnerBuilder builder,
    Func<IServiceProvider, DbDataSource> dataSourceFactory)
{
    return builder.WithConnectionFactory((sp, _) =>
    {
        var dataSource = dataSourceFactory(sp);
        return dataSource.CreateConnection();
    });1
}

@fubar-coder does that mean you reviewed it and approve

Sorry, it was just meant as an opinion and a reason for why this PR is useful.

@jzabroski I updated the PR accordingly.

OK. I appreciate your effort there.

I added some review comments on the code itself; I am still not done reviewing every line but making progress. I clicked submit on my current comments hoping that I won't have anything else to add.

Some last changes requested:

1. We have an ADR for architecture patterns in FluentMigrator, as it relates to DependencyInjection. Can you also update https://github.com/fluentmigrator/fluentmigrator/blob/main/adr/proposed/DependencyInjection.md to add a 13th row, after connection strings? I think the item label should be Connections, Interface = IDbConnection.
   a. One thing DbDataSource does that SqlConnection does not is handle the concept of a Batch; I appreciated your analysis above, although I do wonder if it was generated by Claude I still found it very helpful. I think the main question we should ask if to what extent we may want to eliminate round trips in the future. This does not need to be solved in this PR.
2. It is not clear to me how this feature should interact with the WithGlobalConnectionString API. How do you think it should work?

   fluentmigrator/src/FluentMigrator.Runner.Core/MigrationRunnerBuilderExtensions.cs

   Lines 59 to 86 in 039466a

   	public static IMigrationRunnerBuilder WithGlobalConnectionString(
   	this IMigrationRunnerBuilder builder,
   	string connectionStringOrName)
   	{
   	builder.Services.Configure<ProcessorOptions>(opt => opt.ConnectionString = connectionStringOrName);
   	return builder;
   	}
   	/// <summary>
   	/// Sets the global connection string
   	/// </summary>
   	/// <param name="builder">The runner builder</param>
   	/// <param name="configureConnectionString">The function that creates the connection string.</param>
   	/// <returns>The runner builder</returns>
   	public static IMigrationRunnerBuilder WithGlobalConnectionString(
   	this IMigrationRunnerBuilder builder, Func<IServiceProvider, string> configureConnectionString)
   	{
   	builder.Services
   	.AddSingleton<IConfigureOptions<ProcessorOptions>>(
   	s =>
   	{
   	return new ConfigureNamedOptions<ProcessorOptions>(
   	Options.DefaultName,
   	opt => opt.ConnectionString = configureConnectionString(s));
   	});
   	return builder;
   	}

   
   a. Ideally, add a unit test to pencil in that behavior to avoid regression risk. I see them as orthogonal settings that can be independently combined and you should also be able to "opt in" to the new factory without it disrupting setting WithGlobalConnectionString - WithGlobalConnectionString, if you squint, was our pre-DbDataSource way of handling half of the problem with SqlConnection lacking something like a DbDataSource abstraction.
3. VitePress docs
   a. Can you update https://github.com/fluentmigrator/fluentmigrator/blob/main/docs-website/providers/postgresql.md with your DynamicPasswords example code? I would put it under the Configuration section, as another sub-section after "Basic Configuration".
   b. Update https://github.com/fluentmigrator/fluentmigrator/blob/main/docs-website/runners/in-process.md to replace WithGlobalConnectionString with something more appropriate using DbDataSource, and a > [!NOTE] markdown note to communicate the preferred approach for .NET 8 onwards is to configure a DbDataSource.
   a. Alternatively, maybe WithGlobalConnectionString should be updated to set-up the default connection factory.

Thanks, that makes sense.

My understanding is that DbDataSource is a better long-term abstraction than only passing an IDbConnection, because it can represent the configured data source/pool and leaves room for future provider features such as batching or reducing round-trips, but for now keeping the scope focused on configurable connection creation, while keeping the API compatible with adding more DbDataSource-based behavior later if you want to move in that direction.

And yes, I used AI assistance to help compare the approaches and to help me express the analysis clearly, especially because English is not my mother tongue 😄. That said, I reviewed and adapted the conclusions against the actual FluentMigrator design and the ADO.NET APIs before applying anything.

1. I updated the ADR with the new row after connection strings
   I can also add a short note under the existing “Connections/connection strings” section to clarify that this new connection abstraction is separate from connection string resolution:
   “Database connections are treated as a dependency-injected concept represented by IDbConnection. The runner obtains connections through IMigrationConnectionFactory, allowing connection creation to be configured independently from connection string resolution.”
   a. One small nuance: DbConnection also has CreateBatch() on newer .NET targets, but I agree the DbDataSource point still stands. DbDataSource is the better long-term abstraction because it represents the configured data source/pool, not just one connection instance. That leaves room for future provider features such as batching or reducing round-trips.

2. I added tests for the combined cases to prevent regressions.
   My initial approach was to move the actual connection creation path away from depending directly on IConnectionStringAccessor, so the runner could create connections through IMigrationConnectionFactory.
   After the backward-compatibility discussion, I think the better behavior is to keep WithGlobalConnectionString fully supported and treat it as orthogonal to the new APIs:

• WithGlobalConnectionString(...) continues to configure ProcessorOptions.ConnectionString / IConnectionStringAccessor.
• The default IMigrationConnectionFactory implementation uses that configured connection string, preserving existing behavior.
• WithConnectionFactory(...) or WithDataSource(...) explicitly replace the connection creation path, but they do not clear or invalidate the global connection string.
• If both are configured, the explicit factory/data source is used to create the connection, and the global connection string remains available for that factory if it wants to use it.
  So the new APIs are an opt-in replacement for connection creation, not a replacement/removal of the connection string configuration API.
  Longer term, whether WithGlobalConnectionString should stay as the primary API, become a convenience API over the default factory, or eventually be de-emphasized in favor of DbDataSource is more of a design decision for the project.

3. I updated the VitePress docs in two places:
   a. Add a PostgreSQL “Dynamic Passwords” subsection under Configuration, after Basic Configuration, showing NpgsqlDataSourceBuilder + WithDataSource(...).
   b. Update the in-process runner examples to use a DbDataSource-based setup, with a note that for .NET 8+ the preferred approach is to configure a DbDataSource and pass it through WithDataSource(...).
   c. The current design already does this indirectly:

WithGlobalConnectionString(...)
    -> ProcessorOptions.ConnectionString
    -> IConnectionStringAccessor
    -> ConnectionStringMigrationConnectionFactory
    -> IDbConnection

So WithGlobalConnectionString remains the existing configuration API, and the default IMigrationConnectionFactory consumes it. That keeps old behavior stable and avoids making call order confusing.

1. One small nuance: DbConnection also has CreateBatch() on newer .NET targets, but I agree the DbDataSource point still stands. DbDataSource is the better long-term abstraction because it represents the configured data source/pool, not just one connection instance. That leaves room for future provider features such as batching or reducing round-trips.

I didn't notice that; thanks for pointing that out!

After the backward-compatibility discussion, I think the better behavior is to keep WithGlobalConnectionString fully supported and treat it as orthogonal to the new APIs

👍

Thanks for updating everything. I have to fix the GitHub Actions then I will look into merging a bunch of outstanding PRs. There may be some merge conflicts with Gerard Smit's nativeaot/trimmable PR #2252

This will be included shortly via Copilot. It used a combination of your ideas and the other PR.

Completed via #2320