April 21, 2014 at 11:05 am
Hey guys, its beena while since I've been on a team where there is a web app so I'm having to knock the dust off of some things I haven't had to worry about in the last few years. Here's my question:
Say I have the following sproc:
USE [MyDatabase]
GO
/****** Object: StoredProcedure [dbo].[SearchMember] Script Date: 04/21/2014 11:20:38 ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
ALTER procedure [dbo].[SearchMember](
@memberID varchar(16) = null,
@LastName varchar(60) = null,
@Firstname varchar(35) = null,
@SSN varchar(11) = null,
@DOB char(10) = null,
@MedicaidID varchar(60) = null,
@IsCoverageActive char(1) = null,
@IsPDPOnly char(1) = null,
@IsInpatientAllowed char(1) = null,
@maxcnt int = 1000,
@returnCnt int OUTPUT,
@debug bit = 0
)
as
/***********************************************************************************
Object:dbo.SearchMember
created:2013-01-28Linda Parrish
Purpose:search for member in MemberSearch table
Uses dynamic SQL to build query
Notes:Assumes input values for MemberID, last and firstname parms
are the BEGINNING characters of stored values. This will then leverage
indexes on those fields.
2013-02-05add Hic/Medicaid search parameter = full value
2013-02-08MemberID = full value search
2013-02-18Search MemberID and QNXTMedid for the @MemberID value - causes 2 index seeks
2013-0225Added input params for IsCoverageActive and IsPDPOnly
2013-03-06changed datatypes for 2 new parms to char(1)
2013-07-09add input parm for IsInpatientAllowed and add to output
Example:
exec SearchMember @memberID=null, @LastName='smith', @FirstName='jac', @SSN=null, @DOB=null, @MedicaidID=null, @IsCoverageActive=0, @IsPDPOnly=0, @maxcnt=100, @returnCnt=100 , @debug=1
***********************************************************************************/
set nocount on;
declare @sql NVarchar(2000);
declare @sqlcnt NVarchar(2000);
declare @where nvarchar(2000);
declare @recCnt int;
declare @first bit;
set @first = 1;
set @sqlcnt = N'Select @recCnt = count(*) from MemberSearch where ';
declare @outparm nvarchar(30);
set @outparm = N'@recCnt int OUTPUT';
set @sql = N'Select MemberID
,QNXTMEMid
,LastName
,FirstName
,DOB
,SSN
,MedicaidID
,MedicareID
,IsPDPOnly
,IsReferralAllowed
,IsPreCertAllowed
,IsDual
,IsSinglePlanDual
,IsPPO
,IsCoverageActive
,IsInpatientAllowed
,s.SourceDescription as ApplicationSource from MyMembers m join MyMembers_System s
on m.sourcesystemid = s.sourceid where '
if nullif(@memberid, '') is not null
begin
if @first = 1
begin
set @where = N' (MemberID = ''' + convert(nvarchar,@memberid) + '''';
set @where = @where + N' or QNXTMemid = ''' + convert(nvarchar,@memberid) + ''')';
set @first = 0
end
end
if nullif(@lastname, '') is not null
begin
if @first = 1
begin
set @where = N' lastname like ''' + convert(nvarchar,@lastname) + '%'''
set @first = 0
end
else
set@where = @where + N' and lastname like ''' + convert(nvarchar,@lastname) + '%'''
end
if nullif(@firstname,'') is not null
begin
if @first = 0
set @where = @where + N' and firstname like ''' + convert(nvarchar,@firstname) + '%'''
else
begin
set @where = N' firstname like ''' + convert(nvarchar,@firstname) + '%'''
set @first = 0
end
end
if isdate(@dob) = 1
begin
if @first = 0
set @where = @where + N' and DOB = ''' + convert(nvarchar,@dob) + ''''
else
begin
set @where = N' DOB = ''' + convert(nvarchar,@dob) + ''''
set @first = 0
end
end
if nullif(@ssn, '') is not null
begin
if @first = 0
set @where = @where + N' and SSN = ''' + convert(nvarchar,@ssn) + ''''
else
begin
set @where = N' SSN = ''' + convert(nvarchar,@ssn) + ''''
set @first = 0
end
end
if nullif(@MedicaidID, '') is not null
begin
if @first = 0
set @where = @where + N' and MedicaidID = ''' + convert(nvarchar,@MedicaidID) + ''''
else
begin
set @where = N' MedicaidID = ''' + convert(nvarchar,@MedicaidID) + ''''
set @first = 0
end
end
if nullif(@IsCoverageActive, '') is not null
begin
if @first = 0
set @where = @where + N' and IsCoverageActive = ''' + convert(nvarchar,@IsCoverageActive) + ''''
else
begin
set @where = N' IsCoverageActive = ''' + convert(nvarchar,@IsCoverageActive) + ''''
set @first = 0
end
end
if nullif(@IsPDPOnly, '') is not null
begin
if @first = 0
set @where = @where + N' and IsPDPOnly = ''' + convert(nvarchar,@IsPDPOnly) + ''''
else
begin
set @where = N' IsPDPOnly = ''' + convert(nvarchar,@IsPDPOnly) + ''''
set @first = 0
end
end
if nullif(@IsInpatientAllowed, '') is not null
begin
if @first = 0
set @where = @where + N' and IsInpatientAllowed = ''' + convert(nvarchar,@IsInpatientAllowed) + ''''
else
begin
set @where = N' IsInpatientAllowed = ''' + convert(nvarchar,@IsInpatientAllowed) + ''''
set @first = 0
end
end
set @sqlCnt = @sqlcnt + @where;
execute sp_executesql @sqlcnt, @outparm, @recCnt = @recCnt OUTPUT;
--select @recCnt;
set @returnCnt = @recCnt;
if @debug = 1
begin
print @sql
print @sqlcnt
print 'IsCoverageActive=' + cast(@IsCoverageActive as varchar(1))
print 'IsPDPOnly=' + cast(@IsPDPOnly as varchar(1))
print 'IsInpatientAllowed=' + cast(@IsInpatientAllowed as varchar(1))
print 'First = ' + cast(@first as char(10))
return
end
else
begin
if @recCnt <= @maxcnt
execute sp_executesql @sql
else
begin
--declare @inParm nvarchar(10);
--set @inParm = convert(nvarchar(10), @maxcnt);
--set @sql = N'Select top ' + @inParm + N' MemberID,QNXTMemID, LastName,FirstName,DOB,SSN,MedicaidID,MedicareID
--,IsPDPOnly,IsReferralAllowed,IsPreCertAllowed,IsDual,IsSinglePlanDual,IsPPO,IsCoverageActive,s.sourcedescription as ApplicationSource
--from membersearch m join MemberSearch_SourceSystem s on m.sourcesystemid = s.sourceid where ' + @where
--execute sp_executesql @sql
declare @inParm nvarchar(25);
set @inParm = 'Select top ' + convert(nvarchar(10), @maxcnt + ' ');
set @sql = replace(@sql,'Select ',@inParm)
execute sp_executesql @sql
end
end
return;
How could I change the sproc to defend against SQL Injections? If I use Parameterized queries and the user doesn't submit a value for one of the filters in the where clause I dont want that filter to be considered.
April 21, 2014 at 12:20 pm
See Books Online for details about sp_executesql. You need to be using the fully parameterized capabilities:
EXEC sp_executesql [ @statement = ] statement
[
{ , [ @params = ] N'@parameter_name data_type [ OUT | OUTPUT ][ ,...n ]' }
{ , [ @param1 = ] 'value1' [ ,...n ] }
]
The good thing is that you don't have to do conditionals to build the parameter string. You can declare them all and just simply not pass in values for the one's that aren't used in your built-up @statement.
To my knowledge, fully parameterized TSQL is the ONLY WAY to guarantee protection from SQL Injection (at least without discarding valid input from the user such as CAST, EXEC, DECLARE, etc).
Best,
Kevin G. Boles
SQL Server Consultant
SQL MVP 2007-2012
TheSQLGuru on googles mail service
April 21, 2014 at 12:28 pm
Quite some room for improvements, start by moving the parameters to the sp_executesql params and validating the inputs.
😎
April 21, 2014 at 5:20 pm
+1000 to that!
--Jeff Moden
Change is inevitable... Change for the better is not.
April 23, 2014 at 3:49 pm
This has always been a sticking point for me, it always seemed to lead to "dirty" solutions. This is an approach I came up with to accommodate a flexible user interface. There are about 8 different fields in the original that can be selected from beyond the date range, and the user can select 0 - many items from each field.
This passes all of the selection into the proc in a table type parameter as name value pairs. If you needed to select two different departments for instance you would pass in two different records with the same parameter name, but different parameter values.
There haven't been any performance issues, but the primary table is only about 60,000 rows at this point. I do like that it allows for flexibility with a minimum of fuss, and no dynamic SQL.
CREATE PROCEDURE [dbo].[usp_GetUserSelection]
@paramVal dbo.tdt_ParameterData READONLY
AS
BEGIN
SET NOCOUNT ON;
-- Create internal parameter table so that we can modify values.
DECLARE @paramValues TABLE
(
ParameterName varchar(50),
ParameterValue varchar(200) NULL
);
-- Copy the incoming parameters over.
INSERT INTO @paramValues
(
ParameterName,
ParameterValue
)
SELECT
LTRIM(RTRIM(ParameterName)),
LTRIM(RTRIM(ParameterValue))
FROM @paramVal;
-- Get start and end date, these are the only required params.
DECLARE @startDate datetime = (SELECT CAST(ParameterValue as datetime) FROM @paramValues WHERE ParameterName = 'StartDate');
DECLARE @endDate datetime = (SELECT CAST(ParameterValue as datetime) FROM @paramValues WHERE ParameterName = 'EndDate');
-- Add a null value placeholder for any unused parameters.
IF NOT EXISTS (SELECT ParameterValue FROM @paramValues WHERE ParameterName = 'DeptNumber')
INSERT INTO @paramValues (ParameterName, ParameterValue) VALUES('DeptNumber', NULL);
IF NOT EXISTS (SELECT ParameterValue FROM @paramValues WHERE ParameterName = 'LocationCode')
INSERT INTO @paramValues (ParameterName, ParameterValue) VALUES('LocationCode', NULL);
SELECT
t.ItemID,
ISNULL(lm.LocationCode, '') LocationCode,
ISNULL(lm.[Description], '') LocationDesc,
ISNULL(lm.DeptNumber, '') DeptNumber,
ISNULL(dd.DepartmentDescription, '') DepartmentDescription
FROM dbo.Item t
LEFT OUTER JOIN dbo.Location lm ON t.LocationID = lm.LocationID
LEFT OUTER JOIN dbo.Department dd ON lm.DeptNumber = dd.DepartmentNumber
JOIN @paramValues p0 ON p0.ParameterName = 'LocationCode'
AND lm.LocationID = ISNULL(p0.ParameterValue, lm.LocationID)
JOIN @paramValues p1 ON p1.ParameterName = 'DeptNumber'
AND lm.DeptNumber = ISNULL(p1.ParameterValue, lm.DeptNumber)
WHERE
(
t.EndDate >= @startDate
AND t.StartDate <= @endDate
)
AND
(
(
p0.ParameterValue IS NOT NULL
OR p1.ParameterValue IS NOT NULL
)
OR
(
@startDate IS NOT NULL
AND @endDate IS NOT NULL
)
);
END
April 23, 2014 at 7:34 pm
Sorry MarbryHardin, but your solution is totally awful from a query optimization standpoint and you are guaranteed to get suboptimal performance. Read Gail's blog posts. Also, scale your table up to 6M or 600M rows and look at the query plans you get when you run your code. Also, watch how the plan will get reused with parameters that really need a different plan.
Oh, and your table variable isn't going to do you any favors either. I can give you a one column, one row table variable example that will give you a bad plan where the same in a temp table gives you the correct plan.
Best,
Kevin G. Boles
SQL Server Consultant
SQL MVP 2007-2012
TheSQLGuru on googles mail service
April 24, 2014 at 6:49 am
The bulk of the execution plan cost comes out to a clustered index scan on the primary table. It's more than sufficient for our needs.
If I get the chance it would be nice to play with optimizing it against a large number of records.
April 24, 2014 at 8:14 am
MarbryHardin (4/24/2014)
The bulk of the execution plan cost comes out to a clustered index scan on the primary table. It's more than sufficient for our needs.If I get the chance it would be nice to play with optimizing it against a large number of records.
To be honest, those are famous last words. 🙂 No one ever goes back to plan for the inevitable scalability problems and, when such problems occur, it creates a drop-everything panic.
My recommendation would be that what is "sufficient" for your "needs" really isn't. Do it the right way now. It's not "pre-optimization"... it's just writing good code.
--Jeff Moden
Change is inevitable... Change for the better is not.
April 24, 2014 at 10:19 am
I know what you're saying, but I did a quick test loading 1.2 million rows to a test table which is about 10x what the production table should ever be based on usage and maintenance. It went from sub second to under 2 seconds, and that with unfavorably structured data. Not ideal, but there are some other optimizations that could be made.
And I will say that Gail's approach would be fine. If it fit. It does not allow for matching on an unknown number of values per field.
And you'll have to come saw my foot off to get me to write dynamic SQL. 😉
April 24, 2014 at 9:53 pm
MarbryHardin (4/24/2014)
... And you'll have to come saw my foot off to get me to write dynamic SQL. 😉
Then you are discounting one of the best tools for dealing with a number of data processing problems (of which the open-ended-search is a prime example). I have used it any number of times to achieve 4-5 ORDER OF MAGNITUDE performance gains while simultaneously bringing concurrency up from a molasses-in-February pace.
Best,
Kevin G. Boles
SQL Server Consultant
SQL MVP 2007-2012
TheSQLGuru on googles mail service
April 25, 2014 at 6:06 am
There are other considerations beyond performance alone that discount the use of dynamic SQL. Speaking of writing it "right" in the first place.
Never say never, but it is something to be avoided.
Viewing 12 posts - 1 through 11 (of 11 total)
You must be logged in to reply to this topic. Login to reply