Csla.Data.ConnectionManager change suggestion

Regent posted on Friday, October 30, 2009

As for now, ConnectionManager.GetManager can accept either raw connectionString or connection string's name.

Inside of public static ConnectionManager GetManager(string database, bool isDatabaseName, string label); is isDatabaseName is true the database is assigned to ConnectionStrings[database].ConnectionString so after line 101 there is always connection string in database variable. Later on which is being passed to the private constructor from connection string.

What I would like to suggest is not to reduce database name to connections string but to have ConnectionStringSettings creates before line 101, so if isDatabaseName it will be simply assigned from ConfigurationManager.ConnectionStrings[] or instantiated with raw connection string otherwise. Also constructor should accept ConnectionStringSettings rather than connections string in order to enable this scenario.

The reason for that is because App.config's connectionStrings section's elements can have provideName specified so there is no need in '.AppSettings["dbProvider"]' setting (what allows different connection strings to have different providers).


<add name="RemoteOracle" connectionString="Data Source=LOCALHOST;User Id=user;Password=tiger" providerName="System.Data.OracleClient" />


Thanks

RockfordLhotka replied on Friday, October 30, 2009

Can you provide code for what you are saying, as I am not sure I follow.

Regent replied on Friday, October 30, 2009

First of all I suggest to replace current constructor code to:

private ConnectionManager(ConnectionStringSettings connectionSettings, string label)
{
_label = label;
_connectionString = connectionSettings.ConnectionString;

string provider = connectionSettings.ProviderName;
if (String.IsNullOrEmpty(provider))
{
provider = ConfigurationManager.AppSettings["dbProvider"];
if (String.IsNullOrEmpty(provider))
provider = "System.Data.SqlClient";
}

DbProviderFactory factory = DbProviderFactories.GetFactory(provider);

// open connection
_connection = factory.CreateConnection();
_connection.ConnectionString = _connectionString;
_connection.Open();
}


Then, extract all connection-sharing logic to the new GetManager overload:

public static ConnectionManager GetManager(ConnectionStringSettings connectionSettings, string label)
{
lock (_lock)
{
var ctxName = GetContextName(connectionSettings.ConnectionString, label);
ConnectionManager mgr = null;
if (ApplicationContext.LocalContext.Contains(ctxName))
{
mgr = (ConnectionManager)(ApplicationContext.LocalContext[ctxName]);
}
else
{
mgr = new ConnectionManager(connectionSettings, label);
ApplicationContext.LocalContext[ctxName] = mgr;
}
mgr.AddRef();
return mgr;
}
}


And finally simplify code of the former share-responsible overload:

public static ConnectionManager GetManager(string database, bool isDatabaseName, string label)
{
ConnectionStringSettings connectionSettings;

if (isDatabaseName)
{
connectionSettings = ConfigurationManager.ConnectionStrings[database];
if (connectionSettings == null || String.IsNullOrEmpty(connectionSettings.ConnectionString))
throw new ConfigurationErrorsException(String.Format(Resources.DatabaseNameNotFound, database));
}
else connectionSettings = new ConnectionStringSettings(label, database, String.Empty);

return GetManager(connectionSettings, label);
}


Also I would recommend adding readonly keyword to '_lock', '_connection', '_connectionString' and '_label' fields' declarations since they are not supposed to be changed once an object was created.
As well as marking the ConnectionManager class as sealed.

I've attached unified diff of changes I made.

RockfordLhotka replied on Friday, October 30, 2009

This is good, thank you.

There's an item in the wish list to refactor all the manager classes to
derive from a common base class that takes care of the reference counting
and resource storage/retrieval. I'll link that issue to this thread as well,
because these are good things to do as part of that change.