The "right" way to do stored procedure parameter validation

Question

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?

Answer 1

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
    RAISEERROR('Invalid parameter: @fooString cannot be NULL or empty', 18, 0)
    RETURN
END

Answer 2

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

Answer 3

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