-
Notifications
You must be signed in to change notification settings - Fork 17
feat: option-to-pass-optional-resty-or-http-client #161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3a96466
c18d6cb
6820695
988e197
1d5dda4
2e27b65
4030c20
99fcd81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,9 @@ import ( | |
| "context" | ||
| "fmt" | ||
| "log/slog" | ||
| "net/http" | ||
| "reflect" | ||
| "runtime" | ||
| "strings" | ||
| "sync/atomic" | ||
| "time" | ||
|
|
@@ -34,6 +37,7 @@ type Client struct { | |
| defaultFlagHandler func(string) (Flag, error) | ||
|
|
||
| client *resty.Client | ||
| httpClient *http.Client | ||
| ctxLocalEval context.Context | ||
| ctxAnalytics context.Context | ||
| log *slog.Logger | ||
|
|
@@ -52,26 +56,64 @@ func GetEvaluationContextFromCtx(ctx context.Context) (ec EvaluationContext, ok | |
| return ec, ok | ||
| } | ||
|
|
||
| func getOptionQualifiedName(opt Option) string { | ||
| return runtime.FuncForPC(reflect.ValueOf(opt).Pointer()).Name() | ||
| } | ||
|
|
||
| func isClientOption(name string) bool { | ||
| return strings.Contains(name, OptionWithHTTPClient) || strings.Contains(name, OptionWithRestyClient) | ||
| } | ||
|
|
||
| // NewClient creates instance of Client with given configuration. | ||
| func NewClient(apiKey string, options ...Option) *Client { | ||
| c := &Client{ | ||
| apiKey: apiKey, | ||
| config: defaultConfig(), | ||
| client: resty.New(), | ||
| } | ||
|
|
||
| customClientCount := 0 | ||
| for _, opt := range options { | ||
| name := getOptionQualifiedName(opt) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically here I detect client related options based on names. They need to be ran first because some options requires the client A clean alternative would be to change the option type like It's quite a big refacto so let me know if it's worth going into this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do we think about throwing an error if someone sets a custom client with options like timeout, etc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. IMO timeout could be part of their custom client options that some users might like to keep. I added a guard if no timeout specified here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I meant the user should not be able to set conflicting options. For example, if they are setting an HTTP client, they should not be able to set a Resty client and vice versa. Additionally, if they are passing a custom client, they should not be able to use any options that modify the client. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, so now:
|
||
| if isClientOption(name) { | ||
| customClientCount = customClientCount + 1 | ||
| if customClientCount > 1 { | ||
| panic("Only one client option can be provided") | ||
| } | ||
| opt(c) | ||
| } | ||
| } | ||
|
|
||
| // If a resty custom client has been provided, client is already set - otherwise we use a custom http client or default to a resty | ||
| if c.client == nil { | ||
| if c.httpClient != nil { | ||
| c.client = resty.NewWithClient(c.httpClient) | ||
| c.config.userProvidedClient = true | ||
| } else { | ||
| c.client = resty.New() | ||
| } | ||
| } else { | ||
| c.config.userProvidedClient = true | ||
| } | ||
|
|
||
| c.client.SetHeaders(map[string]string{ | ||
| "Accept": "application/json", | ||
| EnvironmentKeyHeader: c.apiKey, | ||
| }) | ||
| c.client.SetTimeout(c.config.timeout) | ||
|
|
||
| if c.client.GetClient().Timeout == 0 { | ||
| c.client.SetTimeout(c.config.timeout) | ||
| } | ||
|
|
||
| c.log = createLogger() | ||
|
|
||
| for _, opt := range options { | ||
| if opt != nil { | ||
| opt(c) | ||
| name := getOptionQualifiedName(opt) | ||
| if isClientOption(name) { | ||
| continue | ||
| } | ||
| opt(c) | ||
| } | ||
|
|
||
| c.client = c.client. | ||
| SetLogger(newSlogToRestyAdapter(c.log)). | ||
| OnBeforeRequest(newRestyLogRequestMiddleware(c.log)). | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| //go:build test | ||
|
|
||
| package flagsmith | ||
|
|
||
| import ( | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/go-resty/resty/v2" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func (c *Client) ExposeRestyClient() *resty.Client { | ||
| return c.client | ||
| } | ||
|
|
||
| func TestCustomClientRetriesAreSet(t *testing.T) { | ||
| retryCount := 5 | ||
|
|
||
| customResty := resty.New(). | ||
| SetRetryCount(retryCount). | ||
| SetRetryWaitTime(10 * time.Millisecond) | ||
|
|
||
| client := NewClient("env-key", WithRestyClient(customResty)) | ||
|
|
||
| internal := client.ExposeRestyClient() | ||
| assert.Equal(t, retryCount, internal.RetryCount) | ||
| assert.Equal(t, 10*time.Millisecond, internal.RetryWaitTime) | ||
| } | ||
|
|
||
| func TestCustomRestyClientTimeoutIsNotOverriddenWithDefaultTimeout(t *testing.T) { | ||
| customResty := resty.New().SetTimeout(13 * time.Millisecond) | ||
|
|
||
| client := NewClient("env-key", WithRestyClient(customResty)) | ||
|
|
||
| internal := client.ExposeRestyClient() | ||
|
|
||
| assert.Equal(t, 13*time.Millisecond, internal.GetClient().Timeout) | ||
| } | ||
|
|
||
| func TestCustomRestyClientHasDefaultTimeoutIfNotProvided(t *testing.T) { | ||
| customResty := resty.New() | ||
|
|
||
| client := NewClient("env-key", WithRestyClient(customResty)) | ||
|
|
||
| internal := client.ExposeRestyClient() | ||
| assert.Equal(t, 10*time.Second, internal.GetClient().Timeout) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ import ( | |
|
|
||
| flagsmith "github.com/Flagsmith/flagsmith-go-client/v4" | ||
| "github.com/Flagsmith/flagsmith-go-client/v4/fixtures" | ||
| "github.com/go-resty/resty/v2" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
|
|
@@ -1019,3 +1020,160 @@ type writerFunc func(p []byte) (n int, err error) | |
| func (f writerFunc) Write(p []byte) (n int, err error) { | ||
| return f(p) | ||
| } | ||
|
|
||
| // Helper function to implement a header interceptor. | ||
| func roundTripperWithHeader(key, value string) http.RoundTripper { | ||
| return &injectHeaderTransport{key: key, value: value} | ||
| } | ||
|
|
||
| type injectHeaderTransport struct { | ||
| key string | ||
| value string | ||
| } | ||
|
|
||
| func (t *injectHeaderTransport) RoundTrip(req *http.Request) (*http.Response, error) { | ||
| req.Header.Set(t.key, t.value) | ||
| return http.DefaultTransport.RoundTrip(req) | ||
| } | ||
|
|
||
| func TestCustomHTTPClientIsUsed(t *testing.T) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: We follow the Given-When-Then comment pattern, which you are already using. However, it would be great if we explicitly mark those sections using comments. |
||
| ctx := context.Background() | ||
|
|
||
| hasCustomHeader := false | ||
| server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { | ||
| assert.Equal(t, "/api/v1/flags/", req.URL.Path) | ||
| assert.Equal(t, fixtures.EnvironmentAPIKey, req.Header.Get("x-Environment-Key")) | ||
| if req.Header.Get("X-Test-Client") == "http" { | ||
| hasCustomHeader = true | ||
| } | ||
| rw.Header().Set("Content-Type", "application/json") | ||
| rw.WriteHeader(http.StatusOK) | ||
| _, err := io.WriteString(rw, fixtures.FlagsJson) | ||
| assert.NoError(t, err) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| customClient := &http.Client{ | ||
| Transport: roundTripperWithHeader("X-Test-Client", "http"), | ||
| } | ||
|
|
||
| client := flagsmith.NewClient(fixtures.EnvironmentAPIKey, | ||
| flagsmith.WithHTTPClient(customClient), | ||
| flagsmith.WithBaseURL(server.URL+"/api/v1/")) | ||
|
|
||
| flags, err := client.GetFlags(ctx, nil) | ||
| assert.Equal(t, 1, len(flags.AllFlags())) | ||
| assert.NoError(t, err) | ||
| assert.True(t, hasCustomHeader, "Expected http header") | ||
| flag, err := flags.GetFlag(fixtures.Feature1Name) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, fixtures.Feature1Value, flag.Value) | ||
| } | ||
|
|
||
| func TestCustomRestyClientIsUsed(t *testing.T) { | ||
| ctx := context.Background() | ||
|
|
||
| hasCustomHeader := false | ||
| server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { | ||
| if req.Header.Get("X-Custom-Test-Header") == "resty" { | ||
| hasCustomHeader = true | ||
| } | ||
| rw.Header().Set("Content-Type", "application/json") | ||
| rw.WriteHeader(http.StatusOK) | ||
| _, err := io.WriteString(rw, fixtures.FlagsJson) | ||
| assert.NoError(t, err) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| restyClient := resty.New(). | ||
| SetHeader("X-Custom-Test-Header", "resty") | ||
|
|
||
| client := flagsmith.NewClient(fixtures.EnvironmentAPIKey, | ||
| flagsmith.WithRestyClient(restyClient), | ||
| flagsmith.WithBaseURL(server.URL+"/api/v1/")) | ||
|
|
||
| flags, err := client.GetFlags(ctx, nil) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, 1, len(flags.AllFlags())) | ||
| assert.True(t, hasCustomHeader, "Expected custom resty header") | ||
| } | ||
|
|
||
| func TestRestyClientOverridesHTTPClientShouldPanic(t *testing.T) { | ||
| httpClient := &http.Client{ | ||
| Transport: roundTripperWithHeader("X-Test-Client", "http"), | ||
| } | ||
|
|
||
| restyClient := resty.New(). | ||
| SetHeader("X-Test-Client", "resty") | ||
|
|
||
| assert.Panics(t, func() { | ||
| _ = flagsmith.NewClient(fixtures.EnvironmentAPIKey, | ||
| flagsmith.WithHTTPClient(httpClient), | ||
| flagsmith.WithRestyClient(restyClient), | ||
| flagsmith.WithBaseURL("http://example.com/api/v1/")) | ||
| }, "Expected panic when both HTTP and Resty clients are provided") | ||
| } | ||
|
|
||
| func TestDefaultRestyClientIsUsed(t *testing.T) { | ||
| ctx := context.Background() | ||
|
|
||
| serverCalled := false | ||
|
|
||
| server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { | ||
| serverCalled = true | ||
|
|
||
| assert.Equal(t, "/api/v1/flags/", req.URL.Path) | ||
| assert.Equal(t, fixtures.EnvironmentAPIKey, req.Header.Get("x-Environment-Key")) | ||
|
|
||
| rw.Header().Set("Content-Type", "application/json") | ||
| rw.WriteHeader(http.StatusOK) | ||
| _, err := io.WriteString(rw, fixtures.FlagsJson) | ||
| assert.NoError(t, err) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| client := flagsmith.NewClient(fixtures.EnvironmentAPIKey, | ||
| flagsmith.WithBaseURL(server.URL+"/api/v1/")) | ||
|
|
||
| flags, err := client.GetFlags(ctx, nil) | ||
|
|
||
| assert.NoError(t, err) | ||
| assert.True(t, serverCalled, "Expected server to be") | ||
| assert.Equal(t, 1, len(flags.AllFlags())) | ||
| } | ||
|
|
||
| func TestCustomClientOptionsShoudPanic(t *testing.T) { | ||
| restyClient := resty.New() | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| option flagsmith.Option | ||
| }{ | ||
| { | ||
| name: "WithRequestTimeout", | ||
| option: flagsmith.WithRequestTimeout(5 * time.Second), | ||
| }, | ||
| { | ||
| name: "WithRetries", | ||
| option: flagsmith.WithRetries(3, time.Second), | ||
| }, | ||
| { | ||
| name: "WithCustomHeaders", | ||
| option: flagsmith.WithCustomHeaders(map[string]string{"X-Custom": "value"}), | ||
| }, | ||
| { | ||
| name: "WithProxy", | ||
| option: flagsmith.WithProxy("http://proxy.example.com"), | ||
| }, | ||
| } | ||
|
|
||
| for _, test := range testCases { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| assert.Panics(t, func() { | ||
| _ = flagsmith.NewClient(fixtures.EnvironmentAPIKey, | ||
| flagsmith.WithRestyClient(restyClient), | ||
| test.option) | ||
| }, "Expected panic when using %s with custom resty client", test.name) | ||
| }) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this. Specific test cases in which I re-expose the resty client to test the values