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

Add a way to specify more non-standard-compliant fields to Request #50

Merged
merged 3 commits into from
Aug 4, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
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: 9 additions & 0 deletions call_opt.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,15 @@ func Meta(meta interface{}) CallOption {
})
}

// ExtraField returns a call option which attaches the given name/value pair to
// the JSON-RPC 2.0 request. This can be used to add arbitrary extensions to
// JSON RPC 2.0.
func ExtraField(name string, value interface{}) CallOption {
return callOptionFunc(func(r *Request) error {
return r.SetExtraField(name, value)
})
}

// PickID returns a call option which sets the ID on a request. Care must be
// taken to ensure there are no conflicts with any previously picked ID, nor
// with the default sequence ID.
Expand Down
47 changes: 47 additions & 0 deletions call_opt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package jsonrpc2_test

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

Expand Down Expand Up @@ -90,3 +91,49 @@ func TestStringID(t *testing.T) {
t.Fatal(err)
}
}

func TestExtraField(t *testing.T) {

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

a, b := inMemoryPeerConns()
defer a.Close()
defer b.Close()

handler := handlerFunc(func(ctx context.Context, conn *jsonrpc2.Conn, req *jsonrpc2.Request) {
replyWithError := func(msg string) {
respErr := &jsonrpc2.Error{Code: jsonrpc2.CodeInvalidRequest, Message: msg}
if err := conn.ReplyWithError(ctx, req.ID, respErr); err != nil {
t.Error(err)
}
}
var sessionId *json.RawMessage
for _, field := range req.ExtraFields {
if field.Name != "sessionId" {
continue
}
sessionId = field.Value
}
if sessionId == nil {
replyWithError("sessionId must be set")
return
}
if string(*sessionId) != `"session"` {
replyWithError("sessionId has the wrong value")
return
}
if err := conn.Reply(ctx, req.ID, "ok"); err != nil {
t.Error(err)
}
})
connA := jsonrpc2.NewConn(ctx, jsonrpc2.NewBufferedStream(a, jsonrpc2.VSCodeObjectCodec{}), handler)
connB := jsonrpc2.NewConn(ctx, jsonrpc2.NewBufferedStream(b, jsonrpc2.VSCodeObjectCodec{}), noopHandler{})
defer connA.Close()
defer connB.Close()

var res string
if err := connB.Call(ctx, "f", nil, &res, jsonrpc2.ExtraField("sessionId", "session")); err != nil {
t.Fatal(err)
}
}
70 changes: 58 additions & 12 deletions jsonrpc2.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ type JSONRPC2 interface {
Close() error
}

// RequestField is a top-level field that can be added to the JSON-RPC request.
type RequestField struct {
Name string
Value *json.RawMessage
}

// Request represents a JSON-RPC request or
// notification. See
// http://www.jsonrpc.org/specification#request_object and
Expand All @@ -45,25 +51,32 @@ type Request struct {
// NOTE: It is not part of spec. However, it is useful for propogating
// tracing context, etc.
Meta *json.RawMessage `json:"meta,omitempty"`

// ExtraFields optionally adds fields to the root of the JSON-RPC request.
//
// NOTE: It is not part of the spec, but there are other protocols based on
// JSON-RPC 2 that require it.
ExtraFields []RequestField `json:"-"`
}

// MarshalJSON implements json.Marshaler and adds the "jsonrpc":"2.0"
// property.
func (r Request) MarshalJSON() ([]byte, error) {
r2 := struct {
Method string `json:"method"`
Params *json.RawMessage `json:"params,omitempty"`
ID *ID `json:"id,omitempty"`
Meta *json.RawMessage `json:"meta,omitempty"`
JSONRPC string `json:"jsonrpc"`
}{
Method: r.Method,
Params: r.Params,
Meta: r.Meta,
JSONRPC: "2.0",
r2 := map[string]interface{}{
"jsonrpc": "2.0",
"method": r.Method,
}
for _, field := range r.ExtraFields {
r2[field.Name] = field.Value
}
if !r.Notif {
r2.ID = &r.ID
r2["id"] = &r.ID
}
if r.Params != nil {
r2["params"] = r.Params
}
if r.Meta != nil {
r2["meta"] = r.Meta
}
return json.Marshal(r2)
}
Expand All @@ -76,6 +89,8 @@ func (r *Request) UnmarshalJSON(data []byte) error {
Meta *json.RawMessage `json:"meta,omitempty"`
ID *ID `json:"id"`
}
// This is used to get the extra fields, which are not type-safe.
r3 := make(map[string]*json.RawMessage)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

re: #49 (comment)

things i tried:

  • make r2 a map[string]interface{}, and it's possible to extract the method, params, meta, and id, but then i needed to marshal all the extra fields into a *json.RawMessage.
  • make r2 a map[string]*json.RawMessage, and it's now possible to extract the extra fields, but the method, params, meta, and id now need an extra unmarshal.
  • make the RequestField.Value an interface{} so that the first option is easier (but didn't really try that because it felt inconsistent with the Meta and Params fields). it also defers the marshaling error to the MarshalJSON, but that might not be such a big deal since both would happen at the .Call() API level.

Copy link
Member

Choose a reason for hiding this comment

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

  • make r2 a map[string]interface{}, and it's possible to extract the method, params, meta, and id, but then i needed to marshal all the extra fields into a *json.RawMessage.

Can't we just make RequestField.Value an interface{}? This seems like the easiest option. It does mean if you want to avoid marshalling on extra fields you can't. Is this acceptable?

  • make r2 a map[string]*json.RawMessage, and it's now possible to extract the extra fields, but the method, params, meta, and id now need an extra unmarshal.

This seems like the best option for avoiding the extra work/allocations? It would also simplify our handling of detecting if params is JSON null? I'm a bit cognizant of "doubling" the work this does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't we just make RequestField.Value an interface{}? This seems like the easiest option. It does mean if you want to avoid marshalling on extra fields you can't. Is this acceptable?

totally acceptable (and in fact will make one particular test easier for us! haha), I just didn't want to start with an option that introduced inconsistency without checking with y'all.

This seems like the best option for avoiding the extra work/allocations? It would also simplify our handling of detecting if params is JSON null? I'm a bit cognizant of "doubling" the work this does.

it avoids the extra allocations, but introduces extra casts. the amount of work is roughly the same but it makes (our particular usage of) the interface a bit simpler due to the lack of the pre-marshaling for the extra fields.

Copy link
Contributor Author

@lhchavez lhchavez Jul 28, 2021

Choose a reason for hiding this comment

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

oh wait, i lied: this unmarshaling option also adds extra marshaling: the params, id, and meta would now be fully deserialized by the map[string]interface{}, so now we would need to marshsal 'em back to a *json.RawMessage or ID.


// Detect if the "params" field is JSON "null" or just not present
// by seeing if the field gets overwritten to nil.
Expand All @@ -84,6 +99,9 @@ func (r *Request) UnmarshalJSON(data []byte) error {
if err := json.Unmarshal(data, &r2); err != nil {
return err
}
if err := json.Unmarshal(data, &r3); err != nil {
return err
}
r.Method = r2.Method
switch {
case r2.Params == nil:
Expand All @@ -101,6 +119,19 @@ func (r *Request) UnmarshalJSON(data []byte) error {
r.ID = *r2.ID
r.Notif = false
}

// Clear the extra fields before populating them again.
r.ExtraFields = nil
for name, value := range r3 {
switch name {
case "id", "jsonrpc", "meta", "method", "params":
continue
}
r.ExtraFields = append(r.ExtraFields, RequestField{
Name: name,
Value: value,
})
}
return nil
}

Expand All @@ -126,6 +157,21 @@ func (r *Request) SetMeta(v interface{}) error {
return nil
}

// 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 RPC 2.0. If JSON marshaling fails, it returns an error.
func (r *Request) SetExtraField(name string, v interface{}) error {
b, err := json.Marshal(v)
if err != nil {
return err
}
r.ExtraFields = append(r.ExtraFields, RequestField{
Name: name,
Value: (*json.RawMessage)(&b),
})
return nil
}

// Response represents a JSON-RPC response. See
// http://www.jsonrpc.org/specification#response_object.
type Response struct {
Expand Down
2 changes: 1 addition & 1 deletion jsonrpc2_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ func TestRequest_MarshalJSON_jsonrpc(t *testing.T) {
if err != nil {
t.Fatal(err)
}
if want := `{"method":"","id":0,"jsonrpc":"2.0"}`; string(b) != want {
if want := `{"id":0,"jsonrpc":"2.0","method":""}`; string(b) != want {
t.Errorf("got %q, want %q", b, want)
}
}
Expand Down
11 changes: 8 additions & 3 deletions object_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,22 +39,27 @@ func TestAnyMessage(t *testing.T) {
func TestRequest_MarshalUnmarshalJSON(t *testing.T) {
null := json.RawMessage("null")
obj := json.RawMessage(`{"foo":"bar"}`)
requestFieldValue := json.RawMessage(`"session"`)
tests := []struct {
data []byte
want Request
}{
{
data: []byte(`{"method":"m","params":{"foo":"bar"},"id":123,"jsonrpc":"2.0"}`),
data: []byte(`{"id":123,"jsonrpc":"2.0","method":"m","params":{"foo":"bar"}}`),
want: Request{ID: ID{Num: 123}, Method: "m", Params: &obj},
},
{
data: []byte(`{"method":"m","params":null,"id":123,"jsonrpc":"2.0"}`),
data: []byte(`{"id":123,"jsonrpc":"2.0","method":"m","params":null}`),
want: Request{ID: ID{Num: 123}, Method: "m", Params: &null},
},
{
data: []byte(`{"method":"m","id":123,"jsonrpc":"2.0"}`),
data: []byte(`{"id":123,"jsonrpc":"2.0","method":"m"}`),
want: Request{ID: ID{Num: 123}, Method: "m", Params: nil},
},
{
data: []byte(`{"id":123,"jsonrpc":"2.0","method":"m","sessionId":"session"}`),
want: Request{ID: ID{Num: 123}, Method: "m", Params: nil, ExtraFields: []RequestField{{Name: "sessionId", Value: &requestFieldValue}}},
},
}
for _, test := range tests {
var got Request
Expand Down