Always omit params member from request when empty (#67)

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")`.
sourcegraph · Feb 22, 2023 · ae88a5e · ae88a5e
1 parent 6864d8c
commit ae88a5e
Show file tree

Hide file tree

Showing 10 changed files with 240 additions and 159 deletions.
diff --git a/call_opt.go b/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
-	})
-}
diff --git a/call_opt_test.go b/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()
-			})
-		})
-	}
-}
diff --git a/conn.go b/conn.go
@@ -69,10 +69,10 @@ func (c *Conn) Close() error {
 	return c.close(nil)
 }
 
-// Call initiates a JSON-RPC call using the specified method and
-// params, and waits for the response. If the response is successful,
-// its result is stored in result (a pointer to a value that can be
-// JSON-unmarshaled into); otherwise, a non-nil error is returned.
+// Call initiates a JSON-RPC call using the specified method and params, and
+// waits for the response. If the response is successful, its result is stored
+// in result (a pointer to a value that can be JSON-unmarshaled into);
+// otherwise, a non-nil error is returned. See DispatchCall for more details.
 func (c *Conn) Call(ctx context.Context, method string, params, result interface{}, opts ...CallOption) error {
 	call, err := c.DispatchCall(ctx, method, params, opts...)
 	if err != nil {
@@ -87,11 +87,14 @@ func (c *Conn) DisconnectNotify() <-chan struct{} {
 	return c.disconnect
 }
 
-// DispatchCall dispatches a JSON-RPC call using the specified method
-// and params, and returns a call proxy or an error. Call Wait()
-// on the returned proxy to receive the response. Only use this
-// function if you need to do work after dispatching the request,
-// otherwise use Call.
+// DispatchCall dispatches a JSON-RPC call using the specified method and
+// params, and returns a call proxy or an error. Call Wait() on the returned
+// proxy to receive the response. Only use this function if you need to do work
+// after dispatching the request, otherwise use Call.
+//
+// The params member is omitted from the JSON-RPC request if the given params is
+// nil. Use json.RawMessage("null") to send a JSON-RPC request with its params
+// member set to null.
 func (c *Conn) DispatchCall(ctx context.Context, method string, params interface{}, opts ...CallOption) (Waiter, error) {
 	req := &Request{Method: method}
 	for _, opt := range opts {
@@ -102,8 +105,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 {
@@ -112,9 +117,13 @@ func (c *Conn) DispatchCall(ctx context.Context, method string, params interface
 	return Waiter{call: call}, nil
 }
 
-// Notify is like Call, but it returns when the notification request
-// is sent (without waiting for a response, because JSON-RPC
-// notifications do not have responses).
+// Notify is like Call, but it returns when the notification request is sent
+// (without waiting for a response, because JSON-RPC notifications do not have
+// responses).
+//
+// The params member is omitted from the JSON-RPC request if the given params is
+// nil. Use json.RawMessage("null") to send a JSON-RPC request with its params
+// member set to null.
 func (c *Conn) Notify(ctx context.Context, method string, params interface{}, opts ...CallOption) error {
 	req := &Request{Method: method, Notif: true}
 	for _, opt := range opts {
@@ -125,8 +134,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

diff --git a/conn_test.go b/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)
+				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
+}