2

I have a table in my db that stored both successful and failed log-in attempts. I am creating a stored procedure that allow us to delete records which are older than X days old. So far so good, but I want to raise it a notch (or two) and allow us to specify whether to delete records on whether the [Success] column is true, false or both. I am having some issues with the concatenation of the script that needs to be executed, though.

Here is what I did so far:

-- CREATE PROCEDURE [dbo].[sp_delete_log_attempts]
DECLARE @backDays INT = 1 -- Default to 30 days (one test finishes)
DECLARE @successArg BIT = NULL -- default to both true and false success logins
DECLARE @successAnd VARCHAR(50)
DECLARE @query VARCHAR(MAX)

SET @successAnd = CASE
WHEN @successArg = 'true' THEN
    'AND [Success] = ''true'''
WHEN @successArg = 'false' THEN
    'AND [Success] = ''false'''
ELSE
    'AND [Success] = ''true'' OR [Success] = ''false'''
END

PRINT @successAnd -- just for debugging purposes

SET @query = 'SELECT * FROM [audit].LoginAttempt WHERE [TimeStamp] <= DATEADD(day,-' + @backDays + ', GETDATE())'
EXEC @query

At this point I am just trying to select the rows based on the @backDays variable, but I cannot concatenate the @query string with the variable. Not sure of what I am doing wrong here. I am fairly new to dynamic queries, though.

  • The way you're doing things now is wide open to SQL injection. To learn everything you need to know about dynamic SQL, head over here: The Curse and Blessings of Dynamic SQL – Erik Darling Apr 27 '18 at 12:00
  • but I could pout a variable checker to ensure that only given values are accepted, couldn't I? – pmdci Apr 27 '18 at 12:33
  • @sp_BlitzErik Can you give me an example or SQL injection in this case? – pmdci Apr 27 '18 at 12:35
  • In this case, with an int? Likely not. But it's a terrible coding practice, and I'd hate for someone to come along and think this is how they should write dynamic SQL. – Erik Darling Apr 27 '18 at 12:59
5

I don't think you even need dynamic SQL here. You can use variables in the query.

SELECT *
       FROM [audit].[LoginAttempt]
       WHERE [TimeStamp] <= dateadd(day, -1 * @backDays, getdate())
             AND CASE
                   WHEN @successArg IS NOT NULL
                     THEN (SELECT 1
                                  WHERE [Success] = @successArg)
                   ELSE 1
                 END = 1;

(Not tested as no sample data was provided. Just to demonstrate the idea.)

If you look close at it, you'll see, that the entire CASE ... END is the left operand of an = operation. The right side of the operation is 1.

Unfortunately SQL Server knows no Boolean type we could have directly returned from the CASE ... END. (Others, e.g. PostgreSQL do.) So we have to trick it using an expression it can use for a Boolean operation. That's why we use the = operation.

Now we need a way of making this operation evaluate to true when the conditions we actually want to test are met. So the idea is, if we come to the decision, that they are met, we return one designated value for our left side of the =. On the other side, we just use that value literally. The = will be true then. If our conditions don't meet, we return any other value for the left side, and the = won't be true. As a value to indicate the conditions were met, lets choose the 1. It's close to a representation of a Boolean we might have in mind. (But we could have chosen nearly anything (NOT NULL, otherwise we had to change the = operation to IS NULL).).

So how can we make our left expression to return 1, when the conditions are met?

Well, we can use a CASE ... END statement to return a value based on certain conditions. You already know the CASE ... END. It's somewhat similar to a switch or if else constructs in C-like languages (or with different names in other procedural languages).

What we need to test is if the input variable @successArg is null or not. If it's null, that means the caller doesn't care, whether the logged login attempts were successful or not. Otherwise the value of @successArg indicates whether he wants only successful logins (@successArg = 1) or only unsuccessful (@successArg = 0). This will give us to cases on top: Either ignore the successfulness or take it into account. Hence we have two (main) branches in our CASE ... END branching about @successArg IS NOT NULL;

Let's start with the easier case, when @successArg IS NULL. That's the 'ELSE' branch. Here, we don't care about if the login attempt was successful or not. Being successful or not successful (think of it a Boolean expression) is always true, for any row. So we just return our indicator, that this was a match, 1.

When @successArg IS NULL, @successArg holds the value, the rows must have in the column [Success]. This is given only if @successArg = [Success]. As already mentioned, @successArg = [Success] alone won't be something SQL Server could handle in that context. So we need our true indicating value 1 to be returned when @successArg = [Success] for the current row, something else otherwise.

One way of doing this would be another, inner CASE ... WHEN or we might use a correlated subquery. It's "correlated" as it uses values of columns from the current row of the outer query. And it's "sub", well because it's "sub", viewed from the position of the outer query. Furthermore, we use the feature in SQL Server that a SELECT with no FROM will produce one record if the WHERE of that query is true (or if there is none), otherwise an empty set. So let's produce a record with one column holding 1 when @successArg = [Success]. Such a record, with only one column, will implicitly be casted to a scalar if needed, hence it's suitable for our = operation outside. When @successArg <> [Success], that subquery will produce an empty set. The empty set in that scalar context will be implicitly casted to NULL. As NULL = 1 isn't true, this will do it for us.

One side note on this: It also can be done without a CASE ... END but simply a Boolean expression:

SELECT *
       FROM [audit].[LoginAttempt]
       WHERE [TimeStamp] <= dateadd(day, -1 * @backDays, getdate())
             AND (@successArg IS NULL
                   OR [Success] = @successArg);

That follows our thoughts from above. In case @successArg IS NULL we want "true" regardless of anything else. Otherwise we only want "true" when [Success] = @successArg. In any other case "false". (Note: The parenthesis are needed to override the precedence of AND over OR because the previous condition.)

In terms of how the query is executed, hence performance, I don't think this makes a significant difference, if any (but frankly I don't know this for sure). The solution with a CASE ... WHEN though, might be easier to read and therefore maintain. The Boolean only solution, on the other hand, also works in systems, that are not capable of a CASE ... END.

  • This is awesome. But [Success] IS bit, by the way. I just used the terms 'true' or 'false' for better reading. – pmdci Apr 27 '18 at 13:16
  • pmci: My apologies, I have overlooked the declaration. Then a test on (not) null should suffice. See my edit. – sticky bit Apr 27 '18 at 13:20
  • I tested it it works beautifully! This whole SELECT 1 / ELSE 1 / END 1 stuff is news to me though. How is this called and who can I learn more about it? – pmdci Apr 27 '18 at 13:28
  • Actually I need to do some additional edits on it to make it work as a stored procedure. Let me come back to you with this. – pmdci Apr 27 '18 at 13:41
  • Yeah I tested and it works fine! I was just trying to combine this with a check to see that @backDays isn't less than 1 and it didn't work. But this is beyond the original question. – pmdci Apr 27 '18 at 13:51

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.