Add a BeginOption function to DbMap.

[joeblubaugh]

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.

[nelsam]

We support go.mod on versioned branches, but leave master modules-free for anyone who wants to avoid the go modules mess. Frankly, I use go modules at work, but I firmly believe that they are far more buggy and difficult to use than they are worth - hence the desire to support people who aren't using them.

If you want to make a PR using modules, you can make your PR against the v3 branch and I'll do all the cherry-pick/merge magic on my end (warning: this will likely clobber author information); otherwise, please remove go.mod from this PR and I'll merge it into v3 after it passes on master.

I really need to add this information to CONTRIBUTING.md.

[nelsam]

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
}

[joeblubaugh]

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.

[nelsam]

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.

[nelsam]

This will need tests before it can be merged.

[joeblubaugh]

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.

[nelsam]

You should be able to just run m.BeginOption() - i.e. no need to pass nil

[nelsam]

I think this reads better as var opt beginOptions

[nelsam]

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 TxOptions() as an argument, to ensure that the method exists and the return value of TxOptions is a valid parameter to the method.

Something that would thoroughly vet that this works:

1. Open a transaction with a REPEATABLE READ isolation level.
2. Outside of the transaction (i.e. on the db itself), insert a row.
3. Inside the transaction, try to select the row.
4. Confirm that the row does not exist inside the transaction.

AFAIK at least in postgres, if the transaction is started without the REPEATABLE READ isolation level, then it would see the new row.

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. :)

[nelsam]

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.

[yangshengBE]

any plan to merge this?

[nelsam]

not with failing tests