Skip to content

Commit

Permalink
Merge branch 'master' into SNOW-870356-increase-coverage
Browse files Browse the repository at this point in the history
  • Loading branch information
sfc-gh-ext-simba-jl authored Oct 27, 2023
2 parents 211d147 + 8479309 commit 9e3556b
Show file tree
Hide file tree
Showing 17 changed files with 750 additions and 171 deletions.
2 changes: 1 addition & 1 deletion auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ func postAuth(

fullURL := sr.getFullURL(loginRequestPath, params)
logger.Infof("full URL: %v", fullURL)
resp, err := sr.FuncAuthPost(ctx, client, fullURL, headers, bodyCreator, timeout)
resp, err := sr.FuncAuthPost(ctx, client, fullURL, headers, bodyCreator, timeout, sr.MaxRetryCount)
if err != nil {
return nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions chunk_downloader.go
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ func getChunk(
if err != nil {
return nil, err
}
return newRetryHTTP(ctx, sc.rest.Client, http.NewRequest, u, headers, timeout, sc.currentTimeProvider, sc.cfg).execute()
return newRetryHTTP(ctx, sc.rest.Client, http.NewRequest, u, headers, timeout, sc.rest.MaxRetryCount, sc.currentTimeProvider, sc.cfg).execute()
}

func (scd *snowflakeChunkDownloader) startArrowBatches() error {
Expand Down Expand Up @@ -638,7 +638,7 @@ func (f *httpStreamChunkFetcher) fetch(URL string, rows chan<- []*string) error
if err != nil {
return err
}
res, err := newRetryHTTP(context.Background(), f.client, http.NewRequest, fullURL, f.headers, 0, defaultTimeProvider, nil).execute()
res, err := newRetryHTTP(context.Background(), f.client, http.NewRequest, fullURL, f.headers, 0, 0, defaultTimeProvider, nil).execute()
if err != nil {
return err
}
Expand Down
6 changes: 5 additions & 1 deletion client_configuration.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func getClientConfig(filePathFromConnectionString string) (*ClientConfig, error)
configPredefinedFilePaths := clientConfigPredefinedDirs()
filePath, err := findClientConfigFilePath(filePathFromConnectionString, configPredefinedFilePaths)
if err != nil {
return nil, err
return nil, findClientConfigError(err)

Check warning on line 33 in client_configuration.go

View check run for this annotation

Codecov / codecov/patch

client_configuration.go#L33

Added line #L33 was not covered by tests
}
if filePath == "" { // we did not find a config file
return nil, nil
Expand Down Expand Up @@ -63,6 +63,10 @@ func searchForConfigFile(directories []string) (string, error) {
return "", nil
}

func findClientConfigError(err error) error {
return fmt.Errorf("finding client config failed: %w", err)

Check warning on line 67 in client_configuration.go

View check run for this annotation

Codecov / codecov/patch

client_configuration.go#L66-L67

Added lines #L66 - L67 were not covered by tests
}

func existsFile(filePath string) (bool, error) {
_, err := os.Stat(filePath)
if err == nil {
Expand Down
53 changes: 23 additions & 30 deletions client_configuration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"os"
"path"
"strings"
"testing"
)

Expand Down Expand Up @@ -84,12 +85,7 @@ func TestCreatePredefinedDirs(t *testing.T) {
func TestGetClientConfig(t *testing.T) {
dir := t.TempDir()
fileName := "config.json"
configContents := `{
"common": {
"log_level" : "INFO",
"log_path" : "/some-path/some-directory"
}
}`
configContents := createClientConfigContent("INFO", "/some-path/some-directory")
createFile(t, fileName, configContents, dir)
filePath := path.Join(dir, fileName)

Expand Down Expand Up @@ -118,26 +114,16 @@ func TestParseConfiguration(t *testing.T) {
expectedLogPath string
}{
{
testName: "TestWithLogLevelUpperCase",
fileName: "config_1.json",
fileContents: `{
"common": {
"log_level" : "INFO",
"log_path" : "/some-path/some-directory"
}
}`,
testName: "TestWithLogLevelUpperCase",
fileName: "config_1.json",
fileContents: createClientConfigContent("INFO", "/some-path/some-directory"),
expectedLogLevel: "INFO",
expectedLogPath: "/some-path/some-directory",
},
{
testName: "TestWithLogLevelLowerCase",
fileName: "config_2.json",
fileContents: `{
"common": {
"log_level" : "info",
"log_path" : "/some-path/some-directory"
}
}`,
testName: "TestWithLogLevelLowerCase",
fileName: "config_2.json",
fileContents: createClientConfigContent("info", "/some-path/some-directory"),
expectedLogLevel: "info",
expectedLogPath: "/some-path/some-directory",
},
Expand Down Expand Up @@ -193,14 +179,9 @@ func TestParseConfigurationFails(t *testing.T) {
expectedErrorMessageToContain string
}{
{
testName: "TestWithWrongLogLevel",
fileName: "config_1.json",
FileContents: `{
"common": {
"log_level" : "something weird",
"log_path" : "/some-path/some-directory"
}
}`,
testName: "TestWithWrongLogLevel",
fileName: "config_1.json",
FileContents: createClientConfigContent("something weird", "/some-path/some-directory"),
expectedErrorMessageToContain: "unknown log level",
},
{
Expand Down Expand Up @@ -284,3 +265,15 @@ func predefinedTestDirs(dirs struct {
}) []string {
return []string{dirs.predefinedDir1, dirs.predefinedDir2}
}

func createClientConfigContent(logLevel string, logPath string) string {
return fmt.Sprintf(`{
"common": {
"log_level" : "%s",
"log_path" : "%s"
}
}`,
logLevel,
strings.ReplaceAll(logPath, "\\", "\\\\"),
)
}
5 changes: 5 additions & 0 deletions connection.go
Original file line number Diff line number Diff line change
Expand Up @@ -737,6 +737,10 @@ func buildSnowflakeConn(ctx context.Context, config Config) (*snowflakeConn, err
queryContextCache: (&queryContextCache{}).init(),
currentTimeProvider: defaultTimeProvider,
}
err := initEasyLogging(config.ClientConfigFile)
if err != nil {
return nil, err
}
var st http.RoundTripper = SnowflakeTransport
if sc.cfg.Transporter == nil {
if sc.cfg.InsecureMode {
Expand Down Expand Up @@ -785,6 +789,7 @@ func buildSnowflakeConn(ctx context.Context, config Config) (*snowflakeConn, err
TokenAccessor: tokenAccessor,
LoginTimeout: sc.cfg.LoginTimeout,
RequestTimeout: sc.cfg.RequestTimeout,
MaxRetryCount: sc.cfg.MaxRetryCount,
FuncPost: postRestful,
FuncGet: getRestful,
FuncAuthPost: postAuthRestful,
Expand Down
24 changes: 22 additions & 2 deletions dsn.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,13 @@ import (
)

const (
defaultClientTimeout = 300 * time.Second // Timeout for network round trip + read out http response
defaultClientTimeout = 900 * time.Second // Timeout for network round trip + read out http response
defaultJWTClientTimeout = 10 * time.Second // Timeout for network round trip + read out http response but used for JWT auth
defaultLoginTimeout = 60 * time.Second // Timeout for retry for login EXCLUDING clientTimeout
defaultLoginTimeout = 300 * time.Second // Timeout for retry for login EXCLUDING clientTimeout
defaultRequestTimeout = 0 * time.Second // Timeout for retry for request EXCLUDING clientTimeout
defaultJWTTimeout = 60 * time.Second
defaultExternalBrowserTimeout = 120 * time.Second // Timeout for external browser login
defaultMaxRetryCount = 7 // specifies maximum number of subsequent retries
defaultDomain = ".snowflakecomputing.com"
)

Expand Down Expand Up @@ -74,6 +75,7 @@ type Config struct {
ClientTimeout time.Duration // Timeout for network round trip + read out http response
JWTClientTimeout time.Duration // Timeout for network round trip + read out http response used when JWT token auth is taking place
ExternalBrowserTimeout time.Duration // Timeout for external browser login
MaxRetryCount int // Specifies how many times non-periodic HTTP request can be retried

Application string // application name.
InsecureMode bool // driver doesn't check certificate revocation status
Expand Down Expand Up @@ -101,6 +103,8 @@ type Config struct {
DisableQueryContextCache bool // Should HTAP query context cache be disabled

IncludeRetryReason ConfigBool // Should retried request contain retry reason

ClientConfigFile string // File path to the client configuration json file
}

// Validate enables testing if config is correct.
Expand Down Expand Up @@ -203,6 +207,9 @@ func DSN(cfg *Config) (dsn string, err error) {
if cfg.ExternalBrowserTimeout != defaultExternalBrowserTimeout {
params.Add("externalBrowserTimeout", strconv.FormatInt(int64(cfg.ExternalBrowserTimeout/time.Second), 10))
}
if cfg.MaxRetryCount != defaultMaxRetryCount {
params.Add("maxRetryCount", strconv.Itoa(cfg.MaxRetryCount))
}
if cfg.Application != clientType {
params.Add("application", cfg.Application)
}
Expand Down Expand Up @@ -252,6 +259,9 @@ func DSN(cfg *Config) (dsn string, err error) {
if cfg.ClientStoreTemporaryCredential != configBoolNotSet {
params.Add("clientStoreTemporaryCredential", strconv.FormatBool(cfg.ClientStoreTemporaryCredential != ConfigBoolFalse))
}
if cfg.ClientConfigFile != "" {
params.Add("clientConfigFile", cfg.ClientConfigFile)
}

dsn = fmt.Sprintf("%v:%v@%v:%v", url.QueryEscape(cfg.User), url.QueryEscape(cfg.Password), cfg.Host, cfg.Port)
if params.Encode() != "" {
Expand Down Expand Up @@ -466,6 +476,9 @@ func fillMissingConfigParameters(cfg *Config) error {
if cfg.ExternalBrowserTimeout == 0 {
cfg.ExternalBrowserTimeout = defaultExternalBrowserTimeout
}
if cfg.MaxRetryCount == 0 {
cfg.MaxRetryCount = defaultMaxRetryCount
}
if strings.Trim(cfg.Application, " ") == "" {
cfg.Application = clientType
}
Expand Down Expand Up @@ -637,6 +650,11 @@ func parseDSNParams(cfg *Config, params string) (err error) {
if err != nil {
return err
}
case "maxRetryCount":
cfg.MaxRetryCount, err = strconv.Atoi(value)
if err != nil {
return err
}

Check warning on line 657 in dsn.go

View check run for this annotation

Codecov / codecov/patch

dsn.go#L656-L657

Added lines #L656 - L657 were not covered by tests
case "application":
cfg.Application = value
case "authenticator":
Expand Down Expand Up @@ -734,6 +752,8 @@ func parseDSNParams(cfg *Config, params string) (err error) {
} else {
cfg.IncludeRetryReason = ConfigBoolFalse
}
case "clientConfigFile":
cfg.ClientConfigFile = value
default:
if cfg.Params == nil {
cfg.Params = make(map[string]*string)
Expand Down
82 changes: 80 additions & 2 deletions dsn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,23 @@ func TestParseDSN(t *testing.T) {
ClientTimeout: defaultClientTimeout,
JWTClientTimeout: defaultJWTClientTimeout,
IncludeRetryReason: ConfigBoolTrue,
MaxRetryCount: defaultMaxRetryCount,
},
ocspMode: ocspModeFailOpen,
},
{
dsn: "u:p@a?database=d&maxRetryCount=20",
config: &Config{
Account: "a", User: "u", Password: "p",
Protocol: "https", Host: "a.snowflakecomputing.com", Port: 443,
Database: "d", Schema: "",
ExternalBrowserTimeout: defaultExternalBrowserTimeout,
OCSPFailOpen: OCSPFailOpenTrue,
ValidateDefaultParameters: ConfigBoolTrue,
ClientTimeout: defaultClientTimeout,
JWTClientTimeout: defaultJWTClientTimeout,
IncludeRetryReason: ConfigBoolTrue,
MaxRetryCount: 20,
},
ocspMode: ocspModeFailOpen,
},
Expand Down Expand Up @@ -660,9 +677,39 @@ func TestParseDSN(t *testing.T) {
err: nil,
},
{
dsn: "u:[email protected]:443?authenticator=http%3A%2F%2Fsc.okta.com&ocspFailOpen=true&validateDefaultParameters=true",
err: errFailedToParseAuthenticator(),
dsn: "u:[email protected]/db/s?account=a.r.c&includeRetryReason=true&clientConfigFile=%2FUsers%2Fuser%2Fconfig.json",
config: &Config{
Account: "a", User: "u", Password: "p",
Protocol: "https", Host: "a.r.c.snowflakecomputing.com", Port: 443,
Database: "db", Schema: "s", ValidateDefaultParameters: ConfigBoolTrue, OCSPFailOpen: OCSPFailOpenTrue,
ClientTimeout: defaultClientTimeout,
JWTClientTimeout: defaultJWTClientTimeout,
ExternalBrowserTimeout: defaultExternalBrowserTimeout,
IncludeRetryReason: ConfigBoolTrue,
ClientConfigFile: "/Users/user/config.json",
},
ocspMode: ocspModeFailOpen,
err: nil,
},
{
dsn: "u:[email protected]/db/s?account=a.r.c&includeRetryReason=true&clientConfigFile=c%3A%5CUsers%5Cuser%5Cconfig.json",
config: &Config{
Account: "a", User: "u", Password: "p",
Protocol: "https", Host: "a.r.c.snowflakecomputing.com", Port: 443,
Database: "db", Schema: "s", ValidateDefaultParameters: ConfigBoolTrue, OCSPFailOpen: OCSPFailOpenTrue,
ClientTimeout: defaultClientTimeout,
JWTClientTimeout: defaultJWTClientTimeout,
ExternalBrowserTimeout: defaultExternalBrowserTimeout,
IncludeRetryReason: ConfigBoolTrue,
ClientConfigFile: "c:\\Users\\user\\config.json",
},
ocspMode: ocspModeFailOpen,
err: nil,
},
{
dsn: "u:[email protected]:443?authenticator=http%3A%2F%2Fsc.okta.com&ocspFailOpen=true&validateDefaultParameters=true",
err: errFailedToParseAuthenticator(),
},
}

for _, at := range []AuthType{AuthTypeExternalBrowser, AuthTypeOAuth} {
Expand Down Expand Up @@ -822,6 +869,7 @@ func TestParseDSN(t *testing.T) {
if test.config.IncludeRetryReason != cfg.IncludeRetryReason {
t.Fatalf("%v: Failed to match IncludeRetryReason. expected: %v, got: %v", i, test.config.IncludeRetryReason, cfg.IncludeRetryReason)
}
assertEqualF(t, cfg.ClientConfigFile, test.config.ClientConfigFile, "client config file")
case test.err != nil:
driverErrE, okE := test.err.(*SnowflakeError)
driverErrG, okG := err.(*SnowflakeError)
Expand Down Expand Up @@ -1212,6 +1260,16 @@ func TestDSN(t *testing.T) {
},
dsn: "u:[email protected]:443?ocspFailOpen=true&region=b.c&tmpDirPath=%2Ftmp&validateDefaultParameters=true",
},
{
cfg: &Config{
User: "u",
Password: "p",
Account: "a.b.c",
IncludeRetryReason: ConfigBoolFalse,
MaxRetryCount: 30,
},
dsn: "u:[email protected]:443?includeRetryReason=false&maxRetryCount=30&ocspFailOpen=true&region=b.c&validateDefaultParameters=true",
},
{
cfg: &Config{
User: "u",
Expand Down Expand Up @@ -1240,6 +1298,26 @@ func TestDSN(t *testing.T) {
},
dsn: "u:[email protected]:443?ocspFailOpen=true&region=b.c&validateDefaultParameters=true",
},
{
cfg: &Config{
User: "u",
Password: "p",
Account: "a.b.c",
IncludeRetryReason: ConfigBoolTrue,
ClientConfigFile: "/Users/user/config.json",
},
dsn: "u:[email protected]:443?clientConfigFile=%2FUsers%2Fuser%2Fconfig.json&ocspFailOpen=true&region=b.c&validateDefaultParameters=true",
},
{
cfg: &Config{
User: "u",
Password: "p",
Account: "a.b.c",
IncludeRetryReason: ConfigBoolTrue,
ClientConfigFile: "c:\\Users\\user\\config.json",
},
dsn: "u:[email protected]:443?clientConfigFile=c%3A%5CUsers%5Cuser%5Cconfig.json&ocspFailOpen=true&region=b.c&validateDefaultParameters=true",
},
}
for _, test := range testcases {
t.Run(test.dsn, func(t *testing.T) {
Expand Down
Loading

0 comments on commit 9e3556b

Please sign in to comment.