0

I have a SQL Server Table with some basic Sales information as follows

SaleID  |   TotalValue  |   PaymentMethod   |   User    |     Date
--------|---------------|-------------------|-----------|--------------
   1    |     10        |   CASH            |   USER 1  |   2020-01-01
   2    |     120       |   DEBIT CARD      |   USER 2  |   2020-01-01
   3    |     1000      |   CREDIT CARD     |   USER 2  |   2020-01-01
   4    |     305       |   CREDIT CARD     |   USER 1  |   2020-01-01
   5    |     15        |   CASH            |   USER 5  |   2020-01-01

I need to write a Dynamic Procedure os something of the sort that's able to search for the sales information using an array of SaleIDs as a filter. It's a simple select query, as follows:

SELECT
     [SaleID]
    ,[TotalValue] as 'Total Value'
    ,[Date] as 'Date'
FROM
    [Sales]
WHERE
    [SaleID] in (1, 2, 3, 4)

I created the following Stored Procedure so I can input an array of SalesID:

CREATE PROCEDURE [dbo].[usp_SearchSales]
(
      @SaleID NVARCHAR(max)
)
AS
BEGIN

    SET NOCOUNT ON;  

    DECLARE @SQL NVARCHAR(MAX)
    DECLARE @ParameterDef NVARCHAR(500)

    SET @ParameterDef = '@SaleID NVARCHAR(max)'

SET @SQL = 'SELECT
                 [SaleID]
                ,[TotalValue]
                ,[PaymentMethod]
                ,[User]
                ,[Date]
            FROM
                [Sales]
            WHERE 
                -1=-1'

IF @SaleID IS NOT NULL
SET @SQL = @SQL+ ' AND SaleID in (@SaleID)'

EXEC sp_Executesql @SQL, @ParameterDef, @SaleID = @SaleID

END

GO

Then I execute the Procedure through the Exec command

EXEC [usp_SearchSales] @SaleID = '1, 2, 3, 4'
GO

Then the procedure returns the following error uppon execution:

Msg 245, Level 16, State 1, Procedure usp_SearchSales, Line 26 [Batch Start Line 49]

Conversion failed when converting the nvarchar value '1, 2, 3, 4' to data type int.

Here is a SQL Query to create and insert some values on the designed table

CREATE TABLE [Sales] (
    [SaleID] int,
    [TotalValue] float,
    [PaymentMethod] varchar(50),
    [User] varchar(50),
    [Date] date
)

insert into sales values (1, 10, 'CASH', 'USER 1', '2020-01-01')
insert into sales values (2, 120, 'DEBIT CARD', 'USER 2', '2020-01-01')
insert into sales values (3, 10000, 'CREDIT CARD', 'USER 2', '2020-01-01')
insert into sales values (4, 305, 'CREDIT CARD', 'USER 1', '2020-01-01')
insert into sales values (5, 15, 'CASH', 'USER 5', '2020-01-01')

I need some help to create a procedure, or something of the sort, where an array of SalesIDs gives me back the Sales informations of those Sales.

Community
  • 1
  • 1
  • 1
    This line `SET @SQL = @SQL+ ' AND SaleID in (@SaleID)'` should read `SET @SQL = @SQL+ ' AND SaleID in (' + @SaleID + ')'` i.e. you need to add your incoming ids to the dynamic SQL, not reference the variable. – Dale K Feb 19 '20 at 20:56
  • It is a rather complex approach, but it works, thanks @EricBrandt!! – Igor Cattusso Feb 19 '20 at 21:11
  • Thanks @DaleK, that actually solved the question. I did try this aproach, but I must have made a mistake when writing the query, cause it was not working Yours is working just fine, thanks!!! – Igor Cattusso Feb 19 '20 at 21:11
  • 1
    To be clear though, its not the best way to do it, and it leaves you open to SQL injection. See answers below. – Dale K Feb 19 '20 at 21:12

2 Answers2

2

Unfortunately SQL Server does not natively supports arrays, which makes that you are trying to do pretty hard (or cumbersome) to achieve.

While it should be possible to use string functions to search for values in the CSV list, let me suggest a cleaner option, that works by creating table type.

It starts by creating the type:

create type dbo.id_array as table (id int);

Then your procedure can be simplified as a simple query:

create procedure dbo.usp_searchsales
    @sale_ids as dbo.id_array readonly
as
begin
    set nocount on;  
    select
        s.saleid,
        s.totalvalue,
        s.paymentmethod,
        s.[user],
        s.date
    from sales s
    inner join @sale_ids a on a.id = s.saleid;
end

Finally, when you want to invoke the procedure, you create a variable of the given type, fill it, and pass it as argument:

declare @my_sale_ids dbo.id_array;
insert into @my_sale_ids values (1), (2), (3), (4);
execute dbo.usp_searchsales @my_sale_ids;

Demo on DB Fiddle:

saleid | totalvalue | paymentmethod | user   | date      
-----: | ---------: | :------------ | :----- | :---------
     1 |         10 | CASH          | USER 1 | 2020-01-01
     2 |        120 | DEBIT CARD    | USER 2 | 2020-01-01
     3 |      10000 | CREDIT CARD   | USER 2 | 2020-01-01
     4 |        305 | CREDIT CARD   | USER 1 | 2020-01-01
GMB
  • 216,147
  • 25
  • 84
  • 135
-3

change the SP like the following : I changed SET @SQL = @SQL+ CONCAT(' AND SaleID in (',@SaleID,')') part of your query. Regarding the injection, Dynamic queries are vulnerable to inject, I only fixed the issue you had.

alter PROCEDURE [dbo].[usp_SearchSales]
(
      @SaleID NVARCHAR(max)
)
AS
BEGIN

    SET NOCOUNT ON;  

    DECLARE @SQL NVARCHAR(MAX)
    DECLARE @ParameterDef NVARCHAR(500)

    SET @ParameterDef = '@SaleID NVARCHAR(max)'

SET @SQL = 'SELECT
                 [SaleID]
                ,[TotalValue]
                ,[PaymentMethod]
                ,[User]
                ,[Date]
            FROM
                [Sales]
            WHERE 
                -1=-1'

IF @SaleID IS NOT NULL
SET @SQL = @SQL+ CONCAT(' AND SaleID in (',@SaleID,')')
print @SQL

EXEC sp_Executesql @SQL, @ParameterDef, @SaleID = @SaleID

END

GO

Another thing you can do is creating a User Defined Table and call it in SP, like the following :

CREATE TYPE [dbo].[udt_Sales] AS TABLE(
    [SaleID] [int] NULL
)

and the SP would be like this :

CREATE PROCEDURE [dbo].[sp_Name]
(
    @Ids [dbo].[udt_Sales] READONLY  
)
AS
BEGIN
SELECT [SaleID]
      ,[TotalValue]
      ,[PaymentMethod]
      ,[User]
      ,[Date]
FROM [Sales]
WHERE [SaleID] IN (SELECT * FROM @Ids)
END

and the you can call the SP like the following :

DECLARE @IDs udt_Sales

INSERT INTO @IDs VALUES (1)
INSERT INTO @IDs VALUES (2)
INSERT INTO @IDs VALUES (3)
INSERT INTO @IDs VALUES (4)

EXECUTE usp_SearchSales @IDs

This is much faster than Dynamic query. hope it helps

Amin Pashna
  • 141
  • 1
  • 1
  • 6
  • 1
    Please add some text explaining what you have changed and why - otherwise the OP has to do a file compare and still doesn't know what they did wrong. – Dale K Feb 19 '20 at 21:00
  • No explanation. But you did add an sql injection risk and negated the use of the dynamic sql parameter. How does that solve the problem? – SMor Feb 19 '20 at 21:02
  • Hi! This actually would solve it, but I forgot to mention that I have the SQL Server 2008 R2, and the CONCAT function does not exist on this version :( – Igor Cattusso Feb 19 '20 at 21:08
  • 2
    It might "solve" it but it also renders this code vulnerable to sql injection which is borderline criminal activity. And of course using CONCAT here is kind of pointless since it is just two strings. – Sean Lange Feb 19 '20 at 21:13
  • Yeah it really does, thanks for the tip, @SeanLange – Igor Cattusso Feb 19 '20 at 21:19