6

I have a list of keywords that i store in a list.

To fetch records from a table, am using the following query:

sqlBuilder.Append("SELECT name, memberid FROM members WHERE");
StringBuilder sqlBuilder = new StringBuilder();
foreach (string item in keywords)
            {
            sqlBuilder.AppendFormat(" LOWER(Name) LIKE '%{0}%' AND", item); 
            }
string sql = sqlBuilder.ToString();

As you might have noticed, my query is vulnerable to sql injection, thus i want to use parameters using SqlCommand(). I have tried the following but still doesn't work:

foreach (string item in keywords)
            {    
                sqlBuilder.AppendFormat(" LOWER(Name) LIKE '%' + @searchitem + '%' AND", item);
                SqlCommand cmd = new SqlCommand(sqlBuilder.ToString());
                cmd.Parameters.AddWithValue("@searchitem",item);
             }

Where could i be making the mistake, or rather, how should i got about it?

mutiemule
  • 2,319
  • 2
  • 28
  • 34
  • 1
    Because every time you iterate your `keywords`, you creating new `SqlCommand`. I think you should use `AppendFormat` in your `foreach` loop, then create `SqlCommand` and add your parameters outside of your `foreach` loop. – Soner Gönül Dec 03 '13 at 08:10
  • 1
    I suggest to print out cmd.CommandText, you'll see it's not a well formed SQL query (you're doing everything inside your loop). Moreover wildcard will fit better together with parameter's value. – Adriano Repetti Dec 03 '13 at 08:10

2 Answers2

17

You are doing a few things wrong here:

  • You give all your parameters the same name @searchitem. That won't work. The parameters need unique names.
  • You create a new SqlCommand for each item. That won't work. Create the SqlCommand once at the beginning of the loop and then set CommandText once you are done creating the SQL.
  • Your SQL ends with AND, which is not valid syntax.

Improvement suggestions (not wrong per se, but not best practice either):

  • As Frederik suggested, the usual way is to put the % tokens in the parameter, rather than doing string concatenation inside the SQL.
  • Unless you explicitly use a case-sensitive collation for your database, comparisons should be case-insensitive. Thus, you might not need the LOWER.

Code example:

SqlCommand cmd = new SqlCommand();
StringBuilder sqlBuilder = new StringBuilder();
sqlBuilder.Append("SELECT name, memberid FROM members ");

var i = 1;
foreach (string item in keywords)
{
    sqlBuilder.Append(i == 1 ? " WHERE " : " AND ");
    var paramName = "@searchitem" + i.ToString();
    sqlBuilder.AppendFormat(" Name LIKE {0} ", paramName); 
    cmd.Parameters.AddWithValue(paramName, "%" + item + "%");

    i++;
}
cmd.CommandText = sqlBuilder.ToString();
Heinzi
  • 167,459
  • 57
  • 363
  • 519
  • Good suggestions. I have done away with the `AND` part. Also, am not using a case sensitive db collation, thus i think i will do away with the `LOWER` keyword too. How do i got about using the `%` tokens into the parameter? – mutiemule Dec 03 '13 at 08:31
  • Helps much but still no results. When i step through the code, the query is `SELECT name, memberid FROM members WHERE Name LIKE @searchitem1 AND Name LIKE @searchitem2`. When i copy this into a query editor and manually enter the search keywords as `SELECT name, memberid FROM members WHERE Name LIKE '%john%' AND Name LIKE '%smith%'` it returns results. What could be the problem? – mutiemule Dec 03 '13 at 09:05
  • @mutiemule: Sounds like the problem might be elsewhere. What do you do with the SqlCommand afterwards? Do you get any error? – Heinzi Dec 03 '13 at 09:25
  • Sorry my mistake, I were still passing the `sqlBuilder.ToString()` string to the return statement instead of passing `cmd`. Works fine now. Thank you indeed. – mutiemule Dec 03 '13 at 09:52
  • I know this is old but I'm curious how cmd.Parameters.AddWithValue(paramName, "%" + item + "%"); is actually working since cmd hasn't been associated to anything besides an default SQLCommand() object. I would think this would fail to replace anything. – Victor Learned May 28 '15 at 19:09
  • 1
    @VictorLearned: AddWithValue does not replace anything. The SqlCommand simply *stores* the parameter for later use. Likewise, when `CommandText` is assigned to, again, SqlCommand just *stores* the string. Later, when the command is actually *executed* (not shown in the code example above), both the command string and the parameters are sent to SQL Server, which does whatever magic is necessary to ensure that *this* SQL command is executes with *these* parameters. – Heinzi May 28 '15 at 19:30
  • @Heinzi: Ah okay. Thanks for the clarification! – Victor Learned May 28 '15 at 19:35
4

Do not put the wildcard characters in your querystring, but add them to your parameter-value:

sql = "SELECT name FROM members WHERE Name LIKE @p_name";
...
cmd.Parameters.AddWithValue("@p_name", "%" + item + "%");

When you add the wildcard characters inside your query-string, the parameter will be escaped, but the wildcard chars will not; that will result in a query that is sent to the DB that looks like this:

SELECT name FROM members WHERE Name LIKE %'somename'%

which is obviously not correct.

Next to that, you're creating a SqlCommand in a loop which is not necessary. Also, you're creating parameters with a non-unique name, since you're adding them in a loop, and the parameter always has the same name. You also need to remove the last AND keyword, when you exit the loop.

Frederik Gheysels
  • 56,135
  • 11
  • 101
  • 154