From 120094814e879eefc7beda873b60b582af5dc8a1 Mon Sep 17 00:00:00 2001 From: "M. J. Fromberger" Date: Tue, 17 Nov 2020 15:05:08 -0800 Subject: [PATCH 1/8] WIP: Add UnknownFielder interface. A possible solution for #31. --- base.go | 33 +++++++++++++++++++++------------ examples_test.go | 2 +- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/base.go b/base.go index ebda4c8..1b8de1c 100644 --- a/base.go +++ b/base.go @@ -71,10 +71,8 @@ func (r *Request) HasParams() bool { return len(r.params) != 0 } // an InvalidParams error. // // By default, unknown keys are disallowed when unmarshaling into a v of struct -// type. The caller may override this using jrpc2.NonStrict, by implementing -// json.Unmarshaler on the concrete type of v, or by unmarshaling into a -// json.RawMessage and separately decoding the result. The examples demonstrate -// how to do this. +// type. This can be overridden by implementing an UnknownFields method that +// returns true, on the concrete type of v. // // If v has type *json.RawMessage, decoding cannot fail. func (r *Request) UnmarshalParams(v interface{}) error { @@ -85,7 +83,9 @@ func (r *Request) UnmarshalParams(v interface{}) error { return nil } dec := json.NewDecoder(bytes.NewReader(r.params)) - dec.DisallowUnknownFields() + if uf, ok := v.(UnknownFielder); !ok || !uf.UnknownFields() { + dec.DisallowUnknownFields() + } if err := dec.Decode(v); err != nil { return Errorf(code.InvalidParams, "invalid parameters: %v", err.Error()) } @@ -410,18 +410,27 @@ func filterError(e *Error) error { return e } -// NonStrict wraps a value v so that it can be unmarshaled from JSON without -// checking for unknown fields. The v provided must itself be a valid argument -// to json.Unmarshal. +// UnknownFielder is an optional interface that can be implemented by a +// parameter type to allow control over whether unknown fields should be +// allowed when unmarshaling from JSON. +// +// If a type does not implement this interface, unknown fields are disallowed. +type UnknownFielder interface { + // Report whether unknown fields should be permitted when unmarshaling into + // the receiver. + UnknownFields() bool +} + +// NonStrict wraps a value v so that it can be unmarshaled as a parameter value +// from JSON without checking for unknown fields. // -// This can be used to unmarshal request parameters with unknown fields, for -// example: +// For example: // // var obj RequestType // err := req.UnmarshalParams(jrpc2.NonStrict(&obj)) // -func NonStrict(v interface{}) json.Unmarshaler { return &nonStrict{v: v} } +func NonStrict(v interface{}) interface{} { return &nonStrict{v: v} } type nonStrict struct{ v interface{} } -func (n nonStrict) UnmarshalJSON(data []byte) error { return json.Unmarshal(data, n.v) } +func (nonStrict) UnknownFields() bool { return true } diff --git a/examples_test.go b/examples_test.go index 08d207f..4c3c795 100644 --- a/examples_test.go +++ b/examples_test.go @@ -120,7 +120,7 @@ func ExampleRequest_UnmarshalParams() { log.Fatalf("Expected invalid parameters, got: %v", err) } - // Solution 1: Decode with jrpc2.NonStrict. + // Solution 1: Implement jrpc2.UnknownFielder. if err := reqs[0].UnmarshalParams(jrpc2.NonStrict(&t)); err != nil { log.Fatalf("UnmarshalParams: %v", err) } From 5701a5522a6267c49770ea175d05a0c1e0df8a55 Mon Sep 17 00:00:00 2001 From: "M. J. Fromberger" Date: Tue, 17 Nov 2020 15:08:13 -0800 Subject: [PATCH 2/8] WIP: Update documentation. --- base.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/base.go b/base.go index 1b8de1c..5129085 100644 --- a/base.go +++ b/base.go @@ -71,8 +71,10 @@ func (r *Request) HasParams() bool { return len(r.params) != 0 } // an InvalidParams error. // // By default, unknown keys are disallowed when unmarshaling into a v of struct -// type. This can be overridden by implementing an UnknownFields method that -// returns true, on the concrete type of v. +// type. This can be overridden by providing a custom implementation of +// json.Unmarshaler, or by implementing an UnknownFields method that returns +// true, on the concrete type of v. The jrpc2.NonStrict helper function adapts +// existing values to the UnknownFielder interface. // // If v has type *json.RawMessage, decoding cannot fail. func (r *Request) UnmarshalParams(v interface{}) error { From e8255414851e8b7302e7ba9849f3ef1aad15196b Mon Sep 17 00:00:00 2001 From: "M. J. Fromberger" Date: Wed, 18 Nov 2020 08:57:13 -0800 Subject: [PATCH 3/8] WIP: Permit unknown fields by default. --- base.go | 38 +++++++++++++++++--------------------- examples_test.go | 14 +++++++------- internal_test.go | 2 +- jrpc2_test.go | 8 ++++---- 4 files changed, 29 insertions(+), 33 deletions(-) diff --git a/base.go b/base.go index 5129085..8b3c3bc 100644 --- a/base.go +++ b/base.go @@ -70,11 +70,11 @@ func (r *Request) HasParams() bool { return len(r.params) != 0 } // parameters, it returns nil without modifying v. If r is invalid it returns // an InvalidParams error. // -// By default, unknown keys are disallowed when unmarshaling into a v of struct -// type. This can be overridden by providing a custom implementation of -// json.Unmarshaler, or by implementing an UnknownFields method that returns -// true, on the concrete type of v. The jrpc2.NonStrict helper function adapts -// existing values to the UnknownFielder interface. +// By default, unknown object keys are ignored when unmarshaling into a v of +// struct type. This can be overridden either by giving the type of v a custom +// implementation of json.Unmarshaler, or by implementing the StrictFielder +// interface. The jrpc2.StrictFields helper function adapts existing values to +// the StrictFielder interface. // // If v has type *json.RawMessage, decoding cannot fail. func (r *Request) UnmarshalParams(v interface{}) error { @@ -85,7 +85,7 @@ func (r *Request) UnmarshalParams(v interface{}) error { return nil } dec := json.NewDecoder(bytes.NewReader(r.params)) - if uf, ok := v.(UnknownFielder); !ok || !uf.UnknownFields() { + if _, ok := v.(StrictFielder); ok { dec.DisallowUnknownFields() } if err := dec.Decode(v); err != nil { @@ -412,27 +412,23 @@ func filterError(e *Error) error { return e } -// UnknownFielder is an optional interface that can be implemented by a -// parameter type to allow control over whether unknown fields should be -// allowed when unmarshaling from JSON. -// -// If a type does not implement this interface, unknown fields are disallowed. -type UnknownFielder interface { - // Report whether unknown fields should be permitted when unmarshaling into - // the receiver. - UnknownFields() bool +// StrictFielder is an optional interface that can be implemented by a type to +// reject unknown fields when unmarshaling from JSON. If a type does not +// implement this interface, unknown fields are ignored. +type StrictFielder interface { + StrictFields() } -// NonStrict wraps a value v so that it can be unmarshaled as a parameter value -// from JSON without checking for unknown fields. +// StrictFields wraps a value v so that it can be unmarshaled as a parameter +// value from JSON without checking for unknown fields. // // For example: // // var obj RequestType -// err := req.UnmarshalParams(jrpc2.NonStrict(&obj)) +// err := req.UnmarshalParams(jrpc2.StrictFields(&obj)) // -func NonStrict(v interface{}) interface{} { return &nonStrict{v: v} } +func StrictFields(v interface{}) interface{} { return &strict{v: v} } -type nonStrict struct{ v interface{} } +type strict struct{ v interface{} } -func (nonStrict) UnknownFields() bool { return true } +func (strict) StrictFields() {} diff --git a/examples_test.go b/examples_test.go index 4c3c795..ee1419e 100644 --- a/examples_test.go +++ b/examples_test.go @@ -114,15 +114,15 @@ func ExampleRequest_UnmarshalParams() { B int `json:"b"` } - // By default, unmarshaling prohibits unknown fields (here, "c"). - err = reqs[0].UnmarshalParams(&t) - if code.FromError(err) != code.InvalidParams { - log.Fatalf("Expected invalid parameters, got: %v", err) + // By default, unmarshaling ignores unknown fields (here, "c"). + if err := reqs[0].UnmarshalParams(&t); err != nil { + log.Fatalf("UnmarshalParams: %v", err) } - // Solution 1: Implement jrpc2.UnknownFielder. - if err := reqs[0].UnmarshalParams(jrpc2.NonStrict(&t)); err != nil { - log.Fatalf("UnmarshalParams: %v", err) + // Solution 1: Use the jrpc2.StrictFields helper. + err = reqs[0].UnmarshalParams(jrpc2.StrictFields(&t)) + if code.FromError(err) != code.InvalidParams { + log.Fatalf("UnmarshalParams strict: %v", err) } fmt.Printf("t.A=%d, t.B=%d\n", t.A, t.B) diff --git a/internal_test.go b/internal_test.go index 4d0516d..04c4fe0 100644 --- a/internal_test.go +++ b/internal_test.go @@ -107,7 +107,7 @@ func TestUnmarshalParams(t *testing.T) { {`{"jsonrpc":"2.0", "id":5, "method":"Z", "params":{"x":23, "y":true}}`, xy{X: 23, Y: true}, `{"x":23, "y":true}`, code.NoError}, {`{"jsonrpc":"2.0", "id":6, "method":"Z", "params":{"x":23, "z":"wat"}}`, - xy{}, `{"x":23, "z":"wat"}`, code.InvalidParams}, + xy{X: 23}, `{"x":23, "z":"wat"}`, code.NoError}, } for _, test := range tests { req, err := ParseRequests([]byte(test.input)) diff --git a/jrpc2_test.go b/jrpc2_test.go index 907e534..b6277ca 100644 --- a/jrpc2_test.go +++ b/jrpc2_test.go @@ -1065,7 +1065,7 @@ func (buggyChannel) Send([]byte) error { panic("should not be called") } func (b buggyChannel) Recv() ([]byte, error) { return []byte(b.data), b.err } func (buggyChannel) Close() error { return nil } -func TestNonStrict(t *testing.T) { +func TestStrictFields(t *testing.T) { type other struct { C bool `json:"charlie"` } @@ -1078,12 +1078,12 @@ func TestNonStrict(t *testing.T) { "Test": handler.New(func(ctx context.Context, req *jrpc2.Request) error { var ps, qs params - if err := req.UnmarshalParams(&ps); err == nil { + if err := req.UnmarshalParams(jrpc2.StrictFields(&ps)); err == nil { t.Errorf("Unmarshal strict: got %+v, want error", ps) } - if err := req.UnmarshalParams(jrpc2.NonStrict(&qs)); err != nil { - t.Errorf("Unmarshal non-strict: unexpected error: %v", err) + if err := req.UnmarshalParams(&qs); err != nil { + t.Errorf("Unmarshal non-strict (default): unexpected error: %v", err) } else { t.Logf("Parameters OK: %+v", qs) } From a3de7d4bc281d81c3a45863059827e9e8a8056b2 Mon Sep 17 00:00:00 2001 From: "M. J. Fromberger" Date: Wed, 18 Nov 2020 10:13:42 -0800 Subject: [PATCH 4/8] WIP: Update method name. - Unexport the StrictFielder interface. - Rename the method to DisallowUnknownFields. - Fix some documentation errors. --- base.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/base.go b/base.go index 8b3c3bc..cc48698 100644 --- a/base.go +++ b/base.go @@ -72,9 +72,9 @@ func (r *Request) HasParams() bool { return len(r.params) != 0 } // // By default, unknown object keys are ignored when unmarshaling into a v of // struct type. This can be overridden either by giving the type of v a custom -// implementation of json.Unmarshaler, or by implementing the StrictFielder -// interface. The jrpc2.StrictFields helper function adapts existing values to -// the StrictFielder interface. +// implementation of json.Unmarshaler, or implementing a DisallowUnknownFields +// method. The jrpc2.StrictFields helper function adapts existing values to +// this interface. // // If v has type *json.RawMessage, decoding cannot fail. func (r *Request) UnmarshalParams(v interface{}) error { @@ -85,7 +85,7 @@ func (r *Request) UnmarshalParams(v interface{}) error { return nil } dec := json.NewDecoder(bytes.NewReader(r.params)) - if _, ok := v.(StrictFielder); ok { + if _, ok := v.(strictFielder); ok { dec.DisallowUnknownFields() } if err := dec.Decode(v); err != nil { @@ -412,15 +412,15 @@ func filterError(e *Error) error { return e } -// StrictFielder is an optional interface that can be implemented by a type to +// strictFielder is an optional interface that can be implemented by a type to // reject unknown fields when unmarshaling from JSON. If a type does not // implement this interface, unknown fields are ignored. -type StrictFielder interface { - StrictFields() +type strictFielder interface { + DisallowUnknownFields() } -// StrictFields wraps a value v so that it can be unmarshaled as a parameter -// value from JSON without checking for unknown fields. +// StrictFields wraps a value v to implement the DisallowUnknownFields method, +// requiring unknown fields to be rejected when unmarshaling from JSON. // // For example: // @@ -431,4 +431,4 @@ func StrictFields(v interface{}) interface{} { return &strict{v: v} } type strict struct{ v interface{} } -func (strict) StrictFields() {} +func (strict) DisallowUnknownFields() {} From 60da453fd6f81a0587e170a7b7a3f93ab01342a1 Mon Sep 17 00:00:00 2001 From: "M. J. Fromberger" Date: Wed, 18 Nov 2020 10:17:46 -0800 Subject: [PATCH 5/8] WIP: Don't create a decoder if not needed. When unmarshaling parameters without the strict field check, don't create a byte reader and decoer; just call json.Unmarshal directly. --- base.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/base.go b/base.go index cc48698..acd6c94 100644 --- a/base.go +++ b/base.go @@ -84,14 +84,14 @@ func (r *Request) UnmarshalParams(v interface{}) error { *raw = json.RawMessage(string(r.params)) // copy return nil } - dec := json.NewDecoder(bytes.NewReader(r.params)) if _, ok := v.(strictFielder); ok { + dec := json.NewDecoder(bytes.NewReader(r.params)) dec.DisallowUnknownFields() + if err := dec.Decode(v); err != nil { + return Errorf(code.InvalidParams, "invalid parameters: %v", err.Error()) + } } - if err := dec.Decode(v); err != nil { - return Errorf(code.InvalidParams, "invalid parameters: %v", err.Error()) - } - return nil + return json.Unmarshal(r.params, v) } // ParamString returns the encoded request parameters of r as a string. @@ -425,7 +425,7 @@ type strictFielder interface { // For example: // // var obj RequestType -// err := req.UnmarshalParams(jrpc2.StrictFields(&obj)) +// err := req.UnmarshalParams(jrpc2.StrictFields(&obj))` // func StrictFields(v interface{}) interface{} { return &strict{v: v} } From 3ccb76b92c7cc23b6ed7f794217e3ab870cfa36b Mon Sep 17 00:00:00 2001 From: "M. J. Fromberger" Date: Wed, 18 Nov 2020 10:26:31 -0800 Subject: [PATCH 6/8] WIP: Support strict field checks in UnmarshalResult too. --- base.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/base.go b/base.go index acd6c94..9f26986 100644 --- a/base.go +++ b/base.go @@ -154,10 +154,21 @@ func (r *Response) Error() *Error { return r.err } // UnmarshalResult decodes the result message into v. If the request failed, // UnmarshalResult returns the *Error value that would also be returned by // r.Error(), and v is unmodified. +// +// By default, unknown object keys are ignored when unmarshaling into a v of +// struct type. This can be overridden either by giving the type of v a custom +// implementation of json.Unmarshaler, or implementing a DisallowUnknownFields +// method. The jrpc2.StrictFields helper function adapts existing values to +// this interface. func (r *Response) UnmarshalResult(v interface{}) error { if r.err != nil { return r.err } + if _, ok := v.(strictFielder); ok { + dec := json.NewDecoder(bytes.NewReader(r.result)) + dec.DisallowUnknownFields() + return dec.Decode(v) + } return json.Unmarshal(r.result, v) } From 1d0022b27a52a0c4af67a208bb0d3212ef2ff759 Mon Sep 17 00:00:00 2001 From: "M. J. Fromberger" Date: Wed, 18 Nov 2020 10:32:23 -0800 Subject: [PATCH 7/8] WIP: use type switches. --- base.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/base.go b/base.go index 9f26986..3ea16b7 100644 --- a/base.go +++ b/base.go @@ -80,16 +80,18 @@ func (r *Request) HasParams() bool { return len(r.params) != 0 } func (r *Request) UnmarshalParams(v interface{}) error { if len(r.params) == 0 { return nil - } else if raw, ok := v.(*json.RawMessage); ok { - *raw = json.RawMessage(string(r.params)) // copy - return nil } - if _, ok := v.(strictFielder); ok { + switch t := v.(type) { + case *json.RawMessage: + *t = json.RawMessage(string(r.params)) // copy + return nil + case strictFielder: dec := json.NewDecoder(bytes.NewReader(r.params)) dec.DisallowUnknownFields() if err := dec.Decode(v); err != nil { return Errorf(code.InvalidParams, "invalid parameters: %v", err.Error()) } + return nil } return json.Unmarshal(r.params, v) } @@ -164,7 +166,11 @@ func (r *Response) UnmarshalResult(v interface{}) error { if r.err != nil { return r.err } - if _, ok := v.(strictFielder); ok { + switch t := v.(type) { + case *json.RawMessage: + *t = json.RawMessage(string(r.result)) // copy + return nil + case strictFielder: dec := json.NewDecoder(bytes.NewReader(r.result)) dec.DisallowUnknownFields() return dec.Decode(v) From d3f7574d205213fb39f4f27854b096f18497a242 Mon Sep 17 00:00:00 2001 From: "M. J. Fromberger" Date: Wed, 18 Nov 2020 10:38:46 -0800 Subject: [PATCH 8/8] WIP: update tests. --- jrpc2_test.go | 36 +++++++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/jrpc2_test.go b/jrpc2_test.go index b6277ca..31c478d 100644 --- a/jrpc2_test.go +++ b/jrpc2_test.go @@ -1074,8 +1074,11 @@ func TestStrictFields(t *testing.T) { B int `json:"bravo"` other } + type result struct { + X string `json:"xray"` + } loc := server.NewLocal(handler.Map{ - "Test": handler.New(func(ctx context.Context, req *jrpc2.Request) error { + "Test": handler.New(func(ctx context.Context, req *jrpc2.Request) (interface{}, error) { var ps, qs params if err := req.UnmarshalParams(jrpc2.StrictFields(&ps)); err == nil { @@ -1088,16 +1091,43 @@ func TestStrictFields(t *testing.T) { t.Logf("Parameters OK: %+v", qs) } - return nil + return map[string]string{ + "xray": "ok", + "gamma": "not ok", + }, nil }), }, nil) defer loc.Close() ctx := context.Background() - loc.Client.Call(ctx, "Test", handler.Obj{ + req := handler.Obj{ "alpha": "foo", "bravo": 25, "charlie": true, // exercise embedding "delta": 31.5, // unknown field + } + t.Run("NonStrictResult", func(t *testing.T) { + rsp, err := loc.Client.Call(ctx, "Test", req) + if err != nil { + t.Fatalf("Call failed: %v", err) + } + var res result + if err := rsp.UnmarshalResult(&res); err != nil { + t.Errorf("UnmarshalResult: %v", err) + } + t.Logf("Result: %+v", res) + }) + + t.Run("StrictResult", func(t *testing.T) { + rsp, err := loc.Client.Call(ctx, "Test", req) + if err != nil { + t.Fatalf("Call failed: %v", err) + } + var res result + if err := rsp.UnmarshalResult(jrpc2.StrictFields(&res)); err == nil { + t.Errorf("UnmarshalResult: got %+v, want error", res) + } else { + t.Logf("UnmarshalResult: got expected error: %v", err) + } }) }