From cbe955e96c8476101ad6fbd8629789a8b09ee2e5 Mon Sep 17 00:00:00 2001 From: Colin Pistell Date: Thu, 20 Jun 2024 17:31:23 +0000 Subject: [PATCH 1/5] feat: Add option to pass external context into Go SDK's HTTP calls --- go/rtl/auth.go | 34 +++++++++++++++++--------- go/rtl/auth_test.go | 59 +++++++++++++++++++++++++++++++++++++++++++++ go/rtl/settings.go | 5 +++- 3 files changed, 86 insertions(+), 12 deletions(-) diff --git a/go/rtl/auth.go b/go/rtl/auth.go index d993bec7f..0ea7d9fc7 100644 --- a/go/rtl/auth.go +++ b/go/rtl/auth.go @@ -67,8 +67,13 @@ func NewAuthSessionWithTransport(config ApiSettings, transport http.RoundTripper AuthStyle: oauth2.AuthStyleInParams, } + bgCtx := context.Background() + if config.Context != nil { + bgCtx = config.Context + } + ctx := context.WithValue( - context.Background(), + bgCtx, oauth2.HTTPClient, // Will set "x-looker-appid" Header on TokenURL requests &http.Client{Transport: appIdHeaderTransport}, @@ -123,18 +128,25 @@ func (s *AuthSession) Do(result interface{}, method, ver, path string, reqPars m } } - // create request context with timeout - var timeoutInSeconds int32 = 120 //seconds - if s.Config.Timeout != 0 { - timeoutInSeconds = s.Config.Timeout - } - if options != nil && options.Timeout != 0 { - timeoutInSeconds = options.Timeout + var ctx context.Context + if options != nil && options.Context != nil { + ctx = options.Context + } else if s.Config.Context != nil { + ctx = s.Config.Context + } else { + // create request context with timeout + var timeoutInSeconds int32 = 120 //seconds + if s.Config.Timeout != 0 { + timeoutInSeconds = s.Config.Timeout + } + if options != nil && options.Timeout != 0 { + timeoutInSeconds = options.Timeout + } + var cncl context.CancelFunc + ctx, cncl = context.WithTimeout(context.Background(), time.Second*time.Duration(timeoutInSeconds)) + defer cncl() } - ctx, cncl := context.WithTimeout(context.Background(), time.Second*time.Duration(timeoutInSeconds)) - defer cncl() - // create new request req, err := http.NewRequestWithContext(ctx, method, u, bytes.NewBufferString(bodyString)) if err != nil { diff --git a/go/rtl/auth_test.go b/go/rtl/auth_test.go index c63754e52..09ba5d98c 100644 --- a/go/rtl/auth_test.go +++ b/go/rtl/auth_test.go @@ -635,6 +635,65 @@ func TestAuthSession_Do_Timeout(t *testing.T) { t.Errorf("Do() call did not error with context.DeadlineExceeded, got=%v", err) } }) + + t.Run("Do() follows Context set in AuthSession config", func(t *testing.T) { + mux := http.NewServeMux() + setupApi40Login(mux, foreverValidTestToken, http.StatusOK) + server := httptest.NewServer(mux) + defer server.Close() + + mux.HandleFunc("/api"+apiVersion+path, func(w http.ResponseWriter, r *http.Request) { + time.Sleep(4 * time.Second) + }) + + ctx, cncl := context.WithTimeout(context.Background(), 1*time.Second) + defer cncl() + + session := NewAuthSession(ApiSettings{ + BaseUrl: server.URL, + ApiVersion: apiVersion, + Context: ctx, + }) + + err := session.Do(nil, "GET", apiVersion, path, nil, nil, nil) + + if err == nil { + t.Errorf("Do() call did not error/timeout") + } else if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("Do() call did not error with context.DeadlineExceeded, got=%v", err) + } + }) + + t.Run("Do() follows Context set in Do()'s options", func(t *testing.T) { + mux := http.NewServeMux() + setupApi40Login(mux, foreverValidTestToken, http.StatusOK) + server := httptest.NewServer(mux) + defer server.Close() + + mux.HandleFunc("/api"+apiVersion+path, func(w http.ResponseWriter, r *http.Request) { + time.Sleep(4 * time.Second) + }) + + ctx, cncl := context.WithTimeout(context.Background(), 1*time.Second) + defer cncl() + + session := NewAuthSession(ApiSettings{ + BaseUrl: server.URL, + ApiVersion: apiVersion, + }) + + options := ApiSettings{ + Context: ctx, + } + + err := session.Do(nil, "GET", apiVersion, path, nil, nil, &options) + + if err == nil { + t.Errorf("Do() call did not error/timeout") + } else if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("Do() call did not error with context.DeadlineExceeded, got=%v", err) + } + }) } func TestSetQuery(t *testing.T) { diff --git a/go/rtl/settings.go b/go/rtl/settings.go index f9d1334fa..4cd04724e 100644 --- a/go/rtl/settings.go +++ b/go/rtl/settings.go @@ -1,11 +1,13 @@ package rtl import ( + "context" "fmt" - "gopkg.in/ini.v1" "os" "strconv" "strings" + + "gopkg.in/ini.v1" ) var defaultSectionName string = "Looker" @@ -20,6 +22,7 @@ type ApiSettings struct { ClientSecret string `ini:"client_secret"` ApiVersion string `ini:"api_version"` Headers map[string]string + Context context.Context } var defaultSettings ApiSettings = ApiSettings{ From d348f8069467364399ee662027cc716c7498c863 Mon Sep 17 00:00:00 2001 From: Colin Pistell Date: Thu, 20 Jun 2024 18:46:33 +0000 Subject: [PATCH 2/5] adding documentation for custom context --- go/README.md | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/go/README.md b/go/README.md index c20af3156..2f4965697 100644 --- a/go/README.md +++ b/go/README.md @@ -118,3 +118,46 @@ func main() { } } ``` +### Custom Context + +If you want even greater control over the lifecycle of the HTTP requests consider providing a [context](https://pkg.go.dev/context). Contexts can be set for all requests or per request. + +#### Custom Context for all requests + +Follow the example code snippet below if you want all requests to use the same context: + +```go +import "context" + +func main() { + // sets a timeout of 5 minutes + ctx, cancel := context.WithTimout(context.Background(), 5*time.Minute) + defer cancel() + + cfg, err := rtl.NewSettingsFromFile("path/to/looker.ini", nil) + cfg.Context = ctx + + session := rtl.NewAuthSession(cfg) + sdk := v4.NewLookerSDK(session) +} +``` + +#### Custom Context per request + +Follow the example here to set a context for a specific request. **This will override any context set in the SDK config as outlined in the previous section.** + +```go +import "context" + +func main() { + // sets a timeout of 5 minutes + ctx, cancel := context.WithTimout(context.Background(), 5*time.Minute) + defer cancel() + + cfg, err := rtl.NewSettingsFromFile("path/to/looker.ini", nil) + session := rtl.NewAuthSession(cfg) + sdk := v4.NewLookerSDK(session) + + sdk.Me("", &ApiSettings{Context: ctx}) +} +``` \ No newline at end of file From 8fcc052061a8daf6daf38117b1daecaf49a6b466 Mon Sep 17 00:00:00 2001 From: Colin Pistell Date: Fri, 12 Jul 2024 19:10:40 +0000 Subject: [PATCH 3/5] updating tests and readme based on feedback --- go/README.md | 19 +++++++-- go/rtl/auth_test.go | 99 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 4 deletions(-) diff --git a/go/README.md b/go/README.md index 2f4965697..4fac37fac 100644 --- a/go/README.md +++ b/go/README.md @@ -122,6 +122,10 @@ func main() { If you want even greater control over the lifecycle of the HTTP requests consider providing a [context](https://pkg.go.dev/context). Contexts can be set for all requests or per request. +If you wish to include timeout functionality in your custom context then you should leverage [context.WithTimeout](https://pkg.go.dev/context#WithTimeout). + +> Note: Setting a context will override any "Timeout" settings that you have applied! + #### Custom Context for all requests Follow the example code snippet below if you want all requests to use the same context: @@ -131,7 +135,7 @@ import "context" func main() { // sets a timeout of 5 minutes - ctx, cancel := context.WithTimout(context.Background(), 5*time.Minute) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() cfg, err := rtl.NewSettingsFromFile("path/to/looker.ini", nil) @@ -142,16 +146,20 @@ func main() { } ``` +> Note: A context set here will also apply to background requests to fetch/refresh oauth tokens, which are normally separated from contexts set via the Timeout property. + #### Custom Context per request -Follow the example here to set a context for a specific request. **This will override any context set in the SDK config as outlined in the previous section.** +Follow the example here to set a context for a specific request. + +> Note: This will override any "Timeout" settings that you have applied, as well as any context set in the SDK config as outlined in the previus section! ```go import "context" func main() { // sets a timeout of 5 minutes - ctx, cancel := context.WithTimout(context.Background(), 5*time.Minute) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() cfg, err := rtl.NewSettingsFromFile("path/to/looker.ini", nil) @@ -160,4 +168,7 @@ func main() { sdk.Me("", &ApiSettings{Context: ctx}) } -``` \ No newline at end of file +``` + +> Note: Setting a context per request will NOT affect the context used for the background token fetching requests. If you have also set a context for all requests as mentioned above then that context +will still be used for the token requests, otherwise the SDK will fall back on using a completely separate context for the token fetching requests. \ No newline at end of file diff --git a/go/rtl/auth_test.go b/go/rtl/auth_test.go index 09ba5d98c..c7a4e0eea 100644 --- a/go/rtl/auth_test.go +++ b/go/rtl/auth_test.go @@ -694,6 +694,105 @@ func TestAuthSession_Do_Timeout(t *testing.T) { t.Errorf("Do() call did not error with context.DeadlineExceeded, got=%v", err) } }) + + t.Run("Timeout set in Do()'s options overrides Authsession", func(t *testing.T) { + mux := http.NewServeMux() + setupApi40Login(mux, foreverValidTestToken, http.StatusOK) + server := httptest.NewServer(mux) + defer server.Close() + + mux.HandleFunc("/api"+apiVersion+path, func(w http.ResponseWriter, r *http.Request) { + time.Sleep(4 * time.Second) + }) + + session := NewAuthSession(ApiSettings{ + BaseUrl: server.URL, + ApiVersion: apiVersion, + Timeout: 5, + }) + + options := ApiSettings{ + Timeout: 1, //seconds + } + + err := session.Do(nil, "GET", apiVersion, path, nil, nil, &options) + + if err == nil { + t.Errorf("Do() call did not error/timeout") + } else if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("Do() call did not error with context.DeadlineExceeded, got=%v", err) + } + }) + + t.Run("Context set in AuthSession config overrides Timeouts", func(t *testing.T) { + mux := http.NewServeMux() + setupApi40Login(mux, foreverValidTestToken, http.StatusOK) + server := httptest.NewServer(mux) + defer server.Close() + + mux.HandleFunc("/api"+apiVersion+path, func(w http.ResponseWriter, r *http.Request) { + time.Sleep(4 * time.Second) + }) + + ctx, cncl := context.WithTimeout(context.Background(), 1*time.Second) + defer cncl() + + session := NewAuthSession(ApiSettings{ + BaseUrl: server.URL, + ApiVersion: apiVersion, + Context: ctx, + Timeout: 5, + }) + + options := ApiSettings{ + Timeout: 5, + } + + err := session.Do(nil, "GET", apiVersion, path, nil, nil, &options) + + if err == nil { + t.Errorf("Do() call did not error/timeout") + } else if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("Do() call did not error with context.DeadlineExceeded, got=%v", err) + } + }) + + t.Run("Context set in options overrides config ctx and all Timeouts", func(t *testing.T) { + mux := http.NewServeMux() + setupApi40Login(mux, foreverValidTestToken, http.StatusOK) + server := httptest.NewServer(mux) + defer server.Close() + + mux.HandleFunc("/api"+apiVersion+path, func(w http.ResponseWriter, r *http.Request) { + time.Sleep(4 * time.Second) + }) + + octx, ocncl := context.WithTimeout(context.Background(), 1*time.Second) + defer ocncl() + + sctx, scncl := context.WithTimeout(context.Background(), 5*time.Second) + defer scncl() + + session := NewAuthSession(ApiSettings{ + BaseUrl: server.URL, + ApiVersion: apiVersion, + Context: sctx, + Timeout: 5, + }) + + options := ApiSettings{ + Timeout: 5, + Context: octx, + } + + err := session.Do(nil, "GET", apiVersion, path, nil, nil, &options) + + if err == nil { + t.Errorf("Do() call did not error/timeout") + } else if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("Do() call did not error with context.DeadlineExceeded, got=%v", err) + } + }) } func TestSetQuery(t *testing.T) { From 1154ed866f5d40aac82250e09d616707e4d07aa0 Mon Sep 17 00:00:00 2001 From: Colin Pistell Date: Fri, 12 Jul 2024 19:22:22 +0000 Subject: [PATCH 4/5] updating timeout comment based on feedback --- go/rtl/auth.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/rtl/auth.go b/go/rtl/auth.go index 0ea7d9fc7..0efa39a73 100644 --- a/go/rtl/auth.go +++ b/go/rtl/auth.go @@ -134,7 +134,7 @@ func (s *AuthSession) Do(result interface{}, method, ver, path string, reqPars m } else if s.Config.Context != nil { ctx = s.Config.Context } else { - // create request context with timeout + // create request context with timeout from options or else config or else 120 seconds var timeoutInSeconds int32 = 120 //seconds if s.Config.Timeout != 0 { timeoutInSeconds = s.Config.Timeout From 673a3ea06c2522a46cbe95a7b941966645daee58 Mon Sep 17 00:00:00 2001 From: Colin Pistell Date: Thu, 29 Aug 2024 17:31:47 +0000 Subject: [PATCH 5/5] set contexts are now used as parent contexts for timeouts --- go/README.md | 10 +++++----- go/rtl/auth.go | 33 +++++++++++++++++---------------- go/rtl/auth_test.go | 4 ++-- 3 files changed, 24 insertions(+), 23 deletions(-) diff --git a/go/README.md b/go/README.md index 4fac37fac..144eb339f 100644 --- a/go/README.md +++ b/go/README.md @@ -120,15 +120,15 @@ func main() { ``` ### Custom Context -If you want even greater control over the lifecycle of the HTTP requests consider providing a [context](https://pkg.go.dev/context). Contexts can be set for all requests or per request. +If you want even greater control over the lifecycle of the HTTP requests consider providing a [context](https://pkg.go.dev/context). Contexts can be set for all requests or per request. They can be combined with the timeout options mentioned in the previous section to fine-tune the lifecycle of your requests. If you wish to include timeout functionality in your custom context then you should leverage [context.WithTimeout](https://pkg.go.dev/context#WithTimeout). -> Note: Setting a context will override any "Timeout" settings that you have applied! +> Note: Custom contexts will be used as the parent context for any timeout you set as specified in the previous section. If the parent context gets cancelled it will propagate to the child context, but if the timeout context times out it does not propagate to the parent context. #### Custom Context for all requests -Follow the example code snippet below if you want all requests to use the same context: +Follow the example code snippet below if you want all requests to use the same parent context: ```go import "context" @@ -146,13 +146,13 @@ func main() { } ``` -> Note: A context set here will also apply to background requests to fetch/refresh oauth tokens, which are normally separated from contexts set via the Timeout property. +> Note: A context set here will become the parent context for all API calls as well as all requests to fetch/refresh oauth tokens, which are normally completely isolated from contexts set via the Timeout property. In this case the token refresh requests and each individual API call will share a common parent context. #### Custom Context per request Follow the example here to set a context for a specific request. -> Note: This will override any "Timeout" settings that you have applied, as well as any context set in the SDK config as outlined in the previus section! +> Note: This will be used as the parent context for any timeout setting you've specified for API calls. If you've set contexts in both your API config and in the request options the request options context will be used instead. Background requests to fetch/refresh oauth tokens will NOT use a context set via request options - it will default to use a generic background context or, if you've also set a context in the API config it will still use that as specified in the previous section. ```go import "context" diff --git a/go/rtl/auth.go b/go/rtl/auth.go index 0efa39a73..855333ce7 100644 --- a/go/rtl/auth.go +++ b/go/rtl/auth.go @@ -128,25 +128,26 @@ func (s *AuthSession) Do(result interface{}, method, ver, path string, reqPars m } } - var ctx context.Context + parent := context.Background() + if s.Config.Context != nil { + parent = s.Config.Context + } if options != nil && options.Context != nil { - ctx = options.Context - } else if s.Config.Context != nil { - ctx = s.Config.Context - } else { - // create request context with timeout from options or else config or else 120 seconds - var timeoutInSeconds int32 = 120 //seconds - if s.Config.Timeout != 0 { - timeoutInSeconds = s.Config.Timeout - } - if options != nil && options.Timeout != 0 { - timeoutInSeconds = options.Timeout - } - var cncl context.CancelFunc - ctx, cncl = context.WithTimeout(context.Background(), time.Second*time.Duration(timeoutInSeconds)) - defer cncl() + parent = options.Context } + // create request context with timeout from options or else config or else 120 seconds + var timeoutInSeconds int32 = 120 + if s.Config.Timeout != 0 { + timeoutInSeconds = s.Config.Timeout + } + if options != nil && options.Timeout != 0 { + timeoutInSeconds = options.Timeout + } + + ctx, cncl := context.WithTimeout(parent, time.Second*time.Duration(timeoutInSeconds)) + defer cncl() + // create new request req, err := http.NewRequestWithContext(ctx, method, u, bytes.NewBufferString(bodyString)) if err != nil { diff --git a/go/rtl/auth_test.go b/go/rtl/auth_test.go index c7a4e0eea..d955cebe3 100644 --- a/go/rtl/auth_test.go +++ b/go/rtl/auth_test.go @@ -724,7 +724,7 @@ func TestAuthSession_Do_Timeout(t *testing.T) { } }) - t.Run("Context set in AuthSession config overrides Timeouts", func(t *testing.T) { + t.Run("Parent context timeout propagates to timeout child context", func(t *testing.T) { mux := http.NewServeMux() setupApi40Login(mux, foreverValidTestToken, http.StatusOK) server := httptest.NewServer(mux) @@ -757,7 +757,7 @@ func TestAuthSession_Do_Timeout(t *testing.T) { } }) - t.Run("Context set in options overrides config ctx and all Timeouts", func(t *testing.T) { + t.Run("Parent context set in options overrides config ctx and propagates to child timout", func(t *testing.T) { mux := http.NewServeMux() setupApi40Login(mux, foreverValidTestToken, http.StatusOK) server := httptest.NewServer(mux)