Merge pull request #410 from cisco-open/fix/httpclient_error_messages

fixed http client package's error message formatting

Author: TimShi

pkg/integrate/httpclient/client.go

@@ -76,7 +76,7 @@ func (c *client) WithService(service string, opts ...SDOptions) (Client, error)
 
 	instancer, e := c.sdClient.Instancer(service)
 	if e != nil {
-		return nil, NewNoEndpointFoundError("cannot create client with service name: %v", e)
+		return nil, NewNoEndpointFoundError(fmt.Errorf("cannot create client with service name: %s", service), e)
 	}
 
 	defaultOpts := func(opts *SDOption) {
@@ -86,7 +86,7 @@ func (c *client) WithService(service string, opts ...SDOptions) (Client, error)
 	opts = append([]SDOptions{defaultOpts}, opts...)
 	targetResolver, e := NewSDTargetResolver(instancer, opts...)
 	if e != nil {
-		return nil, NewNoEndpointFoundError("cannot create client with service name: %v", e)
+		return nil, NewNoEndpointFoundError(fmt.Errorf("cannot create client with service name: %s", service), e)
 	}
 
 	cp := c.shallowCopy()
@@ -97,7 +97,7 @@ func (c *client) WithService(service string, opts ...SDOptions) (Client, error)
 func (c *client) WithBaseUrl(baseUrl string) (Client, error) {
 	endpointer, e := NewStaticTargetResolver(baseUrl)
 	if e != nil {
-		return nil, NewNoEndpointFoundError("cannot create client with base URL: %v", e)
+		return nil, NewNoEndpointFoundError(fmt.Errorf("cannot create client with base URL: %s", baseUrl), e)
 	}
 
 	cp := c.shallowCopy()

pkg/integrate/httpclient/client_test.go

@@ -103,7 +103,10 @@ func TestWithMockedServer(t *testing.T) {
 		test.GomegaSubTest(SubTestWithNoInfoSD(&di), "TestWithNoInfoSD"),
 		test.GomegaSubTest(SubTestWithSDNoResponseContent(&di), "TestWithSDNoResponseContent"),
 		test.GomegaSubTest(SubTestWithBaseURL(&di), "TestWithBaseURL"),
+		test.GomegaSubTest(SubTestWithBaseURLFailure(&di), "TestWithBaseUrlFailure"),
+		test.GomegaSubTest(SubTestWithSDFailure(&di), "TestWithSDFailure"),
 		test.GomegaSubTest(SubTestWithErrorResponse(&di), "TestWithErrorResponse"),
+		test.GomegaSubTest(SubTestWithUnexpectedStatusCodeInErrorResponse(&di), "TestWithUnexpectedStatusCodeInErrorResponse"),
 		test.GomegaSubTest(SubTestWithNoContentErrorResponse(&di), "SubTestWithNoContentErrorResponse"),
 		test.GomegaSubTest(SubTestWithFailedSD(&di), "TestWithFailedSD"),
 		test.GomegaSubTest(SubTestWithRetry(&di), "TestWithRetry"),
@@ -164,7 +167,7 @@ func SubTestWithNoInfoSD(di *TestDI) test.GomegaSubTestFunc {
 			if len(target.Port()) != 0 {
 				return nil, fmt.Errorf("target URL [%s] should not have port", target.String())
 			}
-			target.Host = fmt.Sprintf(`%s:%d`,target.Host, webtest.CurrentPort(ctx))
+			target.Host = fmt.Sprintf(`%s:%d`, target.Host, webtest.CurrentPort(ctx))
 			return http.NewRequestWithContext(ctx, method, target.String(), nil)
 		}
 		performEchoTest(ctx, t, g, client, httpclient.WithRequestCreator(urlRewriteCreator))
@@ -179,6 +182,16 @@ func SubTestWithSDNoResponseContent(di *TestDI) test.GomegaSubTestFunc {
 	}
 }
 
+func SubTestWithSDFailure(di *TestDI) test.GomegaSubTestFunc {
+	return func(ctx context.Context, t *testing.T, g *gomega.WithT) {
+		_, e := di.HttpClient.WithService("")
+		//checking the error type, ignoring the error message
+		g.Expect(e).To(MatchError(httpclient.NewNoEndpointFoundError("error message doesn't matter")))
+		//check that the message is formatted
+		g.Expect(e).To(MatchError(Not(ContainSubstring("%"))))
+	}
+}
+
 func SubTestWithBaseURL(di *TestDI) test.GomegaSubTestFunc {
 	return func(ctx context.Context, t *testing.T, g *gomega.WithT) {
 		baseUrl := fmt.Sprintf(`http://localhost:%d%s`, webtest.CurrentPort(ctx), webtest.CurrentContextPath(ctx))
@@ -188,6 +201,18 @@ func SubTestWithBaseURL(di *TestDI) test.GomegaSubTestFunc {
 	}
 }
 
+func SubTestWithBaseURLFailure(di *TestDI) test.GomegaSubTestFunc {
+	return func(ctx context.Context, t *testing.T, g *gomega.WithT) {
+		baseUrl := webtest.CurrentContextPath(ctx)
+		_, e := di.HttpClient.WithBaseUrl(baseUrl)
+		//checking the error type, ignoring the error message
+		g.Expect(e).To(MatchError(httpclient.NewNoEndpointFoundError("error message doesn't matter")))
+		//check that the message is formatted
+		g.Expect(e).To(MatchError(Not(ContainSubstring("%"))))
+		g.Expect(e).To(MatchError(ContainSubstring(baseUrl)))
+	}
+}
+
 func SubTestWithErrorResponse(di *TestDI) test.GomegaSubTestFunc {
 	return func(ctx context.Context, t *testing.T, g *gomega.WithT) {
 		client, e := di.HttpClient.WithService(SDServiceNameFullInfo)
@@ -205,6 +230,26 @@ func SubTestWithErrorResponse(di *TestDI) test.GomegaSubTestFunc {
 	}
 }
 
+func SubTestWithUnexpectedStatusCodeInErrorResponse(di *TestDI) test.GomegaSubTestFunc {
+	return func(ctx context.Context, t *testing.T, g *gomega.WithT) {
+		client, e := di.HttpClient.WithService(SDServiceNameFullInfo)
+		g.Expect(e).To(Succeed(), "client with service name should be available")
+
+		sc := 300 + utils.RandomIntN(10)
+		reqBody := makeEchoRequestBody()
+		req := httpclient.NewRequest(TestErrorPath, http.MethodPut,
+			httpclient.WithParam("sc", fmt.Sprintf("%d", sc)),
+			httpclient.WithBody(reqBody),
+		)
+
+		_, err := client.Execute(ctx, req, httpclient.JsonBody(&EchoResponse{}))
+		// check the error type is the expected type
+		g.Expect(err).To(MatchError(httpclient.ErrorSubTypeInternalError))
+		// check the error message is formatted
+		g.Expect(err).To(MatchError(Not(ContainSubstring("%"))))
+	}
+}
+
 func SubTestWithNoContentErrorResponse(di *TestDI) test.GomegaSubTestFunc {
 	return func(ctx context.Context, t *testing.T, g *gomega.WithT) {
 		client, e := di.HttpClient.WithService(SDServiceNameFullInfo)

pkg/integrate/httpclient/error.go

@@ -176,9 +176,12 @@ func (e Error) WithMessage(msg string, args ...interface{}) *Error {
 	return newError(NewCodedError(e.CodedError.Code(), fmt.Errorf(msg, args...)), e.Response)
 }
 
-/**********************
-	Constructors
- **********************/
+/*
+*********************
+
+		Constructors
+	 *********************
+*/
 func newError(codedErr *CodedError, errResp *ErrorResponse) *Error {
 	err := &Error{
 		CodedError: *codedErr,
@@ -218,7 +221,7 @@ func NewErrorWithStatusCode(e interface{}, resp *http.Response, rawBody []byte,
 	case resp.StatusCode >= 500 && resp.StatusCode <= 599:
 		code = ErrorCodeGenericServerSide
 	default:
-		return NewError(ErrorCodeInternal, "attempt to create response error with non error status code %d", resp.StatusCode)
+		return NewError(ErrorCodeInternal, fmt.Errorf("attempt to create response error with non error status code %d", resp.StatusCode))
 	}
 	return NewErrorWithResponse(code, e, resp, rawBody, causes...)
 }

test/sdtest/client.go

@@ -18,18 +18,19 @@ package sdtest
 
 import (
 	"context"
+	"fmt"
 	"github.com/cisco-open/go-lanai/pkg/discovery"
 	"time"
 )
 
 type ClientMock struct {
-	ctx context.Context
+	ctx        context.Context
 	Instancers map[string]*InstancerMock
 }
 
 func NewMockClient(ctx context.Context) *ClientMock {
 	return &ClientMock{
-		ctx: ctx,
+		ctx:        ctx,
 		Instancers: map[string]*InstancerMock{},
 	}
 }
@@ -41,6 +42,10 @@ func (c *ClientMock) Context() context.Context {
 }
 
 func (c *ClientMock) Instancer(serviceName string) (discovery.Instancer, error) {
+	if serviceName == "" {
+		return nil, fmt.Errorf("empty service name")
+	}
+
 	if instancer, ok := c.Instancers[serviceName]; ok {
 		return instancer, nil
 	}
@@ -68,4 +73,4 @@ func (c *ClientMock) UpdateMockedService(svcName string, matcher InstanceMockMat
 func (c *ClientMock) MockError(svcName string, what error, when time.Time) {
 	instancer, _ := c.Instancer(svcName)
 	instancer.(*InstancerMock).MockError(what, when)
-}
+}