improvement: Parse full qualified redirect URI in error handler middleware. (#348)

Parse full qualified redirect URI in error handler middleware.

Author: dtrejod

changelog/@unreleased/pr-348.v2.yml

@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: Parse full qualified redirect URI in error handler middleware.
+  links:
+  - https://github.com/palantir/conjure-go-runtime/pull/348

conjure-go-client/httpclient/client.go

@@ -148,11 +148,11 @@ func (c *clientImpl) doOnce(
 		return nil, werror.ErrorWithContextParams(ctx, "httpclient: use WithRequestMethod() to specify HTTP method")
 	}
 	reqURI := joinURIAndPath(baseURI, b.path)
-	req, err := http.NewRequest(b.method, reqURI, nil)
+	req, err := http.NewRequestWithContext(ctx, b.method, reqURI, nil)
 	if err != nil {
 		return nil, werror.WrapWithContextParams(ctx, err, "failed to build new HTTP request")
 	}
-	req = req.WithContext(ctx)
+
 	req.Header = b.headers
 	if q := b.query.Encode(); q != "" {
 		req.URL.RawQuery = q

conjure-go-client/httpclient/internal/request_retrier.go

@@ -127,13 +127,6 @@ func (r *RequestRetrier) getRetryFn(resp *http.Response, respErr error) func() b
 }
 
 func (r *RequestRetrier) setURIAndResetBackoff(otherURI *url.URL) {
-	// If the URI returned by relocation header is a relative path
-	// We will resolve it with the current URI
-	if !otherURI.IsAbs() {
-		if currentURI := parseLocationURL(r.currentURI); currentURI != nil {
-			otherURI = currentURI.ResolveReference(otherURI)
-		}
-	}
 	nextURI := otherURI.String()
 	r.relocatedURIs[otherURI.String()] = struct{}{}
 	r.retrier.Reset()

conjure-go-client/httpclient/internal/retry.go

@@ -62,9 +62,10 @@ const (
 func isRetryOtherResponse(resp *http.Response, err error, errCode int) (bool, *url.URL) {
 	if errCode == StatusCodeRetryOther || errCode == StatusCodeRetryTemporaryRedirect {
 		locationStr, ok := LocationFromError(err)
-		if ok {
-			return true, parseLocationURL(locationStr)
+		if !ok {
+			return true, nil
 		}
+		return true, parseLocationURL(locationStr)
 	}
 
 	if resp == nil {
@@ -74,8 +75,11 @@ func isRetryOtherResponse(resp *http.Response, err error, errCode int) (bool, *u
 		resp.StatusCode != StatusCodeRetryTemporaryRedirect {
 		return false, nil
 	}
-	locationStr := resp.Header.Get("Location")
-	return true, parseLocationURL(locationStr)
+	location, err := resp.Location()
+	if err != nil {
+		return true, nil
+	}
+	return true, location
 }
 
 func parseLocationURL(locationStr string) *url.URL {
@@ -90,6 +94,8 @@ func parseLocationURL(locationStr string) *url.URL {
 	return locationURL
 }
 
+// isThrottleResponse returns true if the response a throttle response type. It
+// also returns a duration after which the failed URI can be retried
 func isThrottleResponse(resp *http.Response, errCode int) (bool, time.Duration) {
 	if errCode == StatusCodeThrottle {
 		return true, 0

conjure-go-client/httpclient/response_error_decoder_middleware.go

@@ -81,7 +81,10 @@ func (d restErrorDecoder) DecodeError(resp *http.Response) error {
 	unsafeParams := map[string]interface{}{}
 	if resp.StatusCode >= http.StatusTemporaryRedirect &&
 		resp.StatusCode < http.StatusBadRequest {
-		unsafeParams["location"] = resp.Header.Get("Location")
+		location, err := resp.Location()
+		if err == nil {
+			unsafeParams["location"] = location.String()
+		}
 	}
 	wSafeParams := werror.SafeParams(safeParams)
 	wUnsafeParams := werror.UnsafeParams(unsafeParams)

conjure-go-client/httpclient/response_error_decoder_middleware_test.go

@@ -65,7 +65,7 @@ func TestErrorDecoderMiddlewares(t *testing.T) {
 				assert.True(t, ok)
 				assert.Equal(t, 307, code)
 				location, ok := httpclient.LocationFromError(err)
-				assert.True(t, ok)
+				assert.False(t, ok)
 				assert.Equal(t, "", location)
 			},
 		},
@@ -79,6 +79,25 @@ func TestErrorDecoderMiddlewares(t *testing.T) {
 				assert.NoError(t, err)
 			},
 		},
+		{
+			name: "307 with relative location",
+			handler: func(w http.ResponseWriter, r *http.Request) {
+				w.Header().Set("Location", "/newPath")
+				w.WriteHeader(307)
+			},
+			verify: func(t *testing.T, u *url.URL, err error) {
+				assert.EqualError(t, err, "httpclient request failed: 307 Temporary Redirect")
+				code, ok := httpclient.StatusCodeFromError(err)
+				assert.True(t, ok)
+				assert.Equal(t, 307, code)
+				location, ok := httpclient.LocationFromError(err)
+				assert.True(t, ok)
+
+				// The redirect url is a path relative to original URI
+				expected := u.ResolveReference(&url.URL{Path: "/newPath"})
+				assert.Equal(t, expected.String(), location)
+			},
+		},
 		{
 			name:         "404 DisableRestErrors",
 			handler:      http.NotFound,