0

I have developed a SQL query in SSMS-2017 like this:

DECLARE @property NVARCHAR(MAX) = @p;
SET @property = REPLACE(@property, '''', '');

DECLARE @propList TABLE (hproperty NUMERIC(18, 0));

IF CHARINDEX('SELECT', @property) > 0 OR CHARINDEX('select', @property) > 0
BEGIN
    INSERT INTO @propList
        EXECUTE sp_executesql @property;
END;
ELSE
BEGIN
    DECLARE @x TABLE (val NUMERIC(18, 0));

    INSERT INTO @x
        SELECT CONVERT(NUMERIC(18, 0), strval)
        FROM dbo.StringSplit(@property, ',');

    INSERT INTO @propList
        SELECT val
        FROM @x;
END;

SELECT ...columns...
FROM ...tables and joins...
WHERE ...filters...
  AND HMY IN (SELECT hproperty FROM @propList)

The issue is, it is possible that the value of the parameter @p can be a list of IDs (Example: 1,2,3,4) or a direct select query (Example: Select ID from mytable where code='A123').

The code is working well as shown above. However it causes a problem in our system (as we use Yardi7-Voyager), and we need to leave only the select statement as a query. To manage it, I was planning to create a function and use it in the where clause like:

WHERE HMY IN (SELECT myFunction(@p))

However I could not manage it as I see I cannot execute a dynamic query in an SQL Function. Then I am stacked. Any idea at this point to handle this issue will be so appreciated.

marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
Eray Balkanli
  • 7,752
  • 11
  • 48
  • 82
  • [Parameterize an SQL IN clause](https://stackoverflow.com/questions/337704/parameterize-an-sql-in-clause) – Lukasz Szozda Jun 19 '19 at 15:33
  • 1
    Yikes that sounds like a design that leaves a bit to be desired. Delimited list should be avoided if at all possible. In your case a table valued parameter would be a LOT better. And passing in queries to execute is really not good. You are stuck using dynamic sql and likely being vulnerable to sql injection. – Sean Lange Jun 19 '19 at 15:34
  • @LukaszSzozda already using splitstring when the value of the @p includes only IDs. Problem, it is the full query sometimes. – Eray Balkanli Jun 19 '19 at 15:35
  • 2
    Is suspect that this has an injection issue as well; not that we can see where the value of `@p` comes from. Considering, however, you say that `@p` can have a value of `'A123'` or `'1,2,3,4'` and then you check it it contains the work `SELECT` and you execute it if it does just doesn't add up. I think we're missing too much information – Thom A Jun 19 '19 at 15:35
  • 1
    *"Problem, it is the full query sometimes."* Never, ever, pass in a variable with a query to execute; especially if it's unsanitised. That is a massive security hole you need to fill in. – Thom A Jun 19 '19 at 15:36
  • Also, why use `dbo.StringSplit` (a custom function we don't have the DDL for, and I hope isn't using a `WHILE`) when I *assume* you have access to `STRING_SPLIT` (you say SSMS 2017, but I assume you mean SQL Server 2017 , as that's what you tagged). – Thom A Jun 19 '19 at 15:38
  • For @p, the user selects an item from the front-end, if the item has sub-items then the system sends a sql-query instead of the IDs of all the items. I can use STRING_SPLIT as well, yes, but sometimes we are using our queries in older versions and that's why I used SplitString here, to make sure this query would work after a copy/paste @Larnu – Eray Balkanli Jun 19 '19 at 15:41
  • 2
    Seems like you need to change the way the application works first. :/ – Thom A Jun 19 '19 at 15:43
  • 1
    I agree 1000% with @Larnu here. The best way to fix this is to change the design. – Sean Lange Jun 19 '19 at 16:09
  • Not familiar with Yardi/Voyager. Does it support Executing stored procedures, or only single SELECT queries? – Tab Alleman Jun 19 '19 at 17:53
  • @TabAlleman You can execute stored procedures as well, but using anything else than a select query causes the Export to Excel and PDF buttons disappear, which is main problem for this question as getting the p and prepare the propList for my main query caused the buttons disappear, that's why I gotta remove them and leave only the select query. – Eray Balkanli Jun 19 '19 at 18:02

1 Answers1

1

Others have pointed out that the best fix for this would be a design change, and I agree with them. However, I'd also like to treat your question as academic and answer it in case any future readers ever have the same question in a use case where a design change wouldn't be possible/desirable.

I can think of two ways you might be able to do what you're attempting in a single select, as long as there are no other restrictions on what you can do that you haven't mentioned yet. To keep this brief, I'm just going to give you psuedo-code that can be adapted to your situation as well as those of future readers:

  1. OPENQUERY (or OPENROWSET)

You can incorporate your code above into a stored procedure instead of a function, since stored procedures DO allow dynamic sql, unlike functions. Then the SELECT query in your app would be a SELECT from OPENQUERY(Execute Your Stored Prodedure).

  1. UNION ALL possibilities.

I'm about 99% sure no one would ever want to use this, but I'm mentioning it to be as academically complete as I know how to be.

The second possibility would only work if there are a limited, known, number of possible queries that might be supported by your application. For instance, you can only get your Properties from either TableA, filtered by column1, or from TableB, filtered by Column2 and/or Column3.

Could be more than these possibilities, but it has to be a limited, known quantity, and the more possibilities, the more complex and lengthy the code will get.

But if that's the case, you can simply SELECT from a UNION ALL of every possible scenario, and make it so that only one of the SELECTs in the UNION ALL will return results.

For example:

SELECT ... FROM TableA WHERE Column1=fnGetValue(@p, 'Column1')
AND CHARINDEX('SELECT', @property) > 0
AND CHARINDEX('TableA', @property) > 0
AND CHARINDEX('Column1', @property) > 0
AND (Whatever other filters are needed to uniquely identify this case)
UNION ALL 
SELECT
...

Note that fnGetValue() isn't a built-in function. You'd have to write it. It would parse the string in @p, find the location of 'Column1=', and return whatever value comes after it.

At the end of your UNION ALL, you'd need to add a last UNION ALL to a query that will handle the case where the user passed a comma-separated string instead of a query, but that's easy, because all the steps in your code where you populated table variables are unnecessary. You can simply do the final query like this:

WHERE NOT CHARINDEX('SELECT', @p) > 0
AND HMY IN (SELECT strval FROM dbo.StringSplit(@p, ','))

I'm pretty sure this possibility is way more work than its worth, but it is an example of how, in general, dynamic SQL can be replaced with regular SQL that simply covers every possible option you wanted the dynamic sql to be able to handle.

Tab Alleman
  • 31,483
  • 7
  • 36
  • 52