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

Remove runtime reflection introduced in v1.5 #269

Open
AlekSi opened this issue Dec 8, 2020 · 0 comments
Open

Remove runtime reflection introduced in v1.5 #269

AlekSi opened this issue Dec 8, 2020 · 0 comments
Labels
Milestone

Comments

@AlekSi
Copy link
Member

AlekSi commented Dec 8, 2020

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: Get more type information from file parser #246
  3. Get more information about the field type from the runtime during program initialization and use it: Get more type information from runtime parser #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.

@AlekSi AlekSi added this to the v1.6.0 milestone Dec 8, 2020
@AlekSi AlekSi pinned this issue Dec 8, 2020
@AlekSi AlekSi added the chore label Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant