User Search SQL Builder

Question

Using the VS2012 code analyzer i see the following code has a pretty low Maintainability Index(40) but i cant seem to think of a better way to design it. Does anyone have suggestions on cleaning this up?

This code is part of a search screen for a web app I'm developing. They have several text fields they can use as well as some select lists. Is there any way to simplify this or make it cleaner? I'm more interested in cleaning the VB up but if someone notices my sql has issues, im open for that too =-)

Edit: As some commentators correctly pointed out, this code doesn't sanitize the input. Most of the time I use AddInParameter but since this is a bit more dynamic, i decided to sanitize in the BLL before even calling this function. I figured it would help keep this DAL code cleaner if i did it in the BLL instead

Shared Function SearchSession(searchCriteria As SessionSearchCriteria) As List(Of Sessions_BO)
    Dim Result As New List(Of Sessions_BO)
    Dim DatabaseObject As Database = DatabaseFactory.CreateDatabase(SQLUtils.GetConnectionString)
    Dim SqlString = <![CDATA[
        SELECT DISTINCT 
                     Sessions.sessionId, Sessions.copiedSessionId, Sessions.sessionTitle, Sessions.initialDate,
                     Sessions.sessionDescription

        FROM         Sessions LEFT OUTER JOIN
                     CurriculumSessionLink ON Sessions.sessionId = CurriculumSessionLink.sessionId LEFT OUTER JOIN
                     CurriculaHeader ON CurriculumSessionLink.curriculumId = CurriculaHeader.curriculumId LEFT OUTER JOIN
                     TargetLearnerCurriculum ON CurriculaHeader.curriculumId = TargetLearnerCurriculum.curriculumId LEFT OUTER JOIN
                     CurriculumKeywords ON CurriculaHeader.curriculumId = CurriculumKeywords.curriculumId LEFT OUTER JOIN
                     SessionAuthors ON Sessions.sessionId = SessionAuthors.sessionId LEFT OUTER JOIN
                     CurriculumKeywords AS CK ON CurriculaHeader.curriculumId = CK.curriculumId LEFT OUTER JOIN
                     SessionTypeSessions ON Sessions.sessionId = SessionTypeSessions.sessionId

    ]]>.Value

    If (Not String.IsNullOrEmpty(searchCriteria.SearchSessionTitle)) _
        Or (Not String.IsNullOrEmpty(searchCriteria.SearchSessionDateFrom)) _
        Or (Not String.IsNullOrEmpty(searchCriteria.SearchSessionDateTo)) _
        Or (searchCriteria.SessionType <> -1) _
        Or (searchCriteria.SessionAuthor <> -1) _
        Or (Not searchCriteria.SessionSearchKeywords Is Nothing) _
        Or (Not searchCriteria.TargetLearners Is Nothing) _
        Or (searchCriteria.SessionCurriculum <> -1) Then

        Dim whereStr As String = BuildWhereClause(searchCriteria)
        SqlString &= If(whereStr.Length > 0, " WHERE " & whereStr, "")
    End If


    Try
        Using DatabaseCommand = DatabaseObject.GetSqlStringCommand(SqlString)

            Using DataReader As IDataReader = DatabaseObject.ExecuteReader(DatabaseCommand)
                While DataReader.Read()
                    Result.Add(FillSessions_BO(DataReader))
                End While
            End Using
        End Using
    Catch ex As Exception
        Dim err As New ErrorLog_BO
        err.ErrorText = ex.Message
    End Try

    Return Result
End Function

Shared Function BuildWhereClause(searchCriteria As SessionSearchCriteria) As String
    Dim whereStr As New StringBuilder()

    If Not String.IsNullOrEmpty(searchCriteria.SearchSessionTitle) Then
        whereStr.Append(" ( Sessions.sessionTitle LIKE '%" & searchCriteria.SearchSessionTitle & "%') ")
    End If

    If searchCriteria.SessionAuthor <> -1 Then
        If whereStr.Length > 0 Then
            whereStr.Append(" AND ")
        End If

        whereStr.Append(" (SessionAuthors.authorId = " & searchCriteria.SessionAuthor & ") ")

    End If

    If searchCriteria.SessionType <> -1 Then
        If whereStr.Length > 0 Then
            whereStr.Append(" AND ")
        End If

        whereStr.Append(" (SessionTypeSessions.sessionTypeId =  " & searchCriteria.SessionType & ") ")
    End If

    If searchCriteria.SessionCurriculum <> -1 Then
        If whereStr.Length > 0 Then
            whereStr.Append(" AND ")
        End If
        whereStr.Append(" (CurriculumSessionLink.curriculumId = " & searchCriteria.SessionCurriculum & " ) ")
    End If

    If Not String.IsNullOrEmpty(searchCriteria.SearchSessionDateFrom) And Not String.IsNullOrEmpty(searchCriteria.SearchSessionDateTo) Then
        If whereStr.Length > 0 Then
            whereStr.Append(" AND ")
        End If
        whereStr.Append("( initialDate BETWEEN '" & searchCriteria.SearchSessionDateFrom & "' AND '" & searchCriteria.SearchSessionDateTo & "')")
    End If

    If Not searchCriteria.SessionSearchKeywords Is Nothing Then
        If whereStr.Length > 0 Then
            whereStr.Append(" AND ")
        End If

        whereStr.Append("(")
        For i As Integer = 0 To searchCriteria.SessionSearchKeywords.Count - 1
            whereStr.Append(" CurriculumKeywords.keywordId = " & searchCriteria.SessionSearchKeywords(i) & " ")
            If i < searchCriteria.SessionSearchKeywords.Count - 1 Then
                whereStr.Append(" OR ")
            End If
        Next
        whereStr.Append(")")

    End If

    If Not searchCriteria.TargetLearners Is Nothing Then
        If whereStr.Length > 0 Then
            whereStr.Append(" AND ")
        End If

        whereStr.Append("(")
        For i As Integer = 0 To searchCriteria.TargetLearners.Count - 1
            whereStr.Append(" TargetLearnerCurriculum.targetLearnerId = " & searchCriteria.TargetLearners(i) & " ")
            If i < searchCriteria.TargetLearners.Count - 1 Then
                whereStr.Append(" OR ")
            End If
        Next
        whereStr.Append(")")
    End If



    Return whereStr.ToString()

End Function

Answer 1

You are getting a "low Maintainability Index(40)" because you have no separation of concerns here.

Your DAL and SQL Server queries are two totally different things, and then you speak of your Business Logic Layer.

You should never create an entire query string in C#, VB, Python, C, C++, Java, PHP, or any other programming language that you can think of, it is not practical and removes responsibility from the Database.

Here is what you should do

• Create a Stored Procedure on the Database.
• Gather and Sanitize Input
• Run the Input through your Business Logic Layer
• Send the Input to the DAL
  • Create a class that bundles the information and sends it to the Database via SQLCommand, SQLConnection, and other SQL classes

From what I can tell, you will need several Stored Procedures, and doing this will make maintenance of the application much easier.

Answer 2

As some commentators correctly pointed out, this code doesn't sanitize the input. Most of the time I use AddInParameter but since this is a bit more dynamic, i decided to sanitize in the BLL before even calling this function. I figured it would help keep this DAL code cleaner if i did it in the BLL instead

You have just polluted your business logic with a concern that you shouldn't even have to care for.

Don't. Sanitize. Input. Manually.

It's not because you can't crack it, that nobody can. I've seen an "input sanitizer" homebrewed function insert log entries whenever a "blacklisted keyword" was found in the user input. Sounds great, you get a log entry saying "SqlInjection" in your database, with the offending input, with single quotes doubled up. So if you tried good old "Robert'); DROP TABLE Students;--" it would insert a log entry saying "Robert''); DROP TABLE Students;--" was intercepted. The problem is that if the user input was "Robert'); drop table Students;--" the "sanitizer" function wouldn't pick it up, because... the comparison was case-sensitive. Yeah, like that. And I never managed to crack out of the single quote, but I don't care, someone out there will end up escaping it in a clever way I never would have thought possible.

The solution is simple: don't sanitize user input. Don't pretend you can cover all possible angles, there are vulnerabilities and exploits that make SQL injection attacks possible, that no one in their right minds could possibly prevent. All you get, is a false sense of security.

---

One way of efficiently protecting your code from SQL injection attacks, is to use stored procedures, as @Malachi pointed out. The reason why this is safe, is because you have no choice but to pass it parameters, and let the server deal with knowing everything there is to know about parameters, be it their type, or their value.

In your case a stored procedure could very well do the trick, your SQL is really much less "dynamic" than you think it is.

Say you have a table called Books, joined with another table called Authors. You want to be able to search by book title, subtitle, collection or publication year, and/or by author's name or nationality.

This looks like you need something very dynamic, but in reality you don't - consider this:

SELECT book.Id
FROM dbo.Books book
 INNER JOIN dbo.Authors author ON book.AuthorId = author.Id
WHERE book.Title LIKE '%' + ISNULL(@searchTitle, book.Title) + '%'
 AND book.SubTitle LIKE '%' + ISNULL(@searchSubTitle, book.SubTitle) + '%'
 AND book.Collection = LIKE '%' + ISNULL(@searchCollection, book.Collection) + '%'
 AND book.PublicationYear = ISNULL(@searchPublicationYear, book.PublicationYear)
 AND author.Name LIKE '%' + ISNULL(@searchAuthorName, author.Name) + '%'
 AND author.Nationality LIKE '%' + ISNULL(@searchAuthorNationality, author.Nationality) + '%'

All possible search fields are in the WHERE clause, and each search term matches a named parameter, which could very well be NULL (/unspecified) - in which case the WHERE clause takes the value as it exists in the table.

From the code's point of view, you could have a method (or perhaps an extension method that only your DAL knows about?) on your SessionSearchCriteria class that spits out an IEnumerable<SqlParameter> with an SqlParameter for each search criteria.

---

If you don't want to be calling a stored procedure, nothing forbids having the exact same query in your DAL code as a plain String - as long as you keep the parameters and aren't concatenating anything.

The reason why you want to avoid dynamically coming up with an SQL statement isn't just for security, it's also for performance: with named parameters and a query that's always identical, the server can reuse an existing execution plan from a previous execution, and you'll be getting subsequent search results faster than if the server had to calculate a new execution plan every time.

And as a bonus, you no longer need to surround string parameter values with single quotes, you no longer need to "sanitize" the values (and why wouldn't "'); Drop Table Students;--" be a valid book title anyway?) - it's the database server that takes care of sticking parameter values into the query.. not your code.

---

Some random observations

• I don't know enough vb.net to tell if it's good practice or not, but Dim SqlString = <![CDATA[...]]>.Value seems like a funky way of declaring and assigning a String.

• Does DatabaseFactory.CreateDatabase actually create the database, or it just connects to an existing one? If the latter, then your naming is misleading - you're creating a data context, not a database.

• Dim DatabaseObject As Database - appending Object to the end of an identifier is only clutter - everything you instantiate is an object, no need to tell the world about it ;)

• SQLUtils.GetConnectionString - the name of this static (shared) class scares me. Connection strings should be in your web.config / app settings, and retrieved with a ConfigurationManager - if that's what SQLUtils is doing, fine. But the name SQLUtils (or SqlUtils as it should be) raises a big flashy red flag: this class is a potential dumping bag for anything more or less related to dealing with SQL.

• Using DatabaseCommand = DatabaseObject.GetSqlStringCommand(SqlString) - I like that you're Using disposables here, that's a very good thing to do. However GetSqlStringCommand should be taking an IEnumerable<SqlParameter> argument, so that the caller knows not to concatenate parameter values into the SqlString.

• Dim err As New ErrorLog_BO | err.ErrorText = ex.Message this looks suspicious to me. You're creating a new object, and assigning a property value. You lose the precious stack trace, and no one seeing this assignment would expect that setter to have the side-effect of actually performing the error logging. On the other hand, logger.LogError(ex) would be crystal-clear.