Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle nil params in ExecuteSql #75

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

drewlesueur
Copy link

Issue #29

I have tested this with Sybase 12.5, However not with other versions of Sybase/MSSQL.

I took a tip from go-mssqldb where it associates null with "nvarchar(1)", though I am not exactly sure if it's the same context.
https://github.com/denisenkom/go-mssqldb/blob/b91950f658ecd54342d783495e1aadf48a55e967/types.go#L1124

@daniel-redcat
Copy link

@drewlesueur You can test against a local docker instance of latest mssql to make sure it works for that too.
docker pull mcr.microsoft.com/mssql/server:2019-latest
docker run -e 'ACCEPT_EULA=Y' -e 'SA_PASSWORD=yourStrong(!)Password' -p 1433:1433 -d mcr.microsoft.com/mssql/server:2019-latest

@drewlesueur
Copy link
Author

@daniel-redcat Thank you. I did that and added some more tests.
Note that this feature does not work with these types: timestamp, binary, varbinary(max).
But because of the convenience of it working with the other types, I still think this feature is worth adding.

@drewlesueur
Copy link
Author

@ianic Does this feature seem like something you would consider approving?
Thank you!

@gljubojevic
Copy link
Contributor

gljubojevic commented Aug 26, 2020

@drewlesueur would you be so kind to provide a bit more context to a problem proposed PR should resolve.
e.g. at least SQL example where you use ExecuteSql and golang code to execute that SQL including variable types.

Reason why I'm asking is following
While this might work in some cases I would say that in many really can't, e.g. inserting null into int column would require implicit conversion on DB side, and not every DB type can be easily implicitly converted from nvarchar(1).

By further looking at proposed PR, it actually resolves only one use case when ExecuteSql is called explicitly with "nil"
e.g.

ExecuteSql("select * from table where ID=?", nil)

This is crude example and maybe I'm missing something here, please provide example.

More importantly I would expect question more in direction like this:

var p *int
ExecuteSql("insert into table set p=?", p)

@drewlesueur
Copy link
Author

@gljubojevic Yes, thank you for requesting this.

The use case that I am trying to target is when we use gofreetds as a driver for Go's database/sql package, specifically the DB.Exec function.

In a DB.Exec context this change will work if I use an explicit nil, a typed nil, or one of the sql.Null* types.
I understand that this change is really only very helpful when we use this as a driver for database/sql, and not that helpful when using the underlying ExecuteSql call directly.

Also, I have added tests to show that the implicit conversion will work for all the types used in the freetds_types table in the tests, with the exception of timestamp, binary, and varbinary(max). Even though it will not work for those 3 types, because it works with all the other types in the freetds_types test table, I think it is still a worthwhile change.

Here is an example fo Go code that will now work after this change, that wouldn't work before:

package main

import (
    "database/sql"
    _ "github.com/minus5/gofreetds"
)

func main() {
     db, err := sql.Open("mssql", "Your Connection String Here...")
     if err != nil {
         panic(err.Error())
     }
 
     // Works with explicit nil.
     db.Exec("INSERT INTO some_table (some_col) VALUES (?)", nil)
 
     // Works with typed nil.
     var p *int
     db.Exec("INSERT INTO some_table (some_col) VALUES (?)", p)
 
     // Works with sql.Null* types.
     db.Exec("INSERT INTO some_table (some_col) VALUES (?)", sql.NullInt32{Int32: 0, Valid: false})
 }

You are right that this change is not very helpful when using ExecuteSql directly. To get that to work, we'd have to add some more type switching in go2SqlDataType which is doable, but I wanted to focus my change specifically on the database/sql driver use case.

I hope this clarifies my intention and benefits of this change. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants