1

I have been working on a few stored procedures that have conditional parameters to them, but one of them is giving me a problem that I can't quite figure out. This is the code for the procedure:

CREATE PROCEDURE dbo.GetTableData(
    @TblName   VARCHAR(50),
    @Condition VARCHAR(MAX) = NULL,
) AS
BEGIN
    IF(EXISTS(SELECT * FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_NAME = @TblName))
        BEGIN
            DECLARE @SQL NVARCHAR(MAX) = N'
            SELECT * FROM @TblName WHERE 1=1'
            + CASE WHERE @Condition IS NOT NULL THEN
            ' AND ' + @Condition ELSE N'' END

            DECLARE @params NVARCHAR(MAX) = N'
                @TblName   VARCHAR(50),
                @Condition VARCHAR(MAX)';

            PRINT @SQL

            EXEC sys.sp_executesql @SQL, @params,
                @TblName,
                @Condition
        END
    ELSE
        RETURN 1
END

The way I would like the procedure to work, is that it's suppose to allow me to do quick table look ups. So if I want to see everything from my Parts table, I'd just run

EXEC GetTableData 'parts'

Or if I wanted to see everything in the Parts table with a specific supplier I'd run

EXEC GetTableData 'parts', 'supplier LIKE ''A2A Systems'''

Now in the above example, when I run it, the PRINT @SQL line prints out the query as:

SELECT * FROM @TblName WHERE 1=1 AND supplier LIKE 'A2A Systems'

So the query it being put together properly (it seems).

However after it prints I am getting the following error:

Msg 1087, Level 16, State 1, Line 4

Must declare the table variable "@TblName"

I still get this error if I change the EXEC line to:

EXEC GetTableData @TblName='parts', @Condition='supplier LIKE ''A2A Systems'''

So what am I doing wrong here? Why isn't it taking my @TblName variable value?

2

You need to modify your procedure this way:

CREATE PROCEDURE dbo.GetTableData(
@TblName   VARCHAR(50),
@Condition VARCHAR(MAX) = NULL
) AS
BEGIN
    IF(EXISTS(SELECT * FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_NAME = @TblName))
        BEGIN
            DECLARE @SQL NVARCHAR(MAX) = N'
            SELECT * FROM ' + @TblName + 'WHERE 1=1'
        + CASE WHEN @Condition IS NOT NULL THEN
        ' AND ' + @Condition ELSE N'' END

        DECLARE @params NVARCHAR(MAX) = N'
            @TblName   VARCHAR(50),
            @Condition VARCHAR(MAX)';

        PRINT @SQL

        EXEC sys.sp_executesql @SQL, @params,
            @TblName,
            @Condition
    END
ELSE
    RETURN 1
END

Your variable @TblName must not be inside the @SQL string

  • 4
    Important note: @TblName could contain something dangerous, even with only 50 characters. Example: sys.objects; DROP TABLE foo; SELECT * FROM parts . – Aaron Bertrand Jul 20 '18 at 18:58
7

You can't parameterize entity names (tables, columns, views, etc). You need to do it the riskier way:

        DECLARE @SQL NVARCHAR(MAX) = N'
        SELECT * FROM ' + QUOTENAME(@TblName) + N' WHERE 1=1'
        + CASE WHERE @Condition IS NOT NULL THEN
        ' AND ' + @Condition ELSE N'' END

        DECLARE @params NVARCHAR(MAX) = N'
            @Condition VARCHAR(MAX)';

        PRINT @SQL

        EXEC sys.sp_executesql @SQL, @params,
            @Condition

QUOTENAME() is typically enough to protect from dangerous execution (leading, potentially, to SQL injection), but to make it a little more secure you should consider (a) prefixing the table name with the correct schema prefix (e.g. ...FROM dbo.' + QUOTENAME(@TblName) + ... and (b) checking for existence first:

IF NOT EXISTS (SELECT 1 FROM sys.tables WHERE name = @TblName)
BEGIN
  RAISERROR(N'Nice try, robot.', 11, 1);
  RETURN;
END

DECLARE @SQL ...
  • RE: checking for existence first - while I'm all for highlighting the risks of injection, @TblName isn't the one I'd be worried about here -- the OP already checks its validity using INFORMATION_SCHEMA.TABLES. @Condition, on the other hand...well, that's a nightmare waiting to happen -- you're not going to validate string of foo = 1 AND bar = 2; DROP TABLE oops, and you're not going to pass @Condition off to sp_executesql in that way... – jimbobmcgee Jul 20 '18 at 23:04
  • Funny I was just having this conversation recently. stackoverflow.com/q/51423462/61305 I agree the condition thing is bad but there’s nothing that can be done to improve that except not doing it. The check against information_schema is not enough on its own and that’s the info I aim to augment. – Aaron Bertrand Jul 21 '18 at 3:18
  • In any case I did have an idea on that, recently, myself, using sys.dm_exec_describe_first_result_set; but I haven't been through all the use cases I can think of to see if it holds water. Need to put it to the SQL community to see what they reckon... – jimbobmcgee Jul 21 '18 at 3:51
3

I'm going to add to this, to cover off my comments to the question asker and to subsequent answerers.

As I say in the comment above:

This is one of those anti-patterns that coders who are familiar with 'traditional' programming languages making the jump to SQL have to have beaten out of them by experience.

SQL is not like traditional languages and you need to stop thinking of it like it is.

What you're trying to do has been tried by many (including me) and we've all suffered from the results.

Hint: Checking @TblName against INFORMATION_SCHEMA is good - it makes it safer to append - but @Condition is never going to parametise into sp_executesql, leaving a glaring eval-shaped hole in your app security.

A comment is too small a space here, to explain fully why I take the above stance.

In the case of your procedure design, you take two variables, @TblName and @Conditions, and attempt to fold them into dynamic SQL. As you have discovered, only certain parts of the SQL syntax accept variables. This can roughly be divided into places in the syntax where values are expected (these can usually be supplied with variables) and places where syntactic structure is expected (these cannot be replaced with variables).

I'll concede that sometimes it would be nice if certain syntactic structures understood variables but, in the case of the a SELECT * FROM ... statement, the ... is syntactic, not a value. As such, it has to be composed directly within the SQL statement and not supplied by variable.

You can make a generic, unconditional SELECT * FROM ... procedure using dynamic SQL, but the rules for generating dynamic SQL are that you must carefully validate, cleanse and appropriately escape any user-entered parameters before they are appended to a composed SQL string, because it is too easy for a malicious end-user to supply strings that would end your composed SQL command and start another command that the malicious end-user controls. The sp_executesql procedure would not be able to tell the difference between your command and their command, and would run both in a single batch.

The target table name is one such user-entered parameter, but this is easy to validate, cleanse and escape. You are already doing the validation part of this with your check against INFORMATION_SCHEMA.TABLES. The cleanse/escape parts have been nicely covered in the other answers (e.g. QUOTENAME).

However, in addition to the table name, your procedure design also allows you to supply conditions that limit the results from the target table. It is clear from your SQL composition that you will pass SQL-like clauses, such as foo = 1 AND bar = 'hello world', and that you use them in a WHERE clause in your composed SQL string.

Unfortunately, the column name and operator and conjunction of your clauses (whether they are AND or OR) are also syntactic structure elements which cannot be passed by variable. This means that you also have to append them to your composed SQL string, rather using a variable name.

However, validating, cleansing and escaping is a much, much harder job for user-entered WHERE clauses, so you open yourself up to a much higher risk of injection attack than for a simple table name parameter.

It seems to me that the intention of the procedure in the question is to avoid writing SQL. If you want to go ahead and do that -- be it because you either lack confidence in SQL, or because you believe it will makes your application layer easier to write, you should consider implementing whatever well-supported ORM framework exists for the language that you do want to write in.

If it is not your intention to avoid writing SQL, then you should actually write SQL and not try to circumvent it with an anti-pattern.

With that rant over, I'm going to demonstrate how difficult it actually is to write a properly-validated generic SELECT procedure with arbitrary clauses. Note that I am categorically not recommending that you use this code -- I've only written it because it is Friday. I guarantee that I haven't covered every possible edge case. If you did use it, then one day it would probably try to kill you in your sleep.

Anyway, here goes:

IF OBJECT_ID('dbo.get_any', 'P') IS NOT NULL DROP PROCEDURE dbo.get_any;
GO
IF TYPE_ID('dbo.GenericCondition') IS NOT NULL DROP TYPE dbo.GenericCondition;
GO

CREATE TYPE dbo.GenericCondition AS TABLE (
     ordinal        INTEGER         IDENTITY(1, 1)
    ,conjunction    VARCHAR(3)      NULL
    ,colname        SYSNAME         NOT NULL
    ,operator       VARCHAR(2)      NOT NULL
    ,value          SQL_VARIANT     NULL
)
GO

CREATE PROCEDURE dbo.get_any (
     @tablename     NVARCHAR(515)
    ,@conditions    dbo.GenericCondition READONLY
)
WITH EXECUTE AS CALLER
AS
    DECLARE @server SYSNAME
           ,@dbname SYSNAME
           ,@schema SYSNAME
           ,@object SYSNAME;

    -- extract component names from the passed table indicator
    SELECT @server = PARSENAME(@tablename, 4)
          ,@dbname = COALESCE(PARSENAME(@tablename, 3), DB_NAME())
          ,@schema = COALESCE(PARSENAME(@tablename, 2), N'dbo')
          ,@object = PARSENAME(@tablename, 1);

    -- check that the server and database exists
    IF (@server IS NULL OR EXISTS (SELECT 1 FROM sys.servers WHERE name = @server))
       AND EXISTS (SELECT 1 FROM sys.databases WHERE name = @dbname)
    BEGIN
        DECLARE @sql NVARCHAR(MAX);
        DECLARE @params NVARCHAR(MAX);
        DECLARE @target NVARCHAR(2000);
        DECLARE @cols TABLE (cname SYSNAME, tname SYSNAME, tsize NVARCHAR(32));

        -- escape the server and database name for use in dynamic queries
        SET @target = CASE WHEN @server IS NOT NULL
                           THEN N'[' + REPLACE(@server, N']', N']]') + N'].'
                           ELSE N''
                           END
                    + N'[' + REPLACE(@dbname, N']', N']]') + N']'

        -- get column information from the target database's system tables
        SET @sql = N'
            SELECT
                 c.name
                ,t.name
                ,CASE WHEN t.name IN (''char'', ''nchar'', ''binary'', ''varchar'', ''nvarchar'', ''varbinary'')
                      THEN N''('' + COALESCE(CONVERT(NVARCHAR(32), NULLIF(c.max_length, -1)), N''max'') + N'')''
                      WHEN c.max_length = t.max_length
                       AND c.precision = t.precision
                       AND c.scale = t.scale
                      THEN N''''
                      ELSE N''('' + CONVERT(NVARCHAR(32), c.precision) + N'','' + CONVERT(NVARCHAR(32), c.scale) + N'')''
                      END
            FROM ' + @target + N'.sys.objects o
            INNER JOIN ' + @target + N'.sys.schemas s ON s.schema_id = o.schema_id
            INNER JOIN ' + @target + N'.sys.columns c ON c.object_id = o.object_id
            INNER JOIN ' + @target + N'.sys.types t ON c.user_type_id = t.user_type_id
            WHERE s.name = @schema
              AND o.name = @object
              AND o.type_desc IN (''SYSTEM_TABLE'', ''USER_TABLE'', ''VIEW'');
        ';
        SET @params = N'@schema SYSNAME, @object SYSNAME';

        /* debug */-- PRINT ('/* getting types */' + @sql);
        INSERT INTO @cols(cname, tname, tsize)
        EXEC sp_executesql @command = @sql
                          ,@params  = @params
                          ,@schema  = @schema
                          ,@object  = @object;

        /* debug */-- SELECT * FROM @cols;

        -- if we have no columns, then the schema or table does not exist
        IF EXISTS(SELECT 1 FROM @cols) BEGIN
            SET @target = @target
                        + N'.[' + REPLACE(@schema, N']', N']]') + N'].['
                        + REPLACE(@object, N']', N']]') + N']';

            /* debug */-- RAISERROR('/* target = %s /*', 10, 1, @target) WITH NOWAIT;

            -- now we check the columns supplied in any conditions, to make sure they exist
            DECLARE @badlist NVARCHAR(MAX);

            SELECT @badlist = STUFF((SELECT N', "' + colname + N'"'
                                     FROM @conditions
                                     WHERE colname NOT IN (SELECT cname FROM @cols)
                                     FOR XML PATH(N''), TYPE).value(N'.', 'NVARCHAR(MAX)'),
                                     1, 2, N'');

            /* debug */-- RAISERROR('/* badcols = %s /*', 10, 1, @badlist) WITH NOWAIT;
            IF @badlist IS NOT NULL
                RAISERROR('Cannot find column(s) %s in object %s.%s in database "%s" on server "%s"',
                          16, 1, @badlist, @schema, @object, @dbname, @server)
                          WITH NOWAIT;
            ELSE BEGIN
                -- we check the operators in the conditionals now, for valid syntax we support
                SELECT @badlist = STUFF((SELECT N', "' + operator + N'"'
                                         FROM @conditions
                                         WHERE operator NOT IN ('=', '<', '<=', '>', '>=', '<>')
                                         FOR XML PATH(N''), TYPE).value(N'.', 'NVARCHAR(MAX)'),
                                         1, 2, N'');

                /* debug */-- RAISERROR('/* badops = %s /*', 10, 1, @badlist) WITH NOWAIT;
                IF @badlist IS NOT NULL
                    RAISERROR('Invalid operator(s) %s in conditions', 16, 1, @badlist)
                              WITH NOWAIT;
                ELSE BEGIN
                    -- we check the conjunctions, for valid syntax we support
                    SELECT @badlist = STUFF((SELECT N', "' + conjunction + N'"'
                                             FROM @conditions
                                             WHERE (ordinal = 1 AND conjunction IS NOT NULL)
                                                OR (ordinal > 1 AND conjunction NOT IN ('AND', 'OR'))
                                             FOR XML PATH(N''), TYPE).value(N'.', 'NVARCHAR(MAX)'),
                                             1, 2, N'');

                    /* debug */-- RAISERROR('/* badconjs = %s /*', 10, 1, @badlist) WITH NOWAIT;
                    IF @badlist IS NOT NULL
                        RAISERROR('Invalid conjunction(s) %s in conditions', 16, 1, @badlist)
                                  WITH NOWAIT;
                    ELSE BEGIN
                        -- we have done the validations, and can now build our SQL, where
                        -- we use our properly-escaped target and fold in the conditions,
                        -- which we also escape heavily, using a horrid binary/Base64
                        -- conversion below, to cover arbitrary comaprison of as many of
                        -- the standard types as possible...
                        WITH b64 AS (
                            SELECT *,
                                b64 = (SELECT CONVERT(VARBINARY(MAX), value)
                                       FOR XML PATH(''), TYPE, BINARY BASE64)
                                       .value('.', 'VARCHAR(MAX)')
                            FROM @conditions
                        )
                        SELECT @sql = N'SELECT * FROM ' + @target
                                    + COALESCE(
                                      (SELECT NCHAR(13) + NCHAR(10)
                                            + CASE ordinal WHEN 1 THEN N'WHERE' ELSE conjunction END
                                            + N' '
                                            + CASE WHEN x.value IS NULL AND x.operator = '='
                                                   THEN N'[' + REPLACE(colname, N']', N']]') + N'] IS NULL'
                                                   WHEN x.value IS NULL AND x.operator = '<>'
                                                   THEN N'[' + REPLACE(colname, N']', N']]') + N'] IS NOT NULL'
                                                   ELSE N'[' + REPLACE(colname, N']', N']]') + N'] '
                                                      + operator
                                                      + N' CONVERT([' + REPLACE(c.tname, N']', N']]') + N']' + c.tsize
                                                      + N', CONVERT(XML, ''' + b64 + ''').value(''xs:base64Binary(.)'', ''VARBINARY(MAX)''))'
                                                   END
                                        FROM b64 x
                                        INNER JOIN @cols c ON x.colname = c.cname
                                        ORDER BY ordinal
                                        FOR XML PATH(N''), TYPE, BINARY BASE64).value(N'.', N'NVARCHAR(MAX)'),
                                        N'');

                        /* debug */-- PRINT ('/* actual sql */ ' + @sql);

                        EXEC sp_executesql @command = @sql;

                        /* debug */-- RAISERROR('done...', 10, 1) WITH NOWAIT;
                    END
                END
            END
        END
        ELSE RAISERROR(N'Cannot find object "%s.%s" in database "%s" on server "%s"',
                       16, 1, @schema, @object, @dbname, @server)
                       WITH NOWAIT;
    END
    ELSE RAISERROR(N'Cannot find one of server "%s" or database "%s"',
                   16, 1, @server, @dbname)
                   WITH NOWAIT;
GO

If you didn't want any conditions, you would call it quite simply:

EXEC dbo.get_any @tablename = 'dbo.my_target_table'

...and it would generate and run the simple statement:

SELECT * FROM [dbo].[my_target_table]

If you did want conditions, you would call it using the following somewhat horrid convention:

DECLARE @c AS GenericCondition;
INSERT INTO @c (conjunction, colname, operator, value)
VALUES (NULL,  'foo', '=', CONVERT(SQL_VARIANT, 1))
      ,('AND', 'bar', '>', 4)
      ,('AND', 'baz', '<', CONVERT(DATETIME, '2008-03-19T00:00:00'))
      ,('OR',  'qux', '<>', 'arrrrghhh!');

EXEC dbo.get_any @tablename = 'dbo.my_target_table'
                ,@conditions = @c;

...and it would dynamically generate and run:

SELECT * FROM [dbo].[my_target_table]
WHERE [foo] = CONVERT([int], CONVERT(XML, 'AAAAAQ==').value('xs:base64Binary(.)', 'VARBINARY(MAX)'))
AND [bar] > CONVERT([int], CONVERT(XML, 'AAAABA==').value('xs:base64Binary(.)', 'VARBINARY(MAX)'))
AND [baz] < CONVERT([datetime], CONVERT(XML, 'AACaZAAAAAA=').value('xs:base64Binary(.)', 'VARBINARY(MAX)'))
OR [qux] <> CONVERT([varchar](100), CONVERT(XML, 'YXJycnJnaGhoIQ==').value('xs:base64Binary(.)', 'VARBINARY(MAX)'))

That CONVERT-laden base64/binary stuff is massive overkill, designed to allow for supplying as many different kinds of data types as possible. You could probably get away with using implicit casts of string types for 70% of your use cases. I expect even the horrid base64/binary stuff would fail hard for at least 10% of use cases.

But, in any case, frankly, I know that I would rather write and run:

SELECT * 
FROM dbo.my_target_table
WHERE foo = 1
  AND bar > 4
  AND baz < '2008-03-19T00:00:00'
  AND qux <> 'arrrrghhh!'

...than the abomination that I put together above. I would have more control over complex WHERE clauses too.

Now go and bleach your eyes and never consider the above approach ever again!

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.