-
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
Merged
Merged
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
3a96466
feat: option-to-pass-optional-resty-or-http-client
c18d6cb
feat: fixed-linter
6820695
feat: added-new-option-functions-in-type-safe
988e197
feat: prevent-setting-logger-and-default-options-on-configured-custom…
1d5dda4
feat: reverted-logger-excluded-from-custom-client-and-added-tests
2e27b65
feat: throw-panic-with-multiple-custom-clients-and-conflicting-options
Zaimwa9 4030c20
feat: removed-unnecessary-new-line
Zaimwa9 99fcd81
feat: added-http-client-tests-to-actions
Zaimwa9 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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,140 @@ 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 TestRestyClientOverridesHTTPClient(t *testing.T) { | ||
| ctx := context.Background() | ||
|
|
||
| var customHeader string | ||
| server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { | ||
| customHeader = req.Header.Get("X-Test-Client") | ||
| rw.Header().Set("Content-Type", "application/json") | ||
| rw.WriteHeader(http.StatusOK) | ||
| _, err := io.WriteString(rw, fixtures.FlagsJson) | ||
| assert.NoError(t, err) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| httpClient := &http.Client{ | ||
| Transport: roundTripperWithHeader("X-Test-Client", "http"), | ||
| } | ||
|
|
||
| restyClient := resty.New(). | ||
| SetHeader("X-Test-Client", "resty") | ||
|
|
||
| client := flagsmith.NewClient(fixtures.EnvironmentAPIKey, | ||
| flagsmith.WithHTTPClient(httpClient), | ||
| 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.Equal(t, "resty", customHeader, "Expected resty header") | ||
| } | ||
|
|
||
| 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())) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Basically here I detect client related options based on names. They need to be ran first because some options requires the client
e.g
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 comment
The 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
Uh oh!
There was an error while loading. Please reload this page.
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.
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
but it could make sense to have a upper limit too (15sec) ?
Wdyt ?
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Done, so now: