Add a BeginOption function to DbMap. #429
Conversation
This options allows users to pass sql.TxOptions into their database driver to control transaction isolation if desired.
|
We support If you want to make a PR using modules, you can make your PR against the I really need to add this information to |
db.go
Outdated
| } | ||
|
|
||
| // BeginOption starts a gorp Transaction with sql.TxOptions | ||
| func (m *DbMap) BeginOption(opt *sql.TxOptions) (*Transaction, error) { |
There was a problem hiding this comment.
I think this is a good place for Dave Cheney's functional options (https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis). We unfortunately can't change the signature of Begin without breaking anyone who's using an interface type to represent our DbMap type - but as long as we're adding a new signature, it may make sense to support a variadic option func argument so that we can add other options without breaking backward compatibility in the future.
type optCtx struct {
opts *sql.TxOptions
}
type BeginOpt func(optCtx) optCtx
func TxOptions(opts *sql.TxOptions) BeginOpt {
return func(ctx optCtx) optCtx {
ctx.opts = opts
return ctx
}
}
func (m *DbMap) BeginOpt(options ...BeginOpt) (*Transaction, error) {
var opt optCtx
for _, o := range options {
opt = o(opt)
}
// ... logger stuff ...
tx, err := begin(m, opt.opts)
// etc
}There was a problem hiding this comment.
Do you think keeping optCtx unexported is the way to go? i.e. only gorp can create new options, rather than callers? I don't have a strong argument either way.
There was a problem hiding this comment.
In the example above, I don't believe that exporting the type helps anyone. There are no exported functions that accept an option context (other than the BeginOpt functions themselves), so letting callers construct the type doesn't really do anything - especially if the opts field is left unexported.
My main argument: it's easy export something that was previously unexported (no one can use it, so changing it shouldn't break backward compatibility); it's very hard to unexport something that was previously exported (we'd have to cut a new major version).
So generally, I'm of the opinion that we should start with it unexported; if interest is shown in exporting it, we can always export it then.
|
This will need tests before it can be merged. |
|
What kind of tests would be most useful? We can see whether a tx is created with the right isolation level, but otherwise we should see the same behavior as in the other gorp transaction tests. |
| // Begin starts a gorp Transaction | ||
| func (m *DbMap) Begin() (*Transaction, error) { | ||
| return m.BeginOption(nil) |
There was a problem hiding this comment.
You should be able to just run m.BeginOption() - i.e. no need to pass nil
| if m.ctx != nil { | ||
| return m.Db.BeginTx(m.ctx, nil) | ||
| func begin(m *DbMap, opts ...BeginOpt) (*sql.Tx, error) { | ||
| opt := beginOptions{} |
There was a problem hiding this comment.
I think this reads better as var opt beginOptions
|
Sorry for the long absence - I missed the email requesting a review, and only now thought to check back. It's been a busy several months. For tests: basically just something that calls the method using the Something that would thoroughly vet that this works:
AFAIK at least in postgres, if the transaction is started without the I don't get a whole lot of time to look at gorp, so I'm significantly more likely to accidentally break something if there aren't tests in place. The tests are mostly important so that I won't break you by merging a PR that accidentally breaks a feature. :) |
|
Side note: master has been bumped to a more recent go version so 🤞 if you rebase/merge master into your branch tests will run properly. |
|
any plan to merge this? |
|
not with failing tests |
This options allows users to pass sql.TxOptions into their database
driver to control transaction isolation if desired.
Fixes #428
I had to add go.mod in order to run tests on my fork. I think it's reasonable to support go.mod in the package for other users who import gorp, but I'm happy to remove them or submit them in a separate PR if that's desired.