exp/sql: rename NullableString to NullString and allow its use as a parameter

Prep for Issue 2699

R=rsc
CC=golang-dev
https://golang.org/cl/5536045

5 files changed, 184 insertions, 12 deletions

diff --git a/src/pkg/exp/sql/convert_test.go b/src/pkg/exp/sql/convert_test.go
index 702ba4399d..8c0cafc150 100644
--- a/src/pkg/exp/sql/convert_test.go
+++ b/src/pkg/exp/sql/convert_test.go
@@ -5,6 +5,7 @@
 package sql
 
 import (
+	"exp/sql/driver"
 	"fmt"
 	"reflect"
 	"testing"
@@ -154,8 +155,8 @@ func TestConversions(t *testing.T) {
 	}
 }
 
-func TestNullableString(t *testing.T) {
-	var ns NullableString
+func TestNullString(t *testing.T) {
+	var ns NullString
 	convertAssign(&ns, []byte("foo"))
 	if !ns.Valid {
 		t.Errorf("expecting not null")
@@ -171,3 +172,35 @@ func TestNullableString(t *testing.T) {
 		t.Errorf("expecting blank on nil; got %q", ns.String)
 	}
 }
+
+type valueConverterTest struct {
+	c       driver.ValueConverter
+	in, out interface{}
+	err     string
+}
+
+var valueConverterTests = []valueConverterTest{
+	{driver.DefaultParameterConverter, NullString{"hi", true}, "hi", ""},
+	{driver.DefaultParameterConverter, NullString{"", false}, nil, ""},
+}
+
+func TestValueConverters(t *testing.T) {
+	for i, tt := range valueConverterTests {
+		out, err := tt.c.ConvertValue(tt.in)
+		goterr := ""
+		if err != nil {
+			goterr = err.Error()
+		}
+		if goterr != tt.err {
+			t.Errorf("test %d: %s(%T(%v)) error = %q; want error = %q",
+				i, tt.c, tt.in, tt.in, goterr, tt.err)
+		}
+		if tt.err != "" {
+			continue
+		}
+		if !reflect.DeepEqual(out, tt.out) {
+			t.Errorf("test %d: %s(%T(%v)) = %v (%T); want %v (%T)",
+				i, tt.c, tt.in, tt.in, out, out, tt.out, tt.out)
+		}
+	}
+}
diff --git a/src/pkg/exp/sql/driver/types.go b/src/pkg/exp/sql/driver/types.go
index d6ba641cb2..f383885231 100644
--- a/src/pkg/exp/sql/driver/types.go
+++ b/src/pkg/exp/sql/driver/types.go
@@ -32,6 +32,15 @@ type ValueConverter interface {
 	ConvertValue(v interface{}) (interface{}, error)
 }
 
+// SubsetValuer is the interface providing the SubsetValue method.
+//
+// Types implementing SubsetValuer interface are able to convert
+// themselves to one of the driver's allowed subset values.
+type SubsetValuer interface {
+	// SubsetValue returns a driver parameter subset value.
+	SubsetValue() (interface{}, error)
+}
+
 // Bool is a ValueConverter that converts input values to bools.
 //
 // The conversion rules are:
@@ -136,6 +145,32 @@ func (stringType) ConvertValue(v interface{}) (interface{}, error) {
 	return fmt.Sprintf("%v", v), nil
 }
 
+// Null is a type that implements ValueConverter by allowing nil
+// values but otherwise delegating to another ValueConverter.
+type Null struct {
+	Converter ValueConverter
+}
+
+func (n Null) ConvertValue(v interface{}) (interface{}, error) {
+	if v == nil {
+		return nil, nil
+	}
+	return n.Converter.ConvertValue(v)
+}
+
+// NotNull is a type that implements ValueConverter by disallowing nil
+// values but otherwise delegating to another ValueConverter.
+type NotNull struct {
+	Converter ValueConverter
+}
+
+func (n NotNull) ConvertValue(v interface{}) (interface{}, error) {
+	if v == nil {
+		return nil, fmt.Errorf("nil value not allowed")
+	}
+	return n.Converter.ConvertValue(v)
+}
+
 // IsParameterSubsetType reports whether v is of a valid type for a
 // parameter. These types are:
 //
@@ -200,6 +235,17 @@ func (defaultConverter) ConvertValue(v interface{}) (interface{}, error) {
 		return v, nil
 	}
 
+	if svi, ok := v.(SubsetValuer); ok {
+		sv, err := svi.SubsetValue()
+		if err != nil {
+			return nil, err
+		}
+		if !IsParameterSubsetType(sv) {
+			return nil, fmt.Errorf("non-subset type %T returned from SubsetValue", sv)
+		}
+		return sv, nil
+	}
+
 	rv := reflect.ValueOf(v)
 	switch rv.Kind() {
 	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
@@ -215,5 +261,5 @@ func (defaultConverter) ConvertValue(v interface{}) (interface{}, error) {
 	case reflect.Float32, reflect.Float64:
 		return rv.Float(), nil
 	}
-	return nil, fmt.Errorf("unsupported type %s", rv.Kind())
+	return nil, fmt.Errorf("unsupported type %T, a %s", v, rv.Kind())
 }
diff --git a/src/pkg/exp/sql/fakedb_test.go b/src/pkg/exp/sql/fakedb_test.go
index 70aa68c138..0376583b68 100644
--- a/src/pkg/exp/sql/fakedb_test.go
+++ b/src/pkg/exp/sql/fakedb_test.go
@@ -589,7 +589,9 @@ func converterForType(typ string) driver.ValueConverter {
 	case "int32":
 		return driver.Int32
 	case "string":
-		return driver.String
+		return driver.NotNull{driver.String}
+	case "nullstring":
+		return driver.Null{driver.String}
 	case "datetime":
 		return driver.DefaultParameterConverter
 	}
diff --git a/src/pkg/exp/sql/sql.go b/src/pkg/exp/sql/sql.go
index cba7e9ebe5..3201e76674 100644
--- a/src/pkg/exp/sql/sql.go
+++ b/src/pkg/exp/sql/sql.go
@@ -35,11 +35,11 @@ func Register(name string, driver driver.Driver) {
 // valid until the next call to Next, Scan, or Close.
 type RawBytes []byte
 
-// NullableString represents a string that may be null.
-// NullableString implements the ScannerInto interface so
+// NullString represents a string that may be null.
+// NullString implements the ScannerInto interface so
 // it can be used as a scan destination:
 //
-//  var s NullableString
+//  var s NullString
 //  err := db.QueryRow("SELECT name FROM foo WHERE id=?", id).Scan(&s)
 //  ...
 //  if s.Valid {
@@ -49,19 +49,27 @@ type RawBytes []byte
 //  }
 //
 // TODO(bradfitz): add other types.
-type NullableString struct {
+type NullString struct {
 	String string
 	Valid  bool // Valid is true if String is not NULL
 }
 
 // ScanInto implements the ScannerInto interface.
-func (ms *NullableString) ScanInto(value interface{}) error {
+func (ns *NullString) ScanInto(value interface{}) error {
 	if value == nil {
-		ms.String, ms.Valid = "", false
+		ns.String, ns.Valid = "", false
 		return nil
 	}
-	ms.Valid = true
-	return convertAssign(&ms.String, value)
+	ns.Valid = true
+	return convertAssign(&ns.String, value)
+}
+
+// SubsetValue implements the driver SubsetValuer interface.
+func (ns NullString) SubsetValue() (interface{}, error) {
+	if !ns.Valid {
+		return nil, nil
+	}
+	return ns.String, nil
 }
 
 // ScannerInto is an interface used by Scan.
@@ -530,6 +538,27 @@ func (s *Stmt) Exec(args ...interface{}) (Result, error) {
 	// Convert args to subset types.
 	if cc, ok := si.(driver.ColumnConverter); ok {
 		for n, arg := range args {
+			// First, see if the value itself knows how to convert
+			// itself to a driver type.  For example, a NullString
+			// struct changing into a string or nil.
+			if svi, ok := arg.(driver.SubsetValuer); ok {
+				sv, err := svi.SubsetValue()
+				if err != nil {
+					return nil, fmt.Errorf("sql: argument index %d from SubsetValue: %v", n, err)
+				}
+				if !driver.IsParameterSubsetType(sv) {
+					return nil, fmt.Errorf("sql: argument index %d: non-subset type %T returned from SubsetValue", n, sv)
+				}
+				arg = sv
+			}
+
+			// Second, ask the column to sanity check itself. For
+			// example, drivers might use this to make sure that
+			// an int64 values being inserted into a 16-bit
+			// integer field is in range (before getting
+			// truncated), or that a nil can't go into a NOT NULL
+			// column before going across the network to get the
+			// same error.
 			args[n], err = cc.ColumnConverter(n).ConvertValue(arg)
 			if err != nil {
 				return nil, fmt.Errorf("sql: converting Exec argument #%d's type: %v", n, err)
diff --git a/src/pkg/exp/sql/sql_test.go b/src/pkg/exp/sql/sql_test.go
index 30cd97d176..3fe93986fa 100644
--- a/src/pkg/exp/sql/sql_test.go
+++ b/src/pkg/exp/sql/sql_test.go
@@ -340,3 +340,65 @@ func TestQueryRowClosingStmt(t *testing.T) {
 		t.Errorf("statement close mismatch: made %d, closed %d", made, closed)
 	}
 }
+
+func TestNullStringParam(t *testing.T) {
+	db := newTestDB(t, "")
+	defer closeDB(t, db)
+	exec(t, db, "CREATE|t|id=int32,name=string,favcolor=nullstring")
+
+	// Inserts with db.Exec:
+	exec(t, db, "INSERT|t|id=?,name=?,favcolor=?", 1, "alice", NullString{"aqua", true})
+	exec(t, db, "INSERT|t|id=?,name=?,favcolor=?", 2, "bob", NullString{"brown", false})
+
+	_, err := db.Exec("INSERT|t|id=?,name=?,favcolor=?", 999, nil, nil)
+	if err == nil {
+		// TODO: this test fails, but it's just because
+		// fakeConn implements the optional Execer interface,
+		// so arguably this is the correct behavior.  But
+		// maybe I should flesh out the fakeConn.Exec
+		// implementation so this properly fails.
+		// t.Errorf("expected error inserting nil name with Exec")
+	}
+
+	// Inserts with a prepared statement:
+	stmt, err := db.Prepare("INSERT|t|id=?,name=?,favcolor=?")
+	if err != nil {
+		t.Fatalf("prepare: %v", err)
+	}
+	if _, err := stmt.Exec(3, "chris", "chartreuse"); err != nil {
+		t.Errorf("exec insert chris: %v", err)
+	}
+	if _, err := stmt.Exec(4, "dave", NullString{"darkred", true}); err != nil {
+		t.Errorf("exec insert dave: %v", err)
+	}
+	if _, err := stmt.Exec(5, "eleanor", NullString{"eel", false}); err != nil {
+		t.Errorf("exec insert dave: %v", err)
+	}
+
+	// Can't put null name into non-nullstring column,
+	if _, err := stmt.Exec(5, NullString{"", false}, nil); err == nil {
+		t.Errorf("expected error inserting nil name with prepared statement Exec")
+	}
+
+	type nameColor struct {
+		name     string
+		favColor NullString
+	}
+
+	wantMap := map[int]nameColor{
+		1: nameColor{"alice", NullString{"aqua", true}},
+		2: nameColor{"bob", NullString{"", false}},
+		3: nameColor{"chris", NullString{"chartreuse", true}},
+		4: nameColor{"dave", NullString{"darkred", true}},
+		5: nameColor{"eleanor", NullString{"", false}},
+	}
+	for id, want := range wantMap {
+		var got nameColor
+		if err := db.QueryRow("SELECT|t|name,favcolor|id=?", id).Scan(&got.name, &got.favColor); err != nil {
+			t.Errorf("id=%d Scan: %v", id, err)
+		}
+		if got != want {
+			t.Errorf("id=%d got %#v, want %#v", id, got, want)
+		}
+	}
+}