Use Variable In SQL String

Question

How can I add a variable to my SQL string and run it against the server successfully? I want to run this statement through my C#

protected void RunSQLQuery(string salesman, string connectionString)
{
    SqlConnection cnn;
    SqlCommand cmd;
    StringBuilder sql = new StringBuilder();
    SqlDataReader reader;
    cnn = new SqlConnection(connectionString);
    sql = new StringBuilder();
    sql.Append("update database ");
    sql.Append("set shippdate = GetDate() ");
    sql.Append("where salesman = "' + salesman + "'");
    sql.Append("and managerapproval is not null ");
    cnn.Open();
    cmd = new SqlCommand(sql.ToString(), cnn);
    reader = cmd.ExecuteReader();
    reader.Close();
    cmd.Dispose();
    cnn.Close
}

This presents multiple compile errors underlining my +salesman+ code. The errors are:

Only assignment, call, increment, decrement, and new object expressions can be used as a statement

; expected

) expected

Too many characters in character literal Newline in constant

#1 - this is bad. Concatenation will open you up to SQL injection attacks. Look up SQL parameterization. #2 - your quotes are in the wrong order: `salesman = "' + salesman` should be `salesman = '" + salesman` — Glorin Oakenfoot, Jan 13 '16 at 14:11

Soner Gönül · Accepted Answer · 2016-01-13T14:49:07.597

12

You are not adding the string object that salesman refers, you are adding salesman as a string literal.

Just add it as a parameter like;

var cmd = new SqlCommand("update database set shippdate = GetDate() where salesman = @salesman");
cmd.Parameters.Add("@salesman", salesman);
...

And use ExecuteNonQuery to execute your command, not SqlDataReader. This SqlDataReader is for return some data.

But more important, you should always use parameterized queries. This kind of string concatenations are open for SQL Injection attacks.

Also use using statement to dispose your connection and command automatically instead of calling Close or Dispose methods manually.

As a full example;

protected void RunSQLQuery(string salesman, string connectionString)
{
    using(var cnn = new SqlConnection(connectionString))
    using(var cmd = cnn.CreateCommand())
    {
         cmd.CommandText = @"update database set shippdate = GetDate() 
                             where salesman = @salesman";
         // I assume your column is nvarchar
         cmd.Parameters.Add("@salesman", SqlDbType.NVarChar).Value = salesman;
         cnn.Open();
         cmd.ExecuteNonQuery();
    }
}

For myself, I always prefer to use SqlParameterCollection.Add(string, SqlDbType, Int32) overload to specify my parameter type and it's size but since you never mentioned your salesman column type, I couldn't post this in my example.

edited Jan 13 '16 at 14:49

answered Jan 13 '16 at 14:11

Soner Gönül

97,193
102
206
364

looks like you have a typo after `GetDate()` in your query. – user1666620 Jan 13 '16 at 14:14
@SonerGönül - I thought using var variable types was bad practice? Am I mis-informed on that? – FunFly WhiteGuy Jan 13 '16 at 14:25
@FunFlyWhiteGuy you have been misinformed. Var in C# is not bad, the type is inferred from the right hand side. So no its not bad. – Phill Jan 13 '16 at 14:27
@FunFlyWhiteGuy I use `var` all the time, personally. You can read [Use of var keyword in C#](http://stackoverflow.com/q/41479/447156) as well for more perspectives. – Soner Gönül Jan 13 '16 at 14:35
The [Add overload you are calling](https://msdn.microsoft.com/en-us/library/9dd8zze1(v=vs.110).aspx) in your examples is marked as Obsolete in the MSDN. – Scott Chamberlain Jan 13 '16 at 14:43
@ScottChamberlain That's the another reason why I like `Add(string, SqlDbType, Int32)` overload :) Fixed. Thank you. – Soner Gönül Jan 13 '16 at 14:50

score 0 · Answer 2 · answered Jan 13 '16 at 14:18

As you can also see from the syntax highlighting, the compile errors are caused because you did not escape the quotes properly in sql.Append("where salesman = "' + salesman + "'");.

As a side note, you should never insert strings into sql queries without first validating them, or you are open to sql injection, e.g. if i pass "''; drop table database;" as salesman parameter. It is better to use SqlParameter.

score 0 · Answer 3 · edited Jan 13 '16 at 15:16

0

As mentioned in above answers, yes, writing queries in this way is not a good way to do it. But still if you want to do it that way only, you will have to change:

sql.Append("where salesman = "' + salesman + "'");

to

sql.Append("where salesman = '" + salesman + "'");

edited Jan 13 '16 at 15:16

Chewpers

2,430
5
23
30

answered Jan 13 '16 at 14:28

user3560421

1

Stop teaching people bad habits. You say "You should not be doing it this way" but you still only show the bad way to do it, why? Please explain your reasoning for encouraging people to write dangerous code that can cause security compromises in their system. – Scott Chamberlain Jan 13 '16 at 14:46

Jelle · Answer 4 · 2016-01-13T14:33:19.427

0

I would suggest using the AddWithValue method from your sql command combined with the UPPER function to make it case insensitive:

SqlCommand cmd = cnn.CreateCommand();
cmd.CommandText = "UPDATE database SET shippdate = GetDate() WHERE UPPER(salesman) = UPPER(@salesMan)";   
cmd.Parameters.AddWithValue("@salesMan", salesman);
if (cnn.State.Equals(ConnectionState.Closed))
{
      cnn.Open();
}
cmd.ExecuteNonQuery();
cnn.Close();

edited Jan 13 '16 at 14:33

answered Jan 13 '16 at 14:31

Jelle

53
1
10

This doesn't really directly answer the question. SQL is typically case-insensitive anyways. – Glorin Oakenfoot Jan 13 '16 at 14:32

Use Variable In SQL String

4 Answers4