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.

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

@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

@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())
 }

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.

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

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

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

@AlekSi Your bug tests for Scan types which should use valuers if custom. This issue talked about query parameters.

Your bug tests for Scan types which should use valuers if custom. This issue talked about query parameters.

It wasn't my intention to limit this issue only to query parameters, although my analysis was incorrect. Basically, I wanted to be able to insert a value into a database and read it back into the same variable. And I wanted this behavior to be consistent for both predeclared types and named types with predeclared underlying types. As testcase shows, I can do it for type Int64 int64 and type Int32 int32, and I can insert type String string into a database, but I can't read it back.

The reason why my analysis was incorrect is this comment:

// ValueConverter is the interface providing the ConvertValue method.
// …
// … The ValueConverters have several uses:
// …
// * by the sql package, for converting from a driver's Value type
//    to a user's type in a scan.

DefaultParameterConverter implements ValueConverter, but it's not used by scan, that's right. convertAssign implements this logic slightly differently (it does not check for reflect.String), and that's where the problem lies.

I will create a new issue.

Please add the following code to your test:

func (s *String) Scan(v interface{}) error {
	switch t := v.(type) {
	default:
		return fmt.Errorf("Unsupported type %[1]T %[1]v", v)
	case []byte:
		*s = String(string(t))
	case string:
		*s = String(t)
	}
	return nil
}

That works, unsurprisingly. But custom Scan() method is not required for Int32 and Int64. Here reflect.Int32 and reflect.Int64 are checked, but reflect.String (and also slice of bytes) is not.

Filed #18101