The ConnectionString property initialization

Question

Although this question seems like have answers already but my case is different, here's how.

It works the first time but fails for subsequent requests.

I'm creating the connection in the main class and passing to the DB class as a dependency in it's constructor and it's meant to be re-used for each call.

public class DB
{
    private SqlConnection conn;

    public DB(SqlConnection conn)
    {
        this.conn = conn;
    }

    public List<Records> GetRecords()
    {
        using (conn){
            conn.Open();
            using (SqlCommand cmd = new SqlCommand("SELECT * FROM Records", conn))
            using (SqlDataReader reader = cmd.ExecuteReader())
            {
                List<Records> rows = new List<Records>();
                while (reader.Read())
                {
                    rows.Add(new Records(reader.GetString(1)));
                }
                return rows;
            }
        }
    }
}

Caller class

string connection = $@"
    Data Source=;
    Initial Catalog=;
    Persist Security Info=True;
    User ID={env["DATABASE_USER"]};
    Password={env["DATABASE_PASSWORD"]};";

Db db = new DB(new SqlConnection(connection));
db.GetRecords();

fail: Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware[1] An unhandled exception has occurred while executing the request. System.InvalidOperationException: The ConnectionString property has not been initialized.

Inappropriately reusing `SqlConnection` objects in the mistaken belief that they are expensive to create is common. They're not; an `SqlConnection` object represents a handle to a pooled physical connection. Only the first creation is expensive. There is rarely a good reason to put `SqlConnection` instances in fields, let alone pass them between scopes. The most important argument against reusing connections is that they become unusable if an error occurs, requiring a close. This makes error recovery a nightmare if the connection is shared. — Jeroen Mostert, Oct 10 '19 at 14:46
@JeroenMostert I also realize why I did that in the first place, so that I could mock the dependencies in my unit tests which I've done in other languages, new to csharp so not sure how to approach that. — AppDeveloper, Oct 10 '19 at 15:46
Two obvious ways: you can use a LocalDB instance (effectively, keep the database code as it is, and just give it a full featured local database), or refactor the database code in a class that just produces the data, without the caller needing to know that it's coming from a database, and mock that. People occasionally try to go "inbetween" and keep objects from `System.Data` around while you somehow still intercept the physical database access, but if even possible at all, this tends to be much more complicated than the obvious approaches, due to the strong coupling between these classes. — Jeroen Mostert, Oct 10 '19 at 16:29
Furthermore, if your data needs are of rather trivial kind (as they are in this example), consider using a simple ORM like [Dapper](https://github.com/StackExchange/Dapper/) to abstract over the database. This saves you a ton of boilerplate code in serializing objects, and methods that produce `IEnumerable<...>` instances are trivial to mock. — Jeroen Mostert, Oct 10 '19 at 16:30
Thanks, I like the second choice, meaning I'd create a `FakeDB` class instead of `DB` class and just generate the records. — AppDeveloper, Oct 10 '19 at 16:58

score 1 · Accepted Answer · answered Oct 10 '19 at 14:35

1

I'm not 100% sure, but I guess the problem is the

using(conn)

when the using is closed, the SqlConnection will be disposed.
so when you call again db.GetRecords();,
conn.Open() is not initialized. -> exception

answered Oct 10 '19 at 14:35

PandaPlayer

162
9

score 1 · Answer 2 · edited Oct 10 '19 at 14:51

1

You shouldn't use SQLConnection as a field, but as a local variable inside the method. Change your class to take in the connection string inside it's parameter instead of an instance of SqlConnection and initialize it in any method that use it:

public class DB
{
    private string connectionString;

    public DB(string connectionString)
    {
        this.connectionString = connectionString;
    }

    public List<Records> GetRecords()
    {
        using (var conn = new SqlConnection(connectionString)){
            conn.Open();
            using (SqlCommand cmd = new SqlCommand("SELECT * FROM Records", conn))
            using (SqlDataReader reader = cmd.ExecuteReader())
            {
                List<Records> rows = new List<Records>();
                while (reader.Read())
                {
                    rows.Add(new Records(reader.GetString(1)));
                }
                return rows;
            }
        }
    }
}

For more details, read this.

edited Oct 10 '19 at 14:51

AppDeveloper

1,816
7
24
49

answered Oct 10 '19 at 14:41

Zohar Peled

79,642
10
69
121

Can I just keep the connection open and re-use the requests? Will that be inefficient, if then why will it be inefficient. The only reason I can think of it perhaps it will queue all the calls, and each request has to wait for the earlier one to finish, right? – AppDeveloper Oct 10 '19 at 14:45
2

@AppDeveloper: a connection supports no more than one active command (unless Multiple Active Result Sets is used, which has its own caveats) and a command will not return until it has completed, so whether you're reusing a connection or closing and re-opening it has no bearing on that. – Jeroen Mostert Oct 10 '19 at 14:56
The connection pool ensures that disposing and re-creating the connection is a very cheap process. the link at the bottom of my answer explains that. – Zohar Peled Oct 10 '19 at 15:05

The ConnectionString property initialization

2 Answers2