improved cockroach configuration and fixed bugs on connection details (…

…#271)

* improved cockroach configuration and fixed bugs on connection details

* oops! didn't remove debugging log

* bugfix: blank argument also affect as argument, in secure mode

config_test.go

@@ -10,7 +10,7 @@ func Test_LoadsConnectionsFromConfig(t *testing.T) {
 	r := require.New(t)
 
 	conns := Connections
-	r.Equal(5, len(conns))
+	r.Equal(6, len(conns))
 }
 
 func Test_AddLookupPaths(t *testing.T) {

connection_details.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	_mysql "github.com/go-sql-driver/mysql"
+	"github.com/gobuffalo/pop/logging"
 	"github.com/markbates/going/defaults"
 	"github.com/markbates/oncer"
 	"github.com/pkg/errors"
@@ -39,81 +40,102 @@ type ConnectionDetails struct {
 	// Defaults to 0 "unlimited". See https://golang.org/pkg/database/sql/#DB.SetMaxIdleConns
 	IdlePool int
 	Options  map[string]string
+	// Query string encoded options from URL. Example: "sslmode=disable"
+	RawOptions string
 }
 
-var dialectX = regexp.MustCompile(`\s+://`)
+var dialectX = regexp.MustCompile(`\S+://`)
+
+// overrideWithURL parses and overrides all connection details
+// with values form URL except Dialect.
+func (cd *ConnectionDetails) overrideWithURL() error {
+	ul := cd.URL
+	if cd.Dialect != "" && !dialectX.MatchString(ul) {
+		ul = cd.Dialect + "://" + ul
+	}
+
+	u, err := url.Parse(ul)
+	if err != nil {
+		return errors.Wrapf(err, "couldn't parse %s", ul)
+	}
+	//! dialect should not be overrided here (especially for cockroach)
+	if cd.Dialect == "" {
+		cd.Dialect = u.Scheme
+	}
+	// warning message is required to prevent confusion
+	// even though this behavior was documented.
+	if cd.Database+cd.Host+cd.Port+cd.User+cd.Password != "" {
+		log(logging.Warn, "One or more of connection parameters are specified in database.yml. Override them with values in URL.")
+	}
+
+	if strings.HasPrefix(cd.Dialect, "sqlite") {
+		cd.Database = u.Path
+		return nil
+	} else if strings.HasPrefix(cd.Dialect, "mysql") {
+		return cd.overrideWithMySQLURL()
+	}
+
+	cd.Database = strings.TrimPrefix(u.Path, "/")
+
+	hp := strings.Split(u.Host, ":")
+	cd.Host = hp[0]
+	if len(hp) > 1 {
+		cd.Port = hp[1]
+	}
+
+	if u.User != nil {
+		cd.User = u.User.Username()
+		cd.Password, _ = u.User.Password()
+	}
+	cd.RawOptions = u.RawQuery
+
+	return nil
+}
+
+func (cd *ConnectionDetails) overrideWithMySQLURL() error {
+	// parse and verify whether URL is supported by underlying driver or not.
+	cfg, err := _mysql.ParseDSN(strings.TrimPrefix(cd.URL, "mysql://"))
+	if err != nil {
+		return errors.Errorf("The URL '%s' is not supported by MySQL driver.", cd.URL)
+	}
+
+	cd.User = cfg.User
+	cd.Password = cfg.Passwd
+	cd.Database = cfg.DBName
+	cd.Encoding = defaults.String(cfg.Collation, "utf8_general_ci")
+	addr := strings.TrimSuffix(strings.TrimPrefix(cfg.Addr, "("), ")")
+	if cfg.Net == "unix" {
+		cd.Port = "socket"
+		cd.Host = addr
+	} else {
+		tmp := strings.Split(addr, ":")
+		cd.Host = tmp[0]
+		if len(tmp) > 1 {
+			cd.Port = tmp[1]
+		}
+	}
+	return nil
+}
 
 // Finalize cleans up the connection details by normalizing names,
 // filling in default values, etc...
 func (cd *ConnectionDetails) Finalize() error {
 	if cd.URL != "" {
-		ul := cd.URL
-		if cd.Dialect != "" {
-			if !dialectX.MatchString(ul) {
-				ul = cd.Dialect + "://" + ul
-			}
-		}
-		cd.Database = cd.URL
-		if !strings.HasPrefix(cd.Dialect, "sqlite") {
-			u, err := url.Parse(ul)
-			if err != nil {
-				return errors.Wrapf(err, "couldn't parse %s", ul)
-			}
-			cd.Dialect = u.Scheme
-			cd.Database = u.Path
-
-			hp := strings.Split(u.Host, ":")
-			cd.Host = hp[0]
-			if len(hp) > 1 {
-				cd.Port = hp[1]
-			}
-
-			if u.User != nil {
-				cd.User = u.User.Username()
-				cd.Password, _ = u.User.Password()
-			}
+		if err := cd.overrideWithURL(); err != nil {
+			return err
 		}
-
 	}
+
+	// then fill with default values
 	switch strings.ToLower(cd.Dialect) {
 	case "postgres", "postgresql", "pg":
 		cd.Dialect = "postgres"
 		cd.Port = defaults.String(cd.Port, "5432")
-		cd.Database = strings.TrimPrefix(cd.Database, "/")
 	case "cockroach", "cockroachdb", "crdb":
 		cd.Dialect = "cockroach"
 		cd.Port = defaults.String(cd.Port, "26257")
-		cd.Database = strings.TrimPrefix(cd.Database, "/")
 	case "mysql":
-		// parse and verify whether URL is supported by underlying driver or not.
-		if cd.URL != "" {
-			cfg, err := _mysql.ParseDSN(strings.TrimPrefix(cd.URL, "mysql://"))
-			if err != nil {
-				return errors.Errorf("The URL is not supported by MySQL driver.")
-			}
-			cd.User = cfg.User
-			cd.Password = cfg.Passwd
-			cd.Database = cfg.DBName
-			cd.Encoding = defaults.String(cfg.Collation, "utf8_general_ci")
-			addr := strings.TrimSuffix(strings.TrimPrefix(cfg.Addr, "("), ")")
-			if cfg.Net == "unix" {
-				cd.Port = "socket"
-				cd.Host = addr
-			} else {
-				tmp := strings.Split(addr, ":")
-				switch len(tmp) {
-				case 1:
-					cd.Host = tmp[0]
-					cd.Port = "3306"
-				case 2:
-					cd.Host = tmp[0]
-					cd.Port = tmp[1]
-				}
-			}
-		} else {
-			cd.Port = defaults.String(cd.Port, "3306")
-			cd.Database = strings.TrimPrefix(cd.Database, "/")
-		}
+		cd.Port = defaults.String(cd.Port, "3306")
 	case "sqlite", "sqlite3":
 		cd.Dialect = "sqlite3"
 	default:

connection_details_test.go

@@ -23,6 +23,24 @@ func Test_ConnectionDetails_Finalize(t *testing.T) {
 	r.Equal(cd.User, "user")
 }
 
+func Test_ConnectionDetails_Finalize_Cockroach(t *testing.T) {
+	r := require.New(t)
+
+	cd := &ConnectionDetails{
+		Dialect: "cockroach",
+		URL:     "postgres://user:pass@host:port/database?sslmode=require&sslrootcert=certs/ca.crt&sslkey=certs/client.key&sslcert=certs/client.crt",
+	}
+	err := cd.Finalize()
+	r.NoError(err)
+
+	r.Equal("cockroach", cd.Dialect)
+	r.Equal("database", cd.Database)
+	r.Equal("host", cd.Host)
+	r.Equal("port", cd.Port)
+	r.Equal("user", cd.User)
+	r.Equal("pass", cd.Password)
+}
+
 func Test_ConnectionDetails_Finalize_MySQL_DSN(t *testing.T) {
 	r := require.New(t)
 
@@ -132,3 +150,39 @@ func Test_ConnectionDetails_Finalize_SQLite(t *testing.T) {
 	r.Equal(cd.Port, "")
 	r.Equal(cd.User, "")
 }
+
+func Test_ConnectionDetails_Finalize_SQLite_with_Dialect(t *testing.T) {
+	r := require.New(t)
+
+	cd := &ConnectionDetails{
+		Dialect: "sqlite",
+		URL:     "sqlite3:///tmp/foo.db",
+	}
+	err := cd.Finalize()
+	r.NoError(err)
+
+	r.Equal("/tmp/foo.db", cd.Database)
+	r.Equal("sqlite3", cd.Dialect)
+	r.Equal("", cd.Host)
+	r.Equal("", cd.Password)
+	r.Equal("", cd.Port)
+	r.Equal("", cd.User)
+}
+
+func Test_ConnectionDetails_Finalize_SQLite_without_URL(t *testing.T) {
+	r := require.New(t)
+
+	cd := &ConnectionDetails{
+		Dialect:  "sqlite",
+		Database: "./foo.db",
+	}
+	err := cd.Finalize()
+	r.NoError(err)
+
+	r.Equal("./foo.db", cd.Database)
+	r.Equal("sqlite3", cd.Dialect)
+	r.Equal("", cd.Host)
+	r.Equal("", cd.Password)
+	r.Equal("", cd.Port)
+	r.Equal("", cd.User)
+}

database.yml

@@ -29,6 +29,10 @@ cockroach:
   user: {{ envOr "COCKROACH_USER" "root"  }}
   password: {{ envOr "COCKROACH_PASSWORD" ""  }}
 
+cockroach_ssl:
+  dialect: "cockroach"
+  url: "postgres://root@localhost:26257/pop_test?sslmode=require&sslrootcert=./certs/ca.crt&sslkey=./certs/client.root.key&sslcert=./certs/client.root.crt"
+
 sqlite:
   dialect: "sqlite3"
   database: "./sql_scripts/sqlite/test.sqlite"

dialect_cockroach.go

@@ -135,18 +135,30 @@ func (p *cockroach) URL() string {
 	if c.URL != "" {
 		return c.URL
 	}
-	ssl := defaults.String(c.Options["sslmode"], "disable")
-
-	s := "postgres://%s:%s@%s:%s/%s?application_name=cockroach&sslmode=%s"
-	return fmt.Sprintf(s, c.User, c.Password, c.Host, c.Port, c.Database, ssl)
+	s := "postgres://%s:%s@%s:%s/%s?%s"
+	return fmt.Sprintf(s, c.User, c.Password, c.Host, c.Port, c.Database, p.optionString())
 }
 
 func (p *cockroach) urlWithoutDb() string {
 	c := p.ConnectionDetails
-	ssl := defaults.String(c.Options["sslmode"], "disable")
+	s := "postgres://%s:%s@%s:%s/?%s"
+	return fmt.Sprintf(s, c.User, c.Password, c.Host, c.Port, p.optionString())
+}
 
-	s := "postgres://%s:%s@%s:%s/?application_name=cockroach&sslmode=%s"
-	return fmt.Sprintf(s, c.User, c.Password, c.Host, c.Port, ssl)
+func (p *cockroach) optionString() string {
+	c := p.ConnectionDetails
+
+	if c.RawOptions != "" {
+		return c.RawOptions
+	}
+
+	s := "application_name=cockroach"
+	if c.Options != nil {
+		for k := range c.Options {
+			s = fmt.Sprintf("%s&%s=%s", s, k, c.Options[k])
+		}
+	}
+	return s
 }
 
 func (p *cockroach) MigrationURL() string {
@@ -175,12 +187,12 @@ func (p *cockroach) Lock(fn func() error) error {
 }
 
 func (p *cockroach) DumpSchema(w io.Writer) error {
-	secure := ""
+	cmd := exec.Command("cockroach", "dump", p.Details().Database, "--dump-mode=schema")
+
 	c := p.ConnectionDetails
 	if defaults.String(c.Options["sslmode"], "disable") == "disable" {
-		secure = "--insecure"
+		cmd.Args = append(cmd.Args, "--insecure")
 	}
-	cmd := exec.Command("cockroach", "dump", p.Details().Database, "--dump-mode=schema", secure)
 	log(logging.SQL, strings.Join(cmd.Args, " "))
 	cmd.Stdout = w
 	cmd.Stderr = os.Stderr

dialect_cockroach_test.go

@@ -0,0 +1,44 @@
+package pop
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func Test_Cockroach_URL_Raw(t *testing.T) {
+	r := require.New(t)
+
+	cd := &ConnectionDetails{
+		Dialect: "cockroach",
+		URL:     "scheme://user:pass@host:port/database?option1=value1",
+	}
+	err := cd.Finalize()
+	r.NoError(err)
+
+	m := &cockroach{ConnectionDetails: cd}
+	r.Equal("scheme://user:pass@host:port/database?option1=value1", m.URL())
+	r.Equal("postgres://user:pass@host:port/?option1=value1", m.urlWithoutDb())
+}
+
+func Test_Cockroach_URL_Build(t *testing.T) {
+	r := require.New(t)
+
+	cd := &ConnectionDetails{
+		Dialect:  "cockroach",
+		Database: "database",
+		Host:     "host",
+		Port:     "port",
+		User:     "user",
+		Password: "pass",
+		Options: map[string]string{
+			"option1": "value1",
+		},
+	}
+	err := cd.Finalize()
+	r.NoError(err)
+
+	m := &cockroach{ConnectionDetails: cd}
+	r.Equal("postgres://user:pass@host:port/database?application_name=cockroach&option1=value1", m.URL())
+	r.Equal("postgres://user:pass@host:port/?application_name=cockroach&option1=value1", m.urlWithoutDb())
+}