Repository: snowflakedb/gosnowflake
Pull request #897 (2023-08-24 13:33:09)

SNOW-848019 Change global error objects to instantiated by function
### Description
Refactored global error objects to functions that create new instances to avoid thread mutability issues.

### Checklist
- [x] Code compiles correctly
- [ ] Run ``make fmt`` to fix inconsistent formats
- [ ] Run ``make lint`` to get lint errors and fix all of them
- [ ] Created tests which fail without the change (if possible)
- [ ] All tests passing
- [ ] Extended the README / documentation, if necessary



Patch:
diff --git a/connection.go b/connection.go
index a5fd654..9b4b1f8 100644
--- a/connection.go
+++ b/connection.go
@@ -72,7 +72,6 @@ type snowflakeConn struct {
 var (
 	queryIDPattern = `[\w\-_]+`
 	queryIDRegexp  = regexp.MustCompile(queryIDPattern)
-	errMutex       = &sync.Mutex{}
 )
 
 func (sc *snowflakeConn) exec(
@@ -138,8 +137,6 @@ func (sc *snowflakeConn) exec(
 	}
 	logger.WithContext(ctx).Infof("Success: %v, Code: %v", data.Success, code)
 	if !data.Success {
-		errMutex.Lock()
-		defer errMutex.Unlock()
 		err = (populateErrorFields(code, data)).exceptionTelemetry(sc)
 		return nil, err
 	}
diff --git a/dsn.go b/dsn.go
index cdfd805..bf6710c 100644
--- a/dsn.go
+++ b/dsn.go
@@ -139,7 +139,7 @@ func DSN(cfg *Config) (dsn string, err error) {
 	posDot := strings.Index(cfg.Account, ".")
 	if posDot > 0 {
 		if cfg.Region != "" {
-			return "", ErrInvalidRegion
+			return "", errInvalidRegion()
 		}
 		cfg.Region = cfg.Account[posDot+1:]
 		cfg.Account = cfg.Account[:posDot]
@@ -403,15 +403,15 @@ func fillMissingConfigParameters(cfg *Config) error {
 		}
 	}
 	if strings.Trim(cfg.Account, " ") == "" {
-		return ErrEmptyAccount
+		return errEmptyAccount()
 	}
 
 	if authRequiresUser(cfg) && strings.TrimSpace(cfg.User) == "" {
-		return ErrEmptyUsername
+		return errEmptyUsername()
 	}
 
 	if authRequiresPassword(cfg) && strings.TrimSpace(cfg.Password) == "" {
-		return ErrEmptyPassword
+		return errEmptyPassword()
 	}
 	if strings.Trim(cfg.Protocol, " ") == "" {
 		cfg.Protocol = "https"
diff --git a/errors.go b/errors.go
index 4598507..a4104f6 100644
--- a/errors.go
+++ b/errors.go
@@ -82,7 +82,7 @@ func (se *SnowflakeError) exceptionTelemetry(sc *snowflakeConn) *SnowflakeError
 
 // return populated error fields replacing the default response
 func populateErrorFields(code int, data *execResponse) *SnowflakeError {
-	err := ErrUnknownError
+	err := errUnknownError()
 	if code != -1 {
 		err.Number = code
 	}
@@ -290,32 +290,44 @@ const (
 	errMsgInvalidPadding                     = "invalid padding on input"
 )
 
-var (
-	// ErrEmptyAccount is returned if a DNS doesn't include account parameter.
-	ErrEmptyAccount = &SnowflakeError{
+// Returned if a DNS doesn't include account parameter.
+func errEmptyAccount() *SnowflakeError {
+	return &SnowflakeError{
 		Number:  ErrCodeEmptyAccountCode,
 		Message: "account is empty",
 	}
-	// ErrEmptyUsername is returned if a DNS doesn't include user parameter.
-	ErrEmptyUsername = &SnowflakeError{
+}
+
+// Returned if a DNS doesn't include user parameter.
+func errEmptyUsername() *SnowflakeError {
+	return &SnowflakeError{
 		Number:  ErrCodeEmptyUsernameCode,
 		Message: "user is empty",
 	}
-	// ErrEmptyPassword is returned if a DNS doesn't include password parameter.
-	ErrEmptyPassword = &SnowflakeError{
+}
+
+// Returned if a DNS doesn't include password parameter.
+func errEmptyPassword() *SnowflakeError {
+	return &SnowflakeError{
 		Number:  ErrCodeEmptyPasswordCode,
-		Message: "password is empty"}
+		Message: "password is empty",
+	}
+}
 
-	// ErrInvalidRegion is returned if a DSN's implicit region from account parameter and explicit region parameter conflict.
-	ErrInvalidRegion = &SnowflakeError{
+// Returned if a DSN's implicit region from account parameter and explicit region parameter conflict.
+func errInvalidRegion() *SnowflakeError {
+	return &SnowflakeError{
 		Number:  ErrCodeRegionOverlap,
-		Message: "two regions specified"}
+		Message: "two regions specified",
+	}
+}
 
-	// ErrUnknownError is returned if the server side returns an error without meaningful message.
-	ErrUnknownError = &SnowflakeError{
+// Returned if the server side returns an error without meaningful message.
+func errUnknownError() *SnowflakeError {
+	return &SnowflakeError{
 		Number:   -1,
 		SQLState: "-1",
 		Message:  "an unknown server side error occurred",
 		QueryID:  "-1",
 	}
-)
+}


Test patch:
diff --git a/connection_test.go b/connection_test.go
index 90d0bc4..2b4ce57 100644
--- a/connection_test.go
+++ b/connection_test.go
@@ -491,11 +491,12 @@ func TestExecWithServerSideError(t *testing.T) {
 		t.Error("expected a server side error")
 	}
 	sfe := err.(*SnowflakeError)
+	errUnknownError := errUnknownError()
 	if sfe.Number != -1 || sfe.SQLState != "-1" || sfe.QueryID != "-1" {
-		t.Errorf("incorrect snowflake error. expected: %v, got: %v", ErrUnknownError, *sfe)
+		t.Errorf("incorrect snowflake error. expected: %v, got: %v", errUnknownError, *sfe)
 	}
 	if !strings.Contains(sfe.Message, "an unknown server side error occurred") {
-		t.Errorf("incorrect message. expected: %v, got: %v", ErrUnknownError.Message, sfe.Message)
+		t.Errorf("incorrect message. expected: %v, got: %v", errUnknownError.Message, sfe.Message)
 	}
 }
 
@@ -549,6 +550,30 @@ func postQueryFail(_ context.Context, _ *snowflakeRestful, _ *url.Values, header
 	}, errors.New("failed to get query response")
 }
 
+func TestErrorReportingOnConcurrentFails(t *testing.T) {
+	db := openDB(t)
+	defer db.Close()
+	var wg sync.WaitGroup
+	n := 5
+	wg.Add(3 * n)
+	for i := 0; i < n; i++ {
+		go executeQueryAndConfirmMessage(db, "SELECT * FROM TABLE_ABC", "TABLE_ABC", t, &wg)
+		go executeQueryAndConfirmMessage(db, "SELECT * FROM TABLE_DEF", "TABLE_DEF", t, &wg)
+		go executeQueryAndConfirmMessage(db, "SELECT * FROM TABLE_GHI", "TABLE_GHI", t, &wg)
+	}
+	wg.Wait()
+}
+
+func executeQueryAndConfirmMessage(db *sql.DB, query string, expectedErrorTable string, t *testing.T, wg *sync.WaitGroup) {
+	defer wg.Done()
+	_, err := db.Exec(query)
+	message := err.(*SnowflakeError).Message
+	if !strings.Contains(message, expectedErrorTable) {
+		t.Errorf("QueryID: %s, Message %s ###### Expected error message table name: %s",
+			err.(*SnowflakeError).QueryID, err.(*SnowflakeError).Message, expectedErrorTable)
+	}
+}
+
 func TestQueryArrowStreamError(t *testing.T) {
 	ctx := context.Background()
 	numrows := 50000 // approximately 10 ArrowBatch objects
diff --git a/dsn_test.go b/dsn_test.go
index 21ba433..362ad21 100644
--- a/dsn_test.go
+++ b/dsn_test.go
@@ -25,7 +25,6 @@ type tcParseDSN struct {
 }
 
 func TestParseDSN(t *testing.T) {
-
 	privKeyPKCS8 := generatePKCS8StringSupress(testPrivKey)
 	privKeyPKCS1 := generatePKCS1String(testPrivKey)
 	testcases := []tcParseDSN{
@@ -201,7 +200,7 @@ func TestParseDSN(t *testing.T) {
 				ExternalBrowserTimeout:    defaultExternalBrowserTimeout,
 			},
 			ocspMode: ocspModeFailOpen,
-			err:      ErrEmptyPassword,
+			err:      errEmptyPassword(),
 		},
 		{
 			dsn: "@host:123/db/schema?account=ac&protocol=http",
@@ -216,7 +215,7 @@ func TestParseDSN(t *testing.T) {
 				ExternalBrowserTimeout:    defaultExternalBrowserTimeout,
 			},
 			ocspMode: ocspModeFailOpen,
-			err:      ErrEmptyUsername,
+			err:      errEmptyUsername(),
 		},
 		{
 			dsn: "user:p@host:123/db/schema?protocol=http",
@@ -231,7 +230,7 @@ func TestParseDSN(t *testing.T) {
 				ExternalBrowserTimeout:    defaultExternalBrowserTimeout,
 			},
 			ocspMode: ocspModeFailOpen,
-			err:      ErrEmptyAccount,
+			err:      errEmptyAccount(),
 		},
 		{
 			dsn: "u:p@a.snowflakecomputing.com/db/pa?account=a&protocol=https&role=r&timezone=UTC&warehouse=w",
@@ -415,12 +414,12 @@ func TestParseDSN(t *testing.T) {
 		{
 			dsn:    "u:u@/+/+?account=+&=0",
 			config: &Config{},
-			err:    ErrEmptyAccount,
+			err:    errEmptyAccount(),
 		},
 		{
 			dsn:    "u:u@/+/+?account=+&=+&=+",
 			config: &Config{},
-			err:    ErrEmptyAccount,
+			err:    errEmptyAccount(),
 		},
 		{
 			dsn: "user%40%2F1:p%3A%40s@/db%2F?account=ac",
@@ -630,7 +629,7 @@ func TestParseDSN(t *testing.T) {
 				Authenticator:             at,
 			},
 			ocspMode: ocspModeFailOpen,
-			err:      ErrEmptyUsername,
+			err:      errEmptyUsername(),
 		})
 	}
 
@@ -649,7 +648,7 @@ func TestParseDSN(t *testing.T) {
 				Authenticator:             at,
 			},
 			ocspMode: ocspModeFailOpen,
-			err:      ErrEmptyPassword,
+			err:      errEmptyPassword(),
 		})
 	}
 
@@ -775,7 +774,6 @@ type tcDSN struct {
 
 func TestDSN(t *testing.T) {
 	tmfmt := "MM-DD-YYYY"
-
 	testcases := []tcDSN{
 		{
 			cfg: &Config{
@@ -809,7 +807,7 @@ func TestDSN(t *testing.T) {
 				Account:  "a-aofnadsf.global",
 				Region:   "r",
 			},
-			err: ErrInvalidRegion,
+			err: errInvalidRegion(),
 		},
 		{
 			cfg: &Config{
@@ -853,7 +851,7 @@ func TestDSN(t *testing.T) {
 				Password: "p",
 				Account:  "a",
 			},
-			err: ErrEmptyUsername,
+			err: errEmptyUsername(),
 		},
 		{
 			cfg: &Config{
@@ -861,7 +859,7 @@ func TestDSN(t *testing.T) {
 				Password: "",
 				Account:  "a",
 			},
-			err: ErrEmptyPassword,
+			err: errEmptyPassword(),
 		},
 		{
 			cfg: &Config{
@@ -869,7 +867,7 @@ func TestDSN(t *testing.T) {
 				Password: "p",
 				Account:  "",
 			},
-			err: ErrEmptyAccount,
+			err: errEmptyAccount(),
 		},
 		{
 			cfg: &Config{
@@ -895,7 +893,7 @@ func TestDSN(t *testing.T) {
 				Account:  "a.e",
 				Region:   "r",
 			},
-			err: ErrInvalidRegion,
+			err: errInvalidRegion(),
 		},
 		{
 			cfg: &Config{
@@ -1039,7 +1037,7 @@ func TestDSN(t *testing.T) {
 				Account:  "a.b.c",
 				Region:   "r",
 			},
-			err: ErrInvalidRegion,
+			err: errInvalidRegion(),
 		},
 		{
 			cfg: &Config{