1

I have two tables tbl_Products and tbl_Brands, both are joined on BrandId.

I have a stored procedure which should return all the products belong to the brand ids passed to it as parameter.

My code is as follows. 

create proc Sp_ReturnPrdoucts
    @BrandIds varchar(500) = '6,7,8'
AS
BEGIN
    SELECT * 
    FROM tbl_Products as p 
    JOIN tbl_Brands b ON p.ProductBrandId = b.BrandId 
    WHERE b.BrandId IN (@BrandIds)
END

But this is giving error as BrandId is INT and @BrandIds is VARCHAR

When I hard code it this way as follows it works fine and returns the desired data from db ..

create proc Sp_ReturnPrdoucts
    @BrandIds varchar(500) = '6,7,8'
AS
BEGIN
    SELECT * 
    FROM tbl_Products AS p 
    JOIN tbl_Brands b ON p.ProductBrandId = b.BrandId 
    WHERE b.BrandId IN (6,7,8)
END

Any help :)

marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
mzh
  • 515
  • 8
  • 21
  • 2
    Duplicate: http://stackoverflow.com/questions/15585632/how-to-convert-comma-separated-nvarchar-to-table-records-in-sql-server-2005 You just have to create function to convert your list into table and then just inner join it. – Evaldas Buinauskas Jun 13 '15 at 09:24
  • 1
    @EvaldasBuinauskas splitting strings in sql is usually not a solution, it's a workaround. the correct solution is to use the proper data types (in this case, a table) whenever possible. – Zohar Peled Jun 13 '15 at 09:32
  • @ZoharPeled You would still have to put these values somehow in there. – Evaldas Buinauskas Jun 13 '15 at 09:36
  • 1
    Side note: you should **not** use the `sp_` prefix for your stored procedures. Microsoft has [reserved that prefix for its own use (see *Naming Stored Procedures*)](http://msdn.microsoft.com/en-us/library/ms190669%28v=sql.105%29.aspx), and you do run the risk of a name clash sometime in the future. [It's also bad for your stored procedure performance](http://www.sqlperformance.com/2012/10/t-sql-queries/sp_prefix). It's best to just simply avoid `sp_` and use something else as a prefix - or no prefix at all! – marc_s Jun 13 '15 at 10:01
  • @EvaldasBuinauskas: you mean that using a table valued parameter can't have default values? – Zohar Peled Jun 13 '15 at 10:01
  • @ZoharPeled Sorry, no, not that. I had in mind that OP should change a lot of codebase then (unless it's a new project) to support table valued params – Evaldas Buinauskas Jun 13 '15 at 10:03
  • @EvaldasBuinauskas: This is the reason my answer begings with "if possible".... :-) – Zohar Peled Jun 13 '15 at 10:04
  • @ZoharPeled Yep, misunderstood you a bit. You're right, If I'd have to do something from scratch, I'd go for table valued params. – Evaldas Buinauskas Jun 13 '15 at 10:05

2 Answers2

4

If possible, don't use varchar for this kind of things, use a table valued parameter instead.

To use a tabled value parameter you should first declare a user defined table type:

CREATE TYPE IntList As Table
(
    IntValue int
)

Then change your stored procedure to accept this variable instead of the nvarchar:

create proc Sp_ReturnPrdoucts
    @BrandIds dbo.IntList readonly -- Note: readonly is a must!
AS
BEGIN

SELECT * 
FROM tbl_Products as p 
join tbl_Brands b on p.ProductBrandId=b.BrandId 
join @BrandIds ON(b.BrandId = IntValue)

END

The problem is that the IN() operator expects a list of variables separated by commas, while you provide a single variable that it's value is a comma separated string.

If you can't use a table valued parameter, you can use a string spliting function in sql to convert the value of the varchar to a table of ints. there are many splitters out there, I would recommend reading this article before picking one.

Zohar Peled
  • 79,642
  • 10
  • 69
  • 121
2

Another alternative is to use 'indirection' (as I've always called it)

You can then do..

create proc Sp_ReturnPrdoucts
@BrandIds varchar(500) = '6,7,8'
AS
BEGIN
    if (isnumeric(replace(@BrandIds,',',''))=1) 
    begin
        exec('SELECT * FROM tbl_Products as p join tbl_Brands b on p.ProductBrandId=b.BrandId WHERE b.BrandId IN ('+@BrandIds+')')
    end
END

This way the select statement is built as a string, then executed.

I've now added validation to ensure that the string being passed in is purely numeric (after removing all the commas)

Rich S
  • 3,248
  • 3
  • 28
  • 49
  • Downvote reason: Dynamic sql comes with a price, both in sql injection vulnerability and in performance. I would use it only as a last resort. – Zohar Peled Jun 13 '15 at 10:00
  • Downvote? It's a valid solution, and each answer has its own advantages and disadvantages (this one is the shortest and easiest to read). There are ways to protect against sql injection, the performance hit is tiny, and only noticeable if you're running this SP repeatedly. – Rich S Jun 13 '15 at 10:06
  • I would not have downvoted your answer if I thought that dynamic sql is a valid solution to this question. moreover, though it is possible to protect against sql injection, your answer does not even mention this problem, nor does your suggested solution is protedted from it in any way. – Zohar Peled Jun 13 '15 at 10:09
  • In the way you write it here, it is totally vulnerable for SQL injection, because you don't check the parameters. @ZoharPeled I agree, DynSQL can be open for SQL Injection. However, performance can massively be **improved** due to DynSQL, in some situations. – SQL Police Jun 13 '15 at 10:18
  • I've now edited to ensure that the string passed in doesn't contain anything other than commas and numbers. – Rich S Jun 13 '15 at 10:35
  • @abc if an answer solved your problem, you should accept it so that other people know that the problem is solved. – Zohar Peled Jun 14 '15 at 10:22
  • @ZoharPeled i marked it as answered. it worked for me and i thanked already for it.... – mzh Jun 15 '15 at 08:22
  • @ZoharPeled Thankyou for your time,, i learned many things about SQL by the discussion you guys had here for this question.... Thankyou every one of you :) – mzh Jun 15 '15 at 08:23
  • @abc: even though I think you went for a bad solution, the fact that you have learned some new things about SQL is good enough for me. cheers. I don't see the green 'V' sign on any answer... – Zohar Peled Jun 15 '15 at 08:28