A Class to Supply an Open Database Connection during the life of a Web Request

Question

For some database requests, I like to use raw ADO.NET.

In the context of a web request, I have created a class which provides an open IDbConnection object.

I use a Dependency Injection library ("DI") to do this, scoping the object instantiated from this class to a web request. So, disposal of the object is handled by the DI container.

The abstraction for this class is simple and looks like this:

public interface IDbConnectionManager : IDisposable
{
    IDbCommand BuildCommand(DbParameter[] parameters, string query);
    IDbConnection GetOpenConnection();
}

And the concrete implementation, written for Sql Server, looks like this:

public class DbConnectionManager : IDbConnectionManager
{
    public SqlConnection DbConnection;
    public string ConnectionString { get; set; }

    public DbConnectionManager(string connectionString)
    {
        ConnectionString = connectionString;
    }

    public IDbConnection GetOpenConnection()
    {
        return GetOpenSqlConnection();
    }

    private SqlConnection GetOpenSqlConnection()
    {
        if (ReferenceEquals(DbConnection, null))
        {
            DbConnection = new SqlConnection(ConnectionString);
        }

        if (DbConnection.State != ConnectionState.Open)
        {
            DbConnection.Open();
        }

        return DbConnection;
    }

    public IDbCommand BuildCommand(DbParameter[] parameters, string query)
    {
        if (parameters == null) throw new ArgumentNullException(nameof(parameters));
        if (string.IsNullOrWhiteSpace(query)) throw new ArgumentException(nameof(query));

        var command = new SqlCommand { Connection = GetOpenSqlConnection() };
        command.Parameters.AddRange(parameters);
        command.CommandText = query;

        return command;
    }

    public void Dispose()
    {
        DbConnection?.Dispose();
    }
}

You can see the BuildCommand method uses the GetOpenSqlConnection for its connection.

The main reason I have taken this approach is because DbConnections are expensive. But I'm not sure whether that relates to creating a connection or opening one.

I'm aware that connection pooling is available (if enabled), but I figured for simple web requests (think API), with perhaps just 2 queries to the database, that this would probably be a good approach.

And as can be seen, there's not a lot to the code.

I just wanted a bit of feedback on this code/approach. And if you can see any potential problems with it, by all means let me know that too.

As a last comment, I am aware that a developer could manually call Dispose or use it in a using block (thereby calling Dispose). This would be on me as lead developer to ensure that this doesn't happen and that devs understand that the DI container disposes of the object.

Answer 1

As far as I know, in ADO.NET the connection pooling is enabled by default, and if you don't need to pool connections, you'll have to explicitly disable pooling.

So, for your implementation, everything seems okay to me, but DbConnectionManager seems to be specific for handling the connection! so, BuildCommand method will be odd in this class!

I would prefer to rename it to something covers all exposed operations and keep everything simpler.

for GetOpenSqlConnection() this could be unnessary, since you can do it in the property level.

What I suggest is to make DbConnection & ConnectionString static, so you ensure you only have a single instance of SqlConnection. and create two constructors one takes SqlConnection, and the other one takes connectionString. you would have something like this :

public class DbConnectionManager : IDbConnectionManager
{
    // make it static to have a single instance
    private static SqlConnection DbConnection
    {
        get => DbConnection is null ? new SqlConnection(ConnectionString) : DbConnection;
        set => DbConnection = value;
    }

    // store the connectionString of SqlConnection, for connection backup.
    private static string ConnectionString { get; set; } = DbConnection.ConnectionString;


    public DbConnectionManager(string connectionString) 
        : this(new SqlConnection(connectionString)) { }

    public DbConnectionManager(SqlConnection dbConnection)
    {
        DbConnection = dbConnection;
        ConnectionString = dbConnection.ConnectionString;
    }
}

in both constructors, they're initiating a new DbConnection and the ConnectionString is just backup, in case your actual DbConnection is lost, you can re-initiate it with the connectionString that you've stored.

doing that, it'll eleminate the need of :

private SqlConnection GetOpenSqlConnection()
{
    if (ReferenceEquals(DbConnection, null))
    {
        DbConnection = new SqlConnection(ConnectionString);
    }

    if (DbConnection.State != ConnectionState.Open)
    {
        DbConnection.Open();
    }

    return DbConnection;
}

for :

public IDbCommand BuildCommand(DbParameter[] parameters, string query)
{
    if (parameters == null) throw new ArgumentNullException(nameof(parameters));
    if (string.IsNullOrWhiteSpace(query)) throw new ArgumentException(nameof(query));

    var command = new SqlCommand { Connection = GetOpenSqlConnection() };
    command.Parameters.AddRange(parameters);
    command.CommandText = query;

    return command;
}

Since there is no actual execution is going here, opening a connection here is not needed, because you built the command and return the instance back to execute it somewhere else. this may requires you to create two public methods for opening and closing connection before you call the execute methods such as command.ExecuteNonQuery().

What I would do is maybe make this method private, and create public methods for each SQL execution type such as ExecuteNonQuery and ExecuteScalar and ExecuteReader, with the same arguments of BuildCommand has, and all will be from within the same class. Something like this :

public void ExecuteNonQuery(DbParameter[] parameters, string query)
{
    var command = BuildCommand(parameters, query);

    if (DbConnection.State != ConnectionState.Open)
        DbConnection.Open();

    command.ExecuteNonQuery();
}

The best approach for that will be creating a static property of SqlCommand then you initiate it, use it across the class, dispose it whenever you're done.

another question got in my mind is this :

if (parameters == null) throw new ArgumentNullException(nameof(parameters));

why you made parameters required? suppose you need to execute a query with no parameters such as SELECT * FROM table, then, you would have to adjust the current implementation or create new method for that. So, keeping it optional will come in handy.