Fix bug where SQL Server Browser is required if using named instances (…

…microsoft#6)

* Fix bug where SQL Server Browser is required if using named instances

* Add failing test which now passes

Co-authored-by: stuartpa <stuartpa@hotmail.com>

README.md

@@ -32,7 +32,7 @@ Other supported formats are listed below.
 ### Connection parameters for ODBC and ADO style connection strings
 
 * `server` - host or host\instance (default localhost)
-* `port` - used only when there is no instance in server (default 1433)
+* `port` - specifies the host\instance port (default 1433). If instance name is provided but no port, the driver will use SQL Server Browser to discover the port.
 
 ### Less common parameters
 

bad_server_test.go

@@ -10,7 +10,7 @@ import (
 
 // tests simulating bad server
 
-func testConnectionBad(t *testing.T, connStr string) {
+func testConnectionBad(t *testing.T, connStr string) (err error) {
 	conn, err := sql.Open("mssql", connStr)
 	if err != nil {
 		// should not fail here
@@ -24,6 +24,8 @@ func testConnectionBad(t *testing.T, connStr string) {
 	if err == nil {
 		t.Fatal("Scan should fail but it succeeded")
 	}
+
+	return
 }
 
 func testBadServer(t *testing.T, handler func(net.Conn)) {

tds.go

@@ -1050,14 +1050,16 @@ func connect(ctx context.Context, c *Connector, logger ContextLogger, p msdsn.Co
 		dialCtx, cancel = context.WithTimeout(ctx, dt)
 		defer cancel()
 	}
+
 	// if instance is specified use instance resolution service
 	if len(p.Instance) > 0 && p.Port != 0 && uint64(p.LogFlags)&logDebug != 0 {
 		// both instance name and port specified
 		// when port is specified instance name is not used
 		// you should not provide instance name when you provide port
 		logger.Log(ctx, msdsn.LogDebug, "WARN: You specified both instance name and port in the connection string, port will be used and instance name will be ignored")
-	}
-	if len(p.Instance) > 0 {
+	} else if len(p.Instance) > 0 && p.Port == 0 {
+		// If instance is specified, but no port, check SQL Server Browser
+		// for the instance and discover its port.
 		p.Instance = strings.ToUpper(p.Instance)
 		d := c.getDialer(&p)
 		instances, err := getInstances(dialCtx, d, p.Host)
@@ -1077,6 +1079,7 @@ func connect(ctx context.Context, c *Connector, logger ContextLogger, p msdsn.Co
 		}
 		p.Port = port
 	}
+
 	if p.Port == 0 {
 		p.Port = defaultServerPort
 	}

tds_test.go

@@ -13,6 +13,7 @@ import (
 	"os"
 	"path"
 	"runtime"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -584,6 +585,32 @@ func TestBadHost(t *testing.T) {
 	testConnectionBad(t, params.URL().String())
 }
 
+func TestSqlBrowserNotUsedIfPortSpecified(t *testing.T) {
+	const errorSubstrStringToCheckFor = "unable to get instances from Sql Server Browser"
+
+	// Connect to an instance on a host that doesn't exist (so connection will always expectedly fail)
+	params := testConnParams(t)
+	params.Host = "badhost"
+	params.Instance = "foobar"
+
+	// Specify no port, so error must indicate SQL Browser lookup failed
+	params.Port = 0 // No port spcified, sql browser should be used
+
+	err := testConnectionBad(t, params.URL().String())
+
+	if !strings.Contains(err.Error(), errorSubstrStringToCheckFor) {
+		t.Fatal("Connection should have tried to use SQL Browser")
+	}
+
+	// Specify port, ensure error does not indicate SQL Browser lookup failed
+	params.Port = 1500 // Specify a port, sql browser should not be tried
+	err = testConnectionBad(t, params.URL().String())
+
+	if strings.Contains(err.Error(), errorSubstrStringToCheckFor) {
+		t.Fatal("Connection should not have tried to use SQL Browser, because none zero Port specified")
+	}
+}
+
 func TestSSPIAuth(t *testing.T) {
 	if runtime.GOOS != "windows" {
 		t.Skip("Only on windows")