From 1d47fe2beb62d0a5ad912946d6d7183ecb7a49cc Mon Sep 17 00:00:00 2001 From: akrishna Date: Sat, 12 Mar 2022 17:27:55 -0500 Subject: [PATCH 1/6] checkpoint --- .../httpclient/backoff_middleware.go | 72 ++++++++++++ conjure-go-client/httpclient/client.go | 111 +++++++++++++++--- conjure-go-client/httpclient/client_params.go | 3 +- .../httpclient/internal/request_retrier.go | 12 +- .../httpclient/internal/retry.go | 22 ++-- .../httpclient/internal/retry_test.go | 2 +- .../httpclient/uri_middleware.go | 106 +++++++++++++++++ 7 files changed, 294 insertions(+), 34 deletions(-) create mode 100644 conjure-go-client/httpclient/backoff_middleware.go create mode 100644 conjure-go-client/httpclient/uri_middleware.go diff --git a/conjure-go-client/httpclient/backoff_middleware.go b/conjure-go-client/httpclient/backoff_middleware.go new file mode 100644 index 00000000..b264c5ec --- /dev/null +++ b/conjure-go-client/httpclient/backoff_middleware.go @@ -0,0 +1,72 @@ +package httpclient + +import ( + "github.com/palantir/conjure-go-runtime/v2/conjure-go-client/httpclient/internal" + "github.com/palantir/pkg/retry" + "net/http" + "net/url" +) + +type backoffMiddleware struct { + retrier retry.Retrier + attemptedURIs map[string]struct{} + backoffFunc func() +} + +// NewBackoffMiddleware returns middleware that uses a supplied Retrier to backoff before making requests if the client +// has attempted to reach the URI before or has sent too many requests. +func NewBackoffMiddleware(retrier retry.Retrier) Middleware { + return &backoffMiddleware{ + retrier: retrier, + attemptedURIs: map[string]struct{}{}, + } +} + +func (b *backoffMiddleware) RoundTrip(req *http.Request, next http.RoundTripper) (*http.Response, error) { + b.backoffRequest(req) + resp, err := next.RoundTrip(req) + b.handleResponse(resp) + return resp, err +} + +func (b *backoffMiddleware) backoffRequest(req *http.Request) { + baseURI := getBaseURI(req.URL) + defer func() { + b.attemptedURIs[baseURI] = struct{}{} + }() + // Use backoffFunc if backoff behavior was determined by previous response e.g. throttle on 429 + if b.backoffFunc != nil { + b.backoffFunc() + b.backoffFunc = nil + return + } + // Trigger retrier on first attempt so that future attempts have backoff + if len(b.attemptedURIs) == 0 { + b.retrier.Next() + } + // Trigger retrier for backoff if URI was attempted before + if _, performBackoff := b.attemptedURIs[baseURI]; performBackoff { + b.retrier.Next() + } +} + +func (b *backoffMiddleware) handleResponse(resp *http.Response) { + if resp != nil { + switch resp.StatusCode { + case internal.StatusCodeRetryOther, internal.StatusCodeRetryTemporaryRedirect: + b.retrier.Reset() + case internal.StatusCodeThrottle: + b.backoffFunc = func() { b.retrier.Next() } + } + } +} + +func getBaseURI(u *url.URL) string { + uCopy := url.URL{ + Scheme: u.Scheme, + Opaque: u.Opaque, + User: u.User, + Host: u.Host, + } + return uCopy.String() +} diff --git a/conjure-go-client/httpclient/client.go b/conjure-go-client/httpclient/client.go index 136b9752..b206fd52 100644 --- a/conjure-go-client/httpclient/client.go +++ b/conjure-go-client/httpclient/client.go @@ -16,6 +16,7 @@ package httpclient import ( "context" + "github.com/palantir/pkg/retry" "net/http" "net/url" "strings" @@ -25,7 +26,6 @@ import ( "github.com/palantir/pkg/bytesbuffers" "github.com/palantir/pkg/refreshable" werror "github.com/palantir/witchcraft-go-error" - "github.com/palantir/witchcraft-go-logging/wlog/svclog/svc1log" ) // A Client executes requests to a configured service. @@ -87,17 +87,50 @@ func (c *clientImpl) Do(ctx context.Context, params ...RequestParam) (*http.Resp return nil, werror.ErrorWithContextParams(ctx, "no base URIs are configured") } - attempts := 2 * len(uris) + maxAttempts := 2 * len(uris) if c.maxAttempts != nil { if confMaxAttempts := c.maxAttempts.CurrentIntPtr(); confMaxAttempts != nil { - attempts = *confMaxAttempts + maxAttempts = *confMaxAttempts } } - var err error + b, err := applyRequestParams(c.bufferPool, params...) + if err != nil { + return nil, err + } + + req, err := getRequest(ctx, b) + if err != nil { + return nil, err + } + cancelled := false + cancelFunc := func() { cancelled = true } + retrier := c.backoffOptions.CurrentRetryParams().Start(ctx) + clientCopy := c.getClientCopyWithMiddleware(b, uris, retrier, cancelFunc) + attempts := 0 + var resp *http.Response + for !cancelled && (maxAttempts == 0 || attempts < maxAttempts) { + reqCopy := req.Clone(ctx) + resp, err = clientCopy.Do(reqCopy) + // unless this is exactly the scenario where the caller has opted into being responsible for draining and closing + // the response body, be sure to do so here. + if !(err == nil && b.bodyMiddleware.rawOutput) { + internal.DrainBody(resp) + } + attempts++ + if resp != nil && resp.StatusCode < 300 { + break + } + } + if err != nil { + return nil, err + } + return resp, err + + /*var err error var resp *http.Response - retrier := internal.NewRequestRetrier(uris, c.backoffOptions.CurrentRetryParams().Start(ctx), attempts) + retrier := internal.NewRequestRetrier(uris, c.backoffOptions.CurrentRetryParams().Start(ctx), maxAttempts) for { uri, isRelocated := retrier.GetNextURI(resp, err) if uri == "" { @@ -111,10 +144,42 @@ func (c *clientImpl) Do(ctx context.Context, params ...RequestParam) (*http.Resp if err != nil { return nil, err } - return resp, nil + return resp, nil*/ +} + +func applyRequestParams(bufferPool bytesbuffers.Pool, params ...RequestParam) (*requestBuilder, error) { + b := &requestBuilder{ + headers: make(http.Header), + query: make(url.Values), + bodyMiddleware: &bodyMiddleware{bufferPool: bufferPool}, + } + for _, p := range params { + if p == nil { + continue + } + if err := p.apply(b); err != nil { + return nil, err + } + } + return b, nil +} + +func getRequest(ctx context.Context, b *requestBuilder) (*http.Request, error) { + if b.method == "" { + return nil, werror.ErrorWithContextParams(ctx, "httpclient: use WithRequestMethod() to specify HTTP method") + } + req, err := http.NewRequestWithContext(ctx, b.method, b.path, nil) + if err != nil { + return nil, werror.WrapWithContextParams(ctx, err, "failed to build new HTTP request") + } + req.Header = b.headers + if q := b.query.Encode(); q != "" { + req.URL.RawQuery = q + } + return req, nil } -func (c *clientImpl) doOnce( +/*func (c *clientImpl) doOnce( ctx context.Context, baseURI string, useBaseURIOnly bool, @@ -159,11 +224,30 @@ func (c *clientImpl) doOnce( } // 2. create the transport and client + clientCopy := c.getClientCopyWithMiddleware(b) + + // 3. execute the request using the client to get and handle the response + resp, respErr := clientCopy.Do(req) + + // unless this is exactly the scenario where the caller has opted into being responsible for draining and closing + // the response body, be sure to do so here. + if !(respErr == nil && b.bodyMiddleware.rawOutput) { + internal.DrainBody(resp) + } + + return resp, unwrapURLError(ctx, respErr) +} +*/ + +func (c *clientImpl) getClientCopyWithMiddleware(b *requestBuilder, uris []string, backoffRetrier retry.Retrier, cancelFunc func()) http.Client { // shallow copy so we can overwrite the Transport with a wrapped one. clientCopy := *c.client.CurrentHTTPClient() transport := clientCopy.Transport // start with the client's transport configured with default middleware + // must precede URI middleware to track attempted URIs + transport = wrapTransport(transport, NewBackoffMiddleware(backoffRetrier)) // must precede the error decoders to read the status code of the raw response. + transport = wrapTransport(transport, NewURIMiddleware(uris, cancelFunc)) transport = wrapTransport(transport, c.uriScorer.CurrentURIScoringMiddleware()) // request decoder must precede the client decoder // must precede the body middleware to read the response body @@ -172,23 +256,14 @@ func (c *clientImpl) doOnce( transport = wrapTransport(transport, c.middlewares...) // must wrap inner middlewares to mutate the return values transport = wrapTransport(transport, b.bodyMiddleware) + // must wrap inner middlewares to update request with resolved URL // must be the outermost middleware to recover panics in the rest of the request flow // there is a second, inner recoveryMiddleware in the client's default middlewares so that panics // inside the inner-most RoundTrip benefit from traceIDs and loggers set on the context. transport = wrapTransport(transport, c.recoveryMiddleware) clientCopy.Transport = transport - - // 3. execute the request using the client to get and handle the response - resp, respErr := clientCopy.Do(req) - - // unless this is exactly the scenario where the caller has opted into being responsible for draining and closing - // the response body, be sure to do so here. - if !(respErr == nil && b.bodyMiddleware.rawOutput) { - internal.DrainBody(resp) - } - - return resp, unwrapURLError(ctx, respErr) + return clientCopy } // unwrapURLError converts a *url.Error to a werror. We need this because all diff --git a/conjure-go-client/httpclient/client_params.go b/conjure-go-client/httpclient/client_params.go index e0cef56f..2e1140ad 100644 --- a/conjure-go-client/httpclient/client_params.go +++ b/conjure-go-client/httpclient/client_params.go @@ -474,8 +474,7 @@ func WithInitialBackoff(initialBackoff time.Duration) ClientParam { }) } -// WithMaxRetries sets the maximum number of retries on transport errors for every request. Backoffs are -// also capped at this. +// WithMaxRetries sets the maximum number of retries on transport errors for every request. // If unset, the client defaults to 2 * size of URIs // TODO (#151): Rename to WithMaxAttempts and set maxAttempts directly using the argument provided to the function. func WithMaxRetries(maxTransportRetries int) ClientParam { diff --git a/conjure-go-client/httpclient/internal/request_retrier.go b/conjure-go-client/httpclient/internal/request_retrier.go index e7c738f7..bd04de20 100644 --- a/conjure-go-client/httpclient/internal/request_retrier.go +++ b/conjure-go-client/httpclient/internal/request_retrier.go @@ -76,13 +76,13 @@ func (r *RequestRetrier) GetNextURI(resp *http.Response, respErr error) (uri str // but ignore the returned value to ensure that the client can instrument the request even // if the context is done. r.retrier.Next() - return r.removeMeshSchemeIfPresent(r.currentURI), false + return removeMeshSchemeIfPresent(r.currentURI), false } if !r.attemptsRemaining() { // Retries exhausted return "", false } - if r.isMeshURI(r.currentURI) { + if isMeshURI(r.currentURI) { // Mesh uris don't get retried return "", false } @@ -108,7 +108,7 @@ func (r *RequestRetrier) getRetryFn(resp *http.Response, respErr error) func() b } else if isUnavailableResponse(resp, errCode) { // 503: go to next node return r.nextURIOrBackoff - } else if shouldTryOther, otherURI := isRetryOtherResponse(resp, respErr, errCode); shouldTryOther { + } else if shouldTryOther, otherURI := isRetryOtherResponse1(resp, respErr, errCode); shouldTryOther { // 307 or 308: go to next node, or particular node if provided. if otherURI != nil { return func() bool { @@ -166,14 +166,14 @@ func (r *RequestRetrier) markFailedAndMoveToNextURI() { r.offset = nextURIOffset } -func (r *RequestRetrier) removeMeshSchemeIfPresent(uri string) string { - if r.isMeshURI(uri) { +func removeMeshSchemeIfPresent(uri string) string { + if isMeshURI(uri) { return strings.Replace(uri, meshSchemePrefix, "", 1) } return uri } -func (r *RequestRetrier) isMeshURI(uri string) bool { +func isMeshURI(uri string) bool { return strings.HasPrefix(uri, meshSchemePrefix) } diff --git a/conjure-go-client/httpclient/internal/retry.go b/conjure-go-client/httpclient/internal/retry.go index f4c6b484..56561de4 100644 --- a/conjure-go-client/httpclient/internal/retry.go +++ b/conjure-go-client/httpclient/internal/retry.go @@ -59,25 +59,33 @@ const ( StatusCodeUnavailable = http.StatusServiceUnavailable ) -func isRetryOtherResponse(resp *http.Response, err error, errCode int) (bool, *url.URL) { - if errCode == StatusCodeRetryOther || errCode == StatusCodeRetryTemporaryRedirect { +func isRetryOtherResponse1(resp *http.Response, err error, errCode int) (bool, *url.URL) { + if isRetryOtherStatusCode(errCode) { locationStr, ok := LocationFromError(err) if ok { return true, parseLocationURL(locationStr) } } - if resp == nil { - return false, nil - } - if resp.StatusCode != StatusCodeRetryOther && - resp.StatusCode != StatusCodeRetryTemporaryRedirect { + if resp == nil || !isRetryOtherStatusCode(resp.StatusCode) { return false, nil } locationStr := resp.Header.Get("Location") return true, parseLocationURL(locationStr) } +func isRetryOtherResponse(resp *http.Response, err error) bool { + errCode, _ := StatusCodeFromError(err) + if isRetryOtherStatusCode(errCode) { + return true + } + return resp != nil && isRetryOtherStatusCode(resp.StatusCode) +} + +func isRetryOtherStatusCode(statusCode int) bool { + return statusCode == StatusCodeRetryOther || statusCode == StatusCodeRetryTemporaryRedirect +} + func parseLocationURL(locationStr string) *url.URL { if locationStr == "" { return nil diff --git a/conjure-go-client/httpclient/internal/retry_test.go b/conjure-go-client/httpclient/internal/retry_test.go index 78c90856..c00b82dc 100644 --- a/conjure-go-client/httpclient/internal/retry_test.go +++ b/conjure-go-client/httpclient/internal/retry_test.go @@ -130,7 +130,7 @@ func TestRetryResponseParsers(t *testing.T) { } { t.Run(test.Name, func(t *testing.T) { errCode, _ := StatusCodeFromError(test.RespErr) - isRetryOther, retryOtherURL := isRetryOtherResponse(test.Response, test.RespErr, errCode) + isRetryOther, retryOtherURL := isRetryOtherResponse1(test.Response, test.RespErr, errCode) if assert.Equal(t, test.IsRetryOther, isRetryOther) && test.RetryOtherURL != "" { if assert.NotNil(t, retryOtherURL) { assert.Equal(t, test.RetryOtherURL, retryOtherURL.String()) diff --git a/conjure-go-client/httpclient/uri_middleware.go b/conjure-go-client/httpclient/uri_middleware.go new file mode 100644 index 00000000..0896af7b --- /dev/null +++ b/conjure-go-client/httpclient/uri_middleware.go @@ -0,0 +1,106 @@ +package httpclient + +import ( + "net/http" + urlpkg "net/url" + "strings" +) + +const ( + meshSchemePrefix = "mesh-" +) + +type uriMiddleware struct { + uris []string + offset int + redirectURL *urlpkg.URL + cancelFunc func() +} + +func NewURIMiddleware(uris []string, cancelFunc func()) Middleware { + offset := 0 + return &uriMiddleware{ + uris: uris, + offset: offset, + redirectURL: nil, + cancelFunc: cancelFunc, + } +} + +func (u *uriMiddleware) RoundTrip(req *http.Request, next http.RoundTripper) (*http.Response, error) { + err := u.setRequestURLAndHost(req) + if err != nil { + return nil, err + } + resp, err := next.RoundTrip(req) + u.offset = (u.offset + 1) % len(u.uris) + if _, url := isRedirectResponse(resp); url != nil { + if !url.IsAbs() { + url = req.URL.ResolveReference(url) + } + u.redirectURL = url + } else { + u.offset = (u.offset + 1) % len(u.uris) + } + return resp, err +} + +func (u *uriMiddleware) setRequestURLAndHost(req *http.Request) error { + var url *urlpkg.URL + if u.redirectURL != nil { + url = u.redirectURL + u.redirectURL = nil + } else { + uri := u.uris[u.offset] + if isMeshURI(uri) { + // Mesh URIs should not be retried + u.cancelFunc() + uri = strings.Replace(uri, meshSchemePrefix, "", 1) + } + parsedUrl, err := urlpkg.Parse(uri) + if err != nil { + return err + } + removeEmptyPort(parsedUrl) + url = parsedUrl.ResolveReference(req.URL) + } + req.URL = url + req.Host = url.Host + return nil +} + +// removeEmptyPort() strips the empty port in ":port" to "" +// as mandated by RFC 3986 Section 6.2.3. +func removeEmptyPort(url *urlpkg.URL) { + if url.Port() == "" { + url.Host = strings.TrimSuffix(url.Host, ":") + } +} + +func isMeshURI(uri string) bool { + return strings.HasPrefix(uri, meshSchemePrefix) +} + +func isRedirectResponse(resp *http.Response) (bool, *urlpkg.URL) { + if resp == nil || !isRetryOtherStatusCode(resp.StatusCode) { + return false, nil + } + locationStr := resp.Header.Get("Location") + return true, parseLocationURL(locationStr) +} + +func isRetryOtherStatusCode(statusCode int) bool { + return statusCode == 307 || statusCode == 308 +} + +func parseLocationURL(locationStr string) *urlpkg.URL { + if locationStr == "" { + return nil + } + locationURL, err := urlpkg.Parse(locationStr) + if err != nil { + // Unable to parse location as something we recognize + return nil + } + return locationURL +} From 08988a08810818fc11d2d26c946b3c68696af167 Mon Sep 17 00:00:00 2001 From: akrishna Date: Tue, 15 Mar 2022 08:47:33 -0700 Subject: [PATCH 2/6] compiling --- .../httpclient/backoff_middleware.go | 22 +++--- conjure-go-client/httpclient/body_handler.go | 13 ++-- conjure-go-client/httpclient/client.go | 33 ++++---- .../httpclient/request_params.go | 2 +- .../response_error_decoder_middleware_test.go | 14 ++-- .../httpclient/uri_middleware.go | 77 +++++++++++-------- 6 files changed, 89 insertions(+), 72 deletions(-) diff --git a/conjure-go-client/httpclient/backoff_middleware.go b/conjure-go-client/httpclient/backoff_middleware.go index b264c5ec..989c2d96 100644 --- a/conjure-go-client/httpclient/backoff_middleware.go +++ b/conjure-go-client/httpclient/backoff_middleware.go @@ -1,10 +1,11 @@ package httpclient import ( - "github.com/palantir/conjure-go-runtime/v2/conjure-go-client/httpclient/internal" - "github.com/palantir/pkg/retry" "net/http" "net/url" + + "github.com/palantir/conjure-go-runtime/v2/conjure-go-client/httpclient/internal" + "github.com/palantir/pkg/retry" ) type backoffMiddleware struct { @@ -25,7 +26,7 @@ func NewBackoffMiddleware(retrier retry.Retrier) Middleware { func (b *backoffMiddleware) RoundTrip(req *http.Request, next http.RoundTripper) (*http.Response, error) { b.backoffRequest(req) resp, err := next.RoundTrip(req) - b.handleResponse(resp) + b.handleResponse(err) return resp, err } @@ -50,14 +51,13 @@ func (b *backoffMiddleware) backoffRequest(req *http.Request) { } } -func (b *backoffMiddleware) handleResponse(resp *http.Response) { - if resp != nil { - switch resp.StatusCode { - case internal.StatusCodeRetryOther, internal.StatusCodeRetryTemporaryRedirect: - b.retrier.Reset() - case internal.StatusCodeThrottle: - b.backoffFunc = func() { b.retrier.Next() } - } +func (b *backoffMiddleware) handleResponse(err error) { + errCode, _ := StatusCodeFromError(err) + switch errCode { + case internal.StatusCodeRetryOther, internal.StatusCodeRetryTemporaryRedirect: + b.retrier.Reset() + case internal.StatusCodeThrottle: + b.backoffFunc = func() { b.retrier.Next() } } } diff --git a/conjure-go-client/httpclient/body_handler.go b/conjure-go-client/httpclient/body_handler.go index d80bf013..57def3ef 100644 --- a/conjure-go-client/httpclient/body_handler.go +++ b/conjure-go-client/httpclient/body_handler.go @@ -62,13 +62,12 @@ func (b *bodyMiddleware) setRequestBody(req *http.Request) (func(), error) { return cleanup, nil } - // Special case: if the requestInput is an io.ReadCloser and the requestEncoder is nil, - // use the provided input directly as the request body. - if bodyReadCloser, ok := b.requestInput.(io.ReadCloser); ok && b.requestEncoder == nil { - req.Body = bodyReadCloser - // Use the same heuristic as http.NewRequest to generate the "GetBody" function. - if newReq, err := http.NewRequest("", "", bodyReadCloser); err == nil { - req.GetBody = newReq.GetBody + // Special case: if the requestInput is a getBody function and the requestEncoder is nil, + // use the provided function to directly as the request body. + if getBody, ok := b.requestInput.(func() io.ReadCloser); ok && b.requestEncoder == nil { + req.Body = getBody() + req.GetBody = func() (io.ReadCloser, error) { + return getBody(), nil } return cleanup, nil } diff --git a/conjure-go-client/httpclient/client.go b/conjure-go-client/httpclient/client.go index b206fd52..90aaf41f 100644 --- a/conjure-go-client/httpclient/client.go +++ b/conjure-go-client/httpclient/client.go @@ -16,7 +16,6 @@ package httpclient import ( "context" - "github.com/palantir/pkg/retry" "net/http" "net/url" "strings" @@ -25,6 +24,7 @@ import ( "github.com/palantir/conjure-go-runtime/v2/conjure-go-client/httpclient/internal/refreshingclient" "github.com/palantir/pkg/bytesbuffers" "github.com/palantir/pkg/refreshable" + "github.com/palantir/pkg/retry" werror "github.com/palantir/witchcraft-go-error" ) @@ -93,12 +93,13 @@ func (c *clientImpl) Do(ctx context.Context, params ...RequestParam) (*http.Resp maxAttempts = *confMaxAttempts } } - b, err := applyRequestParams(c.bufferPool, params...) if err != nil { return nil, err } - + for _, c := range b.configureCtx { + ctx = c(ctx) + } req, err := getRequest(ctx, b) if err != nil { return nil, err @@ -106,19 +107,20 @@ func (c *clientImpl) Do(ctx context.Context, params ...RequestParam) (*http.Resp cancelled := false cancelFunc := func() { cancelled = true } retrier := c.backoffOptions.CurrentRetryParams().Start(ctx) - clientCopy := c.getClientCopyWithMiddleware(b, uris, retrier, cancelFunc) + clientCopy := c.getClientCopyWithMiddleware(b.errorDecoderMiddleware, b.bodyMiddleware, uris, retrier, cancelFunc) attempts := 0 var resp *http.Response for !cancelled && (maxAttempts == 0 || attempts < maxAttempts) { reqCopy := req.Clone(ctx) resp, err = clientCopy.Do(reqCopy) + err = unwrapURLError(ctx, err) // unless this is exactly the scenario where the caller has opted into being responsible for draining and closing // the response body, be sure to do so here. if !(err == nil && b.bodyMiddleware.rawOutput) { internal.DrainBody(resp) } attempts++ - if resp != nil && resp.StatusCode < 300 { + if resp != nil && isSuccessfulOrBadRequest(resp.StatusCode) { break } } @@ -147,6 +149,10 @@ func (c *clientImpl) Do(ctx context.Context, params ...RequestParam) (*http.Resp return resp, nil*/ } +func isSuccessfulOrBadRequest(statusCode int) bool { + return statusCode < 300 || (statusCode >= http.StatusBadRequest && statusCode < http.StatusInternalServerError) +} + func applyRequestParams(bufferPool bytesbuffers.Pool, params ...RequestParam) (*requestBuilder, error) { b := &requestBuilder{ headers: make(http.Header), @@ -239,24 +245,24 @@ func getRequest(ctx context.Context, b *requestBuilder) (*http.Request, error) { } */ -func (c *clientImpl) getClientCopyWithMiddleware(b *requestBuilder, uris []string, backoffRetrier retry.Retrier, cancelFunc func()) http.Client { +func (c *clientImpl) getClientCopyWithMiddleware(errorDecoderMiddleware Middleware, bodyMiddleware *bodyMiddleware, uris []string, backoffRetrier retry.Retrier, cancelFunc func()) http.Client { // shallow copy so we can overwrite the Transport with a wrapped one. clientCopy := *c.client.CurrentHTTPClient() transport := clientCopy.Transport // start with the client's transport configured with default middleware - // must precede URI middleware to track attempted URIs - transport = wrapTransport(transport, NewBackoffMiddleware(backoffRetrier)) // must precede the error decoders to read the status code of the raw response. - transport = wrapTransport(transport, NewURIMiddleware(uris, cancelFunc)) transport = wrapTransport(transport, c.uriScorer.CurrentURIScoringMiddleware()) // request decoder must precede the client decoder // must precede the body middleware to read the response body - transport = wrapTransport(transport, b.errorDecoderMiddleware, c.errorDecoderMiddleware) + transport = wrapTransport(transport, errorDecoderMiddleware, c.errorDecoderMiddleware) // must precede the body middleware to read the request body transport = wrapTransport(transport, c.middlewares...) // must wrap inner middlewares to mutate the return values - transport = wrapTransport(transport, b.bodyMiddleware) + transport = wrapTransport(transport, bodyMiddleware) + // must precede URI middleware to track attempted URIs + transport = wrapTransport(transport, NewBackoffMiddleware(backoffRetrier)) // must wrap inner middlewares to update request with resolved URL + transport = wrapTransport(transport, NewURIMiddleware(uris, cancelFunc)) // must be the outermost middleware to recover panics in the rest of the request flow // there is a second, inner recoveryMiddleware in the client's default middlewares so that panics // inside the inner-most RoundTrip benefit from traceIDs and loggers set on the context. @@ -280,7 +286,8 @@ func unwrapURLError(ctx context.Context, respErr error) error { // We don't recognize this as a url.Error, just return the original. return respErr } - params := []werror.Param{werror.SafeParam("requestMethod", urlErr.Op)} + return urlErr.Err + /*params := []werror.Param{werror.SafeParam("requestMethod", urlErr.Op)} if parsedURL, _ := url.Parse(urlErr.URL); parsedURL != nil { params = append(params, @@ -288,7 +295,7 @@ func unwrapURLError(ctx context.Context, respErr error) error { werror.UnsafeParam("requestPath", parsedURL.Path)) } - return werror.WrapWithContextParams(ctx, urlErr.Err, "httpclient request failed", params...) + return werror.WrapWithContextParams(ctx, urlErr.Err, "httpclient request failed", params...)*/ } func joinURIAndPath(baseURI, reqPath string) string { diff --git a/conjure-go-client/httpclient/request_params.go b/conjure-go-client/httpclient/request_params.go index c2fffeb6..11c78aec 100644 --- a/conjure-go-client/httpclient/request_params.go +++ b/conjure-go-client/httpclient/request_params.go @@ -125,7 +125,7 @@ func WithRawRequestBodyProvider(getBody func() io.ReadCloser) RequestParam { if getBody == nil { return werror.Error("getBody can not be nil") } - b.bodyMiddleware.requestInput = getBody() + b.bodyMiddleware.requestInput = getBody b.bodyMiddleware.requestEncoder = nil b.headers.Set("Content-Type", "application/octet-stream") return nil diff --git a/conjure-go-client/httpclient/response_error_decoder_middleware_test.go b/conjure-go-client/httpclient/response_error_decoder_middleware_test.go index 846b3afb..53e1564b 100644 --- a/conjure-go-client/httpclient/response_error_decoder_middleware_test.go +++ b/conjure-go-client/httpclient/response_error_decoder_middleware_test.go @@ -94,7 +94,7 @@ func TestErrorDecoderMiddlewares(t *testing.T) { verify404(t, err) assert.EqualError(t, err, "httpclient request failed: 404 Not Found") safeParams, unsafeParams := werror.ParamsFromError(err) - assert.Equal(t, map[string]interface{}{"requestHost": u.Host, "requestMethod": "Get", "statusCode": 404}, safeParams) + assert.Equal(t, map[string]interface{}{"requestHost": u.Host, "requestMethod": "GET", "statusCode": 404}, safeParams) assert.Equal(t, map[string]interface{}{"requestPath": "/path", "responseBody": "404 page not found\n"}, unsafeParams) }, }, @@ -107,7 +107,7 @@ func TestErrorDecoderMiddlewares(t *testing.T) { verify404(t, err) assert.EqualError(t, err, "httpclient request failed: 404 Not Found") safeParams, unsafeParams := werror.ParamsFromError(err) - assert.Equal(t, map[string]interface{}{"requestHost": u.Host, "requestMethod": "Get", "statusCode": 404}, safeParams) + assert.Equal(t, map[string]interface{}{"requestHost": u.Host, "requestMethod": "GET", "statusCode": 404}, safeParams) assert.Equal(t, map[string]interface{}{"requestPath": "/path"}, unsafeParams) }, }, @@ -122,7 +122,7 @@ func TestErrorDecoderMiddlewares(t *testing.T) { verify404(t, err) assert.EqualError(t, err, "httpclient request failed: 404 Not Found") safeParams, unsafeParams := werror.ParamsFromError(err) - assert.Equal(t, map[string]interface{}{"requestHost": u.Host, "requestMethod": "Get", "statusCode": 404}, safeParams) + assert.Equal(t, map[string]interface{}{"requestHost": u.Host, "requestMethod": "GET", "statusCode": 404}, safeParams) assert.Equal(t, map[string]interface{}{"requestPath": "/path", "responseBody": "route does not exist"}, unsafeParams) }, }, @@ -137,7 +137,7 @@ func TestErrorDecoderMiddlewares(t *testing.T) { verify404(t, err) assert.EqualError(t, err, "httpclient request failed: failed to unmarshal body using registered type: errors: error name does not match regexp `^(([A-Z][a-z0-9]+)+):(([A-Z][a-z0-9]+)+)$`") safeParams, unsafeParams := werror.ParamsFromError(err) - assert.Equal(t, map[string]interface{}{"requestHost": u.Host, "requestMethod": "Get", "statusCode": 404, "type": "errors.genericError"}, safeParams) + assert.Equal(t, map[string]interface{}{"requestHost": u.Host, "requestMethod": "GET", "statusCode": 404, "type": "errors.genericError"}, safeParams) assert.Equal(t, map[string]interface{}{"requestPath": "/path", "responseBody": `{"foo":"bar"}`}, unsafeParams) }, }, @@ -159,7 +159,7 @@ func TestErrorDecoderMiddlewares(t *testing.T) { assert.Equal(t, errors.DefaultNotFound.Name(), conjureErr.Name()) safeParams, unsafeParams := werror.ParamsFromError(err) - assert.Equal(t, map[string]interface{}{"requestHost": u.Host, "requestMethod": "Get", "errorInstanceId": id, "errorName": "Default:NotFound", "statusCode": 404}, safeParams) + assert.Equal(t, map[string]interface{}{"requestHost": u.Host, "requestMethod": "GET", "errorInstanceId": id, "errorName": "Default:NotFound", "statusCode": 404}, safeParams) assert.Equal(t, map[string]interface{}{"requestPath": "/path", "stringParam": "stringValue"}, unsafeParams) }, }, @@ -170,7 +170,7 @@ func TestErrorDecoderMiddlewares(t *testing.T) { verify: func(t *testing.T, u *url.URL, err error) { assert.EqualError(t, err, "httpclient request failed: foo error") safeParams, unsafeParams := werror.ParamsFromError(err) - assert.Equal(t, map[string]interface{}{"requestHost": u.Host, "requestMethod": "Get"}, safeParams) + assert.Equal(t, map[string]interface{}{"requestHost": u.Host, "requestMethod": "GET"}, safeParams) assert.Equal(t, map[string]interface{}{"requestPath": "/path"}, unsafeParams) }, }, @@ -181,7 +181,7 @@ func TestErrorDecoderMiddlewares(t *testing.T) { verify: func(t *testing.T, u *url.URL, err error) { assert.EqualError(t, err, "httpclient request failed: error from body: 404 page not found\n") safeParams, unsafeParams := werror.ParamsFromError(err) - assert.Equal(t, map[string]interface{}{"requestHost": u.Host, "requestMethod": "Get"}, safeParams) + assert.Equal(t, map[string]interface{}{"requestHost": u.Host, "requestMethod": "GET"}, safeParams) assert.Equal(t, map[string]interface{}{"requestPath": "/path"}, unsafeParams) }, }, diff --git a/conjure-go-client/httpclient/uri_middleware.go b/conjure-go-client/httpclient/uri_middleware.go index 0896af7b..61b031ca 100644 --- a/conjure-go-client/httpclient/uri_middleware.go +++ b/conjure-go-client/httpclient/uri_middleware.go @@ -1,6 +1,7 @@ package httpclient import ( + werror "github.com/palantir/witchcraft-go-error" "net/http" urlpkg "net/url" "strings" @@ -28,45 +29,50 @@ func NewURIMiddleware(uris []string, cancelFunc func()) Middleware { } func (u *uriMiddleware) RoundTrip(req *http.Request, next http.RoundTripper) (*http.Response, error) { - err := u.setRequestURLAndHost(req) + url, err := u.getURL(req) if err != nil { return nil, err } + req.URL = url + req.Host = url.Host resp, err := next.RoundTrip(req) - u.offset = (u.offset + 1) % len(u.uris) - if _, url := isRedirectResponse(resp); url != nil { - if !url.IsAbs() { - url = req.URL.ResolveReference(url) + if _, redirectURL := isRedirectError(err); redirectURL != nil { + if !redirectURL.IsAbs() { + redirectURL = req.URL.ResolveReference(redirectURL) + } + u.redirectURL = redirectURL + } + if err != nil { + params := []werror.Param{ + werror.SafeParam("requestMethod", req.Method), + werror.SafeParam("requestHost", url.Host), + werror.UnsafeParam("requestPath", url.Path), } - u.redirectURL = url - } else { - u.offset = (u.offset + 1) % len(u.uris) + return nil, werror.Wrap(err, "httpclient request failed", params...) } return resp, err } -func (u *uriMiddleware) setRequestURLAndHost(req *http.Request) error { - var url *urlpkg.URL +func (u *uriMiddleware) getURL(req *http.Request) (*urlpkg.URL, error) { if u.redirectURL != nil { - url = u.redirectURL - u.redirectURL = nil - } else { - uri := u.uris[u.offset] - if isMeshURI(uri) { - // Mesh URIs should not be retried - u.cancelFunc() - uri = strings.Replace(uri, meshSchemePrefix, "", 1) - } - parsedUrl, err := urlpkg.Parse(uri) - if err != nil { - return err - } - removeEmptyPort(parsedUrl) - url = parsedUrl.ResolveReference(req.URL) + defer func() { + u.redirectURL = nil + }() + return u.redirectURL, nil } - req.URL = url - req.Host = url.Host - return nil + uri := u.uris[u.offset] + u.offset = (u.offset + 1) % len(u.uris) + if isMeshURI(uri) { + // Mesh URIs should not be retried + u.cancelFunc() + uri = strings.Replace(uri, meshSchemePrefix, "", 1) + } + parsedUrl, err := urlpkg.Parse(uri) + if err != nil { + return nil, err + } + removeEmptyPort(parsedUrl) + return parsedUrl.ResolveReference(req.URL), nil } // removeEmptyPort() strips the empty port in ":port" to "" @@ -81,16 +87,21 @@ func isMeshURI(uri string) bool { return strings.HasPrefix(uri, meshSchemePrefix) } -func isRedirectResponse(resp *http.Response) (bool, *urlpkg.URL) { - if resp == nil || !isRetryOtherStatusCode(resp.StatusCode) { +func isRedirectError(err error) (bool, *urlpkg.URL) { + errCode, _ := StatusCodeFromError(err) + if !isRedirectStatusCode(errCode) { return false, nil } - locationStr := resp.Header.Get("Location") + _, unsafeParams := werror.ParamsFromError(err) + locationStr, ok := unsafeParams["location"].(string) + if !ok { + return true, nil + } return true, parseLocationURL(locationStr) } -func isRetryOtherStatusCode(statusCode int) bool { - return statusCode == 307 || statusCode == 308 +func isRedirectStatusCode(statusCode int) bool { + return statusCode == http.StatusTemporaryRedirect || statusCode == http.StatusPermanentRedirect } func parseLocationURL(locationStr string) *urlpkg.URL { From b21de2049ae5f4e4fd7600d8ff563aec825bd3a8 Mon Sep 17 00:00:00 2001 From: akrishna Date: Sat, 19 Mar 2022 12:34:18 -0700 Subject: [PATCH 3/6] tmp --- conjure-go-client/httpclient/body_handler.go | 6 +++++- conjure-go-client/httpclient/client.go | 5 ----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/conjure-go-client/httpclient/body_handler.go b/conjure-go-client/httpclient/body_handler.go index 57def3ef..7835baa9 100644 --- a/conjure-go-client/httpclient/body_handler.go +++ b/conjure-go-client/httpclient/body_handler.go @@ -16,6 +16,7 @@ package httpclient import ( "bytes" + "github.com/palantir/conjure-go-runtime/v2/conjure-go-client/httpclient/internal" "io" "io/ioutil" "net/http" @@ -104,6 +105,9 @@ func (b *bodyMiddleware) readResponse(resp *http.Response, respErr error) error if b.rawOutput && respErr == nil { return nil } + defer func() { + internal.DrainBody(resp) + }() if respErr != nil { return respErr @@ -112,6 +116,7 @@ func (b *bodyMiddleware) readResponse(resp *http.Response, respErr error) error // Verify we have a body to unmarshal. If the request was unsuccessful, the errorMiddleware will // set a non-nil error and return no response. if b.responseOutput == nil || resp == nil || resp.Body == nil || resp.ContentLength == 0 { + internal.DrainBody(resp) return nil } @@ -119,6 +124,5 @@ func (b *bodyMiddleware) readResponse(resp *http.Response, respErr error) error if decErr != nil { return decErr } - return nil } diff --git a/conjure-go-client/httpclient/client.go b/conjure-go-client/httpclient/client.go index 90aaf41f..7aa4ab32 100644 --- a/conjure-go-client/httpclient/client.go +++ b/conjure-go-client/httpclient/client.go @@ -114,11 +114,6 @@ func (c *clientImpl) Do(ctx context.Context, params ...RequestParam) (*http.Resp reqCopy := req.Clone(ctx) resp, err = clientCopy.Do(reqCopy) err = unwrapURLError(ctx, err) - // unless this is exactly the scenario where the caller has opted into being responsible for draining and closing - // the response body, be sure to do so here. - if !(err == nil && b.bodyMiddleware.rawOutput) { - internal.DrainBody(resp) - } attempts++ if resp != nil && isSuccessfulOrBadRequest(resp.StatusCode) { break From e668f9d5d0f513858cd7d90dfc977e3804cf103a Mon Sep 17 00:00:00 2001 From: akrishna Date: Sat, 19 Mar 2022 15:47:18 -0700 Subject: [PATCH 4/6] Revert "tmp" This reverts commit b21de2049ae5f4e4fd7600d8ff563aec825bd3a8. --- conjure-go-client/httpclient/body_handler.go | 6 +----- conjure-go-client/httpclient/client.go | 5 +++++ 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/conjure-go-client/httpclient/body_handler.go b/conjure-go-client/httpclient/body_handler.go index 7835baa9..57def3ef 100644 --- a/conjure-go-client/httpclient/body_handler.go +++ b/conjure-go-client/httpclient/body_handler.go @@ -16,7 +16,6 @@ package httpclient import ( "bytes" - "github.com/palantir/conjure-go-runtime/v2/conjure-go-client/httpclient/internal" "io" "io/ioutil" "net/http" @@ -105,9 +104,6 @@ func (b *bodyMiddleware) readResponse(resp *http.Response, respErr error) error if b.rawOutput && respErr == nil { return nil } - defer func() { - internal.DrainBody(resp) - }() if respErr != nil { return respErr @@ -116,7 +112,6 @@ func (b *bodyMiddleware) readResponse(resp *http.Response, respErr error) error // Verify we have a body to unmarshal. If the request was unsuccessful, the errorMiddleware will // set a non-nil error and return no response. if b.responseOutput == nil || resp == nil || resp.Body == nil || resp.ContentLength == 0 { - internal.DrainBody(resp) return nil } @@ -124,5 +119,6 @@ func (b *bodyMiddleware) readResponse(resp *http.Response, respErr error) error if decErr != nil { return decErr } + return nil } diff --git a/conjure-go-client/httpclient/client.go b/conjure-go-client/httpclient/client.go index 7aa4ab32..90aaf41f 100644 --- a/conjure-go-client/httpclient/client.go +++ b/conjure-go-client/httpclient/client.go @@ -114,6 +114,11 @@ func (c *clientImpl) Do(ctx context.Context, params ...RequestParam) (*http.Resp reqCopy := req.Clone(ctx) resp, err = clientCopy.Do(reqCopy) err = unwrapURLError(ctx, err) + // unless this is exactly the scenario where the caller has opted into being responsible for draining and closing + // the response body, be sure to do so here. + if !(err == nil && b.bodyMiddleware.rawOutput) { + internal.DrainBody(resp) + } attempts++ if resp != nil && isSuccessfulOrBadRequest(resp.StatusCode) { break From c433ab8bdbfb9c9cbaaa743daf35ec25bfff6100 Mon Sep 17 00:00:00 2001 From: akrishna Date: Sat, 19 Mar 2022 16:05:07 -0700 Subject: [PATCH 5/6] cleanup --- conjure-go-client/httpclient/body_handler.go | 8 -- conjure-go-client/httpclient/client.go | 90 +------------------ .../httpclient/request_builder.go | 1 + .../httpclient/request_params.go | 2 +- 4 files changed, 3 insertions(+), 98 deletions(-) diff --git a/conjure-go-client/httpclient/body_handler.go b/conjure-go-client/httpclient/body_handler.go index 57def3ef..9c8ad0c0 100644 --- a/conjure-go-client/httpclient/body_handler.go +++ b/conjure-go-client/httpclient/body_handler.go @@ -29,9 +29,6 @@ type bodyMiddleware struct { requestInput interface{} requestEncoder codecs.Encoder - // if rawOutput is true, the body of the response is not drained before returning -- it is the responsibility of the - // caller to read from and properly close the response body. - rawOutput bool responseOutput interface{} responseDecoder codecs.Decoder @@ -100,11 +97,6 @@ func (b *bodyMiddleware) setRequestBody(req *http.Request) (func(), error) { } func (b *bodyMiddleware) readResponse(resp *http.Response, respErr error) error { - // If rawOutput is true, return response directly without draining or closing body - if b.rawOutput && respErr == nil { - return nil - } - if respErr != nil { return respErr } diff --git a/conjure-go-client/httpclient/client.go b/conjure-go-client/httpclient/client.go index 90aaf41f..d9a16817 100644 --- a/conjure-go-client/httpclient/client.go +++ b/conjure-go-client/httpclient/client.go @@ -116,7 +116,7 @@ func (c *clientImpl) Do(ctx context.Context, params ...RequestParam) (*http.Resp err = unwrapURLError(ctx, err) // unless this is exactly the scenario where the caller has opted into being responsible for draining and closing // the response body, be sure to do so here. - if !(err == nil && b.bodyMiddleware.rawOutput) { + if !b.rawOutput { internal.DrainBody(resp) } attempts++ @@ -128,25 +128,6 @@ func (c *clientImpl) Do(ctx context.Context, params ...RequestParam) (*http.Resp return nil, err } return resp, err - - /*var err error - var resp *http.Response - - retrier := internal.NewRequestRetrier(uris, c.backoffOptions.CurrentRetryParams().Start(ctx), maxAttempts) - for { - uri, isRelocated := retrier.GetNextURI(resp, err) - if uri == "" { - break - } - if err != nil { - svc1log.FromContext(ctx).Debug("Retrying request", svc1log.Stacktrace(err)) - } - resp, err = c.doOnce(ctx, uri, isRelocated, params...) - } - if err != nil { - return nil, err - } - return resp, nil*/ } func isSuccessfulOrBadRequest(statusCode int) bool { @@ -185,66 +166,6 @@ func getRequest(ctx context.Context, b *requestBuilder) (*http.Request, error) { return req, nil } -/*func (c *clientImpl) doOnce( - ctx context.Context, - baseURI string, - useBaseURIOnly bool, - params ...RequestParam, -) (*http.Response, error) { - - // 1. create the request - b := &requestBuilder{ - headers: make(http.Header), - query: make(url.Values), - bodyMiddleware: &bodyMiddleware{bufferPool: c.bufferPool}, - } - - for _, p := range params { - if p == nil { - continue - } - if err := p.apply(b); err != nil { - return nil, err - } - } - if useBaseURIOnly { - b.path = "" - } - - for _, c := range b.configureCtx { - ctx = c(ctx) - } - - if b.method == "" { - 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) - 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 - } - - // 2. create the transport and client - clientCopy := c.getClientCopyWithMiddleware(b) - - // 3. execute the request using the client to get and handle the response - resp, respErr := clientCopy.Do(req) - - // unless this is exactly the scenario where the caller has opted into being responsible for draining and closing - // the response body, be sure to do so here. - if !(respErr == nil && b.bodyMiddleware.rawOutput) { - internal.DrainBody(resp) - } - - return resp, unwrapURLError(ctx, respErr) -} -*/ - func (c *clientImpl) getClientCopyWithMiddleware(errorDecoderMiddleware Middleware, bodyMiddleware *bodyMiddleware, uris []string, backoffRetrier retry.Retrier, cancelFunc func()) http.Client { // shallow copy so we can overwrite the Transport with a wrapped one. clientCopy := *c.client.CurrentHTTPClient() @@ -287,15 +208,6 @@ func unwrapURLError(ctx context.Context, respErr error) error { return respErr } return urlErr.Err - /*params := []werror.Param{werror.SafeParam("requestMethod", urlErr.Op)} - - if parsedURL, _ := url.Parse(urlErr.URL); parsedURL != nil { - params = append(params, - werror.SafeParam("requestHost", parsedURL.Host), - werror.UnsafeParam("requestPath", parsedURL.Path)) - } - - return werror.WrapWithContextParams(ctx, urlErr.Err, "httpclient request failed", params...)*/ } func joinURIAndPath(baseURI, reqPath string) string { diff --git a/conjure-go-client/httpclient/request_builder.go b/conjure-go-client/httpclient/request_builder.go index 88998f04..e0b0189f 100644 --- a/conjure-go-client/httpclient/request_builder.go +++ b/conjure-go-client/httpclient/request_builder.go @@ -30,6 +30,7 @@ type requestBuilder struct { query url.Values bodyMiddleware *bodyMiddleware bufferPool bytesbuffers.Pool + rawOutput bool errorDecoderMiddleware Middleware configureCtx []func(context.Context) context.Context diff --git a/conjure-go-client/httpclient/request_params.go b/conjure-go-client/httpclient/request_params.go index 11c78aec..795365d6 100644 --- a/conjure-go-client/httpclient/request_params.go +++ b/conjure-go-client/httpclient/request_params.go @@ -169,7 +169,7 @@ func WithResponseBody(output interface{}, decoder codecs.Decoder) RequestParam { // In the case of an empty response, output will be unmodified (left nil). func WithRawResponseBody() RequestParam { return requestParamFunc(func(b *requestBuilder) error { - b.bodyMiddleware.rawOutput = true + b.rawOutput = true b.bodyMiddleware.responseOutput = nil b.bodyMiddleware.responseDecoder = nil b.headers.Set("Accept", "application/octet-stream") From 3971a0f4bf02c195aa5d4858c9511d9a42e44690 Mon Sep 17 00:00:00 2001 From: akrishna Date: Sat, 19 Mar 2022 16:35:35 -0700 Subject: [PATCH 6/6] verify --- .../httpclient/backoff_middleware.go | 14 +++ conjure-go-client/httpclient/client.go | 9 -- .../httpclient/client_path_test.go | 85 ------------------- .../httpclient/internal/request_retrier.go | 2 +- .../httpclient/internal/retry.go | 10 +-- .../httpclient/internal/retry_test.go | 2 +- .../httpclient/uri_middleware.go | 23 ++++- 7 files changed, 36 insertions(+), 109 deletions(-) delete mode 100644 conjure-go-client/httpclient/client_path_test.go diff --git a/conjure-go-client/httpclient/backoff_middleware.go b/conjure-go-client/httpclient/backoff_middleware.go index 989c2d96..44a72d79 100644 --- a/conjure-go-client/httpclient/backoff_middleware.go +++ b/conjure-go-client/httpclient/backoff_middleware.go @@ -1,3 +1,17 @@ +// Copyright (c) 2022 Palantir Technologies. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package httpclient import ( diff --git a/conjure-go-client/httpclient/client.go b/conjure-go-client/httpclient/client.go index d9a16817..c5045927 100644 --- a/conjure-go-client/httpclient/client.go +++ b/conjure-go-client/httpclient/client.go @@ -18,7 +18,6 @@ import ( "context" "net/http" "net/url" - "strings" "github.com/palantir/conjure-go-runtime/v2/conjure-go-client/httpclient/internal" "github.com/palantir/conjure-go-runtime/v2/conjure-go-client/httpclient/internal/refreshingclient" @@ -209,11 +208,3 @@ func unwrapURLError(ctx context.Context, respErr error) error { } return urlErr.Err } - -func joinURIAndPath(baseURI, reqPath string) string { - fullURI := strings.TrimRight(baseURI, "/") - if reqPath != "" { - fullURI += "/" + strings.TrimLeft(reqPath, "/") - } - return fullURI -} diff --git a/conjure-go-client/httpclient/client_path_test.go b/conjure-go-client/httpclient/client_path_test.go deleted file mode 100644 index 744a8819..00000000 --- a/conjure-go-client/httpclient/client_path_test.go +++ /dev/null @@ -1,85 +0,0 @@ -// Copyright (c) 2019 Palantir Technologies. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package httpclient - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestJoinURIandPath(t *testing.T) { - for _, test := range []struct { - baseURI string - reqPath string - expected string - }{ - { - "https://localhost", - "/api", - "https://localhost/api", - }, - { - "https://localhost:443", - "/api", - "https://localhost:443/api", - }, - { - "https://localhost:443", - "api", - "https://localhost:443/api", - }, - { - "https://localhost:443/", - "api", - "https://localhost:443/api", - }, - { - "https://localhost:443/foo/", - "/api", - "https://localhost:443/foo/api", - }, - { - "https://localhost:443/foo//////", - "////api/", - "https://localhost:443/foo/api/", - }, - { - "https://localhost:443/foo/", - "/api", - "https://localhost:443/foo/api", - }, - { - "https://localhost", - "", - "https://localhost", - }, - { - "https://localhost/api", - "", - "https://localhost/api", - }, - { - "https://localhost", - "/api/+ti%2FQojjmKJxpxmY%2FA=", - "https://localhost/api/+ti%2FQojjmKJxpxmY%2FA=", - }, - } { - t.Run("", func(t *testing.T) { - actual := joinURIAndPath(test.baseURI, test.reqPath) - assert.Equal(t, test.expected, actual) - }) - } -} diff --git a/conjure-go-client/httpclient/internal/request_retrier.go b/conjure-go-client/httpclient/internal/request_retrier.go index bd04de20..37c5942d 100644 --- a/conjure-go-client/httpclient/internal/request_retrier.go +++ b/conjure-go-client/httpclient/internal/request_retrier.go @@ -108,7 +108,7 @@ func (r *RequestRetrier) getRetryFn(resp *http.Response, respErr error) func() b } else if isUnavailableResponse(resp, errCode) { // 503: go to next node return r.nextURIOrBackoff - } else if shouldTryOther, otherURI := isRetryOtherResponse1(resp, respErr, errCode); shouldTryOther { + } else if shouldTryOther, otherURI := isRetryOtherResponse(resp, respErr, errCode); shouldTryOther { // 307 or 308: go to next node, or particular node if provided. if otherURI != nil { return func() bool { diff --git a/conjure-go-client/httpclient/internal/retry.go b/conjure-go-client/httpclient/internal/retry.go index 56561de4..4253ad6a 100644 --- a/conjure-go-client/httpclient/internal/retry.go +++ b/conjure-go-client/httpclient/internal/retry.go @@ -59,7 +59,7 @@ const ( StatusCodeUnavailable = http.StatusServiceUnavailable ) -func isRetryOtherResponse1(resp *http.Response, err error, errCode int) (bool, *url.URL) { +func isRetryOtherResponse(resp *http.Response, err error, errCode int) (bool, *url.URL) { if isRetryOtherStatusCode(errCode) { locationStr, ok := LocationFromError(err) if ok { @@ -74,14 +74,6 @@ func isRetryOtherResponse1(resp *http.Response, err error, errCode int) (bool, * return true, parseLocationURL(locationStr) } -func isRetryOtherResponse(resp *http.Response, err error) bool { - errCode, _ := StatusCodeFromError(err) - if isRetryOtherStatusCode(errCode) { - return true - } - return resp != nil && isRetryOtherStatusCode(resp.StatusCode) -} - func isRetryOtherStatusCode(statusCode int) bool { return statusCode == StatusCodeRetryOther || statusCode == StatusCodeRetryTemporaryRedirect } diff --git a/conjure-go-client/httpclient/internal/retry_test.go b/conjure-go-client/httpclient/internal/retry_test.go index c00b82dc..78c90856 100644 --- a/conjure-go-client/httpclient/internal/retry_test.go +++ b/conjure-go-client/httpclient/internal/retry_test.go @@ -130,7 +130,7 @@ func TestRetryResponseParsers(t *testing.T) { } { t.Run(test.Name, func(t *testing.T) { errCode, _ := StatusCodeFromError(test.RespErr) - isRetryOther, retryOtherURL := isRetryOtherResponse1(test.Response, test.RespErr, errCode) + isRetryOther, retryOtherURL := isRetryOtherResponse(test.Response, test.RespErr, errCode) if assert.Equal(t, test.IsRetryOther, isRetryOther) && test.RetryOtherURL != "" { if assert.NotNil(t, retryOtherURL) { assert.Equal(t, test.RetryOtherURL, retryOtherURL.String()) diff --git a/conjure-go-client/httpclient/uri_middleware.go b/conjure-go-client/httpclient/uri_middleware.go index 61b031ca..501ef5f3 100644 --- a/conjure-go-client/httpclient/uri_middleware.go +++ b/conjure-go-client/httpclient/uri_middleware.go @@ -1,10 +1,25 @@ +// Copyright (c) 2022 Palantir Technologies. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package httpclient import ( - werror "github.com/palantir/witchcraft-go-error" "net/http" urlpkg "net/url" "strings" + + werror "github.com/palantir/witchcraft-go-error" ) const ( @@ -67,12 +82,12 @@ func (u *uriMiddleware) getURL(req *http.Request) (*urlpkg.URL, error) { u.cancelFunc() uri = strings.Replace(uri, meshSchemePrefix, "", 1) } - parsedUrl, err := urlpkg.Parse(uri) + parsedURL, err := urlpkg.Parse(uri) if err != nil { return nil, err } - removeEmptyPort(parsedUrl) - return parsedUrl.ResolveReference(req.URL), nil + removeEmptyPort(parsedURL) + return parsedURL.ResolveReference(req.URL), nil } // removeEmptyPort() strips the empty port in ":port" to ""