SQL command string is it okay to concatenate it?

Question

I've been creating a class for buttons where you can add and delete rows from the table's database but it is my first time concatenate a string I have a suspicion that it is not working due to commandtext.

 public static void deleteButton(string databaseName, string IDname, DataGridView dgv)
    {
        Helper.openConnection();

        SqlCommand cmd = new SqlCommand();
        cmd.Connection = Helper.cn;

        string IDLocation = dgv.SelectedRows[0].Cells[0].Value.ToString();
        cmd.CommandText = "delete from " + databaseName + " where " + IDname + " = " + IDLocation;

        Helper.cn.Close();
        MessageBox.Show("Successfully Deleted!");
    }

    public static void addButton(string databaseName, List<string> values, DataGridView dgv, bool isAdd)
    {
        Helper.openConnection();
        SqlCommand cmd = new SqlCommand();
        cmd.Connection = Helper.cn;
        string message = isAdd == true? "Sucessfully Added" : "Sucessfully Edited";
        string command = "insert into " + databaseName + " values(";

        for (int i = 0; i < values.Count; i++)
        {
            command += values[i];
            if(i != values.Count - 1) command += ", ";
        }

        command += ")";
        cmd.CommandText = command;

        MessageBox.Show(message);
        Helper.cn.Close();
    }

thank you for your time helping me.

What do you mean by "it is not working"? Also, you have to use table names, not database names in your statements. Not to mention that creating statements this way is [a huge security risk](https://en.wikipedia.org/wiki/SQL_injection). — molnarm, Dec 13 '17 at 13:21
If your DataGridViw is editable you invite every user to destroy your database. [Read: What are good ways to prevent SQL injection?](https://stackoverflow.com/questions/14376473/what-are-good-ways-to-prevent-sql-injection) — Tim Schmelter, Dec 13 '17 at 13:21
While it may work, concatenating strings in this context is not a good practice and may leave you open to a SQL Injection atack. — Matheus Lacerda, Dec 13 '17 at 13:23
It's not working because you are adding (supposedly) string values to the query without quotes. The fix would be easy, but I'd rather not help you do this... please learn about query parameters, and do not use string concatenation to give values to your sql sentences (unless you know **exacty** what you are doing) — Jcl, Dec 13 '17 at 13:25

score 3 · Answer 1 · edited Dec 13 '17 at 13:31

3

Two problems:

You're using INSERT INTO [databaseName]. That should be INSERT INTO [tableName]. That's why it's not working.
Don't concatenate values into the SQL text. It opens the door for SQL injection, and it also makes it harder for the SQL server to reuse query plans. Instead, use query parameters. There is an example in the documentation.

edited Dec 13 '17 at 13:31

L. Guthardt

1,990
6
22
44

answered Dec 13 '17 at 13:27

Gabriel Luci

38,328
4
55
84

Thank you so much! I have read the SQL injection in the documentation it do seems very dangerous. My professor haven't discussed it to us though. I will try to use the query parameters – user7524392 Dec 13 '17 at 13:37

score 0 · Answer 2 · answered Dec 13 '17 at 13:28

I'll leave the design up to you and just attempt to answer the question. Have you actually looked at the command text? Have you tried to paste the command text into a query and run it manually? You need to quote string values. Also your functions and query use 'databaseName'. This should be a table name not a database name.

score 0 · Answer 3 · answered Dec 15 '17 at 02:04

The commentary here is all on target, but that aside the key issue with your code is you are not doing anything. You have opened the connection, declared the SQL command, but then you don't execute it.

So yes, use parameters, but if you want your SQL to work you need to execute it:

string IDLocation = dgv.SelectedRows[0].Cells[0].Value.ToString();
cmd.CommandText = string.Format("delete from {0} where IDname = @ID", databaseName);
cmd.Parameters.AddWithValue("@ID", IDLocation);

Note you don't need quotes or anything when you use parameters, even on a non-numeric datatype.

And the feature of the evening, the missing link:

cmd.ExecuteNonQuery();

Same goes for your insert query -- be sure to run the execute method, and USE PARAMETERS!

SQL command string is it okay to concatenate it?

3 Answers3