Let clients configure a redirect policy

pkg/api/client_options.go

@@ -25,6 +25,16 @@ type ClientOptions struct {
 	// Default is 24 hours.
 	CacheTTL time.Duration
 
+	// CheckRedirect specifies the policy for handling redirects, matching the
+	// field of the same name on http.Client. If nil, the default policy of
+	// following up to 10 redirects is used.
+	//
+	// This matters for requests where following a redirect silently changes
+	// the meaning of the request. Go's default policy converts a DELETE into a
+	// GET when it follows a 301, so a caller deleting a renamed resource can
+	// receive a success response having deleted nothing.
+	CheckRedirect func(*http.Request, []*http.Request) error
+
 	// EnableCache specifies if API requests will be cached or not.
 	// Default is no caching.
 	EnableCache bool

pkg/api/http_client.go

@@ -118,7 +118,7 @@ func NewHTTPClient(opts ClientOptions) (*http.Client, error) {
 	}
 	transport = newHeaderRoundTripper(opts.Host, opts.AuthToken, opts.Headers, transport)
 
-	return &http.Client{Transport: transport, Timeout: opts.Timeout}, nil
+	return &http.Client{Transport: transport, Timeout: opts.Timeout, CheckRedirect: opts.CheckRedirect}, nil
 }
 
 func inspectableMIMEType(t string) bool {

pkg/api/http_client_test.go

@@ -158,6 +158,74 @@ func TestNewHTTPClient(t *testing.T) {
 	}
 }
 
+func TestNewHTTPClientCheckRedirect(t *testing.T) {
+	// Redirect handling belongs to http.Client rather than the transport, so a
+	// stub transport still exercises the real policy: the client asks it for the
+	// redirected request only if the policy allows the redirect.
+	newRecordingTransport := func(methods *[]string) tripper {
+		return tripper{
+			roundTrip: func(req *http.Request) (*http.Response, error) {
+				*methods = append(*methods, req.Method)
+				if len(*methods) == 1 {
+					return &http.Response{
+						StatusCode: http.StatusMovedPermanently,
+						Header:     http.Header{"Location": []string{"https://api.github.com/repos/OWNER/NEW"}},
+						Body:       io.NopCloser(bytes.NewBufferString("")),
+					}, nil
+				}
+				return &http.Response{
+					StatusCode: http.StatusNoContent,
+					Body:       io.NopCloser(bytes.NewBufferString("")),
+				}, nil
+			},
+		}
+	}
+
+	t.Run("follows redirects by default, downgrading DELETE to GET", func(t *testing.T) {
+		var methods []string
+		client, err := NewHTTPClient(ClientOptions{
+			Host:      "github.com",
+			AuthToken: "oauth_token",
+			Transport: newRecordingTransport(&methods),
+		})
+		assert.NoError(t, err)
+
+		req, err := http.NewRequest(http.MethodDelete, "https://api.github.com/repos/OWNER/OLD", nil)
+		assert.NoError(t, err)
+		res, err := client.Do(req)
+		assert.NoError(t, err)
+		defer res.Body.Close()
+
+		// This is the behaviour that makes the option necessary. Go turns the
+		// DELETE into a GET when it follows the redirect, so the caller is told
+		// the request succeeded while nothing was deleted.
+		assert.Equal(t, []string{http.MethodDelete, http.MethodGet}, methods)
+		assert.Equal(t, http.StatusNoContent, res.StatusCode)
+	})
+
+	t.Run("honours a CheckRedirect that stops at the redirect", func(t *testing.T) {
+		var methods []string
+		client, err := NewHTTPClient(ClientOptions{
+			Host:      "github.com",
+			AuthToken: "oauth_token",
+			Transport: newRecordingTransport(&methods),
+			CheckRedirect: func(req *http.Request, via []*http.Request) error {
+				return http.ErrUseLastResponse
+			},
+		})
+		assert.NoError(t, err)
+
+		req, err := http.NewRequest(http.MethodDelete, "https://api.github.com/repos/OWNER/OLD", nil)
+		assert.NoError(t, err)
+		res, err := client.Do(req)
+		assert.NoError(t, err)
+		defer res.Body.Close()
+
+		assert.Equal(t, []string{http.MethodDelete}, methods)
+		assert.Equal(t, http.StatusMovedPermanently, res.StatusCode)
+	})
+}
+
 type tripper struct {
 	roundTrip func(*http.Request) (*http.Response, error)
 }