minus5 / gofreetds

Go Sql Server database driver.
MIT License
113 stars 48 forks source link

OUTPUT parameter handling is broken by PR#47 #48

Closed edharper01 closed 7 years ago

edharper01 commented 7 years ago

I believe the change to OUTPUT parameter handling introduced in PR#47 is incorrect and should be reverted.

My understanding of the issue is based on the stored procedure used in the test case added as part of the patch. In pure T-SQL, the code to create this stored procedure is:

create proc test_sp_bit_type_null_output
(
    @shouldReturn bit,
    @bitValue bit=1 output
) as
    if 1 = @shouldReturn
    begin
        set @bitValue = 1
        return 0
    end
    return -1

The patch seems to be based on the premise that the default value for the OUTPUT parameter @bitValue should be used if an input value for the parameter is not explicitly provided. The test case calls the procedure in this way, only providing a value for the first input parameter @shouldReturn:

rst, err := conn.ExecSp(spName, false)

(conn_sp_test.go line 428)

However, I believe this is a misapprehension about how OUTPUT parameters work in T-SQL. To call this procedure from T-SQL and collect the value of the OUTPUT parameter, it is always necessary to provide an input value. This can be seen when calling the procedure with @shouldReturn set to 0, which does not modify the values of @bitValue

declare @out bit --@out is created with the value NULL
exec test_sp_bit_type_null_output @shouldReturn = 0, @bitValue = @out OUTPUT
select @out AS out

The result is:

out
-----
NULL

(1 row(s) affected)

The default value for @bitValue will only be used when the parameter is not referenced in the call - in which case, the value of the OUTPUT parameter cannot be collected:

exec test_sp_bit_type_null_output @shouldReturn = 0

On this basis, I believe the patch is misguided and should be rolled back.

ianic commented 7 years ago

Reverted to the state before patch. Thanks.