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

handler: add an option to disallow array-based parameters #114

Merged
merged 3 commits into from
Apr 1, 2024
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
21 changes: 15 additions & 6 deletions handler/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ type FuncInfo struct {
ReportsError bool // true if the function reports an error

strictFields bool // enforce strict field checking
allowArray bool // allow decoding from array format
posNames []string // positional field names

fn any // the original function value
Expand All @@ -118,6 +119,13 @@ type FuncInfo struct {
// for non-struct arguments.
func (fi *FuncInfo) SetStrict(strict bool) *FuncInfo { fi.strictFields = strict; return fi }

// AllowArray sets the flag on fi that determines whether the wrapper it
// generates allows struct arguments to be sent in array notation. If true, a
// parameter array is decoded into corresponding fields of the struct argument
// in declaration order; if false, array arguments report an error. The default
// value is currently true. This option has no effect for non-struct arguments.
func (fi *FuncInfo) AllowArray(ok bool) *FuncInfo { fi.allowArray = ok; return fi }

// Wrap adapts the function represented by fi to a jrpc2.Handler. The wrapped
// function can obtain the *jrpc2.Request value from its context argument using
// the jrpc2.InboundRequest helper.
Expand Down Expand Up @@ -249,10 +257,11 @@ func (fi *FuncInfo) Wrap() jrpc2.Handler {
// If fn does not have one of these forms, Check reports an error.
//
// If the type of X is a struct or a pointer to a struct, the generated wrapper
// accepts JSON parameters as either an object or an array. Array parameters
// are mapped to the fields of X in the order of field declaration, save that
// unexported fields are skipped. If a field has a `json:"-"` tag, it is also
// skipped. Anonymous fields are skipped unless they are tagged.
// accepts JSON parameters as either an object or an array. The caller may
// disable array support by calling AllowArray(false). When enabled, array
// parameters are mapped to the fields of X in the order of field declaration,
// save that unexported fields are skipped. If a field has a `json:"-"` tag, it
// is also skipped. Anonymous fields are skipped unless they are tagged.
//
// For other (non-struct) argument types, the accepted format is whatever the
// json.Unmarshal function can decode into the value. Note, however, that the
Expand All @@ -274,7 +283,7 @@ func Check(fn any) (*FuncInfo, error) {
return nil, errors.New("nil function")
}

info := &FuncInfo{Type: reflect.TypeOf(fn), fn: fn}
info := &FuncInfo{Type: reflect.TypeOf(fn), fn: fn, allowArray: true}
if info.Type.Kind() != reflect.Func {
return nil, errors.New("not a function")
}
Expand Down Expand Up @@ -365,7 +374,7 @@ func (s *strictStub) UnmarshalJSON(data []byte) error {
func (fi *FuncInfo) argWrapper() func(reflect.Value) any {
strict := fi.strictFields && fi.Argument != nil && !fi.Argument.Implements(strictType)
names := fi.posNames // capture so the wrapper does not pin fi
array := len(names) != 0
array := len(names) != 0 && fi.allowArray
switch {
case strict && array:
return func(v reflect.Value) any {
Expand Down
36 changes: 36 additions & 0 deletions handler/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,42 @@ func TestFuncInfo_SetStrict(t *testing.T) {
}
}

func TestFuncInfo_AllowArray(t *testing.T) {
type arg struct {
A, B string
}
fi, err := handler.Check(func(ctx context.Context, arg *arg) error {
if arg.A != "x" || arg.B != "y" {
return fmt.Errorf("a=%q, b=%q", arg.A, arg.B)
}
return nil
})
if err != nil {
t.Fatalf("Check failed: %v", err)
}

req := testutil.MustParseRequest(t, `{
"jsonrpc": "2.0",
"id": 101,
"method": "f",
"params": ["x", "y"]
}`)

t.Run("true", func(t *testing.T) {
fn := fi.AllowArray(true).Wrap()
if _, err := fn(context.Background(), req); err != nil {
t.Fatalf("Handler unexpectedly failed: %v", err)
}
})
t.Run("false", func(t *testing.T) {
fn := fi.AllowArray(false).Wrap()
rsp, err := fn(context.Background(), req)
if got := jrpc2.ErrorCode(err); got != jrpc2.InvalidParams {
t.Errorf("Handler returned (%+v, %v), want InvalidParams", rsp, err)
}
})
}

// Verify that the handling of pointer-typed arguments does not incorrectly
// introduce another pointer indirection.
func TestNew_pointerRegression(t *testing.T) {
Expand Down
10 changes: 6 additions & 4 deletions handler/positional.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,14 +95,16 @@ func structFieldNames(atype reflect.Type) (bool, []string) {
// // ...
// call := fi.Wrap()
//
// the resulting handler accepts a JSON array with with (exactly) the same
// number of elements as the positional parameters:
// the resulting handler by default accepts a JSON array with with (exactly)
// the same number of elements as the positional parameters:
//
// [17, 23]
//
// No arguments can be omitted in this format, but the caller can use a JSON
// "null" in place of any argument. The handler will also accept a parameter
// object like:
// "null" in place of any argument. The caller may also disable array support
// by setting AllowArray(false) on the resulting FuncInfo.
//
// The handler will also accept a parameter object like:
//
// {"first": 17, "second": 23}
//
Expand Down