fix(generator)!: a redirect refusal is never retried, whatever its message or wrapping

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
 ```

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,106 @@ 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 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 !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},
+		{NewClient(secure.URL, WithHTTPClient(SafeRedirects(secure.Client()))), ErrInsecureRedirect},
+	} {
+		_, err := tc.client.GetThing(t.Context())
+		var refused *RedirectRefusedError
+		if !errors.As(err, &refused) || refused.Err != tc.want {
+			t.Errorf("got %v, want a single *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 }}

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():

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
 }
 

internal/templates/reserved.go

@@ -38,6 +38,7 @@ var ReservedIdentifiers = []string{
 	"ProblemDetails",
 	"ParseCallback",
 	"ParseWebhook",
+	"RedirectRefusedError",
 	"ResponseMeta",
 	"RetryConfig",
 	"RoundTripFunc",

internal/templates/retry.go.tmpl

@@ -155,14 +155,13 @@ func isRetryableNetworkError(err error) bool {
 	if errors.As(err, &recordErr) {
 		return false
 	}
-	if errors.Is(err, ErrInsecureRedirect) || errors.Is(err, ErrTooManyRedirects) {
+	var refused *RedirectRefusedError
+	if errors.As(err, &refused) {
 		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".
-		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
 }