How to embed @Parameters in a string passed to SQL Server that updates a Comment field

Question

Below is the code I am trying to get to work, and think I have tried this 100 ways at this point. The code 
below does not work and update the comment field with the entire string and none of the numeric parameters. 
 
I need this update to work and I need to build dynamically passing in three parameters:
Update Database.dbo.ReportLog set Comment = 'SubjectStatus #132 and Retention #132 were used to build this report where ReportID = 179.'
Set @ParmDefinition = 'N @MaxSubjectStatusReportID varchar (20),@MaxRetentionReportID varchar (20),@ReportID nvarchar(20)'
Set @CMDString = 'Update ' + @DestDBName + '.dbo.ReportLog set Comment = ' + 
"'SubjectStatus #' + @MaxSubjectStatusReportID +  ' and Retention #' + @MaxRetentionReportID +
' were used to build this report where ReportID = ' + @ReportID + '.'"

Exec sp_executesql @CMDString, @ParmDefinition, @MaxSubjectStatusReportID,@MaxRetentionReportID,@ReportID
If @Debug = 1 Print @CMDString
Set @CMDString = ''

Answer 1

Take a look at the following:

Set @ParmDefinition = N'@MaxSubjectStatusReportID varchar (20),@MaxRetentionReportID varchar (20),@ReportID nvarchar(20)'


DECLARE @CMDString nvarchar(max);

Set @CMDString = 'Update ' + @DestDBName + '.dbo.ReportLog set Comment = ' + 
'SubjectStatus #' + convert(varchar,@MaxSubjectStatusReportID) +  ' and Retention #' + 
CONVERT(varchar,@MaxRetentionReportID) +
' were used to build this report where ReportID ='+convert(varchar, @ReportID)+'.';


Exec sp_executesql @CMDString, @ParmDefinition, @MaxSubjectStatusReportID, @MaxRetentionReportID, @ReportID
If @Debug = 1 Print @CMDString
Set @CMDString = ''

---

Kalman Toth SQL 2008 GRAND SLAM
Paperback: Pass SQL Exam 70-461 & Job Interview: Programming SQL Server 2012

Answer 2

Thanks for the reply but this did not work. All of the @Parameters are defined as varchar (20) at the beginning of this stored procedure so there is no need to convert them to varchar.  The error I got when running it this way is:

Msg 102, Level 15, State 1, Line 1

Incorrect syntax near '#134'.

Update COMETREPORT.dbo.ReportLog set Comment = SubjectStatus #134 and Retention #153 were used to build this report where ReportID =179.

You can see from the Update that 'SubjectStatus #134 and Retention #153 were used to build this report where ReportID =179.' should have single quotes around it.  I can't seem to get these around it AND pass in the three numberic parameters.

Any one else have an idea??

Answer 3

Try:

/* This is to test

use tempdb
create table ReportLog (ReportId int, Comment varchar(max))
insert into ReportLog (ReportId) values (179)

-- Should be parameters to SP
declare @MaxSubjectStatusReportID varchar (20),@MaxRetentionReportID varchar (20),@ReportID int

select @MaxSubjectStatusReportID ='132',@MaxRetentionReportID =132,@ReportID =179;

*/

declare @CMDString nvarchar(max), @ParmDefinition nvarchar(1000), @DestDBName sysname = 'TempDb';
Set @ParmDefinition = N'@MaxSubjectStatusReportID varchar (20),@MaxRetentionReportID varchar (20), @ReportID int';
Set @CMDString = 'Update ' + @DestDBName + '.dbo.ReportLog set Comment = ''SubjectStatus #'' + 
@MaxSubjectStatusReportID +  '' and Retention #'' + @MaxRetentionReportID +
'' were used to build this report.''  
where ReportID = @ReportID';
--
--If @Debug = 1 
Print @CMDString
Exec sp_executesql @CMDString, @ParmDefinition, @MaxSubjectStatusReportID,@MaxRetentionReportID,@ReportID
--
-- To test result ---
select * from dbo.ReportLog 

---

For every expert, there is an equal and opposite expert. - Becker's Law


My blog

Answer 4

This script works for me. Things to note -

• Using NVARCHAR as datatype
• Using two quote signs while building the string wherever one is required

CREATE TABLE ReportLog 
  (  ReportId int, 
     Comment  varchar(200) 
  ) 

INSERT INTO ReportLog (ReportId, Comment) 
VALUES      ( 179, NULL ), ( 180, NULL ) 

SELECT * 
FROM   ReportLog 

DECLARE @ParmDefinition           nvarchar(200), 
        @CMDString                nvarchar(200), 
        @DestDBName               nvarchar(100), 
        @MaxSubjectStatusReportID nvarchar(10), 
        @MaxRetentionReportID     nvarchar(10), 
        @ReportID                 nvarchar(10), 
        @Debug                    int = 1 

SET @ParmDefinition = N'@MaxSubjectStatusReportID nvarchar(20), @MaxRetentionReportID nvarchar(20), @ReportID nvarchar(20)'
SET @DestDBName = N'MyDatabase' 
SET @CMDString = N'Update ' + @DestDBName +
                 N'.dbo.ReportLog set Comment = 
                 N''SubjectStatus #'' +@MaxSubjectStatusReportID+ 
                 N'' and Retention #'' +@MaxRetentionReportID+ 
                 N'' were used to build this report.'' where ReportID = @ReportID'
 
--SELECT @CMDString
EXEC sp_executesql
        @statement               = @CMDString,
        @params                  = @ParmDefinition,
        @MaxSubjectStatusReportID= N'132',
        @MaxRetentionReportID    = N'132',
        @ReportID                = N'179'

IF @Debug                        = 1
PRINT @CMDString
SET @CMDString = '' 
SELECT * 
FROM   ReportLog

---

Please mark this reply as the answer or vote as helpful, as appropriate, to make it useful for other readers.
Thanks!
Aalam | Blog (http://aalamrangi.wordpress.com)

Answer 5

The database name can also be put in the dynamic string if you need it that way. Please let me know if need to try that.

---

Please mark this reply as the answer or vote as helpful, as appropriate, to make it useful for other readers.
Thanks!
Aalam | Blog (http://aalamrangi.wordpress.com)

Answer 6

Here is the final code below that worked.  I was also making a mistake with needing to insert the string 'where ReportID = @ReportID' along with using the where clause 'Where ReportID = @ReportID' at the very end to filter on the actual ReportID.  Thank you to all who replied.  Naomi, you have gotten me on the path once again! I think I am picking up string manipulation and passing these crazy parameters in and out of all these stored procedures. Thank you! 

If @Debug = 1 Print 'Step 17'
Declare @ReportID2 varchar (20)
Set @ReportID2 = @ReportID
Set @ParmDefinition = N'@MaxSubjectStatusReportID varchar (20),@MaxRetentionReportID varchar (20),@ReportID2 nvarchar(20),@ReportID nvarchar(20) ';
Set @CMDString = 'Update ' + @DestDBName + '.dbo.ReportLog set Comment = ''SubjectStatus #'' 
+ @MaxSubjectStatusReportID +  '' and Retention #'' + @MaxRetentionReportID + 
'' were used to build this report where ReportID = '' + @ReportID + ''.'' where ReportID = @ReportID';

Exec sp_executesql @CMDString, @ParmDefinition,@MaxSubjectStatusReportID,@MaxRetentionReportID,@ReportID2,@ReportID
If @Debug = 1 Print @CMDString
Set @CMDString = '' 

Answer 7

Are you doing it this way simply because you are wanting to use multiple database names? if so I'd suggest that from a security and best practice point of view that you shouldn't be using this approach. Queries such as these are open to SQL injection attacks and they do not maintain any standards when it comes to transaction management and data integrity.

I'd suggest that you should either rewrite your applications data layer to overcome this or you could use the SQL Service broker to act as a bridge between your various databases. 

http://msdn.microsoft.com/en-us/library/ms345108(v=sql.90).aspx

---

---
Shaun Turner

My Blog | My LinkedIn

If you're in the UK please join The SQL Developers / DBA's user group for the UK on LinkedIn

Answer 8

Shaun,

I am aware of the risks of SQL injection.  This is part of a report engine that exists within a corporation and will be used in a corporate Intranet  only. Unfortunately, there was no data layer (3-tier implementation) when I came into this project and it is too late to implement anything like that.  This also all embedded within stored procedures where the parameters are passed.

---

Steven DeSalvo

Answer 9

Steven,

I'm afraid so say that whenever you put variables in to a code like this then there is some level of risk with injection attack, even if you're on a corp network you should assume that your system will be subject to abuse from someone who knows a little too much (i've seen it in the past).

I understand why you've done it this way however it means that your db is no longer standalone, any cross database communication means that you end up with an amount of risk around processes. If you need to distribute the query in this manner for legitimate reasons then I'd still recommend using the service broker to manage this, it'll mean that in the future you can move the database to another server, etc and only change configuration not have to change everything.

If however you're determined to maintain this approach might i suggest that you amend your script as follows:

Set @ParmDefinition = N'@MaxSubjectStatusReportID INT,@MaxRetentionReportID INT,@ReportID2 INT,@ReportID INT ';

Set @CMDString = 'Update ' + @DestDBName + '.dbo.ReportLog set Comment = ''SubjectStatus #'' 
+ CONVERT(NVARCHAR(MAX), @MaxSubjectStatusReportID) +  '' and Retention #'' + CONVERT(NVARCHAR(4000), @MaxRetentionReportID) + 
'' were used to build this report where ReportID ='' + CONVERT(NVARCHAR(4000), @ReportID) + ''.'' where ReportID = @ReportID';

Or even better you could call directly to a stored procedure on the other end

-- on all of the destination db's
CREATE PROCEDURE dbo.uspUpdateReportComment
	@MaxSubjectStatusReportID INT,
	@MaxRetentionReportID INT,
	@ReportID2 INT,
	@ReportID INT
AS
Update dbo.ReportLog set Comment = 'SubjectStatus #' + CONVERT(NVARCHAR(MAX), @MaxSubjectStatusReportID) +  ' and Retention #' + CONVERT(NVARCHAR(4000), @MaxRetentionReportID) + 
' were used to build this report where ReportID =' + CONVERT(NVARCHAR(4000), @ReportID) + '.' where ReportID = @ReportID
GO

-- on the other db
Set @CMDString = '[' + @DestDBName + '].[dbo].[uspUpdateReportComment]'
Exec @CMDString @MaxSubjectStatusReportID,@MaxRetentionReportID,@ReportID2,@ReportID

This should be a lot cleaner

---

---
Shaun Turner

My Blog | My LinkedIn

If you're in the UK please join The SQL Developers / DBA's user group for the UK on LinkedIn

Answer 10

Shaun, thank you for your thoughtful considerations and taking the time to write this.  I make the changes I can at this time.

---

Steven DeSalvo

Answer 11

Shaun,

All of these stored procedures I am writing are part of a report engine.  The only thing calling them is SQLAgent scheduler.  I am not aware of any SQL Injection issues through SQL Agent, are you?

---

Steven DeSalvo

Answer 12

Of course SQL Agent (or other scheduled processes) are not an issue by themselves. The issue would be that the stored procs are out there and thus pose a potential risk if used maliciously. The fact that these procs are not used via any external-facing web site/app is some consolation but there is still a chance of internal misuse un/intentionally. Using SP_EXECUTE helps in avoiding SQL injection by parameterising the dynamic SQL and is lot better than simply running the dynamic SQL with EXEC command.

---

Please mark this reply as the answer or vote as helpful, as appropriate, to make it useful for other readers.
Thanks!
Aalam | Blog (http://aalamrangi.wordpress.com)

Answer 13

Did you mean SP_EXECUTESQL?  Can you give an example of this construction?

---

Steven DeSalvo

Answer 14

My bad! It is a typo above. Yes, I meant SP_EXECUTESQL. That is what we are using in our examples in this thread.

Not sure if you are asking for an example of an SQL injection construction. If yes, then here is what I wrote to demonstrate -

----------------------
-- With EXECUTE
----------------------
DECLARE @q NVARCHAR(200)

DECLARE @tname SYSNAME
DECLARE @wildcard NVARCHAR(50)

-- Set the parameters. These can be passed through the front-end apps
SET @tname = 'sysobjects'
SET @wildcard = 'sys'

-- Build query dynamically from parameters passed
SET @q = N'SELECT * FROM ' + quotename(@tname) + ' WHERE NAME LIKE ''' + @wildcard + '%'''
EXECUTE (@q)


-- Set the parameters that can cause SQL Injection. These can be passed through the front-end apps
-- putting the following string potentially allows malicious code to be injected
-- %''; select @@servername;--

SET @tname = 'sysobjects'
SET @wildcard = 'sys%''; select @@servername;--'

-- Build query dynamically from parameters passed
SET @q = N'SELECT * FROM ' + quotename(@tname) + ' WHERE NAME LIKE ''' + @wildcard + '%'''
EXECUTE (@q)

GO

----------------------
-- With SP_EXECUTESQL
----------------------
DECLARE @q NVARCHAR(200)

DECLARE @p NVARCHAR(200)
SET @p = N'@wildcard NVARCHAR(50)'

DECLARE @tname SYSNAME
DECLARE @wildcard NVARCHAR(50)

-- Set the parameters. These can be passed through the front-end apps
SET @tname = 'sysobjects'
SET @wildcard = 'sys'

-- Build query dynamically from parameters passed
SET @q = N'SELECT * FROM '+ quotename(@tname) +' WHERE NAME LIKE @wildcard + ''%'' '
print @q

EXEC sp_executesql
        @statement = @q,
        @params    = @p,
        @wildcard  = @wildcard


-- Set the parameters that can cause SQL Injection. These can be passed through the front-end apps
-- putting the following string potentially allows malicious code to be injected
-- %''; select @@servername;--

SET @tname = 'sysobjects'
SET @wildcard = 'sys%''; select @@servername;--'

-- Build query dynamically from parameters passed
SET @q = N'SELECT * FROM '+ quotename(@tname) +' WHERE NAME LIKE @wildcard + ''%'' '
print @q

EXEC sp_executesql
        @statement = @q,
        @params    = @p,
        @wildcard  = @wildcard

---

Please mark this reply as the answer or vote as helpful, as appropriate, to make it useful for other readers.
Thanks!
Aalam | Blog (http://aalamrangi.wordpress.com)