Cannot inherit Database when targeting netcoreapp without using a shared DbConnection

[qstarin]

I inherit Database to override a handful of key methods to wrap them with retry policies.

While retargeting my applications I notice that the only constructor available when targeting netcoreapp (any version) seems to be Database(DbConnection connection) and this treats the passed in connection as shared, forcing connection management back on the inheritor.

Implementing the same retries by compositing Database, using the static Create method to avoid a shared DbConnection, appears much more tedious.

It would be very helpful if I could access the private Database(DatabaseType dbType, Func<DbConnection> createConnection) constructor; however, the DatabaseType is internal.

Perhaps this constructor could be added:

public Database(string providerName, Func<DbConnection> createConnection)
    : this(DatabaseType.Resolve(null, providerName), createConnection)
{ }

This would allow me to continue letting AsyncPoco handle connection lifetime while also continuing to allow me to effectively override Database methods when targeting the .Net Core runtime.

I can work around this by setting _connectionFactory, _sharedConnection, and _sharedConnectionDepth through reflection in my inheriting class's constructor after calling the base(DbConnection connection) constructor, but a public, non-reflection-based method would be preferable.

Thanks for all your hard work on this library!

[tmenier]

Hey Q! I guess I hadn't thought of an inheritance scenario, and I don't mind making these changes, but the reason for the constructor mess in the first place was that .NET Core didn't support DbProviderFactories and related functionality at the time. They brought it back in 2.1 and I'm guessing you're targeting something newer than that, so I could just add back the old ctors for >= netcoreapp2.1:

public Database(string connectionString, string providerName)
public Database(string connectionString, DbProviderFactory provider)
public Database(string connectionStringName)

Would that work? If you'd still rather have the ctor you suggested I'm good with that too. Might even make sense to make it protected to indicate that this is the right scenario to use it. Hell, I'm kind of inclined to just do both.

[qstarin]

Sure, that works. I might suggest that any constructors that are available publicly for netfx should also be publicly available for corefx, baring any missing functionality, but I don't have a strong opinion on that.

A note for anyone who might be reading this later: to use the constructors that take a provider name or connection string name when targeting the core runtime requires explicitly registering DbProviderFactory's with DbProviderFactories.RegisterFactory(string providerInvariantName, DbProviderFactory factory)

In case it's helpful, here's how we use inheritance with Database:

public class ResilientAsyncPocoDatabase : AsyncPoco.Database
{
    static private RetryPolicy Retry => RetryPolicy.Defaults.Db;

    public ResilientAsyncPocoDatabase(string connectionString)
        : base(connectionString, "Microsoft.Data.SqlClient")
    { }

    public delegate Task<int> ExecuteSqlAsyncDelegate(string sql, params object[] args);
    public delegate Task<T> ExecuteScalarSqlAsyncDelegate<T>(string sql, params object[] args);
    public delegate Task<List<T>> FetchAsyncDelegate<T>(string sql, params object[] args);

    public Task ExecuteAsync(Func<ExecuteSqlAsyncDelegate, Task> action)
        => Retry.RunAsync(() => action(base.ExecuteAsync));
    
    public Task<int> ExecuteAsync(Func<ExecuteSqlAsyncDelegate, Task<int>> action)
        => Retry.RunAsync(() => action(base.ExecuteAsync));
    
    public Task<T> ExecuteScalarAsync<T>(Func<ExecuteScalarSqlAsyncDelegate<T>, Task<T>> action)
        => Retry.RunAsync(() => action(base.ExecuteScalarAsync<T>));
    
    public Task<List<T>> FetchAsync<T>(Func<RetryContext, FetchAsyncDelegate<T>, Task<List<T>>> action)
        => Retry.RunAsync(context =>
            action(context, async (sql, args) => {
                List<T> list = new List<T>();
                await base.QueryAsync<T>(sql, args, v => { list.Add(v); return true; });
                return list;
            }));

    public override Task BeginTransactionAsync()
        => Retry.RunAsync(base.BeginTransactionAsync);
    
    public override Task<int> ExecuteAsync(string sql, params object[] args)
        => Retry.RunAsync(() => base.ExecuteAsync(sql, args));
    
    public override Task<T> ExecuteScalarAsync<T>(string sql, params object[] args)
        => Retry.RunAsync(() => base.ExecuteScalarAsync<T>(sql, args));
    
    public override Task<object> InsertAsync(string tableName, string primaryKeyName, bool autoIncrement, object poco)
        => Retry.RunAsync(() => base.InsertAsync(tableName, primaryKeyName, autoIncrement, poco));
    
    public override Task QueryAsync<T>(string sql, object[] args, Func<T, bool> func)
        => Retry.RunAsync(() => base.QueryAsync(sql, args, func));
    
    public override Task QueryAsync<TRet>(Type[] types, object cb, string sql, object[] args, Action<TRet> action)
        => Retry.RunAsync(() => base.QueryAsync(types, cb, sql, args, action));
    
    public override Task<int> UpdateAsync(string tableName, string primaryKeyName, object poco, object primaryKeyValue, IEnumerable<string> columns)
        => Retry.RunAsync(() => base.UpdateAsync(tableName, primaryKeyName, poco, primaryKeyValue, columns));
}

[tmenier]

I might suggest that any constructors that are available publicly for netfx should also be publicly available for corefx, baring any missing functionality

I agree. Missing functionality was the whole problem 2.5 years ago when I did this, but I gotta think most netcore apps today are on at least 2.1 so I'll add these back and it shouldn't be an issue. I'm thinking the one exception will be the one that takes connectionStringName. ConfigurationManager has fallen out of favor in the netcore/net5 world, maybe I should deprecate that one altogether. The config system being used should probably be a concern of the app, not the library.

to use the constructors that take a provider name or connection string name when targeting the core runtime requires explicitly registering...

Good point, I forgot about that. That's a good argument for providing a public or protected ctor like the one you suggested originally.

[qstarin]

Probably don't need to worry much about people targeting core v1.0-2.0, and I agree the connectionStringName constructor is unnecessary.

You could possibly target netcoreapp1.0, 1.1, and 2.0 and use those monikers as the compiler conditional to hide the connectionString constructors, but it might just be more hassle than it's worth - and I haven't tested if that even works as needed if you have say, Lib A targeting netstandard and referencing AsyncPoco used by App B targeting, say, netcoreapp2.0 and referencing Lib A (but not referencing AsyncPoco directly).

v2.2 is the minimum I'm targeting, as I already have ASP.Net Core apps on that version (but targeting net472 currently) - 2.2 is just a brief stop to flush out issues between full framework and core runtime targeting before updating to 3.1.

[tmenier]

I'm smelling some breaking changes in the near future, maybe I'll just do this right and bump the lib to 3.0. :)

The recommendation for cross-platform targeting in libraries these days is netstandard2.0 only. What sucks about that is it includes core 2.0 and yet DbProviderFactories wasn't added until 2.1. I like what the static Create methods do because passing in a connection type avoids DbProviderFactories entirely. I think I just need to a way to make it work with inheritance. In the short term, maybe something like this would be the path of least resistance for you?

protected Database(Type connectionType, string connectionString) {
    if (!typeof(DbConnection).IsAssignableFrom(connectionType))
        throw new ArgumentException(...);
    ...
}

At least with that you don't need to register a provider factory. For 3.0 I'm starting to wonder if the object model should be simplified by making Database abstract and subclassing with SqlServerDatabase etc. Composing with DatabaseType is a little awkward; I could just factor that out.

[qstarin]

That also looks like it could work..

One thing I should mention is that I'm also trying to use Microsoft.Data.SqlClient over System.Data.SqlClient - the relevant area in AsyncPoco is DatabaseType.Resolve where there is no check for the Microsoft namespace, but that causes the code to fall through to the default of Singleton<SqlServerDatabaseType>.Instance which works fine as long as _connectionFactory is returning the Microsoft.Data.SqlClient objects.

With your protected Database(Type connectionType, string connectionString) would you find yourself using Activator.CreateInstance as the factory since you'll only have a Type there? Probably not a huge issue but there's of course a bit of performance loss using that over new(), which you can constrain for on the static factory methods.

As I write all this, though, and look at the source - I wonder was DbProviderFactory available in Core v1.0-2.0 because the public Database(string connectionString, DbProviderFactory provider) would work well also. And it does appear DbProviderFactory has been available for all of .Net Core - https://docs.microsoft.com/en-us/dotnet/api/system.data.common.dbproviderfactory?view=netcore-1.0

[tmenier]

All good points. It was actually DbProviderFactories and not DbProviderFactory that was introduced in 2.1: https://docs.microsoft.com/en-us/dotnet/api/system.data.common.dbproviderfactories. IIRC that basically comes with some providers pre-registered so that providing a provider name just worked.

Guessing you need something in the near term to get unblocked? I think we have a couple options here, so maybe I'll just go with the path of least resistance for now and we can keep the dialog going on a possible 3.0. You probably know this but PetaPoco supports async now so AsyncPoco has sort of lost its only distinguishing feature and I certainly wouldn't fault anyone if they wanted to switch back. But, I'd also be happy to move this project forward and maybe 3.0 can get some new distinguishing features. Support for retry policies like you built perhaps?

[qstarin]

No rush, I have a reflection-based work-around for now, and it will probably be several weeks still before I'm fully through the rest of work to retarget for .Net Core.