-
Notifications
You must be signed in to change notification settings - Fork 681
Feature/custom db provider factory #2002
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Feature/custom db provider factory #2002
Conversation
build failed |
0dc3604
to
6d26b78
Compare
so that alternative implementations can be registered with the DI container.
This commit is a pure refactoring. - Move ReflectionBasedDbFactory to file of the same name. - Rename interface PostgresDbFactor to IPostgresDbFactory.
via an additional optional argument to the `PostgresRunnerBuilderExtensions.AddPostgres*` methods.
This example shows how to use Npgsql dynamic passwords by supplying a custom DbProviderFactory.
6d26b78
to
ccac274
Compare
Build passing. Will try to review some of this later today. Have not had time yet. |
/// <returns>The migration runner builder</returns> | ||
public static IMigrationRunnerBuilder AddPostgres(this IMigrationRunnerBuilder builder) | ||
public static IMigrationRunnerBuilder AddPostgres(this IMigrationRunnerBuilder builder, DbProviderFactory dbProviderFactory = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done a bit of investigation in the codebase and there are only two members of DbProviderFactory
in use: CreateCommand
and CreateConnection
. There is only one call site for CreateCommand
: GenericProcessorBase.CreateCommand, which could be easily replaced with a call to connection.CreateCommand
. This would leave only a single member in use: CreateCommand
.
This means that DbProviderFactory
in this public method signature could be replaced by either an interface with a single CreateConnection
method:
public interface IDbContext
{
public IDbConnection CreateConnection();
}
or even by a delegate:
public delegate IDbConnection DbConnector();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it would be possible to minimize the amount of change using an adapter to present this as a DbProviderFactory
to the rest of the codebase:
public class DbProviderFactoryAdapter : DbProviderFactory
{
private readonly Func<DbConnection> _dbConnector;
public DbProviderFactoryAdapter(Func<DbConnection> dbConnector)
{
_dbConnector = dbConnector;
}
public override DbConnection CreateConnection() => _dbConnector();
public override DbCommand CreateCommand()
{
throw new NotImplementedException();
}
...
}
this admits the possibility that new code will use other members and throw exceptions at runtime. Instead, I would prefer to propagate the new interface throughout the codebase and let the compiler prove that none of the other members of DbProviderFactory
are in use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I have not responded. I plan to get to this this week. Wrapping up my taxes and a few other start of the year chores and then I can have fun again and review things like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it would be possible to minimize the amount of change using an adapter to present this as a
DbProviderFactory
to the rest of the codebase:
OK, thought about this. I don't love this as a solution. At minimum:
public override DbCommand CreateCommand()
{
throw new NotImplementedException();
}
should have [Obsolete("Use DbProviderFactoryAdapter.")]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead, I would prefer to propagate the new interface throughout the codebase and let the compiler prove that none of the other members of
DbProviderFactory
are in use.
Yes, exactly.
This change allows you to supply a custom DbProviderFactory instance when configuring your DI container in your application's composition root.
This change is motivated by #1981 so new sample application is probably the best place to start a review. Note that this implementation delegates the dynamic password feature to the underlying database library (
Npgsql
in this case).I have attempted to keep this change minimal:
AddPostgres*
methods ofFluentMigrator.Runner.Postgres/PostgresRunnerBuilderExtensions
.DbProviderFactory
primarily because it is the type returned byDBFactoryBase.Factory
and therefore minimizes the changes needed to consumers.On point 3 above, I think this is a reasonable decision internally, but it is probably the wrong type to expose in the public interface of the library, which is why this PR is in draft status. I rather like the suggestion of a new interface made by @jzabroski here.