Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always omit params member from request when empty #67

Merged
merged 3 commits into from
Feb 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions call_opt.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
}
111 changes: 0 additions & 111 deletions call_opt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,7 @@ package jsonrpc2_test

import (
"context"
"encoding/json"
"fmt"
"net"
"sync"
"testing"

"github.com/sourcegraph/jsonrpc2"
Expand Down Expand Up @@ -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()
})
})
}
}
43 changes: 27 additions & 16 deletions conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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
Expand Down
123 changes: 120 additions & 3 deletions conn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)),
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change doesn't actually have anything to do with the rest of this PR. I slipped this in because I kept seeing jsonrpc2: protocol error: invalid character 'i' looking for beginning of value in the test logs while working on tests. I introduced this test in 028a50b (#64) and somehow missed that this test was spamming the test logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am only noticing now that this line is commented out. Wow, that's sloppy of me. Will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f74a03b.

connA.Write([]byte("invalid json"))
assertDisconnect(t, c, connB)
})
Expand Down Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched from t.Fatalf to t.Errorf because this function is being reused in multiple tests. A reusable function such as this one cannot know if it's being used in the main goroutine or in a sub-goroutine. It's invalid to call t.Fatal* in a goroutine other than the main goroutine. Using t.Fatalf in this function hasn't caused any issues (so far), but I figured I would make this change for better hygiene. 🙂

}
}

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
}
Loading