database/sql: different handling of underlying types in DefaultParameterConverter

[AlekSi]

After e405c29 (#11489) DefaultParameterConverter documentation says:

// DefaultParameterConverter returns its argument directly if
// IsValue(arg). Otherwise, if the argument implements Valuer, its
// Value method is used to return a Value. As a fallback, the provided
// argument's underlying type is used to convert it to a Value:
// underlying integer types are converted to int64, floats to float64,
// and strings to []byte. If the argument is a nil pointer,
// ConvertValue returns a nil Value. If the argument is a non-nil
// pointer, it is dereferenced and ConvertValue is called
// recursively. Other types are an error.

Underlying integers are really converted to int64, but underlying strings are not in fact converted – there is no code for this.

Testcase: https://github.com/AlekSi/go-sql-bugs/blob/master/issue15174_test.go
Test output: https://travis-ci.org/AlekSi/go-sql-bugs/builds/121408329 (see 1.6 and tip)

I propose to do what documentation says and convert the underlying string to []byte, it will help with custom string-like types with constants (enums).
Also, can we do the same thing for bools?

/cc @bradfitz

[sctb]

Is there a reason to convert underlying strings to []byte instead of just string (it is a Value after all)? We could then go ahead and support nearly all of the Value types consistently: int64, float64, bool, []byte, string.

[sctb]

I've got a change ready to go once 1.7 is out.

[220kts]

@sctb A bit of a tangent, any chance you could take a look at #8415 and see if my proposed fix there ties in with the code changes you are working on here?

cc @ianlancetaylor , @ConnorFoody

[sctb]

@220kts The change I had in mind here is quite straightforward,
but I didn't include and changes to the handling of nil values:

@@ -245,6 +245,16 @@ func (defaultConverter) ConvertValue(v interface{}) (Value, error) {
                return int64(u64), nil
        case reflect.Float32, reflect.Float64:
                return rv.Float(), nil
+       case reflect.Bool:
+               return rv.Bool(), nil
+       case reflect.Slice:
+               ek := rv.Type().Elem().Kind()
+               if ek == reflect.Uint8 {
+                       return rv.Bytes(), nil
+               }
+               return nil, fmt.Errorf("unsupported type %T, a slice of %s", v, ek)
+       case reflect.String:
+               return rv.String(), nil
        }
        return nil, fmt.Errorf("unsupported type %T, a %s", v, rv.Kind())
 }

[220kts]

Thanks @sctb The current implementation is a bit inconsistent in its handling of nil pointers: a nil pointer to a basic type returns nil, nil but a nil pointer to a type that implements the Valuer interface calls the type's Value function. This in turn breaks when the Value function is defined on a value (not on a pointer).

I would suggest to handle nil pointers consistently between basic types and types that implement the Valuer interface. In the case of a type that implements the Valuer interface, this would allow the caller to decide whether to pass a pointer or a value to the ConvertValue function (which logically tends to correspond to database columns with or without NOT NULL constraint).

Just thought it might make sense to consider that along with the change you propose.

[gopherbot]

CL https://golang.org/cl/27812 mentions this issue.

[AlekSi]

Original testcase still fails on tip: https://travis-ci.org/AlekSi/go-sql-bugs/jobs/169501389.

[bradfitz]

@AlekSi, closed bugs aren't tracked. File a new bug if you must.