From a600c9afd01a53447190e96c7372e4137184fd48 Mon Sep 17 00:00:00 2001 From: Michael McQuade Date: Sun, 25 Oct 2026 22:21:11 -0500 Subject: [PATCH 1/4] refactor(generator): retry declines redirect refusals by one check net/http wraps every CheckRedirect error in a *url.Error, so the "stopped after" prefix check already covers ErrInsecureRedirect and ErrTooManyRedirects. The errors.Is check before it was a second path to the same answer. --- internal/templates/retry.go.tmpl | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/internal/templates/retry.go.tmpl b/internal/templates/retry.go.tmpl index 632f4ff..82d67b7 100644 --- a/internal/templates/retry.go.tmpl +++ b/internal/templates/retry.go.tmpl @@ -155,12 +155,9 @@ func isRetryableNetworkError(err error) bool { if errors.As(err, &recordErr) { return false } - if errors.Is(err, ErrInsecureRedirect) || errors.Is(err, ErrTooManyRedirects) { - return false - } var urlErr *url.Error if errors.As(err, &urlErr) { - // A caller's own CheckRedirect refuses the way net/http's does, with an error that starts "stopped after". + // CheckRedirect's refusals, and a caller's own policy's, start "stopped after" as net/http's do. msg := urlErr.Err.Error() return !strings.Contains(msg, "unsupported protocol scheme") && !strings.HasPrefix(msg, "stopped after ") } From 4616df2ef357bd226abacc98c510b6e8dda86dee Mon Sep 17 00:00:00 2001 From: Michael McQuade Date: Sun, 25 Oct 2026 22:26:14 -0500 Subject: [PATCH 2/4] fix(generator): a caller's redirect refusal is not retried when it wraps the SDK's or has its own message --- internal/generator/e2e_redirect_test.go | 60 +++++++++++++++++++------ internal/templates/client.go.tmpl | 5 ++- internal/templates/retry.go.tmpl | 2 +- 3 files changed, 51 insertions(+), 16 deletions(-) diff --git a/internal/generator/e2e_redirect_test.go b/internal/generator/e2e_redirect_test.go index 6a5f7f7..f6369fe 100644 --- a/internal/generator/e2e_redirect_test.go +++ b/internal/generator/e2e_redirect_test.go @@ -47,6 +47,7 @@ func TestE2E_Redirects(t *testing.T) { import ( "errors" + "fmt" "net/http" "net/http/httptest" "strings" @@ -204,20 +205,26 @@ func TestARangedGetKeepsItsRangeElsewhere(t *testing.T) { } func TestAClientsOwnRedirectRefusalIsNotRetried(t *testing.T) { - var calls atomic.Int32 - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - calls.Add(1) - http.Redirect(w, r, "/elsewhere", http.StatusFound) - })) - t.Cleanup(srv.Close) - own := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return errors.New("stopped after a redirect this caller refuses") }} - client := NewClient(srv.URL, fastRetry(), WithHTTPClient(own)) + for _, refusal := range []error{ + errors.New("stopped after a redirect this caller refuses"), + errors.New("redirect refused"), + fmt.Errorf("refusing: %w", ErrInsecureRedirect), + } { + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + http.Redirect(w, r, "/elsewhere", http.StatusFound) + })) + t.Cleanup(srv.Close) + own := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return refusal }} + client := NewClient(srv.URL, fastRetry(), WithHTTPClient(own)) - if _, err := client.GetThing(t.Context()); err == nil { - t.Fatal("expected the caller's refusal") - } - if n := calls.Load(); n != 1 { - t.Errorf("calls = %d, want a single attempt", n) + if _, err := client.GetThing(t.Context()); !errors.Is(err, refusal) { + t.Fatalf("got %v, want the caller's refusal %q", err, refusal) + } + if n := calls.Load(); n != 1 { + t.Errorf("calls = %d for %q, want a single attempt", n, refusal) + } } } @@ -310,6 +317,33 @@ func TestTooManyRedirectsAreNotRetried(t *testing.T) { } } +func TestARedirectRefusalAMiddlewareDropsTheResponseOfIsNotRetried(t *testing.T) { + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + http.Redirect(w, r, r.URL.Path, http.StatusFound) + })) + t.Cleanup(srv.Close) + dropping := func(req *http.Request, next RoundTripFunc) (*http.Response, error) { + resp, err := next(req) + if err != nil { + if resp != nil { + resp.Body.Close() + } + return nil, err + } + return resp, nil + } + client := NewClient(srv.URL, fastRetry(), WithMiddleware(dropping)) + + if _, err := client.GetThing(t.Context()); !errors.Is(err, ErrTooManyRedirects) { + t.Fatalf("got %v, want ErrTooManyRedirects", err) + } + if n := calls.Load(); n != 10 { + t.Errorf("calls = %d, want one chain of redirects", n) + } +} + func TestAClientsOwnRedirectPolicyIsKept(t *testing.T) { other, rc := elsewhere(t) own := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }} diff --git a/internal/templates/client.go.tmpl b/internal/templates/client.go.tmpl index 9062b9b..d1aaf64 100644 --- a/internal/templates/client.go.tmpl +++ b/internal/templates/client.go.tmpl @@ -189,8 +189,9 @@ func (c *Client) send(ctx context.Context, method string, path string, body any, if resp != nil { resp.Body.Close() } - // Retry idempotent requests on transient network errors. - if c.retryConfig != nil && attempt < maxAttempts-1 && isIdempotentMethod(method) && isRetryableNetworkError(err) { + // Retry idempotent requests on transient network errors. A response with + // the error is net/http's answer to a CheckRedirect refusal, whatever it says. + if c.retryConfig != nil && resp == nil && attempt < maxAttempts-1 && isIdempotentMethod(method) && isRetryableNetworkError(err) { timer := time.NewTimer(retryDelay(attempt, *c.retryConfig, nil)) select { case <-ctx.Done(): diff --git a/internal/templates/retry.go.tmpl b/internal/templates/retry.go.tmpl index 82d67b7..b909872 100644 --- a/internal/templates/retry.go.tmpl +++ b/internal/templates/retry.go.tmpl @@ -157,7 +157,7 @@ func isRetryableNetworkError(err error) bool { } var urlErr *url.Error if errors.As(err, &urlErr) { - // CheckRedirect's refusals, and a caller's own policy's, start "stopped after" as net/http's do. + // CheckRedirect's refusals start "stopped after" as net/http's do, for a middleware that drops their response. msg := urlErr.Err.Error() return !strings.Contains(msg, "unsupported protocol scheme") && !strings.HasPrefix(msg, "stopped after ") } From 9e47d6b5466f1072db0d3c2707a05c468ecd3c2f Mon Sep 17 00:00:00 2001 From: Michael McQuade Date: Sun, 25 Oct 2026 22:28:03 -0500 Subject: [PATCH 3/4] fix(generator): a redirect refusal is never retried, whatever its message or wrapping SafeRedirects now wraps the client's redirect policy, the generated CheckRedirect or the caller's own, so each refusal comes back as a *RedirectRefusedError that unwraps to the original error. The retry declines on that type instead of on a "stopped after" message prefix, which missed a caller's refusal worded otherwise once a middleware dropped its response. A caller's policy still decides every redirect, and http.ErrUseLastResponse passes through unwrapped. --- README.md | 2 +- internal/generator/e2e_redirect_test.go | 72 +++++++++++++++++++++++++ internal/templates/redirect.go.tmpl | 27 ++++++++-- internal/templates/reserved.go | 1 + internal/templates/retry.go.tmpl | 8 +-- 5 files changed, 102 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 14c89ac..98ff6f5 100644 --- a/README.md +++ b/README.md @@ -73,7 +73,7 @@ gen/petstore/ ├── operations.go # ListPets(), CreatePet(), GetPetByID(), DeletePet() ├── options.go # WithHTTPClient(), WithUserAgent() ├── pagination.go # PageIterator[T], ListPetsIter() -├── redirect.go # CheckRedirect, SafeRedirects(), ErrInsecureRedirect +├── redirect.go # CheckRedirect, SafeRedirects(), ErrInsecureRedirect, RedirectRefusedError ├── retry.go # RetryConfig, WithRetry(), WithDefaultRetry() └── types.go # Pet, PetStatus, PetList, CreatePetRequest, Error ``` diff --git a/internal/generator/e2e_redirect_test.go b/internal/generator/e2e_redirect_test.go index f6369fe..57621a8 100644 --- a/internal/generator/e2e_redirect_test.go +++ b/internal/generator/e2e_redirect_test.go @@ -344,6 +344,78 @@ func TestARedirectRefusalAMiddlewareDropsTheResponseOfIsNotRetried(t *testing.T) } } +func TestACallersRefusalAMiddlewareDropsTheResponseOfIsNotRetried(t *testing.T) { + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + http.Redirect(w, r, "/elsewhere", http.StatusFound) + })) + t.Cleanup(srv.Close) + refusal := errors.New("redirect refused") + own := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return refusal }} + dropping := func(req *http.Request, next RoundTripFunc) (*http.Response, error) { + resp, err := next(req) + if resp != nil && err != nil { + resp.Body.Close() + resp = nil + } + return resp, err + } + client := NewClient(srv.URL, fastRetry(), WithHTTPClient(own), WithMiddleware(dropping)) + + _, err := client.GetThing(t.Context()) + var refused *RedirectRefusedError + if !errors.As(err, &refused) || !errors.Is(err, refusal) { + t.Fatalf("got %v, want a *RedirectRefusedError wrapping the caller's refusal", err) + } + if err.Error() == "" || !strings.HasSuffix(err.Error(), "redirect refused") { + t.Errorf("message = %q, want the caller's own", err.Error()) + } + if n := calls.Load(); n != 1 { + t.Errorf("calls = %d, want a single attempt", n) + } +} + +func TestTheSDKsRefusalsAreRedirectRefusedErrors(t *testing.T) { + plain := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + t.Cleanup(plain.Close) + secure := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, plain.URL+r.URL.Path, http.StatusFound) + })) + t.Cleanup(secure.Close) + looping := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, r.URL.Path, http.StatusFound) + })) + t.Cleanup(looping.Close) + + for _, tc := range []struct { + client *Client + want error + }{ + {NewClient(secure.URL, WithHTTPClient(secure.Client())), ErrInsecureRedirect}, + {NewClient(looping.URL), ErrTooManyRedirects}, + } { + _, err := tc.client.GetThing(t.Context()) + var refused *RedirectRefusedError + if !errors.As(err, &refused) || !errors.Is(err, tc.want) { + t.Errorf("got %v, want a *RedirectRefusedError wrapping %v", err, tc.want) + } + } +} + +func TestACallersPolicyFollowsWhatTheSDKsWouldStop(t *testing.T) { + other, rc := elsewhere(t) + own := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return nil }} + client := NewClient(redirecting(t, other.URL).URL, WithHTTPClient(own)) + + if _, err := client.CreateThing(t.Context()); err != nil { + t.Fatalf("CreateThing: %v", err) + } + if rc.hits.Load() != 1 { + t.Error("the caller's policy did not decide the redirect") + } +} + func TestAClientsOwnRedirectPolicyIsKept(t *testing.T) { other, rc := elsewhere(t) own := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }} diff --git a/internal/templates/redirect.go.tmpl b/internal/templates/redirect.go.tmpl index 45deca2..baa8f16 100644 --- a/internal/templates/redirect.go.tmpl +++ b/internal/templates/redirect.go.tmpl @@ -56,16 +56,35 @@ func bodilessMethod(method string) bool { return method == http.MethodGet || method == http.MethodHead || method == "" } -// SafeRedirects is c following redirects by CheckRedirect, unless it has a redirect policy of its own. +// RedirectRefusedError is a redirect the client's redirect policy refused, which a retry would only refuse again. +type RedirectRefusedError struct { + Err error +} + +func (e *RedirectRefusedError) Error() string { return e.Err.Error() } + +func (e *RedirectRefusedError) Unwrap() error { return e.Err } + +// SafeRedirects is c following redirects by its own redirect policy, or by CheckRedirect when it has none, with +// each refusal returned as a *RedirectRefusedError. func SafeRedirects(c *http.Client) *http.Client { if c == nil { c = http.DefaultClient } - if c.CheckRedirect != nil { - return c + policy := c.CheckRedirect + if policy == nil { + policy = CheckRedirect } safe := *c - safe.CheckRedirect = CheckRedirect + safe.CheckRedirect = func(req *http.Request, via []*http.Request) error { + err := policy(req, via) + var refused *RedirectRefusedError + // net/http compares ErrUseLastResponse with ==, so it must come back unwrapped. + if err == nil || err == http.ErrUseLastResponse || errors.As(err, &refused) { + return err + } + return &RedirectRefusedError{Err: err} + } return &safe } diff --git a/internal/templates/reserved.go b/internal/templates/reserved.go index 6120611..4032a6a 100644 --- a/internal/templates/reserved.go +++ b/internal/templates/reserved.go @@ -36,6 +36,7 @@ var ReservedIdentifiers = []string{ "NewClient", "PageIterator", "ProblemDetails", + "RedirectRefusedError", "ParseCallback", "ParseWebhook", "ResponseMeta", diff --git a/internal/templates/retry.go.tmpl b/internal/templates/retry.go.tmpl index b909872..44b9192 100644 --- a/internal/templates/retry.go.tmpl +++ b/internal/templates/retry.go.tmpl @@ -155,11 +155,13 @@ func isRetryableNetworkError(err error) bool { if errors.As(err, &recordErr) { return false } + var refused *RedirectRefusedError + if errors.As(err, &refused) { + return false + } var urlErr *url.Error if errors.As(err, &urlErr) { - // CheckRedirect's refusals start "stopped after" as net/http's do, for a middleware that drops their response. - msg := urlErr.Err.Error() - return !strings.Contains(msg, "unsupported protocol scheme") && !strings.HasPrefix(msg, "stopped after ") + return !strings.Contains(urlErr.Err.Error(), "unsupported protocol scheme") } return true } From 5286831f6c81d0c761fa6041812cf70ad80d2df0 Mon Sep 17 00:00:00 2001 From: Michael McQuade Date: Sun, 25 Oct 2026 22:32:09 -0500 Subject: [PATCH 4/4] test(generator)!: a refusal already marked is not wrapped twice Also drops a redundant test check and keeps the reserved identifiers in order. BREAKING CHANGE: every redirect refusal, the generated CheckRedirect's or a caller's own, now comes back wrapped in a *RedirectRefusedError, and SafeRedirects always returns a copy of the client. Code that compares a refusal with ==, as in urlErr.Err == ErrInsecureRedirect, checks the result of errors.Unwrap, uses a type switch on its own CheckRedirect error, or expects SafeRedirects(c) == c must use errors.Is or errors.As instead. --- internal/generator/e2e_redirect_test.go | 7 ++++--- internal/templates/reserved.go | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/generator/e2e_redirect_test.go b/internal/generator/e2e_redirect_test.go index 57621a8..096150c 100644 --- a/internal/generator/e2e_redirect_test.go +++ b/internal/generator/e2e_redirect_test.go @@ -368,7 +368,7 @@ func TestACallersRefusalAMiddlewareDropsTheResponseOfIsNotRetried(t *testing.T) if !errors.As(err, &refused) || !errors.Is(err, refusal) { t.Fatalf("got %v, want a *RedirectRefusedError wrapping the caller's refusal", err) } - if err.Error() == "" || !strings.HasSuffix(err.Error(), "redirect refused") { + if !strings.HasSuffix(err.Error(), "redirect refused") { t.Errorf("message = %q, want the caller's own", err.Error()) } if n := calls.Load(); n != 1 { @@ -394,11 +394,12 @@ func TestTheSDKsRefusalsAreRedirectRefusedErrors(t *testing.T) { }{ {NewClient(secure.URL, WithHTTPClient(secure.Client())), ErrInsecureRedirect}, {NewClient(looping.URL), ErrTooManyRedirects}, + {NewClient(secure.URL, WithHTTPClient(SafeRedirects(secure.Client()))), ErrInsecureRedirect}, } { _, err := tc.client.GetThing(t.Context()) var refused *RedirectRefusedError - if !errors.As(err, &refused) || !errors.Is(err, tc.want) { - t.Errorf("got %v, want a *RedirectRefusedError wrapping %v", err, tc.want) + if !errors.As(err, &refused) || refused.Err != tc.want { + t.Errorf("got %v, want a single *RedirectRefusedError wrapping %v", err, tc.want) } } } diff --git a/internal/templates/reserved.go b/internal/templates/reserved.go index 4032a6a..2eefba5 100644 --- a/internal/templates/reserved.go +++ b/internal/templates/reserved.go @@ -36,9 +36,9 @@ var ReservedIdentifiers = []string{ "NewClient", "PageIterator", "ProblemDetails", - "RedirectRefusedError", "ParseCallback", "ParseWebhook", + "RedirectRefusedError", "ResponseMeta", "RetryConfig", "RoundTripFunc",