From c9cd26f4967016b08f9944e8257320f720b7e7bb Mon Sep 17 00:00:00 2001 From: Joseph Little Date: Mon, 8 Jul 2024 13:43:36 +0100 Subject: [PATCH] Tidy up and flattening --- httpclient/cookies.go | 2 + httpclient/headers.go | 14 -- httpclient/request.go | 14 +- httpclient/status.go | 4 +- httpclient/utility.go | 9 + ratehandler/{ratehandler.go => main.go} | 16 +- ratehandler/ratehandler_test.go.bk | 101 ---------- redirecthandler/redirecthandler_test.go.TODO | 197 ------------------- 8 files changed, 26 insertions(+), 331 deletions(-) delete mode 100644 httpclient/headers.go rename ratehandler/{ratehandler.go => main.go} (86%) delete mode 100644 ratehandler/ratehandler_test.go.bk delete mode 100644 redirecthandler/redirecthandler_test.go.TODO diff --git a/httpclient/cookies.go b/httpclient/cookies.go index 6a20bf2..74a2111 100644 --- a/httpclient/cookies.go +++ b/httpclient/cookies.go @@ -5,11 +5,13 @@ import ( "net/url" ) +// loadCustomCookies applies the custom cookies supplied in the config and applies them to the http session. func (c *Client) loadCustomCookies() error { cookieJar, err := cookiejar.New(nil) if err != nil { return err } + c.http.Jar = cookieJar cookieUrl, err := url.Parse((*c.Integration).GetFQDN()) diff --git a/httpclient/headers.go b/httpclient/headers.go deleted file mode 100644 index 34b77fc..0000000 --- a/httpclient/headers.go +++ /dev/null @@ -1,14 +0,0 @@ -// headers/headers.go -package httpclient - -import ( - "net/http" -) - -// CheckDeprecationHeader checks the response headers for the Deprecation header and logs a warning if present. -func (c *Client) CheckDeprecationHeader(resp *http.Response) { - deprecationHeader := resp.Header.Get("Deprecation") - if deprecationHeader != "" { - c.Sugar.Warn("API endpoint is deprecated", deprecationHeader, resp.Request.URL.String()) - } -} diff --git a/httpclient/request.go b/httpclient/request.go index 7eda666..5bedbe0 100644 --- a/httpclient/request.go +++ b/httpclient/request.go @@ -113,17 +113,18 @@ func (c *Client) DoRequest(method, endpoint string, body, out interface{}) (*htt // - The retry mechanism employs exponential backoff with jitter to mitigate the impact of retries on the server. // endregion func (c *Client) executeRequestWithRetries(method, endpoint string, body interface{}) (*http.Response, error) { - ctx := context.Background() - totalRetryDeadline := time.Now().Add(c.config.TotalRetryDuration) - var resp *http.Response var err error var retryCount int + ctx := context.Background() + c.Sugar.Debug("Executing request with retries", zap.String("method", method), zap.String("endpoint", endpoint)) // TODO removed the blocked comments + // Simplify this? // Timer + totalRetryDeadline := time.Now().Add(c.config.TotalRetryDuration) for time.Now().Before(totalRetryDeadline) { // Resp @@ -133,21 +134,22 @@ func (c *Client) executeRequestWithRetries(method, endpoint string, body interfa } // Success - if resp.StatusCode >= 200 && resp.StatusCode < 400 { - if resp.StatusCode >= 300 { + if resp.StatusCode >= http.StatusOK && resp.StatusCode < http.StatusBadRequest { + if resp.StatusCode == http.StatusPermanentRedirect || resp.StatusCode == http.StatusTemporaryRedirect { c.Sugar.Warn("Redirect response received", zap.Int("status_code", resp.StatusCode), zap.String("location", resp.Header.Get("Location"))) } c.Sugar.Infof("%s request successful at %v", resp.Request.Method, resp.Request.URL) return resp, nil } + // Message statusMessage := http.StatusText(resp.StatusCode) if statusMessage == "" { statusMessage = "unknown response code" } // Non Retry - if IsNonRetryableStatusCode(resp) { + if IsNonRetryableStatusCode(resp.StatusCode) { c.Sugar.Warn("Non-retryable error received", zap.Int("status_code", resp.StatusCode), zap.String("status_message", statusMessage)) return resp, response.HandleAPIErrorResponse(resp, c.Sugar) } diff --git a/httpclient/status.go b/httpclient/status.go index 98848ac..5e84211 100644 --- a/httpclient/status.go +++ b/httpclient/status.go @@ -3,7 +3,7 @@ package httpclient import "net/http" // IsNonRetryableStatusCode checks if the provided response indicates a non-retryable error. -func IsNonRetryableStatusCode(resp *http.Response) bool { +func IsNonRetryableStatusCode(statusCode int) bool { nonRetryableStatusCodes := map[int]bool{ http.StatusBadRequest: true, // 400 - Bad Request http.StatusUnauthorized: true, // 401 - Unauthorized @@ -31,7 +31,7 @@ func IsNonRetryableStatusCode(resp *http.Response) bool { http.StatusUnavailableForLegalReasons: true, // 451 - Unavailable For Legal Reasons } - _, isNonRetryable := nonRetryableStatusCodes[resp.StatusCode] + _, isNonRetryable := nonRetryableStatusCodes[statusCode] return isNonRetryable } diff --git a/httpclient/utility.go b/httpclient/utility.go index 2852ca9..19e331f 100644 --- a/httpclient/utility.go +++ b/httpclient/utility.go @@ -4,6 +4,7 @@ package httpclient import ( "errors" "fmt" + "net/http" "os" "path/filepath" "regexp" @@ -152,3 +153,11 @@ func setDefaultDuration(field *time.Duration, defaultValue time.Duration) { *field = defaultValue } } + +// CheckDeprecationHeader checks the response headers for the Deprecation header and logs a warning if present. +func (c *Client) CheckDeprecationHeader(resp *http.Response) { + deprecationHeader := resp.Header.Get("Deprecation") + if deprecationHeader != "" { + c.Sugar.Warn("API endpoint is deprecated", deprecationHeader, resp.Request.URL.String()) + } +} diff --git a/ratehandler/ratehandler.go b/ratehandler/main.go similarity index 86% rename from ratehandler/ratehandler.go rename to ratehandler/main.go index b4bb04a..a1e0cff 100644 --- a/ratehandler/ratehandler.go +++ b/ratehandler/main.go @@ -40,11 +40,8 @@ const ( // The jitterFactor adds randomness to the delay to avoid simultaneous retries (thundering herd problem). // The delay is capped at maxDelay to prevent excessive wait times. func CalculateBackoff(retry int) time.Duration { - if retry < 0 { - retry = 0 // Ensure non-negative retry count - } - delay := float64(baseDelay) * math.Pow(2, float64(retry)) + jitter := (rand.Float64() - 0.5) * jitterFactor * 2.0 // Random value between -jitterFactor and +jitterFactor delayWithJitter := delay * (1.0 + jitter) @@ -57,32 +54,29 @@ func CalculateBackoff(retry int) time.Duration { // ParseRateLimitHeaders parses common rate limit headers and adjusts behavior accordingly. // It handles both Retry-After (in seconds or HTTP-date format) and X-RateLimit-Reset headers. func ParseRateLimitHeaders(resp *http.Response, logger *zap.SugaredLogger) time.Duration { - // Check for the Retry-After header in seconds if retryAfter := resp.Header.Get("Retry-After"); retryAfter != "" { if waitSeconds, err := strconv.Atoi(retryAfter); err == nil { return time.Duration(waitSeconds) * time.Second + } else if retryAfterDate, err := time.Parse(time.RFC1123, retryAfter); err == nil { - // Handle HTTP-date format in Retry-After return time.Until(retryAfterDate) + } else { logger.Debug("Unable to parse Retry-After header", zap.String("value", retryAfter), zap.Error(err)) } } - // Check for X-RateLimit-Remaining; if it's 0, use X-RateLimit-Reset to determine how long to wait if remaining := resp.Header.Get("X-RateLimit-Remaining"); remaining == "0" { if resetTimeStr := resp.Header.Get("X-RateLimit-Reset"); resetTimeStr != "" { if resetTimeEpoch, err := strconv.ParseInt(resetTimeStr, 10, 64); err == nil { resetTime := time.Unix(resetTimeEpoch, 0) - // Add a buffer to account for potential clock skew - const skewBuffer = 5 * time.Second - return time.Until(resetTime) + skewBuffer + return time.Until(resetTime) + (5 * time.Second) + } else { logger.Debug("Unable to parse X-RateLimit-Reset header", zap.String("value", resetTimeStr), zap.Error(err)) } } } - // No relevant rate limiting headers found, return 0 return 0 } diff --git a/ratehandler/ratehandler_test.go.bk b/ratehandler/ratehandler_test.go.bk deleted file mode 100644 index 3443649..0000000 --- a/ratehandler/ratehandler_test.go.bk +++ /dev/null @@ -1,101 +0,0 @@ -// ratehandler/ratehandler.go -package ratehandler - -import ( - "net/http" - "strconv" - "testing" - "time" - - "github.com/deploymenttheory/go-api-http-client/mocklogger" - "github.com/stretchr/testify/assert" -) - -// TestCalculateBackoff tests the backoff calculation for various retry counts -func TestCalculateBackoff(t *testing.T) { - tests := []struct { - retry int - expectedMin time.Duration - expectedMax time.Duration - }{ - {retry: 0, expectedMin: time.Duration(float64(baseDelay) * (1 - jitterFactor)), expectedMax: maxDelay}, - {retry: 1, expectedMin: time.Duration(float64(baseDelay*2) * (1 - jitterFactor)), expectedMax: maxDelay}, - {retry: 2, expectedMin: time.Duration(float64(baseDelay*4) * (1 - jitterFactor)), expectedMax: maxDelay}, - {retry: 5, expectedMin: time.Duration(float64(baseDelay*32) * (1 - jitterFactor)), expectedMax: maxDelay}, - } - - for _, tt := range tests { - t.Run("RetryCount"+strconv.Itoa(tt.retry), func(t *testing.T) { - delay := CalculateBackoff(tt.retry) - - // The delay should be within the expected range - assert.GreaterOrEqual(t, delay, tt.expectedMin, "Delay should be greater than or equal to expected minimum after jitter adjustment") - assert.LessOrEqual(t, delay, tt.expectedMax, "Delay should be less than or equal to expected maximum") - }) - } -} - -// TestParseRateLimitHeaders evaluates the functionality of the parseRateLimitHeaders function, -// ensuring it correctly interprets various rate-limiting headers from an HTTP response and calculates -// the appropriate wait duration. The function tests different scenarios including 'Retry-After' headers -// with both date and delay values, 'X-RateLimit-Reset' headers indicating the reset time for rate limiting, -// and situations where no relevant headers are present. Each test case mimics an HTTP response with specific -// headers set, and asserts that the calculated wait duration falls within an acceptable range of the expected -// value, allowing for slight variances due to execution time and rounding. The use of a mock logger ensures -// that the function's logging behavior can also be verified without affecting the output of the test runner. -func TestParseRateLimitHeaders(t *testing.T) { - tests := []struct { - name string - headers map[string]string - expectedWait time.Duration - }{ - { - name: "RetryAfterInSeconds", - headers: map[string]string{ - "Retry-After": "120", // 2 minutes in seconds - }, - expectedWait: 2 * time.Minute, - }, - { - name: "RetryAfterHTTPDate", - headers: map[string]string{ - "Retry-After": time.Now().UTC().Format(time.RFC1123), // Use current time in RFC1123 format - }, - expectedWait: 0, // Immediate retry since the date is current - }, - { - name: "XRateLimitReset", - headers: map[string]string{ - "X-RateLimit-Remaining": "0", - "X-RateLimit-Reset": strconv.FormatInt(time.Now().Add(90*time.Second).Unix(), 10), // 90 seconds from now - }, - expectedWait: 90*time.Second + 5*time.Second, // Add 5 seconds for skew buffer - }, - { - name: "NoHeaders", - headers: map[string]string{}, - expectedWait: 0, // No wait since no headers are present - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - resp := &http.Response{Header: http.Header{}} - for k, v := range tt.headers { - resp.Header.Add(k, v) - } - - mockLog := mocklogger.NewMockLogger() - wait := ParseRateLimitHeaders(resp, mockLog) - - // Adjust the delta based on the expected wait duration - delta := time.Duration(1) * time.Second - if tt.expectedWait == 0 { - // For immediate retries, allow a larger delta - delta = time.Duration(5) * time.Second - } - - assert.InDelta(t, tt.expectedWait, wait, float64(delta), "Wait duration should be within expected range") - }) - } -} diff --git a/redirecthandler/redirecthandler_test.go.TODO b/redirecthandler/redirecthandler_test.go.TODO deleted file mode 100644 index 0a302c5..0000000 --- a/redirecthandler/redirecthandler_test.go.TODO +++ /dev/null @@ -1,197 +0,0 @@ -package redirecthandler - -import ( - "net/http" - "net/url" - "testing" - - "github.com/deploymenttheory/go-api-http-client/logger" - "github.com/deploymenttheory/go-api-http-client/mocklogger" - "github.com/stretchr/testify/assert" -) - -// TestRedirectHandler_CheckRedirect tests the checkRedirect method of the RedirectHandler. -// It covers various scenarios including redirect loop detection, maximum redirects limit, -// resolving relative redirects, cross-domain security measures, and handling of 303 See Other response. -func TestRedirectHandler_CheckRedirect(t *testing.T) { - redirectHandler := NewRedirectHandler(nil, 10) // Logger is not needed for these tests - - reqURL, _ := url.Parse("http://example.com") - req := &http.Request{URL: reqURL, Method: http.MethodPost} - - // Test cases - tests := []struct { - name string - prepare func() *http.Response // Function to prepare the response for each test case - expectedErr error - expectedURL string - }{ - { - name: "Redirect Loop Detection", - prepare: func() *http.Response { - redirectHandler.VisitedURLs = map[string]int{"http://example.com": 1} - return nil - }, - expectedErr: http.ErrUseLastResponse, - }, - { - name: "Maximum Redirects Reached", - prepare: func() *http.Response { - redirectHandler.VisitedURLs = map[string]int{} - redirectHandler.MaxRedirects = 1 - return nil - }, - expectedErr: http.ErrUseLastResponse, - }, - { - name: "Resolve Relative Redirects", - prepare: func() *http.Response { - redirectHandler.MaxRedirects = 10 - return &http.Response{ - StatusCode: http.StatusSeeOther, - Header: http.Header{"Location": []string{"http://example.com/new"}}, - } - }, - expectedURL: "http://example.com/new", - }, - { - name: "Cross-Domain Security Measures", - prepare: func() *http.Response { - return &http.Response{ - Header: http.Header{"Location": []string{"http://anotherdomain.com/new"}}, - } - }, - expectedErr: nil, - }, - { - name: "Handling 303 See Other", - prepare: func() *http.Response { - return &http.Response{ - StatusCode: http.StatusSeeOther, - Header: http.Header{"Location": []string{"http://example.com/new"}}, - } - }, - expectedErr: nil, - expectedURL: "http://example.com/new", - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - resp := tc.prepare() - err := redirectHandler.checkRedirect(req, []*http.Request{{Response: resp}, {}}) - - assert.Equal(t, tc.expectedErr, err) - if tc.expectedURL != "" { - assert.Equal(t, tc.expectedURL, req.URL.String()) - } - }) - } -} - -// TestRedirectHandler_ResolveRedirectURL tests the resolveRedirectURL method of the RedirectHandler. -// It checks the correct resolution of absolute and relative URLs including those with query parameters and fragments. -func TestRedirectHandler_ResolveRedirectURL(t *testing.T) { - redirectHandler := RedirectHandler{} - - t.Run("Absolute URL", func(t *testing.T) { - reqURL, _ := url.Parse("http://example.com") - redirectURL, _ := url.Parse("http://newexample.com/path") - newReqURL, err := redirectHandler.resolveRedirectURL(reqURL, redirectURL) - assert.Nil(t, err) - assert.Equal(t, redirectURL.String(), newReqURL.String()) - }) - - t.Run("Relative URL", func(t *testing.T) { - reqURL, _ := url.Parse("http://example.com/current") - redirectURL, _ := url.Parse("/newpath") - newReqURL, err := redirectHandler.resolveRedirectURL(reqURL, redirectURL) - assert.Nil(t, err) - assert.Equal(t, "http://example.com/newpath", newReqURL.String()) - }) - - t.Run("Relative URL with Query and Fragment", func(t *testing.T) { - reqURL, _ := url.Parse("http://example.com/current?param=value#fragment") - redirectURL, _ := url.Parse("newpath?newparam=newvalue#newfragment") - newReqURL, err := redirectHandler.resolveRedirectURL(reqURL, redirectURL) - assert.Nil(t, err) - assert.Equal(t, "http://example.com/newpath?newparam=newvalue#newfragment", newReqURL.String()) - }) -} - -// TestRedirectHandler_SecureRequest tests the secureRequest method of the RedirectHandler. -// It verifies that sensitive headers are correctly removed when a request is redirected to a different domain. -func TestRedirectHandler_SecureRequest(t *testing.T) { - mockLogger := mocklogger.NewMockLogger() - mockLogger.SetLevel(logger.LogLevelDebug) - - redirectHandler := RedirectHandler{Logger: mockLogger} - req := &http.Request{Header: http.Header{"Authorization": []string{"token"}, "Cookie": []string{"session"}}} - - t.Run("Secure Cross-Domain Redirect", func(t *testing.T) { - redirectHandler.secureRequest(req) - // Ensure sensitive headers are removed and log messages were recorded - assert.Empty(t, req.Header.Get("Authorization")) - assert.Empty(t, req.Header.Get("Cookie")) - assert.Contains(t, mockLogger.Calls[0].Arguments.String(0), "Removed sensitive header") - }) -} - -// Test for Redirect Loop Detection - This test ensures that the redirect handler correctly identifies and stops redirect loops. -func TestRedirectLoopDetection(t *testing.T) { - // Setup - mockLogger := mocklogger.NewMockLogger() - handler := NewRedirectHandler(mockLogger, 5) - loopURL, _ := url.Parse("http://example.com/loop") - req := &http.Request{URL: loopURL} - - // Simulate a redirect loop by adding the same URL to the history multiple times - handler.RedirectHistories[req] = []*url.URL{loopURL, loopURL} - - // Test - err := handler.checkRedirect(req, []*http.Request{req, req}) - - // Assertions - assert.NotNil(t, err) - assert.Contains(t, err.Error(), "redirect loop detected") - // Verify log message for loop detection - assert.Contains(t, mockLogger.Calls[0].Arguments.String(0), "Redirect loop detected") -} - -// TestRedirectHistoryCleanup - This test ensures that the redirect history for each request is properly cleaned up to prevent memory leaks. -func TestRedirectHistoryCleanup(t *testing.T) { - // Setup - mockLogger := mocklogger.NewMockLogger() - handler := NewRedirectHandler(mockLogger, 5) - req := &http.Request{URL: &url.URL{Path: "/test"}} - - // Simulate adding some history - handler.RedirectHistories[req] = []*url.URL{{Path: "/redirect1"}, {Path: "/redirect2"}} - - // Perform a redirect that will trigger the cleanup - handler.checkRedirect(req, []*http.Request{req}) - - // Assertions - _, exists := handler.RedirectHistories[req] - assert.False(t, exists) -} - -// TestMaxRedirectsReached - This test checks that the handler stops redirects after reaching the maximum limit. -func TestMaxRedirectsReached(t *testing.T) { - // Setup - mockLogger := mocklogger.NewMockLogger() - handler := NewRedirectHandler(mockLogger, 1) // Set max redirects to 1 - req := &http.Request{URL: &url.URL{Path: "/start"}} - via := []*http.Request{{}, {}} // Simulate one redirect has already occurred - - // Test - err := handler.checkRedirect(req, via) - - // Assertions - assert.NotNil(t, err) - assert.IsType(t, &MaxRedirectsError{}, err) - maxRedirectsError := err.(*MaxRedirectsError) - assert.Equal(t, 1, maxRedirectsError.MaxRedirects) - // Verify log message for max redirects reached - assert.Contains(t, mockLogger.Calls[0].Arguments.String(0), "Maximum redirects reached") -}