Disable "automagic" mapping of tablemap.Version

[sqdk]

Adding a table from a struct containing a member with name "Version" and type other than int results in a panic:
2015/01/27 15:42:57 http: panic serving 127.0.0.1:60027: reflect: call of reflect.Value.Int on string Value goroutine 23 [running]: net/http.func·011() c:/go/src/pkg/net/http/server.go:1100 +0xbe runtime.panic(0x7059e0, 0xc082007420) c:/go/src/pkg/runtime/panic.c:248 +0x1d3 reflect.Value.Int(0x697be0, 0xc082016ca0, 0x0, 0x186, 0x77e3d0) c:/go/src/pkg/reflect/value.go:1053 +0xc3 github.com/coopernurse/gorp.bindPlan.createBindInstance(0xc08205ef00, 0x162, 0xc08201e400, 0x12, 0x20, 0x0, 0x0, 0x0, 0x77e3d0, 0x7, ...) C:/Users/Christian/Google Drive/Development/go/src/github.com/coopernurse/gorp/gorp.go:289 +0x177 github.com/coopernurse/gorp.(*TableMap).bindInsert(0xc082020b40, 0x761280, 0xc082016c60, 0x0, 0x196, 0x0, 0x0, 0x0, 0x0, 0x0, ...) C:/Users/Christian/Google Drive/Development/go/src/github.com/coopernurse/gorp/gorp.go:394 +0x6e0 github.com/coopernurse/gorp.insert(0xc0820104e0, 0x181da0, 0xc0820104e0, 0x3a3a38, 0x1, 0x1, 0x0, 0x0) C:/Users/Christian/Google Drive/Development/go/src/github.com/coopernurse/gorp/gorp.go:1913 +0x2dc github.com/coopernurse/gorp.(*DbMap).Insert(0xc0820104e0, 0x3a3a38, 0x1, 0x1, 0x0, 0x0) C:/Users/Christian/Google Drive/Development/go/src/github.com/coopernurse/gorp/gorp.go:955 +0x97

From my POV, this is convention over configuration, but the wrong way around. If i needed such a feature, i would actively look for it, not assume that it worked automatically.

My proposed solution is to disable setting the version ColMap in AddTable(i interface{}), forcing the developer to manually set the version column name by using SetVersionCol(). This is a crude fix, but it shows the problem and how to solve it.

[GeertJohan]

Won't this break a lot of existing code out there which are expecting the automagic behavoiur?

[GeertJohan]

Maybe we should add a warning message for people expecting the old behavior?

[sqdk]

Sounds like a good idea to warn people. It can be done in a way that only warns if the version column is found. I can make a PR for this and i think the warning should be merged into v1 as it won't break behaviour.

The fix should stay in the unstable branch and merged into v2 when time comes.

[sqdk]

Made a PR to v1: #219

[GeertJohan]

I think readStructColumns should return just one value. So the detection of the Version field can be removed all together. Also please update the README and add the migration guide.

And the PR should be pointed at the master branch, not develop.

[GeertJohan]

Any update on this? We can already merge it in master without affecting v1.

[sqdk]

Should be all set now. All versions except 1.1 passes tests. It fails with an error in the mysql driver. Could be travis using a wrong version of the driver with 1.1.

[sqdk]

Targetting wrong branch. PR will continue in #224

[nd2s]

Stupid question: I just got these deprecation warnings and added SetVersionCol() like so:

db.AddTableWithName(User{}, "users").
        SetKeys(true, "id").
        SetVersionCol("version")

That doesn't get rid of them. How can I stop that? It's a bit annoying...

[sqdk]

Are you on master (github) or on v1 (gopkg)? Master doesn't not output these warnings. V1 will however. There is currently not a way to disable these warnings. If you think this is a necessary feature, you can create an issue, or make a PR with the necessary changes.

Warnings is outputted here: https://github.com/go-gorp/gorp/blob/v1/gorp.go#L780

The warnings gets printed because the automatic mapping happens in AddTableWithName() before you explicitly set it with SetVersionCol(). That was the default behaviour before. The only way to get around this would be to not use AddTableWithName (or any of the helper functions), and i am not sure how easy that is.

[nd2s]

Thanks for the answer. I'm on v1.

In readStructColumns() I don't see an obvious way of checking if user set version col manually so I'll just change to master.

[GeertJohan]

@nd2s note that master is unstable and can break your code! I think an option to disable the warning would be a better option.
Or, (@sqdk), maybe we should fall back to the explicit logger set by (*DbMap).TraceOn(..)...

[nd2s]

I don't really know the code: Maybe it would be possible to check on version column check if SetVersionCol() was used? So that the message is not printed on initialisation but on usage, and only when the problem actually exists.

Only logging with the explicit logger would defeat the purpose, I rarely ever use that. But always logging is bad, libraries shouldn't produce any output when used as intended.

Update: Another possibility would be a "DeprecationWarnings bool" variable a user can set, but that is very ugly and once set the user wouldn't get any more notices. So not really an option.