Support for connection attributes #350
Conversation
|
@julienschmidt I tried to fix everything you suggested in #343 |
|
ProTip™: You can update existing pull-requests by adding commits to the respective branch in your fork |
|
@julienschmidt What work remains before this can be merged? |
|
I am planning on rebasing this against master and to fix the merge conflicts and then will squash all the commits. |
| pktLen := 4 + 4 + 1 + 23 + len(mc.cfg.user) + 1 + 1 + len(scrambleBuff) + 21 + 1 | ||
| pktLen += attrlen + 1 // one byte to store the total length of attrs |
There was a problem hiding this comment.
Skip all this when clientConnectAttrs is missing in server capabilities
|
@julienschmidt any blockers? |
dveeden
force-pushed
the
connattrs_v4
branch
2 times, most recently
from
510077a
to
c8528af
Compare
|
cc @sjmudd |
|
Pushed a new version to get rid of the merge conflict in AUTHORS |
This sets attribute _client_name with the value "Go MySQL Driver" Also sets _os, _platform, _pid and program_name by default. This also decodes the uppper two bytes of the capability flags. The dsn_test.go only tests for one attribute because there is no guaranteed sort order for a map and Printf %+v as used by TestDSNParser().
|
Is there anything preventing this from getting merged? |
|
Are default attrs same to libmysqclient? |
|
LGTM, sorry for the long wait |
| @@ -30,6 +33,10 @@ type MySQLDriver struct{} | |||
| // Custom dial functions must be registered with RegisterDial | |||
| type DialFunc func(addr string) (net.Conn, error) | |||
|
|
|||
| var pid string | |||
| var os_user string | |||
| var os_user_full string | |||
|
Tests are also missing (parser tests are not sufficient). I'm tempted to revert this merge |
| MultiStatements bool // Allow multiple statements in one query | ||
| ParseTime bool // Parse time values to time.Time | ||
| Strict bool // Return warnings as errors | ||
| ConnAttrs map[string]string // Connection Attributes |
There was a problem hiding this comment.
A map[string]string is rather heavy. This could be pre-packed into 1 string
There was a problem hiding this comment.
forget what I said, this should stay user-friendly. The Config struct is now exported
There was a problem hiding this comment.
If we move this to a string than we would have to parse it every time we make a connection. So there is a trade-off.
|
OK. Let's summerize: to get this remerged or fixed we need
|
|
Reverted for now. I think 99% of the users don't care about connections attributes. Should they maybe only be sent when a DSN param is set? |
|
I can see their value, but requiring a DSN arg is fine by me. Do you have a preference for a
|
|
Instead of |
|
|
Is it? RFC study time 😄 |
| data[pos] = byte(attrlen) | ||
| pos++ | ||
|
|
||
| for attrname, attrvalue := range attrs { |
There was a problem hiding this comment.
@dveeden That is what I meant. You could directly pack this into one byte sequence / string when the params are parsed.
But forget that, the Config is exported for a reason now.
|
What would you want in addition to the parser tests I added? |
|
A functional test |
|
And is this backwards-compatible? |
|
@julienschmidt I leave this in your opinionated and very capable hands. RE RFC: I can't find docs saying we have to adhere to an RFC (though we are inspired by one). The unescaped pwd, and RE backwards compatibility: comparison with server flags https://github.com/go-sql-driver/mysql/pull/350/files#diff-2357b8494bbd2f27c09e61fc8ef5f092R260 |
|
I think it's clear that our DSNs are no URLs (unfortunately, that is one historic mistake. Should we CC the letsencrypt guys again?). |
|
RE DSN format - here's what Google does for webfonts: |
This supersedes #343.
This is to support connection attributes. A number of connection attributes like _pid, _client_name are sent by default and it is possible to send custom attributes.
I noticed that user.Current() doesn't work on Fedora 22 with golang, but it does work with gccgo. If this doesn't work then it won't send this attribute to the server.
Edit (js): List of required fixes: #350 (comment)