From 299546d1e4919e2d0e51cf37624aff3e413e78e5 Mon Sep 17 00:00:00 2001 From: Ajinkya Suryawanshi Date: Wed, 25 Sep 2024 14:23:57 -0700 Subject: [PATCH 1/4] feat: Add pollErrorHandler to handle pollEnvironment errors --- client.go | 14 +++++++++----- client_export_test.go | 7 +++++++ client_test.go | 32 ++++++++++++++++++++++++++++++++ options.go | 7 +++++++ 4 files changed, 55 insertions(+), 5 deletions(-) create mode 100644 client_export_test.go diff --git a/client.go b/client.go index 6bcdb5f..ea94e9a 100644 --- a/client.go +++ b/client.go @@ -26,11 +26,12 @@ type Client struct { analyticsProcessor *AnalyticsProcessor defaultFlagHandler func(string) (Flag, error) - client *resty.Client - ctxLocalEval context.Context - ctxAnalytics context.Context - log Logger - offlineHandler OfflineHandler + client *resty.Client + ctxLocalEval context.Context + ctxAnalytics context.Context + log Logger + offlineHandler OfflineHandler + pollErrorHandler func(error) } // NewClient creates instance of Client with given configuration. @@ -244,6 +245,9 @@ func (c *Client) pollEnvironment(ctx context.Context) { err := c.UpdateEnvironment(ctx) if err != nil { c.log.Errorf("Failed to update environment: %v", err) + if c.pollErrorHandler != nil { + c.pollErrorHandler(err) + } } } update() diff --git a/client_export_test.go b/client_export_test.go new file mode 100644 index 0000000..92bf042 --- /dev/null +++ b/client_export_test.go @@ -0,0 +1,7 @@ +package flagsmith + +import "context" + +func PollEnvironment(client *Client, ctx context.Context) { + client.pollEnvironment(ctx) +} diff --git a/client_test.go b/client_test.go index 4a7739c..b830717 100644 --- a/client_test.go +++ b/client_test.go @@ -675,3 +675,35 @@ func TestOfflineHandlerIsUsedWhenRequestFails(t *testing.T) { assert.Equal(t, fixtures.Feature1ID, allFlags[0].FeatureID) assert.Equal(t, fixtures.Feature1Value, allFlags[0].Value) } + +func TestPollErrorHandlerIsUsedWhenPollFails(t *testing.T) { + // Given + ctx := context.Background() + expectedErrorMessage := "flagsmith: unexpected response from Flagsmith API: 500 Internal Server Error" + var capturedError error + + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + rw.WriteHeader(http.StatusInternalServerError) + })) + defer server.Close() + + // When + client := flagsmith.NewClient(fixtures.EnvironmentAPIKey, + flagsmith.WithBaseURL(server.URL+"/api/v1/"), + flagsmith.WithEnvironmentRefreshInterval(time.Duration(2)*time.Second), + flagsmith.WithPollErrorHandler(func(err error) { + capturedError = err + }), + ) + + // when + go func() { flagsmith.PollEnvironment(client, ctx) }() + + // stop method in 2 seconds + time.Sleep(1 * time.Second) + ctx.Done() + + // Then + assert.NotNil(t, capturedError) + assert.Contains(t, capturedError.Error(), expectedErrorMessage) +} diff --git a/options.go b/options.go index 13660df..482a66d 100644 --- a/options.go +++ b/options.go @@ -117,3 +117,10 @@ func WithOfflineMode() Option { c.config.offlineMode = true } } + +// WithPollErrorHandler provides a way to handle errors that occur during polling of environment +func WithPollErrorHandler(handler func(err error)) Option { + return func(c *Client) { + c.pollErrorHandler = handler + } +} From 54ff6e5125e1a887a6b44b6c0b23911fc95babdc Mon Sep 17 00:00:00 2001 From: Ajinkya Suryawanshi Date: Thu, 26 Sep 2024 10:53:52 -0700 Subject: [PATCH 2/4] Review Feedback * Remove handler from pollEnvironment to UpdateEnvironment * Rename handler to errorHandler * Create a struct FlagsmithErrorHandler so that we can return error and other client response details --- client.go | 27 ++++++++++++++++----------- client_export_test.go | 7 ------- client_test.go | 30 +++++++++++++++--------------- errors.go | 6 ++++++ options.go | 10 +++++----- 5 files changed, 42 insertions(+), 38 deletions(-) delete mode 100644 client_export_test.go diff --git a/client.go b/client.go index ea94e9a..355677b 100644 --- a/client.go +++ b/client.go @@ -26,12 +26,12 @@ type Client struct { analyticsProcessor *AnalyticsProcessor defaultFlagHandler func(string) (Flag, error) - client *resty.Client - ctxLocalEval context.Context - ctxAnalytics context.Context - log Logger - offlineHandler OfflineHandler - pollErrorHandler func(error) + client *resty.Client + ctxLocalEval context.Context + ctxAnalytics context.Context + log Logger + offlineHandler OfflineHandler + errorHandler func(handler FlagsmithErrorHandler) } // NewClient creates instance of Client with given configuration. @@ -245,9 +245,6 @@ func (c *Client) pollEnvironment(ctx context.Context) { err := c.UpdateEnvironment(ctx) if err != nil { c.log.Errorf("Failed to update environment: %v", err) - if c.pollErrorHandler != nil { - c.pollErrorHandler(err) - } } } update() @@ -271,10 +268,18 @@ func (c *Client) UpdateEnvironment(ctx context.Context) error { Get(c.config.baseURL + "environment-document/") if err != nil { - return &FlagsmithAPIError{msg: fmt.Sprintf("flagsmith: error performing request to Flagsmith API: %s", err)} + f := &FlagsmithAPIError{msg: fmt.Sprintf("flagsmith: error performing request to Flagsmith API: %s", err)} + if c.errorHandler != nil { + c.errorHandler(FlagsmithErrorHandler{err, resp.StatusCode(), resp.Status()}) + } + return f } if resp.StatusCode() != 200 { - return &FlagsmithAPIError{msg: fmt.Sprintf("flagsmith: unexpected response from Flagsmith API: %s", resp.Status())} + f := &FlagsmithAPIError{msg: fmt.Sprintf("flagsmith: unexpected response from Flagsmith API: %s", resp.Status())} + if c.errorHandler != nil { + c.errorHandler(FlagsmithErrorHandler{err, resp.StatusCode(), resp.Status()}) + } + return f } c.environment.Store(&env) identitiesWithOverrides := make(map[string]identities.IdentityModel) diff --git a/client_export_test.go b/client_export_test.go deleted file mode 100644 index 92bf042..0000000 --- a/client_export_test.go +++ /dev/null @@ -1,7 +0,0 @@ -package flagsmith - -import "context" - -func PollEnvironment(client *Client, ctx context.Context) { - client.pollEnvironment(ctx) -} diff --git a/client_test.go b/client_test.go index b830717..43c3c4e 100644 --- a/client_test.go +++ b/client_test.go @@ -677,10 +677,11 @@ func TestOfflineHandlerIsUsedWhenRequestFails(t *testing.T) { } func TestPollErrorHandlerIsUsedWhenPollFails(t *testing.T) { - // Given - ctx := context.Background() - expectedErrorMessage := "flagsmith: unexpected response from Flagsmith API: 500 Internal Server Error" - var capturedError error + // Given + ctx := context.Background() + var capturedError error + var statusCode int + var status string server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { rw.WriteHeader(http.StatusInternalServerError) @@ -691,19 +692,18 @@ func TestPollErrorHandlerIsUsedWhenPollFails(t *testing.T) { client := flagsmith.NewClient(fixtures.EnvironmentAPIKey, flagsmith.WithBaseURL(server.URL+"/api/v1/"), flagsmith.WithEnvironmentRefreshInterval(time.Duration(2)*time.Second), - flagsmith.WithPollErrorHandler(func(err error) { - capturedError = err + flagsmith.WithErrorHandler(func(handler flagsmith.FlagsmithErrorHandler) { + capturedError = handler.Err + statusCode = handler.ResponseStatusCode + status = handler.ResponseStatus }), ) - // when - go func() { flagsmith.PollEnvironment(client, ctx) }() + // when + _ = client.UpdateEnvironment(ctx) - // stop method in 2 seconds - time.Sleep(1 * time.Second) - ctx.Done() - - // Then - assert.NotNil(t, capturedError) - assert.Contains(t, capturedError.Error(), expectedErrorMessage) + // Then + assert.Equal(t, capturedError, nil) + assert.Equal(t, statusCode, 500) + assert.Equal(t, status, "500 Internal Server Error") } diff --git a/errors.go b/errors.go index dc15538..9b956a3 100644 --- a/errors.go +++ b/errors.go @@ -8,6 +8,12 @@ type FlagsmithAPIError struct { msg string } +type FlagsmithErrorHandler struct { + Err error + ResponseStatusCode int + ResponseStatus string +} + func (e FlagsmithClientError) Error() string { return e.msg } diff --git a/options.go b/options.go index 482a66d..acff841 100644 --- a/options.go +++ b/options.go @@ -118,9 +118,9 @@ func WithOfflineMode() Option { } } -// WithPollErrorHandler provides a way to handle errors that occur during polling of environment -func WithPollErrorHandler(handler func(err error)) Option { - return func(c *Client) { - c.pollErrorHandler = handler - } +// WithErrorHandler provides a way to handle errors that occur during update of an environment +func WithErrorHandler(handler func(handler FlagsmithErrorHandler)) Option { + return func(c *Client) { + c.errorHandler = handler + } } From 42fdaa01e78b2b9986c3430398a1ac375d210e78 Mon Sep 17 00:00:00 2001 From: Ajinkya Suryawanshi Date: Fri, 27 Sep 2024 10:00:49 -0700 Subject: [PATCH 3/4] Address review comments - use existing FlagsmithAPIError --- client.go | 36 +++++++++++++++++++++++------------- client_test.go | 3 +-- errors.go | 7 ++----- options.go | 2 +- 4 files changed, 27 insertions(+), 21 deletions(-) diff --git a/client.go b/client.go index 355677b..53aefeb 100644 --- a/client.go +++ b/client.go @@ -31,7 +31,7 @@ type Client struct { ctxAnalytics context.Context log Logger offlineHandler OfflineHandler - errorHandler func(handler FlagsmithErrorHandler) + errorHandler func(handler *FlagsmithAPIError) } // NewClient creates instance of Client with given configuration. @@ -148,7 +148,8 @@ func (c *Client) GetIdentitySegments(identifier string, traits []*Trait) ([]*seg // NOTE: This method only works with Edge API endpoint. func (c *Client) BulkIdentify(ctx context.Context, batch []*IdentityTraits) error { if len(batch) > bulkIdentifyMaxCount { - return &FlagsmithAPIError{msg: fmt.Sprintf("flagsmith: batch size must be less than %d", bulkIdentifyMaxCount)} + msg := fmt.Sprintf("flagsmith: batch size must be less than %d", bulkIdentifyMaxCount) + return &FlagsmithAPIError{Msg: msg} } body := struct { @@ -161,13 +162,16 @@ func (c *Client) BulkIdentify(ctx context.Context, batch []*IdentityTraits) erro ForceContentType("application/json"). Post(c.config.baseURL + "bulk-identities/") if resp.StatusCode() == 404 { - return &FlagsmithAPIError{msg: "flagsmith: Bulk identify endpoint not found; Please make sure you are using Edge API endpoint"} + msg := "flagsmith: Bulk identify endpoint not found; Please make sure you are using Edge API endpoint" + return &FlagsmithAPIError{Msg: msg, Err: err, ResponseStatusCode: resp.StatusCode(), ResponseStatus: resp.Status()} } if err != nil { - return &FlagsmithAPIError{msg: fmt.Sprintf("flagsmith: error performing request to Flagsmith API: %s", err)} + msg := fmt.Sprintf("flagsmith: error performing request to Flagsmith API: %s", err) + return &FlagsmithAPIError{Msg: msg, Err: err, ResponseStatusCode: resp.StatusCode(), ResponseStatus: resp.Status()} } if !resp.IsSuccess() { - return &FlagsmithAPIError{msg: fmt.Sprintf("flagsmith: unexpected response from Flagsmith API: %s", resp.Status())} + msg := fmt.Sprintf("flagsmith: unexpected response from Flagsmith API: %s", resp.Status()) + return &FlagsmithAPIError{Msg: msg, Err: err, ResponseStatusCode: resp.StatusCode(), ResponseStatus: resp.Status()} } return nil } @@ -180,10 +184,12 @@ func (c *Client) GetEnvironmentFlagsFromAPI(ctx context.Context) (Flags, error) ForceContentType("application/json"). Get(c.config.baseURL + "flags/") if err != nil { - return Flags{}, &FlagsmithAPIError{msg: fmt.Sprintf("flagsmith: error performing request to Flagsmith API: %s", err)} + msg := fmt.Sprintf("flagsmith: error performing request to Flagsmith API: %s", err) + return Flags{}, &FlagsmithAPIError{Msg: msg, Err: err, ResponseStatusCode: resp.StatusCode(), ResponseStatus: resp.Status()} } if !resp.IsSuccess() { - return Flags{}, &FlagsmithAPIError{msg: fmt.Sprintf("flagsmith: unexpected response from Flagsmith API: %s", resp.Status())} + msg := fmt.Sprintf("flagsmith: unexpected response from Flagsmith API: %s", resp.Status()) + return Flags{}, &FlagsmithAPIError{Msg: msg, Err: err, ResponseStatusCode: resp.StatusCode(), ResponseStatus: resp.Status()} } return makeFlagsFromAPIFlags(resp.Body(), c.analyticsProcessor, c.defaultFlagHandler) } @@ -201,10 +207,12 @@ func (c *Client) GetIdentityFlagsFromAPI(ctx context.Context, identifier string, ForceContentType("application/json"). Post(c.config.baseURL + "identities/") if err != nil { - return Flags{}, &FlagsmithAPIError{msg: fmt.Sprintf("flagsmith: error performing request to Flagsmith API: %s", err)} + msg := fmt.Sprintf("flagsmith: error performing request to Flagsmith API: %s", err) + return Flags{}, &FlagsmithAPIError{Msg: msg, Err: err, ResponseStatusCode: resp.StatusCode(), ResponseStatus: resp.Status()} } if !resp.IsSuccess() { - return Flags{}, &FlagsmithAPIError{msg: fmt.Sprintf("flagsmith: unexpected response from Flagsmith API: %s", resp.Status())} + msg := fmt.Sprintf("flagsmith: unexpected response from Flagsmith API: %s", resp.Status()) + return Flags{}, &FlagsmithAPIError{Msg: msg, Err: err, ResponseStatusCode: resp.StatusCode(), ResponseStatus: resp.Status()} } return makeFlagsfromIdentityAPIJson(resp.Body(), c.analyticsProcessor, c.defaultFlagHandler) } @@ -268,16 +276,18 @@ func (c *Client) UpdateEnvironment(ctx context.Context) error { Get(c.config.baseURL + "environment-document/") if err != nil { - f := &FlagsmithAPIError{msg: fmt.Sprintf("flagsmith: error performing request to Flagsmith API: %s", err)} + msg := fmt.Sprintf("flagsmith: error performing request to Flagsmith API: %s", err) + f := &FlagsmithAPIError{Msg: msg, Err: err, ResponseStatusCode: resp.StatusCode(), ResponseStatus: resp.Status()} if c.errorHandler != nil { - c.errorHandler(FlagsmithErrorHandler{err, resp.StatusCode(), resp.Status()}) + c.errorHandler(f) } return f } if resp.StatusCode() != 200 { - f := &FlagsmithAPIError{msg: fmt.Sprintf("flagsmith: unexpected response from Flagsmith API: %s", resp.Status())} + msg := fmt.Sprintf("flagsmith: unexpected response from Flagsmith API: %s", resp.Status()) + f := &FlagsmithAPIError{Msg: msg, Err: err, ResponseStatusCode: resp.StatusCode(), ResponseStatus: resp.Status()} if c.errorHandler != nil { - c.errorHandler(FlagsmithErrorHandler{err, resp.StatusCode(), resp.Status()}) + c.errorHandler(f) } return f } diff --git a/client_test.go b/client_test.go index 43c3c4e..cb732ad 100644 --- a/client_test.go +++ b/client_test.go @@ -691,8 +691,7 @@ func TestPollErrorHandlerIsUsedWhenPollFails(t *testing.T) { // When client := flagsmith.NewClient(fixtures.EnvironmentAPIKey, flagsmith.WithBaseURL(server.URL+"/api/v1/"), - flagsmith.WithEnvironmentRefreshInterval(time.Duration(2)*time.Second), - flagsmith.WithErrorHandler(func(handler flagsmith.FlagsmithErrorHandler) { + flagsmith.WithErrorHandler(func(handler *flagsmith.FlagsmithAPIError) { capturedError = handler.Err statusCode = handler.ResponseStatusCode status = handler.ResponseStatus diff --git a/errors.go b/errors.go index 9b956a3..794c062 100644 --- a/errors.go +++ b/errors.go @@ -5,10 +5,7 @@ type FlagsmithClientError struct { } type FlagsmithAPIError struct { - msg string -} - -type FlagsmithErrorHandler struct { + Msg string Err error ResponseStatusCode int ResponseStatus string @@ -19,5 +16,5 @@ func (e FlagsmithClientError) Error() string { } func (e FlagsmithAPIError) Error() string { - return e.msg + return e.Msg } diff --git a/options.go b/options.go index acff841..4bc3eb0 100644 --- a/options.go +++ b/options.go @@ -119,7 +119,7 @@ func WithOfflineMode() Option { } // WithErrorHandler provides a way to handle errors that occur during update of an environment -func WithErrorHandler(handler func(handler FlagsmithErrorHandler)) Option { +func WithErrorHandler(handler func(handler *FlagsmithAPIError)) Option { return func(c *Client) { c.errorHandler = handler } From 880cf36d46d3943bd9ecd4f0d88f7a22decd5c80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20L=C3=B3pez=20Dato?= Date: Mon, 30 Sep 2024 08:54:45 -0300 Subject: [PATCH 4/4] lint --- options.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/options.go b/options.go index 4bc3eb0..c49084b 100644 --- a/options.go +++ b/options.go @@ -118,7 +118,7 @@ func WithOfflineMode() Option { } } -// WithErrorHandler provides a way to handle errors that occur during update of an environment +// WithErrorHandler provides a way to handle errors that occur during update of an environment. func WithErrorHandler(handler func(handler *FlagsmithAPIError)) Option { return func(c *Client) { c.errorHandler = handler