Building a dynamic sql query from VB.NET

Question

I have searched and searched for an answer for this and I cannot find one. I have a checked list box where the user will select issues they are experiencing with their computer. In the checked list box you will see things like... Slow, Viruses, Bad Hard Drive... and based on what they select I will tell them an estimated cost for repair. Currently this is how I build my query:

Dim mIssues As String = ""

For i = 0 To lstIssues.CheckedItemsCount - 1
 If mIssues = "" Then
  mIssues = String.Format("IssueName = '{0}'", lstIssues.CheckedItems(i))
 Else
  mIssues = String.Format("{0} OR IssueName = '{1}'", mIssues, lstIssues.CheckedItems(i))               
 End If
Next

The above code will look to see how many issues they have selected. If they only select one issue then it will return a string like this: IssueName = 'Whatever they selected'. If they have selected more than one issue it will return a string like this: IssueName = 'Whatever they selected' OR IssueName = 'The second selection'. So basically I will append an OR between all the selections if they select more than one issue. I do this to dynamically build my where clause in my query.

Here is my query:

Dim mySQL As String = "SELECT IssueID, IssueTypeID, IssueName, IssueDescription, " _
  & "CustomerID, IndividualCost, GroupCost, Active, ChargeType " _
  & "FROM (SELECT IssueID, IssueTypeID, IssueName, IssueDescription, " _
  & "CustomerID, IndividualCost, GroupCost, Active, ChargeType " _
  & "FROM(cfg_Issues) " _
  & "WHERE " & mIssues & " " _
  & "GROUP BY IssueID, IssueTypeID, IssueName, IssueDescription, CustomerID, " _
  & "IndividualCost, GroupCost, Active) " _
  & "ORDER BY IndividualCost DESC, GroupCost ASC;"

As you can see my where clause comes from the first section of code. My question is this, is there a better way to do this??? I know there has to be a better way to build a dynamic where clause query and I would like to see how. Thank you for any guidance you can help me with.

Answer 1

The first problem here is the open door to SQL Injection. I hope you have a complete control on what is inserted in your lstIssues because the string concatenation is Always a Dangerous thing when you create commands for a database engine.

Your code could be reduced using a StringBuilder class instance that helps when you have many elements to concatenate in strings

Dim mIssues As StringBuilder  = new StringBuilder()

For i = 0 To lstIssues.CheckedItemsCount - 1
  mIssues.AppendFormat("IssueName = '{0}' OR ", lstIssues.CheckedItems(i))
Next

' I suppose that you have a check in place to not allow to run this query if you don't have at 
' least one element checked in the list (if not the WHERE condition will fail)'
mIssues.Length -= 4

This will remove the IF inside the loop and, to remove the extra OR, it is enough reduce the length of the StringBuilder instance when you exit the loop.
In your query text the StringBuilder could return its internal string using

mIssues.ToString

You could also try to use the IN sql clause with code like this

Dim mIssues As StringBuilder  = new StringBuilder()

For i = 0 To lstIssues.CheckedItemsCount - 1
  mIssues.AppendFormat("'{0}', ", lstIssues.CheckedItems(i))
Next

' I suppose that you have a check in place to not allow this query if you don't have at 
' least one element checked in the list (if not the WHERE condition will fail)'  
mIssues.Length -= 2
mIssues.Insert(0, "IssueName IN(")
mIssues.Append(")")

Answer 2

I have always preferred to have SQL do all the dynamic work for me. The advantages, as many people are showing is all your SQL is done behind the scenes and is more secure and cant be injected. There are ways that evil people can actually (I would rather not explain the details of the hack itself), inject their own SQL into your form and potentially do lots of damage.

Here is the basic premise behind Dynamic SQL (code may not be exact): You create a stored procedure that essentially writes the SQL code and then executes it

CREATE PROCEDURE p_getIssues
 as 

/*This is where you will place all your input parameters like this
 @paramname1  datatype,
 @paramname1  datatype,

 @mIssues varchar(max)   example: virus = 1 and slowcomp = 1


*/
BEGIN

 DECLARE @SQL as varchar(max)
 SET @SQL = 'SELECT * FROM tablename where ' + @mIssues 

 EXEC (@SQL)

END

Note: If you ever have to debug the sproc... comment out the EXEC and ADD A PRINT(@SQL) and you can see what SQL the sproc will run.

If you have to go dynamic this is always a preferred method to go. Also on a side note sql inside your VB.NET code is not recommended IMHO because then from a scalability and updateability standpoint, if you were to add more services, you have to push out new VB.NET code. Instead, you log into SQL and you can make the changes and you are done!