Implement OAuth2 Password Grant flow and refactor client initialization#10
Implement OAuth2 Password Grant flow and refactor client initialization#10
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRefactors CLI/config/auth from UUID to API key, adds OAuth2 password-grant token source, propagates context through HTTP/reporting clients and request methods, changes client constructors to accept ClientOption, adds pagination helper, expands tests/docs, and upgrades Go toolchain and linters. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Config as Config Loader
participant AuthOpt as ClientOption
participant HTTP as HTTP Client
participant RP as ReportPortal API
CLI->>Config: load file / flags
Config->>Config: validate ApiKey / Project / Host
Config->>AuthOpt: create client option (WithApiKeyAuth / PasswordGrant)
CLI->>AuthOpt: provide ctx to option
AuthOpt->>HTTP: build configured *http.Client (oauth2 transport if used)
CLI->>RP: NewReportingClient(host, project, clientOpt)
loop per test event
CLI->>RP: StartLaunch(ctx, ...)
RP-->>CLI: launch id
CLI->>RP: SaveLog(ctx, ...)
RP-->>CLI: ok
end
CLI->>RP: FinishLaunch(ctx, id)
RP-->>CLI: finished
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…nction signatures for better readability
# Conflicts: # README.md # Taskfile.yml # go.mod # go.sum
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
internal/commands/quality_gate.go (1)
160-177:⚠️ Potential issue | 🟠 MajorRun one check before the ticker and guard invalid intervals.
time.NewTicker(qgCheckInterval)panics for<= 0, and the firstcheckF(ctx)no longer runs until the first tick. With--quality-gate-check-interval >= --quality-gate-timeout, an already finished launch now times out without making a single API call. Thectx.Done()branch also reports parent cancellations as timeouts.🔧 Suggested fix
pollForStatusF := func(ctx context.Context) error { + if qgCheckInterval <= 0 { + return fmt.Errorf("quality gate check interval must be > 0") + } + + ok, cErr := checkF(ctx) + if cErr != nil { + return cErr + } + if ok { + return nil + } + ticker := time.NewTicker(qgCheckInterval) defer ticker.Stop() for { select { case <-ctx.Done(): - return fmt.Errorf("timeout waiting for quality gate status") + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + return fmt.Errorf("timeout waiting for quality gate status") + } + return ctx.Err() case <-ticker.C: ok, cErr := checkF(ctx) if cErr != nil { return cErr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/quality_gate.go` around lines 160 - 177, The polling closure pollForStatusF should validate qgCheckInterval > 0 and qgCheckInterval < timeout, perform an immediate call to checkF(ctx) before creating the ticker, and propagate context cancellations instead of masking them as timeouts; update pollForStatusF to (1) return the error from ctx.Err() when ctx.Done() fires (or wrap it) rather than returning a fixed "timeout" string, (2) call checkF(ctx) once up-front and handle its (ok, err) result before starting time.NewTicker(qgCheckInterval), and (3) guard against non-positive qgCheckInterval by returning a descriptive error if qgCheckInterval <= 0 (or fallback to a sane default) to avoid time.NewTicker panics.internal/commands/launch.go (1)
85-99:⚠️ Potential issue | 🟠 MajorPaginate filtered merges or later launch pages are silently dropped.
getMergeIDsstill buildsMergeLaunchesRQ.Launchesfrom a singlePageLaunchResource. If--filteror--filter-namematches more than one page, only the first page IDs are merged. This PR already addspkg/gorp/launches.go, Lines 35-52for all-pages filter retrieval, so this command path should use that (or resolvefilter-nameto a filter string and page through it) before sending the merge request.Also applies to: 141-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/launch.go` around lines 85 - 99, mergeLaunches builds MergeLaunchesRQ.Launches from a single PageLaunchResource so filtered results beyond the first page are dropped; update mergeLaunches (and the getMergeIDs call path) to collect all matching launch IDs by paging through results (or by using the all-pages helper in pkg/gorp/launches.go that returns every matching launch) before assigning MergeLaunchesRQ.Launches, ensuring both --filter and --filter-name resolve to the full set of pages instead of only the first page.internal/commands/commands.go (1)
57-60:⚠️ Potential issue | 🟠 MajorTruncate the existing config file before rewriting it.
Line 58 reopens
~/.gorpwithoutos.O_TRUNC, so replacing a longer JSON document with a shorter one leaves trailing bytes behind and the nextgetConfig()decode can fail.💡 Suggested fix
- f, err := os.OpenFile(getConfigFile(), os.O_CREATE|os.O_WRONLY, 0o600) + f, err := os.OpenFile(getConfigFile(), os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/commands.go` around lines 57 - 60, The config file is opened for create/write without truncation, which can leave trailing bytes when writing a shorter JSON; change the open call that uses getConfigFile() to include os.O_TRUNC (e.g., os.OpenFile(getConfigFile(), os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600)) or explicitly truncate the file (f.Truncate(0)) before writing so the new JSON doesn't leave leftover bytes; keep existing error handling around f, ensure f is closed, and update the single call that assigns f, err to reflect this truncation behavior.internal/commands/report.go (1)
125-142:⚠️ Potential issue | 🔴 Critical
reportLaunchcan hang ifreceive()fails during shutdown.
errChanis unbuffered, and after the scan loop finishes there is no receiver left. Ifrep.receive()returns an error while flushing logs or finishing the launch, the goroutine blocks onerrChan <- recErr, and the deferredwg.Wait()never completes.Also applies to: 171-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/report.go` around lines 125 - 142, The goroutine sending rep.receive() errors can block because errChan is unbuffered and is closed or left without a receiver during shutdown; change errChan := make(chan error) to a buffered channel (make(chan error, 1)) so the send in the goroutine never blocks, and remove or move the early defer close(errChan) so the channel is not closed while the goroutine may still write (close it only after wg.Wait() or simply omit closing). Apply the same change for the second occurrence referenced around lines 171-181 where an unbuffered errChan is used.pkg/gorp/client.go (1)
43-85:⚠️ Potential issue | 🟠 MajorThis is a source-breaking v5 API change that requires a major version bump.
The module path is
github.com/reportportal/goRP/v5, yet the exported constructors have been refactored incompatibly:
NewClient(line 75) now takesClientOptioninstead ofapiKey stringNewReportingClient(line 25) now takesClientOptioninstead of separate auth parametersDownstream consumers will encounter compile failures on routine upgrades. Either release this as v6 (semantic versioning requires major version for breaking changes) or provide compatibility wrappers to maintain v5 backward compatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/gorp/client.go` around lines 43 - 85, The exported constructors NewClient and NewReportingClient now accept ClientOption (and callers will break); to preserve v5 compatibility add overload-compatible wrapper functions with the original signatures that construct the appropriate ClientOption (e.g., call WithApiKeyAuth(ctx, apiKey) or WithPasswordOwnerGrantAuth(ctx, oauth2Config, username, password)) and forward to the new NewClient/NewReportingClient implementations, or alternatively bump the module major version; implement wrapper functions named NewClient (original signature) and NewReportingClient (original signature) that internally create the ClientOption and call the refactored constructors so downstream code still compiles while keeping the new ClientOption-based API.
🧹 Nitpick comments (1)
pkg/gorp/oauth2_test.go (1)
142-143: Test with 2-second sleep slows down the test suite.Consider reducing
expires_into a smaller value or restructuring the test to avoid long sleeps. While this works, it adds unnecessary latency to CI runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/gorp/oauth2_test.go` around lines 142 - 143, The test currently uses time.Sleep(2 * time.Second) to wait for the token to expire which slows CI; change the test to set the token's expires_in to a much smaller duration (e.g., 100-200ms) and reduce the sleep to match (e.g., time.Sleep(150 * time.Millisecond)), or better, replace the hard sleep with a short polling loop that checks token expiry (using the same expiry check used by the code under test) until it reports expired; update the test that constructs the token’s expires_in and remove the 2-second sleep to speed up the test run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/commands/commands.go`:
- Around line 18-24: The client builders currently always call WithApiKeyAuth
and the clientConfig struct (which now embeds oauth2.Config) is still validated
to require ApiKey; update the builder logic and validation to allow
OAuth2/password-grant configs: in places that call WithApiKeyAuth (and the
duplicate block around the 145-167 region) change the flow to use
WithApiKeyAuth(ctx, cfg.ApiKey) only when cfg.ApiKey is non-empty, otherwise
detect an OAuth2/password-grant configuration from the embedded oauth2.Config
fields (e.g., ClientID, ClientSecret, TokenURL, Username/Password if used) and
construct/use the appropriate OAuth2 auth helper instead; also update the
validation function (the one in util.go that currently rejects empty ApiKey) to
accept an empty ApiKey when cfg.oauth2.Config contains the required fields for
password-grant so a password-grant-only config can pass validation and build a
client.
In `@internal/commands/report.go`:
- Around line 306-311: The report path is posting an empty log batch because
receive() always calls r.flushLogs(true) which invokes SaveLogs even when
r.output is empty; modify SaveLogs (or the flush path) to detect an empty batch
and return immediately without making the POST (e.g., if len(r.output) == 0) so
no request is sent; update both call-sites (the flush in receive() and the other
flush around lines 395-405) to rely on this early-return and ensure
errGroup.Wait() cannot fail due to an empty-batch server rejection.
In `@main.go`:
- Around line 42-47: The flag rename (cli.StringFlag with Name "api-key" and
Sources cli.EnvVars("GORP_API_KEY")) is a breaking change; update the Usage
string from "Access Token" to "API Key" and restore backward compatibility by
accepting the old names: add "uuid" as an alias to the cli.StringFlag (or add a
separate cli.StringFlag with Name "uuid"), include both environment variables in
Sources (cli.EnvVars("GORP_API_KEY","GORP_UUID")), and emit a deprecation
warning at startup when the old flag/env ("uuid" or GORP_UUID) is used so
existing users are informed of the change.
In `@pkg/gorp/launches_test.go`:
- Around line 122-127: The test increments shared variable callCount inside the
httptest.NewServer handler and reads it later from the main test goroutine,
causing a data race; replace the plain int callCount with a concurrency-safe
counter (e.g., an int32/int64 used with sync/atomic or protect it with a
sync.Mutex) and update the handler to increment via atomic.AddInt32/64 (or
lock/unlock the mutex) and read it with atomic.LoadInt32/64 (or under the same
mutex) before asserting the request count in the test.
In `@pkg/gorp/oauth2_test.go`:
- Around line 74-77: The tests use a shared mutable requestCount variable that
is incremented in the httptest handler goroutine and read from the test
goroutine, causing a data race; change requestCount to an atomic counter (e.g.,
an int32/int64 with sync/atomic) and replace increments in the handler with
atomic.AddInt32/Int64 and reads in the tests with atomic.LoadInt32/Int64; apply
this change for requestCount in TestPasswordGrantTokenSource_Token,
TestPasswordGrantTokenSource_Token_RefreshesExpiredToken, and
TestPasswordGrantTokenSource_Token_ConcurrentAccess so all increments and reads
are synchronized.
In `@pkg/gorp/oauth2.go`:
- Around line 12-18: The passwordGrantTokenSource currently stores a request
context (ctx) that is reused by its Token() method causing refreshes to fail if
that context is cancelled; update the construction of passwordGrantTokenSource
so it does not capture an external request context but instead uses
context.Background() (or context.TODO()) for the internal ctx field, and ensure
Token() continues to use that internal background context; locate the type
passwordGrantTokenSource and its constructor/creation site and replace
passing/storing the externally provided ctx with
context.Background()/context.TODO().
In `@README.md`:
- Around line 21-24: The cURL example in the README uses the wrong GitHub
repository URL (github.com/avarabyeu/goRP) and should point to the official
reportportal/goRP releases; update the download URL string in the example (the
line containing the cURL command) to use
github.com/reportportal/goRP/releases/download/v5.0.2/goRP_5.0.2_darwin_amd64.tar.gz
so it matches the rest of the README and pulls from the correct release feed.
- Around line 17-20: The README's Go install example uses "go install
github.com/reportportal/goRP@latest" but the module is declared as
"github.com/reportportal/goRP/v5" in go.mod; update the install command to
include the semantic import version suffix (use "go install
github.com/reportportal/goRP/v5@latest") so it matches the module path and
resolves correctly.
---
Outside diff comments:
In `@internal/commands/commands.go`:
- Around line 57-60: The config file is opened for create/write without
truncation, which can leave trailing bytes when writing a shorter JSON; change
the open call that uses getConfigFile() to include os.O_TRUNC (e.g.,
os.OpenFile(getConfigFile(), os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600)) or
explicitly truncate the file (f.Truncate(0)) before writing so the new JSON
doesn't leave leftover bytes; keep existing error handling around f, ensure f is
closed, and update the single call that assigns f, err to reflect this
truncation behavior.
In `@internal/commands/launch.go`:
- Around line 85-99: mergeLaunches builds MergeLaunchesRQ.Launches from a single
PageLaunchResource so filtered results beyond the first page are dropped; update
mergeLaunches (and the getMergeIDs call path) to collect all matching launch IDs
by paging through results (or by using the all-pages helper in
pkg/gorp/launches.go that returns every matching launch) before assigning
MergeLaunchesRQ.Launches, ensuring both --filter and --filter-name resolve to
the full set of pages instead of only the first page.
In `@internal/commands/quality_gate.go`:
- Around line 160-177: The polling closure pollForStatusF should validate
qgCheckInterval > 0 and qgCheckInterval < timeout, perform an immediate call to
checkF(ctx) before creating the ticker, and propagate context cancellations
instead of masking them as timeouts; update pollForStatusF to (1) return the
error from ctx.Err() when ctx.Done() fires (or wrap it) rather than returning a
fixed "timeout" string, (2) call checkF(ctx) once up-front and handle its (ok,
err) result before starting time.NewTicker(qgCheckInterval), and (3) guard
against non-positive qgCheckInterval by returning a descriptive error if
qgCheckInterval <= 0 (or fallback to a sane default) to avoid time.NewTicker
panics.
In `@internal/commands/report.go`:
- Around line 125-142: The goroutine sending rep.receive() errors can block
because errChan is unbuffered and is closed or left without a receiver during
shutdown; change errChan := make(chan error) to a buffered channel (make(chan
error, 1)) so the send in the goroutine never blocks, and remove or move the
early defer close(errChan) so the channel is not closed while the goroutine may
still write (close it only after wg.Wait() or simply omit closing). Apply the
same change for the second occurrence referenced around lines 171-181 where an
unbuffered errChan is used.
In `@pkg/gorp/client.go`:
- Around line 43-85: The exported constructors NewClient and NewReportingClient
now accept ClientOption (and callers will break); to preserve v5 compatibility
add overload-compatible wrapper functions with the original signatures that
construct the appropriate ClientOption (e.g., call WithApiKeyAuth(ctx, apiKey)
or WithPasswordOwnerGrantAuth(ctx, oauth2Config, username, password)) and
forward to the new NewClient/NewReportingClient implementations, or
alternatively bump the module major version; implement wrapper functions named
NewClient (original signature) and NewReportingClient (original signature) that
internally create the ClientOption and call the refactored constructors so
downstream code still compiles while keeping the new ClientOption-based API.
---
Nitpick comments:
In `@pkg/gorp/oauth2_test.go`:
- Around line 142-143: The test currently uses time.Sleep(2 * time.Second) to
wait for the token to expire which slows CI; change the test to set the token's
expires_in to a much smaller duration (e.g., 100-200ms) and reduce the sleep to
match (e.g., time.Sleep(150 * time.Millisecond)), or better, replace the hard
sleep with a short polling loop that checks token expiry (using the same expiry
check used by the code under test) until it reports expired; update the test
that constructs the token’s expires_in and remove the 2-second sleep to speed up
the test run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9dc8d99-6a2e-4473-b6a0-3b43cef99c9a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
.github/workflows/build.yml.golangci.ymlREADME.mdTaskfile.ymlgo.modinternal/commands/commands.gointernal/commands/commands_test.gointernal/commands/launch.gointernal/commands/quality_gate.gointernal/commands/report.gointernal/commands/report_test.gointernal/commands/util.gomain.gopkg/gorp/client.gopkg/gorp/client_test.gopkg/gorp/example_test.gopkg/gorp/launches.gopkg/gorp/launches_test.gopkg/gorp/multipart.gopkg/gorp/oauth2.gopkg/gorp/oauth2_test.gopkg/gorp/quality_gate.gopkg/gorp/reporting_client.gopkg/gorp/reporting_client_test.go
| // make sure we flush all logs that are left | ||
| r.flushLogs(true) | ||
| // wait for requests to get sent | ||
| r.waitQueue.Wait() | ||
| // wait for all log batch goroutines and surface any failures | ||
| if err := r.errGroup.Wait(); err != nil { | ||
| return fmt.Errorf("reporting logs: %w", err) | ||
| } |
There was a problem hiding this comment.
Skip SaveLogs when the batch is empty.
receive() always calls flushLogs(true), so runs with no buffered output events still POST /log. With the new errGroup.Wait() path, any server-side rejection of that empty batch will fail an otherwise valid launch.
💡 Suggested fix
func (r *reporter) flushLogs(force bool) {
+ if len(r.logs) == 0 {
+ return
+ }
if force || (len(r.logs) >= r.logsBatchSize) {
batch := r.logs
r.errGroup.Go(func() error {
if _, err := r.client.SaveLogs(r.ctx, batch...); err != nil {Also applies to: 395-405
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/commands/report.go` around lines 306 - 311, The report path is
posting an empty log batch because receive() always calls r.flushLogs(true)
which invokes SaveLogs even when r.output is empty; modify SaveLogs (or the
flush path) to detect an empty batch and return immediately without making the
POST (e.g., if len(r.output) == 0) so no request is sent; update both call-sites
(the flush in receive() and the other flush around lines 395-405) to rely on
this early-return and ensure errGroup.Wait() cannot fail due to an empty-batch
server rejection.
| project := "prj" | ||
| callCount := 0 | ||
|
|
||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| assert.Equal(t, "/api/v1/prj/launch", r.URL.Path) | ||
| callCount++ |
There was a problem hiding this comment.
Protect callCount with an atomic or mutex.
The handler increments callCount from its own goroutine, and the test reads it on Line 171 from the main goroutine. That unsynchronized access will trip -race and can make the request-count assertion flaky.
🔧 Suggested fix
import (
"encoding/json"
"net/http"
"net/http/httptest"
"net/url"
+ "sync/atomic"
"testing"
@@
- callCount := 0
+ var callCount atomic.Int64
@@
- callCount++
+ callCount.Add(1)
@@
- assert.Equal(t, 2, callCount, "expected exactly 2 page requests")
+ assert.EqualValues(t, 2, callCount.Load(), "expected exactly 2 page requests")Also applies to: 171-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/gorp/launches_test.go` around lines 122 - 127, The test increments shared
variable callCount inside the httptest.NewServer handler and reads it later from
the main test goroutine, causing a data race; replace the plain int callCount
with a concurrency-safe counter (e.g., an int32/int64 used with sync/atomic or
protect it with a sync.Mutex) and update the handler to increment via
atomic.AddInt32/64 (or lock/unlock the mutex) and read it with
atomic.LoadInt32/64 (or under the same mutex) before asserting the request count
in the test.
| requestCount := 0 | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| requestCount++ | ||
| w.Header().Set("Content-Type", "application/json") |
There was a problem hiding this comment.
Data race on requestCount variable.
The requestCount variable is modified by the HTTP handler (which runs in a separate goroutine) and read by the main test goroutine without synchronization. This can cause flaky tests or race detector failures.
🔧 Proposed fix using atomic counter
+import "sync/atomic"
+
func TestPasswordGrantTokenSource_Token_ReusesCachedToken(t *testing.T) {
t.Parallel()
- requestCount := 0
+ var requestCount atomic.Int32
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- requestCount++
+ requestCount.Add(1)
w.Header().Set("Content-Type", "application/json")
// ...
}))
// ...
- assert.Equal(t, 1, requestCount, "Expected only one request due to token reuse")
+ assert.Equal(t, int32(1), requestCount.Load(), "Expected only one request due to token reuse")
}The same pattern applies to TestPasswordGrantTokenSource_Token_RefreshesExpiredToken (line 111) and TestPasswordGrantTokenSource_Token_ConcurrentAccess (line 277).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/gorp/oauth2_test.go` around lines 74 - 77, The tests use a shared mutable
requestCount variable that is incremented in the httptest handler goroutine and
read from the test goroutine, causing a data race; change requestCount to an
atomic counter (e.g., an int32/int64 with sync/atomic) and replace increments in
the handler with atomic.AddInt32/Int64 and reads in the tests with
atomic.LoadInt32/Int64; apply this change for requestCount in
TestPasswordGrantTokenSource_Token,
TestPasswordGrantTokenSource_Token_RefreshesExpiredToken, and
TestPasswordGrantTokenSource_Token_ConcurrentAccess so all increments and reads
are synchronized.
| type passwordGrantTokenSource struct { | ||
| ctx context.Context | ||
| config *oauth2.Config | ||
|
|
||
| user string | ||
| pass string | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
golang oauth2 TokenSource context best practices
💡 Result:
In Go's golang.org/x/oauth2 package (latest v0.36.0 as of 2026), the TokenSource interface is defined as: type TokenSource interface { Token (*Token, error) } It does not accept a context.Context parameter. The Token method must be safe for concurrent use and returns a Token or error. The library uses reuseTokenSource and tokenRefresher internally for caching and automatic refresh. Best practices for using TokenSource with context: 1. Create TokenSource with context via Config methods like Config.TokenSource(ctx, token) or clientcredentials.Config.TokenSource(ctx). The context controls token refresh operations (e.g., HTTP client for refresh requests). 2. Use oauth2.NewClient(ctx, ts) or Config.Client(ctx) to create http.Client. This wraps TokenSource in oauth2.Transport, which calls ts.Token during RoundTrip to add Authorization headers. Per docs and source, the ctx in NewClient configures the base Transport but does not propagate to Token calls. 3. For request cancellation and timeouts, use req = req.WithContext(ctx); client.Do(req). The request ctx controls the main request but NOT synchronous token refresh in Transport.RoundTrip, as TokenSource.Token ignores context (open issue since 2017: github.com/golang/oauth2/issues/262). 4. Common pattern for persistent tokens with refresh and saving: ts := oauthConfig.TokenSource(ctx, token) savingSource := &SavingTokenSource{source: ts, store: tokenStore} client := oauth2.NewClient(ctx, savingSource) Where SavingTokenSource implements TokenSource by calling source.Token and saving. 5. Avoid long-lived TokenSource with background ctx for apps needing cancellation propagation to refreshes; create per-operation or accept limitation. For production, use context.Background or context.TODO for TokenSource creation, derive timeouts on requests. 6. Official subpackages (jwt.Config.TokenSource(ctx), google.DefaultTokenSource(ctx), clientcredentials.Config.TokenSource(ctx)) all take ctx for their HTTP clients used in token ops. TokenSource.Token lacks ctx support, limiting cancellation during refresh. Use request ctx for main requests; refreshes use TokenSource ctx.
Citations:
- 1: https://github.com/golang/oauth2/blob/master/token.go
- 2: https://go.googlesource.com/oauth2/+/refs/heads/master/oauth2.go
- 3: https://go.googlesource.com/oauth2/+/refs/tags/v0.29.0/token.go
- 4: https://go.googlesource.com/oauth2/+/b9c813be7d0ec3262d46deb8677ba5cda93d95ec/transport.go
- 5: https://deepwiki.com/golang/oauth2/2.2-token-and-tokensource
- 6: https://pkg.go.dev/golang.org/x/oauth2/jwt
- 7: https://pkg.go.dev/golang.org/x/oauth2/authhandler
- 8: https://pkg.go.dev/golang.org/x/oauth2/google
- 9: https://pkg.go.dev/golang.org/x/oauth2/clientcredentials
- 10: https://pkg.go.dev/golang.org/x/oauth2
🏁 Script executed:
cat -n pkg/gorp/oauth2.go | head -50Repository: reportportal/goRP
Length of output: 1451
Context stored in struct will be reused across all token refresh operations.
The ctx captured at construction time (line 38) is reused by all subsequent Token() calls (line 23). If the original context is cancelled or has an expiring deadline, token refreshes will fail. This is a known limitation of the oauth2.TokenSource interface, which doesn't accept a context parameter.
For long-lived token sources, use context.Background() or context.TODO() when constructing the token source. Request-level cancellation should be handled via request context, not at the TokenSource level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/gorp/oauth2.go` around lines 12 - 18, The passwordGrantTokenSource
currently stores a request context (ctx) that is reused by its Token() method
causing refreshes to fail if that context is cancelled; update the construction
of passwordGrantTokenSource so it does not capture an external request context
but instead uses context.Background() (or context.TODO()) for the internal ctx
field, and ensure Token() continues to use that internal background context;
locate the type passwordGrantTokenSource and its constructor/creation site and
replace passing/storing the externally provided ctx with
context.Background()/context.TODO().
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.go`:
- Line 43: The warning string passed to slog.Warn is too long for golines;
update the slog.Warn calls (the slog.Warn invocation that mentions GORP_UUID and
the similar occurrence for GORP_API_KEY) to split the message into shorter
pieces so lines wrap (for example concatenate two shorter string literals with +
or build the message from two parts) so the function call fits within
line-length limits while preserving the exact text content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
1 participant
Summary by CodeRabbit