Repository: denisenkom/go-mssqldb
Pull request #543 (2019-11-22 16:43:43)

Fixing instance resolution if both instance and port are set in connection string.
Existing implementation sets default port to 1433 right during connection string parsing. I have removed setting to default port in order for being able to determine if the port was specified.

Now, instance resolution takes place only if the default port is not set. At each place connection parameter's port is used a port resolution is applied. If it is 0 then default port is applied.

P.S. Didn't know why SPN is currently required. Didn't find where it is used.

codecov[bot]: # [Codecov](https://codecov.io/gh/denisenkom/go-mssqldb/pull/543?src=pr&el=h1) Report
> Merging [#543](https://codecov.io/gh/denisenkom/go-mssqldb/pull/543?src=pr&el=desc) into [master](https://codecov.io/gh/denisenkom/go-mssqldb/commit/cfbb681360f0a7de54ae77703318f0e60d422e00?src=pr&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `66.66%`.

[![Impacted file tree graph](https://codecov.io/gh/denisenkom/go-mssqldb/pull/543/graphs/tree.svg?width=650&token=MoMB2u5avp&height=150&src=pr)](https://codecov.io/gh/denisenkom/go-mssqldb/pull/543?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #543      +/-   ##
==========================================
+ Coverage   68.86%   68.89%   +0.02%     
==========================================
  Files          22       22              
  Lines        5058     5066       +8     
==========================================
+ Hits         3483     3490       +7     
- Misses       1369     1370       +1     
  Partials      206      206
```


| [Impacted Files](https://codecov.io/gh/denisenkom/go-mssqldb/pull/543?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [conn\_str.go](https://codecov.io/gh/denisenkom/go-mssqldb/pull/543/diff?src=pr&el=tree#diff-Y29ubl9zdHIuZ28=) | `100% <100%> (ø)` | :arrow_up: |
| [tds.go](https://codecov.io/gh/denisenkom/go-mssqldb/pull/543/diff?src=pr&el=tree#diff-dGRzLmdv) | `60.33% <16.66%> (-0.11%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/denisenkom/go-mssqldb/pull/543?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/denisenkom/go-mssqldb/pull/543?src=pr&el=footer). Last update [cfbb681...272ccdb](https://codecov.io/gh/denisenkom/go-mssqldb/pull/543?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/conn_str.go b/conn_str.go
index 0742755..4ff54b8 100644
--- a/conn_str.go
+++ b/conn_str.go
@@ -11,6 +11,8 @@ import (
 	"unicode"
 )
 
+const defaultServerPort = 1433
+
 type connectParams struct {
 	logFlags                  uint64
 	port                      uint64
@@ -78,7 +80,7 @@ func parseConnectParams(dsn string) (connectParams, error) {
 	p.user = params["user id"]
 	p.password = params["password"]
 
-	p.port = 1433
+	p.port = 0
 	strport, ok := params["port"]
 	if ok {
 		var err error
@@ -183,7 +185,7 @@ func parseConnectParams(dsn string) (connectParams, error) {
 	if ok {
 		p.serverSPN = serverSPN
 	} else {
-		p.serverSPN = fmt.Sprintf("MSSQLSvc/%s:%d", p.host, p.port)
+		p.serverSPN = generateSpn(p.host, resolveServerPort(p.port))
 	}
 
 	workstation, ok := params["workstation id"]
@@ -454,3 +456,15 @@ func splitConnectionStringOdbc(dsn string) (map[string]string, error) {
 func normalizeOdbcKey(s string) string {
 	return strings.ToLower(strings.TrimRightFunc(s, unicode.IsSpace))
 }
+
+func resolveServerPort(port uint64) uint64 {
+	if port == 0 {
+		return defaultServerPort
+	}
+
+	return port
+}
+
+func generateSpn(host string, port uint64) string {
+	return fmt.Sprintf("MSSQLSvc/%s:%d", host, port)
+}
diff --git a/tds.go b/tds.go
index 5a9f53b..9419836 100644
--- a/tds.go
+++ b/tds.go
@@ -666,14 +666,14 @@ func dialConnection(ctx context.Context, c *Connector, p connectParams) (conn ne
 	}
 	if len(ips) == 1 {
 		d := c.getDialer(&p)
-		addr := net.JoinHostPort(ips[0].String(), strconv.Itoa(int(p.port)))
+		addr := net.JoinHostPort(ips[0].String(), strconv.Itoa(int(resolveServerPort(p.port))))
 		conn, err = d.DialContext(ctx, "tcp", addr)
 
 	} else {
 		//Try Dials in parallel to avoid waiting for timeouts.
 		connChan := make(chan net.Conn, len(ips))
 		errChan := make(chan error, len(ips))
-		portStr := strconv.Itoa(int(p.port))
+		portStr := strconv.Itoa(int(resolveServerPort(p.port)))
 		for _, ip := range ips {
 			go func(ip net.IP) {
 				d := c.getDialer(&p)
@@ -711,7 +711,7 @@ func dialConnection(ctx context.Context, c *Connector, p connectParams) (conn ne
 	// Can't do the usual err != nil check, as it is possible to have gotten an error before a successful connection
 	if conn == nil {
 		f := "Unable to open tcp connection with host '%v:%v': %v"
-		return nil, fmt.Errorf(f, p.host, p.port, err.Error())
+		return nil, fmt.Errorf(f, p.host, resolveServerPort(p.port), err.Error())
 	}
 	return conn, err
 }
@@ -724,7 +724,7 @@ func connect(ctx context.Context, c *Connector, log optionalLogger, p connectPar
 		defer cancel()
 	}
 	// if instance is specified use instance resolution service
-	if p.instance != "" {
+	if p.instance != "" && p.port == 0 {
 		p.instance = strings.ToUpper(p.instance)
 		d := c.getDialer(&p)
 		instances, err := getInstances(dialCtx, d, p.host)
@@ -737,11 +737,12 @@ func connect(ctx context.Context, c *Connector, log optionalLogger, p connectPar
 			f := "No instance matching '%v' returned from host '%v'"
 			return nil, fmt.Errorf(f, p.instance, p.host)
 		}
-		p.port, err = strconv.ParseUint(strport, 0, 16)
+		port, err := strconv.ParseUint(strport, 0, 16)
 		if err != nil {
 			f := "Invalid tcp port returned from Sql Server Browser '%v': %v"
 			return nil, fmt.Errorf(f, strport, err.Error())
 		}
+		p.port = port
 	}
 
 initiate_connection:


Test patch:
diff --git a/conn_str_test.go b/conn_str_test.go
index 06f3808..7419572 100644
--- a/conn_str_test.go
+++ b/conn_str_test.go
@@ -66,7 +66,7 @@ func TestValidConnectionString(t *testing.T) {
 		{"connection timeout=3;dial timeout=4;keepalive=5", func(p connectParams) bool {
 			return p.conn_timeout == 3*time.Second && p.dial_timeout == 4*time.Second && p.keepAlive == 5*time.Second
 		}},
-		{"log=63", func(p connectParams) bool { return p.logFlags == 63 && p.port == 1433 }},
+		{"log=63", func(p connectParams) bool { return p.logFlags == 63 && p.port == 0 }},
 		{"log=63;port=1000", func(p connectParams) bool { return p.logFlags == 63 && p.port == 1000 }},
 		{"log=64", func(p connectParams) bool { return p.logFlags == 64 && p.packetSize == 4096 }},
 		{"log=64;packet size=0", func(p connectParams) bool { return p.logFlags == 64 && p.packetSize == 512 }},
@@ -127,16 +127,16 @@ func TestValidConnectionString(t *testing.T) {
 
 		// URL mode
 		{"sqlserver://somehost?connection+timeout=30", func(p connectParams) bool {
-			return p.host == "somehost" && p.port == 1433 && p.instance == "" && p.conn_timeout == 30*time.Second
+			return p.host == "somehost" && p.port == 0 && p.instance == "" && p.conn_timeout == 30*time.Second
 		}},
 		{"sqlserver://someuser@somehost?connection+timeout=30", func(p connectParams) bool {
-			return p.host == "somehost" && p.port == 1433 && p.instance == "" && p.user == "someuser" && p.password == "" && p.conn_timeout == 30*time.Second
+			return p.host == "somehost" && p.port == 0 && p.instance == "" && p.user == "someuser" && p.password == "" && p.conn_timeout == 30*time.Second
 		}},
 		{"sqlserver://someuser:@somehost?connection+timeout=30", func(p connectParams) bool {
-			return p.host == "somehost" && p.port == 1433 && p.instance == "" && p.user == "someuser" && p.password == "" && p.conn_timeout == 30*time.Second
+			return p.host == "somehost" && p.port == 0 && p.instance == "" && p.user == "someuser" && p.password == "" && p.conn_timeout == 30*time.Second
 		}},
 		{"sqlserver://someuser:foo%3A%2F%5C%21~%40;bar@somehost?connection+timeout=30", func(p connectParams) bool {
-			return p.host == "somehost" && p.port == 1433 && p.instance == "" && p.user == "someuser" && p.password == "foo:/\\!~@;bar" && p.conn_timeout == 30*time.Second
+			return p.host == "somehost" && p.port == 0 && p.instance == "" && p.user == "someuser" && p.password == "foo:/\\!~@;bar" && p.conn_timeout == 30*time.Second
 		}},
 		{"sqlserver://someuser:foo%3A%2F%5C%21~%40;bar@somehost:1434?connection+timeout=30", func(p connectParams) bool {
 			return p.host == "somehost" && p.port == 1434 && p.instance == "" && p.user == "someuser" && p.password == "foo:/\\!~@;bar" && p.conn_timeout == 30*time.Second