Close MySQL Connection outside Function

Question

I currently have a little app sends a lot of different MySQL Queries to the server. My idea was to wrap the connection, the query and the read to a function with only the actual query as a parameter.

Here is what I got:

 public static MySqlDataReader mySqlRead(string cmdText)
    {

        string connString = "server=" + ORVars.sqlServerAddr + ";port=" + ORVars.sqlServerPort + ";uid=" + ORVars.sqlServerUID + ";pwd=" + ORVars.sqlServerPass + ";database=" + ORVars.sqlServerDB + ";";

        MySqlConnection conn = new MySqlConnection(connString);
        MySqlCommand command = conn.CreateCommand();

        command.CommandText = cmdText;

        try
        {
            conn.Open();
            MySqlDataReader reader = command.ExecuteReader();
            return reader;
        }

        catch (MySqlException)
        {
            throw;
        }

    }

I connect and send the query here:

private void btnLogin_Click(object sender, EventArgs e)
    {
        string username = txtLogin.Text;
        string password = ORFunc.GetMD5Hash(txtPassword.Text);

        MySqlDataReader orRead = ORFunc.mySqlRead("SELECT * FROM orUsers WHERE username = '" + username + "' AND pass = '" + password + "'");
        while (orRead.Read())
        {
            MessageBox.Show(orRead["id"].ToString());
        }
    }

Works like a charm... BUT, as you can see above, the connection is never closed. When I add the conn.Close() behind the .ExecuteReader() the reader is empty and everything after return is of course useless.

Maybe it's a stupid question but I'm rather new to C# so please be generous, any hint is appreciated.

cheers,

PrimuS

Russell Uhl · Answer 1 · 2013-06-26T19:55:40.780

I had a similar problem in JAVA recently, but I think the same will work for you. Essentially, you can create a class that represents a "SqlCall" object (or something). The class would have accessible members including the connection and the results. The ctor for the class would take in your query text.

Then, all you would have to do is create a new instance of that class, run the query in a method in that class (which would set and/or return the results), GET the results, and then when you are done, call close() on your class (which would then have to be coded such that it closes the connection held internally).

Technically, a better way to do this is to EXTEND the connection class itself, but as you are new to C#, I will not go into the details of doing so.

As I was writing the code below, I realized I may have not actually answered your question. But there's no point in backing out now, so here's what I have:

public class SqlCall {

    private static connString = "server=" + ORVars.sqlServerAddr + ";port=" + ORVars.sqlServerPort + ";uid=" + ORVars.sqlServerUID + ";pwd=" + ORVars.sqlServerPass + ";database=" + ORVars.sqlServerDB + ";";
    private MySqlConnection conn;
    private MySqlCommand command;
    private MySqlDataReader reader;

    public SqlCall(String query) {

        conn = new MySqlConnection(connString);
        command = conn.CreateCommand();
        command.CommandText = query;

    }

    public MySqlDataReader execute() throws Exception {
        conn.Open();
        reader = command.ExecuteReader();
        return reader;
    }

    public void close() {
        reader.close();
        conn.close();
    }

}

Your login code would be:

private void btnLogin_Click(object sender, EventArgs e) {
    string username = txtLogin.Text;
    string password = ORFunc.GetMD5Hash(txtPassword.Text);

    SqlCall sqlcall = new SqlCall("SELECT * FROM orUsers WHERE username = '" + username + "' AND pass = '" + password + "'");

    try {
        MySqlDataReader orRead = sqlcall.execute();
        while (orRead.Read())
        {
            MessageBox.Show(orRead["id"].ToString());
        }
        sqlcall.close();
    } catch (Exception ex) {
        // dostuff
    }
}

The point is, unless you copy the data into a new datatable at the very beginning, you'll have to keep the connection open.

On a separate note, YOUR CODE IS PRONE TO SQL INJECTION. Don't know what that is? An example: if I said my username was ';DROP TABLE orUsers;--, then your entire user database would be gone. Look into stored procedures if you want a (very healthy) way around this.

score 1 · Accepted Answer · edited May 23 '17 at 11:50

You have difficulties because your idea works against the pattern expected by programs that connects to a database in NET Framework.
Usually, in this pattern you have a method that

INITIALIZE/OPEN/USE/CLOSE/DESTROY

the ADO.NET objects connected to the work required to extract or update data

Also your code has a serious problem called Sql Injection (see this famous explanation) because when you concatenate strings to form your command text you have no defense against a malicious user that try to attack your database

private void btnLogin_Click(object sender, EventArgs e)
{
    string username = txtLogin.Text;
    string password = ORFunc.GetMD5Hash(txtPassword.Text);

    MySqlParameter p1 = new MySqlParameter("@uname", username);
    MySqlParameter p2 = new MySqlParameter("@pass", pass);
    string cmdText = "SELECT * FROM orUsers WHERE username = @uname AND pass = @pass"
    DataTable dt = ORFunc.GetTable(cmdText, p1, p2);
    foreach(DataRow r in dt.Rows)
    {
       Console.WriteLine(r["ID"].ToString());
    }
}

public static DataTable GetTable(string cmdText, params MySqlParameter[] prms)
{
    string connString = "server=" + ORVars.sqlServerAddr + ";port=" + ORVars.sqlServerPort + ";uid=" + ORVars.sqlServerUID + ";pwd=" + ORVars.sqlServerPass + ";database=" + ORVars.sqlServerDB + ";";
    // This is the INITIALIZE part
    using(MySqlConnection conn = new MySqlConnection(connString))
    using(MySqlCommand command = new MySqlCommand(cmdText, conn))
    {
         // OPEN
         conn.Open();
         DataTable dt = new DataTable();
         command.Parameters.AddRange(prms);

         // USE
         MySqlDataReader reader = command.ExecuteReader();
         dt.Load(reader);
         return dt;
    } // The closing brace of the using statement is the CLOSE/DESTROY part of the pattern
}

Of course this is a generic example and in my real work I don't use very often these generic methods and prefer to write specialized data access code that return the base object needed to the upper layer of code

Close MySQL Connection outside Function

2 Answers2