Implement Connector interface #705

agruetz · 2017-11-13T20:46:52Z

Description

Implemented the connector interface support that is coming out in go 1.10. Moved the connection code in the Open interface to a connectServer method that can be shared while still maintaining the differences between the Open interface that accepts a DSN and the Connect interface that allows for other types of configuration options.

All existing tests are passing. I wanted to get feed back before writing the tests for this. Unsure where/how documentation should change.

fixes #671

Checklist

Code compiles correctly
Created tests which fail without the change (if possible)
All tests passing
Extended the README / documentation, if necessary
Added myself / the copyright holder to the AUTHORS file

methane · 2017-11-14T03:38:21Z

I don't like adding new API to MySQLDriver type.
Please add separated type which wraps config object and adding Open() method.

agruetz · 2017-11-14T06:16:03Z

May I ask why you do not like adding it there? Seems like the most logical place to add it give that the connector interface is part of the driver.

I will be happy to make the change just would like to understand the reasoning.

methane · 2017-11-14T07:14:25Z

MySQLDriver has Open and Connect methods and Cfg member.
And only Connect use Cfg. I feel it's very bad smell.

Adding new Connector type seems better to me.

type Connector {
    Config *Config
}

func (c *Connector) Connect(ctx context.Context) (driver.Conn, error) {
    return connectServer(ctx, c.Config)  // MySQLDriver.Open() can use this function too.
}

func (c *Connector) Driver() driver.Driver {
    return MySQLDriver{}
}

agruetz · 2017-11-22T06:41:13Z

Expect the requested changes around 11/27 - 11/28.

jellevandenhooff · 2017-11-22T21:20:21Z

Hello, thank you for working on this! It would be great if the Connector API threaded through the context object to the connectServer call so that the initial TCP connection can be given a deadline.

agruetz · 2017-12-06T01:26:10Z

Update to address methane concerns.

…Added reasonable testing.

agruetz · 2018-02-21T05:21:41Z

I believe this is ready for review. It is at least ready for feed back. Only thing missing is the usage readme.md however I do not want to write that until I am sure there are no major code changes.

@julienschmidt @arnehormann @methane

methane · 2018-02-21T07:07:31Z

driver.go

+	//Check to see if there was a canelation during creating the connection
+	select {
+	case <-cxt.Done():
+		err = mc.Close()


Checking ctx.Done() after connectServer() is really required?

We do not have to, I debated just doing it once before the connection or just doing it after the connection. Once before is probably the correct/best option. The question really is at what points to do you want to check if there has been a cancelation or deadline hit that this is no longer needed. Ideally you exit as soon as possible as fast as possible. Checking pre-connection is probably adequate.

agruetz · 2018-02-21T07:08:11Z

This email address is no longer used or monitored.

methane · 2018-02-21T07:11:23Z

driver.go

@@ -73,7 +72,7 @@ func (d MySQLDriver) Open(dsn string) (driver.Conn, error) {
 		mc.netConn, err = nd.Dial(mc.cfg.Net, mc.cfg.Addr)


I think we should use nd.DialContext() when context is not nil.

I will look at it. I mis-understood this diff on my previous comment. Seems reasonable enough to thread the context thru however I am not sure the best way to do that off hand as the other entry point sql.Open does carry any context and uses this same connectServer function.

agruetz · 2018-02-22T06:14:53Z

@methane feedback incorporated

methane

LGTM

nemith · 2018-03-17T17:27:57Z

Any chance of landing this?

agruetz · 2018-04-18T10:27:59Z

Best I can tell this is complete for the first attempt. I am sure there will be features to add etc... @methane what do you think about finishing this one up and merging it?

methane · 2018-04-19T09:18:03Z

driver.go

+	errInvalidUser   = errors.New("invalid Connection: User is not set or longer than 32 chars")
+	errInvalidAddr   = errors.New("invalid Connection: Addr config is missing")
+	errInvalidNet    = errors.New("invalid Connection: Only tcp is valid for Net")
+	errInvalidDBName = errors.New("invalid Connection: DBName config is missing")


Maybe, "invalid config" is better than "invalid Connection".

I would concur and will make that change.

methane · 2018-04-19T09:29:03Z

driver.go

+
+type MySQLConnector struct {
+	Cfg *Config
+}


I prefer private type and constructor function.

type mysqlConnector *Config func NewConnector(cfg *Config) (driver.Connector, error) { if err := cfg.normalize(); err != nil { return nil, err } // any additional validation here. return mysqlConnector(cfg) }

Then, we can move validation from Connector.Connect() to cfg.normalize().
User can pass config object created by ParseDSN().

methane · 2018-04-19T09:29:47Z

driver.go

+	//The other optional parameters are not checks
+	//as GO will automatically enforce proper bool types on the options
+	if len(c.Cfg.User) > 32 || len(c.Cfg.User) <= 0 {
+		return nil, errInvalidUser


Is this check really needed?
It seems ParseDSN() doesn't check user name length. Empty user name is impossible?

As far as I am aware an empty username will never authenticate. I guess it is not required press as you would just get a return from the server as a bad connection attempt however it does short cut a connection attempt thus using less resources/connections to the mysql server if the user forgets to provide a username. It also gives a very clear error message as to what is wrong.

methane · 2018-04-19T09:30:08Z

driver.go

+
+	if len(c.Cfg.DBName) <= 0 {
+		return nil, errInvalidDBName
+	}


Empty DB name should be supported.

I concur. I had not thought of the SQL queries that do not require a database. I will correct.

I use it when I create monitoring tools. (e.g. SHOW VARIABLES or SHOW FULL PROCESSLIST)

methane · 2018-04-19T09:30:38Z

driver.go

+
+	if c.Cfg.Net != "tcp" {
+		return nil, errInvalidNet
+	}


agruetz · 2018-04-27T18:05:13Z

@methane just saw all these comments not sure why it said my email address is not monitored. I will review shortly and make appropriate adjustments.

sheeley · 2018-05-17T20:13:13Z

@agruetz would you like a hand with this? We'd like to adopt this for our own RDS IAM connections as well :)

agruetz · 2018-05-18T01:38:11Z

Feel free to. Work has had me to busy to finish. Thanks, Anthony Gruetzmacher

…

Sent from my iPhone

On May 17, 2018, at 1:13 PM, Johnny Sheeley ***@***.***> wrote: @agruetz would you like a hand with this? We'd like to adopt this for our own RDS IAM connections as well :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

dolmen · 2018-06-15T10:08:26Z

#778 looks better (with my suggested changes).

methane · 2018-06-15T11:23:14Z

I prefer connectServer() in this pull request to #778's approach (copy and paste)

nemith · 2018-07-11T18:29:06Z

What needs to happen on this pull request (or #778) to get it shipped?

Go 1.10 has been released for 145 days already.
Mysql driver is broken for less than trivial username password (#668, #591) with this slated as the fix.

Can we get a core developer to comment on the direction on #778 or this pull request and state the direction to go in? Is there anything I can help out with to push this over (implement requested changes, etc)?

agruetz · 2018-07-12T02:24:02Z

+1 on nemith. I apologize for not having gotten back to this sooner. I have been crazy busy with work and the family. I think we probably just need to sync the two PR's and resolve the merge conflicts and get this merged in. We can always make more features/improvements to it as time goes on. That is my 2 cents. Looks like it is slated for the v1.5 release already. Some direction from what the core dev team like to do would be helpful and we will see what we can do to get this wrapped up.

Implement Connector interface go-sql-driver#671

59cb281

methane changed the title ~~Implement Connector interface #671~~ Implement Connector interface Nov 14, 2017

Requested modifications

39714cd

Added support to check the context passed into the connect function. …

ad4a9e9

…Added reasonable testing.

methane reviewed Feb 21, 2018

View reviewed changes

Addresses feedback

bc89eae

small codestyle fix

46e7d3d

methane approved these changes Feb 22, 2018

View reviewed changes

julienschmidt added this to the v1.5 milestone Mar 3, 2018

Merge branch 'master' into master

965ecef

methane mentioned this pull request Mar 28, 2018

Allow dialers and TLS configuration to be added directly to MySQLDriver type #771

Closed

shogo82148 mentioned this pull request Apr 7, 2018

Implement Connector and DriverContext interface #778

Closed

5 tasks

go-sql-driver deleted a comment from agruetz Apr 19, 2018

methane reviewed Apr 19, 2018

View reviewed changes

driver.go

if c.Cfg.Net != "tcp" {

return nil, errInvalidNet

}

Copy link

Member

methane Apr 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No "unix"?

methane mentioned this pull request May 15, 2018

kill queries on timeout #791

Closed

5 tasks

julienschmidt added the enhancement label May 22, 2018

methane mentioned this pull request Sep 5, 2018

PingContext only times out after a second #851

Closed

methane closed this Apr 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Connector interface #705

Implement Connector interface #705

agruetz commented Nov 13, 2017 •

edited

Loading

methane commented Nov 14, 2017

agruetz commented Nov 14, 2017

methane commented Nov 14, 2017

agruetz commented Nov 22, 2017

jellevandenhooff commented Nov 22, 2017

agruetz commented Dec 6, 2017

agruetz commented Feb 21, 2018 •

edited

Loading

methane Feb 21, 2018

agruetz Feb 21, 2018

agruetz commented Feb 21, 2018 via email

methane Feb 21, 2018

agruetz Feb 22, 2018

agruetz commented Feb 22, 2018

methane left a comment

nemith commented Mar 17, 2018

agruetz commented Apr 18, 2018

methane Apr 19, 2018

agruetz Apr 27, 2018

methane Apr 19, 2018 •

edited

Loading

methane Apr 19, 2018

agruetz Apr 27, 2018

methane Apr 19, 2018

agruetz Apr 27, 2018

methane Apr 30, 2018

methane Apr 19, 2018

agruetz commented Apr 27, 2018

sheeley commented May 17, 2018

agruetz commented May 18, 2018 via email

dolmen commented Jun 15, 2018

methane commented Jun 15, 2018

nemith commented Jul 11, 2018

agruetz commented Jul 12, 2018

		@@ -73,7 +72,7 @@ func (d MySQLDriver) Open(dsn string) (driver.Conn, error) {
		mc.netConn, err = nd.Dial(mc.cfg.Net, mc.cfg.Addr)

Implement Connector interface #705

Implement Connector interface #705

Conversation

agruetz commented Nov 13, 2017 • edited Loading

Description

Checklist

methane commented Nov 14, 2017

agruetz commented Nov 14, 2017

methane commented Nov 14, 2017

agruetz commented Nov 22, 2017

jellevandenhooff commented Nov 22, 2017

agruetz commented Dec 6, 2017

agruetz commented Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agruetz commented Feb 21, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agruetz commented Feb 22, 2018

methane left a comment

Choose a reason for hiding this comment

nemith commented Mar 17, 2018

agruetz commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

methane Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agruetz commented Apr 27, 2018

sheeley commented May 17, 2018

agruetz commented May 18, 2018 via email

dolmen commented Jun 15, 2018

methane commented Jun 15, 2018

nemith commented Jul 11, 2018

agruetz commented Jul 12, 2018

agruetz commented Nov 13, 2017 •

edited

Loading

agruetz commented Feb 21, 2018 •

edited

Loading

methane Apr 19, 2018 •

edited

Loading