7

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?

Improve this question
2
  • 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. Commented Dec 3, 2013 at 8: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. Commented Dec 3, 2013 at 8:10

2 Answers 2

Reset to default
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();
Improve this answer
Sign up to request clarification or add additional context in comments.

6 Comments

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?
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: Sounds like the problem might be elsewhere. What do you do with the SqlCommand afterwards? Do you get any error?
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.
@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.
|
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.

Improve this answer

2 Comments

Valid point, but it's not the reason why his query doesn't work.
Please note that am using an sqlbuilder. Tried getting this work in the sqlbuilder to no avail.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.