Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(DO NOT MERGE)(MODULES-2312) Use sp_executesql to execute T-SQL #127

Conversation

Iristyle
Copy link
Contributor

@Iristyle Iristyle commented Aug 1, 2015

  • Previously, the SQL server module executed arbitrary TSQL by
    injecting user supplied data into a template verbatim. Unfortunately
    this mechanism does nothing to address syntax errors, which will
    prevent the try / catch mechanism employed from working and
    propagating errors.

    The stored procedure sp_executesql can be used to execute arbitrary
    SQL strings, and will properly propagate an exception that can be
    caught, so use that method.

    Of note, since the user query is being placed into a string, it must
    have single quotes escaped properly. An additional test has been
    added to verify that single quotes are escaped as expected.

 - Previously, the SQL server module executed arbitrary TSQL by
   injecting user supplied data into a template verbatim.  Unfortunately
   this mechanism does nothing to address syntax errors, which will
   prevent the try / catch mechanism employed from working and
   propagating errors.

   The stored procedure sp_executesql can be used to execute arbitrary
   SQL strings, and will properly propagate an exception that can be
   caught, so use that method.

   Of note, since the user query is being placed into a string, it must
   have single quotes escaped properly. An additional test has been
   added to verify that single quotes are escaped as expected.
@Iristyle
Copy link
Contributor Author

Iristyle commented Aug 1, 2015

FYI @phongdly / @zreichert / @cyberious / @ferventcoder / @jpogran - I reworked how the module executes arbitrary SQL. Obviously there were a few ways that this could be done, but this is the most simplistic approach to dealing with the "user syntax error" problem. A couple of notes:

  • Since the module internally uses the same templating to execute queries we might want to think about using one mechanism for "known good" queries that we generate vs arbitrary tsql from users that could have syntax errors. It wouldn't take much more code to do that.
  • We will take a small perf hit for using sp_executeql, but I think the benefits probably outweigh the downsides.
  • I did some rudimentary testing to make sure the approach worked, but I might have missed something (more escaping needed?) since I put this together pretty quickly. I used one of the simple queries that the module generates, but I think we want to do a full acceptance over these changes if we intend to move them forward:
BEGIN TRY
    DECLARE @SQLString NVARCHAR(max);
    SET @SQLString = N'USE [database1];
DROP USER [Bob];
IF EXISTS(SELECT name FROM sys.database_principals WHERE name = ''Bob'')
    THROW 51000, ''Failed to drop user Bob'', 10
'
    EXECUTE sp_executesql @SQLString
END TRY
BEGIN CATCH
    DECLARE @msg as VARCHAR(max);
    SELECT @msg = 'THROW CAUGHT: ' + ERROR_MESSAGE();
    THROW 51000, @msg, 10
END CATCH

Result

Msg 51000, Level 16, State 10, Line 13
THROW CAUGHT: Cannot drop the user 'Bob', because it does not exist or you do not have permission.

@Iristyle Iristyle changed the title (MODULES-2312) Use sp_executesql to execute T-SQL (DO NOT MERGE)(MODULES-2312) Use sp_executesql to execute T-SQL Aug 1, 2015
cyberious added a commit that referenced this pull request Aug 3, 2015
…sp_executesql-for-arbitrary-queries

(DO NOT MERGE)(MODULES-2312) Use sp_executesql to execute T-SQL
@cyberious cyberious merged commit cc63029 into puppetlabs:master Aug 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants