Skip to content

Conversation

@jbliao
Copy link

@jbliao jbliao commented Mar 24, 2025

Reusing http request that contains body will cause error in #228 .
This PR recreate req.Body using req.GetBody() when retrying to fix the issue.

@nicklaw5 nicklaw5 requested a review from Copilot November 8, 2025 09:25
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes an issue where automatic token refresh fails for requests with a request body. When a 401 Unauthorized response triggers a token refresh and retry, the request body needs to be reset since it was already consumed by the first request attempt.

  • Adds request body recreation logic using req.GetBody() before retry attempts
  • Introduces an attempt counter to track retry attempts
  • Adds a test case specifically for token refresh with POST requests that have a body
  • Updates the mock HTTP client to validate request body reading

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
helix.go Adds attempt counter and request body recreation logic before retry attempts in doRequest method
helix_test.go Adds mock client body validation and new test case for token refresh with request bodies

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +358 to +367
if attempt > 0 &&
req.Body != nil &&
req.GetBody != nil {

var err error
req.Body, err = req.GetBody()
if err != nil {
return err
}
}
Copy link

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code relies on req.GetBody being set, but newJSONRequest doesn't explicitly set this field. While http.NewRequestWithContext may automatically set GetBody for *bytes.Buffer, this is not guaranteed for all cases and could lead to silent failures where retry attempts fail without proper error handling. Consider explicitly setting req.GetBody in newJSONRequest after creating the request, like: req.GetBody = func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewBuffer(b)), nil }

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant