sa: use mysql.Config to set flags on connect strings

riking · riking · commit f44038033082 · 2016-02-29T15:38:43.000-08:00
This also exposes an interface to open a connection using a mysql.Config, so we can start changing config files to use those. Part of letsencrypt#1505
diff --git a/sa/database.go b/sa/database.go
@@ -11,30 +11,50 @@ import (
 	"net/url"
 	"strings"
 
-	// Provide access to the MySQL driver
-	_ "github.com/letsencrypt/boulder/Godeps/_workspace/src/github.com/go-sql-driver/mysql"
+	"github.com/letsencrypt/boulder/Godeps/_workspace/src/github.com/go-sql-driver/mysql"
 	gorp "github.com/letsencrypt/boulder/Godeps/_workspace/src/gopkg.in/gorp.v1"
 	"github.com/letsencrypt/boulder/core"
 	blog "github.com/letsencrypt/boulder/log"
 )
 
 // NewDbMap creates the root gorp mapping object. Create one of these for each
-// database schema you wish to map. Each DbMap contains a list of mapped tables.
-// It automatically maps the tables for the primary parts of Boulder around the
-// Storage Authority. This may require some further work when we use a disjoint
-// schema, like that for `certificate-authority-data.go`.
+// database schema you wish to map. Each DbMap contains a list of mapped
+// tables. It automatically maps the tables for the primary parts of Boulder
+// around the Storage Authority.
 func NewDbMap(dbConnect string) (*gorp.DbMap, error) {
-	logger := blog.GetAuditLogger()
-
 	var err error
-	dbConnect, err = recombineURLForDB(dbConnect)
+	var config *mysql.Config
+	if strings.HasPrefix(dbConnect, "mysql+tcp://") {
+		dbConnect, err = recombineCustomMySQLURL(dbConnect)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	config, err = mysql.ParseDSN(dbConnect)
 	if err != nil {
 		return nil, err
 	}
 
+	return NewDbMapFromConfig(config)
+}
+
+// sqlOpen is used in the tests to check that the arguments are properly
+// transformed
+var sqlOpen = func(dbType, connectStr string) (*sql.DB, error) {
+	return sql.Open(dbType, connectStr)
+}
+
+// NewDbMapFromConfig functions similarly to NewDbMap, but it takes the
+// decomposed form of the connection string, a *mysql.Config.
+func NewDbMapFromConfig(config *mysql.Config) (*gorp.DbMap, error) {
+	adjustMySQLConfig(config)
+
+	logger := blog.GetAuditLogger()
+
 	logger.Debug("Connecting to database")
 
-	db, err := sql.Open("mysql", dbConnect)
+	db, err := sqlOpen("mysql", config.FormatDSN())
 	if err != nil {
 		return nil, err
 	}
@@ -52,18 +72,45 @@ func NewDbMap(dbConnect string) (*gorp.DbMap, error) {
 	return dbmap, err
 }
 
-// recombineURLForDB transforms a database URL to a URL-like string
-// that the mysql driver can use. The mysql driver needs the Host data
-// to be wrapped in "tcp()" but url.Parse will escape the parentheses
-// and the mysql driver doesn't understand them. So, we can't have
-// "tcp()" in the configs, but can't leave it out before passing it to
-// the mysql driver. Similarly, the driver needs the password and
-// username unescaped. Compromise by doing the leg work if the config
-// says the database URL's scheme is a fake one called
-// "mysql+tcp://". See
-// https://github.com/go-sql-driver/mysql/issues/362 for why we have
-// to futz around and avoid URL.String.
-func recombineURLForDB(dbConnect string) (string, error) {
+// adjustMySQLConfig sets certain flags that we want on every connection.
+func adjustMySQLConfig(conf *mysql.Config) *mysql.Config {
+	// Required to turn DATETIME fields into time.Time
+	conf.ParseTime = true
+
+	// Required to make UPDATE return the number of rows matched,
+	// instead of the number of rows changed by the UPDATE.
+	conf.ClientFoundRows = true
+
+	// Ensures that MySQL/MariaDB warnings are treated as errors. This
+	// avoids a number of nasty edge conditions we could wander into.
+	// Common things this discovers includes places where data being sent
+	// had a different type than what is in the schema, strings being
+	// truncated, writing null to a NOT NULL column, and so on. See
+	// <https://dev.mysql.com/doc/refman/5.0/en/sql-mode.html#sql-mode-strict>.
+	conf.Strict = true
+
+	return conf
+}
+
+// recombineCustomMySQLURL transforms the legacy database URLs into the
+// URL-like strings expected by the mysql database driver.
+//
+// In the past, changes to the connection string were achieved by passing it
+// into url.Parse and editing the query string that way, so the string had to
+// be a valid URL. The mysql driver needs the Host data to be wrapped in
+// "tcp()" but url.Parse will escape the parentheses and the mysql driver
+// doesn't understand them. So, we couldn't have "tcp()" in the configs, but
+// couldn't leave it out before passing it to the mysql driver.  Similarly, the
+// driver needs the password and username unescaped. The compromise was to do
+// the leg work if the connection string's scheme is a fake one called
+// "mysql+tcp://".
+//
+// Upon the addition of
+// https://godoc.org/github.com/go-sql-driver/mysql#Config, this was no longer
+// necessary, as the changes could be made on the decomposed struct version of
+// the connection url. This method converts the old format into the format
+// expected by the library.
+func recombineCustomMySQLURL(dbConnect string) (string, error) {
 	dbConnect = strings.TrimSpace(dbConnect)
 	dbURL, err := url.Parse(dbConnect)
 	if err != nil {
@@ -75,26 +122,6 @@ func recombineURLForDB(dbConnect string) (string, error) {
 		return "", fmt.Errorf(format, dbURL.Scheme)
 	}
 
-	dsnVals, err := url.ParseQuery(dbURL.RawQuery)
-	if err != nil {
-		return "", err
-	}
-
-	dsnVals.Set("parseTime", "true")
-
-	// Required to make UPDATE return the number of rows matched,
-	// instead of the number of rows changed by the UPDATE.
-	dsnVals.Set("clientFoundRows", "true")
-
-	// Ensures that MySQL/MariaDB warnings are treated as errors. This
-	// avoids a number of nasty edge conditions we could wander
-	// into. Common things this discovers includes places where data
-	// being sent had a different type than what is in the schema,
-	// strings being truncated, writing null to a NOT NULL column, and
-	// so on. See
-	// <https://dev.mysql.com/doc/refman/5.0/en/sql-mode.html#sql-mode-strict>.
-	dsnVals.Set("strict", "true")
-
 	user := dbURL.User.Username()
 	passwd, hasPass := dbURL.User.Password()
 	dbConn := ""
@@ -105,7 +132,7 @@ func recombineURLForDB(dbConnect string) (string, error) {
 		dbConn += ":" + passwd
 	}
 	dbConn += "@tcp(" + dbURL.Host + ")"
-	return dbConn + dbURL.EscapedPath() + "?" + dsnVals.Encode(), nil
+	return dbConn + dbURL.EscapedPath() + "?" + dbURL.RawQuery, nil
 }
 
 // SetSQLDebug enables/disables GORP SQL-level Debugging
diff --git a/sa/database_test.go b/sa/database_test.go
@@ -6,6 +6,8 @@
 package sa
 
 import (
+	"database/sql"
+	"errors"
 	"testing"
 
 	"github.com/letsencrypt/boulder/test"
@@ -15,3 +17,30 @@ func TestInvalidDSN(t *testing.T) {
 	_, err := NewDbMap("invalid")
 	test.AssertError(t, err, "DB connect string missing the slash separating the database name")
 }
+
+var errExpected = errors.New("expected")
+
+func TestNewDbMap(t *testing.T) {
+	const mysqlConnectUrl = "mysql+tcp://policy:password@localhost:3306/boulder_policy_integration?readTimeout=800ms&writeTimeout=800ms"
+	const expectedTransformed = "policy:password@tcp(localhost:3306)/boulder_policy_integration?clientFoundRows=true&parseTime=true&readTimeout=800ms&strict=true&writeTimeout=800ms"
+
+	oldSqlOpen := sqlOpen
+	defer func() {
+		sqlOpen = oldSqlOpen
+	}()
+	sqlOpen = func(dbType, connectString string) (*sql.DB, error) {
+		if connectString != expectedTransformed {
+			t.Errorf("incorrect connection string mangling, got %v", connectString)
+		}
+		return nil, errExpected
+	}
+
+	dbMap, err := NewDbMap(mysqlConnectUrl)
+	if err != errExpected {
+		t.Errorf("got incorrect error: %v", err)
+	}
+	if dbMap != nil {
+		t.Errorf("expected nil, got %v", dbMap)
+	}
+
+}
