Returning an SQL @parameter from a stored procedure into my application

Question

I have looked into this issue before I posted, and found some solutions which I have tried to apply, but no luck in it working.

I have a stored procedure to insert records into a database. I want to return a value as a status (I have seen that you can only return an int on a RETURN and that is fine in this case).

    ALTER PROCEDURE [dbo].[_example] 
    -- Add the parameters for the stored procedure here
    @Parameters)
AS
BEGIN
    -- SET NOCOUNT ON added to prevent extra result sets from
    -- interfering with SELECT statements.
    SET NOCOUNT ON;

DECLARE @Example int

DECLARE @retVal int = 0

IF (@example is null)
    BEGIN
        SET @retVal +=1;
    END
    
    return @retVal

END

in my c# code I have written this:

    public static int ExecuteSPWithReturnValue(string queryString)//not returning the value correctly
    {

        connection.Open();
        SqlCommand command = new SqlCommand(queryString, connection)
        {
            CommandTimeout = 0
        };
        SqlParameter retVal = command.Parameters.Add("@retVal", SqlDbType.Int);
        retVal.Direction = ParameterDirection.ReturnValue;
        command.ExecuteNonQuery();
        int result = (int)retVal.Value;

        connection.Close();
        return result;
    }

I call it from another class like this:

            int returnStatus;

            returnStatus = SQLUtility.ExecuteSPWithReturnValue("EXEC dbo._example " +
            "@param1= '" + string + "'," +
            "@param2= '" + string + "'," +
            "@param3 = '" + string + "'," +
            "@param4 ='" + string + "'");

but it just returns 0 no matter what. I know the value of the parameter is between 0-3, but that's not being passed into the int return Status.

Why?

Instead of doing `EXEC dbo._example` you should set the `command.CommandType` to `StoredProcedure`, set the `CommandText` to only `dbo._example`, and add the input parameters without that string concatenation. As a bonus, you would avoid the SQL Injection you introduced in your code with that string concatenation. — Cleptus, Aug 11 '21 at 13:05
The pattern where you build a wrapper method that accepts a query string tends to force or encourage code that's scary-vulnerable to sql injection issues. — Joel Coehoorn, Aug 11 '21 at 13:18
What if you (temporarily) initialize that @retVal to a non-0 value, do you still get 0 as result? IOW is the issue in setting that value in the procedure or in getting the param value? — Hans Keﬆing, Aug 11 '21 at 13:53

score 3 · Accepted Answer · edited Aug 12 '21 at 12:08

3

The issue seems to be that you are sending through an ad-hoc batch, which is not returning the value.

You could change your batch to

DECLARE @retval int;
EXEC @retval = dbo._example .....
RETURN @retval;

But it's much better to just execute the procedure directly, using CommandType.StoredProcedure. This avoids nasty SQL injection also.

Note also that you should dispose the connection and command objects with using, do not cache them

public static int ExecuteSPWithReturnValue(string queryString, bool isProcedure, params SqlParameter[] parameters)
{
    using (var connection = new SqlConnection(myConnectionString))
    using (SqlCommand command = new SqlCommand(queryString, connection))
    {
        if(isProcedure)
            command.CommandType = CommandType.StoredProcedure;
        command.CommandTimeout = 0;
        command.Parameters.AddRange(parameters);
        var retVal = command.Parameters.Add("@retVal", SqlDbType.Int);
        retVal.Direction = ParameterDirection.ReturnValue;
        connection.Open();
        command.ExecuteNonQuery();
        return (int)retVal.Value;
    }

edited Aug 12 '21 at 12:08

Cleptus

3,446
4
28
34

answered Aug 11 '21 at 13:49

Charlieface

52,284
6
19
43

Thank you for this, the code looks a lot neater and I'll test it now. I just have one comment and a follow up question. I am actually opening my connection as a global variable in this class, this might be bad practice? but i'm pulling the connection string from the settings and it is a static class so I assume as I am not creating an instance of the class the connection should be closed anyway? either way i'm calling connection.Close at the end of each function I write. – Craig Aug 11 '21 at 15:15
My question is around params SqlParameter[] parameters. Normally there is the type and then the assigned name, here you have three things? how do I assign values into this in my main code? – Craig Aug 11 '21 at 15:21
`params` means you can pass them in as if there were an unlimited number of parameters, eg `ExecuteSPWithReturnValue("SomeSP", true, new SqlParameter(....), new SqlParameter(...), new SqlParameter(...));` see also https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/params – Charlieface Aug 11 '21 at 16:45
2

@Craig Re caching the connection, see https://stackoverflow.com/questions/17552829/c-sharp-data-connections-best-practice. The problem with just calling `Close` is that it leaks in the event of an exception. Because of connection pooling, ADO.Net will manage the internal connection, and close it if there is no command for some time, so you don't lose, but if you leak a connection then the connection pool could fill up. – Charlieface Aug 11 '21 at 16:48
1

In a function named Execute**SP**WithReturnValue is kinda weird adding a `isProcedure` parameter. Usually "SP" stands for Stored Procedure. – Cleptus Aug 12 '21 at 12:07
@Craig In addition to what Charlieface answered in the follow up, I should say it is also problematic when no exceptions are raised but MARS is enabled (by default it is not). – Cleptus Aug 12 '21 at 12:14
@Cleptus I'll rename the function! good point. I like the isProcedure parameter as it gives the function more flexibility. – Craig Aug 12 '21 at 14:28
Thank you for all your support with this. I've learned a little more about best practice and it now working by using the suggested code. Much appreciated. – Craig Aug 17 '21 at 09:15

Returning an SQL @parameter from a stored procedure into my application

1 Answers1