From ae88a5e7c0fede2c609c9ac56162c73e688f8b3d Mon Sep 17 00:00:00 2001 From: Sam Herrmann Date: Wed, 22 Feb 2023 03:53:44 -0500 Subject: [PATCH] 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 8012d496 (#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 | 9 ---- call_opt_test.go | 111 ------------------------------------------ conn.go | 43 +++++++++++------ conn_test.go | 123 +++++++++++++++++++++++++++++++++++++++++++++-- example_test.go | 78 ++++++++++++++++++++++++++++++ jsonrpc2.go | 2 +- jsonrpc2_test.go | 7 +++ request.go | 17 ++----- request_test.go | 3 +- response_test.go | 6 +-- 10 files changed, 240 insertions(+), 159 deletions(-) create mode 100644 example_test.go diff --git a/call_opt.go b/call_opt.go index 84a04c2..851baa5 100644 --- 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 index aff8003..b64f661 100644 --- 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 index c2016bf..9de994a 100644 --- 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 index 773c5cb..56e0350 100644 --- 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 +} diff --git a/example_test.go b/example_test.go new file mode 100644 index 0000000..9b2b75a --- /dev/null +++ b/example_test.go @@ -0,0 +1,78 @@ +package jsonrpc2_test + +import ( + "context" + "encoding/json" + "fmt" + "net" + "os" + + "github.com/sourcegraph/jsonrpc2" +) + +// Send a JSON-RPC notification with its params member omitted. +func ExampleConn_Notify_paramsOmitted() { + ctx := context.Background() + + connA, connB := net.Pipe() + defer connA.Close() + defer connB.Close() + + rpcConn := jsonrpc2.NewConn(ctx, jsonrpc2.NewPlainObjectStream(connA), nil) + + // Send the JSON-RPC notification. + go func() { + // Set params to nil. + if err := rpcConn.Notify(ctx, "foo", nil); err != nil { + fmt.Fprintln(os.Stderr, "notify:", err) + } + }() + + // Read the raw JSON-RPC notification on connB. + // + // Reading the raw JSON-RPC request is for the purpose of this example only. + // Use a jsonrpc2.Handler to read parsed requests. + buf := make([]byte, 64) + n, err := connB.Read(buf) + if err != nil { + fmt.Fprintln(os.Stderr, "read:", err) + } + + fmt.Printf("%s\n", buf[:n]) + + // Output: {"jsonrpc":"2.0","method":"foo"} +} + +// Send a JSON-RPC notification with its params member set to null. +func ExampleConn_Notify_nullParams() { + ctx := context.Background() + + connA, connB := net.Pipe() + defer connA.Close() + defer connB.Close() + + rpcConn := jsonrpc2.NewConn(ctx, jsonrpc2.NewPlainObjectStream(connA), nil) + + // Send the JSON-RPC notification. + go func() { + // Set params to the JSON null value. + params := json.RawMessage("null") + if err := rpcConn.Notify(ctx, "foo", params); err != nil { + fmt.Fprintln(os.Stderr, "notify:", err) + } + }() + + // Read the raw JSON-RPC notification on connB. + // + // Reading the raw JSON-RPC request is for the purpose of this example only. + // Use a jsonrpc2.Handler to read parsed requests. + buf := make([]byte, 64) + n, err := connB.Read(buf) + if err != nil { + fmt.Fprintln(os.Stderr, "read:", err) + } + + fmt.Printf("%s\n", buf[:n]) + + // Output: {"jsonrpc":"2.0","method":"foo","params":null} +} diff --git a/jsonrpc2.go b/jsonrpc2.go index 46273bc..97e26d7 100644 --- a/jsonrpc2.go +++ b/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) diff --git a/jsonrpc2_test.go b/jsonrpc2_test.go index dcc2679..8d7968f 100644 --- a/jsonrpc2_test.go +++ b/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") diff --git a/request.go b/request.go index 666573b..372b3e7 100644 --- a/request.go +++ b/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 { diff --git a/request_test.go b/request_test.go index 9d6c243..0e7a7f4 100644 --- a/request_test.go +++ b/request_test.go @@ -20,7 +20,6 @@ func TestRequest_MarshalJSON_jsonrpc(t *testing.T) { } func TestRequest_MarshalUnmarshalJSON(t *testing.T) { - null := json.RawMessage("null") obj := json.RawMessage(`{"foo":"bar"}`) tests := []struct { data []byte @@ -32,7 +31,7 @@ func TestRequest_MarshalUnmarshalJSON(t *testing.T) { }, { data: []byte(`{"id":123,"jsonrpc":"2.0","method":"m","params":null}`), - want: jsonrpc2.Request{ID: jsonrpc2.ID{Num: 123}, Method: "m", Params: &null}, + want: jsonrpc2.Request{ID: jsonrpc2.ID{Num: 123}, Method: "m", Params: &jsonNull}, }, { data: []byte(`{"id":123,"jsonrpc":"2.0","method":"m"}`), diff --git a/response_test.go b/response_test.go index 38eeb52..4819c0e 100644 --- a/response_test.go +++ b/response_test.go @@ -10,8 +10,7 @@ import ( ) func TestResponse_MarshalJSON_jsonrpc(t *testing.T) { - null := json.RawMessage("null") - b, err := json.Marshal(&jsonrpc2.Response{Result: &null}) + b, err := json.Marshal(&jsonrpc2.Response{Result: &jsonNull}) if err != nil { t.Fatal(err) } @@ -60,7 +59,6 @@ func TestResponseUnmarshalJSON_Notif(t *testing.T) { } func TestResponse_MarshalUnmarshalJSON(t *testing.T) { - null := json.RawMessage("null") obj := json.RawMessage(`{"foo":"bar"}`) tests := []struct { data []byte @@ -73,7 +71,7 @@ func TestResponse_MarshalUnmarshalJSON(t *testing.T) { }, { data: []byte(`{"id":123,"result":null,"jsonrpc":"2.0"}`), - want: jsonrpc2.Response{ID: jsonrpc2.ID{Num: 123}, Result: &null}, + want: jsonrpc2.Response{ID: jsonrpc2.ID{Num: 123}, Result: &jsonNull}, }, { data: []byte(`{"id":123,"jsonrpc":"2.0"}`),