CODEWITHSUNDEEP
sqlsql-serverstored-procedureserror-handling
VVS
VVS

Reputation: 19612

The "right" way to do stored procedure parameter validation

I have a stored procedure that does some parameter validation and should fail and stop execution if the parameter is not valid.

My first approach for error checking looked like this:

create proc spBaz
(
  @fooInt int = 0,
  @fooString varchar(10) = null,
  @barInt int = 0,
  @barString varchar(10) = null
)
as
begin
  if (@fooInt = 0 and (@fooString is null or @fooString = ''))
    raiserror('invalid parameter: foo', 18, 0)

  if (@barInt = 0 and (@barString is null or @barString = ''))
    raiserror('invalid parameter: bar', 18, 0)

  print 'validation succeeded'
  -- do some work
end

This didn't do the trick since severity 18 doesn't stop the execution and 'validation succeeded' is printed together with the error messages.

I know I could simply add a return after every raiserror but this looks kind of ugly to me:

  if (@fooInt = 0 and (@fooString is null or @fooString = ''))
  begin
    raiserror('invalid parameter: foo', 18, 0)
    return
  end

  ...

  print 'validation succeeded'
  -- do some work

Since errors with severity 11 and higher are caught within a try/catch block another approach I tested was to encapsulate my error checking inside such a try/catch block. The problem was that the error was swallowed and not sent to the client at all. So I did some research and found a way to rethrow the error:

  begin try
    if (@fooInt = 0 and (@fooString is null or @fooString = ''))
      raiserror('invalid parameter: foo', 18, 0)

    ...
  end try
  begin catch
    exec usp_RethrowError
    return
  end catch

  print 'validation succeeded'
  -- do some work

I'm still not happy with this approach so I'm asking you:

How does your parameter validation look like? Is there some kind of "best practice" to do this kind of checking?

Upvotes: 39

Views: 59313

Answers (6)

logeshpalani31
logeshpalani31

Reputation: 1638

The sample code for validation in mySQL stored procedure:

DROP PROCEDURE IF EXISTS update_payment;

delimiter $$
CREATE PROCEDURE update_payment
(
    payment_id INT,
    amount DECIMAL(5,2),
    date DATE
)

BEGIN 
    IF amount <= 0 THEN
        SIGNAL SQLSTATE '22003' SET MESSAGE_TEXT = 'Amount should not be less or qual to zero' ;
    END IF;
    UPDATE payments p
    SET p.amount = amount,
    p.date = date
    Where p.payment_id = payment_id;
END $$

delimiter ;

Upvotes: 1

LukeH
LukeH

Reputation: 269558

I don't think that there is a single "right" way to do this.

My own preference would be similar to your second example, but with a separate validation step for each parameter and more explicit error messages.

As you say, it's a bit cumbersome and ugly, but the intent of the code is obvious to anyone reading it, and it gets the job done.

IF (ISNULL(@fooInt, 0) = 0)
BEGIN
    RAISERROR('Invalid parameter: @fooInt cannot be NULL or zero', 18, 0)
    RETURN
END

IF (ISNULL(@fooString, '') = '')
BEGIN
    RAISERROR('Invalid parameter: @fooString cannot be NULL or empty', 18, 0)
    RETURN
END

Upvotes: 54

Andras Zoltan
Andras Zoltan

Reputation: 42363

As you can see from this answer history I followed this question and accepted answer, and then proceeded to 'invent' a solution that was basically the same as your second approach.

Caffeine is my main source of energy, due to the fact that I spend most of my life half-asleep as I spend far too much time coding; thus I didn't realise my faux-pas until you rightly pointed it out.

Therefore, for the record, I prefer your second approach: using an SP to raise the current error, and then using a TRY/CATCH around your parameter validation.

It reduces the need for all the IF/BEGIN/END blocks and therefore reduces the line count as well as puts the focus back on the validation. When reading through the code for the SP it's important to be able to see the tests being performed on the parameters; all the extra syntactic fluff to satisfy the SQL parser just gets in the way, in my opinion.

Upvotes: 0

Dalex
Dalex

Reputation: 3625

I always use parameter @Is_Success bit as OUTPUT. So if I have an error then @Is_success=0. When parent procedure checks that @Is_Success=0 then it rolls back its transaction(with child transactions) and sends error message from @Error_Message to client.

Upvotes: 0

KM.
KM.

Reputation: 103667

I prefer to return out as soon an possible, and see not point to having everything return out from the same point at the end of the procedure. I picked up this habit doing assembly, years ago. Also, I always return a value:

RETURN 10

The application will display a fatal error on positive numbers, and will display the user warning message on negative values.

We always pass back an OUTPUT parameter with the text of the error message.

example:

IF ~error~
BEGIN
    --if it is possible to be within a transaction, so any error logging is not ROLLBACK later
    IF XACT_STATE()!=0
    BEGIN
        ROLLBACK
    END

    SET @OutputErrMsg='your message here!!'
    INSERT INTO ErrorLog (....) VALUES (.... @OutputErrMsg)
    RETURN 10

END

Upvotes: 0

Andomar
Andomar

Reputation: 238246

We normally avoid raiseerror() and return a value that indicates an error, for example a negative number:

if <errorcondition>
    return -1

Or pass the result in two out parameters:

create procedure dbo.TestProc
    ....
    @result int output,
    @errormessage varchar(256) output
as
set @result = -99
set @errormessage = null
....
if <errorcondition>
    begin
    set @result = -1
    set @errormessage = 'Condition failed'
    return @result
    end

Upvotes: 1

Related Questions