Support for connection attributes#350

dveeden · 2015-06-21T12:25:51Z

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)

dveeden · 2015-06-21T12:28:40Z

@julienschmidt I tried to fix everything you suggested in #343

julienschmidt · 2015-06-21T15:00:20Z

ProTip™: You can update existing pull-requests by adding commits to the respective branch in your fork

dgryski · 2016-02-25T16:12:21Z

@julienschmidt What work remains before this can be merged?

dgryski · 2016-02-26T09:08:47Z

I am planning on rebasing this against master and to fix the merge conflicts and then will squash all the commits.

arnehormann · 2016-02-26T10:10:06Z

packets.go

+ }
+
 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


Skip all this when clientConnectAttrs is missing in server capabilities

arnehormann · 2016-02-26T10:22:17Z

@julienschmidt any blockers?

dveeden · 2016-03-15T13:11:14Z

cc @sjmudd

dveeden · 2016-03-15T13:15:28Z

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

dveeden · 2016-03-29T07:50:40Z

Is there anything preventing this from getting merged?

methane · 2016-03-29T08:29:42Z

Are default attrs same to libmysqclient?
If so, LGTM.

arnehormann · 2016-03-29T08:36:34Z

LGTM, sorry for the long wait

julienschmidt · 2016-03-29T08:52:34Z

driver.go


+varpidstring
+varos_userstring
+varos_user_fullstring


Wrong naming convention

Filed #442 for this lint issue.

julienschmidt · 2016-03-29T08:57:19Z

Tests are also missing (parser tests are not sufficient). I'm tempted to revert this merge

julienschmidt · 2016-03-29T09:00:54Z

dsn.go

+MultiStatementsbool// Allow multiple statements in one query
+ParseTimebool// Parse time values to time.Time
+Strictbool// Return warnings as errors
+ConnAttrsmap[string]string// Connection Attributes


A map[string]string is rather heavy. This could be pre-packed into 1 string

forget what I said, this should stay user-friendly. The Config struct is now exported

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.

arnehormann · 2016-03-29T09:03:35Z

OK. Let's summerize: to get this remerged or fixed we need

an additional test (finding the connection attrs in a query)
a paren-less syntax - comma separated?
conforming names
cached attributes

This reverts commit 2625e19, reversing changes made to 66312f7.

julienschmidt · 2016-03-29T09:08:36Z

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?
cannAttrs=true to set defaults, connAttrs=whatever to add custom ones?

arnehormann · 2016-03-29T09:16:02Z

I can see their value, but requiring a DSN arg is fine by me.

Do you have a preference for a whatever flavor?

separator connAttrs=key1=value1,key2=value2
multiple params connAttrs=key1=value1&connAttrs=key2=value2

dveeden · 2016-03-29T09:22:35Z

Instead of connattrs=(foo=bar,foo2=bar2) we could do connattrs=foo:bar,foo2:bar2
Would that be ok?

arnehormann · 2016-03-29T09:26:59Z

= is ok - as long as there's no &. Just split on , and then on =.

julienschmidt · 2016-03-29T09:28:43Z

= is ok - as long as there's no &.

Is it? RFC study time 😄

julienschmidt · 2016-03-29T09:31:37Z

packets.go

+data[pos] =byte(attrlen)
+pos++
+
+forattrname, attrvalue:=rangeattrs{


@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.

dveeden · 2016-03-29T09:34:01Z

What would you want in addition to the parser tests I added?

julienschmidt · 2016-03-29T09:34:57Z

A functional test

julienschmidt · 2016-03-29T09:39:39Z

And is this backwards-compatible?
It seems like connection attributes were only added in MySQL 5.6

arnehormann · 2016-03-29T09:45:51Z

@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 tcp(...) would also be a problem for a URL.

RE backwards compatibility: comparison with server flags https://github.com/go-sql-driver/mysql/pull/350/files#diff-2357b8494bbd2f27c09e61fc8ef5f092R260

julienschmidt · 2016-03-29T09:49:52Z

I think it's clear that our DSNs are no URLs (unfortunately, that is one historic mistake. Should we CC the letsencrypt guys again?).
But let's adhere to URI RFCs as much as possible and don't reinvent the wheel (any further).

arnehormann · 2016-03-30T11:48:16Z

RE DSN format - here's what Google does for webfonts:
key1:value1|key2:value1,value2
https://fonts.googleapis.com/css?family=Open+Sans:400,600italic,700italic|Roboto:400,500

julienschmidt added this to the v1.3 milestone Jul 24, 2015

julienschmidt added the enhancement label Jul 24, 2015

julienschmidt modified the milestones: v1.4, v1.3 Jan 13, 2016

arnehormann reviewed Feb 26, 2016
View reviewed changes

dveeden force-pushed the connattrs_v4 branch 2 times, most recently from 510077a to c8528afCompare March 6, 2016 22:09

dveeden force-pushed the connattrs_v4 branch from c8528af to 2d90a15Compare March 15, 2016 13:14

dveeden force-pushed the connattrs_v4 branch from 2d90a15 to 82cddefCompare March 15, 2016 14:01

arnehormann merged commit 2625e19 into go-sql-driver:masterMar 29, 2016

julienschmidt reviewed Mar 29, 2016
View reviewed changes

julienschmidt added a commit that referenced this pull request Mar 29, 2016
Revert "Merge pull request#350from dveeden/connattrs_v4"
1421caf
This reverts commit 2625e19, reversing changes made to 66312f7.

arnehormann assigned julienschmidt Mar 29, 2016

julienschmidt reviewed Mar 29, 2016
View reviewed changes

julienschmidt modified the milestones: v1.3, v1.4Mar 29, 2016

methane mentioned this pull request Mar 8, 2019
Set program_name on connectionString or somewhere else #930
Closed

Daemonxiao mentioned this pull request Jan 31, 2023
Add connection attributes #1389
Merged
5 tasks

Support for connection attributes#350

Support for connection attributes #350

Uh oh!

Conversation

dveeden commented Jun 21, 2015

Uh oh!

dveeden commented Jun 21, 2015

Uh oh!

julienschmidt commented Jun 21, 2015

Uh oh!

dgryski commented Feb 25, 2016

Uh oh!

dgryski commented Feb 26, 2016

Uh oh!

arnehormannFeb 26, 2016

Choose a reason for hiding this comment

Uh oh!

arnehormann commented Feb 26, 2016

Uh oh!

dveeden commented Mar 15, 2016

Uh oh!

dveeden commented Mar 15, 2016

Uh oh!

dveeden commented Mar 29, 2016

Uh oh!

methane commented Mar 29, 2016

Uh oh!

arnehormann commented Mar 29, 2016

Uh oh!

julienschmidtMar 29, 2016

Choose a reason for hiding this comment

Uh oh!

dgryskiMar 29, 2016

Choose a reason for hiding this comment

Uh oh!

julienschmidt commented Mar 29, 2016

Uh oh!

julienschmidtMar 29, 2016

Choose a reason for hiding this comment

Uh oh!

julienschmidtMar 29, 2016

Choose a reason for hiding this comment

Uh oh!

dveedenMar 29, 2016

Choose a reason for hiding this comment

Uh oh!

arnehormann commented Mar 29, 2016

Uh oh!

julienschmidt commented Mar 29, 2016

Uh oh!

arnehormann commented Mar 29, 2016

Uh oh!

dveeden commented Mar 29, 2016

Uh oh!

arnehormann commented Mar 29, 2016

Uh oh!

julienschmidt commented Mar 29, 2016

Uh oh!

julienschmidtMar 29, 2016

Choose a reason for hiding this comment

Uh oh!

dveeden commented Mar 29, 2016

Uh oh!

julienschmidt commented Mar 29, 2016

Uh oh!

julienschmidt commented Mar 29, 2016

Uh oh!

arnehormann commented Mar 29, 2016

Uh oh!

julienschmidt commented Mar 29, 2016

Uh oh!

arnehormann commented Mar 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!