I like the idea of automatically closing, but throwing away type safety with
`interface{}` feels like we're just trading one class of bugs for another.
>Right now we're kind of all over the place in terms of sql vs sqlx, Query vs
>Queryx vs NamedQuery, Scan vs StructScan
I don't really understand this. Each of those serves a different purpose—the
convenience of struct-field mapping, the power of raw SQL, and using native Go
instead of incurring the myriad costs of a library where it isn't needed.
Insisting on "consistency" and only allowing one or some of those would be like
shoving a round peg in a square hole, and limit our power and flexibility.
What about something like this?
```Go
func WithRows(f func(rows *sql.Rows) error, tx *sql.Tx, query string, args
...interface{}) error {
rows, err := tx.Query(query, args...)
if err != nil {
return errors.New("querying: " + err.Error())
}
defer rows.Close()
return f(rows)
}
func WithRowsX(f func(rows *sqlx.Rows) error, tx *sqlx.Tx, query string, args
...interface{}) error {…}
```
```Go
func getUnassignedParametersByProfileID(tx *sql.Tx, profileID int)
([]tc.ProfileParameterByName, error) {
qry := `SELECT pa.id, pa.name, pa.value, pa.config_file, pa.secure,
pa.last_updated
FROM parameter pa WHERE pa.id NOT IN (SELECT parameter FROM profile_parameter
pp WHERE pp.profile = $1)`
params := []tc.ProfileParameterByName{}
err := api.WithRows(func(rows *sql.Rows) error {
for rows.Next() {
p := tc.ProfileParameterByName{}
if err := rows.Scan(&p.ID, &p.Name, &p.Value,
&p.ConfigFile, &p.Secure, &p.LastUpdated); err != nil {
return errors.New("scanning profile id
parameters: " + err.Error())
}
params = append(params, p)
}
return nil
}, tx, qry, profileID)
if err != nil {
return nil, errors.New("querying profile name parameters: " +
err.Error())
}
return params, nil
}
```
That gives you the safety of an enforced `Close`, while still retaining the
type safety (notwithstanding the `args` in the standard library), and the
flexibility of allowing callers to use `Rows` however they want.
I actually wrote something very similar in the Riak library, `func
WithClusterTx(tx *sql.Tx, authOpts *riak.AuthOptions, f func(StorageCluster)
error) error`:
https://github.com/apache/trafficcontrol/blob/master/traffic_ops/traffic_ops_golang/riaksvc/riak_services.go#L265
Personally, I think I decided it wasn't worth the cost. The readability cost is
pretty high; it requires additional indentation, which is harder to read; it
requires programmers be familiar with first-class functions, which are
difficult for a lot of people coming from imperative programming backgrounds to
get a handle on; and it's just un-idiomatic Go, the Go language doesn't have
destructors, and the `Closer` interface is common in the standard library,
unsafe or not, using lambdas to hack in destructors just feels unexpected and
not simple and easy code, and that much more difficult for a new programmer on
the project to learn.
But, that's my personal feeling. And I'm not strongly opposed, the pattern just
feels a little worse, not a lot worse. I certainly agree with the value of
automatically closing. If you feel strongly about it, I don't know that I'll
give you a +1, but I probably won't -1 it. I would really like to see the type
safety preserved, though.
Incidentally, we can do the same thing with `APIInfo`:
```Go
// WithInfo executes the given function f, with an APIInfo object containing
the commonly needed API data.
// The http.Request r, requiredParams, and intParams are used to create the
APIInfo object, parsing the parameters from the list of required parameters and
integer parameters. If there is an error, for example creating the transaction,
or if the user didn't pass required parameters, an appropriate error is
returned to the client and f is not called.
// The http.ResponseWriter w is used to write the error to the client, if there
is an error getting the APIInfo data, or if the func f returns an error.
// If f returns a non-nil error, the appropriate error is written to the
client. The func f must return nil errors if it has written to the client, and
likewise must return a non-nil error if a response was not written to the
client.
func WithInfo(w http.ResponseWriter, r *http.Request, requiredParams []string,
intParams []string, f func(inf *APIInfo) (error, error, int)) {
inf, userErr, sysErr, errCode := NewInfo(r, []string{"id"},
[]string{"id"})
if userErr != nil || sysErr != nil {
HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
return
}
defer inf.Close()
userErr, sysErr, errCode = f(inf)
if userErr != nil || sysErr != nil {
HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
return
}
}
```
```Go
func GetUnassigned(w http.ResponseWriter, r *http.Request) {
api.WithInfo(w, r, []string{"id"}, []string{"id"}, func(inf
*api.APIInfo) (error, error, int) {
params, err := getUnassignedParametersByProfileID(inf.Tx.Tx,
inf.IntParams["id"])
if err != nil {
return nil, errors.New("getting params: " +
err.Error()), http.StatusInternalServerError
}
api.WriteResp(w, r, params)
return nil, nil, http.StatusOK
})
}
```
[ Full content available at: https://github.com/apache/trafficcontrol/pull/2862
]
This message was relayed via gitbox.apache.org for [email protected]
