Always omit params member from request when empty

With this commit, the JSON encoding of `Request` always omits the params
member when calling `Conn.Call`, `Conn.DispatchCall`, or `Conn.Notify`
with the `params` argument set to `nil`. This change also removes the
`OmitNilParams` call option that was added in commit 8012d49 (#62).

As of this commit, if users desire to send a JSON-RPC request with a
`params` value of `null`, then they may do so by explicitly setting the
`params` argument of `Conn.Call`/`Conn.DispatchCall`/`Conn.Notify` to
`json.RawMessage("null")`.

call_opt.go

@@ -46,12 +46,3 @@ func StringID() CallOption {
 		return nil
 	})
 }
-
-// OmitNilParams returns a call option that instructs requests to omit params
-// values of nil instead of JSON encoding them to null.
-func OmitNilParams() CallOption {
-	return callOptionFunc(func(r *Request) error {
-		r.OmitNilParams = true
-		return nil
-	})
-}

call_opt_test.go

@@ -2,10 +2,7 @@ package jsonrpc2_test
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
-	"net"
-	"sync"
 	"testing"
 
 	"github.com/sourcegraph/jsonrpc2"
@@ -143,111 +140,3 @@ func TestExtraField(t *testing.T) {
 		t.Fatal(err)
 	}
 }
-
-func TestOmitNilParams(t *testing.T) {
-	rawJSONMessage := func(v string) *json.RawMessage {
-		b := []byte(v)
-		return (*json.RawMessage)(&b)
-	}
-
-	type testCase struct {
-		callOpt    jsonrpc2.CallOption
-		sendParams interface{}
-		wantParams *json.RawMessage
-	}
-
-	testCases := []testCase{
-		{
-			sendParams: nil,
-			wantParams: rawJSONMessage("null"),
-		},
-		{
-			sendParams: rawJSONMessage("null"),
-			wantParams: rawJSONMessage("null"),
-		},
-		{
-			callOpt:    jsonrpc2.OmitNilParams(),
-			sendParams: nil,
-			wantParams: nil,
-		},
-		{
-			callOpt:    jsonrpc2.OmitNilParams(),
-			sendParams: rawJSONMessage("null"),
-			wantParams: rawJSONMessage("null"),
-		},
-	}
-
-	assert := func(got *json.RawMessage, want *json.RawMessage) error {
-		// Assert pointers.
-		if got == nil || want == nil {
-			if got != want {
-				return fmt.Errorf("got %v, want %v", got, want)
-			}
-			return nil
-		}
-		{
-			// If pointers are not nil, then assert values.
-			got := string(*got)
-			want := string(*want)
-			if got != want {
-				return fmt.Errorf("got %q, want %q", got, want)
-			}
-		}
-		return nil
-	}
-
-	newClientServer := func(handler jsonrpc2.Handler) (client *jsonrpc2.Conn, server *jsonrpc2.Conn) {
-		ctx := context.Background()
-		connA, connB := net.Pipe()
-		client = jsonrpc2.NewConn(
-			ctx,
-			jsonrpc2.NewPlainObjectStream(connA),
-			noopHandler{},
-		)
-		server = jsonrpc2.NewConn(
-			ctx,
-			jsonrpc2.NewPlainObjectStream(connB),
-			handler,
-		)
-		return client, server
-	}
-
-	for i, tc := range testCases {
-
-		t.Run(fmt.Sprintf("test case %v", i), func(t *testing.T) {
-			t.Run("call", func(t *testing.T) {
-				handler := jsonrpc2.HandlerWithError(func(ctx context.Context, c *jsonrpc2.Conn, r *jsonrpc2.Request) (result interface{}, err error) {
-					return nil, assert(r.Params, tc.wantParams)
-				})
-
-				client, server := newClientServer(handler)
-				defer client.Close()
-				defer server.Close()
-
-				if err := client.Call(context.Background(), "f", tc.sendParams, nil, tc.callOpt); err != nil {
-					t.Fatal(err)
-				}
-			})
-			t.Run("notify", func(t *testing.T) {
-				wg := &sync.WaitGroup{}
-				handler := handlerFunc(func(ctx context.Context, conn *jsonrpc2.Conn, req *jsonrpc2.Request) {
-					err := assert(req.Params, tc.wantParams)
-					if err != nil {
-						t.Error(err)
-					}
-					wg.Done()
-				})
-
-				client, server := newClientServer(handler)
-				defer client.Close()
-				defer server.Close()
-
-				wg.Add(1)
-				if err := client.Notify(context.Background(), "f", tc.sendParams, tc.callOpt); err != nil {
-					t.Fatal(err)
-				}
-				wg.Wait()
-			})
-		})
-	}
-}

conn.go

@@ -102,8 +102,10 @@ func (c *Conn) DispatchCall(ctx context.Context, method string, params interface
 			return Waiter{}, err
 		}
 	}
-	if err := req.SetParams(params); err != nil {
-		return Waiter{}, err
+	if params != nil {
+		if err := req.SetParams(params); err != nil {
+			return Waiter{}, err
+		}
 	}
 	call, err := c.send(ctx, &anyMessage{request: req}, true)
 	if err != nil {
@@ -125,8 +127,10 @@ func (c *Conn) Notify(ctx context.Context, method string, params interface{}, op
 			return err
 		}
 	}
-	if err := req.SetParams(params); err != nil {
-		return err
+	if params != nil {
+		if err := req.SetParams(params); err != nil {
+			return err
+		}
 	}
 	_, err := c.send(ctx, &anyMessage{request: req}, false)
 	return err

conn_test.go

@@ -2,14 +2,69 @@ package jsonrpc2_test
 
 import (
 	"context"
+	"encoding/json"
+	"fmt"
 	"io"
+	"log"
 	"net"
+	"sync"
 	"testing"
 	"time"
 
 	"github.com/sourcegraph/jsonrpc2"
 )
 
+var paramsTests = []struct {
+	sendParams interface{}
+	wantParams *json.RawMessage
+}{
+	{
+		sendParams: nil,
+		wantParams: nil,
+	},
+	{
+		sendParams: jsonNull,
+		wantParams: &jsonNull,
+	},
+	{
+		sendParams: false,
+		wantParams: rawJSONMessage("false"),
+	},
+	{
+		sendParams: 0,
+		wantParams: rawJSONMessage("0"),
+	},
+	{
+		sendParams: "",
+		wantParams: rawJSONMessage(`""`),
+	},
+	{
+		sendParams: rawJSONMessage(`{"foo":"bar"}`),
+		wantParams: rawJSONMessage(`{"foo":"bar"}`),
+	},
+}
+
+func TestConn_DispatchCall(t *testing.T) {
+	for _, test := range paramsTests {
+		t.Run(fmt.Sprintf("%s", test.sendParams), func(t *testing.T) {
+			testParams(t, test.wantParams, func(c *jsonrpc2.Conn) error {
+				_, err := c.DispatchCall(context.Background(), "f", test.sendParams, nil)
+				return err
+			})
+		})
+	}
+}
+
+func TestConn_Notify(t *testing.T) {
+	for _, test := range paramsTests {
+		t.Run(fmt.Sprintf("%s", test.sendParams), func(t *testing.T) {
+			testParams(t, test.wantParams, func(c *jsonrpc2.Conn) error {
+				return c.Notify(context.Background(), "f", test.sendParams)
+			})
+		})
+	}
+}
+
 func TestConn_DisconnectNotify(t *testing.T) {
 
 	t.Run("EOF", func(t *testing.T) {
@@ -47,7 +102,16 @@ func TestConn_DisconnectNotify(t *testing.T) {
 
 	t.Run("protocol error", func(t *testing.T) {
 		connA, connB := net.Pipe()
-		c := jsonrpc2.NewConn(context.Background(), jsonrpc2.NewPlainObjectStream(connB), nil)
+		c := jsonrpc2.NewConn(
+			context.Background(),
+			jsonrpc2.NewPlainObjectStream(connB),
+			noopHandler{},
+			// Suppress log message. This connection receives an invalid JSON
+			// message that causes an error to be written to the logger. We
+			// don't want this expected error to appear in os.Stderr though when
+			// running tests in verbose mode or when other tests fail.
+			jsonrpc2.SetLogger(log.New(io.Discard, "", 0)),
+		)
 		connA.Write([]byte("invalid json"))
 		assertDisconnect(t, c, connB)
 	})
@@ -88,16 +152,69 @@ func TestConn_Close(t *testing.T) {
 	})
 }
 
+func testParams(t *testing.T, want *json.RawMessage, fn func(c *jsonrpc2.Conn) error) {
+	wg := &sync.WaitGroup{}
+	handler := handlerFunc(func(ctx context.Context, conn *jsonrpc2.Conn, r *jsonrpc2.Request) {
+		assertRawJSONMessage(t, r.Params, want)
+		wg.Done()
+	})
+
+	client, server := newClientServer(handler)
+	defer client.Close()
+	defer server.Close()
+
+	wg.Add(1)
+	if err := fn(client); err != nil {
+		t.Error(err)
+	}
+	wg.Wait()
+}
+
 func assertDisconnect(t *testing.T, c *jsonrpc2.Conn, conn io.Writer) {
 	select {
 	case <-c.DisconnectNotify():
 	case <-time.After(200 * time.Millisecond):
-		t.Fatal("no disconnect notification")
+		t.Error("no disconnect notification")
+		return
 	}
 	// Assert that conn is closed by trying to write to it.
 	_, got := conn.Write(nil)
 	want := io.ErrClosedPipe
 	if got != want {
-		t.Fatalf("got %q, want %q", got, want)
+		t.Errorf("got %s, want %s", got, want)
 	}
 }
+
+func assertRawJSONMessage(t *testing.T, got *json.RawMessage, want *json.RawMessage) {
+	// Assert pointers.
+	if got == nil || want == nil {
+		if got != want {
+			t.Errorf("pointer: got %s, want %s", got, want)
+		}
+		return
+	}
+	{
+		// If pointers are not nil, then assert values.
+		got := string(*got)
+		want := string(*want)
+		if got != want {
+			t.Errorf("value: got %q, want %q", got, want)
+		}
+	}
+}
+
+func newClientServer(handler jsonrpc2.Handler) (client *jsonrpc2.Conn, server *jsonrpc2.Conn) {
+	ctx := context.Background()
+	connA, connB := net.Pipe()
+	client = jsonrpc2.NewConn(
+		ctx,
+		jsonrpc2.NewPlainObjectStream(connA),
+		noopHandler{},
+	)
+	server = jsonrpc2.NewConn(
+		ctx,
+		jsonrpc2.NewPlainObjectStream(connB),
+		handler,
+	)
+	return client, server
+}

jsonrpc2.go

@@ -32,7 +32,7 @@ type Error struct {
 	Data    *json.RawMessage `json:"data,omitempty"`
 }
 
-// SetError sets e.Data to the JSON representation of v. If JSON
+// SetError sets e.Data to the JSON encoding of v. If JSON
 // marshaling fails, it panics.
 func (e *Error) SetError(v interface{}) {
 	b, err := json.Marshal(v)

jsonrpc2_test.go

@@ -321,3 +321,10 @@ func serve(ctx context.Context, lis net.Listener, h jsonrpc2.Handler, streamMake
 		jsonrpc2.NewConn(ctx, streamMaker(conn), h, opts...)
 	}
 }
+
+func rawJSONMessage(v string) *json.RawMessage {
+	b := []byte(v)
+	return (*json.RawMessage)(&b)
+}
+
+var jsonNull = json.RawMessage("null")

request.go

@@ -28,9 +28,6 @@ type Request struct {
 	// NOTE: It is not part of the spec, but there are other protocols based on
 	// JSON-RPC 2 that require it.
 	ExtraFields []RequestField `json:"-"`
-	// OmitNilParams instructs the SetParams method to not JSON encode a nil
-	// value and set Params to nil instead.
-	OmitNilParams bool `json:"-"`
 }
 
 // MarshalJSON implements json.Marshaler and adds the "jsonrpc":"2.0"
@@ -133,15 +130,9 @@ func (r *Request) UnmarshalJSON(data []byte) error {
 	return nil
 }
 
-// SetParams sets r.Params to the JSON representation of v. If JSON marshaling
-// fails, it returns an error. Beware that the JSON encoding of nil is null. If
-// r.OmitNilParams is true and v is nil, then r.Params is set to nil and
-// therefore omitted from the JSON-RPC request.
+// SetParams sets r.Params to the JSON encoding of v. If JSON
+// marshaling fails, it returns an error.
 func (r *Request) SetParams(v interface{}) error {
-	if r.OmitNilParams && v == nil {
-		r.Params = nil
-		return nil
-	}
 	b, err := json.Marshal(v)
 	if err != nil {
 		return err
@@ -150,7 +141,7 @@ func (r *Request) SetParams(v interface{}) error {
 	return nil
 }
 
-// SetMeta sets r.Meta to the JSON representation of v. If JSON
+// SetMeta sets r.Meta to the JSON encoding of v. If JSON
 // marshaling fails, it returns an error.
 func (r *Request) SetMeta(v interface{}) error {
 	b, err := json.Marshal(v)
@@ -162,7 +153,7 @@ func (r *Request) SetMeta(v interface{}) error {
 }
 
 // SetExtraField adds an entry to r.ExtraFields, so that it is added to the
-// JSON representation of the request, as a way to add arbitrary extensions to
+// JSON encoding of the request, as a way to add arbitrary extensions to
 // JSON RPC 2.0. If JSON marshaling fails, it returns an error.
 func (r *Request) SetExtraField(name string, v interface{}) error {
 	switch name {