go-reform/reform

Remove runtime reflection introduced in v1.5

AlekSi opened this issue · 0 comments

Since at least reform v1.0.0, it generated the following code:

reform/reform/template.go

Lines 127 to 134 in 213ddec

// SetPK sets record primary key.
func (s *{{ .Type }}) SetPK(pk interface{}) {
if i64, ok := pk.(int64); ok {
s.{{ .PKField.Name }} = {{ .PKField.Type }}(i64)
} else {
s.{{ .PKField.Name }} = pk.({{ .PKField.Type }})
}
}

Type there is a Record struct type, PKField.Name is record's primary key field name, int64 is a fixed type returned by database/sql.Result.LastInsertId, and PKField.Type is an actual primary key filed type as written in the source file. The first branch is used for databases and drivers that use autoincrement integer primary keys via LastInsertId. The second branch is used for other databases like PostgreSQL, where RETURNING clause is used for that. Reform itself actually uses that generated method only in one place:

switch lastInsertIdMethod {
case LastInsertId:
res, err := q.Exec(query, values...)
if err != nil {
return err
}
if record != nil && !record.HasPK() {
id, err := res.LastInsertId()
if err != nil {
return err
}
record.SetPK(id)
}
return nil

Effectively, only the first branch is actually used, but SetPK was kept in the current form for generability. In retrospect, that was a mistake: go vet in 1.15 started to complain about that code as reported at #245 due to golang/go#32479.

I investigated three ways to fix that problem:

  1. Significantly change the behavior of this method, change the signature, or completely remove it. That would be a breaking change. It would be for the better, but I still don't want to make it in the v1 module and don't want to make v2 now.
  2. Get more information about the field type using static file parsing with go/types and related packages to generate branchless code: #246
  3. Get more information about the field type from the runtime during program initialization and use it: #249

The latter two approaches turned out to be complicated and time-consuming for custom types, types from other packages, type aliases, etc. So I released v1.5.0, which uses runtime reflection for setting the primary key: #266 It should be changed to approach 2 or 3 in v1.6.0.