Stored Procedure, building SQL statement

Question

Ref: I looked at SQL Server stored procedure parameters to get a start.

Issue:

I want to make different SQL statements depending on the data passed in via the stored procedure. This example deals with one version but there will be several variations and I want to keep my code consise.

Error:

Msg 102, Level 15, State 1, Procedure spSearchGrid, Line 60 Incorrect syntax near '@SQL'. Msg 103, Level 15, State 4, Procedure spSearchGrid, Line 60 The identifier that starts with 'SELECT p.ID AS ID, p.UPRN AS UPRN, COALESCE(a.OverallRiskCategory,'Unknown') AS OverallRiskCategory, COALESCE(a.TypeOfUtility,'U' is too long. Maximum length is 128. Msg 102, Level 15, State 1, Procedure spSearchGrid, Line 65 Incorrect syntax near '@SQL'. Msg 103, Level 15, State 4, Procedure spSearchGrid, Line 66 The identifier that starts with 'a.SurveyDate between @sDateFrom and @sDateTo AND (p.UPRN LIKE '%' + @sUPRN + '%' or p.PostCode LIKE '%' + @sPostcode + ' is too long. Maximum length is 128. Msg 102, Level 15, State 1, Procedure spSearchGrid, Line 76 Incorrect syntax near 'END'.

Tried:

I have tried using both single and double speechmarks around the outside but this has not helped fixed the issue.

Code:

USE [Database]
GO
/****** Object:  StoredProcedure [dbo].[spSearchGrid]    Script Date: 18/06/2015 15:14:06 ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
-- =============================================
ALTER PROCEDURE [dbo].[spSearchGrid]

@sUPRN varchar(150),
@sPostcode varchar(20),
@sDateFrom datetime,
@sDateTo datetime,
--@sUCARN varchar(20),
@sPropertyName varchar(20),
@sStreet varchar(150),
@sSurveyCompany  varchar(150),
@sRiskRating varchar(150),
@sRegion varchar(150)
-- Add the parameters for the stored procedure here
 --@test1 VARCHAR(30) OUTPUT

AS

BEGIN
--and 
DECLARE @SQL VARCHAR(MAX)

 If @sUPRN = 'Test' 
 BEGIN
    @SQL = SELECT p.ID AS ID, p.UPRN AS UPRN, COALESCE(a.OverallRiskCategory,'Unknown') AS OverallRiskCategory, COALESCE(a.TypeOfUtility,'Unknown') AS TypeOfUtility, COALESCE(a.SurveyDate,'') AS SurveyDate, COALESCE(a.ItemRef, '') AS ItemRef, COALESCE(a.NextAsbestosSurveyDue,'') AS NextAsbestosSurveyDue , COALESCE(a.Recommendations,'NO DATA') AS Recommendations, COALESCE(a.StatusOfIssue,'0') As StatusOfIssue 
    FROM TblProperty AS p LEFT  JOIN TblAsbestos AS a on  a.UPRN = p.UPRN WHERE

    IF LTRIM(RTRIM(@sRiskRating)) = '1234xyz'

    @SQL += a.OverallRiskCategory = LTRIM(RTRIM(@sRiskRating)) AND 
    @SQL += a.SurveyDate between @sDateFrom and @sDateTo AND (p.UPRN LIKE  '%' + @sUPRN + '%'   or
                    p.PostCode LIKE '%' + @sPostcode + '%'  or
                    p.ShopName LIKE '%' + @sPropertyName + '%'  or
                    p.Street LIKE '%' + @sStreet + '%'  or
                    p.Reg = @sRegion  or
                    a.SurveyCompany LIKE '%' + @sSurveyCompany + '%' )
 END

  --PRINT(@SQL)
  EXEC(@SQL)    
END

This has two pretty big problems. The first and most glaring is that this is wide open to sql injection. You need to parameterize your dynamic sql. The second is the performance of this is going to be awful because you have leading wildcards on all those columns. That means that each and every time this runs it has to scan every value of each of those columns. Please see this article for a much better approach in dealing with this type of query. http://sqlinthewild.co.za/index.php/2009/03/19/catch-all-queries/ — Sean Lange, Jun 18 '15 at 16:05
Need wildcard due to the client :-( @SeanLange I'll be breaking it down further :-) Just starting with first parameter and working from there. :-) — indofraiser, Jun 18 '15 at 16:12
The bigger issue is sql injection. Your code is a textbook example. — Sean Lange, Jun 18 '15 at 16:19
Hi @SeanLange how do I ask wildcard search (I know no one likes them but real world logistics requires them in this case) I should mention there are set reports which as used the majority of the time so this is when they need something a little more bespoke. — indofraiser, Jun 19 '15 at 08:37

Radu Gheorghiu · Answer 1 · 2015-06-18T16:16:39.610

You need to make sure your quotes are escaped by adding another quote before it.

Also, there are a few issues when initializing your @SQL variable and when you're trying to append more code to it in the IF part.

Try this:

USE [Database]
GO
/****** Object:  StoredProcedure [dbo].[spSearchGrid]    Script Date: 18/06/2015 15:14:06 ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
-- =============================================
ALTER PROCEDURE [dbo].[spSearchGrid]

@sUPRN varchar(150),
@sPostcode varchar(20),
@sDateFrom datetime,
@sDateTo datetime,
--@sUCARN varchar(20),
@sPropertyName varchar(20),
@sStreet varchar(150),
@sSurveyCompany  varchar(150),
@sRiskRating varchar(150) = NULL,
@sRegion varchar(150)
-- Add the parameters for the stored procedure here
 --@test1 VARCHAR(30) OUTPUT

AS

BEGIN
--and 
DECLARE @SQL VARCHAR(MAX)

 If @sUPRN = 'Test' 
 BEGIN
    SET @SQL = 'SELECT p.ID AS ID, p.UPRN AS UPRN, COALESCE(a.OverallRiskCategory,''Unknown'') AS OverallRiskCategory, COALESCE(a.TypeOfUtility,''Unknown'') AS TypeOfUtility, COALESCE(a.SurveyDate,'''') AS SurveyDate, COALESCE(a.ItemRef, '''') AS ItemRef, COALESCE(a.NextAsbestosSurveyDue,'''') AS NextAsbestosSurveyDue , COALESCE(a.Recommendations,''NO DATA'') AS Recommendations, COALESCE(a.StatusOfIssue,''0'') As StatusOfIssue 
    FROM TblProperty AS p LEFT  JOIN TblAsbestos AS a on  a.UPRN = p.UPRN WHERE'

    IF LTRIM(RTRIM(@sRiskRating)) = '1234xyz'
    BEGIN
    SET @SQL = @SQL + 'a.OverallRiskCategory = LTRIM(RTRIM(@sRiskRating)) AND '
    SET @SQL = @SQL + 'a.SurveyDate between @sDateFrom and @sDateTo AND (p.UPRN LIKE  ''%'' + @sUPRN + ''%''   or
                    p.PostCode LIKE ''%'' + @sPostcode + ''%''  or
                    p.ShopName LIKE ''%'' + @sPropertyName + ''%''  or
                    p.Street LIKE ''%'' + @sStreet + ''%''  or
                    p.Reg = @sRegion  or
                    a.SurveyCompany LIKE ''%'' + @sSurveyCompany + ''%'' )'
    END
 END

  --PRINT(@SQL)
  EXEC(@SQL)    
END

Thanks, didn't realise you had to double up the single spechmarks !! — indofraiser, Jun 18 '15 at 15:55
Will tidy the WHERE part up. That's me cutting the example down to a sensible size so fixing :-0 — indofraiser, Jun 18 '15 at 15:57
@indofraiser You do that. I think syntactically it should be correct and build the string correctly, but I haven't checked the syntax for the actual code that you append to the `@SQL` variable so you'll have to check that yourself. — Radu Gheorghiu, Jun 18 '15 at 16:00
Compiles but I get "Must declare the scalar variable "@sRiskRating".",everything has default value on the .aspx page — indofraiser, Jun 18 '15 at 16:08
Please note, this is still vulnerable to sql injection. You need to parameterize your dynamic sql. — Sean Lange, Jun 18 '15 at 16:17
@indofraiser If you do not pass a value for the `@sRiskRating` parameter of the stored procedure you will need a `DEFAULT` value defined for it in the top of the procedure. I have defaulted it to `NULL`. — Radu Gheorghiu, Jun 18 '15 at 16:17
This is full of logical errors and the sql injection is quite frankly shocking. — Sean Lange, Jun 19 '15 at 13:33

score 0 · Answer 2 · answered Jun 19 '15 at 13:52

I don't see the need at all for dynamic sql here. What you have is a procedure with multiple execution paths based on some parameters. This is quite common and you really don't need to resort to dynamic for this. There seem to be some logical issues with the original logic. If the value of @sUPRN is not 'Test' this procedure doesn't do anything. I am guessing that is not correct but that is up to the OP to determine.

Keeping all this logic in a single procedure is feasible but it has a slight performance hit. Because you need to execute two different queries you probably need to execution plans for the different queries. One way to accomplish this (the way I will post shortly) is to add the recompile option to your queries. This forces the optimizer to throw away the execution plan after it runs so it will always get a fresh compile. This means that every time this procedure runs it will have to recompile it first. A better approach, and one I would take on my system is to create a sub procedure for each path. That allows the plans to be cached for each branch.

Here is how you could do this without any dynamic sql and keeping the performance decent.

ALTER PROCEDURE [dbo].[spSearchGrid]
(
    @sUPRN varchar(150),
    @sPostcode varchar(20),
    @sDateFrom datetime,
    @sDateTo datetime,
    @sPropertyName varchar(20),
    @sStreet varchar(150),
    @sSurveyCompany  varchar(150),
    @sRiskRating varchar(150) = NULL,
    @sRegion varchar(150)
) AS
BEGIN
    SET NOCOUNT ON

    If @sUPRN = 'Test' 
        IF LTRIM(RTRIM(@sRiskRating)) = '1234xyz'
            SELECT p.ID AS ID
                , p.UPRN AS UPRN
                , COALESCE(a.OverallRiskCategory, 'Unknown') AS OverallRiskCategory
                , COALESCE(a.TypeOfUtility, 'Unknown') AS TypeOfUtility
                , COALESCE(a.SurveyDate, '') AS SurveyDate --Is this really a date? If so, this will become 1/1/1900
                , COALESCE(a.ItemRef, '') AS ItemRef
                , COALESCE(a.NextAsbestosSurveyDue, '') AS NextAsbestosSurveyDue
                , COALESCE(a.Recommendations, 'NO DATA') AS Recommendations
                , COALESCE(a.StatusOfIssue, '0') As StatusOfIssue 
            FROM TblProperty AS p 
            LEFT JOIN TblAsbestos AS a on a.UPRN = p.UPRN 
            WHERE a.OverallRiskCategory = LTRIM(RTRIM(@sRiskRating)) 
                AND a.SurveyDate between @sDateFrom and @sDateTo 
                AND 
                (
                    p.UPRN LIKE '%' + @sUPRN + '%'   
                    OR p.PostCode LIKE '%' + @sPostcode + '%'
                    OR p.ShopName LIKE '%' + @sPropertyName + '%'
                    OR p.Street LIKE '%' + @sStreet + '%'
                    OR p.Reg = @sRegion
                    OR a.SurveyCompany LIKE '%' + @sSurveyCompany + '%'
                )
            OPTION (RECOMPILE)
        ELSE
            SELECT p.ID AS ID
                , p.UPRN AS UPRN
                , COALESCE(a.OverallRiskCategory, 'Unknown') AS OverallRiskCategory
                , COALESCE(a.TypeOfUtility, 'Unknown') AS TypeOfUtility
                , COALESCE(a.SurveyDate, '') AS SurveyDate --Is this really a date? If so, this will become 1/1/1900
                , COALESCE(a.ItemRef, '') AS ItemRef
                , COALESCE(a.NextAsbestosSurveyDue, '') AS NextAsbestosSurveyDue
                , COALESCE(a.Recommendations, 'NO DATA') AS Recommendations
                , COALESCE(a.StatusOfIssue, '0') As StatusOfIssue 
            FROM TblProperty AS p 
            LEFT JOIN TblAsbestos AS a on a.UPRN = p.UPRN 
            OPTION (RECOMPILE)
    --there is nothing to do if @sUPRN is not 'Test'???
END

Gail Shaw has an excellent post on her blog about this topic. She also prefers breaking this into a total of 3 procedures but I did not do that here. Read this article and make your own decision about what works for you. http://sqlinthewild.co.za/index.php/2009/09/15/multiple-execution-paths/

Stored Procedure, building SQL statement

2 Answers2