Repository: go-kit/kit
Pull request #969 (2020-03-07 13:24:25)

jsonrpc.DefaultErrorEncoder: add RequestID in error body
This PR adds requestID field in `jsonrpc.DefaultErrorEncoder` as requested in issue #959.


Patch:
diff --git a/.codecov.yml b/codecov.yml
similarity index 100%
rename from .codecov.yml
rename to codecov.yml
diff --git a/log/logrus/logrus_logger.go b/log/logrus/logrus_logger.go
index c8a11d5..a5dc9ca 100644
--- a/log/logrus/logrus_logger.go
+++ b/log/logrus/logrus_logger.go
@@ -10,18 +10,37 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-type logrusLogger struct {
-	logrus.FieldLogger
+type Logger struct {
+	field logrus.FieldLogger
+	level logrus.Level
 }
 
+type Option func(*Logger)
+
 var errMissingValue = errors.New("(MISSING)")
 
-// NewLogrusLogger returns a go-kit log.Logger that sends log events to a Logrus logger.
-func NewLogrusLogger(logger logrus.FieldLogger) log.Logger {
-	return &logrusLogger{logger}
+// NewLogger returns a Go kit log.Logger that sends log events to a logrus.Logger.
+func NewLogger(logger logrus.FieldLogger, options ...Option) log.Logger {
+	l := &Logger{
+		field: logger,
+		level: logrus.InfoLevel,
+	}
+
+	for _, optFunc := range options {
+		optFunc(l)
+	}
+
+	return l
 }
 
-func (l logrusLogger) Log(keyvals ...interface{}) error {
+// WithLevel configures a logrus logger to log at level for all events.
+func WithLevel(level logrus.Level) Option {
+	return func(c *Logger) {
+		c.level = level
+	}
+}
+
+func (l Logger) Log(keyvals ...interface{}) error {
 	fields := logrus.Fields{}
 	for i := 0; i < len(keyvals); i += 2 {
 		if i+1 < len(keyvals) {
@@ -30,6 +49,21 @@ func (l logrusLogger) Log(keyvals ...interface{}) error {
 			fields[fmt.Sprint(keyvals[i])] = errMissingValue
 		}
 	}
-	l.WithFields(fields).Info()
+
+	switch l.level {
+	case logrus.InfoLevel:
+		l.field.WithFields(fields).Info()
+	case logrus.ErrorLevel:
+		l.field.WithFields(fields).Error()
+	case logrus.DebugLevel:
+		l.field.WithFields(fields).Debug()
+	case logrus.WarnLevel:
+		l.field.WithFields(fields).Warn()
+	case logrus.TraceLevel:
+		l.field.WithFields(fields).Trace()
+	default:
+		l.field.WithFields(fields).Print()
+	}
+
 	return nil
 }
diff --git a/transport/http/client.go b/transport/http/client.go
index 4cd8f27..7d868ee 100644
--- a/transport/http/client.go
+++ b/transport/http/client.go
@@ -21,9 +21,7 @@ type HTTPClient interface {
 // Client wraps a URL and provides a method that implements endpoint.Endpoint.
 type Client struct {
 	client         HTTPClient
-	method         string
-	tgt            *url.URL
-	enc            EncodeRequestFunc
+	req            CreateRequestFunc
 	dec            DecodeResponseFunc
 	before         []RequestFunc
 	after          []ClientResponseFunc
@@ -32,22 +30,18 @@ type Client struct {
 }
 
 // NewClient constructs a usable Client for a single remote method.
-func NewClient(
-	method string,
-	tgt *url.URL,
-	enc EncodeRequestFunc,
-	dec DecodeResponseFunc,
-	options ...ClientOption,
-) *Client {
+func NewClient(method string, tgt *url.URL, enc EncodeRequestFunc, dec DecodeResponseFunc, options ...ClientOption) *Client {
+	return NewExplicitClient(makeCreateRequestFunc(method, tgt, enc), dec, options...)
+}
+
+// NewExplicitClient is like NewClient but uses a CreateRequestFunc instead of a
+// method, target URL, and EncodeRequestFunc, which allows for more control over
+// the outgoing HTTP request.
+func NewExplicitClient(req CreateRequestFunc, dec DecodeResponseFunc, options ...ClientOption) *Client {
 	c := &Client{
-		client:         http.DefaultClient,
-		method:         method,
-		tgt:            tgt,
-		enc:            enc,
-		dec:            dec,
-		before:         []RequestFunc{},
-		after:          []ClientResponseFunc{},
-		bufferedStream: false,
+		client: http.DefaultClient,
+		req:    req,
+		dec:    dec,
 	}
 	for _, option := range options {
 		option(c)
@@ -64,33 +58,35 @@ func SetClient(client HTTPClient) ClientOption {
 	return func(c *Client) { c.client = client }
 }
 
-// ClientBefore sets the RequestFuncs that are applied to the outgoing HTTP
+// ClientBefore adds one or more RequestFuncs to be applied to the outgoing HTTP
 // request before it's invoked.
 func ClientBefore(before ...RequestFunc) ClientOption {
 	return func(c *Client) { c.before = append(c.before, before...) }
 }
 
-// ClientAfter sets the ClientResponseFuncs applied to the incoming HTTP
-// request prior to it being decoded. This is useful for obtaining anything off
-// of the response and adding onto the context prior to decoding.
+// ClientAfter adds one or more ClientResponseFuncs, which are applied to the
+// incoming HTTP response prior to it being decoded. This is useful for
+// obtaining anything off of the response and adding it into the context prior
+// to decoding.
 func ClientAfter(after ...ClientResponseFunc) ClientOption {
 	return func(c *Client) { c.after = append(c.after, after...) }
 }
 
-// ClientFinalizer is executed at the end of every HTTP request.
-// By default, no finalizer is registered.
+// ClientFinalizer adds one or more ClientFinalizerFuncs to be executed at the
+// end of every HTTP request. Finalizers are executed in the order in which they
+// were added. By default, no finalizer is registered.
 func ClientFinalizer(f ...ClientFinalizerFunc) ClientOption {
 	return func(s *Client) { s.finalizer = append(s.finalizer, f...) }
 }
 
-// BufferedStream sets whether the Response.Body is left open, allowing it
+// BufferedStream sets whether the HTTP response body is left open, allowing it
 // to be read from later. Useful for transporting a file as a buffered stream.
-// That body has to be Closed to propery end the request.
+// That body has to be drained and closed to properly end the request.
 func BufferedStream(buffered bool) ClientOption {
 	return func(c *Client) { c.bufferedStream = buffered }
 }
 
-// Endpoint returns a usable endpoint that invokes the remote endpoint.
+// Endpoint returns a usable Go kit endpoint that calls the remote HTTP endpoint.
 func (c Client) Endpoint() endpoint.Endpoint {
 	return func(ctx context.Context, request interface{}) (interface{}, error) {
 		ctx, cancel := context.WithCancel(ctx)
@@ -111,30 +107,25 @@ func (c Client) Endpoint() endpoint.Endpoint {
 			}()
 		}
 
-		req, err := http.NewRequest(c.method, c.tgt.String(), nil)
+		req, err := c.req(ctx, request)
 		if err != nil {
 			cancel()
 			return nil, err
 		}
 
-		if err = c.enc(ctx, req, request); err != nil {
-			cancel()
-			return nil, err
-		}
-
 		for _, f := range c.before {
 			ctx = f(ctx, req)
 		}
 
 		resp, err = c.client.Do(req.WithContext(ctx))
-
 		if err != nil {
 			cancel()
 			return nil, err
 		}
 
-		// If we expect a buffered stream, we don't cancel the context when the endpoint returns.
-		// Instead, we should call the cancel func when closing the response body.
+		// If the caller asked for a buffered stream, we don't cancel the
+		// context when the endpoint returns. Instead, we should call the
+		// cancel func when closing the response body.
 		if c.bufferedStream {
 			resp.Body = bodyWithCancel{ReadCloser: resp.Body, cancel: cancel}
 		} else {
@@ -207,3 +198,22 @@ func EncodeXMLRequest(c context.Context, r *http.Request, request interface{}) e
 	r.Body = ioutil.NopCloser(&b)
 	return xml.NewEncoder(&b).Encode(request)
 }
+
+//
+//
+//
+
+func makeCreateRequestFunc(method string, target *url.URL, enc EncodeRequestFunc) CreateRequestFunc {
+	return func(ctx context.Context, request interface{}) (*http.Request, error) {
+		req, err := http.NewRequest(method, target.String(), nil)
+		if err != nil {
+			return nil, err
+		}
+
+		if err = enc(ctx, req, request); err != nil {
+			return nil, err
+		}
+
+		return req, nil
+	}
+}
diff --git a/transport/http/encode_decode.go b/transport/http/encode_decode.go
index 2b0334b..b3de462 100644
--- a/transport/http/encode_decode.go
+++ b/transport/http/encode_decode.go
@@ -17,6 +17,12 @@ type DecodeRequestFunc func(context.Context, *http.Request) (request interface{}
 // encodes the object directly to the request body.
 type EncodeRequestFunc func(context.Context, *http.Request, interface{}) error
 
+// CreateRequestFunc creates an outgoing HTTP request based on the passed
+// request object. It's designed to be used in HTTP clients, for client-side
+// endpoints. It's a more powerful version of EncodeRequestFunc, and can be used
+// if more fine-grained control of the HTTP request is required.
+type CreateRequestFunc func(context.Context, interface{}) (*http.Request, error)
+
 // EncodeResponseFunc encodes the passed response object to the HTTP response
 // writer. It's designed to be used in HTTP servers, for server-side
 // endpoints. One straightforward EncodeResponseFunc could be something that
diff --git a/transport/http/jsonrpc/server.go b/transport/http/jsonrpc/server.go
index 0310f3e..75666e4 100644
--- a/transport/http/jsonrpc/server.go
+++ b/transport/http/jsonrpc/server.go
@@ -11,6 +11,10 @@ import (
 	httptransport "github.com/go-kit/kit/transport/http"
 )
 
+type requestIDKeyType struct{}
+
+var requestIDKey requestIDKeyType
+
 // Server wraps an endpoint and implements http.Handler.
 type Server struct {
 	ecm          EndpointCodecMap
@@ -105,6 +109,8 @@ func (s Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	ctx = context.WithValue(ctx, requestIDKey, req.ID)
+
 	// Get the endpoint and codecs from the map using the method
 	// defined in the JSON  object
 	ecm, ok := s.ecm[req.Method]
@@ -160,7 +166,7 @@ func (s Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 // If the error implements ErrorCoder, the provided code will be set on the
 // response error.
 // If the error implements Headerer, the given headers will be set.
-func DefaultErrorEncoder(_ context.Context, err error, w http.ResponseWriter) {
+func DefaultErrorEncoder(ctx context.Context, err error, w http.ResponseWriter) {
 	w.Header().Set("Content-Type", ContentType)
 	if headerer, ok := err.(httptransport.Headerer); ok {
 		for k := range headerer.Headers() {
@@ -177,7 +183,13 @@ func DefaultErrorEncoder(_ context.Context, err error, w http.ResponseWriter) {
 	}
 
 	w.WriteHeader(http.StatusOK)
+
+	var requestID *RequestID
+	if v := ctx.Value(requestIDKey); v != nil {
+		requestID = v.(*RequestID)
+	}
 	_ = json.NewEncoder(w).Encode(Response{
+		ID:      requestID,
 		JSONRPC: Version,
 		Error:   &e,
 	})


Test patch:
diff --git a/log/logrus/logrus_logger_test.go b/log/logrus/logrus_logger_test.go
index cc656ce..cc68184 100644
--- a/log/logrus/logrus_logger_test.go
+++ b/log/logrus/logrus_logger_test.go
@@ -2,6 +2,7 @@ package logrus_test
 
 import (
 	"bytes"
+	"encoding/json"
 	"errors"
 	"strings"
 	"testing"
@@ -16,7 +17,7 @@ func TestLogrusLogger(t *testing.T) {
 	logrusLogger := logrus.New()
 	logrusLogger.Out = buf
 	logrusLogger.Formatter = &logrus.TextFormatter{TimestampFormat: "02-01-2006 15:04:05", FullTimestamp: true}
-	logger := log.NewLogrusLogger(logrusLogger)
+	logger := log.NewLogger(logrusLogger)
 
 	if err := logger.Log("hello", "world"); err != nil {
 		t.Fatal(err)
@@ -53,3 +54,66 @@ func TestLogrusLogger(t *testing.T) {
 type mymap map[int]int
 
 func (m mymap) String() string { return "special_behavior" }
+
+func TestWithLevel(t *testing.T) {
+	tests := []struct {
+		name          string
+		level         logrus.Level
+		expectedLevel logrus.Level
+	}{
+		{
+			name:          "Test Debug level",
+			level:         logrus.DebugLevel,
+			expectedLevel: logrus.DebugLevel,
+		},
+		{
+			name:          "Test Error level",
+			level:         logrus.ErrorLevel,
+			expectedLevel: logrus.ErrorLevel,
+		},
+		{
+			name:          "Test Warn level",
+			level:         logrus.WarnLevel,
+			expectedLevel: logrus.WarnLevel,
+		},
+		{
+			name:          "Test Info level",
+			level:         logrus.InfoLevel,
+			expectedLevel: logrus.InfoLevel,
+		},
+		{
+			name:          "Test Trace level",
+			level:         logrus.TraceLevel,
+			expectedLevel: logrus.TraceLevel,
+		},
+		{
+			name:          "Test not existing level",
+			level:         999,
+			expectedLevel: logrus.InfoLevel,
+		},
+	}
+	for _, tt := range tests {
+		buf := &bytes.Buffer{}
+		logrusLogger := logrus.New()
+		logrusLogger.Out = buf
+		logrusLogger.Level = tt.level
+		logrusLogger.Formatter = &logrus.JSONFormatter{}
+		logger := log.NewLogger(logrusLogger, log.WithLevel(tt.level))
+
+		t.Run(tt.name, func(t *testing.T) {
+			if err := logger.Log(); err != nil {
+				t.Fatal(err)
+			}
+
+			l := map[string]interface{}{}
+			if err := json.Unmarshal(buf.Bytes(), &l); err != nil {
+				t.Fatal(err)
+			}
+
+			if v, ok := l["level"].(string); !ok || v != tt.expectedLevel.String() {
+				t.Fatalf("Logging levels doesn't match. Expected: %s, got: %s", tt.level, v)
+			}
+
+		})
+	}
+}
diff --git a/transport/http/client_test.go b/transport/http/client_test.go
index 9ec1a6c..3748546 100644
--- a/transport/http/client_test.go
+++ b/transport/http/client_test.go
@@ -3,11 +3,13 @@ package http_test
 import (
 	"bytes"
 	"context"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"strings"
 	"testing"
 	"time"
 
@@ -297,6 +299,36 @@ func TestSetClient(t *testing.T) {
 	}
 }
 
+func TestNewExplicitClient(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprintf(w, "%d", r.ContentLength)
+	}))
+	defer srv.Close()
+
+	req := func(ctx context.Context, request interface{}) (*http.Request, error) {
+		req, _ := http.NewRequest("POST", srv.URL, strings.NewReader(request.(string)))
+		return req, nil
+	}
+
+	dec := func(_ context.Context, resp *http.Response) (response interface{}, err error) {
+		buf, err := ioutil.ReadAll(resp.Body)
+		resp.Body.Close()
+		return string(buf), err
+	}
+
+	client := httptransport.NewExplicitClient(req, dec)
+
+	request := "hello world"
+	response, err := client.Endpoint()(context.Background(), request)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if want, have := "11", response.(string); want != have {
+		t.Fatalf("want %q, have %q", want, have)
+	}
+}
+
 func mustParse(s string) *url.URL {
 	u, err := url.Parse(s)
 	if err != nil {
diff --git a/transport/http/jsonrpc/server_test.go b/transport/http/jsonrpc/server_test.go
index d7960fe..c279d3c 100644
--- a/transport/http/jsonrpc/server_test.go
+++ b/transport/http/jsonrpc/server_test.go
@@ -24,11 +24,17 @@ func body(in string) io.Reader {
 	return strings.NewReader(in)
 }
 
+func unmarshalResponse(body []byte) (resp jsonrpc.Response, err error) {
+	err = json.Unmarshal(body, &resp)
+	return
+}
+
 func expectErrorCode(t *testing.T, want int, body []byte) {
-	var r jsonrpc.Response
-	err := json.Unmarshal(body, &r)
+	t.Helper()
+
+	r, err := unmarshalResponse(body)
 	if err != nil {
-		t.Fatalf("Cant' decode response. err=%s, body=%s", err, body)
+		t.Fatalf("Can't decode response: %v (%s)", err, body)
 	}
 	if r.Error == nil {
 		t.Fatalf("Expected error on response. Got none: %s", body)
@@ -38,6 +44,34 @@ func expectErrorCode(t *testing.T, want int, body []byte) {
 	}
 }
 
+func expectValidRequestID(t *testing.T, want int, body []byte) {
+	t.Helper()
+
+	r, err := unmarshalResponse(body)
+	if err != nil {
+		t.Fatalf("Can't decode response: %v (%s)", err, body)
+	}
+	have, err := r.ID.Int()
+	if err != nil {
+		t.Fatalf("Can't get requestID in response. err=%s, body=%s", err, body)
+	}
+	if want != have {
+		t.Fatalf("Request ID: want %d, have %d (%s)", want, have, body)
+	}
+}
+
+func expectNilRequestID(t *testing.T, body []byte) {
+	t.Helper()
+
+	r, err := unmarshalResponse(body)
+	if err != nil {
+		t.Fatalf("Can't decode response: %v (%s)", err, body)
+	}
+	if r.ID != nil {
+		t.Fatalf("Request ID: want nil, have %v", r.ID)
+	}
+}
+
 func nopDecoder(context.Context, json.RawMessage) (interface{}, error) { return struct{}{}, nil }
 func nopEncoder(context.Context, interface{}) (json.RawMessage, error) { return []byte("[]"), nil }
 
@@ -92,6 +126,7 @@ func TestServerBadEndpoint(t *testing.T) {
 	}
 	buf, _ := ioutil.ReadAll(resp.Body)
 	expectErrorCode(t, jsonrpc.InternalError, buf)
+	expectValidRequestID(t, 1, buf)
 }
 
 func TestServerBadEncode(t *testing.T) {
@@ -111,6 +146,7 @@ func TestServerBadEncode(t *testing.T) {
 	}
 	buf, _ := ioutil.ReadAll(resp.Body)
 	expectErrorCode(t, jsonrpc.InternalError, buf)
+	expectValidRequestID(t, 1, buf)
 }
 
 func TestServerErrorEncoder(t *testing.T) {
@@ -162,6 +198,7 @@ func TestCanRejectInvalidJSON(t *testing.T) {
 	}
 	buf, _ := ioutil.ReadAll(resp.Body)
 	expectErrorCode(t, jsonrpc.ParseError, buf)
+	expectNilRequestID(t, buf)
 }
 
 func TestServerUnregisteredMethod(t *testing.T) {
@@ -186,10 +223,9 @@ func TestServerHappyPath(t *testing.T) {
 	if want, have := http.StatusOK, resp.StatusCode; want != have {
 		t.Errorf("want %d, have %d (%s)", want, have, buf)
 	}
-	var r jsonrpc.Response
-	err := json.Unmarshal(buf, &r)
+	r, err := unmarshalResponse(buf)
 	if err != nil {
-		t.Fatalf("Cant' decode response. err=%s, body=%s", err, buf)
+		t.Fatalf("Can't decode response. err=%s, body=%s", err, buf)
 	}
 	if r.JSONRPC != jsonrpc.Version {
 		t.Fatalf("JSONRPC Version: want=%s, got=%s", jsonrpc.Version, r.JSONRPC)