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

Proposal: database/sql: NULL column values should be parsed as zero values for floats and ints instead of trying to parse with strconv #22542

Closed
odeke-em opened this issue Nov 2, 2017 · 4 comments

Comments

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Nov 2, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version devel +f56f853 Tue Oct 31 01:23:40 2017 -0700 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN="/Users/emmanuelodeke/go/bin"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/emmanuelodeke/go"
GORACE=""
GOROOT="/Users/emmanuelodeke/go/src/go.googlesource.com/go"
GOTOOLDIR="/Users/emmanuelodeke/go/src/go.googlesource.com/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/v3/7z434qpx5v3bw0wh8h2myfpw0000gn/T/go-build329208117=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

You'll need a MySQL server and then run the code below.

package main

import (
	"database/sql"
	"flag"
	"log"

	// MySQL driver
	_ "github.com/go-sql-driver/mysql"
)

type trade struct {
	DBID uint64
	Buy  float64
	Sell float64
	Exch string
}

func main() {
	var dbURL string
	flag.StringVar(&dbURL, "db-url", "user:@/repro", "the URL to connect to MySQL")
	flag.Parse()
	db, err := sql.Open("mysql", dbURL)
	if err != nil {
		log.Fatalf("openDB: %v", err)
	}
	setup := []string{
		"create database if not exists repro",
		"use repro",
		`create table if not exists repro(
			 id integer not null AUTO_INCREMENT,
			 buy float(53),
			 sell float(53),
			 exch varchar(128),
			primary key(id)
		);`,
		"insert into repro (buy, exch) values(10, 'gophexch')",
		"insert into repro (sell, exch) values(-97.6, 'goos')",
	}

	for i, line := range setup {
		if _, err := db.Exec(line); err != nil {
			log.Fatalf("line: #%d err: %v", i, err)
		}
	}

	rows, err := db.Query("select buy, sell, exch from repro")
	if err != nil {
		log.Fatalf("row err: %v", err)
	}
	for rows.Next() {
		curTrade := new(trade)
		err := rows.Scan(&curTrade.Buy, &curTrade.Sell, &curTrade.Exch)
		if err != nil {
			log.Printf("error: %v\n", err)
		}
	}
}

and the respective MySQL table looks like this

mysql> select * from repro;
+----+------+-------+----------+
| id | buy  | sell  | exch     |
+----+------+-------+----------+
|  1 |   10 |  NULL | gophexch |
|  2 | NULL | -97.6 | goos     |
+----+------+-------+----------+
2 rows in set (0.00 sec)

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link, sure: https://gist.github.com/odeke-em/78a237c1b4788ad36162c47e5903217a

What did you expect to see?

Successfully ran

What did you see instead?

2017/11/02 02:37:13 error: sql: Scan error on column index 1: converting driver.Value type ("") to a float64: invalid syntax
2017/11/02 02:37:13 error: sql: Scan error on column index 0: converting driver.Value type ("") to a float64: invalid syntax

This issue arises because we assume that data will never be nil and automatically convert it to a string to be parsed by strconv as in

case reflect.Float32, reflect.Float64:
s := asString(src)

The above can be fixed if we detect that the byte slice of the representative data is nil and automatically set that to zero like in this patch

diff --git a/src/database/sql/convert.go b/src/database/sql/convert.go
index b79ec3f..bb384a9 100644
--- a/src/database/sql/convert.go
+++ b/src/database/sql/convert.go
@@ -402,11 +402,15 @@ func convertAssign(dest, src interface{}) error {
 		dv.SetUint(u64)
 		return nil
 	case reflect.Float32, reflect.Float64:
-		s := asString(src)
-		f64, err := strconv.ParseFloat(s, dv.Type().Bits())
-		if err != nil {
-			err = strconvErr(err)
-			return fmt.Errorf("converting driver.Value type %T (%q) to a %s: %v", src, s, dv.Kind(), err)
+		var f64 float64
+		if src != nil {
+			var err error
+			s := asString(src)
+			f64, err = strconv.ParseFloat(s, dv.Type().Bits())
+			if err != nil {
+				err = strconvErr(err)
+				return fmt.Errorf("converting driver.Value type %T (%q) to a %s: %v", src, s, dv.Kind(), err)
+			}
 		}
 		dv.SetFloat(f64)
 		return nil

or a similar patch that avoids passing in "", "", "NULL" into strconv.

Could we recognize the special case when the column value is NULL thus a nil byte slice, and instead of passing it into strconv, just set that to the zero value?
This issue kept me up as I thought there was some other issue with my database and code so I went on a digging expedition, it has bitten me in the past as well and I suspect that other folks have experienced it before.

I believe that this is a bug, but since it has been this behavior has existed for ages, I'll title this as a proposal.

@gopherbot gopherbot added this to the Proposal milestone Nov 2, 2017
@gopherbot gopherbot added the Proposal label Nov 2, 2017
@odeke-em
Copy link
Member Author

@odeke-em odeke-em commented Nov 2, 2017

@cznic
Copy link
Contributor

@cznic cznic commented Nov 2, 2017

IMO, silently converting NULL to a float64/int64 zero value is a recipe for disaster. It goes directly against the purpose why databases have the concept of NULL in the first place. Users should use sql.NullFloat64 or sql.NullInt64 etc. instead where column values may be NULL.

@rsc
Copy link
Contributor

@rsc rsc commented Nov 6, 2017

Right, this is why we have sql.NullInt64 etc.

@odeke-em
Copy link
Member Author

@odeke-em odeke-em commented Nov 7, 2017

@cznic and @rsc thank you for the input and yeah it makes sense that we have sql.NullInt64 et al, implementing sql.Scanner. It just seems odd to me that I have to radically modify my structs' fields in order to use successfully sql.Scan moreover on fields that may or may not be NULL. I think I'll just replace all my structs with the respective sql.Scanner implementing types. TIL :)

@odeke-em odeke-em closed this Nov 7, 2017
@golang golang locked and limited conversation to collaborators Nov 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.