diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 676d0a9..da5ce31 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -125,7 +125,9 @@ jobs:
with:
go-version-file: go.mod
cache: true
- - name: go test -tags=live (etag + retry)
+ - name: go test -tags=live (etag + retry + pages + polling)
env:
GITHUB_TOKEN: ${{ github.token }}
- run: go test -tags=live -run 'TestETag_Live|TestRetry_Live' ./etag/... ./retry/...
+ # Server-to-server endpoints only; user-to-server ones 403 under
+ # the App installation token.
+ run: go test -tags=live -run 'TestETag_Live|TestRetry_Live|TestPages_Live|TestPoll_Live' ./etag/... ./retry/... ./pages/... ./polling/...
diff --git a/.github/workflows/fuzz.yml b/.github/workflows/fuzz.yml
index c8a1a03..db8c0a4 100644
--- a/.github/workflows/fuzz.yml
+++ b/.github/workflows/fuzz.yml
@@ -28,7 +28,30 @@ jobs:
if: always()
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
with:
- name: fuzz-corpus
+ name: fuzz-corpus-etag
path: etag/testdata/fuzz/
if-no-files-found: ignore
retention-days: 30
+
+ fuzz-retry-after:
+ name: Fuzz Retry-After parser
+ runs-on: ubuntu-latest
+ timeout-minutes: 15
+ steps:
+ - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+ - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0
+ with:
+ go-version-file: go.mod
+ cache: true
+ # -fuzz takes one target, hence a separate job.
+ - name: Run fuzz for 10 minutes
+ id: fuzz
+ run: go test -fuzz=FuzzParseRetryAfter -fuzztime=10m ./retry/...
+ - name: Upload crash corpus
+ if: always()
+ uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
+ with:
+ name: fuzz-corpus-retry
+ path: retry/testdata/fuzz/
+ if-no-files-found: ignore
+ retention-days: 30
diff --git a/CHANGELOG.md b/CHANGELOG.md
index f89cf03..e9ebc69 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,6 +5,128 @@ All notable changes to **go-github-kit** are documented in this file.
The format is based on [Keep a Changelog 1.1.0](https://keepachangelog.com/en/1.1.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
+## [1.7.0] - 2026-08-19
+
+Compatibility and correctness release. Fixes an ETag defect that could
+serve an empty response body, adds `NewE` for SDK constructors that return
+an error, and updates all documentation and examples for go-github v87 and
+later. No API breaks: every addition is a new symbol, and no exported
+signature or type shape changed. One behavioural change needs action if you
+supply your own `etag.Cache`; see Changed.
+
+### Added
+
+- `ghkit.NewE[T](factory func(*http.Client) (T, error), opts ...Option)`,
+ for SDK constructors that can fail. It does not restore type inference on
+ `github.NewClient`, which is variadic over its own options, so the
+ closure is still required; what it adds is somewhere for the
+ constructor's error to go. Factory errors are wrapped as
+ `"ghkit: factory: %w"`, so `errors.Is` still separates them from ghkit's
+ own config sentinels. `ghkit.New` is unchanged and still binds directly
+ to single-argument constructors such as `githubv4.NewClient`.
+- `ghkit.WithETagTransport(func(*etag.Transport))`, which hands the caller
+ the constructed transport so `Stats()` can be polled for a `/healthz`
+ endpoint or a metrics gauge. Previously the transport was built inside
+ `HTTPClient` and unreachable, so the `Stats()` recipe the README and
+ `MIGRATION.md` describe had no supported entry point. The option enables
+ the ETag layer on its own, so it cannot silently never fire.
+- `polling.WithFullJitter(frac)`, applying uniform jitter over
+ `[interval - span/2, interval + span/2]` so concurrent pollers
+ de-correlate. `polling.WithJitter` keeps its documented deterministic
+ offset; whichever of the two is applied last wins.
+- `ErrNilClient` in `pages`, `polling` and `search`, plus
+ `search.ErrEmptyQuery`. These conditions previously returned inline
+ `errors.New` values that no caller could match with `errors.Is`. Message
+ strings are unchanged.
+- `etag.KindBypassEmptyBody`, the `Event` kind for the empty-body bypass
+ described under Fixed. It counts toward `Stats().TotalBypasses` like the
+ other bypass kinds.
+- `ghkit.ErrETagTransportType`, returned by `HTTPClient` if the constructed
+ ETag transport is not an `*etag.Transport`. Unreachable today; exported so
+ it is matchable rather than an opaque string if it ever fires.
+
+### Fixed
+
+- `etag`: HEAD requests now bypass the cache layer entirely. A response to
+ a HEAD carries no body (`net/http` sets `Body` to `http.NoBody`), so the
+ layer hashed an empty body against the server's ETag over the real one.
+ Every such request recorded a drift mismatch, and ten inside the 60s
+ window degraded the whole transport to passive mode. Once degraded, a
+ HEAD-stored entry made a later GET on the same URL receive a synthesised
+ 200 carrying an empty body and `Content-Length: 0`. The mirror case
+ needed no degradation at all: a GET populated the entry and a subsequent
+ HEAD was handed a 200 with a body, violating HEAD semantics. HEAD
+ responses also keep their `ContentLength`, which the bounded read
+ previously zeroed. Two consequences follow: a HEAD no longer sends
+ `If-None-Match`, so it can no longer come back as a free 304, and it no
+ longer carries the `cond` cache-status header, so `cond.StatusOf` reports
+ `Updated` for every HEAD.
+- `etag`: responses with an empty body now bypass the cache, and a cached
+ entry with an empty body is evicted and treated as a miss. Storing one was
+ pointless (replaying it hands the caller nothing) and validating one
+ recorded a false drift mismatch on every request, which is the HEAD defect
+ above in general form. Evicting rather than merely skipping matters
+ because a pre-1.7.0 persistent `Cache` can hold body-less HEAD entries
+ under the key a GET uses, and skipping alone would leave them permanently:
+ nothing overwrites them when the wire response is also empty.
+ GitHub serves an empty body this way only for the raw representation of an
+ empty file or blob, so a poller watching one now spends a rate-limit unit
+ per poll where it previously got a free 304.
+- `retry`: guard against a nil `resp.Body` when draining a response before
+ a retry or a `Retry-After` abort. Reachable through a custom
+ `WithBaseTransport` whose `RoundTrip` returns a bare `&http.Response`,
+ where it panicked the caller's goroutine.
+
+### Changed
+
+- `go.mod` and `examples/go.mod` relax the Go directive from `1.26.5` to
+ `1.26` and add `toolchain go1.26.6`. The directive was a patch-level floor
+ imposed on every consumer and hard-failed builds under
+ `GOTOOLCHAIN=local`; the floor is now `1.26`, which lowers a requirement
+ rather than raising one. The `toolchain` line pins CI and local
+ development to 1.26.6, which carries the `net/http`, `net/url` and
+ `crypto/tls` fixes govulncheck requires. A `toolchain` directive in a
+ dependency is ignored, so it imposes nothing downstream.
+- `etag`: the cache key now includes a digest of `Accept` and
+ `X-GitHub-Api-Version` when either is present, so requests differing only
+ by those headers no longer share an entry and evict each other. Requests
+ carrying neither header keep exactly the key they had before. The
+ in-process LRU is bounded and self-evicts; a consumer-supplied
+ `etag.Cache` with no TTL retains old-format entries indefinitely, so set
+ a TTL or flush the key prefix on upgrade. During a rolling deploy both
+ formats coexist, which costs hit rate but is safe.
+
+### CI
+
+- The `pages` and `polling` live probes now run in the live job; both were
+ previously present but never executed. The `pages` probe moved off
+ `/user/repos`, a user-to-server endpoint that 403s under the App
+ installation token CI supplies, onto a public commits endpoint.
+- `FuzzParseRetryAfter` now gets coverage-guided fuzzing in its own job.
+ `-fuzz` takes a single target, so it cannot share the ETag job. The crash
+ corpus upload previously hardcoded `etag/testdata/fuzz/`, silently
+ discarding any `retry` crash.
+
+### Documentation
+
+- Quick start, recipes and all nine go-github examples updated for v87 and
+ later, which changed `NewClient` to
+ `(opts ...ClientOptionsFunc) (*Client, error)` and turned `UserAgent`,
+ `BaseURL` and `UploadURL` into read-only methods with no setters.
+ `examples/go.mod` moves from go-github v85 to v90.
+- Removed an incorrect claim that `WithEnterpriseURLs` requires a trailing
+ slash and errors without one. go-github normalises it, and has in every
+ version this project has documented.
+- The Prometheus recipe now builds on `ghkit.HTTPClient` instead of
+ hand-rolling `etag.NewTransport` plus `ratelimit.NewTransport`, which
+ silently omitted retry, throttle and oauth2.
+- README badges: the Go Report Card badge is removed, the service is retired
+ and rendered "go report: retired". Replaced with per-workflow status
+ badges plus release, Go version and license badges.
+- `ghtest` is documented as shipping four helpers rather than two;
+ `ETagServer` (1.5.0) and `LinkHeader` (1.4.0) shipped undocumented.
+ `examples/README.md` gained the missing `graphql-v4` row.
+
## [1.6.2] - 2026-07-30
Maintenance release. Bumps the Go toolchain to 1.26.5 to pick up the
@@ -683,6 +805,7 @@ and rotating PATs alike.
- `golang.org/x/oauth2` v0.36.0
- `golang.org/x/time` v0.15.0
+[1.7.0]: https://github.com/pcanilho/go-github-kit/releases/tag/v1.7.0
[1.6.2]: https://github.com/pcanilho/go-github-kit/releases/tag/v1.6.2
[1.6.1]: https://github.com/pcanilho/go-github-kit/releases/tag/v1.6.1
[1.6.0]: https://github.com/pcanilho/go-github-kit/releases/tag/v1.6.0
diff --git a/MIGRATION.md b/MIGRATION.md
index bc1522e..a7b36ec 100644
--- a/MIGRATION.md
+++ b/MIGRATION.md
@@ -11,14 +11,14 @@ If your repo already has a hand-rolled stack of `oauth2.Transport`, `go-github-r
## What the swap changes
-- **Drift safety hatch is automatic, not a flag**. If your repo today exposes a runtime flag like `--enable-precomputed-etag=false` to fall back to passive mode under drift, you can retire it. ghkit detects drift on every cacheable 200 and silently switches to passive mode after 10 mismatches in a 60-second window; after a 1-hour cooldown it probes back to precompute and recovers if the algorithm is working again. Wire `etag.WithEventCallback(...)` and filter on `KindDriftDetected`/`KindDriftRecovered` for transition alerts; poll `(*etag.Transport).Stats()` for live state on `/healthz`. As of v1.6.0, `Stats` also carries `TotalHits`/`TotalMisses`/`TotalStores`/`TotalBypasses` so a polling adapter can publish per-Outcome counters without scraping DEBUG-level slog records.
+- **Drift safety hatch is automatic, not a flag**. If your repo today exposes a runtime flag like `--enable-precomputed-etag=false` to fall back to passive mode under drift, you can retire it. ghkit detects drift on every cacheable 200 and silently switches to passive mode after 10 mismatches in a 60-second window; after a 1-hour cooldown it probes back to precompute and recovers if the algorithm is working again. Wire `etag.WithEventCallback(...)` and filter on `KindDriftDetected`/`KindDriftRecovered` for transition alerts; obtain the transport with `ghkit.WithETagTransport(func(tr *etag.Transport) { ... })` and poll its `Stats()` for live state on `/healthz`. As of v1.6.0, `Stats` also carries `TotalHits`/`TotalMisses`/`TotalStores`/`TotalBypasses` so a polling adapter can publish per-Outcome counters without scraping DEBUG-level slog records.
- **Stricter cache-policy gate**. ghkit's etag transport refuses to cache responses carrying `Cache-Control: no-store` or `Vary: *` (RFC 9111 alignment). GitHub does not currently emit either, so the practical effect is zero, but the behavior is stricter than a typical in-tree port.
- **Log/metric label fidelity**. ghkit's etag transport emits a small built-in slog event set with a generic path-template allowlist. Your repo's bespoke route table (`/repos/{o}/{r}/commits/{sha}/branches-where-head` and similar) collapses to a coarse fallback label. If your dashboards key on per-route labels, keep emitting metrics from your own RoundTripper above ghkit's transport; do not rely on ghkit's slog event labels for app-specific path cardinality.
- **Upstream features ghkit does not curate**. The named `ratelimit.With*` options cover the common callbacks; for upstream features ghkit does not expose (the abort callback on `WithTotalSleepLimit`, custom limit providers, before-request hooks), use `ratelimit.WithUpstreamOptions(opts ...any)` to forward raw `gofri/go-github-ratelimit/v2` options.
## Recipe 1: Kubernetes operator with rotating PAT
-Shape: a long-lived process holds one `*http.Client`. Each reconcile reads a fresh token from disk and clones a `*github.Client` on top of the shared transport with `(*github.Client).WithAuthToken(tok)`. The transport keeps the ETag cache and rate-limit bucket warm across reconciles.
+Shape: a long-lived process holds one `*http.Client`. Each reconcile reads a fresh token from disk and builds a `*github.Client` on top of the shared transport, passing the token as a construction option. The SDK copies the `*http.Client` it is given, so the shared transport is not mutated. The transport keeps the ETag cache and rate-limit bucket warm across reconciles.
### Before
@@ -60,7 +60,7 @@ import (
hc, err := ghkit.HTTPClient(
ghkit.WithETagCache(
etag.WithCache(etag.NewLRUCache(flags.Controller.ETagCacheSize)),
- etag.WithKeyScope(installationID),
+ etag.WithKeyScope(strconv.FormatInt(installationID, 10)),
),
ghkit.WithRateLimit(
ratelimit.WithPrimaryLimitDetected(func(c *ratelimit.PrimaryEvent) {
@@ -78,12 +78,17 @@ hc, err := ghkit.HTTPClient(
if err != nil { return err }
// Per reconcile -- new client, same transport:
-gh := github.NewClient(hc).WithAuthToken(readGitHubToken())
+gh, err := github.NewClient(
+ github.WithHTTPClient(hc),
+ github.WithAuthToken(readGitHubToken()),
+)
```
What moves: the bespoke `etag_transport.go` and the manual `github_ratelimit.NewClient(...)` call collapse into options. Compression handling is implicit. The reconcile-loop call site is unchanged. `PrimaryEvent` / `SecondaryEvent` are type aliases of the upstream `gofri` types, so callback bodies do not need to change.
-If you are pinned to `go-github/v84` and ghkit is on `v85`, keep your major: ghkit does not import `go-github` from `HTTPClient()` -- it returns `*http.Client`, which you hand to your own go-github version.
+Keep whichever `go-github` major you are pinned to. ghkit does not import `go-github` at all, so no ghkit release can force you to bump it or rewrite import paths. `HTTPClient()` returns an `*http.Client` you hand to your own version.
+
+Note that the "before" snippet above uses the pre-v87 `github.NewClient(hc).WithAuthToken(...)` API. From v87 the constructor is `NewClient(opts ...ClientOptionsFunc) (*Client, error)`, as shown in the "after" snippet.
## Recipe 2: Multi-installation webhook processor
@@ -139,7 +144,7 @@ func (p *Processor) getGitHubClient(ctx context.Context, installationID int64) (
ghkit.WithTimeout(5 * time.Second),
)
if err != nil { return nil, err }
- return github.NewClient(hc), nil
+ return github.NewClient(github.WithHTTPClient(hc))
}
```
@@ -187,12 +192,16 @@ func NewClient(token string, opts ...ETagOptions) (*Client, error) {
```go
import (
+ "net/http"
+
ghkit "github.com/pcanilho/go-github-kit"
"github.com/pcanilho/go-github-kit/etag"
)
func NewClient(token string, etagSize int) (*github.Client, error) {
- return ghkit.New(github.NewClient,
+ return ghkit.NewE(func(hc *http.Client) (*github.Client, error) {
+ return github.NewClient(github.WithHTTPClient(hc))
+ },
ghkit.WithToken(token),
ghkit.WithETagCache(etag.WithCache(etag.NewLRUCache(etagSize))),
ghkit.WithRequestsPerSecond(1.3, 1),
diff --git a/Makefile b/Makefile
index 0e4b19e..547b565 100644
--- a/Makefile
+++ b/Makefile
@@ -5,10 +5,10 @@ help:
@echo "Common targets:"
@echo " make test -- run the full test suite with -race"
@echo " make test-unit -- run short unit tests only"
- @echo " make test-live -- run the live ETag drift probe (needs GITHUB_TOKEN)"
- @echo " make test-fuzz -- fuzz the ETag hash for 30 seconds"
+ @echo " make test-live -- run the live probes (needs GITHUB_TOKEN)"
+ @echo " make test-fuzz -- fuzz the ETag hash and Retry-After parser"
@echo " make bench -- write benchmarks to dist/bench-current.txt"
- @echo " make bench-update -- prompt to update docs/bench-baseline.txt manually"
+ @echo " make bench-update -- prompt to update the benchmark baseline"
@echo " make lint -- golangci-lint run"
@echo " make vuln -- govulncheck on the module"
@echo " make tidy -- go mod tidy with a diff gate"
@@ -23,17 +23,18 @@ test-unit:
test-live:
@[ -n "$$GITHUB_TOKEN" ] || { echo "GITHUB_TOKEN required"; exit 1; }
- go test -tags=live -run TestETag_Live ./etag/...
+ go test -tags=live -run 'TestETag_Live|TestRetry_Live|TestPages_Live|TestPoll_Live' ./etag/... ./retry/... ./pages/... ./polling/...
test-fuzz:
go test -fuzz=FuzzETag_ComputeExpectedETag -fuzztime=30s ./etag/...
+ go test -fuzz=FuzzParseRetryAfter -fuzztime=30s ./retry/...
bench:
@mkdir -p dist
go test -bench=. -benchmem -run=^$$ ./... | tee dist/bench-current.txt
bench-update:
- @echo "Review dist/bench-current.txt and copy manually to docs/bench-baseline.txt."
+ @echo "Review dist/bench-current.txt and copy manually to dist/bench-baseline.txt."
lint:
golangci-lint run
diff --git a/README.md b/README.md
index b5acfc9..1c3094a 100644
--- a/README.md
+++ b/README.md
@@ -1,11 +1,16 @@
# `ghkit`
-A small Go toolkit that wraps [`github.com/google/go-github`](https://github.com/google/go-github) (REST), [`github.com/shurcooL/githubv4`](https://github.com/shurcooL/githubv4) (GraphQL), or any `func(*http.Client) T` client factory with ETag caching, reactive rate limiting, and a client-side token bucket. Opt into what you need; compose the rest yourself.
+A small Go toolkit that wraps [`github.com/google/go-github`](https://github.com/google/go-github) (REST), [`github.com/shurcooL/githubv4`](https://github.com/shurcooL/githubv4) (GraphQL), or any client factory with ETag caching, reactive rate limiting, and a client-side token bucket. Opt into what you need; compose the rest yourself.
[](https://github.com/pcanilho/go-github-kit/actions/workflows/ci.yml)
+[](https://github.com/pcanilho/go-github-kit/actions/workflows/fuzz.yml)
+[](https://github.com/pcanilho/go-github-kit/actions/workflows/gitleaks.yml)
+[](https://github.com/pcanilho/go-github-kit/actions/workflows/release.yml)
+
[](https://pkg.go.dev/github.com/pcanilho/go-github-kit)
-[](https://goreportcard.com/report/github.com/pcanilho/go-github-kit)
-[](LICENSE)
+[](https://github.com/pcanilho/go-github-kit/releases)
+[](go.mod)
+[](LICENSE)
## Why?
@@ -30,18 +35,22 @@ import (
"log"
"os"
- "github.com/google/go-github/v85/github"
+ "github.com/google/go-github/v90/github"
ghkit "github.com/pcanilho/go-github-kit"
)
func main() {
- gh, err := ghkit.New(github.NewClient,
+ hc, err := ghkit.HTTPClient(
ghkit.WithToken(os.Getenv("GITHUB_TOKEN")),
ghkit.WithETagCache(),
)
if err != nil {
log.Fatal(err)
}
+ gh, err := github.NewClient(github.WithHTTPClient(hc))
+ if err != nil {
+ log.Fatal(err)
+ }
repo, _, err := gh.Repositories.Get(context.Background(), "google", "go-github")
if err != nil {
log.Fatal(err)
@@ -50,7 +59,26 @@ func main() {
}
```
-`ghkit.New` is generic over the returned type; passing `github.NewClient` lets type inference pick up `*github.Client`. ghkit itself has zero dependency on `go-github`. It isn't in `go.mod`, isn't imported, and won't end up in your compiled binary unless you pull it in yourself. Pass whichever go-github major (or any other `func(*http.Client) T` factory) you want.
+ghkit itself has zero dependency on `go-github`. It isn't in `go.mod`, isn't imported, and won't end up in your compiled binary unless you pull it in yourself. Use whichever go-github major you like: **no ghkit release can force an SDK version on you.**
+
+`ghkit.HTTPClient` returns the `*http.Client` and lets you construct the SDK client yourself. If you prefer a single call, `ghkit.NewE` takes a `func(*http.Client) (T, error)` factory, and `ghkit.New` takes a `func(*http.Client) T` one for constructors that cannot fail, such as `githubv4.NewClient`.
+
+### go-github v87 and later
+
+`github.NewClient` returns `(*github.Client, error)` since v87, and `UserAgent`, `BaseURL` and `UploadURL` are now read-only methods. Either use the two-step form above, or:
+
+```go
+gh, err := ghkit.NewE(func(hc *http.Client) (*github.Client, error) {
+ return github.NewClient(
+ github.WithHTTPClient(hc),
+ github.WithUserAgent("my-app/1.0"),
+ )
+}, ghkit.WithToken(tok))
+```
+
+On v86 and earlier, `ghkit.New(github.NewClient, ...)` still works.
+
+Do not pass `github.WithTransport` or `github.WithEnvProxy`: the first replaces ghkit's transport stack, the second errors because ghkit's outermost transport is not an `*http.Transport`.
For runnable starter programs, see [`examples/`](examples/): `static-pat`, `installation-token`, `graphql-v4`, `backfill`, `github-enterprise`, and `retry-on-flaky` are each a complete `main()` you can copy-paste.
@@ -77,7 +105,9 @@ The rate-limit layer's named options (`WithPrimaryLimitDetected`, `WithSecondary
Recommended setup for a long-lived service
```go
-gh, err := ghkit.New(github.NewClient,
+gh, err := ghkit.NewE(func(hc *http.Client) (*github.Client, error) {
+ return github.NewClient(github.WithHTTPClient(hc))
+},
ghkit.WithToken(os.Getenv("GITHUB_TOKEN")),
ghkit.WithETagCache(),
ghkit.WithRetry(),
@@ -93,7 +123,9 @@ Defaults are tuned for steady-state operators: rate-limit on, retry 3 attempts w
Static PAT with ETag caching
```go
-gh, err := ghkit.New(github.NewClient,
+gh, err := ghkit.NewE(func(hc *http.Client) (*github.Client, error) {
+ return github.NewClient(github.WithHTTPClient(hc))
+},
ghkit.WithToken(os.Getenv("GITHUB_TOKEN")),
ghkit.WithETagCache(),
)
@@ -117,7 +149,7 @@ v4, err := ghkit.New(githubv4.NewClient,
)
```
-`ghkit.New` is generic; `githubv4.NewClient` satisfies `func(*http.Client) *githubv4.Client` and gets oauth2 + retry + ratelimit + throttle + UA from the transport stack. ETag caching is REST-only by design (the etag layer no-ops on POST), so `WithETagCache` is a no-op for v4 traffic; leave it off unless you also issue REST GETs through the same client.
+`ghkit.New` is generic; `githubv4.NewClient` satisfies `func(*http.Client) *githubv4.Client` and gets oauth2 + retry + ratelimit + throttle + UA from the transport stack. ETag caching is REST-only by design (the etag layer no-ops on anything but GET), so `WithETagCache` is a no-op for v4 traffic; leave it off unless you also issue REST GETs through the same client.
A runnable version lives at [`examples/graphql-v4/`](examples/graphql-v4/main.go).
@@ -134,7 +166,7 @@ import (
"net/http"
"os"
- "github.com/google/go-github/v85/github"
+ "github.com/google/go-github/v90/github"
ghkit "github.com/pcanilho/go-github-kit"
"github.com/pcanilho/go-github-kit/pages"
)
@@ -181,7 +213,7 @@ import (
"os"
"time"
- "github.com/google/go-github/v85/github"
+ "github.com/google/go-github/v90/github"
ghkit "github.com/pcanilho/go-github-kit"
"github.com/pcanilho/go-github-kit/polling"
"github.com/pcanilho/go-github-kit/retry"
@@ -204,7 +236,7 @@ seq := polling.As[*github.WorkflowRun](
nil, 15*time.Second,
polling.WithDoneT(func(r *github.WorkflowRun) bool { return r.GetStatus() == "completed" }),
polling.WithMaxWallClock(30*time.Minute),
- polling.WithJitter(0.2),
+ polling.WithFullJitter(0.2),
)
var run *github.WorkflowRun
@@ -218,6 +250,8 @@ for r, err := range seq {
log.Printf("conclusion=%s", run.GetConclusion())
```
+`WithFullJitter` samples uniformly around the interval so concurrent pollers de-correlate; `WithJitter` applies a fixed offset instead and leaves pollers started together in step. The last of the two applied wins.
+
Sharp edges: each `c.Do` may itself loop through `retry.Transport` (pass `retry.WithMaxAttempts(1)` when polling owns the outer loop); `throttle.WithRequestsPerSecond` below `1/interval` dominates cadence; with `WithETagCache` an unchanged resource yields identical decoded bytes per tick (pair with `polling.WithChangeOnly` to skip those silently). Pages-shape body ownership: `Poll` yields `*http.Response` and the caller closes; `As[T]` owns and closes via defer.
See [`examples/poll-workflow-run/`](examples/poll-workflow-run/main.go) for a runnable demo.
@@ -234,7 +268,7 @@ import (
"errors"
"fmt"
- "github.com/google/go-github/v85/github"
+ "github.com/google/go-github/v90/github"
"github.com/pcanilho/go-github-kit/search"
)
@@ -273,7 +307,7 @@ import (
"io"
"net/http"
- "github.com/google/go-github/v85/github"
+ "github.com/google/go-github/v90/github"
"github.com/pcanilho/go-github-kit/cond"
)
@@ -324,7 +358,7 @@ hc, err := ghkit.HTTPClient(
ghkit.WithTimeout(5 * time.Second),
)
if err != nil { return err }
-gh := github.NewClient(hc)
+gh, err := github.NewClient(github.WithHTTPClient(hc))
```
`WithKeyScope` is required whenever you supply a `Cache` yourself. It namespaces entries so two installations hitting the same URL never read each other's bodies.
@@ -395,7 +429,9 @@ Use `WithAutoKeyScope` instead of `WithKeyScope` when one `*http.Client` serves
Backfill shape with a proactive RPS cap
```go
-gh, err := ghkit.New(github.NewClient,
+gh, err := ghkit.NewE(func(hc *http.Client) (*github.Client, error) {
+ return github.NewClient(github.WithHTTPClient(hc))
+},
ghkit.WithToken(os.Getenv("GITHUB_TOKEN")),
ghkit.WithETagCache(etag.WithCache(etag.NewLRUCache(8192))),
ghkit.WithRequestsPerSecond(1.3, 1),
@@ -409,27 +445,30 @@ gh, err := ghkit.New(github.NewClient,
GitHub Enterprise Server
```go
-gh, err := ghkit.New(func(hc *http.Client) *github.Client {
- c, ghErr := github.NewClient(hc).WithEnterpriseURLs(
- "https://github.example.com/api/v3/",
- "https://github.example.com/api/uploads/",
+gh, err := ghkit.NewE(func(hc *http.Client) (*github.Client, error) {
+ return github.NewClient(
+ github.WithHTTPClient(hc),
+ github.WithEnterpriseURLs(
+ "https://github.example.com/api/v3/",
+ "https://github.example.com/api/uploads/",
+ ),
+ github.WithUserAgent("my-app/1.0"),
)
- if ghErr != nil {
- return github.NewClient(hc) // fall back to github.com on a bad URL
- }
- c.UserAgent = "my-app/1.0"
- return c
}, ghkit.WithToken(os.Getenv("GITHUB_ENTERPRISE_TOKEN")))
```
-`WithEnterpriseURLs` requires both URLs to end with a trailing slash and returns an error otherwise. `UserAgent` can also be set at the transport level via `ghkit.WithUserAgent("my-app/1.0")`, which applies to every outbound request regardless of which SDK you wrap around `HTTPClient()`.
+`NewE` propagates the constructor's error, so a bad enterprise URL fails here rather than silently yielding a github.com client. Sending an Enterprise token to public github.com is a credential-leak path, so do not fall back.
+
+go-github normalises the trailing slash on both URLs for you; it rejects an empty string. `UserAgent` can also be set at the transport level via `ghkit.WithUserAgent("my-app/1.0")`, which applies to every outbound request regardless of which SDK you wrap around `HTTPClient()`.
Retry on transient failures (5xx, network errors)
```go
-gh, err := ghkit.New(github.NewClient,
+gh, err := ghkit.NewE(func(hc *http.Client) (*github.Client, error) {
+ return github.NewClient(github.WithHTTPClient(hc))
+},
ghkit.WithToken(os.Getenv("GITHUB_TOKEN")),
ghkit.WithRetry(), // 3 attempts, 200ms..2s decorrelated jitter, idempotent methods only
)
@@ -440,7 +479,9 @@ Tuned policy with POST opt-in via `Idempotency-Key`:
```go
import "github.com/pcanilho/go-github-kit/retry"
-gh, err := ghkit.New(github.NewClient,
+gh, err := ghkit.NewE(func(hc *http.Client) (*github.Client, error) {
+ return github.NewClient(github.WithHTTPClient(hc))
+},
ghkit.WithToken(token),
ghkit.WithRetry(
retry.WithMaxAttempts(5),
@@ -478,6 +519,7 @@ import (
"time"
"github.com/prometheus/client_golang/prometheus"
+ ghkit "github.com/pcanilho/go-github-kit"
"github.com/pcanilho/go-github-kit/etag"
"github.com/pcanilho/go-github-kit/ratelimit"
)
@@ -489,43 +531,62 @@ var (
rl = prometheus.NewCounterVec(prometheus.CounterOpts{Name: "ghkit_ratelimit_total"}, []string{"kind", "category"})
)
-func transport(scope string) (http.RoundTripper, error) {
- etagRT, err := etag.NewTransport(nil,
- etag.WithKeyScope(scope),
- etag.WithEventCallback(func(_ context.Context, ev etag.Event) {
- cacheEvents.WithLabelValues(string(ev.Kind)).Inc()
- if ev.Status > 0 {
- fromCache := ev.Kind == etag.KindHit || ev.Kind == etag.KindValidatedOK
- reqs.WithLabelValues(fmt.Sprintf("%dxx", ev.Status/100), fmt.Sprint(fromCache)).Inc()
- }
- if ev.Kind == etag.KindMismatch {
- mismatches.WithLabelValues(ev.PathTemplate).Inc()
- }
- }),
+func client(scope, token string) (*http.Client, error) {
+ return ghkit.HTTPClient(
+ ghkit.WithToken(token),
+ ghkit.WithETagCache(
+ etag.WithKeyScope(scope),
+ etag.WithEventCallback(func(_ context.Context, ev etag.Event) {
+ cacheEvents.WithLabelValues(string(ev.Kind)).Inc()
+ if ev.Status > 0 {
+ fromCache := ev.Kind == etag.KindHit || ev.Kind == etag.KindValidatedOK
+ reqs.WithLabelValues(fmt.Sprintf("%dxx", ev.Status/100), fmt.Sprint(fromCache)).Inc()
+ }
+ if ev.Kind == etag.KindMismatch {
+ mismatches.WithLabelValues(ev.PathTemplate).Inc()
+ }
+ }),
+ ),
+ ghkit.WithRateLimit(
+ ratelimit.WithTotalSleepLimit(time.Hour),
+ ratelimit.WithPrimaryLimitDetected(func(ev *ratelimit.PrimaryEvent) {
+ rl.WithLabelValues("primary", string(ev.Category)).Inc()
+ }),
+ ratelimit.WithSecondaryLimitDetected(func(*ratelimit.SecondaryEvent) {
+ rl.WithLabelValues("secondary", "").Inc()
+ }),
+ ),
+ ghkit.WithRetry(),
)
- if err != nil {
- return nil, err
- }
- return ratelimit.NewTransport(etagRT,
- ratelimit.WithTotalSleepLimit(time.Hour),
- ratelimit.WithPrimaryLimitDetected(func(ev *ratelimit.PrimaryEvent) {
- rl.WithLabelValues("primary", string(ev.Category)).Inc()
- }),
- ratelimit.WithSecondaryLimitDetected(func(*ratelimit.SecondaryEvent) {
- rl.WithLabelValues("secondary", "").Inc()
- }),
- ), nil
}
```
Metric names are illustrative; substitute your registry conventions.
+
+Build this through `ghkit.HTTPClient` rather than stacking `etag.NewTransport` and `ratelimit.NewTransport` by hand: the hand-built version silently omits retry, throttle and oauth2, and has to repeat the layer ordering correctly. `WithETagCache` and `WithRateLimit` forward their sub-options verbatim, so nothing is lost.
+
+For pull-based gauges, `ghkit.WithETagTransport` hands you the `*etag.Transport` so you can poll `Stats()`:
+
+```go
+var etagRT *etag.Transport
+hc, err := ghkit.HTTPClient(
+ ghkit.WithToken(token),
+ ghkit.WithETagTransport(func(t *etag.Transport) { etagRT = t }),
+)
+// etagRT.Stats() -> {Degraded, TotalHits, TotalMisses, TotalStores, ...}
+```
Use only the etag sub-package in a hand-built stack
```go
-import "github.com/pcanilho/go-github-kit/etag"
+import (
+ "net/http"
+
+ "github.com/google/go-github/v90/github"
+ "github.com/pcanilho/go-github-kit/etag"
+)
rt, err := etag.NewTransport(nil, // nil = default base with DisableCompression=true
etag.WithCache(etag.NewLRUCache(1024)),
@@ -533,16 +594,17 @@ rt, err := etag.NewTransport(nil, // nil = default base with DisableCompression=
)
if err != nil { return err }
hc := &http.Client{Transport: rt}
-gh := github.NewClient(hc)
+gh, err := github.NewClient(github.WithHTTPClient(hc))
```
## Testing your code
-The `ghtest` sub-package provides two helpers for the GitHub-specific
-traps in writing tests: secondary-rate-limit classification and the
-bored-engineer ETag hash domain. See [`TESTING.md`](TESTING.md) for the
-full recipe set.
+The `ghtest` sub-package provides four helpers for the GitHub-specific
+traps in writing tests: `WriteSecondaryLimit`, `Write304IfMatch`,
+`ETagServer` (a ready-made ETag/304 test server) and `LinkHeader` (RFC 8288
+pagination fixtures). See [`TESTING.md`](TESTING.md) for the full recipe
+set.
## Migrating from an in-tree GitHub transport
@@ -556,7 +618,7 @@ The precompute trick, reverse-engineered by [bored-engineer](https://github.com/
The algorithm walkthrough lives at .
-**What happens when GitHub changes the algorithm.** Every cacheable 200 is validated: the transport recomputes the expected ETag and compares it to the server's. After 10 mismatches inside a 60-second window, the transport silently switches to sending the server's stored ETag as `If-None-Match` -- 304s resume on stable bodies, you pay at most one extra miss per URL when the algorithm changes. After a 1-hour cooldown, the transport probes back to precompute on a small fraction of requests; consecutive successes restore precompute mode automatically, so a transient drift blip doesn't permanently degrade a long-running process. Wire `etag.WithEventCallback(...)` and filter on `etag.KindDriftDetected` / `etag.KindDriftRecovered` for transition alerts; call `(*etag.Transport).Stats()` for `/healthz` or dashboard polling. `Stats` exposes per-Outcome counters (`TotalHits`/`TotalMisses`/`TotalStores`/`TotalBypasses`) for hit-rate metrics without paying for DEBUG-level slog ingestion. For per-call attribution (URL, repo, consumer-side context like webhook event type), the same `WithEventCallback` hook delivers every cache decision. The fallback itself is unconditional and has no public knob -- this is by design.
+**What happens when GitHub changes the algorithm.** Every cacheable 200 is validated: the transport recomputes the expected ETag and compares it to the server's. After 10 mismatches inside a 60-second window, the transport silently switches to sending the server's stored ETag as `If-None-Match` -- 304s resume on stable bodies, you pay at most one extra miss per URL when the algorithm changes. After a 1-hour cooldown, the transport probes back to precompute on a small fraction of requests; consecutive successes restore precompute mode automatically, so a transient drift blip doesn't permanently degrade a long-running process. Wire `etag.WithEventCallback(...)` and filter on `etag.KindDriftDetected` / `etag.KindDriftRecovered` for transition alerts; call `(*etag.Transport).Stats()` for `/healthz` or dashboard polling, obtaining the transport via `ghkit.WithETagTransport`. `Stats` exposes per-Outcome counters (`TotalHits`/`TotalMisses`/`TotalStores`/`TotalBypasses`) for hit-rate metrics without paying for DEBUG-level slog ingestion. For per-call attribution (URL, repo, consumer-side context like webhook event type), the same `WithEventCallback` hook delivers every cache decision. The fallback itself is unconditional and has no public knob -- this is by design.
What this kit adds on top of the original idea:
@@ -589,40 +651,48 @@ Each retry attempt is a real HTTP call from the throttle layer's perspective. `W
## Using a different go-github version
-The kit has no compile-time pin on `go-github`. Its main `go.mod` does not require `github.com/google/go-github`, so you choose the major. Two equally valid shapes:
+The kit has no compile-time pin on `go-github`. Its main `go.mod` does not require `github.com/google/go-github`, so you choose the major, and **upgrading ghkit never forces you to change your SDK version or rewrite import paths.**
-**Generic factory** (when you want type inference to pick up `*github.Client`):
+**Library-agnostic** (the `*http.Client` on its own; works with every SDK and every major):
```go
import githubX "github.com/google/go-github/vX/github"
-gh, err := ghkit.New(githubX.NewClient,
+hc, err := ghkit.HTTPClient(
ghkit.WithToken(os.Getenv("GITHUB_TOKEN")),
ghkit.WithETagCache(),
)
+if err != nil { log.Fatal(err) }
+
+gh, err := githubX.NewClient(githubX.WithHTTPClient(hc)) // v87+
+if err != nil { log.Fatal(err) }
```
-**Library-agnostic** (when you want the `*http.Client` and will wire your own client library):
+**Single call** via `NewE`, for any constructor returning `(T, error)`:
```go
-import githubX "github.com/google/go-github/vX/github"
+gh, err := ghkit.NewE(func(hc *http.Client) (*githubX.Client, error) {
+ return githubX.NewClient(githubX.WithHTTPClient(hc))
+}, ghkit.WithToken(os.Getenv("GITHUB_TOKEN")))
+```
-hc, err := ghkit.HTTPClient(
+**Generic factory** via `New`, for constructors that take the `*http.Client` alone and cannot fail. This fits `githubv4.NewClient`, and `go-github` up to v86:
+
+```go
+v4, err := ghkit.New(githubv4.NewClient,
ghkit.WithToken(os.Getenv("GITHUB_TOKEN")),
- ghkit.WithETagCache(),
)
-gh := githubX.NewClient(hc)
```
-The runnable demos under [`examples/`](examples/) live in their own sub-module and pin a specific go-github version (currently `v85`) so the kit's main `go.mod` stays clean across go-github upgrades.
+The runnable demos under [`examples/`](examples/) live in their own sub-module and pin a specific go-github version (currently `v90`) so the kit's main `go.mod` stays clean across go-github upgrades.
## Development
```sh
make test # go test -race ./...
make test-unit # short tests only
-make test-live # the live ETag drift probe (needs GITHUB_TOKEN)
-make test-fuzz # fuzz the ETag hash for 30s
+make test-live # the live probes: etag, retry, pages, polling (needs GITHUB_TOKEN)
+make test-fuzz # fuzz the ETag hash and Retry-After parser, 30s each
make lint # golangci-lint v2
make vuln # govulncheck on the module
make bench # write benchmarks to dist/bench-current.txt
diff --git a/SECURITY.md b/SECURITY.md
index 4c753d5..c6b3838 100644
--- a/SECURITY.md
+++ b/SECURITY.md
@@ -46,7 +46,7 @@ Code in this repository:
### Out of scope: `google/go-github`
-`ghkit.New` is generic over the returned client type, and **no part of this kit's main module imports `github.com/google/go-github`**. It is absent from the kit's `go.mod`, source tree, and compiled binary. Consumers wire whichever `go-github` major they choose via the generic factory (see the [README's "Using a different go-github version"](https://github.com/pcanilho/go-github-kit#using-a-different-go-github-version) section, and runnable starters in [`examples/`](https://github.com/pcanilho/go-github-kit/tree/main/examples)).
+`ghkit.New` and `ghkit.NewE` are generic over the returned client type, and **no part of this kit's main module imports `github.com/google/go-github`**. It is absent from the kit's `go.mod`, source tree, and compiled binary. Consumers wire whichever `go-github` major they choose via the generic factory (see the [README's "Using a different go-github version"](https://github.com/pcanilho/go-github-kit#using-a-different-go-github-version) section, and runnable starters in [`examples/`](https://github.com/pcanilho/go-github-kit/tree/main/examples)).
`go-github` vulnerabilities are therefore not in scope for this kit's advisories. Please report and track them via the [`google/go-github` advisory channel](https://github.com/google/go-github/security) directly. The same applies to any other client library you wire into the generic factory.
diff --git a/TESTING.md b/TESTING.md
index 530722a..b8474ff 100644
--- a/TESTING.md
+++ b/TESTING.md
@@ -1,10 +1,16 @@
# Testing code that uses ghkit
-The `ghtest` sub-package ships two helpers for the GitHub-specific traps in
-testing ghkit-using code: secondary-rate-limit classification
-(`WriteSecondaryLimit`) and the bored-engineer ETag hash domain
-(`Write304IfMatch`). Everything else is plain stdlib code, shown inline as
-recipes you can copy.
+The `ghtest` sub-package ships four helpers for the GitHub-specific traps in
+testing ghkit-using code:
+
+| Helper | What it covers |
+|---|---|
+| `WriteSecondaryLimit` | 403 plus `Retry-After` and the `documentation_url` suffix go-github classifies on |
+| `Write304IfMatch` | The bored-engineer ETag hash domain, comma-splitting `If-None-Match` correctly |
+| `ETagServer` | A whole `httptest.Server` that computes ETags and synthesises 304s, so you do not hand-roll the handler |
+| `LinkHeader` | RFC 8288 `Link` headers for pagination fixtures |
+
+Everything else is plain stdlib code, shown inline as recipes you can copy.
`ghtest` is shape-correct, not behaviour-correct. It does not enforce
rate-limit budgets or run a real ETag database. For behaviour fidelity,
@@ -12,8 +18,8 @@ run integration tests against `api.github.com` with a throwaway token.
## Routing a ghkit-built client at a test server
-go-github's `*Client` has a `BaseURL` field. Point it at the test server
-URL and every request the SDK builds is sent there instead of
+Pass the test server URL to `github.WithURLs` at construction and every
+request the SDK builds is sent there instead of
api.github.com.
```go
@@ -22,10 +28,9 @@ package myservice_test
import (
"net/http"
"net/http/httptest"
- "net/url"
"testing"
- "github.com/google/go-github/v85/github"
+ "github.com/google/go-github/v90/github"
ghkit "github.com/pcanilho/go-github-kit"
)
@@ -37,9 +42,9 @@ func TestRouting(t *testing.T) {
hc, err := ghkit.HTTPClient(ghkit.WithToken("test"))
if err != nil { t.Fatal(err) }
- gh := github.NewClient(hc)
- base, _ := url.Parse(srv.URL + "/")
- gh.BaseURL = base // trailing slash required; go-github appends API paths
+ base := srv.URL + "/"
+ gh, err := github.NewClient(github.WithHTTPClient(hc), github.WithURLs(&base, nil))
+ if err != nil { t.Fatal(err) }
_ = gh
}
```
@@ -52,10 +57,9 @@ package myservice_test
import (
"net/http"
"net/http/httptest"
- "net/url"
"testing"
- "github.com/google/go-github/v85/github"
+ "github.com/google/go-github/v90/github"
ghkit "github.com/pcanilho/go-github-kit"
"github.com/pcanilho/go-github-kit/etag"
"github.com/pcanilho/go-github-kit/ghtest"
@@ -76,9 +80,9 @@ func TestETag304(t *testing.T) {
defer srv.Close()
hc, _ := ghkit.HTTPClient(ghkit.WithToken("test"), ghkit.WithETagCache())
- gh := github.NewClient(hc)
- base, _ := url.Parse(srv.URL + "/")
- gh.BaseURL = base
+ base := srv.URL + "/"
+ gh, err := github.NewClient(github.WithHTTPClient(hc), github.WithURLs(&base, nil))
+ if err != nil { t.Fatal(err) }
_ = gh
// drive your service: first call primes the cache, second call sees 304
}
diff --git a/doc.go b/doc.go
index 1cb74be..b52758d 100644
--- a/doc.go
+++ b/doc.go
@@ -1,11 +1,18 @@
// Package ghkit bundles ETag caching, rate limiting, retry on transient
// failures, and a proactive token bucket behind a single options-pattern
-// API. New is generic over the returned client type, so ghkit has no
-// compile-time dependency on any specific GitHub SDK; pass any
-// func(*http.Client) T factory at the call site:
-// github.com/google/go-github's NewClient for REST,
-// github.com/shurcooL/githubv4's NewClient for GraphQL, or any other
-// client constructor that takes an *http.Client.
+// API. New and NewE are generic over the returned client type, so ghkit
+// has no compile-time dependency on any specific GitHub SDK, and no ghkit
+// release ever forces an SDK version on you.
+//
+// New takes a func(*http.Client) T factory, which fits
+// github.com/shurcooL/githubv4's NewClient. NewE takes a
+// func(*http.Client) (T, error) factory, for constructors that can fail.
+// HTTPClient returns the *http.Client on its own if you would rather
+// construct the SDK client yourself, which is the plainest option for
+// github.com/google/go-github v87 and later:
+//
+// hc, err := ghkit.HTTPClient(ghkit.WithToken(tok))
+// gh, err := github.NewClient(github.WithHTTPClient(hc))
//
// Transport stack (outer -> inner, each layer optional):
//
@@ -49,8 +56,8 @@
// 2. ghkit is auth-free; the SDK owns auth via per-call cloning. Omit
// WithToken/WithTokenSource. Build one ghkit HTTPClient at startup,
// hand it to your SDK, and let the SDK inject the current token per
-// call (e.g. go-github's (*Client).WithAuthToken, which clones the
-// go-github Client above ghkit's shared transport). The ETag LRU and
+// call (e.g. go-github's WithAuthToken option, which sets auth above
+// ghkit's shared transport). The ETag LRU and
// rate-limit bucket persist across token rotation. This is the
// canonical pattern for Kubernetes operators that reconcile with a
// per-reconcile installation token.
@@ -68,7 +75,7 @@
// # GraphQL / v4 compatibility
//
// HTTPClient returns an *http.Client usable with any GraphQL v4 library
-// (e.g. github.com/shurcooL/githubv4). The etag layer no-ops on POST, so
+// (e.g. github.com/shurcooL/githubv4). The etag layer no-ops on anything but GET, so
// v4 traffic flows through oauth2 + retry + ratelimit + throttle + UA
// without ETag caching. Use WithETagCache only when you also issue REST
// GETs through the same client.
diff --git a/etag/algo.go b/etag/algo.go
index b054659..6ac811e 100644
--- a/etag/algo.go
+++ b/etag/algo.go
@@ -10,11 +10,19 @@ import (
"strings"
)
+// Header names in the ETag hash domain. headerAccept is also used by the
+// cache-key variant set (variantHeaders in transport.go).
+const (
+ headerAccept = "Accept"
+ headerAuthorization = "Authorization"
+ headerCookie = "Cookie"
+)
+
// varyHeaders is the canonical list of response-Vary header names GitHub
// participates in. Iteration order is part of the algorithm contract; do not
// alphabetize or reorder. Unexported to prevent mutation of in-process state;
// callers who need the list call VaryHeaders().
-var varyHeaders = []string{"Accept", "Authorization", "Cookie"}
+var varyHeaders = []string{headerAccept, headerAuthorization, headerCookie}
// VaryHeaders returns an immutable copy of the canonical Vary header list.
// Mutating the returned slice does not affect internal state.
@@ -95,8 +103,12 @@ func ParseVary(h http.Header) []string {
// cacheable returns true for requests whose responses carry an ETag we can
// revalidate against. The request-side checks run before we issue any call;
// response-side checks (cacheableResponse) run after we read the response.
+//
+// GET only. A HEAD response carries no body (net/http sets http.NoBody), so
+// hashing it against the server's ETag mismatches every time and feeds the
+// drift detector false positives.
func cacheable(req *http.Request) bool {
- if req.Method != http.MethodGet && req.Method != http.MethodHead {
+ if req.Method != http.MethodGet {
return false
}
if req.Header.Get("Range") != "" {
diff --git a/etag/algo_fuzz_test.go b/etag/algo_fuzz_test.go
index b33102a..0340a2d 100644
--- a/etag/algo_fuzz_test.go
+++ b/etag/algo_fuzz_test.go
@@ -24,7 +24,7 @@ func FuzzETag_ComputeExpectedETag(f *testing.F) {
h["Accept"] = strings.Split(accept, "\n")
}
if auth != "" {
- h["Authorization"] = strings.Split(auth, "\n")
+ h[headerAuthorization] = strings.Split(auth, "\n")
}
if cookie != "" {
h["Cookie"] = strings.Split(cookie, "\n")
diff --git a/etag/algo_test.go b/etag/algo_test.go
index 5747cb0..1819121 100644
--- a/etag/algo_test.go
+++ b/etag/algo_test.go
@@ -8,7 +8,7 @@ import (
)
func TestETag_Hash_Deterministic(t *testing.T) {
- h := http.Header{"Accept": {"application/vnd.github.v3+json"}, "Authorization": {"token abc"}}
+ h := http.Header{headerAccept: {"application/vnd.github.v3+json"}, headerAuthorization: {"token abc"}}
a := ComputeExpectedETag(h, nil, []byte("hello"))
b := ComputeExpectedETag(h, nil, []byte("hello"))
if a != b {
@@ -17,17 +17,17 @@ func TestETag_Hash_Deterministic(t *testing.T) {
}
func TestETag_Hash_VaryFallbackOnEmpty(t *testing.T) {
- h := http.Header{"Accept": {"a"}, "Authorization": {"b"}, "Cookie": {"c"}}
+ h := http.Header{headerAccept: {"a"}, headerAuthorization: {"b"}, "Cookie": {"c"}}
withNil := ComputeExpectedETag(h, nil, []byte("body"))
- withAll := ComputeExpectedETag(h, []string{"Accept", "Authorization", "Cookie"}, []byte("body"))
+ withAll := ComputeExpectedETag(h, []string{"Accept", headerAuthorization, "Cookie"}, []byte("body"))
if withNil != withAll {
t.Fatalf("nil vary should equal explicit full list; %q vs %q", withNil, withAll)
}
}
func TestETag_Hash_DifferentAuthProducesDifferentHash(t *testing.T) {
- h1 := http.Header{"Authorization": {"token-1"}}
- h2 := http.Header{"Authorization": {"token-2"}}
+ h1 := http.Header{headerAuthorization: {"token-1"}}
+ h2 := http.Header{headerAuthorization: {"token-2"}}
if ComputeExpectedETag(h1, nil, []byte("x")) == ComputeExpectedETag(h2, nil, []byte("x")) {
t.Fatal("different auth must produce different hash")
}
@@ -46,10 +46,10 @@ func TestETag_Hash_UnknownVaryHeaderWritesEmptyValue(t *testing.T) {
func TestETag_NormaliseETag_StripsWeakAndQuotes(t *testing.T) {
cases := []struct{ in, want string }{
- {`"abc"`, "abc"},
- {`W/"abc"`, "abc"},
- {`abc`, "abc"},
- {` W/"abc"`, "abc"}, // leading whitespace must be trimmed
+ {`"abc"`, testTokenABC},
+ {`W/"abc"`, testTokenABC},
+ {`abc`, testTokenABC},
+ {` W/"abc"`, testTokenABC}, // leading whitespace must be trimmed
}
for _, c := range cases {
if got := NormaliseETag(c.in); got != c.want {
@@ -63,7 +63,7 @@ func TestETag_ParseVary_OrdersAndDedups(t *testing.T) {
h.Add("Vary", "Accept, Authorization")
h.Add("Vary", "Cookie")
got := ParseVary(h)
- want := []string{"Accept", "Authorization", "Cookie"}
+ want := []string{"Accept", headerAuthorization, "Cookie"}
if len(got) != len(want) {
t.Fatalf("ParseVary len = %d; want %d (%v)", len(got), len(want), got)
}
@@ -93,9 +93,9 @@ func TestETag_Cacheable(t *testing.T) {
header http.Header
want bool
}{
- {"GET cacheable", "GET", "/users/octocat", nil, true},
- {"HEAD cacheable", "HEAD", "/users/octocat", nil, true},
- {"POST not cacheable", "POST", "/users/octocat", nil, false},
+ {"GET cacheable", "GET", testPathOctocat, nil, true},
+ {"HEAD not cacheable", "HEAD", testPathOctocat, nil, false},
+ {"POST not cacheable", "POST", testPathOctocat, nil, false},
{"PUT not cacheable", "PUT", "/x", nil, false},
{"Range request not cacheable", "GET", "/x", http.Header{"Range": {"bytes=0-100"}}, false},
{"/rate_limit not cacheable", "GET", "/rate_limit", nil, false},
@@ -156,12 +156,12 @@ func TestETag_NormalisePath(t *testing.T) {
"/repos/google/go-github": "/repos/{o}/{r}",
"/repos/google/go-github/commits/abc1234567": "/repos/{o}/{r}/commits/{sha}",
"/repos/google/go-github/compare/main...feature": "/repos/{o}/{r}/compare/{base...head}",
- "/users/octocat": "/users/{u}",
- "/orgs/github": "/orgs/{o}",
- "/app/installations/12345": "/app/installations/{id}",
- "/meta": "/meta",
- "/gists/1234": "/gists/_", // unknown-route fallback
- "/unmapped": "unknown",
+ testPathOctocat: "/users/{u}",
+ "/orgs/github": "/orgs/{o}",
+ "/app/installations/12345": "/app/installations/{id}",
+ "/meta": "/meta",
+ "/gists/1234": "/gists/_", // unknown-route fallback
+ "/unmapped": "unknown",
}
for in, want := range cases {
if got := normalisePath(in); got != want {
@@ -174,7 +174,7 @@ func TestETag_NormalisePath(t *testing.T) {
// that ComputeExpectedETag returns 64-char hex (SHA256). Byte equality
// against the real server is gated by the -tags=live tests.
func TestETag_ComputeExpectedETag_GoldenShape(t *testing.T) {
- got := ComputeExpectedETag(http.Header{"Accept": {"application/json"}}, nil, []byte("hello"))
+ got := ComputeExpectedETag(http.Header{headerAccept: {"application/json"}}, nil, []byte("hello"))
if len(got) != 64 {
t.Fatalf("expected 64-char hex, got %d: %q", len(got), got)
}
@@ -183,3 +183,9 @@ func TestETag_ComputeExpectedETag_GoldenShape(t *testing.T) {
t.Fatalf("expected lowercase hex, got %q", got)
}
}
+
+// Shared fixtures for the etag package tests.
+const (
+ testPathOctocat = "/users/octocat"
+ testTokenABC = "abc"
+)
diff --git a/etag/drift.go b/etag/drift.go
index 97674d1..97bd79d 100644
--- a/etag/drift.go
+++ b/etag/drift.go
@@ -64,9 +64,12 @@ type Stats struct {
DegradedAt time.Time // zero when not degraded
TotalMismatches int64 // monotonic over Transport lifetime
- // TotalHits counts cache lookups that matched (transport.go:218 site).
+ // TotalHits counts cache lookups that matched (the hit path in
+ // RoundTrip). An entry with an empty body is not a hit: it is evicted
+ // and counted as a miss.
TotalHits int64
- // TotalMisses counts cache lookups that missed (transport.go:231 site).
+ // TotalMisses counts cache lookups that missed (the miss path in
+ // RoundTrip).
TotalMisses int64
// TotalStores counts wire-200 entries written to cache. Includes
// re-validated stores: a 200 whose ETag matched precompute also
@@ -74,7 +77,8 @@ type Stats struct {
// cache backend", not "stores of new entries".
TotalStores int64
// TotalBypasses aggregates uncached pass-throughs: bypass_oversize,
- // bypass_noncacheable, and the two no_etag_header sites.
+ // bypass_noncacheable, bypass_empty_body, and the two no_etag_header
+ // sites.
TotalBypasses int64
}
diff --git a/etag/drift_test.go b/etag/drift_test.go
index 0e9f4e0..3477e92 100644
--- a/etag/drift_test.go
+++ b/etag/drift_test.go
@@ -4,7 +4,6 @@ import (
"context"
"net/http"
"net/http/httptest"
- "net/url"
"strconv"
"sync"
"sync/atomic"
@@ -27,13 +26,15 @@ func tripSuccess(tr *Transport) {
}
}
-func mustParseURL(t *testing.T, raw string) *url.URL {
+// mustGetRequest builds the GET that cacheKey derives a key from. It
+// carries no variant headers, matching what doGet issues.
+func mustGetRequest(t *testing.T, raw string) *http.Request {
t.Helper()
- u, err := url.Parse(raw)
+ req, err := http.NewRequest(http.MethodGet, raw, nil)
if err != nil {
- t.Fatalf("url.Parse(%q): %v", raw, err)
+ t.Fatalf("http.NewRequest(%q): %v", raw, err)
}
- return u
+ return req
}
// newDriftTransport returns the concrete *Transport so unit tests can call
@@ -329,7 +330,7 @@ func TestDrift_MismatchUnderDegradedStillCachesEntry(t *testing.T) {
// First request: cache cold, server returns garbage ETag, validation
// fails. Cache must still take the entry.
_ = doGet(t, c, s.URL+"/repos/a/b").StatusCode
- cached, ok, err := tr.cache.Get(t.Context(), cacheKey(mustParseURL(t, s.URL+"/repos/a/b"), tr.scopeDigest))
+ cached, ok, err := tr.cache.Get(t.Context(), cacheKey(mustGetRequest(t, s.URL+"/repos/a/b"), tr.scopeDigest))
if err != nil {
t.Fatalf("cache.Get: %v", err)
}
diff --git a/etag/event.go b/etag/event.go
index e771aa4..c5dbf58 100644
--- a/etag/event.go
+++ b/etag/event.go
@@ -16,6 +16,7 @@ const (
KindMiss Kind = "miss"
KindBypassOversize Kind = "bypass_oversize"
KindBypassNoncache Kind = "bypass_noncacheable"
+ KindBypassEmptyBody Kind = "bypass_empty_body"
KindNoEtagHeader Kind = "no_etag_header"
KindValidatedOK Kind = "validated_ok"
KindMismatch Kind = "mismatch"
diff --git a/etag/event_test.go b/etag/event_test.go
index 8315ccc..58acd0b 100644
--- a/etag/event_test.go
+++ b/etag/event_test.go
@@ -90,7 +90,7 @@ func TestEventCallback_URLPopulated(t *testing.T) {
r := &recorder{}
c := newTestClient(t, WithEventCallback(r.record))
- doGet(t, c, s.URL+"/users/octocat")
+ doGet(t, c, s.URL+testPathOctocat)
for _, e := range r.snapshot() {
if e.Kind == KindDriftDetected || e.Kind == KindDriftRecovered {
@@ -102,7 +102,7 @@ func TestEventCallback_URLPopulated(t *testing.T) {
if !strings.Contains(e.URL.Host, "127.0.0.1") && !strings.Contains(e.URL.Host, "localhost") {
t.Fatalf("unexpected host: %s", e.URL.Host)
}
- if e.URL.Path != "/users/octocat" {
+ if e.URL.Path != testPathOctocat {
t.Fatalf("Path=%s, want /users/octocat", e.URL.Path)
}
}
@@ -114,7 +114,7 @@ func TestEventCallback_PathTemplateNormalised(t *testing.T) {
r := &recorder{}
c := newTestClient(t, WithEventCallback(r.record))
- doGet(t, c, s.URL+"/users/octocat")
+ doGet(t, c, s.URL+testPathOctocat)
stores := r.byKind(KindStore)
if len(stores) != 1 {
@@ -443,6 +443,7 @@ func TestEventCallback_KindStringsMatchSlogKindAttribute(t *testing.T) {
{"RemoveError", KindRemoveError, "remove_error"},
{"BypassOversize", KindBypassOversize, "bypass_oversize"},
{"BypassNoncache", KindBypassNoncache, "bypass_noncacheable"},
+ {"BypassEmptyBody", KindBypassEmptyBody, "bypass_empty_body"},
{"NoEtagHeader", KindNoEtagHeader, "no_etag_header"},
{"ValidatedOK", KindValidatedOK, "validated_ok"},
{"InvalidatedGone", KindInvalidatedGone, "invalidated_gone"},
diff --git a/etag/head_test.go b/etag/head_test.go
new file mode 100644
index 0000000..d4234e7
--- /dev/null
+++ b/etag/head_test.go
@@ -0,0 +1,318 @@
+package etag
+
+import (
+ "bytes"
+ "io"
+ "net/http"
+ "net/http/httptest"
+ "testing"
+ "time"
+)
+
+// doHead issues a HEAD and drains the body.
+func doHead(t *testing.T, c *http.Client, url string) (*http.Response, []byte) {
+ t.Helper()
+ req, err := http.NewRequest(http.MethodHead, url, nil)
+ if err != nil {
+ t.Fatalf("NewRequest HEAD: %v", err)
+ }
+ resp, err := c.Do(req)
+ if err != nil {
+ t.Fatalf("HEAD %s: %v", url, err)
+ }
+ body, readErr := io.ReadAll(resp.Body)
+ closeErr := resp.Body.Close()
+ if readErr != nil {
+ t.Fatalf("HEAD %s read body: %v", url, readErr)
+ }
+ if closeErr != nil {
+ t.Fatalf("HEAD %s Body.Close: %v", url, closeErr)
+ }
+ return resp, body
+}
+
+// A HEAD response has no body, so hashing it always mismatched and fed
+// the drift detector.
+func TestETag_HEADRecordsNoMismatch(t *testing.T) {
+ body := []byte(`{"hello":"world"}`)
+ s, _ := ghServer(t, body)
+ rt := newTestTransport(t)
+ c := &http.Client{Transport: rt}
+ tr := rt.(*Transport)
+
+ for range 3 {
+ doHead(t, c, s.URL+"/users/a")
+ }
+
+ got := tr.Stats()
+ if got.TotalMismatches != 0 {
+ t.Fatalf("HEAD recorded %d mismatch(es); want 0 (%+v)", got.TotalMismatches, got)
+ }
+ if got.TotalStores != 0 {
+ t.Fatalf("HEAD stored %d entr(ies); want 0 (%+v)", got.TotalStores, got)
+ }
+}
+
+// driftThreshold mismatches in driftWindow degrade the transport to
+// passive mode. HEAD traffic must not trip it.
+func TestETag_HEADDoesNotDegradeDrift(t *testing.T) {
+ body := []byte(`{"hello":"world"}`)
+ s, _ := ghServer(t, body)
+ rt := newTestTransport(t)
+ c := &http.Client{Transport: rt}
+ tr := rt.(*Transport)
+
+ for range driftThreshold + 2 {
+ doHead(t, c, s.URL+"/users/a")
+ }
+
+ if got := tr.Stats(); got.Degraded {
+ t.Fatalf("HEAD traffic degraded the drift detector: %+v", got)
+ }
+}
+
+// Mirror case: a HEAD hitting a GET-populated entry used to be handed
+// that entry's body.
+func TestETag_HEADAfterGETReturnsNoBody(t *testing.T) {
+ body := []byte(`{"hello":"world"}`)
+ s, _ := ghServer(t, body)
+ c := newTestClient(t)
+
+ if got := doGet(t, c, s.URL+"/users/a"); got.StatusCode != 200 {
+ t.Fatalf("seed GET status = %d", got.StatusCode)
+ }
+
+ resp, headBody := doHead(t, c, s.URL+"/users/a")
+ if len(headBody) != 0 {
+ t.Fatalf("HEAD returned %d body bytes; want 0", len(headBody))
+ }
+ if resp.StatusCode != http.StatusOK {
+ t.Fatalf("HEAD status = %d; want 200", resp.StatusCode)
+ }
+}
+
+// readBounded used to overwrite ContentLength with the empty body's
+// length.
+func TestETag_HEADPreservesContentLength(t *testing.T) {
+ body := []byte(`{"hello":"world"}`)
+ s, _ := ghServer(t, body)
+ c := newTestClient(t)
+
+ resp, _ := doHead(t, c, s.URL+"/users/a")
+ if resp.ContentLength != int64(len(body)) {
+ t.Fatalf("HEAD ContentLength = %d; want %d", resp.ContentLength, len(body))
+ }
+}
+
+// Once degraded, a HEAD-stored entry made a later GET receive a
+// synthesised 200 with an empty body.
+func TestETag_DegradedGETAfterHEADReturnsRealBody(t *testing.T) {
+ body := []byte(`{"hello":"world"}`)
+ s, _ := ghServer(t, body)
+ rt := newTestTransport(t)
+ c := &http.Client{Transport: rt}
+ tr := rt.(*Transport)
+
+ // Pre-fix, the HEAD stored an empty-bodied entry and the GET replayed it
+ // as an empty 200. DegradedAt must be recent: a zero value reads as past
+ // the cooldown and sends a recomputed probe, not the stored ETag.
+ doHead(t, c, s.URL+"/users/a")
+ tr.driftDegraded.Store(true)
+ tr.driftDegradedAt.Store(time.Now().UnixNano())
+
+ got := doGet(t, c, s.URL+"/users/a")
+ if got.StatusCode != http.StatusOK {
+ t.Fatalf("GET status = %d; want 200", got.StatusCode)
+ }
+ if !bytes.Equal(got.Body, body) {
+ t.Fatalf("GET body = %q; want %q", got.Body, body)
+ }
+}
+
+// doGetWith issues a GET with the given headers.
+func doGetWith(t *testing.T, c *http.Client, url string, hdr http.Header) getResult {
+ t.Helper()
+ req, err := http.NewRequest(http.MethodGet, url, nil)
+ if err != nil {
+ t.Fatalf("NewRequest: %v", err)
+ }
+ for k, vs := range hdr {
+ for _, v := range vs {
+ req.Header.Add(k, v)
+ }
+ }
+ resp, err := c.Do(req)
+ if err != nil {
+ t.Fatalf("GET %s: %v", url, err)
+ }
+ body, readErr := io.ReadAll(resp.Body)
+ closeErr := resp.Body.Close()
+ if readErr != nil {
+ t.Fatalf("read body: %v", readErr)
+ }
+ if closeErr != nil {
+ t.Fatalf("Body.Close: %v", closeErr)
+ }
+ return getResult{StatusCode: resp.StatusCode, Header: resp.Header, Body: body}
+}
+
+// Two Accept values are different representations; they used to share a
+// key and evict each other.
+func TestETag_AcceptVariantsDoNotShareAnEntry(t *testing.T) {
+ body := []byte(`{"hello":"world"}`)
+ s, _ := ghServer(t, body)
+ rt := newTestTransport(t)
+ c := &http.Client{Transport: rt}
+ tr := rt.(*Transport)
+
+ raw := http.Header{headerAccept: {"application/vnd.github.raw"}}
+ js := http.Header{headerAccept: {"application/vnd.github+json"}}
+
+ doGetWith(t, c, s.URL+"/users/a", raw)
+ doGetWith(t, c, s.URL+"/users/a", js)
+ // Both are now cached under their own key, so each repeat is a hit.
+ doGetWith(t, c, s.URL+"/users/a", raw)
+ doGetWith(t, c, s.URL+"/users/a", js)
+
+ got := tr.Stats()
+ if got.TotalMisses != 2 {
+ t.Fatalf("TotalMisses = %d; want 2 (one per Accept variant): %+v", got.TotalMisses, got)
+ }
+ if got.TotalHits != 2 {
+ t.Fatalf("TotalHits = %d; want 2: %+v", got.TotalHits, got)
+ }
+}
+
+// The API version is not in the hash domain, so the key names it
+// explicitly. Both spellings must land on one entry.
+func TestETag_APIVersionVariants(t *testing.T) {
+ body := []byte(`{"hello":"world"}`)
+ s, _ := ghServer(t, body)
+ rt := newTestTransport(t)
+ c := &http.Client{Transport: rt}
+ tr := rt.(*Transport)
+
+ doGetWith(t, c, s.URL+"/users/a", http.Header{"X-GitHub-Api-Version": {"2022-11-28"}})
+ doGetWith(t, c, s.URL+"/users/a", http.Header{"X-GitHub-Api-Version": {"2026-03-10"}})
+ if got := tr.Stats(); got.TotalMisses != 2 {
+ t.Fatalf("distinct API versions shared an entry: %+v", got)
+ }
+
+ // go-github spells it X-Github-Api-Version; net/http canonicalises
+ // both to the same key, so this must be a hit, not a third miss.
+ doGetWith(t, c, s.URL+"/users/a", http.Header{"X-Github-Api-Version": {"2022-11-28"}})
+ if got := tr.Stats(); got.TotalMisses != 2 || got.TotalHits != 1 {
+ t.Fatalf("header spelling split the entry: %+v", got)
+ }
+}
+
+// No variant header keeps the pre-1.7.0 key, so external caches skip a
+// cold pass in the common case.
+func TestETag_NoVariantHeadersKeepsBaseKey(t *testing.T) {
+ rt := newTestTransport(t)
+ tr := rt.(*Transport)
+ req := mustGetRequest(t, "https://api.github.com/users/a")
+
+ got := cacheKey(req, tr.scopeDigest)
+ want := "https://api.github.com/users/a|" + tr.scopeDigest
+ if got != want {
+ t.Fatalf("cacheKey = %q; want %q", got, want)
+ }
+}
+
+// Pre-1.7.0 stored HEAD responses under the key a GET uses. A persistent
+// Cache survives the upgrade; such an entry must not be replayed.
+func TestETag_PoisonedEmptyBodyEntryIsTreatedAsMiss(t *testing.T) {
+ body := []byte(`{"hello":"world"}`)
+ s, _ := ghServer(t, body)
+ rt := newTestTransport(t)
+ c := &http.Client{Transport: rt}
+ tr := rt.(*Transport)
+
+ // Hand-plant the entry a pre-1.7.0 HEAD would have left behind.
+ req := mustGetRequest(t, s.URL+"/users/a")
+ key := cacheKey(req, tr.scopeDigest)
+ // Must be the ETag the server really computes, or the 200 comes back
+ // for the wrong reason.
+ realETag := `"` + ComputeExpectedETag(req.Header, nil, body) + `"`
+ if err := tr.cache.Add(t.Context(), key, Entry{
+ ETag: realETag,
+ Body: []byte{},
+ Headers: http.Header{},
+ }); err != nil {
+ t.Fatalf("cache.Add: %v", err)
+ }
+ tr.driftDegraded.Store(true)
+ tr.driftDegradedAt.Store(time.Now().UnixNano())
+
+ got := doGet(t, c, s.URL+"/users/a")
+ if got.StatusCode != http.StatusOK {
+ t.Fatalf("status = %d; want 200", got.StatusCode)
+ }
+ if !bytes.Equal(got.Body, body) {
+ t.Fatalf("body = %q; want %q", got.Body, body)
+ }
+}
+
+// Replaying an empty body gives the caller nothing, and validating it
+// records a false mismatch.
+func TestETag_EmptyBodyResponseBypassesCache(t *testing.T) {
+ var hits int
+ s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+ hits++
+ w.Header().Set("ETag", `"an-etag-over-nothing"`)
+ w.WriteHeader(http.StatusOK)
+ }))
+ t.Cleanup(s.Close)
+
+ rt := newTestTransport(t)
+ c := &http.Client{Transport: rt}
+ tr := rt.(*Transport)
+
+ for range 3 {
+ if got := doGet(t, c, s.URL+"/empty"); got.StatusCode != http.StatusOK {
+ t.Fatalf("status = %d", got.StatusCode)
+ }
+ }
+
+ got := tr.Stats()
+ if got.TotalMismatches != 0 {
+ t.Fatalf("empty body recorded %d mismatch(es); want 0 (%+v)", got.TotalMismatches, got)
+ }
+ if got.TotalStores != 0 {
+ t.Fatalf("empty body stored %d entr(ies); want 0 (%+v)", got.TotalStores, got)
+ }
+ if got.TotalBypasses != 3 {
+ t.Fatalf("TotalBypasses = %d; want 3 (%+v)", got.TotalBypasses, got)
+ }
+}
+
+// Skipping is not enough: if the wire response is also empty, nothing
+// overwrites the entry and it lives forever.
+func TestETag_PoisonedEntryIsEvictedNotJustSkipped(t *testing.T) {
+ s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+ w.Header().Set("ETag", `"still-empty"`)
+ w.WriteHeader(http.StatusOK)
+ }))
+ t.Cleanup(s.Close)
+
+ rt := newTestTransport(t)
+ c := &http.Client{Transport: rt}
+ tr := rt.(*Transport)
+
+ req := mustGetRequest(t, s.URL+"/empty")
+ key := cacheKey(req, tr.scopeDigest)
+ if err := tr.cache.Add(t.Context(), key, Entry{
+ ETag: `"planted"`, Body: []byte{}, Headers: http.Header{},
+ }); err != nil {
+ t.Fatalf("cache.Add: %v", err)
+ }
+
+ doGet(t, c, s.URL+"/empty")
+
+ if _, ok, err := tr.cache.Get(t.Context(), key); err != nil {
+ t.Fatalf("cache.Get: %v", err)
+ } else if ok {
+ t.Fatal("poisoned entry survived; it must be evicted, not just skipped")
+ }
+}
diff --git a/etag/live_check_test.go b/etag/live_check_test.go
index 0a72d60..46f98ca 100644
--- a/etag/live_check_test.go
+++ b/etag/live_check_test.go
@@ -50,7 +50,7 @@ func TestETag_Live_DriftCheck(t *testing.T) {
t.Fatal(err)
}
req.Header.Set("Accept", "application/vnd.github.v3+json")
- req.Header.Set("Authorization", "token "+tok)
+ req.Header.Set(headerAuthorization, "token "+tok)
req.Header.Set("User-Agent", "go-github-kit-drift-check")
resp, err := client.Do(req)
diff --git a/etag/transport.go b/etag/transport.go
index 17ee5e3..fddaa33 100644
--- a/etag/transport.go
+++ b/etag/transport.go
@@ -31,6 +31,7 @@ import (
"net/url"
"slices"
"strconv"
+ "strings"
"sync"
"sync/atomic"
"time"
@@ -73,8 +74,9 @@ var (
)
// Transport is an http.RoundTripper that adds If-None-Match on cacheable
-// GET/HEAD requests and replays the cached body as a synthesised 200 when
-// the server answers with 304 Not Modified.
+// GET requests and replays the cached body as a synthesised 200 when the
+// server answers with 304 Not Modified. Every other method, HEAD included,
+// passes straight through; see cacheable in algo.go for why.
//
// Transport runs precompute-mode by default: the If-None-Match value is
// computed from the cached body and the CURRENT request headers, so cached
@@ -217,15 +219,8 @@ func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) {
if err != nil {
return nil, fmt.Errorf("etag: scopeFn: %w", err)
}
- key := cacheKey(req.URL, digest)
- entry, haveEntry, getErr := t.cache.Get(ctx, key)
- if getErr != nil {
- // Backend-side error (e.g. Redis network blip). Treat as a miss
- // and log; never fail the request on a cache-read failure.
- t.logEvent(ctx, "get_error", req.URL.Path, nil, nil)
- t.emit(ctx, Event{Kind: KindGetError, URL: origURL, PathTemplate: tmpl, Err: getErr})
- haveEntry = false
- }
+ key := cacheKey(req, digest)
+ entry, haveEntry := t.lookup(ctx, req, key, origURL, tmpl)
if haveEntry {
t.logEvent(ctx, "hit", req.URL.Path, &entry, nil)
t.totalHits.Add(1)
@@ -334,6 +329,18 @@ func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) {
return resp, nil
}
+ // Nothing to replay, and hashing it against the server's ETag would
+ // record a false drift mismatch. Bypass before validate.
+ if len(body) == 0 {
+ t.logEvent(ctx, "bypass_empty_body", req.URL.Path, nil, resp)
+ t.totalBypasses.Add(1)
+ t.emit(ctx, Event{
+ Kind: KindBypassEmptyBody, URL: origURL, PathTemplate: tmpl,
+ Status: resp.StatusCode, GitHubRequestID: resp.Header.Get("X-GitHub-Request-Id"),
+ })
+ return resp, nil
+ }
+
// Validation feeds the drift detector and the warn log. Storage
// proceeds in both cases: passive mode needs the latest server
// ETag to send, and precompute mode never reads entry.ETag.
@@ -420,6 +427,27 @@ func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) {
return resp, nil
}
+// lookup reads the cache. A read error is logged and treated as a miss.
+// An empty-bodied entry is evicted, not just skipped: skipping alone leaves
+// it immortal when the wire response is also empty, and bypasses the
+// 404/410 eviction, which is gated on having an entry.
+func (t *Transport) lookup(ctx context.Context, req *http.Request, key string, origURL *url.URL, tmpl string) (Entry, bool) {
+ entry, ok, err := t.cache.Get(ctx, key)
+ if err != nil {
+ t.logEvent(ctx, "get_error", req.URL.Path, nil, nil)
+ t.emit(ctx, Event{Kind: KindGetError, URL: origURL, PathTemplate: tmpl, Err: err})
+ return Entry{}, false
+ }
+ if ok && len(entry.Body) == 0 {
+ if rmErr := t.cache.Remove(ctx, key); rmErr != nil {
+ t.logEvent(ctx, "remove_error", req.URL.Path, nil, nil)
+ t.emit(ctx, Event{Kind: KindRemoveError, URL: origURL, PathTemplate: tmpl, Err: rmErr})
+ }
+ return Entry{}, false
+ }
+ return entry, ok
+}
+
// buildIfNoneMatch returns the If-None-Match value to send on a cache-hit
// request along with a probe marker. In precompute mode the value is the
// recomputed hash; in degraded mode it's entry.ETag verbatim, except every
@@ -509,12 +537,36 @@ func (t *Transport) emit(ctx context.Context, evt Event) {
t.eventCallback(ctx, evt)
}
-// cacheKey is the URL plus the per-Transport scope digest. Fragments are
-// stripped because they are never sent over the wire.
-func cacheKey(u *url.URL, scopeDigest string) string {
- stripped := *u
+// variantHeaders select a different representation of the same URL. Accept
+// is already in the ETag hash domain (algo.go); X-GitHub-Api-Version is not,
+// but GitHub serves a different shape per version.
+var variantHeaders = []string{headerAccept, "X-GitHub-Api-Version"}
+
+// cacheKey is the URL plus the per-Transport scope digest, plus a digest of
+// the variant headers when any are set. Fragments are stripped (never sent).
+// The suffix is omitted when no variant header is set, so those keys are
+// unchanged from before 1.7.0.
+func cacheKey(req *http.Request, scopeDigest string) string {
+ stripped := *req.URL
stripped.Fragment = ""
- return stripped.String() + "|" + scopeDigest
+ base := stripped.String() + "|" + scopeDigest
+
+ // Header.Values canonicalises, so "X-GitHub-Api-Version" and go-github's
+ // "X-Github-Api-Version" land on one key.
+ var variant strings.Builder
+ for _, name := range variantHeaders {
+ for _, v := range req.Header.Values(name) {
+ variant.WriteString(name)
+ variant.WriteByte(':')
+ variant.WriteString(v)
+ variant.WriteByte('\n')
+ }
+ }
+ if variant.Len() == 0 {
+ return base
+ }
+ sum := sha256.Sum256([]byte(variant.String()))
+ return base + "|" + hex.EncodeToString(sum[:])
}
// resolveScope returns the per-request scope digest. With WithKeyScope it
diff --git a/etag/transport_test.go b/etag/transport_test.go
index b859470..b4c2bb1 100644
--- a/etag/transport_test.go
+++ b/etag/transport_test.go
@@ -99,7 +99,7 @@ func TestETag_ColdMissStoresEtagThenSendsIfNoneMatch(t *testing.T) {
c := newTestClient(t)
// Cold miss.
- resp, err := c.Get(s.URL + "/users/octocat")
+ resp, err := c.Get(s.URL + testPathOctocat)
if err != nil {
t.Fatalf("get 1: %v", err)
}
@@ -111,7 +111,7 @@ func TestETag_ColdMissStoresEtagThenSendsIfNoneMatch(t *testing.T) {
// Warm request: server sees If-None-Match and returns 304, transport
// replays the cached body as a synthesised 200.
- resp2, err := c.Get(s.URL + "/users/octocat")
+ resp2, err := c.Get(s.URL + testPathOctocat)
if err != nil {
t.Fatalf("get 2: %v", err)
}
@@ -157,8 +157,8 @@ func TestETag_TokenRotationSurvival(t *testing.T) {
s, reqs := ghServer(t, body)
c := newTestClient(t)
- req1, _ := http.NewRequest("GET", s.URL+"/users/octocat", nil)
- req1.Header.Set("Authorization", "token AAA")
+ req1, _ := http.NewRequest("GET", s.URL+testPathOctocat, nil)
+ req1.Header.Set(headerAuthorization, "token AAA")
r1, err := c.Do(req1)
if err != nil {
t.Fatalf("req1: %v", err)
@@ -168,8 +168,8 @@ func TestETag_TokenRotationSurvival(t *testing.T) {
// Rotate token mid-stream. Passive mode would miss on the server side
// (different auth -> different ETag). Precompute mode recomputes with
// the CURRENT Authorization and still gets a 304 that we replay.
- req2, _ := http.NewRequest("GET", s.URL+"/users/octocat", nil)
- req2.Header.Set("Authorization", "token BBB")
+ req2, _ := http.NewRequest("GET", s.URL+testPathOctocat, nil)
+ req2.Header.Set(headerAuthorization, "token BBB")
r2, err := c.Do(req2)
if err != nil {
t.Fatalf("req2: %v", err)
@@ -227,7 +227,7 @@ func TestETag_WeakETag(t *testing.T) {
var buf bytes.Buffer
logger := slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelWarn}))
c := newTestClient(t, WithLogger(logger))
- resp, _ := c.Get(s.URL + "/users/octocat")
+ resp, _ := c.Get(s.URL + testPathOctocat)
_ = resp.Body.Close()
if strings.Contains(buf.String(), "etag_mismatch") {
t.Fatalf("weak ETag should validate; got mismatch log: %q", buf.String())
@@ -395,7 +395,7 @@ func TestETag_WriteInvalidation_404(t *testing.T) {
c := newTestClient(t)
// Seed.
- r1, err := c.Get(s.URL + "/users/octocat")
+ r1, err := c.Get(s.URL + testPathOctocat)
if err != nil {
t.Fatalf("seed: %v", err)
}
@@ -403,7 +403,7 @@ func TestETag_WriteInvalidation_404(t *testing.T) {
t.Fatalf("seed body close: %v", err)
}
// 404 triggers invalidation.
- r2, err := c.Get(s.URL + "/users/octocat")
+ r2, err := c.Get(s.URL + testPathOctocat)
if err != nil {
t.Fatalf("404 request: %v", err)
}
@@ -411,7 +411,7 @@ func TestETag_WriteInvalidation_404(t *testing.T) {
t.Fatalf("404 body close: %v", err)
}
// Post-invalidation: should be a cold miss (no If-None-Match).
- r3, err := c.Get(s.URL + "/users/octocat")
+ r3, err := c.Get(s.URL + testPathOctocat)
if err != nil {
t.Fatalf("post-invalidation: %v", err)
}
@@ -431,13 +431,13 @@ func TestETag_ConcurrentAccessSafe(t *testing.T) {
s, _ := ghServer(t, body)
c := newTestClient(t)
// Warm the cache first.
- _ = doGet(t, c, s.URL+"/users/octocat")
+ _ = doGet(t, c, s.URL+testPathOctocat)
var wg sync.WaitGroup
for range 16 {
wg.Go(func() {
for range 20 {
- r, err := c.Get(s.URL + "/users/octocat")
+ r, err := c.Get(s.URL + testPathOctocat)
if err != nil {
t.Errorf("get: %v", err)
return
@@ -456,7 +456,7 @@ func TestETag_MultiTenantIsolation(t *testing.T) {
// Server returns different bodies based on Authorization header.
s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
body := bodyA
- if strings.Contains(r.Header.Get("Authorization"), "B") {
+ if strings.Contains(r.Header.Get(headerAuthorization), "B") {
body = bodyB
}
expected := ComputeExpectedETag(r.Header, nil, body)
@@ -485,8 +485,8 @@ func TestETag_MultiTenantIsolation(t *testing.T) {
cB := &http.Client{Transport: rtB}
do := func(c *http.Client, tok string) []byte {
- req, _ := http.NewRequest("GET", s.URL+"/users/octocat", nil)
- req.Header.Set("Authorization", "token "+tok)
+ req, _ := http.NewRequest("GET", s.URL+testPathOctocat, nil)
+ req.Header.Set(headerAuthorization, "token "+tok)
r, err := c.Do(req)
if err != nil {
t.Fatal(err)
@@ -789,7 +789,7 @@ func TestETag_AutoKeyScope_MultiTenantIsolation(t *testing.T) {
bodyB := []byte(`{"tenant":"B"}`)
s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
body := bodyA
- if strings.Contains(r.Header.Get("Authorization"), "B") {
+ if strings.Contains(r.Header.Get(headerAuthorization), "B") {
body = bodyB
}
expected := ComputeExpectedETag(r.Header, nil, body)
@@ -818,9 +818,9 @@ func TestETag_AutoKeyScope_MultiTenantIsolation(t *testing.T) {
c := &http.Client{Transport: rt}
do := func(tenant, tok string) []byte {
- req, _ := http.NewRequest("GET", s.URL+"/users/octocat", nil)
+ req, _ := http.NewRequest("GET", s.URL+testPathOctocat, nil)
req = req.WithContext(context.WithValue(req.Context(), tenantKey{}, tenant))
- req.Header.Set("Authorization", "token "+tok)
+ req.Header.Set(headerAuthorization, "token "+tok)
r, err := c.Do(req)
if err != nil {
t.Fatal(err)
@@ -876,7 +876,7 @@ func TestETag_AutoKeyScope_EmptyScopeIsError(t *testing.T) {
}
c := &http.Client{Transport: rt}
- resp, err := c.Get(s.URL + "/users/octocat")
+ resp, err := c.Get(s.URL + testPathOctocat)
if err == nil {
_ = resp.Body.Close()
t.Fatal("want error from c.Get; got nil")
@@ -899,7 +899,7 @@ func TestETag_AutoKeyScope_ErrorPropagates(t *testing.T) {
}
c := &http.Client{Transport: rt}
- resp, err := c.Get(s.URL + "/users/octocat")
+ resp, err := c.Get(s.URL + testPathOctocat)
if err == nil {
_ = resp.Body.Close()
t.Fatal("want error from c.Get; got nil")
diff --git a/examples/README.md b/examples/README.md
index 71bf869..5839c96 100644
--- a/examples/README.md
+++ b/examples/README.md
@@ -2,7 +2,7 @@
Runnable demo programs for [`github.com/pcanilho/go-github-kit`](https://github.com/pcanilho/go-github-kit).
-This directory is a **separate Go module** (its own `go.mod`). The kit itself has no compile-time dependency on `github.com/google/go-github` (`ghkit.New` is generic over the returned client type), so the kit's main `go.mod` does not pin `go-github`. The examples here do, because they show concrete usage with `github.NewClient`. Keeping this module separate means a breaking `go-github` major bumps the examples without touching the kit's own dependency surface.
+This directory is a **separate Go module** (its own `go.mod`). The kit itself has no compile-time dependency on `github.com/google/go-github` (`ghkit.New` and `ghkit.NewE` are generic over the returned client type), so the kit's main `go.mod` does not pin `go-github`. The examples here do, because they show concrete usage with `github.NewClient`. Keeping this module separate means a breaking `go-github` major bumps the examples without touching the kit's own dependency surface, and without forcing anything on consumers. These examples currently pin **v90**.
## Examples are copy-paste templates, not directly installable
@@ -31,5 +31,6 @@ When **copying an example into your own project**: drop the `replace` directive
| `poll-workflow-run/` | Waits for a workflow run to reach `status="completed"` with `polling.As[*github.WorkflowRun]` + `WithDoneT` + `WithMaxWallClock` + `WithJitter`. |
| `search-issues/` | Walks `/search/issues` with `search.Issues[*github.Issue]`; surfaces `incomplete_results` and the 1000-result cap as `ErrResultCapHit`. |
| `conditional-fetch/` | Visible 304: `cond.Fetch[*github.Repository]` returns `cond.Unchanged` on the second call so downstream work can be skipped. |
+| `graphql-v4/` | GraphQL v4 via `shurcooL/githubv4`, whose `NewClient` still binds directly to `ghkit.New`. |
Each example reads its credentials from environment variables (e.g. `GITHUB_TOKEN`, `GITHUB_ENTERPRISE_TOKEN`) and exercises one or more REST endpoints. They will fail at the API call without valid credentials. That's expected; they're starting templates, not standalone tools.
diff --git a/examples/backfill/main.go b/examples/backfill/main.go
index ab79784..e8916b2 100644
--- a/examples/backfill/main.go
+++ b/examples/backfill/main.go
@@ -5,21 +5,24 @@ import (
"context"
"fmt"
"log"
+ "net/http"
"os"
- "github.com/google/go-github/v85/github"
+ "github.com/google/go-github/v90/github"
ghkit "github.com/pcanilho/go-github-kit"
"github.com/pcanilho/go-github-kit/etag"
)
func main() {
- gh, err := ghkit.New(github.NewClient,
+ gh, err := ghkit.NewE(func(hc *http.Client) (*github.Client, error) {
+ return github.NewClient(github.WithHTTPClient(hc))
+ },
ghkit.WithToken(os.Getenv("GITHUB_TOKEN")),
ghkit.WithETagCache(etag.WithCache(etag.NewLRUCache(8192))),
ghkit.WithRequestsPerSecond(1.3, 1),
)
if err != nil {
- log.Fatalf("ghkit.New: %v", err)
+ log.Fatalf("ghkit.NewE: %v", err)
}
repo, _, err := gh.Repositories.Get(context.Background(), "google", "go-github")
diff --git a/examples/conditional-fetch/main.go b/examples/conditional-fetch/main.go
index a9f56dd..be19b1a 100644
--- a/examples/conditional-fetch/main.go
+++ b/examples/conditional-fetch/main.go
@@ -12,7 +12,7 @@ import (
"net/http"
"os"
- "github.com/google/go-github/v85/github"
+ "github.com/google/go-github/v90/github"
ghkit "github.com/pcanilho/go-github-kit"
"github.com/pcanilho/go-github-kit/cond"
)
diff --git a/examples/github-enterprise/main.go b/examples/github-enterprise/main.go
index 15b428f..06b23e2 100644
--- a/examples/github-enterprise/main.go
+++ b/examples/github-enterprise/main.go
@@ -10,34 +10,27 @@ import (
"net/http"
"os"
- "github.com/google/go-github/v85/github"
+ "github.com/google/go-github/v90/github"
ghkit "github.com/pcanilho/go-github-kit"
)
func main() {
token := os.Getenv("GITHUB_ENTERPRISE_TOKEN")
- var factoryErr error
- gh, err := ghkit.New(func(hc *http.Client) *github.Client {
- c, ghErr := github.NewClient(hc).WithEnterpriseURLs(
- "https://github.example.com/api/v3/",
- "https://github.example.com/api/uploads/",
+ // NewE propagates the constructor error, so a bad enterprise URL stops
+ // us rather than yielding a github.com client.
+ gh, err := ghkit.NewE(func(hc *http.Client) (*github.Client, error) {
+ return github.NewClient(
+ github.WithHTTPClient(hc),
+ github.WithEnterpriseURLs(
+ "https://github.example.com/api/v3/",
+ "https://github.example.com/api/uploads/",
+ ),
+ github.WithUserAgent("my-app/1.0"),
)
- if ghErr != nil {
- factoryErr = ghErr
- return nil
- }
- c.UserAgent = "my-app/1.0"
- return c
}, ghkit.WithToken(token))
if err != nil {
- log.Fatalf("ghkit.New: %v", err)
- }
- if factoryErr != nil {
- log.Fatalf("WithEnterpriseURLs: %v", factoryErr)
- }
- if gh == nil {
- log.Fatal("enterprise client construction failed (see factory error)")
+ log.Fatalf("ghkit.NewE: %v", err)
}
repo, _, err := gh.Repositories.Get(context.Background(), "internal", "service-x")
diff --git a/examples/go.mod b/examples/go.mod
index 5fdeceb..c9ea6e7 100644
--- a/examples/go.mod
+++ b/examples/go.mod
@@ -1,10 +1,12 @@
module github.com/pcanilho/go-github-kit/examples
-go 1.26.5
+go 1.26
+
+toolchain go1.26.6
require (
github.com/bradleyfalzon/ghinstallation/v2 v2.19.0
- github.com/google/go-github/v85 v85.0.0
+ github.com/google/go-github/v90 v90.0.0
github.com/pcanilho/go-github-kit v1.0.0
github.com/shurcooL/githubv4 v0.0.0-20260209031235-2402fdf4a9ed
golang.org/x/oauth2 v0.36.0
diff --git a/examples/go.sum b/examples/go.sum
index 57e3e3d..65c693f 100644
--- a/examples/go.sum
+++ b/examples/go.sum
@@ -7,10 +7,10 @@ github.com/golang-jwt/jwt/v4 v4.5.2/go.mod h1:m21LjoU+eqJr34lmDMbreY2eSTRJ1cv77w
github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8=
github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU=
-github.com/google/go-github/v85 v85.0.0 h1:1+TLFX/akTFXK7o9Z9uAloQGufOn4ySa5DItUM1VWT4=
-github.com/google/go-github/v85 v85.0.0/go.mod h1:jYkBnqN+SzR2A2fGKYfbt6DEEQAyxeK0Q2XpPV9ZFsU=
github.com/google/go-github/v88 v88.0.0 h1:dZA9IKkPK1eXZj4ypngnpRj5FwdpTv4whix2PrQMP7M=
github.com/google/go-github/v88 v88.0.0/go.mod h1:rufTDgn2N45wjhukLTyxmvc9nilSp3mr3Rgtt6b1MPw=
+github.com/google/go-github/v90 v90.0.0 h1:EnX9HvTfqvuJbUSWu1/jLrYH6JJLMz0w0qfQVbTxPzE=
+github.com/google/go-github/v90 v90.0.0/go.mod h1:pLzt1FZURZyoTHT5/Z1UQY3b9fYyrbXH6aj7X+qgID4=
github.com/google/go-querystring v1.2.0 h1:yhqkPbu2/OH+V9BfpCVPZkNmUXhb2gBxJArfhIxNtP0=
github.com/google/go-querystring v1.2.0/go.mod h1:8IFJqpSRITyJ8QhQ13bmbeMBDfmeEJZD5A0egEOmkqU=
github.com/hashicorp/golang-lru/v2 v2.0.7 h1:a+bsQ5rvGLjzHuww6tVxozPZFVghXaHOwFs4luLUK2k=
diff --git a/examples/installation-token/main.go b/examples/installation-token/main.go
index efa9b46..5dc3c80 100644
--- a/examples/installation-token/main.go
+++ b/examples/installation-token/main.go
@@ -13,12 +13,15 @@ import (
"time"
"github.com/bradleyfalzon/ghinstallation/v2"
- "github.com/google/go-github/v85/github"
+ "github.com/google/go-github/v90/github"
ghkit "github.com/pcanilho/go-github-kit"
"github.com/pcanilho/go-github-kit/etag"
"golang.org/x/oauth2"
)
+// ghinstallation pins go-github v88. Only strings cross this boundary,
+// so the two majors do not meet.
+//
// Adapter from ghinstallation.Transport to oauth2.TokenSource. ghkit does
// not ship this bridge so the root module stays at four runtime deps;
// callers copy these few lines (or substitute ghait for KMS-backed signing).
@@ -66,7 +69,11 @@ func main() {
log.Fatalf("ghkit.HTTPClient: %v", err)
}
- gh := github.NewClient(hc)
+ gh, err := github.NewClient(github.WithHTTPClient(hc))
+ if err != nil {
+ log.Fatalf("github.NewClient: %v", err)
+ }
+
repo, _, err := gh.Repositories.Get(context.Background(), "google", "go-github")
if err != nil {
log.Fatalf("Repositories.Get: %v", err)
diff --git a/examples/list-all-repos/main.go b/examples/list-all-repos/main.go
index 05eb6ae..2c395d1 100644
--- a/examples/list-all-repos/main.go
+++ b/examples/list-all-repos/main.go
@@ -12,7 +12,7 @@ import (
"net/http"
"os"
- "github.com/google/go-github/v85/github"
+ "github.com/google/go-github/v90/github"
ghkit "github.com/pcanilho/go-github-kit"
"github.com/pcanilho/go-github-kit/pages"
)
diff --git a/examples/poll-workflow-run/main.go b/examples/poll-workflow-run/main.go
index 890090e..671603a 100644
--- a/examples/poll-workflow-run/main.go
+++ b/examples/poll-workflow-run/main.go
@@ -20,7 +20,7 @@ import (
"strconv"
"time"
- "github.com/google/go-github/v85/github"
+ "github.com/google/go-github/v90/github"
ghkit "github.com/pcanilho/go-github-kit"
"github.com/pcanilho/go-github-kit/polling"
"github.com/pcanilho/go-github-kit/retry"
diff --git a/examples/retry-on-flaky/main.go b/examples/retry-on-flaky/main.go
index 916b37a..5286f3e 100644
--- a/examples/retry-on-flaky/main.go
+++ b/examples/retry-on-flaky/main.go
@@ -11,13 +11,15 @@ import (
"os"
"time"
- "github.com/google/go-github/v85/github"
+ "github.com/google/go-github/v90/github"
ghkit "github.com/pcanilho/go-github-kit"
"github.com/pcanilho/go-github-kit/retry"
)
func main() {
- gh, err := ghkit.New(github.NewClient,
+ gh, err := ghkit.NewE(func(hc *http.Client) (*github.Client, error) {
+ return github.NewClient(github.WithHTTPClient(hc))
+ },
ghkit.WithToken(os.Getenv("GITHUB_TOKEN")),
ghkit.WithRetry(
retry.WithMaxAttempts(5),
@@ -40,7 +42,7 @@ func main() {
),
)
if err != nil {
- log.Fatalf("ghkit.New: %v", err)
+ log.Fatalf("ghkit.NewE: %v", err)
}
repo, _, err := gh.Repositories.Get(context.Background(), "google", "go-github")
diff --git a/examples/search-issues/main.go b/examples/search-issues/main.go
index 4c0e4f6..e8ee382 100644
--- a/examples/search-issues/main.go
+++ b/examples/search-issues/main.go
@@ -9,7 +9,7 @@ import (
"log"
"os"
- "github.com/google/go-github/v85/github"
+ "github.com/google/go-github/v90/github"
ghkit "github.com/pcanilho/go-github-kit"
"github.com/pcanilho/go-github-kit/search"
)
diff --git a/examples/static-pat/main.go b/examples/static-pat/main.go
index 5ff8d9c..edaa5c5 100644
--- a/examples/static-pat/main.go
+++ b/examples/static-pat/main.go
@@ -5,19 +5,22 @@ import (
"context"
"fmt"
"log"
+ "net/http"
"os"
- "github.com/google/go-github/v85/github"
+ "github.com/google/go-github/v90/github"
ghkit "github.com/pcanilho/go-github-kit"
)
func main() {
- gh, err := ghkit.New(github.NewClient,
+ gh, err := ghkit.NewE(func(hc *http.Client) (*github.Client, error) {
+ return github.NewClient(github.WithHTTPClient(hc))
+ },
ghkit.WithToken(os.Getenv("GITHUB_TOKEN")),
ghkit.WithETagCache(),
)
if err != nil {
- log.Fatalf("ghkit.New: %v", err)
+ log.Fatalf("ghkit.NewE: %v", err)
}
repo, _, err := gh.Repositories.Get(context.Background(), "google", "go-github")
diff --git a/ghkit.go b/ghkit.go
index 60d0fcd..a8a98fb 100644
--- a/ghkit.go
+++ b/ghkit.go
@@ -22,6 +22,7 @@ var (
ErrPreAuthedBaseWithAuth = errors.New("ghkit: WithBaseTransport with a non-*http.Transport base cannot be combined with WithToken or WithTokenSource")
ErrNonPositiveRPS = errors.New("ghkit: WithRequestsPerSecond requires rps > 0 and burst >= 1")
ErrNilFactory = errors.New("ghkit: New requires a non-nil factory function")
+ ErrETagTransportType = errors.New("ghkit: WithETagTransport: constructed transport is not an *etag.Transport")
)
// HTTPClient builds an *http.Client with the configured transport stack.
@@ -54,6 +55,17 @@ func HTTPClient(opts ...Option) (*http.Client, error) {
if err != nil {
return nil, fmt.Errorf("ghkit: etag: %w", err)
}
+ // NewTransport returns http.RoundTripper; comma-ok keeps this
+ // correct if that ever changes.
+ if len(cfg.etagTransportFns) > 0 {
+ et, ok := inner.(*etag.Transport)
+ if !ok {
+ return nil, fmt.Errorf("%w: got %T", ErrETagTransportType, inner)
+ }
+ for _, fn := range cfg.etagTransportFns {
+ fn(et)
+ }
+ }
rt = inner
}
@@ -119,27 +131,18 @@ func (t *userAgentTransport) RoundTrip(req *http.Request) (*http.Response, error
// compile-time dependency on any specific GitHub SDK; pass whichever
// constructor you use at the call site.
//
-// Canonical usage:
+// Use New for constructors that cannot fail, which take the *http.Client
+// as their only argument:
//
-// import "github.com/google/go-github/v85/github"
+// import "github.com/shurcooL/githubv4"
//
-// gh, err := ghkit.New(github.NewClient,
+// v4, err := ghkit.New(githubv4.NewClient,
// ghkit.WithToken(os.Getenv("GITHUB_TOKEN")),
-// ghkit.WithETagCache(),
// )
//
-// Custom construction (UserAgent, GitHub Enterprise BaseURL, any other
-// post-construction tweaks) goes inside a factory closure:
-//
-// gh, err := ghkit.New(func(hc *http.Client) *github.Client {
-// c := github.NewClient(hc)
-// c.UserAgent = "my-app/1.0"
-// return c
-// }, opts...)
-//
-// For GitHub Enterprise, call github.NewClient(hc).WithEnterpriseURLs(base, upload)
-// inside the closure. The base URL must end with a trailing slash; go-github
-// returns an error if it does not.
+// go-github v87 changed NewClient to
+// `NewClient(opts ...ClientOptionsFunc) (*Client, error)`, so it no longer
+// binds here. Use NewE, or build the client in two steps with HTTPClient.
//
// When factory is nil, New returns the zero value of T and ErrNilFactory.
// When HTTPClient returns an error (invalid option combination), New
@@ -156,6 +159,34 @@ func New[T any](factory func(*http.Client) T, opts ...Option) (T, error) {
return factory(hc), nil
}
+// NewE is New for SDK constructors that return an error:
+//
+// gh, err := ghkit.NewE(func(hc *http.Client) (*github.Client, error) {
+// return github.NewClient(github.WithHTTPClient(hc))
+// }, ghkit.WithToken(tok))
+//
+// go-github v87+ is variadic over its own options, so the closure is needed
+// either way; NewE just gives the constructor's error somewhere to go. Use
+// New for constructors that cannot fail, such as githubv4.NewClient.
+//
+// Factory errors are wrapped as "ghkit: factory: %w". A nil factory returns
+// the zero value of T and ErrNilFactory.
+func NewE[T any](factory func(*http.Client) (T, error), opts ...Option) (T, error) {
+ var zero T
+ if factory == nil {
+ return zero, ErrNilFactory
+ }
+ hc, err := HTTPClient(opts...)
+ if err != nil {
+ return zero, err
+ }
+ v, err := factory(hc)
+ if err != nil {
+ return zero, fmt.Errorf("ghkit: factory: %w", err)
+ }
+ return v, nil
+}
+
func validateConfig(c *config) error {
if c.token != "" && c.tokenSource != nil {
return ErrConflictingAuth
diff --git a/ghkit_test.go b/ghkit_test.go
index 572ccc6..9db0245 100644
--- a/ghkit_test.go
+++ b/ghkit_test.go
@@ -431,10 +431,7 @@ func (s *cooldownServer) ServeHTTP(w http.ResponseWriter, _ *http.Request) {
s.mu.Unlock()
if inCooldown {
- retryAfter := int(math.Ceil(time.Until(cooldownUntil).Seconds()))
- if retryAfter < 1 {
- retryAfter = 1
- }
+ retryAfter := max(int(math.Ceil(time.Until(cooldownUntil).Seconds())), 1)
w.Header().Set("Content-Type", "application/json")
w.Header().Set("Retry-After", strconv.Itoa(retryAfter))
w.WriteHeader(http.StatusForbidden)
@@ -494,9 +491,7 @@ func TestGHKit_E2E_PostCooldownReleaseBoundedByBurst(t *testing.T) {
defer cancel()
var trigWG sync.WaitGroup
- trigWG.Add(1)
- go func() {
- defer trigWG.Done()
+ trigWG.Go(func() {
req, _ := http.NewRequestWithContext(ctx, "GET", srv.URL, nil)
r, err := hc.Do(req)
if err != nil {
@@ -504,7 +499,7 @@ func TestGHKit_E2E_PostCooldownReleaseBoundedByBurst(t *testing.T) {
return
}
_ = r.Body.Close()
- }()
+ })
select {
case <-firstHitDone:
@@ -597,9 +592,7 @@ func TestGHKit_E2E_RateLimitParksDuringSecondaryLimit(t *testing.T) {
defer cancel()
var trigWG sync.WaitGroup
- trigWG.Add(1)
- go func() {
- defer trigWG.Done()
+ trigWG.Go(func() {
req, _ := http.NewRequestWithContext(ctx, "GET", srv.URL, nil)
r, err := hc.Do(req)
if err != nil {
@@ -607,7 +600,7 @@ func TestGHKit_E2E_RateLimitParksDuringSecondaryLimit(t *testing.T) {
return
}
_ = r.Body.Close()
- }()
+ })
select {
case <-firstHitDone:
@@ -718,14 +711,12 @@ func TestGHKit_E2E_ContextCancelDuringCooldown(t *testing.T) {
}
var trigWG sync.WaitGroup
- trigWG.Add(1)
- go func() {
- defer trigWG.Done()
+ trigWG.Go(func() {
r, err := hc.Get(srv.URL)
if err == nil {
_ = r.Body.Close()
}
- }()
+ })
select {
case <-firstHitDone:
diff --git a/go.mod b/go.mod
index 29496ba..b958087 100644
--- a/go.mod
+++ b/go.mod
@@ -1,6 +1,8 @@
module github.com/pcanilho/go-github-kit
-go 1.26.5
+go 1.26
+
+toolchain go1.26.6
require (
github.com/gofri/go-github-ratelimit/v2 v2.0.2
diff --git a/newe_test.go b/newe_test.go
new file mode 100644
index 0000000..edd601f
--- /dev/null
+++ b/newe_test.go
@@ -0,0 +1,130 @@
+package ghkit_test
+
+import (
+ "errors"
+ "net/http"
+ "testing"
+
+ ghkit "github.com/pcanilho/go-github-kit"
+ "github.com/pcanilho/go-github-kit/etag"
+ "golang.org/x/oauth2"
+)
+
+var errFactory = errors.New("constructor blew up")
+
+func TestGHKit_NewEHappyPath(t *testing.T) {
+ fc, err := ghkit.NewE(func(hc *http.Client) (*fakeClient, error) {
+ return &fakeClient{hc: hc}, nil
+ }, ghkit.WithToken("abc"), ghkit.WithRateLimitDisabled())
+ if err != nil {
+ t.Fatalf("NewE: %v", err)
+ }
+ if fc == nil || fc.hc == nil {
+ t.Fatal("NewE returned a client with no *http.Client")
+ }
+}
+
+func TestGHKit_NewENilFactory(t *testing.T) {
+ fc, err := ghkit.NewE[*fakeClient](nil, ghkit.WithToken("abc"))
+ if !errors.Is(err, ghkit.ErrNilFactory) {
+ t.Fatalf("want ErrNilFactory; got %v", err)
+ }
+ if fc != nil {
+ t.Fatalf("expected nil client on error; got %+v", fc)
+ }
+}
+
+// A factory error must stay distinguishable from ghkit's own sentinels.
+func TestGHKit_NewEWrapsFactoryError(t *testing.T) {
+ fc, err := ghkit.NewE(func(*http.Client) (*fakeClient, error) {
+ return nil, errFactory
+ }, ghkit.WithToken("abc"))
+ if !errors.Is(err, errFactory) {
+ t.Fatalf("factory error not wrapped: %v", err)
+ }
+ if errors.Is(err, ghkit.ErrConflictingAuth) {
+ t.Fatal("factory error must not read as a ghkit config error")
+ }
+ if fc != nil {
+ t.Fatalf("expected nil client on error; got %+v", fc)
+ }
+}
+
+func TestGHKit_NewEConfigErrorShortCircuits(t *testing.T) {
+ called := false
+ _, err := ghkit.NewE(func(*http.Client) (*fakeClient, error) {
+ called = true
+ return &fakeClient{}, nil
+ },
+ ghkit.WithToken("abc"),
+ ghkit.WithTokenSource(oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "x"})),
+ )
+ if !errors.Is(err, ghkit.ErrConflictingAuth) {
+ t.Fatalf("want ErrConflictingAuth; got %v", err)
+ }
+ if called {
+ t.Fatal("factory ran despite an invalid option combination")
+ }
+}
+
+// Must enable the ETag layer on its own, or the callback never fires.
+func TestGHKit_WithETagTransportWithoutETagCache(t *testing.T) {
+ var got *etag.Transport
+ hc, err := ghkit.HTTPClient(
+ ghkit.WithToken("abc"),
+ ghkit.WithETagTransport(func(tr *etag.Transport) { got = tr }),
+ )
+ if err != nil {
+ t.Fatalf("HTTPClient: %v", err)
+ }
+ if hc == nil {
+ t.Fatal("nil client")
+ }
+ if got == nil {
+ t.Fatal("WithETagTransport callback never fired without WithETagCache")
+ }
+ if s := got.Stats(); s.Degraded {
+ t.Fatalf("fresh transport reports degraded: %+v", s)
+ }
+}
+
+func TestGHKit_WithETagTransportAlongsideETagCache(t *testing.T) {
+ var got *etag.Transport
+ if _, err := ghkit.HTTPClient(
+ ghkit.WithToken("abc"),
+ ghkit.WithETagCache(),
+ ghkit.WithETagTransport(func(tr *etag.Transport) { got = tr }),
+ ); err != nil {
+ t.Fatalf("HTTPClient: %v", err)
+ }
+ if got == nil {
+ t.Fatal("callback never fired")
+ }
+}
+
+func TestGHKit_WithETagTransportNilFunc(t *testing.T) {
+ if _, err := ghkit.HTTPClient(
+ ghkit.WithToken("abc"),
+ ghkit.WithETagTransport(nil),
+ ); err != nil {
+ t.Fatalf("HTTPClient: %v", err)
+ }
+}
+
+// A shared helper and a call site can each want the handle.
+func TestGHKit_WithETagTransportAccumulates(t *testing.T) {
+ var first, second *etag.Transport
+ if _, err := ghkit.HTTPClient(
+ ghkit.WithToken("abc"),
+ ghkit.WithETagTransport(func(tr *etag.Transport) { first = tr }),
+ ghkit.WithETagTransport(func(tr *etag.Transport) { second = tr }),
+ ); err != nil {
+ t.Fatalf("HTTPClient: %v", err)
+ }
+ if first == nil || second == nil {
+ t.Fatalf("callbacks dropped: first=%v second=%v", first != nil, second != nil)
+ }
+ if first != second {
+ t.Fatal("callbacks received different transports")
+ }
+}
diff --git a/options.go b/options.go
index 4d3e8a7..6494664 100644
--- a/options.go
+++ b/options.go
@@ -32,8 +32,9 @@ type config struct {
timeout time.Duration
// ETag settings.
- etagEnabled bool
- etagOpts []etag.Option
+ etagEnabled bool
+ etagOpts []etag.Option
+ etagTransportFns []func(*etag.Transport)
// Reactive rate limiter (go-github-ratelimit).
rateLimitEnabled bool // set to false by WithRateLimitDisabled
@@ -102,6 +103,21 @@ func WithETagCache(opts ...etag.Option) Option {
})
}
+// WithETagTransport hands the constructed *etag.Transport to fn, so callers
+// can poll Stats(). Without it the transport is unreachable: HTTPClient
+// builds it internally and buries it under the layers above.
+//
+// Enables the ETag layer on its own. fn runs once before HTTPClient returns.
+// Repeated use accumulates rather than overwrites; a nil fn is ignored.
+func WithETagTransport(fn func(*etag.Transport)) Option {
+ return optionFunc(func(c *config) {
+ c.etagEnabled = true
+ if fn != nil {
+ c.etagTransportFns = append(c.etagTransportFns, fn)
+ }
+ })
+}
+
// WithRateLimit configures the reactive rate limiter (go-github-ratelimit).
// The rate limiter is ENABLED by default; call this only to register
// callbacks or tune sleep limits.
diff --git a/pages/bench_test.go b/pages/bench_test.go
index a1e3b27..72ff398 100644
--- a/pages/bench_test.go
+++ b/pages/bench_test.go
@@ -33,7 +33,7 @@ func benchPaginatedServer(b *testing.B, totalPages, perPage int) *httptest.Serve
}
body := make([]byte, 0, 64)
body = append(body, '[')
- for i := 0; i < perPage; i++ {
+ for i := range perPage {
if i > 0 {
body = append(body, ',')
}
diff --git a/pages/doc.go b/pages/doc.go
index 72768f5..a7b301d 100644
--- a/pages/doc.go
+++ b/pages/doc.go
@@ -21,4 +21,5 @@
// iteration after one yield. ErrInvalidLinkHeader is returned when the
// response Link header is structurally malformed; a header with no
// rel="next" is treated as a clean end of pagination, not an error.
+// ErrNilClient is returned when the supplied *http.Client is nil.
package pages
diff --git a/pages/live_check_test.go b/pages/live_check_test.go
index db80499..61701b4 100644
--- a/pages/live_check_test.go
+++ b/pages/live_check_test.go
@@ -15,17 +15,18 @@ import (
"github.com/pcanilho/go-github-kit/pages"
)
-// TestPages_Live_UserRepos walks /user/repos against api.github.com via
-// the real ghkit transport stack. It pins the iterator's behaviour
-// against actual GitHub Link headers so a server-side change in the
-// response shape (header ordering, rel quoting, additional params)
-// surfaces here rather than as a silent regression in production.
+// TestPages_Live_Commits walks a public repository's commits against
+// api.github.com, pinning the iterator against real Link headers so a
+// server-side shape change surfaces here.
//
-// Hard-fatals on missing GITHUB_TOKEN so the live gate can never be
-// silently skipped in CI. For local development:
+// The endpoint must be readable by CI's App installation token. /user/repos
+// and /repos/{o}/{r}/stargazers both 403 with "not accessible by
+// integration"; commits work, as the etag probe's single-commit URL shows.
+//
+// Hard-fatals on missing GITHUB_TOKEN. For local development:
//
// GITHUB_TOKEN=$(gh auth token) go test -tags=live -run TestPages_Live ./pages/...
-func TestPages_Live_UserRepos(t *testing.T) {
+func TestPages_Live_Commits(t *testing.T) {
tok := os.Getenv("GITHUB_TOKEN")
if tok == "" {
t.Fatal("GITHUB_TOKEN is required for the live pagination check; this gate is intentionally non-skippable.\n" +
@@ -53,7 +54,7 @@ func TestPages_Live_UserRepos(t *testing.T) {
var pagesWalked int
var totalItems int
- for resp, err := range pages.Pages(ctx, hc, "GET", "https://api.github.com/user/repos?per_page=1", headers) {
+ for resp, err := range pages.Pages(ctx, hc, "GET", "https://api.github.com/repos/octocat/Spoon-Knife/commits?per_page=1", headers) {
if err != nil {
t.Fatalf("walk error on page %d: %v", pagesWalked+1, err)
}
@@ -70,15 +71,14 @@ func TestPages_Live_UserRepos(t *testing.T) {
_ = resp.Body.Close()
totalItems += len(arr)
pagesWalked++
- // Cap the walk at 5 pages so the test does not hammer api.github.com
- // for accounts with thousands of repos.
+ // Cap the walk; Spoon-Knife has many commits.
if pagesWalked >= 5 {
break
}
}
if pagesWalked == 0 {
- t.Fatal("no pages walked; live endpoint may be down or the account has no repos")
+ t.Fatal("no pages walked; live endpoint may be down")
}
t.Logf("walked %d pages, %d items via Link header", pagesWalked, totalItems)
}
diff --git a/pages/pages.go b/pages/pages.go
index 630f75d..c35a5f5 100644
--- a/pages/pages.go
+++ b/pages/pages.go
@@ -17,6 +17,10 @@ import (
// error.
var ErrInvalidLinkHeader = errors.New("pages: malformed Link header")
+// ErrNilClient is returned by Pages and As when the supplied
+// *http.Client is nil.
+var ErrNilClient = errors.New("pages: nil *http.Client")
+
// Pages iterates paginated HTTP responses by following the
// Link: rel="next" header. The configured *http.Client carries the full
// transport stack (RateLimit, Throttle, Retry, oauth2, ETag in
@@ -41,7 +45,7 @@ func Pages(
) iter.Seq2[*http.Response, error] {
return func(yield func(*http.Response, error) bool) {
if client == nil {
- yield(nil, errors.New("pages: nil *http.Client"))
+ yield(nil, ErrNilClient)
return
}
next := url
diff --git a/polling/doc.go b/polling/doc.go
index 455608b..646297c 100644
--- a/polling/doc.go
+++ b/polling/doc.go
@@ -20,7 +20,8 @@
// closed by the caller's prior normal-yield iteration (per Poll's
// body-ownership contract). lastResp may be nil if WithChangeOnly
// suppressed every yield. ErrMaxWallClockExceeded wraps
-// context.DeadlineExceeded.
+// context.DeadlineExceeded. ErrNilClient is returned when the supplied
+// *http.Client is nil.
//
// Sharp edges:
//
@@ -35,8 +36,10 @@
// next inner round-trip, not synchronously.
//
// Determinism: production uses time.NewTimer + select-on-ctx. Tests
-// inject WithSleepFunc / WithNowFunc. Jitter is a deterministic
-// mid-point clamp; not applied when honoring Retry-After.
+// inject WithSleepFunc / WithNowFunc. Two jitter modes: WithJitter is a
+// deterministic mid-point offset, WithFullJitter samples uniformly so
+// concurrent pollers de-correlate. The last of the two applied wins.
+// Neither applies when honoring Retry-After.
//
// Body argument: the body []byte passed to Poll/As is not deep-copied;
// the iterator constructs a fresh bytes.NewReader(body) per attempt
diff --git a/polling/fulljitter_test.go b/polling/fulljitter_test.go
new file mode 100644
index 0000000..15e5ee0
--- /dev/null
+++ b/polling/fulljitter_test.go
@@ -0,0 +1,134 @@
+package polling
+
+import (
+ "io"
+ "math"
+ "net/http"
+ "net/http/httptest"
+ "testing"
+ "time"
+)
+
+func jsonServer(t *testing.T) *httptest.Server {
+ t.Helper()
+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+ _, _ = io.WriteString(w, `{}`)
+ }))
+ t.Cleanup(srv.Close)
+ return srv
+}
+
+func collectSleeps(t *testing.T, attempts int, opts ...Option) []time.Duration {
+ t.Helper()
+ srv := jsonServer(t)
+ sleeps, sleepOpt := captureSleeps()
+ for resp := range Poll(t.Context(), srv.Client(), http.MethodGet, srv.URL, nil, nil,
+ 100*time.Millisecond,
+ append(append([]Option{WithMaxAttempts(attempts)}, opts...), sleepOpt)...) {
+ if resp != nil {
+ _, _ = io.Copy(io.Discard, resp.Body)
+ _ = resp.Body.Close()
+ }
+ }
+ return *sleeps
+}
+
+// WithJitter stays deterministic.
+func TestPoll_WithJitterStaysDeterministic(t *testing.T) {
+ got := collectSleeps(t, 6, WithJitter(0.5))
+ if len(got) == 0 {
+ t.Fatal("no sleeps recorded")
+ }
+ const want = 125 * time.Millisecond // 100ms + (100ms * 0.5)/2
+ for i, d := range got {
+ if d != want {
+ t.Fatalf("sleep[%d] = %v; want %v", i, d, want)
+ }
+ }
+}
+
+func TestPoll_WithFullJitterStaysInBounds(t *testing.T) {
+ const interval = 100 * time.Millisecond
+ got := collectSleeps(t, 12, WithFullJitter(0.5))
+ if len(got) == 0 {
+ t.Fatal("no sleeps recorded")
+ }
+ lo := interval - (interval/2)/2 // interval - span/2, span = 50ms
+ hi := interval + (interval/2)/2
+ for i, d := range got {
+ if d < lo || d > hi {
+ t.Fatalf("sleep[%d] = %v; outside [%v, %v]", i, d, lo, hi)
+ }
+ }
+}
+
+// Successive intervals must differ, or pollers stay in lockstep.
+func TestPoll_WithFullJitterVaries(t *testing.T) {
+ got := collectSleeps(t, 12, WithFullJitter(1))
+ if len(got) < 2 {
+ t.Fatalf("need at least 2 sleeps, got %d", len(got))
+ }
+ allSame := true
+ for _, d := range got[1:] {
+ if d != got[0] {
+ allSame = false
+ break
+ }
+ }
+ if allSame {
+ t.Fatalf("all %d sleeps identical (%v); full jitter is not varying", len(got), got[0])
+ }
+}
+
+// frac=1 spans the documented range and must not escalate.
+func TestPoll_WithFullJitterDoesNotEscalate(t *testing.T) {
+ const interval = 100 * time.Millisecond
+ got := collectSleeps(t, 20, WithFullJitter(1))
+ for i, d := range got {
+ if d < interval/2 || d > 3*interval/2 {
+ t.Fatalf("sleep[%d] = %v; outside [%v, %v]", i, d, interval/2, 3*interval/2)
+ }
+ }
+}
+
+// Last-one-wins has to hold in both directions.
+func TestPoll_JitterOptionsLastOneWins(t *testing.T) {
+ t.Run("WithJitter after WithFullJitter is deterministic", func(t *testing.T) {
+ got := collectSleeps(t, 8, WithFullJitter(0.5), WithJitter(0.5))
+ if len(got) == 0 {
+ t.Fatal("no sleeps recorded")
+ }
+ const want = 125 * time.Millisecond
+ for i, d := range got {
+ if d != want {
+ t.Fatalf("sleep[%d] = %v; want deterministic %v", i, d, want)
+ }
+ }
+ })
+
+ t.Run("WithFullJitter after WithJitter varies", func(t *testing.T) {
+ got := collectSleeps(t, 12, WithJitter(1), WithFullJitter(1))
+ if len(got) < 2 {
+ t.Fatalf("need at least 2 sleeps, got %d", len(got))
+ }
+ allSame := true
+ for _, d := range got[1:] {
+ if d != got[0] {
+ allSame = false
+ break
+ }
+ }
+ if allSame {
+ t.Fatalf("all sleeps identical (%v); WithFullJitter did not win", got[0])
+ }
+ })
+}
+
+// float64(interval)*frac can round past MaxInt64. Must not panic.
+func TestPoll_FullJitterPathologicalInterval(t *testing.T) {
+ cfg := &config{jitter: 1, fullJitter: true}
+ got := nextSleep(cfg, nil, time.Duration(math.MaxInt64))
+ if got <= 0 {
+ t.Fatalf("nextSleep = %v; want a positive duration", got)
+ }
+}
diff --git a/polling/polling.go b/polling/polling.go
index e363e60..fd6b47f 100644
--- a/polling/polling.go
+++ b/polling/polling.go
@@ -9,6 +9,8 @@ import (
"io"
"iter"
"log/slog"
+ "math"
+ "math/rand/v2"
"net/http"
"time"
@@ -33,6 +35,10 @@ var ErrInvalidOption = errors.New("polling: invalid option")
// ErrPredicatePanic is yielded when WithDone or WithDoneT panics.
var ErrPredicatePanic = errors.New("polling: predicate panicked")
+// ErrNilClient is returned by Poll and As when the supplied
+// *http.Client is nil.
+var ErrNilClient = errors.New("polling: nil *http.Client")
+
// Option configures a polling iterator.
type Option func(*config)
@@ -43,6 +49,7 @@ type config struct {
maxAttempts int
maxWall time.Duration
jitter float64
+ fullJitter bool
honorRA bool
changeOnly bool
logger *slog.Logger
@@ -108,8 +115,28 @@ func WithMaxWallClock(d time.Duration) Option {
// WithJitter applies a deterministic mid-point jitter:
// interval + (interval * frac / 2), clamped to [interval/2, 3*interval/2].
// Frac is clamped to [0, 1]. Not applied when honoring Retry-After.
+//
+// Clears WithFullJitter, so the last of the two applied wins.
func WithJitter(frac float64) Option {
- return func(c *config) { c.jitter = frac }
+ return func(c *config) {
+ c.jitter = frac
+ c.fullJitter = false
+ }
+}
+
+// WithFullJitter applies uniform jitter over
+// [interval - span/2, interval + span/2] where span = interval * frac, so
+// concurrent pollers de-correlate. Frac is clamped to [0, 1]; at frac=1 the
+// range is [interval/2, 3*interval/2]. Not applied when honoring
+// Retry-After.
+//
+// Prefer this over WithJitter, which adds a fixed offset and leaves pollers
+// started together in step. The last of the two applied wins.
+func WithFullJitter(frac float64) Option {
+ return func(c *config) {
+ c.jitter = frac
+ c.fullJitter = true
+ }
}
// WithHonorRetryAfter (default true) honors the upstream Retry-After
@@ -230,7 +257,7 @@ func Poll(
) iter.Seq2[*http.Response, error] {
return func(yield func(*http.Response, error) bool) {
if c == nil {
- yield(nil, errors.New("polling: nil *http.Client"))
+ yield(nil, ErrNilClient)
return
}
if interval <= 0 {
@@ -440,10 +467,7 @@ func nextSleep(cfg *config, resp *http.Response, interval time.Duration) time.Du
if cfg.honorRA && resp != nil {
if d, ok := retry.RetryAfter(resp); ok {
lo := interval
- hi := interval
- if cfg.maxWall > hi {
- hi = cfg.maxWall
- }
+ hi := max(cfg.maxWall, interval)
if d < lo {
d = lo
}
@@ -455,8 +479,21 @@ func nextSleep(cfg *config, resp *http.Response, interval time.Duration) time.Du
}
d := interval
if cfg.jitter > 0 {
- span := time.Duration(float64(interval) * cfg.jitter)
- d = interval + span/2
+ // Clamp in float space. Out-of-range float64->int64 is
+ // implementation-defined: amd64 yields MinInt64, arm64 saturates to
+ // MaxInt64, so no post-conversion check catches both.
+ var span time.Duration
+ if f := float64(interval) * cfg.jitter; f >= 1 && f < float64(math.MaxInt64) {
+ span = time.Duration(f)
+ }
+ if cfg.fullJitter {
+ // Uniform across the span, centred on interval. Unlike retry's
+ // decorrelated backoff this must not drift upward.
+ //nolint:gosec // G404: math/rand/v2 is intentional for jitter; not a crypto context.
+ d = interval - span/2 + time.Duration(rand.Int64N(int64(span)+1))
+ } else {
+ d = interval + span/2
+ }
if d < interval/2 {
d = interval / 2
}
diff --git a/retry/nilbody_test.go b/retry/nilbody_test.go
new file mode 100644
index 0000000..0fc44c2
--- /dev/null
+++ b/retry/nilbody_test.go
@@ -0,0 +1,75 @@
+package retry
+
+import (
+ "errors"
+ "net/http"
+ "testing"
+ "time"
+)
+
+// A hand-rolled base transport can return a bare &http.Response with no
+// Body; WithBaseTransport allows that.
+type nilBodyTransport struct{ calls int }
+
+func (t *nilBodyTransport) RoundTrip(*http.Request) (*http.Response, error) {
+ t.calls++
+ if t.calls == 1 {
+ return &http.Response{StatusCode: http.StatusServiceUnavailable, Header: http.Header{}}, nil
+ }
+ return &http.Response{StatusCode: http.StatusOK, Header: http.Header{}}, nil
+}
+
+// Draining a nil Body used to panic.
+func TestRetry_NilResponseBodyDoesNotPanic(t *testing.T) {
+ base := &nilBodyTransport{}
+ rt, err := NewTransport(base,
+ WithMaxAttempts(3),
+ WithBackoff(time.Millisecond, 2*time.Millisecond),
+ )
+ if err != nil {
+ t.Fatalf("NewTransport: %v", err)
+ }
+
+ req, reqErr := http.NewRequest(http.MethodGet, "https://example.invalid/x", nil)
+ if reqErr != nil {
+ t.Fatalf("NewRequest: %v", reqErr)
+ }
+ resp, err := rt.RoundTrip(req)
+ if err != nil {
+ t.Fatalf("RoundTrip: %v", err)
+ }
+ if resp.StatusCode != http.StatusOK {
+ t.Fatalf("status = %d; want 200 after one retry", resp.StatusCode)
+ }
+ if base.calls != 2 {
+ t.Fatalf("base called %d times; want 2", base.calls)
+ }
+}
+
+// The Retry-After abort path drains too.
+func TestRetry_NilResponseBodyOnRetryAfterAbort(t *testing.T) {
+ base := retryAfterNilBody{}
+ rt, err := NewTransport(base,
+ WithMaxAttempts(3),
+ WithBackoff(time.Millisecond, time.Second),
+ )
+ if err != nil {
+ t.Fatalf("NewTransport: %v", err)
+ }
+
+ req, reqErr := http.NewRequest(http.MethodGet, "https://example.invalid/x", nil)
+ if reqErr != nil {
+ t.Fatalf("NewRequest: %v", reqErr)
+ }
+ if _, err := rt.RoundTrip(req); !errors.Is(err, ErrRetryAfterExceedsMax) {
+ t.Fatalf("got %v; want ErrRetryAfterExceedsMax", err)
+ }
+}
+
+type retryAfterNilBody struct{}
+
+func (retryAfterNilBody) RoundTrip(*http.Request) (*http.Response, error) {
+ h := http.Header{}
+ h.Set("Retry-After", "3600")
+ return &http.Response{StatusCode: http.StatusServiceUnavailable, Header: h}, nil
+}
diff --git a/retry/retry.go b/retry/retry.go
index 480bc60..adb4480 100644
--- a/retry/retry.go
+++ b/retry/retry.go
@@ -245,7 +245,7 @@ func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) {
}
if raOverride > t.maxDelay {
- if resp != nil {
+ if resp != nil && resp.Body != nil {
_, _ = io.CopyN(io.Discard, resp.Body, drainCap)
_ = resp.Body.Close()
}
@@ -262,9 +262,13 @@ func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) {
sleep = raOverride
}
+ // Body can be nil when a hand-rolled base transport supplied
+ // via WithBaseTransport returns a bare &http.Response.
if resp != nil {
- _, _ = io.CopyN(io.Discard, resp.Body, drainCap)
- _ = resp.Body.Close()
+ if resp.Body != nil {
+ _, _ = io.CopyN(io.Discard, resp.Body, drainCap)
+ _ = resp.Body.Close()
+ }
resp = nil
}
@@ -443,6 +447,10 @@ func parseRetryAfter(resp *http.Response) (time.Duration, parseOutcome) {
return 0, outcomeUnparseable
}
+// sourceJitter labels a sleep whose duration came from computeJitter
+// rather than an upstream Retry-After.
+const sourceJitter = "jitter"
+
func sourceLabel(raOverride time.Duration, outcome parseOutcome) string {
switch {
case raOverride > 0:
@@ -450,7 +458,7 @@ func sourceLabel(raOverride time.Duration, outcome parseOutcome) string {
case outcome == outcomeUnparseable:
return "malformed"
default:
- return "jitter"
+ return sourceJitter
}
}
diff --git a/retry/retry_fuzz_test.go b/retry/retry_fuzz_test.go
index 1e4c622..3c75b33 100644
--- a/retry/retry_fuzz_test.go
+++ b/retry/retry_fuzz_test.go
@@ -43,7 +43,7 @@ func FuzzParseRetryAfter(f *testing.F) {
}
switch sourceLabel(dur, outcome) {
- case "retry_after", "jitter", "malformed":
+ case "retry_after", sourceJitter, "malformed":
default:
t.Fatalf("unknown source label for (%v, %v) input %q", dur, outcome, header)
}
diff --git a/retry/retry_test.go b/retry/retry_test.go
index 704b4df..c3de7a9 100644
--- a/retry/retry_test.go
+++ b/retry/retry_test.go
@@ -694,11 +694,11 @@ func TestRetry_SourceLabel(t *testing.T) {
outcome parseOutcome
wantSrc string
}{
- {"absent", 0, outcomeAbsent, "jitter"},
+ {"absent", 0, outcomeAbsent, sourceJitter},
{"numeric-positive", 5 * time.Second, outcomeNumeric, "retry_after"},
- {"numeric-clamped-zero-stays-jitter", 0, outcomeNumeric, "jitter"},
+ {"numeric-clamped-zero-stays-jitter", 0, outcomeNumeric, sourceJitter},
{"date-future", 30 * time.Minute, outcomeDate, "retry_after"},
- {"date-past-clamped", 0, outcomeDate, "jitter"},
+ {"date-past-clamped", 0, outcomeDate, sourceJitter},
{"unparseable", 0, outcomeUnparseable, "malformed"},
}
for _, tc := range cases {
diff --git a/search/doc.go b/search/doc.go
index f7c8663..7f30640 100644
--- a/search/doc.go
+++ b/search/doc.go
@@ -5,7 +5,8 @@
// per page.
//
// Each Result[T] carries the per-page TotalCount and IncompleteResults
-// flag alongside the typed Item. ErrResultCapHit signals GitHub's
+// flag alongside the typed Item. ErrNilClient and ErrEmptyQuery guard the
+// arguments. ErrResultCapHit signals GitHub's
// 1000-result hard cap (a 422 after page 10).
//
// Four endpoints are exposed: Issues, Code, Repos, Users. They share
diff --git a/search/search.go b/search/search.go
index ed3323e..7dd84df 100644
--- a/search/search.go
+++ b/search/search.go
@@ -20,6 +20,13 @@ import (
// after page 10 (10 * per_page=100 = 1000 items).
var ErrResultCapHit = errors.New("search: GitHub 1000-result cap reached")
+// ErrNilClient is returned by the iterators when the supplied
+// *http.Client is nil.
+var ErrNilClient = errors.New("search: nil *http.Client")
+
+// ErrEmptyQuery is returned when the q parameter is empty.
+var ErrEmptyQuery = errors.New("search: q is required")
+
// capSubstring is undocumented in GitHub's status table. Pinned
// in tests so drift surfaces.
const capSubstring = "Only the first 1000 search results are available"
@@ -114,7 +121,7 @@ func iterate[T any](ctx context.Context, c *http.Client, path, q string, opts []
return func(yield func(Result[T], error) bool) {
var zero Result[T]
if c == nil {
- yield(zero, errors.New("search: nil *http.Client"))
+ yield(zero, ErrNilClient)
return
}
cfg := newConfig(opts)
@@ -178,7 +185,7 @@ func decodeEnvelope[T any](resp *http.Response) (envelope[T], error) {
func buildURL(base, q string, cfg *config) (string, error) {
if q == "" {
- return "", errors.New("search: q is required")
+ return "", ErrEmptyQuery
}
u, err := url.Parse(base)
if err != nil {
@@ -188,10 +195,7 @@ func buildURL(base, q string, cfg *config) (string, error) {
v.Set("q", q)
perPage := 100
if cfg.perPage > 0 {
- perPage = cfg.perPage
- if perPage > 100 {
- perPage = 100
- }
+ perPage = min(cfg.perPage, 100)
}
v.Set("per_page", strconv.Itoa(perPage))
if cfg.sort != "" {
diff --git a/search/search_test.go b/search/search_test.go
index 1b3b7d9..98da3e1 100644
--- a/search/search_test.go
+++ b/search/search_test.go
@@ -36,8 +36,8 @@ func envelopeServer(t *testing.T, totalPages, perPage int, incomplete bool) *htt
w.Header().Set("Link", link)
}
startID := (page - 1) * perPage
- var items []string
- for i := 0; i < perPage; i++ {
+ items := make([]string, 0, perPage)
+ for i := range perPage {
items = append(items, fmt.Sprintf(`{"id":%d,"title":"i-%d"}`, startID+i, startID+i))
}
incFlag := "false"
diff --git a/sentinels_test.go b/sentinels_test.go
new file mode 100644
index 0000000..deaf594
--- /dev/null
+++ b/sentinels_test.go
@@ -0,0 +1,111 @@
+package ghkit_test
+
+import (
+ "errors"
+ "iter"
+ "net/http"
+ "testing"
+
+ "github.com/pcanilho/go-github-kit/pages"
+ "github.com/pcanilho/go-github-kit/polling"
+ "github.com/pcanilho/go-github-kit/search"
+)
+
+// These used to be inline errors.New, unmatchable by errors.Is.
+func TestSentinels_NilClientIsMatchable(t *testing.T) {
+ t.Run("pages", func(t *testing.T) {
+ for _, err := range pages.Pages(t.Context(), nil, http.MethodGet, "https://x/y", nil) {
+ if !errors.Is(err, pages.ErrNilClient) {
+ t.Fatalf("got %v; want pages.ErrNilClient", err)
+ }
+ return
+ }
+ t.Fatal("iterator yielded nothing")
+ })
+
+ t.Run("polling", func(t *testing.T) {
+ for _, err := range polling.Poll(t.Context(), nil, http.MethodGet, "https://x/y", nil, nil, 0) {
+ if !errors.Is(err, polling.ErrNilClient) {
+ t.Fatalf("got %v; want polling.ErrNilClient", err)
+ }
+ return
+ }
+ t.Fatal("iterator yielded nothing")
+ })
+
+ t.Run("search", func(t *testing.T) {
+ for _, err := range search.Issues[map[string]any](t.Context(), nil, "q") {
+ if !errors.Is(err, search.ErrNilClient) {
+ t.Fatalf("got %v; want search.ErrNilClient", err)
+ }
+ return
+ }
+ t.Fatal("iterator yielded nothing")
+ })
+}
+
+// The As wrappers delegate to the same guards.
+func TestSentinels_NilClientViaWrappers(t *testing.T) {
+ t.Run("pages.As", func(t *testing.T) {
+ for _, err := range pages.As[map[string]any](t.Context(), nil, http.MethodGet, "https://x/y", nil) {
+ if !errors.Is(err, pages.ErrNilClient) {
+ t.Fatalf("got %v; want pages.ErrNilClient", err)
+ }
+ return
+ }
+ t.Fatal("iterator yielded nothing")
+ })
+
+ t.Run("polling.As", func(t *testing.T) {
+ for _, err := range polling.As[map[string]any](t.Context(), nil, http.MethodGet, "https://x/y", nil, nil, 0) {
+ if !errors.Is(err, polling.ErrNilClient) {
+ t.Fatalf("got %v; want polling.ErrNilClient", err)
+ }
+ return
+ }
+ t.Fatal("iterator yielded nothing")
+ })
+
+ for name, fn := range map[string]func() error{
+ "search.Code": func() error { return firstErr(search.Code[map[string]any](t.Context(), nil, "q")) },
+ "search.Repos": func() error { return firstErr(search.Repos[map[string]any](t.Context(), nil, "q")) },
+ "search.Users": func() error { return firstErr(search.Users[map[string]any](t.Context(), nil, "q")) },
+ } {
+ t.Run(name, func(t *testing.T) {
+ if err := fn(); !errors.Is(err, search.ErrNilClient) {
+ t.Fatalf("got %v; want search.ErrNilClient", err)
+ }
+ })
+ }
+}
+
+func firstErr[T any](seq iter.Seq2[search.Result[T], error]) error {
+ for _, err := range seq {
+ return err
+ }
+ return nil
+}
+
+func TestSentinels_MessagesUnchanged(t *testing.T) {
+ cases := map[error]string{
+ pages.ErrNilClient: "pages: nil *http.Client",
+ polling.ErrNilClient: "polling: nil *http.Client",
+ search.ErrNilClient: "search: nil *http.Client",
+ search.ErrEmptyQuery: "search: q is required",
+ }
+ for err, want := range cases {
+ if err.Error() != want {
+ t.Errorf("message drift: got %q, want %q", err.Error(), want)
+ }
+ }
+}
+
+func TestSentinels_EmptyQueryIsMatchable(t *testing.T) {
+ for _, err := range search.Issues[map[string]any](t.Context(), &http.Client{}, "") {
+ if !errors.Is(err, search.ErrEmptyQuery) {
+ t.Fatalf("got %v; want search.ErrEmptyQuery", err)
+ }
+ return
+ }
+ t.Fatal("iterator yielded nothing")
+}
diff --git a/tests/integration/integration_test.go b/tests/integration/integration_test.go
index d7dcabb..9325db9 100644
--- a/tests/integration/integration_test.go
+++ b/tests/integration/integration_test.go
@@ -144,10 +144,8 @@ func TestIntegration_ConcurrentPollingSameResource(t *testing.T) {
_, sleepOpt := captureSleeps()
var wg sync.WaitGroup
- for w := 0; w < 4; w++ {
- wg.Add(1)
- go func() {
- defer wg.Done()
+ for range 4 {
+ wg.Go(func() {
for v, err := range polling.As[*payload](
t.Context(), hc, http.MethodGet, srv.URL, nil, nil,
time.Millisecond,
@@ -157,7 +155,7 @@ func TestIntegration_ConcurrentPollingSameResource(t *testing.T) {
_ = v
_ = err
}
- }()
+ })
}
wg.Wait()
}
@@ -183,16 +181,14 @@ func TestIntegration_ConcurrentCondFetch(t *testing.T) {
}
var wg sync.WaitGroup
- for w := 0; w < 16; w++ {
- wg.Add(1)
- go func() {
- defer wg.Done()
+ for range 16 {
+ wg.Go(func() {
req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL, nil)
_, _, err := cond.Fetch(t.Context(), hc, req, decode)
if err != nil {
t.Errorf("Fetch: %v", err)
}
- }()
+ })
}
wg.Wait()
}