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

database/sql: different handling of underlying types in DefaultParameterConverter #15174

Closed
AlekSi opened this issue Apr 7, 2016 · 13 comments
Closed

Comments

@AlekSi
Copy link
Contributor

@AlekSi AlekSi commented Apr 7, 2016

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

@bradfitz bradfitz added this to the Go1.7 milestone Apr 7, 2016
@sctb
Copy link
Contributor

@sctb sctb commented May 17, 2016

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.

@rsc rsc modified the milestones: Go1.8, Go1.7 May 18, 2016
@sctb
Copy link
Contributor

@sctb sctb commented May 18, 2016

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

@220kts
Copy link

@220kts 220kts commented Jul 8, 2016

@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
Copy link
Contributor

@sctb sctb commented Jul 8, 2016

@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
Copy link

@220kts 220kts commented Jul 8, 2016

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
Copy link

@gopherbot gopherbot commented Aug 25, 2016

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

@quentinmit quentinmit added the NeedsFix label Oct 6, 2016
@rsc rsc modified the milestones: Go1.9, Go1.8 Nov 11, 2016
@gopherbot gopherbot closed this in d7c0de9 Nov 17, 2016
@AlekSi
Copy link
Contributor Author

@AlekSi AlekSi commented Nov 27, 2016

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

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 27, 2016

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

@kardianos
Copy link
Contributor

@kardianos kardianos commented Nov 27, 2016

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

@AlekSi
Copy link
Contributor Author

@AlekSi AlekSi commented Nov 27, 2016

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.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Nov 27, 2016

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
}
@AlekSi
Copy link
Contributor Author

@AlekSi AlekSi commented Nov 29, 2016

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.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Nov 29, 2016

Filed #18101

@golang golang locked and limited conversation to collaborators Nov 29, 2017
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
8 participants
You can’t perform that action at this time.