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 6a5f7f7..096150c 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,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 }} 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/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..2eefba5 100644 --- a/internal/templates/reserved.go +++ b/internal/templates/reserved.go @@ -38,6 +38,7 @@ var ReservedIdentifiers = []string{ "ProblemDetails", "ParseCallback", "ParseWebhook", + "RedirectRefusedError", "ResponseMeta", "RetryConfig", "RoundTripFunc", diff --git a/internal/templates/retry.go.tmpl b/internal/templates/retry.go.tmpl index 632f4ff..44b9192 100644 --- a/internal/templates/retry.go.tmpl +++ b/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 }